Hello Peter, On Fri Aug 9, 2024 at 3:19 AM CEST, Peter Chen wrote: > On 24-07-26 20:17:48, Théo Lebrun wrote: > > Currently, system-wide suspend is broken on J7200 because of a > > controller reset. The TI wrapper does not get re-initialised at resume > > and the first register access from cdns core fails. > > > > We address that in a few ways: > > - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW. > > - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects > > a resume. > > - We add a xhci->lost_power flag. > > > > The previous revision had one big issue: we had to know if > > reset-on-resume was true, at probe-time. This is where the main > > difference with previous revisions is. We now pass the information from > > wrapper devices back up into XHCI. The xhci->lost_power flag gets its > > default value from the XHCI_RESET_ON_RESUME quirk. It however allows > > wrappers to signal *at resume* if they still expect a reset. > > > > That means wrappers that are unsure if they will reset should: > > - (1) set the quirk at probe and, > > - (2) potentially set xhci->lost_power to false at resume. > > Judge if controller is power lost has implemented at cdns_power_is_lost > Please check if you could use that. That function is being used! Its return value is passed as second argument to the resume() callback in struct cdns_role_driver. We set xhci->lost_power using that exact value. My cover letter explanation was slightly off, as it is not wrappers that set xhci->lost_power, but instead role drivers. Wrappers don't have any reason to touch the xhci struct directly, they are one layer above. Related: [PATCH v5 08/15] commit message looks like this: ------------------------------------------------------------------------ The cdns_role_driver->resume() callback takes a second boolean argument named `hibernated` in its implementations. This is mistaken; the only potential caller is: int cdns_resume(struct cdns *cdns) { /* ... */ if (cdns->roles[cdns->role]->resume) cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns)); return 0; } The argument can be true in cases outside of return from hibernation. Reflect the true meaning by renaming both arguments to `lost_power`. ------------------------------------------------------------------------ Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com