[PATCH 0/2] Refactor xhci quirks and plat private data

[PATCH 0/2] Refactor xhci quirks and plat private data

[Sandeep Maheswaram]

This refactoring allows drivers like dwc3 host glue driver to
specify thier xhci quirks.

Pavankumar Kondeti (1):
  usb: xhci: refactor quirks and plat private data

Sandeep Maheswaram (1):
  usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT

 drivers/usb/cdns3/host.c        |  2 +-
 drivers/usb/dwc3/host.c         | 15 +++++++++
 drivers/usb/host/xhci-plat.c    |  3 +-
 drivers/usb/host/xhci-plat.h    | 25 ---------------
 drivers/usb/host/xhci-rcar.c    |  3 +-
 drivers/usb/host/xhci.h         | 60 ++++--------------------------------
 include/linux/usb/xhci-plat.h   | 24 +++++++++++++++
 include/linux/usb/xhci-quirks.h | 67 +++++++++++++++++++++++++++++++++++++++++
 8 files changed, 117 insertions(+), 82 deletions(-)
 delete mode 100644 drivers/usb/host/xhci-plat.h
 create mode 100644 include/linux/usb/xhci-plat.h
 create mode 100644 include/linux/usb/xhci-quirks.h

-- 
2.7.4

[PATCH 1/2] usb: xhci: refactor quirks and plat private data

[Sandeep Maheswaram]

From: Pavankumar Kondeti <quic_pkondeti@quicinc.com>

This refactoring allows drivers like dwc3 host glue driver to
specify their xhci quirks.

Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
---
 drivers/usb/cdns3/host.c        |  2 +-
 drivers/usb/host/xhci-plat.c    |  3 +-
 drivers/usb/host/xhci-plat.h    | 25 ---------------
 drivers/usb/host/xhci-rcar.c    |  3 +-
 drivers/usb/host/xhci.h         | 60 ++++--------------------------------
 include/linux/usb/xhci-plat.h   | 24 +++++++++++++++
 include/linux/usb/xhci-quirks.h | 67 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 102 insertions(+), 82 deletions(-)
 delete mode 100644 drivers/usb/host/xhci-plat.h
 create mode 100644 include/linux/usb/xhci-plat.h
 create mode 100644 include/linux/usb/xhci-quirks.h

diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 9643b90..7fb8049 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -15,8 +15,8 @@
 #include "drd.h"
 #include "host-export.h"
 #include <linux/usb/hcd.h>
+#include <linux/usb/xhci-plat.h>
 #include "../host/xhci.h"
-#include "../host/xhci-plat.h"
 
 #define XECP_PORT_CAP_REG	0x8000
 #define XECP_AUX_CTRL_REG1	0x8120
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 8094da3..8dfe6b3 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -19,9 +19,10 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/usb/of.h>
+#include <linux/usb/xhci-quirks.h>
+#include <linux/usb/xhci-plat.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
 
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
deleted file mode 100644
index 561d0b7..0000000
--- a/drivers/usb/host/xhci-plat.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * xhci-plat.h - xHCI host controller driver platform Bus Glue.
- *
- * Copyright (C) 2015 Renesas Electronics Corporation
- */
-
-#ifndef _XHCI_PLAT_H
-#define _XHCI_PLAT_H
-
-#include "xhci.h"	/* for hcd_to_xhci() */
-
-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 *);
-};
-
-#define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
-#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
-#endif	/* _XHCI_PLAT_H */
diff --git a/drivers/usb/host/xhci-rcar.c b/drivers/usb/host/xhci-rcar.c
index 9888ba7..cbcb6ba 100644
--- a/drivers/usb/host/xhci-rcar.c
+++ b/drivers/usb/host/xhci-rcar.c
@@ -12,9 +12,10 @@
 #include <linux/of.h>
 #include <linux/usb/phy.h>
 #include <linux/sys_soc.h>
+#include <linux/usb/xhci-quirks.h>
+#include <linux/usb/xhci-plat.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-rcar.h"
 
 /*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 5a75fe5..4e80d08 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -17,6 +17,8 @@
 #include <linux/kernel.h>
 #include <linux/usb/hcd.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/usb/xhci-quirks.h>
+#include <linux/usb/xhci-plat.h>
 
 /* Code sharing between pci-quirks and xhci hcd */
 #include	"xhci-ext-caps.h"
