From 9453b4cca6ab93eb26d58f78e209d67ffed6d440 Mon Sep 17 00:00:00 2001 From: gly11 Date: Wed, 27 May 2026 12:43:32 +0800 Subject: [PATCH] fix(controller): harden OHCI bringup defaults --- ASFWDriver/ASFWDriver.cpp | 41 ++++-- ASFWDriver/Controller/BringupOverrides.hpp | 12 +- .../Controller/ControllerCoreLifecycle.cpp | 129 ++++++++---------- ASFWDriver/Hardware/OHCIConstants.hpp | 12 +- ASFWDriver/Info.plist | 2 + 5 files changed, 105 insertions(+), 91 deletions(-) diff --git a/ASFWDriver/ASFWDriver.cpp b/ASFWDriver/ASFWDriver.cpp index 5d7e6df9..a4fe42c3 100644 --- a/ASFWDriver/ASFWDriver.cpp +++ b/ASFWDriver/ASFWDriver.cpp @@ -69,6 +69,27 @@ class ASFWDriverUserClient; namespace { constexpr uint64_t kAsyncWatchdogPeriodUsec = 1000; // 1 ms tick (hybrid: interrupt + timer backup) + +bool PropertyIsEnabled(const OSObject* property) { + if (property == nullptr) { + return false; + } + + if (const auto booleanProp = OSDynamicCast(OSBoolean, property)) { + return booleanProp == kOSBooleanTrue; + } + + if (const auto numberProp = OSDynamicCast(OSNumber, property)) { + return numberProp->unsigned32BitValue() != 0; + } + + if (const auto stringProp = OSDynamicCast(OSString, property)) { + return stringProp->isEqualTo("1") || stringProp->isEqualTo("true") || + stringProp->isEqualTo("TRUE"); + } + + return false; +} } // namespace bool ASFWDriver::init() { @@ -109,22 +130,20 @@ kern_return_t IMPL(ASFWDriver, Start) { ctx.stopping.store(false, std::memory_order_release); DriverWiring::EnsureDeps(this, ctx); bool traceProperty = false; + bool experimentalHostCycleMasterBringup = false; if (OSDictionary* serviceProperties = nullptr; CopyProperties(&serviceProperties) == kIOReturnSuccess && serviceProperties != nullptr) { - if (auto property = serviceProperties->getObject("ASFWTraceDMACoherency")) { - if (auto booleanProp = OSDynamicCast(OSBoolean, property)) { - traceProperty = (booleanProp == kOSBooleanTrue); - } else if (auto numberProp = OSDynamicCast(OSNumber, property)) { - traceProperty = numberProp->unsigned32BitValue() != 0; - } else if (auto stringProp = OSDynamicCast(OSString, property)) { - traceProperty = stringProp->isEqualTo("1") || stringProp->isEqualTo("true") || - stringProp->isEqualTo("TRUE"); - } - } + traceProperty = PropertyIsEnabled(serviceProperties->getObject("ASFWTraceDMACoherency")); + experimentalHostCycleMasterBringup = + PropertyIsEnabled(serviceProperties->getObject("ASFWExperimentalHostCycleMasterBringup")); serviceProperties->release(); } + ctx.config.experimentalHostCycleMasterBringup = experimentalHostCycleMasterBringup; ASFW_LOG(Controller, "ASFWDriver::Start(): ASFWTraceDMACoherency property=%{public}s", traceProperty ? "true" : "false"); + ASFW_LOG(Controller, + "ASFWDriver::Start(): ASFWExperimentalHostCycleMasterBringup property=%{public}s", + experimentalHostCycleMasterBringup ? "true" : "false"); if (auto statusKr = ctx.statusPublisher.Prepare(); statusKr != kIOReturnSuccess) { DriverWiring::CleanupStartFailure(ctx); return statusKr; @@ -246,7 +265,7 @@ kern_return_t IMPL(ASFWDriver, Start) { } return ASFW::Async::ResponseCode::NoResponse; }); - ASFW_LOG(Controller, "✅ FCPResponseRouter wired to PacketRouter (tCode 0x1)"); + ASFW_LOG(Controller, "FCPResponseRouter wired to PacketRouter (tCode 0x1)"); } } diff --git a/ASFWDriver/Controller/BringupOverrides.hpp b/ASFWDriver/Controller/BringupOverrides.hpp index d732fc7b..4605c99c 100644 --- a/ASFWDriver/Controller/BringupOverrides.hpp +++ b/ASFWDriver/Controller/BringupOverrides.hpp @@ -5,10 +5,16 @@ namespace ASFW::Driver { -// Host cycle-master bring-up configuration. Linux firewire_ohci and Apple -// IOFireWireController both make the local PHY contender-capable during init, -// while root delegation remains policy-controlled by BusManager. +// Host cycle-master bring-up configuration: +// - enable local contender / cycle-master eligibility (matches Linux/Apple default) +// - delegation controlled by experimentalHostCycleMasterBringup property +// +// Per Linux firewire_ohci: PHY contender bit is always set during init. +// Per Apple IOFireWireController: contender is set for most configurations. +// The host MUST be contender-capable for proper bus management (IRM election, +// cycle-start generation), especially in 2-node topologies with SBP-2 devices. inline void ApplyBringupOverrides(ControllerConfig& config, BusManager* busManager) { + // Always enable contender; this matches Linux/Apple behavior. config.allowCycleMasterEligibility = true; if (busManager != nullptr) { diff --git a/ASFWDriver/Controller/ControllerCoreLifecycle.cpp b/ASFWDriver/Controller/ControllerCoreLifecycle.cpp index 6fcabe8f..aef2dba5 100644 --- a/ASFWDriver/Controller/ControllerCoreLifecycle.cpp +++ b/ASFWDriver/Controller/ControllerCoreLifecycle.cpp @@ -334,6 +334,12 @@ kern_return_t ControllerCore::InitialiseHardware(IOService* provider) { // auto-allocation behavior if (isOHCI_1_1_OrLater) { hw.WriteAndFlush(Register32::kInitialChannelsAvailableHi, 0xFFFFFFFE); + hw.WriteAndFlush(Register32::kInitialChannelsAvailableLo, 0xFFFFFFFF); + // Initialize BandwidthAvailable to maximum isochronous bandwidth. + // Per Linux ohci_enable(): reg_write(ohci, OHCI1394_BandwidthAvailable, 4915) + // Value 4915 (0x1333) is about S1600 full-cycle bandwidth. Without this, the IRM + // reports zero available bandwidth and devices may consider bus management inactive. + hw.WriteAndFlush(Register32::kInitialBandwidthAvailable, 4915); } // Step 4: Clear noByteSwapData - enable byte-swapping for data phases per OHCI spec @@ -388,66 +394,22 @@ kern_return_t ControllerCore::InitialiseHardware(IOService* provider) { ASFW_LOG(Hardware, "PHY probe failed after retry; relying on firmware defaults"); } else { uint8_t reg1Value = phyId.value(); - ASFW_LOG_PHY("PHY probe OK (reg1=0x%02x)", reg1Value); - - // --- FIX START: Force Gap Count to 0x3F --- - // Problem: Some PHYs report the strapped value over the register interface - // but require a write to latch it into the active core after reset. - // Fix: Always write register 1 so the latch is triggered even if the - // desired value already appears to be programmed. - const uint8_t kTargetGap = ASFW::Driver::kPhyGapCountMask; - const uint8_t newReg1 = (reg1Value & 0xC0U) | kTargetGap; - - ASFW_LOG_PHY("Forcing PHY Gap Count write (Reg 1): 0x%02x -> 0x%02x", reg1Value, - newReg1); - - constexpr int kMaxPhyWriteAttempts = 3; - bool wroteOk = false; - for (int attempt = 0; attempt < kMaxPhyWriteAttempts; ++attempt) { - if (!hw.WritePhyRegister(1, newReg1)) { - ASFW_LOG_PHY("PHY write attempt %d failed (writePhyRegister returned false)", - attempt + 1); - // Short delay before retry - IOSleep(1); - continue; - } - - // Give PHY time to latch the value (some parts need an explicit delay) - IODelay(2000); - - // Read-back verification - auto verify = hw.ReadPhyRegister(1); - if (verify && ((*verify & ASFW::Driver::kPhyGapCountMask) == kTargetGap)) { - ASFW_LOG_PHY("✅ PHY Gap Count confirmed: 0x%02x -> 0x%02x (attempt %d)", - reg1Value, *verify, attempt + 1); - wroteOk = true; - break; - } + ASFW_LOG_PHY("PHY probe OK (reg1=0x%02x, gap_count=%u)", + reg1Value, reg1Value & ASFW::Driver::kPhyGapCountMask); - // Toggle LPS to try to force PHY latch, then small pause and retry - ASFW_LOG_PHY("PHY gap write verify failed on attempt %d (readback=0x%02x)", - attempt + 1, verify.value_or(0)); - hw.ClearHCControlBits(HCControlBits::kLPS); - IODelay(5); - hw.SetHCControlBits(HCControlBits::kLPS); - IOSleep(5); - } - - if (!wroteOk) { - ASFW_LOG(Hardware, - "Failed to reliably write PHY Register 1 (gap count) after %d attempts", - kMaxPhyWriteAttempts); - } - // --- FIX END --- + // Note: We do NOT force gap count during init, unlike previous code. + // Linux ohci_enable() uses the PHY's default gap count and only + // optimizes it later after topology discovery (see GapCountOptimizer). + // Forcing gap count = 63 at init could cause communication issues + // with devices that expect the standard default (5). // Step 4: Configure PHY register 4 (Link Active + Contender) // Use constants from IEEE1394.hpp // (kPhyReg4Address, kPhyLinkActive, kPhyContender) // - // CRITICAL FIX: Only set Contender bit if allowCycleMasterEligibility is true - // - This matches Apple's behavior (conditional PHY reg 4 setup) - // - Prevents two-contender bus topology issues with devices like Apogee Duet - // - Default config has allowCycleMasterEligibility=false (delegate mode) + // Per Linux firewire_ohci and Apple IOFireWireController: + // Always set Contender bit. ApplyBringupOverrides ensures + // allowCycleMasterEligibility=true (matching Linux/Apple defaults). const uint8_t phyReg4Bits = config_.allowCycleMasterEligibility @@ -468,14 +430,17 @@ kern_return_t ControllerCore::InitialiseHardware(IOService* provider) { ASFW_LOG(Hardware, "Failed to configure PHY register 4"); } - // Enable PHY accelerated arbitration (IEEE 1394a reg5 bit6) before linkEnable. + // Enable PHY accelerated arbitration + multi-speed packet concatenation + // (IEEE 1394a reg5 bit6 + bit5) before linkEnable. + // Per Linux configure_1394a_enhancements(): both bits are set together. if (phyConfigOk) { const bool accelEnabled = - hw.UpdatePhyRegister(kPhyReg5Address, 0, kPhyEnableAcceleration); + hw.UpdatePhyRegister(kPhyReg5Address, 0, + kPhyEnableAcceleration | kPhyEnableMulti); if (accelEnabled) { - ASFW_LOG_PHY("PHY reg5 configured: Enab_accel=1 (gap writes will stick)"); + ASFW_LOG_PHY("PHY reg5 configured: Enab_accel=1 Enab_multi=1"); } else { - ASFW_LOG(Hardware, "Failed to enable PHY accelerated arbitration (reg5 bit6)"); + ASFW_LOG(Hardware, "Failed to enable PHY 1394a enhancements (reg5)"); phyConfigOk = false; } } @@ -536,14 +501,23 @@ kern_return_t ControllerCore::InitialiseHardware(IOService* provider) { return configRomStatus; } - // Step 7: Set Physical Upper Bound (256MB CSR address range) - // TODO(ASFW-DMA): Confirm whether remote DMA still requires this register programming. - // Per Linux ohci_enable(): Don't pre-write NodeID; bus reset will assign it from Self-ID - // The kProvisionalNodeId value would be immediately overwritten anyway + // Step 7: Set Physical Upper Bound (OHCI section 5.5.5) + // Per Linux ohci_enable() (ohci.c:2346): + // reg_write(ohci, OHCI1394_PhyUpperBound, FW_MAX_PHYSICAL_RANGE >> 16); + // FW_MAX_PHYSICAL_RANGE = 1ULL << 32, >> 16 = 0x10000 (4GB physical DMA range) + // This MUST be set before linkEnable to ensure proper physical address routing. + // Without this, CSR high-address space (e.g. ConfigROM at 0xF0000800+) may + // return RCODE_ADDRESS_ERROR on some controllers. + hw.WriteAndFlush(Register32::kPhyUpperBound, 0x10000u); + ASFW_LOG(Hardware, "PhyUpperBound set to 0x10000 (4GB physical DMA range)"); + hw.SetLinkControlBits(ASFW::Driver::kDefaultLinkControl); ASFW_LOG(Hardware, - "LinkControl: rcvSelfID | rcvPhyPkt | cycleTimerEnable (cycleMaster deferred)"); + "LinkControl: rcvSelfID | rcvPhyPkt | cycleTimerEnable | cycleMaster"); hw.WriteAndFlush(Register32::kAsReqFilterHiSet, ASFW::Driver::kAsReqAcceptAllMask); + hw.WriteAndFlush(Register32::kAsReqFilterLoSet, 0xFFFFFFFF); + ASFW_LOG(Hardware, "AsReqFilter: Hi=0x%08x Lo=0xFFFFFFFF (accept all async requests)", + ASFW::Driver::kAsReqAcceptAllMask); // Build full 32-bit value explicitly per OHCI spec: // [31:24]=reserved(0), [23:16]=cycleLimit, [15:8]=maxPhys, [7:4]=maxResp, [3:0]=maxReq @@ -553,10 +527,26 @@ kern_return_t ControllerCore::InitialiseHardware(IOService* provider) { hw.WriteAndFlush(Register32::kATRetries, atRetriesVal); // Force readback to flush write pipeline const uint32_t atRetriesReadback = hw.Read(Register32::kATRetries); - ASFW_LOG(Hardware, "ATRetries configured: maxReq=3 maxResp=3 maxPhys=3 cycleLimit=200"); ASFW_LOG(Hardware, "ATRetries write/readback: 0x%08x / 0x%08x", atRetriesVal, atRetriesReadback); + // FairnessControl (0x0DC): Probe for priority budget support, then clear. + // Per Linux ohci_enable() (firewire/ohci.c): + // reg_write(ohci, OHCI1394_FairnessControl, 0x3f); + // if (reg_read(ohci, OHCI1394_FairnessControl) == 0x3f) + // reg_write(ohci, OHCI1394_FairnessControl, 0); + // Some controllers implement priority arbitration; clearing the register + // ensures standard fairness (equal access for all nodes on the bus). + hw.WriteAndFlush(Register32::kFairnessControl, 0x3F); + const uint32_t fcReadback = hw.Read(Register32::kFairnessControl); + if (fcReadback == 0x3F) { + hw.WriteAndFlush(Register32::kFairnessControl, 0); + ASFW_LOG(Hardware, "FairnessControl: priority-budget capable, cleared to 0"); + } else { + ASFW_LOG(Hardware, "FairnessControl: readback=0x%08x (no priority-budget support)", + fcReadback); + } + // Bus timing state: mark cycle timer as inactive during init // Linux: ohci->bus_time_running = false; // Ensures init path doesn't assume active isochronous timing @@ -637,16 +627,7 @@ kern_return_t ControllerCore::EnableInterruptsAndStartBus() { ASFW_LOG(Hardware, "Setting linkEnable + BIBimageValid atomically - will trigger auto bus reset"); hw.SetHCControlBits(HCControlBits::kLinkEnable | HCControlBits::kBibImageValid); - - if (phyProgramSupported_ && phyConfigOk_) { - ASFW_LOG(Hardware, "Forcing bus reset via PHY to guarantee Config ROM shadow activation"); - const bool forced = hw.InitiateBusReset(false); - if (!forced) { - ASFW_LOG(Hardware, "WARNING: Forced bus reset failed; will rely on auto reset"); - } - } else { - ASFW_LOG(Hardware, "Skipping forced reset; relying on auto reset from linkEnable"); - } + ASFW_LOG(Hardware, "Relying on linkEnable auto reset for Config ROM shadow activation"); if (deps_.asyncController) { const kern_return_t armStatus = deps_.asyncController->ArmARContextsOnly(); diff --git a/ASFWDriver/Hardware/OHCIConstants.hpp b/ASFWDriver/Hardware/OHCIConstants.hpp index 0ff70dbe..fc8ff4f3 100644 --- a/ASFWDriver/Hardware/OHCIConstants.hpp +++ b/ASFWDriver/Hardware/OHCIConstants.hpp @@ -14,16 +14,22 @@ namespace ASFW::Driver { constexpr uint32_t kAsReqAcceptAllMask = 0x80000000u; // Default link control configuration used during controller initialization +// Per Linux ohci_enable() (ohci.c:2317-2318): cycleMaster is set at init time, +// not deferred. For simple 2-node topologies the host is root and must immediately +// act as cycle master to generate cycle-start packets. constexpr uint32_t kDefaultLinkControl = LinkControlBits::kRcvSelfID | LinkControlBits::kRcvPhyPkt | - LinkControlBits::kCycleTimerEnable; + LinkControlBits::kCycleTimerEnable | + LinkControlBits::kCycleMaster; // Posted write priming bits (OHCI HCControl - enable posted writes and LPS) constexpr uint32_t kPostedWritePrimingBits = HCControlBits::kPostedWriteEnable | HCControlBits::kLPS; -// Default ATRetries value (cycleLimit=200 maxPhys=3 maxResp=3 maxReq=3) -constexpr uint32_t kDefaultATRetries = (3u << 0) | (3u << 4) | (3u << 8) | (200u << 16); +// Default ATRetries value +// Per Linux firewire_ohci ohci_enable(): maxReq=15, maxResp=2, maxPhys=8, cycleLimit=200 +// Higher maxReq/maxPhys reduce transaction failures on slow or busy devices. +constexpr uint32_t kDefaultATRetries = (15u << 0) | (2u << 4) | (8u << 8) | (200u << 16); // Node capabilities advertised in our local Config ROM. Keep the baseline // conservative and only set cPhyEnhance when the PHY/Link 1394a enhancement diff --git a/ASFWDriver/Info.plist b/ASFWDriver/Info.plist index c02a2368..2e97a059 100644 --- a/ASFWDriver/Info.plist +++ b/ASFWDriver/Info.plist @@ -42,6 +42,8 @@ ASFWLogStatistics + ASFWExperimentalHostCycleMasterBringup + CFBundleIdentifier $(PRODUCT_BUNDLE_IDENTIFIER) IOClass