[PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

Currently init_quirk callbacks for xhci platform drivers are called
xhci_plat_setup() function which is called after chip reset completes.
It happens in the middle of the usb_add_hcd() function.

But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
currently does nothing as prior xhci_plat_setup() it is not set.

Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
which really needs to be set prior calling usb_add_hcd() as this function
at its beginning skips PHY init if this member is set.

This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
XHCI_SKIP_PHY_INIT is set.

Fixes: f768e718911e0 ("usb: host: xhci-plat: add priv quirk for skip PHY initialization")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/usb/host/xhci-plat.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 4d34f6005381..58704c5b002b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 /* called during probe() after chip reset completes */
 static int xhci_plat_setup(struct usb_hcd *hcd)
 {
-	int ret;
-
-
-	ret = xhci_priv_init_quirk(hcd);
-	if (ret)
-		return ret;
-
 	return xhci_gen_setup(hcd, xhci_plat_quirks);
 }
 
@@ -330,6 +323,13 @@ 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) {
+		ret = xhci_priv_init_quirk(hcd);
+		if (ret)
+			goto disable_usb_phy;
+	}
+
 	if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
 		hcd->skip_phy_initialization = 1;
 
-- 
2.20.1

Re: [PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Peter Chen]

On 20-12-21 16:09:03, Pali Rohár wrote:
> Currently init_quirk callbacks for xhci platform drivers are called
> xhci_plat_setup() function which is called after chip reset completes.
> It happens in the middle of the usb_add_hcd() function.
> 
> But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> currently does nothing as prior xhci_plat_setup() it is not set.
> 
> Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> which really needs to be set prior calling usb_add_hcd() as this function
> at its beginning skips PHY init if this member is set.
> 
> This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> XHCI_SKIP_PHY_INIT is set.
> 
> Fixes: f768e718911e0 ("usb: host: xhci-plat: add priv quirk for skip PHY initialization")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/usb/host/xhci-plat.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 4d34f6005381..58704c5b002b 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>  /* called during probe() after chip reset completes */
>  static int xhci_plat_setup(struct usb_hcd *hcd)
>  {
> -	int ret;
> -
> -
> -	ret = xhci_priv_init_quirk(hcd);
> -	if (ret)
> -		return ret;
> -
>  	return xhci_gen_setup(hcd, xhci_plat_quirks);
>  }
>  
> @@ -330,6 +323,13 @@ 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) {
> +		ret = xhci_priv_init_quirk(hcd);
> +		if (ret)
> +			goto disable_usb_phy;
> +	}
> +
>  	if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
>  		hcd->skip_phy_initialization = 1;
>  

Hi Pali,

What's problem you have met? In structure xhci_plat_priv, the quirks are
defined at .quirks entry which is got at below code. .init_quirk is the
routine if special initializations are needed.

	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) {
		priv = hcd_to_xhci_priv(hcd);
		/* Just copy data for now */
		*priv = *priv_match;
	}
-- 

Thanks,
Peter Chen

Re: [PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

On Tuesday 22 December 2020 02:14:45 Peter Chen wrote:
> On 20-12-21 16:09:03, Pali Rohár wrote:
> > Currently init_quirk callbacks for xhci platform drivers are called
> > xhci_plat_setup() function which is called after chip reset completes.
> > It happens in the middle of the usb_add_hcd() function.
> > 
> > But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> > prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> > currently does nothing as prior xhci_plat_setup() it is not set.
> > 
> > Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> > which really needs to be set prior calling usb_add_hcd() as this function
> > at its beginning skips PHY init if this member is set.
> > 
> > This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> > init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> > XHCI_SKIP_PHY_INIT is set.
> > 
> > Fixes: f768e718911e0 ("usb: host: xhci-plat: add priv quirk for skip PHY initialization")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  drivers/usb/host/xhci-plat.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 4d34f6005381..58704c5b002b 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> >  /* called during probe() after chip reset completes */
> >  static int xhci_plat_setup(struct usb_hcd *hcd)
> >  {
> > -	int ret;
> > -
> > -
> > -	ret = xhci_priv_init_quirk(hcd);
> > -	if (ret)
> > -		return ret;
> > -
> >  	return xhci_gen_setup(hcd, xhci_plat_quirks);
> >  }
> >  
> > @@ -330,6 +323,13 @@ 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) {
> > +		ret = xhci_priv_init_quirk(hcd);
> > +		if (ret)
> > +			goto disable_usb_phy;
> > +	}
> > +
> >  	if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
> >  		hcd->skip_phy_initialization = 1;
> >  
> 
> Hi Pali,
> 
> What's problem you have met? In structure xhci_plat_priv, the quirks are
> defined at .quirks entry which is got at below code. .init_quirk is the
> routine if special initializations are needed.

Hello!

I'm talking about .init_quirk. And if XHCI_SKIP_PHY_INIT quirk is set in
this function then has no effect.

I'm working currently on patch for xhci mvebu which conditionally enable
or disable XHCI_SKIP_PHY_INIT quirk (it is going to fix existing
regression since v5.1 kernel) and without this patch XHCI_SKIP_PHY_INIT
quirk from the init_quirk does not work.