@@ -1846,60 +1848,6 @@ struct xhci_hcd {
 #define XHCI_STATE_HALTED	(1 << 1)
 #define XHCI_STATE_REMOVING	(1 << 2)
 	unsigned long long	quirks;
-#define	XHCI_LINK_TRB_QUIRK	BIT_ULL(0)
-#define XHCI_RESET_EP_QUIRK	BIT_ULL(1)
-#define XHCI_NEC_HOST		BIT_ULL(2)
-#define XHCI_AMD_PLL_FIX	BIT_ULL(3)
-#define XHCI_SPURIOUS_SUCCESS	BIT_ULL(4)
-/*
- * Certain Intel host controllers have a limit to the number of endpoint
- * contexts they can handle.  Ideally, they would signal that they can't handle
- * anymore endpoint contexts by returning a Resource Error for the Configure
- * Endpoint command, but they don't.  Instead they expect software to keep track
- * of the number of active endpoints for them, across configure endpoint
- * commands, reset device commands, disable slot commands, and address device
- * commands.
- */
-#define XHCI_EP_LIMIT_QUIRK	BIT_ULL(5)
-#define XHCI_BROKEN_MSI		BIT_ULL(6)
-#define XHCI_RESET_ON_RESUME	BIT_ULL(7)
-#define	XHCI_SW_BW_CHECKING	BIT_ULL(8)
-#define XHCI_AMD_0x96_HOST	BIT_ULL(9)
-#define XHCI_TRUST_TX_LENGTH	BIT_ULL(10)
-#define XHCI_LPM_SUPPORT	BIT_ULL(11)
-#define XHCI_INTEL_HOST		BIT_ULL(12)
-#define XHCI_SPURIOUS_REBOOT	BIT_ULL(13)
-#define XHCI_COMP_MODE_QUIRK	BIT_ULL(14)
-#define XHCI_AVOID_BEI		BIT_ULL(15)
-#define XHCI_PLAT		BIT_ULL(16)
-#define XHCI_SLOW_SUSPEND	BIT_ULL(17)
-#define XHCI_SPURIOUS_WAKEUP	BIT_ULL(18)
-/* For controllers with a broken beyond repair streams implementation */
-#define XHCI_BROKEN_STREAMS	BIT_ULL(19)
-#define XHCI_PME_STUCK_QUIRK	BIT_ULL(20)
-#define XHCI_MTK_HOST		BIT_ULL(21)
-#define XHCI_SSIC_PORT_UNUSED	BIT_ULL(22)
-#define XHCI_NO_64BIT_SUPPORT	BIT_ULL(23)
-#define XHCI_MISSING_CAS	BIT_ULL(24)
-/* For controller with a broken Port Disable implementation */
-#define XHCI_BROKEN_PORT_PED	BIT_ULL(25)
-#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
-#define XHCI_U2_DISABLE_WAKE	BIT_ULL(27)
-#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
-#define XHCI_HW_LPM_DISABLE	BIT_ULL(29)
-#define XHCI_SUSPEND_DELAY	BIT_ULL(30)
-#define XHCI_INTEL_USB_ROLE_SW	BIT_ULL(31)
-#define XHCI_ZERO_64B_REGS	BIT_ULL(32)
-#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
-#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)
-#define XHCI_DISABLE_SPARSE	BIT_ULL(38)
-#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
-#define XHCI_NO_SOFT_RETRY	BIT_ULL(40)
-#define XHCI_BROKEN_D3COLD	BIT_ULL(41)
-#define XHCI_EP_CTX_BROKEN_DCS	BIT_ULL(42)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
@@ -1965,6 +1913,10 @@ static inline struct usb_hcd *xhci_to_hcd(struct xhci_hcd *xhci)
 	return xhci->main_hcd;
 }
 
