diff mbox series

[4/6] usb: cdns3: support power-off of controller when in host role

Message ID 20231113-j7200-usb-suspend-v1-4-ad1ee714835c@bootlin.com
State New
Headers
Series usb: cdns: fix suspend on J7200 by assuming reset on resume |

Commit Message

Théo Lebrun Nov. 13, 2023, 2:26 p.m. UTC 
  The controller is not being reconfigured at resume. Change resume to
redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.

Platform data comes from the parent driver (eg cdns3-ti).

The quirk should be passed if the platform driver knows that the
controller might be in reset state at resume. We do NOT reconfigure the
hardware without this quirk to avoid losing state if we did a suspend
without reset.

If the quirk is on, we notify the xHCI subsystem that:

1. We reset on resume. It will therefore redo the xHC init & trigger
   such message as "root hub lost power or was reset" in dmesg.

2. It should disable/enable clocks on suspend/resume. This does not
   matter on our platform as xhci-plat does not get access to any clock
   but it would be the right thing to do if we indeed had such clocks.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/core.h |  1 +
 drivers/usb/cdns3/host.c | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

Comments

Peter Chen Nov. 14, 2023, 8:38 a.m. UTC | #1 
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.

> 
> Platform data comes from the parent driver (eg cdns3-ti).
> 
> The quirk should be passed if the platform driver knows that the
> controller might be in reset state at resume. We do NOT reconfigure the
> hardware without this quirk to avoid losing state if we did a suspend
> without reset.
> 
> If the quirk is on, we notify the xHCI subsystem that:
> 
> 1. We reset on resume. It will therefore redo the xHC init & trigger
>    such message as "root hub lost power or was reset" in dmesg.
> 
> 2. It should disable/enable clocks on suspend/resume. This does not
>    matter on our platform as xhci-plat does not get access to any clock
>    but it would be the right thing to do if we indeed had such clocks.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/core.h |  1 +
>  drivers/usb/cdns3/host.c | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> index 81a9c9d6be08..7487067ba23f 100644
> --- a/drivers/usb/cdns3/core.h
> +++ b/drivers/usb/cdns3/core.h
> @@ -44,6 +44,7 @@ struct cdns3_platform_data {
>  			bool suspend, bool wakeup);
>  	unsigned long quirks;
>  #define CDNS3_DEFAULT_PM_RUNTIME_ALLOW	BIT(0)
> +#define CDNS3_RESET_ON_RESUME		BIT(1)
>  };
>  
>  /**
> diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
> index 6164fc4c96a4..a81019a7c8cc 100644
> --- a/drivers/usb/cdns3/host.c
> +++ b/drivers/usb/cdns3/host.c
> @@ -88,6 +88,9 @@ static int __cdns_host_init(struct cdns *cdns)
>  		goto err1;
>  	}
>  
> +	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.

>  	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
>  		cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
>  
> @@ -124,6 +127,18 @@ static void cdns_host_exit(struct cdns *cdns)
>  	cdns_drd_host_off(cdns);
>  }
>  
> +static int cdns_host_suspend(struct cdns *cdns, bool do_wakeup)
> +{
> +	if (!do_wakeup)
> +		cdns_drd_host_off(cdns);
> +	return 0;
> +}
> +
> +static int cdns_host_resume(struct cdns *cdns, bool hibernated)
> +{
> +	return cdns_drd_host_on(cdns);

This one will redo if controller's power is off, please consider both
on and power situation.

> +}
> +
>  int cdns_host_init(struct cdns *cdns)
>  {
>  	struct cdns_role_driver *rdrv;
> @@ -137,6 +152,11 @@ int cdns_host_init(struct cdns *cdns)
>  	rdrv->state	= CDNS_ROLE_STATE_INACTIVE;
>  	rdrv->name	= "host";
>  
> +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME) {
> +		rdrv->suspend = cdns_host_suspend;
> +		rdrv->resume = cdns_host_resume;
> +	}
> +
>  	cdns->roles[USB_ROLE_HOST] = rdrv;
>  
>  	return 0;
> 
> -- 
> 2.41.0
>
Théo Lebrun Nov. 14, 2023, 11:10 a.m. UTC | #2 
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.

> > +static int cdns_host_resume(struct cdns *cdns, bool hibernated)
> > +{
> > +	return cdns_drd_host_on(cdns);
>
> This one will redo if controller's power is off, please consider both
> on and power situation.

Clearly. Can see that at runtime.

Thanks for the review!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Peter Chen Nov. 17, 2023, 3:38 a.m. UTC | #3 
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);
	}
Théo Lebrun Nov. 17, 2023, 9:58 a.m. UTC | #4 
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
Peter Chen Nov. 20, 2023, 5:44 a.m. UTC | #5 
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.
diff mbox series

Patch

diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 81a9c9d6be08..7487067ba23f 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -44,6 +44,7 @@  struct cdns3_platform_data {
 			bool suspend, bool wakeup);
 	unsigned long quirks;
 #define CDNS3_DEFAULT_PM_RUNTIME_ALLOW	BIT(0)
+#define CDNS3_RESET_ON_RESUME		BIT(1)
 };
 
 /**
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 6164fc4c96a4..a81019a7c8cc 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -88,6 +88,9 @@  static int __cdns_host_init(struct cdns *cdns)
 		goto err1;
 	}
 
+	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
+		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
+
 	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
 		cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
 
@@ -124,6 +127,18 @@  static void cdns_host_exit(struct cdns *cdns)
 	cdns_drd_host_off(cdns);
 }
 
+static int cdns_host_suspend(struct cdns *cdns, bool do_wakeup)
+{
+	if (!do_wakeup)
+		cdns_drd_host_off(cdns);
+	return 0;
+}
+
+static int cdns_host_resume(struct cdns *cdns, bool hibernated)
+{
+	return cdns_drd_host_on(cdns);
+}
+
 int cdns_host_init(struct cdns *cdns)
 {
 	struct cdns_role_driver *rdrv;
@@ -137,6 +152,11 @@  int cdns_host_init(struct cdns *cdns)
 	rdrv->state	= CDNS_ROLE_STATE_INACTIVE;
 	rdrv->name	= "host";
 
+	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME) {
+		rdrv->suspend = cdns_host_suspend;
+		rdrv->resume = cdns_host_resume;
+	}
+
 	cdns->roles[USB_ROLE_HOST] = rdrv;
 
 	return 0;