> 	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) {
> 		priv = hcd_to_xhci_priv(hcd);
> 		/* Just copy data for now */
> 		*priv = *priv_match;
> 	}
> -- 
> 
> Thanks,
> Peter Chen

Re: [PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

On Tuesday 22 December 2020 10:23:27 Pali Rohár wrote:
> On Tuesday 22 December 2020 02:14:45 Peter Chen wrote:
> > On 20-12-21 16:09:03, Pali Rohár wrote:
> > > Currently init_quirk callbacks for xhci platform drivers are called
> > > xhci_plat_setup() function which is called after chip reset completes.
> > > It happens in the middle of the usb_add_hcd() function.
> > > 
> > > But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> > > prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> > > currently does nothing as prior xhci_plat_setup() it is not set.
> > > 
> > > Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> > > which really needs to be set prior calling usb_add_hcd() as this function
> > > at its beginning skips PHY init if this member is set.
> > > 
> > > This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> > > init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> > > XHCI_SKIP_PHY_INIT is set.
> > > 
> > > Fixes: f768e718911e0 ("usb: host: xhci-plat: add priv quirk for skip PHY initialization")
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >  drivers/usb/host/xhci-plat.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > index 4d34f6005381..58704c5b002b 100644
> > > --- a/drivers/usb/host/xhci-plat.c
> > > +++ b/drivers/usb/host/xhci-plat.c
> > > @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> > >  /* called during probe() after chip reset completes */
> > >  static int xhci_plat_setup(struct usb_hcd *hcd)
> > >  {
> > > -	int ret;
> > > -
> > > -
> > > -	ret = xhci_priv_init_quirk(hcd);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	return xhci_gen_setup(hcd, xhci_plat_quirks);
> > >  }
> > >  
> > > @@ -330,6 +323,13 @@ 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) {
> > > +		ret = xhci_priv_init_quirk(hcd);
> > > +		if (ret)
> > > +			goto disable_usb_phy;
> > > +	}
> > > +
> > >  	if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
> > >  		hcd->skip_phy_initialization = 1;
> > >  
> > 
> > Hi Pali,
> > 
> > What's problem you have met? In structure xhci_plat_priv, the quirks are
> > defined at .quirks entry which is got at below code. .init_quirk is the
> > routine if special initializations are needed.
> 
> Hello!
> 
> I'm talking about .init_quirk. And if XHCI_SKIP_PHY_INIT quirk is set in
> this function then has no effect.

Ok, this patch is not enough, I will send V2.

> I'm working currently on patch for xhci mvebu which conditionally enable
> or disable XHCI_SKIP_PHY_INIT quirk (it is going to fix existing
> regression since v5.1 kernel) and without this patch XHCI_SKIP_PHY_INIT
> quirk from the init_quirk does not work.

And now I have tested V2 with my mvebu regression fix. I will send it to
mailing list for review.

> > 	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) {
> > 		priv = hcd_to_xhci_priv(hcd);
> > 		/* Just copy data for now */
> > 		*priv = *priv_match;
> > 	}
> > -- 
> > 
> > Thanks,
> > Peter Chen

Re: [PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Peter Chen]

On 20-12-22 14:30:51, Pali Rohár wrote:
> On Tuesday 22 December 2020 10:23:27 Pali Rohár wrote:
> > On Tuesday 22 December 2020 02:14:45 Peter Chen wrote:
> > > On 20-12-21 16:09:03, Pali Rohár wrote:
> > > > Currently init_quirk callbacks for xhci platform drivers are called
> > > > xhci_plat_setup() function which is called after chip reset completes.
> > > > It happens in the middle of the usb_add_hcd() function.
> > > > 
> > > > But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> > > > prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> > > > currently does nothing as prior xhci_plat_setup() it is not set.
> > > > 
> > > > Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> > > > which really needs to be set prior calling usb_add_hcd() as this function
> > > > at its beginning skips PHY init if this member is set.
> > > > 
> > > > This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> > > > init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> > > > XHCI_SKIP_PHY_INIT is set.
> > > > 
> > > > Fixes: f768e718911e0 ("usb: host: xhci-plat: add priv quirk for skip PHY initialization")
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>

Hi Pali,

I know your case, you need to choose XHCI_SKIP_PHY_INIT quirk
conditionally, but you may can't add Fixes tag at your patch
since your issue was existed before my patch.

Peter


