| 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 |
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");
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 >
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
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
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 --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");
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(+)