+/* For xhci-plat drivers */
+#define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
+#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
+
 #define xhci_dbg(xhci, fmt, args...) \
 	dev_dbg(xhci_to_hcd(xhci)->self.controller , fmt , ## args)
 #define xhci_err(xhci, fmt, args...) \
diff --git a/include/linux/usb/xhci-plat.h b/include/linux/usb/xhci-plat.h
new file mode 100644
index 0000000..58a56ae
--- /dev/null
+++ b/include/linux/usb/xhci-plat.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * xhci-plat.h - xHCI host controller driver platform Bus Glue.
+ *
+ * Copyright (C) 2015 Renesas Electronics Corporation
+ */
+
+#ifndef _XHCI_PLAT_H
+#define _XHCI_PLAT_H
+
+#include <linux/types.h>
+#include <linux/usb/hcd.h>
+
+struct xhci_plat_priv {
+	const char *firmware_name;
+	unsigned long long quirks;
+	int (*plat_setup)(struct usb_hcd *hcd);
+	void (*plat_start)(struct usb_hcd *hcd);
+	int (*init_quirk)(struct usb_hcd *hcd);
+	int (*suspend_quirk)(struct usb_hcd *hcd);
+	int (*resume_quirk)(struct usb_hcd *hcd);
+};
+
+#endif	/* _XHCI_PLAT_H */
diff --git a/include/linux/usb/xhci-quirks.h b/include/linux/usb/xhci-quirks.h
new file mode 100644
index 0000000..57ae0b1
--- /dev/null
+++ b/include/linux/usb/xhci-quirks.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * xHCI host controller driver quirks
+ *
+ * Copyright (C) 2008 Intel Corp.
+ */
+
+#ifndef _XHCI_QUIRKS_H
+#define _XHCI_QUIRKS_H
+
+#define XHCI_LINK_TRB_QUIRK	BIT_ULL(0)
+#define XHCI_RESET_EP_QUIRK	BIT_ULL(1)
+#define XHCI_NEC_HOST		BIT_ULL(2)
+#define XHCI_AMD_PLL_FIX	BIT_ULL(3)
+#define XHCI_SPURIOUS_SUCCESS	BIT_ULL(4)
+/*
+ * Certain Intel host controllers have a limit to the number of endpoint
+ * contexts they can handle.  Ideally, they would signal that they can't handle
+ * anymore endpoint contexts by returning a Resource Error for the Configure
+ * Endpoint command, but they don't.  Instead they expect software to keep track
+ * of the number of active endpoints for them, across configure endpoint
+ * commands, reset device commands, disable slot commands, and address device
+ * commands.
+ */
+#define XHCI_EP_LIMIT_QUIRK	BIT_ULL(5)
+#define XHCI_BROKEN_MSI		BIT_ULL(6)
+#define XHCI_RESET_ON_RESUME	BIT_ULL(7)
+#define XHCI_SW_BW_CHECKING	BIT_ULL(8)
+#define XHCI_AMD_0x96_HOST	BIT_ULL(9)
+#define XHCI_TRUST_TX_LENGTH	BIT_ULL(10)
+#define XHCI_LPM_SUPPORT	BIT_ULL(11)
+#define XHCI_INTEL_HOST		BIT_ULL(12)
+#define XHCI_SPURIOUS_REBOOT	BIT_ULL(13)
+#define XHCI_COMP_MODE_QUIRK	BIT_ULL(14)
+#define XHCI_AVOID_BEI		BIT_ULL(15)
+#define XHCI_PLAT		BIT_ULL(16)
+#define XHCI_SLOW_SUSPEND	BIT_ULL(17)
+#define XHCI_SPURIOUS_WAKEUP	BIT_ULL(18)
+/* For controllers with a broken beyond repair streams implementation */
+#define XHCI_BROKEN_STREAMS	BIT_ULL(19)
+#define XHCI_PME_STUCK_QUIRK	BIT_ULL(20)
+#define XHCI_MTK_HOST		BIT_ULL(21)
+#define XHCI_SSIC_PORT_UNUSED	BIT_ULL(22)
+#define XHCI_NO_64BIT_SUPPORT	BIT_ULL(23)
+#define XHCI_MISSING_CAS	BIT_ULL(24)
+/* For controller with a broken Port Disable implementation */
+#define XHCI_BROKEN_PORT_PED	BIT_ULL(25)
+#define XHCI_LIMIT_ENDPOINT_INTERVAL_7	BIT_ULL(26)
+#define XHCI_U2_DISABLE_WAKE	BIT_ULL(27)
+#define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	BIT_ULL(28)
+#define XHCI_HW_LPM_DISABLE	BIT_ULL(29)
+#define XHCI_SUSPEND_DELAY	BIT_ULL(30)
+#define XHCI_INTEL_USB_ROLE_SW	BIT_ULL(31)
+#define XHCI_ZERO_64B_REGS	BIT_ULL(32)
+#define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
+#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)
+#define XHCI_DISABLE_SPARSE	BIT_ULL(38)
+#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
+#define XHCI_NO_SOFT_RETRY	BIT_ULL(40)
+#define XHCI_BROKEN_D3COLD	BIT_ULL(41)
+#define XHCI_EP_CTX_BROKEN_DCS	BIT_ULL(42)
+
+#endif /* _XHCI_QUIRKS_H */
-- 
2.7.4

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

[Sandeep Maheswaram]

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,
+};
 
 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