> > > > ---
> > > >  drivers/usb/host/xhci-plat.c | 14 +++++++-------
> > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > > index 4d34f6005381..58704c5b002b 100644
> > > > --- a/drivers/usb/host/xhci-plat.c
> > > > +++ b/drivers/usb/host/xhci-plat.c
> > > > @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> > > >  /* called during probe() after chip reset completes */
> > > >  static int xhci_plat_setup(struct usb_hcd *hcd)
> > > >  {
> > > > -	int ret;
> > > > -
> > > > -
> > > > -	ret = xhci_priv_init_quirk(hcd);
> > > > -	if (ret)
> > > > -		return ret;
> > > > -
> > > >  	return xhci_gen_setup(hcd, xhci_plat_quirks);
> > > >  }
> > > >  
> > > > @@ -330,6 +323,13 @@ 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) {
> > > > +		ret = xhci_priv_init_quirk(hcd);
> > > > +		if (ret)
> > > > +			goto disable_usb_phy;
> > > > +	}
> > > > +
> > > >  	if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
> > > >  		hcd->skip_phy_initialization = 1;
> > > >  
> > > 
> > > Hi Pali,
> > > 
> > > What's problem you have met? In structure xhci_plat_priv, the quirks are
> > > defined at .quirks entry which is got at below code. .init_quirk is the
> > > routine if special initializations are needed.
> > 
> > Hello!
> > 
> > I'm talking about .init_quirk. And if XHCI_SKIP_PHY_INIT quirk is set in
> > this function then has no effect.
> 
> Ok, this patch is not enough, I will send V2.
> 
> > I'm working currently on patch for xhci mvebu which conditionally enable
> > or disable XHCI_SKIP_PHY_INIT quirk (it is going to fix existing
> > regression since v5.1 kernel) and without this patch XHCI_SKIP_PHY_INIT
> > quirk from the init_quirk does not work.
> 
> And now I have tested V2 with my mvebu regression fix. I will send it to
> mailing list for review.
> 
> > > 	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) {
> > > 		priv = hcd_to_xhci_priv(hcd);
> > > 		/* Just copy data for now */
> > > 		*priv = *priv_match;
> > > 	}
> > > -- 
> > > 
> > > Thanks,
> > > Peter Chen

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

On Wednesday 23 December 2020 01:02:43 Peter Chen wrote:
> On 20-12-22 14:30:51, Pali Rohár wrote:
> > On Tuesday 22 December 2020 10:23:27 Pali Rohár wrote:
> > > On Tuesday 22 December 2020 02:14:45 Peter Chen wrote:
> > > > On 20-12-21 16:09:03, Pali Rohár wrote:
> > > > > Currently init_quirk callbacks for xhci platform drivers are called
> > > > > xhci_plat_setup() function which is called after chip reset completes.
> > > > > It happens in the middle of the usb_add_hcd() function.
> > > > > 
> > > > > But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> > > > > prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> > > > > currently does nothing as prior xhci_plat_setup() it is not set.
> > > > > 
> > > > > Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> > > > > which really needs to be set prior calling usb_add_hcd() as this function
> > > > > at its beginning skips PHY init if this member is set.
> > > > > 
> > > > > This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> > > > > init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> > > > > XHCI_SKIP_PHY_INIT is set.
> > > > > 
> > > > > Fixes: f768e718911e0 ("usb: host: xhci-plat: add priv quirk for skip PHY initialization")
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> Hi Pali,
> 
> I know your case, you need to choose XHCI_SKIP_PHY_INIT quirk
> conditionally, but you may can't add Fixes tag at your patch
> since your issue was existed before my patch.

Ok, no problem, in V2 I will not add it.

> Peter
> 
> 
> > > > > ---
> > > > >  drivers/usb/host/xhci-plat.c | 14 +++++++-------
> > > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > > > index 4d34f6005381..58704c5b002b 100644
> > > > > --- a/drivers/usb/host/xhci-plat.c
> > > > > +++ b/drivers/usb/host/xhci-plat.c
> > > > > @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> > > > >  /* called during probe() after chip reset completes */
> > > > >  static int xhci_plat_setup(struct usb_hcd *hcd)
> > > > >  {
> > > > > -	int ret;
> > > > > -
> > > > > -
> > > > > -	ret = xhci_priv_init_quirk(hcd);
> > > > > -	if (ret)
> > > > > -		return ret;
> > > > > -
> > > > >  	return xhci_gen_setup(hcd, xhci_plat_quirks);
> > > > >  }
> > > > >  
> > > > > @@ -330,6 +323,13 @@ 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) {
> > > > > +		ret = xhci_priv_init_quirk(hcd);
> > > > > +		if (ret)
> > > > > +			goto disable_usb_phy;
> > > > > +	}
> > > > > +
> > > > >  	if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
> > > > >  		hcd->skip_phy_initialization = 1;
> > > > >  
> > > > 
> > > > Hi Pali,
> > > > 
> > > > What's problem you have met? In structure xhci_plat_priv, the quirks are
> > > > defined at .quirks entry which is got at below code. .init_quirk is the
> > > > routine if special initializations are needed.
> > > 
> > > Hello!
> > > 
> > > I'm talking about .init_quirk. And if XHCI_SKIP_PHY_INIT quirk is set in
> > > this function then has no effect.
> > 
> > Ok, this patch is not enough, I will send V2.
> > 
> > > I'm working currently on patch for xhci mvebu which conditionally enable
> > > or disable XHCI_SKIP_PHY_INIT quirk (it is going to fix existing
> > > regression since v5.1 kernel) and without this patch XHCI_SKIP_PHY_INIT
> > > quirk from the init_quirk does not work.
> > 
> > And now I have tested V2 with my mvebu regression fix. I will send it to
> > mailing list for review.
> > 
> > > > 	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) {
> > > > 		priv = hcd_to_xhci_priv(hcd);
> > > > 		/* Just copy data for now */
> > > > 		*priv = *priv_match;
> > > > 	}
> > > > -- 
> > > > 
> > > > Thanks,
> > > > Peter Chen
> 
> -- 
> 
> Thanks,
> Peter Chen

[PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

Currently init_quirk callbacks for xhci platform drivers are called
xhci_plat_setup() function which is called after chip reset completes.
It happens in the middle of the usb_add_hcd() function.

But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
currently does nothing as prior xhci_plat_setup() it is not set.

Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
which really needs to be set prior calling usb_add_hcd() as this function
at its beginning skips PHY init if this member is set.

This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
XHCI_SKIP_PHY_INIT is set. Also checking if either xhci->quirks or
priv->quirks contains this XHCI_SKIP_PHY_INIT quirk.

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* Check also xhci->quirks as xhci_priv_init_quirk() callbacks are setting xhci->quirks
* Tested with "usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720" patch
* Removed Fixes: line
---
 drivers/usb/host/xhci-plat.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 4d34f6005381..0eab7cb5a767 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 /* called during probe() after chip reset completes */
 static int xhci_plat_setup(struct usb_hcd *hcd)
 {
-	int ret;
-
-
-	ret = xhci_priv_init_quirk(hcd);
-	if (ret)
-		return ret;
-
 	return xhci_gen_setup(hcd, xhci_plat_quirks);
 }
 
@@ -330,7 +323,14 @@ 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))
+
+	if (priv) {
+		ret = xhci_priv_init_quirk(hcd);
+		if (ret)
+			goto disable_usb_phy;
+	}
+
+	if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks & XHCI_SKIP_PHY_INIT)))
 		hcd->skip_phy_initialization = 1;
 
 	if (priv && (priv->quirks & XHCI_SG_TRB_CACHE_SIZE_QUIRK))
-- 
2.20.1

Re: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Peter Chen]

On 20-12-23 17:18:47, Pali Rohár wrote:
> Currently init_quirk callbacks for xhci platform drivers are called
> xhci_plat_setup() function which is called after chip reset completes.
> It happens in the middle of the usb_add_hcd() function.
> 
> But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> currently does nothing as prior xhci_plat_setup() it is not set.
> 
> Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> which really needs to be set prior calling usb_add_hcd() as this function
> at its beginning skips PHY init if this member is set.
> 
> This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> XHCI_SKIP_PHY_INIT is set. Also checking if either xhci->quirks or
> priv->quirks contains this XHCI_SKIP_PHY_INIT quirk.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v2:
> * Check also xhci->quirks as xhci_priv_init_quirk() callbacks are setting xhci->quirks
> * Tested with "usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720" patch
> * Removed Fixes: line
> ---
>  drivers/usb/host/xhci-plat.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 4d34f6005381..0eab7cb5a767 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>  /* called during probe() after chip reset completes */
>  static int xhci_plat_setup(struct usb_hcd *hcd)
>  {
> -	int ret;
> -
> -
> -	ret = xhci_priv_init_quirk(hcd);
> -	if (ret)
> -		return ret;
> -
>  	return xhci_gen_setup(hcd, xhci_plat_quirks);
>  }
>  
> @@ -330,7 +323,14 @@ 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))
> +
> +	if (priv) {
> +		ret = xhci_priv_init_quirk(hcd);
> +		if (ret)
> +			goto disable_usb_phy;
> +	}
> +
> +	if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks & XHCI_SKIP_PHY_INIT)))
>  		hcd->skip_phy_initialization = 1;

I am not sure if others agree with you move the position of
xhci_priv_init_quirk, Let's see Mathias opinion.

-- 

Thanks,
Peter Chen

Re: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

On Thursday 24 December 2020 05:59:05 Peter Chen wrote:
> On 20-12-23 17:18:47, Pali Rohár wrote:
> > Currently init_quirk callbacks for xhci platform drivers are called
> > xhci_plat_setup() function which is called after chip reset completes.
> > It happens in the middle of the usb_add_hcd() function.
> > 
> > But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> > prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> > currently does nothing as prior xhci_plat_setup() it is not set.
> > 
> > Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> > which really needs to be set prior calling usb_add_hcd() as this function
> > at its beginning skips PHY init if this member is set.
> > 
> > This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> > init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> > XHCI_SKIP_PHY_INIT is set. Also checking if either xhci->quirks or
> > priv->quirks contains this XHCI_SKIP_PHY_INIT quirk.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > ---
> > Changes in v2:
> > * Check also xhci->quirks as xhci_priv_init_quirk() callbacks are setting xhci->quirks
> > * Tested with "usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720" patch
> > * Removed Fixes: line
> > ---
> >  drivers/usb/host/xhci-plat.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 4d34f6005381..0eab7cb5a767 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> >  /* called during probe() after chip reset completes */
> >  static int xhci_plat_setup(struct usb_hcd *hcd)
> >  {
> > -	int ret;
> > -
> > -
> > -	ret = xhci_priv_init_quirk(hcd);
> > -	if (ret)
> > -		return ret;
> > -
> >  	return xhci_gen_setup(hcd, xhci_plat_quirks);
> >  }
> >  
> > @@ -330,7 +323,14 @@ 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))
> > +
> > +	if (priv) {
> > +		ret = xhci_priv_init_quirk(hcd);
> > +		if (ret)
> > +			goto disable_usb_phy;
> > +	}
> > +
> > +	if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks & XHCI_SKIP_PHY_INIT)))
> >  		hcd->skip_phy_initialization = 1;
> 
> I am not sure if others agree with you move the position of
> xhci_priv_init_quirk, Let's see Mathias opinion.

