On Tue, Sep 02, 2025 at 10:34:51AM GMT, Siddharth Vadapalli wrote: > On Mon, Sep 01, 2025 at 08:15:24PM +0530, Manivannan Sadhasivam wrote: > > On Mon, Sep 01, 2025 at 04:50:02PM GMT, Siddharth Vadapalli wrote: > > > On Mon, Sep 01, 2025 at 12:14:51PM +0530, Manivannan Sadhasivam wrote: > > > > On Mon, Sep 01, 2025 at 11:51:33AM GMT, Siddharth Vadapalli wrote: > > [...] > > > > > > > > > > > If I understand correctly, are you suggesting the following? > > > > > > > > > > j721e_pcie_probe() > > > > > pm_runtime_set_active() > > > > > pm_runtime_enable() > > > > > ret = j721e_pcie_ctrl_init(pcie); > > > > > /* > > > > > * PCIe Controller should be powered off here, but is there > > > > > * a way to ensure that it has been powered off? > > > > > */ > > > > > => Program the strap settings and return to > > > > > j721e_pcie_probe() > > > > > /* Power on the PCIe Controller now */ > > > > > ret = pm_runtime_get_sync(dev); > > > > > > > > This pm_runtime_get_sync() should be part of j721e_pcie_ctrl_init() where you > > > > do power off, program strap and power on. > > > > > > > > This should not be part of probe() as by that time, controller is already > > > > powered on. So pm_runtime_set_active() and pm_runtime_enable() should be enough > > > > to convey the state of the device to PM core. > > > > > > I have tried out the suggestion in the following manner: > > > > > > j721e_pcie_probe() > > > ... > > > pm_runtime_set_active(dev); > > > pm_runtime_enable(dev); > > > ret = j721e_pcie_ctrl_init(pcie); > > > ... within j721e_pcie_ctrl_init() > > > ret = pm_runtime_put_sync(dev); > > > /* Program Strap Settings */ > > > ret = pm_runtime_get_sync(dev); > > > ... > > > ... > > > exit probe > > > > > > Since a 'pm_runtime_get_sync()' hasn't yet been invoked prior to the > > > section where 'pm_runtime_put_sync()' is invoked, it leads to a ref-count > > > underflow error at runtime. Please let me know if I am missing > > > something. > > > > > > > Doh... At the start of probe(), device PM usage_count will be 0. So we cannot > > decrement it without incrementing it. > > > > Could you try below sequence? > > > > probe() > > ... > > pm_runtime_set_active() > > pm_runtime_enable() > > j721e_pcie_ctrl_init() > > ... > > pm_runtime_get() /* Just increment usage_count */ > > pm_runtime_put_sync() /* ask PM core to turn off */ > > /* program strap setting */ > > pm_runtime_get_sync() /* ask PM core to turn on */ > > pm_runtime_put_noidle() /* balance the usage_count without > > suspending the device */ > > ... > > The above sequence powers off the controller at the point in time that > the strap settings are programmed. 'pm_runtime_get_sync()' is powering > on the controller afterwards. However, the 'pm_runtime_put_noidle()' > at the end is causing the controller to be powered off again leading to > a crash. Removing 'pm_runtime_put_noidle()' results in a functional > sequence. > > Please consider the existing sequence prior to this patch: > > probe() > ... > pm_runtime_enable() > pm_runtime_get_sync() => usage_count is 1 > ... > exit probe > > With the suggested sequence above, we have: > > probe() > ... > pm_runtime_set_active() > pm_runtime_enable() > j721e_pcie_ctrl_init() > ... > pm_runtime_get() => usage_count is 1 > pm_runtime_put_sync() => usage_count is 0 > /* Controller is powered off now */ > /* Strap settings are programmed */ > pm_runtime_get_sync() => usage_count is 1 > /* Controller is powered on now */ > pm_runtime_put_noidle() => usage_count is 0 > /* Controller is powered off in a while */ > ... > /* Crash is observed aroung this point before probe finishes */ > > Please let me know if the fix is to drop 'pm_runtime_put_noidle()' > from the above sequence. > I thought put_noidle() will just reduce the refcount and not invoke the idle/suspend callbacks, but I seem to be wrong here. Anyway, I guess we have no option here other than to drop the pm_runtime_put_noidle() call. This will keep refcount as 1 and will prevent the parent (genpd) to not enter runtime suspend, but we have to live with it (this was also the previous beahvior as well). Btw, pm_runtime_set_active/enable change belongs to a separate patch. - Mani -- மணிவண்ணன் சதாசிவம்