Re: [PATCH 1/2] usb: xhci: refactor quirks and plat private data

[Stephen Boyd]

Quoting Sandeep Maheswaram (2022-02-15 10:24:13)
> From: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
>
> This refactoring allows drivers like dwc3 host glue driver to
> specify their xhci quirks.
>
> Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>

Your SoB should be here as well.

> diff --git a/include/linux/usb/xhci-plat.h b/include/linux/usb/xhci-plat.h
> new file mode 100644
> index 0000000..58a56ae
> --- /dev/null
> +++ b/include/linux/usb/xhci-plat.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * xhci-plat.h - xHCI host controller driver platform Bus Glue.
> + *
> + * Copyright (C) 2015 Renesas Electronics Corporation
> + */
> +
> +#ifndef _XHCI_PLAT_H
> +#define _XHCI_PLAT_H
> +
> +#include <linux/types.h>
> +#include <linux/usb/hcd.h>

It would be great to remove this include and forward declare struct
usb_hcd instead to avoid include hell. Maybe a followup patch?

> +
> +struct xhci_plat_priv {
> +       const char *firmware_name;
> +       unsigned long long quirks;
> +       int (*plat_setup)(struct usb_hcd *hcd);
> +       void (*plat_start)(struct usb_hcd *hcd);
> +       int (*init_quirk)(struct usb_hcd *hcd);
> +       int (*suspend_quirk)(struct usb_hcd *hcd);
> +       int (*resume_quirk)(struct usb_hcd *hcd);
> +};
> +
> +#endif /* _XHCI_PLAT_H */

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

[Stephen Boyd]

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

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

[Jun Li]

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
>

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

[Pavan Kondeti]

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

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

[Jun Li]

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

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

[Pavan Kondeti]

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

Re: [PATCH 1/2] usb: xhci: refactor quirks and plat private data

[Sandeep Maheswaram]

On 2/16/2022 7:43 AM, Stephen Boyd wrote:
> Quoting Sandeep Maheswaram (2022-02-15 10:24:13)
>> From: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
>>
>> This refactoring allows drivers like dwc3 host glue driver to
>> specify their xhci quirks.
>>
>> Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> Your SoB should be here as well.
okay. Will add in next version.
>
>> diff --git a/include/linux/usb/xhci-plat.h b/include/linux/usb/xhci-plat.h
>> new file mode 100644
>> index 0000000..58a56ae
>> --- /dev/null
>> +++ b/include/linux/usb/xhci-plat.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * xhci-plat.h - xHCI host controller driver platform Bus Glue.
>> + *
>> + * Copyright (C) 2015 Renesas Electronics Corporation
>> + */
>> +
>> +#ifndef _XHCI_PLAT_H
>> +#define _XHCI_PLAT_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/usb/hcd.h>
> It would be great to remove this include and forward declare struct
> usb_hcd instead to avoid include hell. Maybe a followup patch?
okay.  Will do in next version.
>> +
>> +struct xhci_plat_priv {
>> +       const char *firmware_name;
>> +       unsigned long long quirks;
>> +       int (*plat_setup)(struct usb_hcd *hcd);
>> +       void (*plat_start)(struct usb_hcd *hcd);
>> +       int (*init_quirk)(struct usb_hcd *hcd);
>> +       int (*suspend_quirk)(struct usb_hcd *hcd);
>> +       int (*resume_quirk)(struct usb_hcd *hcd);
>> +};
>> +
>> +#endif /* _XHCI_PLAT_H */