On 23-11-14 12:10:18, Th?o Lebrun wrote:
> Hello,
>
> On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > On 23-11-13 15:26:59, Th?o Lebrun wrote:
> > > The controller is not being reconfigured at resume. Change resume to
> > > redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.
> >
> > Current logic has power off judgement, see cdns3_controller_resume for
> > detail.
>
> Indeed! Thanks for the pointer. I had not noticed that, those patches
> come from an older kernel which didn't have it. That'll make for less
> changes; patches 4 & 5 can go away.
>
> > > + if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > + cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > +
> >
> > If you set this flag, how could you support the USB remote wakeup
> > request? In that case, the USB bus does not expect re-enumeration.
>
> We didn't support remote USB wakeup. Only S2R mattered in our case and
> USB remote wakeup wasn't a possibility.
Without this patch, will below be hit for your platform:
/* re-initialize the HC on Restore Error, or Host Controller Error */
if (temp & (STS_SRE | STS_HCE)) {
reinit_xhc = true;
if (!xhci->broken_suspend)
xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
}
--
Thanks,
Peter Chen
Hello,
On Fri Nov 17, 2023 at 4:38 AM CET, Peter Chen wrote:
> On 23-11-14 12:10:18, Théo Lebrun wrote:
> > Hello,
> >
> > On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > > > + if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > > + cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > > +
> > >
> > > If you set this flag, how could you support the USB remote wakeup
> > > request? In that case, the USB bus does not expect re-enumeration.
> >
> > We didn't support remote USB wakeup. Only S2R mattered in our case and
> > USB remote wakeup wasn't a possibility.
>
> Without this patch, will below be hit for your platform:
>
> /* re-initialize the HC on Restore Error, or Host Controller Error */
> if (temp & (STS_SRE | STS_HCE)) {
> reinit_xhc = true;
> if (!xhci->broken_suspend)
> xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> }
Yes it hits. The warning as well. How big of an issue is that?
My understanding is that this is the expected behavior with reset on
resume if we don't explicitely pass the flag XHCI_RESET_ON_RESUME. I
don't think we should be having the broken_suspend bit set as its
mentioning some specific quirk on AMD hardware.
Is the only expected difference inbetween having CDNS3_RESET_ON_RESUME &
not having it is resume time? For reference, the status read is 0x411
ie STS_HALT | STS_PCD | STS_SRE. xhc_state is zero.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 23-11-17 10:58:12, Th?o Lebrun wrote:
> Hello,
>
> On Fri Nov 17, 2023 at 4:38 AM CET, Peter Chen wrote:
> > On 23-11-14 12:10:18, Th?o Lebrun wrote:
> > > Hello,
> > >
> > > On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > > > > + if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > > > + cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > > > +
> > > >
> > > > If you set this flag, how could you support the USB remote wakeup
> > > > request? In that case, the USB bus does not expect re-enumeration.
> > >
> > > We didn't support remote USB wakeup. Only S2R mattered in our case and
> > > USB remote wakeup wasn't a possibility.
> >
> > Without this patch, will below be hit for your platform:
> >
> > /* re-initialize the HC on Restore Error, or Host Controller Error */
> > if (temp & (STS_SRE | STS_HCE)) {
> > reinit_xhc = true;
> > if (!xhci->broken_suspend)
> > xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> > }
>
> Yes it hits. The warning as well. How big of an issue is that?
>
> My understanding is that this is the expected behavior with reset on
> resume if we don't explicitely pass the flag XHCI_RESET_ON_RESUME. I
> don't think we should be having the broken_suspend bit set as its
> mentioning some specific quirk on AMD hardware.
>
> Is the only expected difference inbetween having CDNS3_RESET_ON_RESUME &
> not having it is resume time? For reference, the status read is 0x411
> ie STS_HALT | STS_PCD | STS_SRE. xhc_state is zero.
>
Yes. I know some xHCI controllers have powered off during suspend will
hit it, just would like to confirm if it is the same with yours. If you
don't want remote wakeup, it is most probably the same with quirks you set.
--
Thanks,
Peter Chen