diff mbox series

[2/2] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT

Message ID 1644949454-814-3-git-send-email-quic_c_sanm@quicinc.com
State Superseded
Headers show
Series Refactor xhci quirks and plat private data | expand

Commit Message

Sandeep Maheswaram Feb. 15, 2022, 6:24 p.m. UTC
dwc3 manages PHY by own DRD driver, so skip the management by
HCD core.
During runtime suspend phy was not getting suspend because
runtime_usage value is 2.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
---
 drivers/usb/dwc3/host.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Stephen Boyd Feb. 16, 2022, 2:15 a.m. UTC | #1
Quoting Sandeep Maheswaram (2022-02-15 10:24:14)
> dwc3 manages PHY by own DRD driver, so skip the management by
> HCD core.
> During runtime suspend phy was not getting suspend because
> runtime_usage value is 2.
>
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>

Any Fixes tag?

> ---
>  drivers/usb/dwc3/host.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index eda8719..4a035a8 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -13,6 +13,14 @@
>  #include <linux/platform_device.h>
>
>  #include "core.h"
> +#include <linux/usb/hcd.h>

What is this include used for now?

> +#include <linux/usb/xhci-plat.h>
> +#include <linux/usb/xhci-quirks.h>
> +
> +

Nitpick: Remove one newline.

> +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
> +       .quirks = XHCI_SKIP_PHY_INIT,
> +};
>
>  static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
>                                         int irq, char *name)
> @@ -122,6 +130,13 @@ int dwc3_host_init(struct dwc3 *dwc)
>                 }
>         }
>
> +       ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
> +                       sizeof(struct xhci_plat_priv));

Nitpick: sizeof(xhci_plat_dwc3_xhci) so the type can change without
changing this line.

> +       if (ret) {
> +               dev_err(dwc->dev, "failed to add data to xHCI\n");
> +               goto err;
> +       }
> +
>         ret = platform_device_add(xhci);
>         if (ret) {
>                 dev_err(dwc->dev, "failed to register xHCI device\n");
Jun Li Feb. 16, 2022, 7:16 a.m. UTC | #2
Sandeep Maheswaram <quic_c_sanm@quicinc.com> 于2022年2月16日周三 14:58写道:
>
> dwc3 manages PHY by own DRD driver, so skip the management by
> HCD core.
> During runtime suspend phy was not getting suspend because
> runtime_usage value is 2.
>
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> ---
>  drivers/usb/dwc3/host.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index eda8719..4a035a8 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -13,6 +13,14 @@
>  #include <linux/platform_device.h>
>
>  #include "core.h"
> +#include <linux/usb/hcd.h>
> +#include <linux/usb/xhci-plat.h>
> +#include <linux/usb/xhci-quirks.h>
> +
> +
> +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
> +       .quirks = XHCI_SKIP_PHY_INIT,
> +};

It's better to create this xhci_plat_priv by each dwc3 glue layer,
with that, we can use this priv to pass other flags and possibly
override APIs by each glue driver which may not apply to all dwc3
platforms.

thanks
Li Jun
>
>  static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
>                                         int irq, char *name)
> @@ -122,6 +130,13 @@ int dwc3_host_init(struct dwc3 *dwc)
>                 }
>         }
>
> +       ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
> +                       sizeof(struct xhci_plat_priv));
> +       if (ret) {
> +               dev_err(dwc->dev, "failed to add data to xHCI\n");
> +               goto err;
> +       }
> +
>         ret = platform_device_add(xhci);
>         if (ret) {
>                 dev_err(dwc->dev, "failed to register xHCI device\n");
> --
> 2.7.4
>
Pavan Kondeti Feb. 16, 2022, 8 a.m. UTC | #3
On Wed, Feb 16, 2022 at 03:16:40PM +0800, Jun Li wrote:
> Sandeep Maheswaram <quic_c_sanm@quicinc.com> 于2022年2月16日周三 14:58写道:
> >
> > dwc3 manages PHY by own DRD driver, so skip the management by
> > HCD core.
> > During runtime suspend phy was not getting suspend because
> > runtime_usage value is 2.
> >
> > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > ---
> >  drivers/usb/dwc3/host.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > index eda8719..4a035a8 100644
> > --- a/drivers/usb/dwc3/host.c
> > +++ b/drivers/usb/dwc3/host.c
> > @@ -13,6 +13,14 @@
> >  #include <linux/platform_device.h>
> >
> >  #include "core.h"
> > +#include <linux/usb/hcd.h>
> > +#include <linux/usb/xhci-plat.h>
> > +#include <linux/usb/xhci-quirks.h>
> > +
> > +
> > +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
> > +       .quirks = XHCI_SKIP_PHY_INIT,
> > +};
> 
> It's better to create this xhci_plat_priv by each dwc3 glue layer,
> with that, we can use this priv to pass other flags and possibly
> override APIs by each glue driver which may not apply to all dwc3
> platforms.
> 

Do you see a need for any glue driver to know about this xHC platform data?
AFAICT, glue driver has no direction connection with the dwc3 core. All
the required data is coming from dT on ARM based boards. Adding a private
interface between dwc3 core and glue for passing xhci platform data seems
to be overkill. If there is a pressing need, why not?

