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

Peter Chen
2023-11-14T08:38:58Z

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
>

--

Thanks,
Peter Chen