Hello! Do you have an opinion how to handle this issue? As currently it
is needed for another patch which is fixing issue/regression in xhci-mvebu:
https://lore.kernel.org/linux-usb/20201223162403.10897-1-pali@kernel.org/

Re: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Mathias Nyman]

On 14.1.2021 1.20, Pali Rohár wrote:
> On Thursday 24 December 2020 05:59:05 Peter Chen wrote:
>> On 20-12-23 17:18:47, Pali Rohár wrote:
>>> Currently init_quirk callbacks for xhci platform drivers are called
>>> xhci_plat_setup() function which is called after chip reset completes.
>>> It happens in the middle of the usb_add_hcd() function.
>>>
>>> But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
>>> prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
>>> currently does nothing as prior xhci_plat_setup() it is not set.
>>>
>>> Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
>>> which really needs to be set prior calling usb_add_hcd() as this function
>>> at its beginning skips PHY init if this member is set.
>>>
>>> This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
>>> init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
>>> XHCI_SKIP_PHY_INIT is set. Also checking if either xhci->quirks or
>>> priv->quirks contains this XHCI_SKIP_PHY_INIT quirk.
>>>
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>>
>>> ---
>>> Changes in v2:
>>> * Check also xhci->quirks as xhci_priv_init_quirk() callbacks are setting xhci->quirks
>>> * Tested with "usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720" patch
>>> * Removed Fixes: line
>>> ---
>>>  drivers/usb/host/xhci-plat.c | 16 ++++++++--------
>>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>> index 4d34f6005381..0eab7cb5a767 100644
>>> --- a/drivers/usb/host/xhci-plat.c
>>> +++ b/drivers/usb/host/xhci-plat.c
>>> @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>>>  /* called during probe() after chip reset completes */
>>>  static int xhci_plat_setup(struct usb_hcd *hcd)
>>>  {
>>> -	int ret;
>>> -
>>> -
>>> -	ret = xhci_priv_init_quirk(hcd);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>>  	return xhci_gen_setup(hcd, xhci_plat_quirks);
>>>  }
>>>  
>>> @@ -330,7 +323,14 @@ 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))
>>> +
>>> +	if (priv) {
>>> +		ret = xhci_priv_init_quirk(hcd);
>>> +		if (ret)
>>> +			goto disable_usb_phy;
>>> +	}
>>> +
>>> +	if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks & XHCI_SKIP_PHY_INIT)))
>>>  		hcd->skip_phy_initialization = 1;
>>
>> I am not sure if others agree with you move the position of
>> xhci_priv_init_quirk, Let's see Mathias opinion.
> 
> Hello! Do you have an opinion how to handle this issue? As currently it
> is needed for another patch which is fixing issue/regression in xhci-mvebu:
> https://lore.kernel.org/linux-usb/20201223162403.10897-1-pali@kernel.org/
> 

I can see the benefit in this. 
In the xhci-plat case usb_create_hcd and usb_add_hcd are separate steps, and
we could both copy the xhci_plat_priv .quirks and run the .init_qurks before
adding the hcd.
I guess the current way is inherited from pci case where the earliest place
to do this after hcd is created is the hcd->driver->reset callback (which is
set to xhci_pci_setup() or xhci_plat_setup()).

xhci-rcar.c is using the .init_quirk to load firmware, we need to check with
them if this change is ok. (added Yoshihiro Shimoda to cc)
Their firmware would be loaded before phy parts are initialized, usb bus
registered, or roothub device allocated.

Thanks
-Mathias

Re: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