Thanks,
Pavan
Jun Li Feb. 16, 2022, 8:49 a.m. UTC | #4
Pavan Kondeti <quic_pkondeti@quicinc.com> 于2022年2月16日周三 16:00写道:
>
> On Wed, Feb 16, 2022 at 03:16:40PM +0800, Jun Li wrote:
> > Sandeep Maheswaram <quic_c_sanm@quicinc.com> 于2022年2月16日周三 14:58写道:
> > >
> > > dwc3 manages PHY by own DRD driver, so skip the management by
> > > HCD core.
> > > During runtime suspend phy was not getting suspend because
> > > runtime_usage value is 2.
> > >
> > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > > ---
> > >  drivers/usb/dwc3/host.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > > index eda8719..4a035a8 100644
> > > --- a/drivers/usb/dwc3/host.c
> > > +++ b/drivers/usb/dwc3/host.c
> > > @@ -13,6 +13,14 @@
> > >  #include <linux/platform_device.h>
> > >
> > >  #include "core.h"
> > > +#include <linux/usb/hcd.h>
> > > +#include <linux/usb/xhci-plat.h>
> > > +#include <linux/usb/xhci-quirks.h>
> > > +
> > > +
> > > +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
> > > +       .quirks = XHCI_SKIP_PHY_INIT,
> > > +};
> >
> > It's better to create this xhci_plat_priv by each dwc3 glue layer,
> > with that, we can use this priv to pass other flags and possibly
> > override APIs by each glue driver which may not apply to all dwc3
> > platforms.
> >
>
> Do you see a need for any glue driver to know about this xHC platform data?

Yes. I have some xhci quirks which are specifix to NXP iMX platforms.

> AFAICT, glue driver has no direction connection with the dwc3 core. All
> the required data is coming from dT on ARM based boards. Adding a private
> interface between dwc3 core and glue for passing xhci platform data seems
> to be overkill. If there is a pressing need, why not?

And looking at xhci_plat_priv members

-struct xhci_plat_priv {
-       const char *firmware_name;
-       unsigned long long quirks;
-       int (*plat_setup)(struct usb_hcd *);
-       void (*plat_start)(struct usb_hcd *);
-       int (*init_quirk)(struct usb_hcd *);
-       int (*suspend_quirk)(struct usb_hcd *);
-       int (*resume_quirk)(struct usb_hcd *);
-};

Are we going to share the same all those quirks and APIs
implementation across all dwc3 platforms?

Thanks
Li Jun
>
> Thanks,
> Pavan
Pavan Kondeti Feb. 16, 2022, 8:58 a.m. UTC | #5
On Wed, Feb 16, 2022 at 04:49:32PM +0800, Jun Li wrote:
> Pavan Kondeti <quic_pkondeti@quicinc.com> 于2022年2月16日周三 16:00写道:
> >
> > On Wed, Feb 16, 2022 at 03:16:40PM +0800, Jun Li wrote:
> > > Sandeep Maheswaram <quic_c_sanm@quicinc.com> 于2022年2月16日周三 14:58写道:
> > > >
> > > > dwc3 manages PHY by own DRD driver, so skip the management by
> > > > HCD core.
> > > > During runtime suspend phy was not getting suspend because
> > > > runtime_usage value is 2.
> > > >
> > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > > > ---
> > > >  drivers/usb/dwc3/host.c | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > > > index eda8719..4a035a8 100644
> > > > --- a/drivers/usb/dwc3/host.c
> > > > +++ b/drivers/usb/dwc3/host.c
> > > > @@ -13,6 +13,14 @@
> > > >  #include <linux/platform_device.h>
> > > >
> > > >  #include "core.h"
> > > > +#include <linux/usb/hcd.h>
> > > > +#include <linux/usb/xhci-plat.h>
> > > > +#include <linux/usb/xhci-quirks.h>
> > > > +
> > > > +
> > > > +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
> > > > +       .quirks = XHCI_SKIP_PHY_INIT,
> > > > +};
> > >
> > > It's better to create this xhci_plat_priv by each dwc3 glue layer,
> > > with that, we can use this priv to pass other flags and possibly
> > > override APIs by each glue driver which may not apply to all dwc3
> > > platforms.
> > >
> >
> > Do you see a need for any glue driver to know about this xHC platform data?
> 
> Yes. I have some xhci quirks which are specifix to NXP iMX platforms.
> 
> > AFAICT, glue driver has no direction connection with the dwc3 core. All
> > the required data is coming from dT on ARM based boards. Adding a private
> > interface between dwc3 core and glue for passing xhci platform data seems
> > to be overkill. If there is a pressing need, why not?
> 
> And looking at xhci_plat_priv members
> 
> -struct xhci_plat_priv {
> -       const char *firmware_name;
> -       unsigned long long quirks;
> -       int (*plat_setup)(struct usb_hcd *);
> -       void (*plat_start)(struct usb_hcd *);
> -       int (*init_quirk)(struct usb_hcd *);
> -       int (*suspend_quirk)(struct usb_hcd *);
> -       int (*resume_quirk)(struct usb_hcd *);
> -};
> 
> Are we going to share the same all those quirks and APIs
> implementation across all dwc3 platforms?
> 
Currently Yes. Thats why I am asking if there is a pressing need to
make this more complex than it needs to be..

Thanks,
Pavan
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index eda8719..4a035a8 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -13,6 +13,14 @@ 
 #include <linux/platform_device.h>
 
 #include "core.h"
+#include <linux/usb/hcd.h>
+#include <linux/usb/xhci-plat.h>
+#include <linux/usb/xhci-quirks.h>
+
+
+static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
+	.quirks = XHCI_SKIP_PHY_INIT,
+};
 
 static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
 					int irq, char *name)
@@ -122,6 +130,13 @@  int dwc3_host_init(struct dwc3 *dwc)
 		}
 	}
 
+	ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
+			sizeof(struct xhci_plat_priv));
+	if (ret) {
+		dev_err(dwc->dev, "failed to add data to xHCI\n");
+		goto err;
+	}
+
 	ret = platform_device_add(xhci);
 	if (ret) {
 		dev_err(dwc->dev, "failed to register xHCI device\n");