[RESEND PATCH 0/7] usb: host: xhci: some changes for xhci-plat.c

[RESEND PATCH 0/7] usb: host: xhci: some changes for xhci-plat.c

[Peter Chen]

Hi Mathias,

This series mainly includes some changes for xhci-plat.c. Some of the
patches were reviewed by you, some were sent during your holiday. To
easy your review and save, I re-send them together. The patches are
already rebased with the lastest usb/next, thanks.

Peter

Peter Chen (7):
  usb: host: xhci-plat: add platform data support
  usb: host: xhci-plat: add .suspend_quirk for struct xhci_plat_priv
  usb: host: xhci-plat: delete the unnecessary code
  usb: host: xhci-plat: add priv quirk for skip PHY initialization
  usb: host: xhci-plat: initialize device wakeup default enabled
  usb: host: xhci-plat: improve the comments for xhci_plat_suspend
  usb: host: xhci: avoid calling contineous two times for xhci_suspend

 drivers/usb/host/xhci-plat.c | 44 +++++++++++++++++++++++++++---------
 drivers/usb/host/xhci-plat.h |  1 +
 drivers/usb/host/xhci.c      |  7 ++++--
 drivers/usb/host/xhci.h      |  1 +
 4 files changed, 40 insertions(+), 13 deletions(-)

-- 
2.17.1

[RESEND PATCH 1/7] usb: host: xhci-plat: add platform data support

[Peter Chen]

Some xhci hosts (eg dwc3 and cdns3) do not use OF to create
platform device, they create xhci-plat platform device runtime.
And these platforms may also have quirks, and the quirks could
be supplied by their parent device through platform data.

Reviewed-by: Jun Li <jun.li@nxp.com>
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 3057cfc76d6a..c7f98edc5678 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -264,7 +264,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_reg_clk;
 
-	priv_match = of_device_get_match_data(&pdev->dev);
+	if (pdev->dev.of_node)
+		priv_match = of_device_get_match_data(&pdev->dev);
+	else
+		priv_match = dev_get_platdata(&pdev->dev);
+
 	if (priv_match) {
 		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
 
-- 
2.17.1

[RESEND PATCH 2/7] usb: host: xhci-plat: add .suspend_quirk for struct xhci_plat_priv

[Peter Chen]

Some platforms (eg cdns3) may have special sequences between
xhci_bus_suspend and xhci_suspend, add .suspend_quick for it.

Reviewed-by: Jun Li <jun.li@nxp.com>
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 19 +++++++++++++++++++
 drivers/usb/host/xhci-plat.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c7f98edc5678..c3ce4d762adf 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -54,6 +54,16 @@ static int xhci_priv_init_quirk(struct usb_hcd *hcd)
 	return priv->init_quirk(hcd);
 }
 
+static int xhci_priv_suspend_quirk(struct usb_hcd *hcd)
+{
+	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+
+	if (!priv->suspend_quirk)
+		return 0;
+
+	return priv->suspend_quirk(hcd);
+}
+
 static int xhci_priv_resume_quirk(struct usb_hcd *hcd)
 {
 	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
@@ -401,7 +411,11 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	int ret;
 
+	ret = xhci_priv_suspend_quirk(hcd);
+	if (ret)
+		return ret;
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
 	 * to do wakeup during suspend. Since xhci_plat_suspend is currently
@@ -438,6 +452,11 @@ static int __maybe_unused xhci_plat_runtime_suspend(struct device *dev)
 {
 	struct usb_hcd  *hcd = dev_get_drvdata(dev);
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	int ret;
+
+	ret = xhci_priv_suspend_quirk(hcd);
+	if (ret)
+		return ret;
 
 	return xhci_suspend(xhci, true);
 }
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index b49f6447bd3a..1fb149d1fbce 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -15,6 +15,7 @@ struct xhci_plat_priv {
 	unsigned long long quirks;
 	void (*plat_start)(struct usb_hcd *);
 	int (*init_quirk)(struct usb_hcd *);
+	int (*suspend_quirk)(struct usb_hcd *);
 	int (*resume_quirk)(struct usb_hcd *);
 };
 
-- 
2.17.1

[RESEND PATCH 3/7] usb: host: xhci-plat: delete the unnecessary code

[Peter Chen]

The if {} condition is duplicated with outer if {} condition.

Reviewed-by: Jun Li <jun.li@nxp.com>
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c3ce4d762adf..07ca000a0084 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -283,8 +283,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
 
 		/* Just copy data for now */
-		if (priv_match)
-			*priv = *priv_match;
+		*priv = *priv_match;
 	}
 
 	device_wakeup_enable(hcd->self.controller);