On Friday 15 January 2021 15:32:30 Mathias Nyman wrote:
> On 14.1.2021 1.20, Pali Rohár wrote:
> > On Thursday 24 December 2020 05:59:05 Peter Chen wrote:
> >> On 20-12-23 17:18:47, Pali Rohár wrote:
> >>> Currently init_quirk callbacks for xhci platform drivers are called
> >>> xhci_plat_setup() function which is called after chip reset completes.
> >>> It happens in the middle of the usb_add_hcd() function.
> >>>
> >>> But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> >>> prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> >>> currently does nothing as prior xhci_plat_setup() it is not set.
> >>>
> >>> Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> >>> which really needs to be set prior calling usb_add_hcd() as this function
> >>> at its beginning skips PHY init if this member is set.
> >>>
> >>> This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> >>> init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> >>> XHCI_SKIP_PHY_INIT is set. Also checking if either xhci->quirks or
> >>> priv->quirks contains this XHCI_SKIP_PHY_INIT quirk.
> >>>
> >>> Signed-off-by: Pali Rohár <pali@kernel.org>
> >>>
> >>> ---
> >>> Changes in v2:
> >>> * Check also xhci->quirks as xhci_priv_init_quirk() callbacks are setting xhci->quirks
> >>> * Tested with "usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720" patch
> >>> * Removed Fixes: line
> >>> ---
> >>>  drivers/usb/host/xhci-plat.c | 16 ++++++++--------
> >>>  1 file changed, 8 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> >>> index 4d34f6005381..0eab7cb5a767 100644
> >>> --- a/drivers/usb/host/xhci-plat.c
> >>> +++ b/drivers/usb/host/xhci-plat.c
> >>> @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> >>>  /* called during probe() after chip reset completes */
> >>>  static int xhci_plat_setup(struct usb_hcd *hcd)
> >>>  {
> >>> -	int ret;
> >>> -
> >>> -
> >>> -	ret = xhci_priv_init_quirk(hcd);
> >>> -	if (ret)
> >>> -		return ret;
> >>> -
> >>>  	return xhci_gen_setup(hcd, xhci_plat_quirks);
> >>>  }
> >>>  
> >>> @@ -330,7 +323,14 @@ 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))
> >>> +
> >>> +	if (priv) {
> >>> +		ret = xhci_priv_init_quirk(hcd);
> >>> +		if (ret)
> >>> +			goto disable_usb_phy;
> >>> +	}
> >>> +
> >>> +	if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks & XHCI_SKIP_PHY_INIT)))
> >>>  		hcd->skip_phy_initialization = 1;
> >>
> >> I am not sure if others agree with you move the position of
> >> xhci_priv_init_quirk, Let's see Mathias opinion.
> > 
> > Hello! Do you have an opinion how to handle this issue? As currently it
> > is needed for another patch which is fixing issue/regression in xhci-mvebu:
> > https://lore.kernel.org/linux-usb/20201223162403.10897-1-pali@kernel.org/
> > 
> 
> I can see the benefit in this. 
> In the xhci-plat case usb_create_hcd and usb_add_hcd are separate steps, and
> we could both copy the xhci_plat_priv .quirks and run the .init_qurks before
> adding the hcd.
> I guess the current way is inherited from pci case where the earliest place
> to do this after hcd is created is the hcd->driver->reset callback (which is
> set to xhci_pci_setup() or xhci_plat_setup()).
> 
> xhci-rcar.c is using the .init_quirk to load firmware, we need to check with
> them if this change is ok. (added Yoshihiro Shimoda to cc)

Yoshihiro, is this change OK?

Can we move forward? I really need to now how to handle regression in
xhci-mvebu driver. And one option is with this patch...

> Their firmware would be loaded before phy parts are initialized, usb bus
> registered, or roothub device allocated.
> 
> Thanks
> -Mathias

RE: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Yoshihiro Shimoda]

Hi Pali,

> From: Pali Rohár, Sent: Monday, January 25, 2021 11:20 PM
> On Friday 15 January 2021 15:32:30 Mathias Nyman wrote:
> > On 14.1.2021 1.20, Pali Rohár wrote:
> > > On Thursday 24 December 2020 05:59:05 Peter Chen wrote:
> > >> On 20-12-23 17:18:47, Pali Rohár wrote:
> > >>> Currently init_quirk callbacks for xhci platform drivers are called
> > >>> xhci_plat_setup() function which is called after chip reset completes.
> > >>> It happens in the middle of the usb_add_hcd() function.
> > >>>
> > >>> But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> > >>> prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> > >>> currently does nothing as prior xhci_plat_setup() it is not set.
> > >>>
> > >>> Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> > >>> which really needs to be set prior calling usb_add_hcd() as this function
> > >>> at its beginning skips PHY init if this member is set.
> > >>>
> > >>> This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> > >>> init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> > >>> XHCI_SKIP_PHY_INIT is set. Also checking if either xhci->quirks or
> > >>> priv->quirks contains this XHCI_SKIP_PHY_INIT quirk.
> > >>>
> > >>> Signed-off-by: Pali Rohár <pali@kernel.org>
> > >>>
> > >>> ---
> > >>> Changes in v2:
> > >>> * Check also xhci->quirks as xhci_priv_init_quirk() callbacks are setting xhci->quirks
> > >>> * Tested with "usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720" patch
> > >>> * Removed Fixes: line
> > >>> ---
> > >>>  drivers/usb/host/xhci-plat.c | 16 ++++++++--------
> > >>>  1 file changed, 8 insertions(+), 8 deletions(-)
> > >>>
> > >>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > >>> index 4d34f6005381..0eab7cb5a767 100644
> > >>> --- a/drivers/usb/host/xhci-plat.c
> > >>> +++ b/drivers/usb/host/xhci-plat.c
> > >>> @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> > >>>  /* called during probe() after chip reset completes */
> > >>>  static int xhci_plat_setup(struct usb_hcd *hcd)
> > >>>  {
> > >>> -	int ret;
> > >>> -
> > >>> -
> > >>> -	ret = xhci_priv_init_quirk(hcd);
> > >>> -	if (ret)
> > >>> -		return ret;
> > >>> -
> > >>>  	return xhci_gen_setup(hcd, xhci_plat_quirks);
> > >>>  }
> > >>>
> > >>> @@ -330,7 +323,14 @@ 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))
> > >>> +
> > >>> +	if (priv) {
> > >>> +		ret = xhci_priv_init_quirk(hcd);
> > >>> +		if (ret)
> > >>> +			goto disable_usb_phy;
> > >>> +	}
> > >>> +
> > >>> +	if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks & XHCI_SKIP_PHY_INIT)))
> > >>>  		hcd->skip_phy_initialization = 1;
> > >>
> > >> I am not sure if others agree with you move the position of
> > >> xhci_priv_init_quirk, Let's see Mathias opinion.
> > >
> > > Hello! Do you have an opinion how to handle this issue? As currently it
> > > is needed for another patch which is fixing issue/regression in xhci-mvebu:
> > >
<snip>
> > >
> >
> > I can see the benefit in this.
> > In the xhci-plat case usb_create_hcd and usb_add_hcd are separate steps, and
> > we could both copy the xhci_plat_priv .quirks and run the .init_qurks before
> > adding the hcd.
> > I guess the current way is inherited from pci case where the earliest place
> > to do this after hcd is created is the hcd->driver->reset callback (which is
> > set to xhci_pci_setup() or xhci_plat_setup()).
> >
> > xhci-rcar.c is using the .init_quirk to load firmware, we need to check with
> > them if this change is ok. (added Yoshihiro Shimoda to cc)
> 
> Yoshihiro, is this change OK?
> 
> Can we move forward? I really need to now how to handle regression in
> xhci-mvebu driver. And one option is with this patch...

