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