-- 
2.17.1

[RESEND PATCH 4/7] usb: host: xhci-plat: add priv quirk for skip PHY initialization

[Peter Chen]

Some DRD controllers (eg, dwc3 & cdns3) have PHY management at
their own driver to cover both device and host mode, so add one
priv quirk for such users to skip PHY management from HCD core.

Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 8 ++++++--
 drivers/usb/host/xhci.h      | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 07ca000a0084..14ff65a387e8 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -183,6 +183,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct usb_hcd		*hcd;
 	int			ret;
 	int			irq;
+	struct xhci_plat_priv	*priv = NULL;
+
 
 	if (usb_disabled())
 		return -ENODEV;
@@ -280,8 +282,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		priv_match = dev_get_platdata(&pdev->dev);
 
 	if (priv_match) {
-		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
-
+		priv = hcd_to_xhci_priv(hcd);
 		/* Just copy data for now */
 		*priv = *priv_match;
 	}
@@ -329,6 +330,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 
 	hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
 	xhci->shared_hcd->tpl_support = hcd->tpl_support;
+	if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
+		hcd->skip_phy_initialization = 1;
+
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto disable_usb_phy;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index ea1754f185a2..7658abdfca28 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1874,6 +1874,7 @@ struct xhci_hcd {
 #define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
 #define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
 #define XHCI_RENESAS_FW_QUIRK	BIT_ULL(36)
+#define XHCI_SKIP_PHY_INIT	BIT_ULL(37)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
-- 
2.17.1

[RESEND PATCH 5/7] usb: host: xhci-plat: initialize device wakeup default enabled

[Peter Chen]

It initializes the controller wakeup setting as default enabled
unless the user changes it, whether the controller responds
the wakeup event depends on roothub's wakeup setting since the
wakeup occurs at the bus not the controller itself.

With this change, the controller uses this driver could have
wakeup capability due to device_may_wakeup(dev) at xhci_plat_suspend
is default true; without this change, this value is always false.

Reviewed-by: Jun Li <jun.li@nxp.com>
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 14ff65a387e8..0ef28b2caea3 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -287,7 +287,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		*priv = *priv_match;
 	}
 
-	device_wakeup_enable(hcd->self.controller);
+	device_init_wakeup(hcd->self.controller, true);
 
 	xhci->main_hcd = hcd;
 	xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
-- 
2.17.1

[RESEND PATCH 6/7] usb: host: xhci-plat: improve the comments for xhci_plat_suspend

[Peter Chen]

To reflect the current code status.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 0ef28b2caea3..7e2eb0439c03 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -421,11 +421,7 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
 		return ret;
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
-	 * to do wakeup during suspend. Since xhci_plat_suspend is currently
-	 * only designed for system suspend, device_may_wakeup() is enough
-	 * to dertermine whether host is allowed to do wakeup. Need to
-	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
-	 * also applies to runtime suspend.
+	 * to do wakeup during suspend, the wakeup is enabled by default.
 	 */
 	return xhci_suspend(xhci, device_may_wakeup(dev));
 }
-- 
2.17.1

[RESEND PATCH 7/7] usb: host: xhci: avoid calling contineous two times for xhci_suspend