Thank you for asking me about this topic. I tested the patch, but unfortunately,
this patch is possible to break a rcar platform because a phy initialization is
needed before the firmware loading if the platform uses the phy. (Note that
upstream code (salvator-common.dtsi) doesn't use the phy for xhci. But,
if we use the phy on other board with this patch, the xhci will not work.)

So, I think we need to add a new function pointer for your case.

Best regards,
Yoshihiro Shimoda

> > Their firmware would be loaded before phy parts are initialized, usb bus
> > registered, or roothub device allocated.
> >
> > Thanks
> > -Mathias

Re: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

On Tuesday 26 January 2021 04:27:37 Yoshihiro Shimoda wrote:
> Hi Pali,
> 
> > From: Pali Rohár, Sent: Monday, January 25, 2021 11:20 PM
> > On Friday 15 January 2021 15:32:30 Mathias Nyman wrote:
> > > On 14.1.2021 1.20, Pali Rohár wrote:
> > > > On Thursday 24 December 2020 05:59:05 Peter Chen wrote:
> > > >> On 20-12-23 17:18:47, Pali Rohár wrote:
> > > >>> Currently init_quirk callbacks for xhci platform drivers are called
> > > >>> xhci_plat_setup() function which is called after chip reset completes.
> > > >>> It happens in the middle of the usb_add_hcd() function.
> > > >>>
> > > >>> But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> > > >>> prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> > > >>> currently does nothing as prior xhci_plat_setup() it is not set.
> > > >>>
> > > >>> Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> > > >>> which really needs to be set prior calling usb_add_hcd() as this function
> > > >>> at its beginning skips PHY init if this member is set.
> > > >>>
> > > >>> This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> > > >>> init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> > > >>> XHCI_SKIP_PHY_INIT is set. Also checking if either xhci->quirks or
> > > >>> priv->quirks contains this XHCI_SKIP_PHY_INIT quirk.
> > > >>>
> > > >>> Signed-off-by: Pali Rohár <pali@kernel.org>
> > > >>>
> > > >>> ---
> > > >>> Changes in v2:
> > > >>> * Check also xhci->quirks as xhci_priv_init_quirk() callbacks are setting xhci->quirks
> > > >>> * Tested with "usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720" patch
> > > >>> * Removed Fixes: line
> > > >>> ---
> > > >>>  drivers/usb/host/xhci-plat.c | 16 ++++++++--------
> > > >>>  1 file changed, 8 insertions(+), 8 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > >>> index 4d34f6005381..0eab7cb5a767 100644
> > > >>> --- a/drivers/usb/host/xhci-plat.c
> > > >>> +++ b/drivers/usb/host/xhci-plat.c
> > > >>> @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> > > >>>  /* called during probe() after chip reset completes */
> > > >>>  static int xhci_plat_setup(struct usb_hcd *hcd)
> > > >>>  {
> > > >>> -	int ret;
> > > >>> -
> > > >>> -
> > > >>> -	ret = xhci_priv_init_quirk(hcd);
> > > >>> -	if (ret)
> > > >>> -		return ret;
> > > >>> -
> > > >>>  	return xhci_gen_setup(hcd, xhci_plat_quirks);
> > > >>>  }
> > > >>>
> > > >>> @@ -330,7 +323,14 @@ 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))
> > > >>> +
> > > >>> +	if (priv) {
> > > >>> +		ret = xhci_priv_init_quirk(hcd);
> > > >>> +		if (ret)
> > > >>> +			goto disable_usb_phy;
> > > >>> +	}
> > > >>> +
> > > >>> +	if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks & XHCI_SKIP_PHY_INIT)))
> > > >>>  		hcd->skip_phy_initialization = 1;
> > > >>
> > > >> I am not sure if others agree with you move the position of
> > > >> xhci_priv_init_quirk, Let's see Mathias opinion.
> > > >
> > > > Hello! Do you have an opinion how to handle this issue? As currently it
> > > > is needed for another patch which is fixing issue/regression in xhci-mvebu:
> > > >
> <snip>
> > > >
> > >
> > > I can see the benefit in this.
> > > In the xhci-plat case usb_create_hcd and usb_add_hcd are separate steps, and
> > > we could both copy the xhci_plat_priv .quirks and run the .init_qurks before
> > > adding the hcd.
> > > I guess the current way is inherited from pci case where the earliest place
> > > to do this after hcd is created is the hcd->driver->reset callback (which is
> > > set to xhci_pci_setup() or xhci_plat_setup()).
> > >
> > > xhci-rcar.c is using the .init_quirk to load firmware, we need to check with
> > > them if this change is ok. (added Yoshihiro Shimoda to cc)
> > 
> > Yoshihiro, is this change OK?
> > 
> > Can we move forward? I really need to now how to handle regression in
> > xhci-mvebu driver. And one option is with this patch...
> 
> Thank you for asking me about this topic. I tested the patch, but unfortunately,
> this patch is possible to break a rcar platform because a phy initialization is
> needed before the firmware loading if the platform uses the phy. (Note that
> upstream code (salvator-common.dtsi) doesn't use the phy for xhci. But,
> if we use the phy on other board with this patch, the xhci will not work.)
> 
> So, I think we need to add a new function pointer for your case.