[Peter Chen]

If the xhci-plat.c is the platform driver, after the runtime pm is
enabled, the xhci_suspend is called if nothing is connected on
the port. When the system goes to suspend, it will call xhci_suspend again
if USB wakeup is enabled.

Since the runtime suspend wakeup setting is not always the same with
system suspend wakeup setting, eg, at runtime suspend, we always need
wakeup if the controller is in low power mode; but at suspend system,
we may not need wakeup. So, we move the judgement after changing
wakeup setting.

Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 3c41b14ecce7..cde71f3b9767 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -982,12 +982,15 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 			xhci->shared_hcd->state != HC_STATE_SUSPENDED)
 		return -EINVAL;
 
-	xhci_dbc_suspend(xhci);
-
 	/* Clear root port wake on bits if wakeup not allowed. */
 	if (!do_wakeup)
 		xhci_disable_port_wake_on_bits(xhci);
 
+	if (!HCD_HW_ACCESSIBLE(hcd))
+		return 0;
+
+	xhci_dbc_suspend(xhci);
+
 	/* Don't poll the roothubs on bus suspend. */
 	xhci_dbg(xhci, "%s: stopping port polling.\n", __func__);
 	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
-- 
2.17.1

Re: [RESEND PATCH 5/7] usb: host: xhci-plat: initialize device wakeup default enabled

[Peter Chen]

On 20-08-13 11:37:39, Peter Chen wrote:
> It initializes the controller wakeup setting as default enabled
> unless the user changes it, whether the controller responds
> the wakeup event depends on roothub's wakeup setting since the
> wakeup occurs at the bus not the controller itself.
> 
> With this change, the controller uses this driver could have
> wakeup capability due to device_may_wakeup(dev) at xhci_plat_suspend
> is default true; without this change, this value is always false.
> 
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/host/xhci-plat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 14ff65a387e8..0ef28b2caea3 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -287,7 +287,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  		*priv = *priv_match;
>  	}
>  
> -	device_wakeup_enable(hcd->self.controller);
> +	device_init_wakeup(hcd->self.controller, true);

Hi Alan,

We may can't set the wakeup enabled as default value since it will
affect the parent's wakeup setting. See function:
dpm_propagate_wakeup_to_parent at drivers/base/power/main.c.
The parent's wakeup_path is set as true during the system
system routine, then the power domain for the parent will not be
off (See genpd_finish_suspend) after system suspend, it does not
meet design expectation. The expectation is the device's power
domain is only on if the device as wakeup source.

Even with host-only use case, the thing is the same. If wakeup enabled
as default, the related power domain will be on during the system
suspend no matter the wakeup is really wanted.

Do you agree I set the wakeup default value as disabled?

-- 

Thanks,
Peter Chen

Re: [RESEND PATCH 5/7] usb: host: xhci-plat: initialize device wakeup default enabled

[stern]

On Fri, Aug 14, 2020 at 06:25:04AM +0000, Peter Chen wrote:
> Hi Alan,
> 
> We may can't set the wakeup enabled as default value since it will
> affect the parent's wakeup setting. See function:
> dpm_propagate_wakeup_to_parent at drivers/base/power/main.c.
> The parent's wakeup_path is set as true during the system
> system routine, then the power domain for the parent will not be
> off (See genpd_finish_suspend) after system suspend, it does not
> meet design expectation. The expectation is the device's power
> domain is only on if the device as wakeup source.
> 
> Even with host-only use case, the thing is the same. If wakeup enabled
> as default, the related power domain will be on during the system
> suspend no matter the wakeup is really wanted.
> 
> Do you agree I set the wakeup default value as disabled?

Yes, I guess so.  There doesn't seem to be any other way to handle this.

What we really should have is a setting which means "Leave wakeup turned 
off unless it is enabled for a child device".  Maybe something like that 
can be added in the future.

Alan Stern