Ok, thank you for testing! I will try to come up with other solution to
mentioned mvebu-xhci issue.

> Best regards,
> Yoshihiro Shimoda
> 
> > > Their firmware would be loaded before phy parts are initialized, usb bus
> > > registered, or roothub device allocated.
> > >
> > > Thanks
> > > -Mathias

Re: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Pali Rohár]

On Tuesday 26 January 2021 10:06:06 Pali Rohár wrote:
> On Tuesday 26 January 2021 04:27:37 Yoshihiro Shimoda wrote:
> > Hi Pali,
> > > > I can see the benefit in this.
> > > > In the xhci-plat case usb_create_hcd and usb_add_hcd are separate steps, and
> > > > we could both copy the xhci_plat_priv .quirks and run the .init_qurks before
> > > > adding the hcd.
> > > > I guess the current way is inherited from pci case where the earliest place
> > > > to do this after hcd is created is the hcd->driver->reset callback (which is
> > > > set to xhci_pci_setup() or xhci_plat_setup()).
> > > >
> > > > xhci-rcar.c is using the .init_quirk to load firmware, we need to check with
> > > > them if this change is ok. (added Yoshihiro Shimoda to cc)
> > > 
> > > Yoshihiro, is this change OK?
> > > 
> > > Can we move forward? I really need to now how to handle regression in
> > > xhci-mvebu driver. And one option is with this patch...
> > 
> > Thank you for asking me about this topic. I tested the patch, but unfortunately,
> > this patch is possible to break a rcar platform because a phy initialization is
> > needed before the firmware loading if the platform uses the phy. (Note that
> > upstream code (salvator-common.dtsi) doesn't use the phy for xhci. But,
> > if we use the phy on other board with this patch, the xhci will not work.)
> > 
> > So, I think we need to add a new function pointer for your case.
> 
> Ok, thank you for testing! I will try to come up with other solution to
> mentioned mvebu-xhci issue.

Hello! New version of this patch is in following thread, please review it:
"[PATCH v2] usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720"
https://lore.kernel.org/linux-usb/20210201150803.7305-1-pali@kernel.org/

> > Best regards,
> > Yoshihiro Shimoda
> > 
> > > > Their firmware would be loaded before phy parts are initialized, usb bus
> > > > registered, or roothub device allocated.
> > > >
> > > > Thanks
> > > > -Mathias

RE: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk

[Yoshihiro Shimoda]

> From: Pali Rohár, Sent: Tuesday, February 2, 2021 12:12 AM
> On Tuesday 26 January 2021 10:06:06 Pali Rohár wrote:
> > On Tuesday 26 January 2021 04:27:37 Yoshihiro Shimoda wrote:
> > > Hi Pali,
> > > > > I can see the benefit in this.
> > > > > In the xhci-plat case usb_create_hcd and usb_add_hcd are separate steps, and
> > > > > we could both copy the xhci_plat_priv .quirks and run the .init_qurks before
> > > > > adding the hcd.
> > > > > I guess the current way is inherited from pci case where the earliest place
> > > > > to do this after hcd is created is the hcd->driver->reset callback (which is
> > > > > set to xhci_pci_setup() or xhci_plat_setup()).
> > > > >
> > > > > xhci-rcar.c is using the .init_quirk to load firmware, we need to check with
> > > > > them if this change is ok. (added Yoshihiro Shimoda to cc)
> > > >
> > > > Yoshihiro, is this change OK?
> > > >
> > > > Can we move forward? I really need to now how to handle regression in
> > > > xhci-mvebu driver. And one option is with this patch...
> > >
> > > Thank you for asking me about this topic. I tested the patch, but unfortunately,
> > > this patch is possible to break a rcar platform because a phy initialization is
> > > needed before the firmware loading if the platform uses the phy. (Note that
> > > upstream code (salvator-common.dtsi) doesn't use the phy for xhci. But,
> > > if we use the phy on other board with this patch, the xhci will not work.)
> > >
> > > So, I think we need to add a new function pointer for your case.
> >
> > Ok, thank you for testing! I will try to come up with other solution to
> > mentioned mvebu-xhci issue.
> 
> Hello! New version of this patch is in following thread, please review it:
> "[PATCH v2] usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720"

Thanks! I'll review it.

Best regards,
Yoshihiro Shimoda