* [PATCH v3 0/3] Refactor xhci quirks and plat private data @ 2022-03-24 6:37 Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 1/3] usb: xhci: refactor " Sandeep Maheswaram ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Sandeep Maheswaram @ 2022-03-24 6:37 UTC (permalink / raw) To: Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju Cc: linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap, Sandeep Maheswaram changes in v3: Updated the commit description for PATCH 2/3 and PATCH 3/3. changes in v2: Added a PATCH 2/3 to remove unwanted header inclusion. Fixed minor nitpicks in PATCH 3/3. Pavankumar Kondeti (1): usb: xhci: refactor quirks and plat private data Sandeep Maheswaram (2): usb: xhci: Remove unwanted header inclusion usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT drivers/usb/cdns3/host.c | 2 +- drivers/usb/dwc3/host.c | 13 ++++++++ drivers/usb/host/xhci-plat.c | 3 +- drivers/usb/host/xhci-plat.h | 24 --------------- 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, 115 insertions(+), 81 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 1/3] usb: xhci: refactor quirks and plat private data 2022-03-24 6:37 [PATCH v3 0/3] Refactor xhci quirks and plat private data Sandeep Maheswaram @ 2022-03-24 6:37 ` Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 2/3] usb: xhci: Remove unwanted header inclusion Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT Sandeep Maheswaram 2 siblings, 0 replies; 20+ messages in thread From: Sandeep Maheswaram @ 2022-03-24 6:37 UTC (permalink / raw) To: Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju Cc: linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap, 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> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- drivers/usb/cdns3/host.c | 2 +- drivers/usb/host/xhci-plat.c | 3 +- drivers/usb/host/xhci-plat.h | 24 --------------- drivers/usb/host/xhci-rcar.c | 3 +- drivers/usb/host/xhci.h | 60 ++++-------------------------------- include/linux/usb/xhci-plat.h | 23 ++++++++++++++ include/linux/usb/xhci-quirks.h | 67 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 101 insertions(+), 81 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 649ffd8..601e656 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 1fb149d..0000000 --- a/drivers/usb/host/xhci-plat.h +++ /dev/null @@ -1,24 +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; - 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 aef0258..18d3272 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 473a33c..ae400da 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" @@ -1847,60 +1849,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; @@ -1966,6 +1914,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..906e907 --- /dev/null +++ b/include/linux/usb/xhci-plat.h @@ -0,0 +1,23 @@ +/* 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; + 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/3] usb: xhci: Remove unwanted header inclusion 2022-03-24 6:37 [PATCH v3 0/3] Refactor xhci quirks and plat private data Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 1/3] usb: xhci: refactor " Sandeep Maheswaram @ 2022-03-24 6:37 ` Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT Sandeep Maheswaram 2 siblings, 0 replies; 20+ messages in thread From: Sandeep Maheswaram @ 2022-03-24 6:37 UTC (permalink / raw) To: Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju Cc: linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap, Sandeep Maheswaram The header file linux/usb/hcd.h is already included where we are using xhci-plat.h so remove the header file and forward declare struct usb_hcd to avoid multiple inclusions. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- This patch is to address below comment. https://patchwork.kernel.org/project/linux-arm-msm/patch/1644949454-814-2-git-send-email-quic_c_sanm@quicinc.com/#24739885 include/linux/usb/xhci-plat.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/linux/usb/xhci-plat.h b/include/linux/usb/xhci-plat.h index 906e907..576e400 100644 --- a/include/linux/usb/xhci-plat.h +++ b/include/linux/usb/xhci-plat.h @@ -9,7 +9,8 @@ #define _XHCI_PLAT_H #include <linux/types.h> -#include <linux/usb/hcd.h> + +struct usb_hcd; struct xhci_plat_priv { const char *firmware_name; -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-24 6:37 [PATCH v3 0/3] Refactor xhci quirks and plat private data Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 1/3] usb: xhci: refactor " Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 2/3] usb: xhci: Remove unwanted header inclusion Sandeep Maheswaram @ 2022-03-24 6:37 ` Sandeep Maheswaram 2022-03-24 12:27 ` Heikki Krogerus 2 siblings, 1 reply; 20+ messages in thread From: Sandeep Maheswaram @ 2022-03-24 6:37 UTC (permalink / raw) To: Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju Cc: linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap, Sandeep Maheswaram Currently the phy init is done from dwc3 and also xhci which makes the runtime_usage value 2 for the phy which causes issue during runtime suspend. When we run the below command the runtime_status still shows active. echo auto > /sys/bus/platform/devices/88e3000.phy/power/control dwc3 manages PHY by own DRD driver, so skip the management by HCD core by setting this quirk. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- drivers/usb/dwc3/host.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index eda8719..d4fcf06 100644 --- a/drivers/usb/dwc3/host.c +++ b/drivers/usb/dwc3/host.c @@ -13,6 +13,12 @@ #include <linux/platform_device.h> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) } } + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, + sizeof(xhci_plat_dwc3_xhci)); + 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-24 6:37 ` [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT Sandeep Maheswaram @ 2022-03-24 12:27 ` Heikki Krogerus 2022-03-24 22:36 ` Mathias Nyman 0 siblings, 1 reply; 20+ messages in thread From: Heikki Krogerus @ 2022-03-24 12:27 UTC (permalink / raw) To: Sandeep Maheswaram Cc: Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > Currently the phy init is done from dwc3 and also xhci which makes the > runtime_usage value 2 for the phy which causes issue during runtime > suspend. When we run the below command the runtime_status still shows > active. > echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > > dwc3 manages PHY by own DRD driver, so skip the management by > HCD core by setting this quirk. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > --- > drivers/usb/dwc3/host.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > index eda8719..d4fcf06 100644 > --- a/drivers/usb/dwc3/host.c > +++ b/drivers/usb/dwc3/host.c > @@ -13,6 +13,12 @@ > #include <linux/platform_device.h> > > #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > } > } > > + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > + sizeof(xhci_plat_dwc3_xhci)); > + 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"); I think you should just use device property: diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index eda871973d6cc..dbff7b8ed6d5e 100644 --- a/drivers/usb/dwc3/host.c +++ b/drivers/usb/dwc3/host.c @@ -67,7 +67,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) int dwc3_host_init(struct dwc3 *dwc) { - struct property_entry props[4]; + struct property_entry props[5]; struct platform_device *xhci; int ret, irq; int prop_idx = 0; @@ -114,12 +114,12 @@ int dwc3_host_init(struct dwc3 *dwc) if (DWC3_VER_IS_WITHIN(DWC3, ANY, 300A)) props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped"); - if (prop_idx) { - ret = device_create_managed_software_node(&xhci->dev, props, NULL); - if (ret) { - dev_err(dwc->dev, "failed to add properties to xHCI\n"); - goto err; - } + props[prop_idx++] = PROPERTY_ENTRY_BOOL("skip-phy-init"); + + ret = device_create_managed_software_node(&xhci->dev, props, NULL); + if (ret) { + dev_err(dwc->dev, "failed to add properties to xHCI\n"); + goto err; } ret = platform_device_add(xhci); diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 649ffd861b44e..31ed39d06e29b 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -307,6 +307,9 @@ static int xhci_plat_probe(struct platform_device *pdev) device_property_read_u32(tmpdev, "imod-interval-ns", &xhci->imod_interval); + + if (device_property_read_bool(tmpdev, "skip-phy-init")) + hcd->skip_phy_initialization = 1; } hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0); -- heikki ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-24 12:27 ` Heikki Krogerus @ 2022-03-24 22:36 ` Mathias Nyman 2022-03-25 11:27 ` Heikki Krogerus 0 siblings, 1 reply; 20+ messages in thread From: Mathias Nyman @ 2022-03-24 22:36 UTC (permalink / raw) To: Heikki Krogerus, Sandeep Maheswaram Cc: Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap On 24.3.2022 14.27, Heikki Krogerus wrote: > On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: >> Currently the phy init is done from dwc3 and also xhci which makes the >> runtime_usage value 2 for the phy which causes issue during runtime >> suspend. When we run the below command the runtime_status still shows >> active. >> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control >> >> dwc3 manages PHY by own DRD driver, so skip the management by >> HCD core by setting this quirk. >> >> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >> --- >> drivers/usb/dwc3/host.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >> index eda8719..d4fcf06 100644 >> --- a/drivers/usb/dwc3/host.c >> +++ b/drivers/usb/dwc3/host.c >> @@ -13,6 +13,12 @@ >> #include <linux/platform_device.h> >> >> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) >> } >> } >> >> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, >> + sizeof(xhci_plat_dwc3_xhci)); >> + 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"); > > I think you should just use device property: > This was suggested in an earlier series, but was rejected as it also added the property as a device tree parameter. I think adding more device properties can be messy in the long run, especially if we need to add them for many of the existing xhci quirks. We also end up with a mix where some device properties are listed as device tree parameters, and some not. Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 allow those drivers to easily set any existing xhci quirk, or other possible optional callbacks. cdns3 driver is already doing this, but it includes the full xhci.h header. This series cleans up that a bit so cdns3 will only include xhci quirk bits and platform data structure. On the downside we add a couple xhci related header files to include/linux/usb/ Let me know if you see any other issues I missed with this approach. Thanks Mathias ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-24 22:36 ` Mathias Nyman @ 2022-03-25 11:27 ` Heikki Krogerus 2022-03-25 14:33 ` Mathias Nyman 0 siblings, 1 reply; 20+ messages in thread From: Heikki Krogerus @ 2022-03-25 11:27 UTC (permalink / raw) To: Mathias Nyman Cc: Sandeep Maheswaram, Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > On 24.3.2022 14.27, Heikki Krogerus wrote: > > On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > >> Currently the phy init is done from dwc3 and also xhci which makes the > >> runtime_usage value 2 for the phy which causes issue during runtime > >> suspend. When we run the below command the runtime_status still shows > >> active. > >> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > >> > >> dwc3 manages PHY by own DRD driver, so skip the management by > >> HCD core by setting this quirk. > >> > >> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >> --- > >> drivers/usb/dwc3/host.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >> index eda8719..d4fcf06 100644 > >> --- a/drivers/usb/dwc3/host.c > >> +++ b/drivers/usb/dwc3/host.c > >> @@ -13,6 +13,12 @@ > >> #include <linux/platform_device.h> > >> > >> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > >> } > >> } > >> > >> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > >> + sizeof(xhci_plat_dwc3_xhci)); > >> + 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"); > > > > I think you should just use device property: > > > > This was suggested in an earlier series, but was rejected as it also added > the property as a device tree parameter. > > I think adding more device properties can be messy in the long run, especially if we > need to add them for many of the existing xhci quirks. > We also end up with a mix where some device properties are listed as device tree > parameters, and some not. > > Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > allow those drivers to easily set any existing xhci quirk, or other possible optional > callbacks. > > cdns3 driver is already doing this, but it includes the full xhci.h header. > This series cleans up that a bit so cdns3 will only include xhci quirk bits and > platform data structure. > > On the downside we add a couple xhci related header files to include/linux/usb/ > Let me know if you see any other issues I missed with this approach. The problem here is that these drivers are now coupled together, and that should not be taken lightly. We have a dependency hell in our hands with a lot of drivers, and the culprit is always platform data. Build-in device properties may be messy, but I would still say they are less messy than those quirk flags - you got to admit, they are a mess. The benefit from build-in properties is in any case the fact that they remove the need to couple these drivers together. You can also use something like naming convention if you are worried about confusion between devicetree properties and build-in only properties ("build-in:skip-phy-init" or whatever), and of course require that each of the build-in only property is documented clearly in drivers/usb/host/xhci-plat.c. But this in any case really can not be justification for a platform data blob just so you can avoid using the properties - honestly, it really should to be the other way around. Platform data is in practice always problematic. On top of the driver coupling, it creates maintenance burden, code duplication, etc. Please don't just accept it lightly. I'm telling you, for hacks like this, the build-in device properties is a much much safer bet. thanks, -- heikki ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-25 11:27 ` Heikki Krogerus @ 2022-03-25 14:33 ` Mathias Nyman 2022-03-25 15:38 ` Heikki Krogerus 0 siblings, 1 reply; 20+ messages in thread From: Mathias Nyman @ 2022-03-25 14:33 UTC (permalink / raw) To: Heikki Krogerus Cc: Sandeep Maheswaram, Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap On 25.3.2022 13.27, Heikki Krogerus wrote: > On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: >> On 24.3.2022 14.27, Heikki Krogerus wrote: >>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: >>>> Currently the phy init is done from dwc3 and also xhci which makes the >>>> runtime_usage value 2 for the phy which causes issue during runtime >>>> suspend. When we run the below command the runtime_status still shows >>>> active. >>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control >>>> >>>> dwc3 manages PHY by own DRD driver, so skip the management by >>>> HCD core by setting this quirk. >>>> >>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>>> --- >>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>> index eda8719..d4fcf06 100644 >>>> --- a/drivers/usb/dwc3/host.c >>>> +++ b/drivers/usb/dwc3/host.c >>>> @@ -13,6 +13,12 @@ >>>> #include <linux/platform_device.h> >>>> >>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) >>>> } >>>> } >>>> >>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, >>>> + sizeof(xhci_plat_dwc3_xhci)); >>>> + 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"); >>> >>> I think you should just use device property: >>> >> >> This was suggested in an earlier series, but was rejected as it also added >> the property as a device tree parameter. >> >> I think adding more device properties can be messy in the long run, especially if we >> need to add them for many of the existing xhci quirks. >> We also end up with a mix where some device properties are listed as device tree >> parameters, and some not. >> >> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 >> allow those drivers to easily set any existing xhci quirk, or other possible optional >> callbacks. >> >> cdns3 driver is already doing this, but it includes the full xhci.h header. >> This series cleans up that a bit so cdns3 will only include xhci quirk bits and >> platform data structure. >> >> On the downside we add a couple xhci related header files to include/linux/usb/ >> Let me know if you see any other issues I missed with this approach. > > The problem here is that these drivers are now coupled together, and > that should not be taken lightly. We have a dependency hell in our > hands with a lot of drivers, and the culprit is always platform data. > > Build-in device properties may be messy, but I would still say they > are less messy than those quirk flags - you got to admit, they are a > mess. The benefit from build-in properties is in any case the fact > that they remove the need to couple these drivers together. Agree, quirk bits are messy. Any suggestion that would work with PCI xHCI devices, devicetree, and "pure" platform devices? > > You can also use something like naming convention if you are worried > about confusion between devicetree properties and build-in only > properties ("build-in:skip-phy-init" or whatever), and of course > require that each of the build-in only property is documented clearly > in drivers/usb/host/xhci-plat.c. But this in any case really can not > be justification for a platform data blob just so you can avoid using > the properties - honestly, it really should to be the other way > around. > > Platform data is in practice always problematic. On top of the driver > coupling, it creates maintenance burden, code duplication, etc. Please > don't just accept it lightly. I'm telling you, for hacks like this, the > build-in device properties is a much much safer bet. > Fair enough. Any idea if there's a long term solution for platform devices? Adding some type of "compatibility" id to platform devices in addition to name? Also just noticed cdns3 driver passes a function pointer via platform_data to xhci-plat. This should be cleared up as well. Thanks -Mathias ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-25 14:33 ` Mathias Nyman @ 2022-03-25 15:38 ` Heikki Krogerus 2022-03-29 9:18 ` Sandeep Maheswaram (Temp) 0 siblings, 1 reply; 20+ messages in thread From: Heikki Krogerus @ 2022-03-25 15:38 UTC (permalink / raw) To: Mathias Nyman Cc: Sandeep Maheswaram, Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: > On 25.3.2022 13.27, Heikki Krogerus wrote: > > On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > >> On 24.3.2022 14.27, Heikki Krogerus wrote: > >>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > >>>> Currently the phy init is done from dwc3 and also xhci which makes the > >>>> runtime_usage value 2 for the phy which causes issue during runtime > >>>> suspend. When we run the below command the runtime_status still shows > >>>> active. > >>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > >>>> > >>>> dwc3 manages PHY by own DRD driver, so skip the management by > >>>> HCD core by setting this quirk. > >>>> > >>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >>>> --- > >>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ > >>>> 1 file changed, 13 insertions(+) > >>>> > >>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >>>> index eda8719..d4fcf06 100644 > >>>> --- a/drivers/usb/dwc3/host.c > >>>> +++ b/drivers/usb/dwc3/host.c > >>>> @@ -13,6 +13,12 @@ > >>>> #include <linux/platform_device.h> > >>>> > >>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > >>>> } > >>>> } > >>>> > >>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > >>>> + sizeof(xhci_plat_dwc3_xhci)); > >>>> + 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"); > >>> > >>> I think you should just use device property: > >>> > >> > >> This was suggested in an earlier series, but was rejected as it also added > >> the property as a device tree parameter. > >> > >> I think adding more device properties can be messy in the long run, especially if we > >> need to add them for many of the existing xhci quirks. > >> We also end up with a mix where some device properties are listed as device tree > >> parameters, and some not. > >> > >> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > >> allow those drivers to easily set any existing xhci quirk, or other possible optional > >> callbacks. > >> > >> cdns3 driver is already doing this, but it includes the full xhci.h header. > >> This series cleans up that a bit so cdns3 will only include xhci quirk bits and > >> platform data structure. > >> > >> On the downside we add a couple xhci related header files to include/linux/usb/ > >> Let me know if you see any other issues I missed with this approach. > > > > The problem here is that these drivers are now coupled together, and > > that should not be taken lightly. We have a dependency hell in our > > hands with a lot of drivers, and the culprit is always platform data. > > > > Build-in device properties may be messy, but I would still say they > > are less messy than those quirk flags - you got to admit, they are a > > mess. The benefit from build-in properties is in any case the fact > > that they remove the need to couple these drivers together. > > Agree, quirk bits are messy. Any suggestion that would work with > PCI xHCI devices, devicetree, and "pure" platform devices? I think xHCI driver should always be able to rely on being able to read this kind of information from the fwnode. If there is no actual firmware node (DT or ACPI), or if it's missing some information, the glue driver needs to populate software node for the xHCI. Right now I just want to avoid having to pass the quirks using platform data from drivers such as drivers/usb/cdns3/host.c and drivers/usb/dwc3/host.c to xHCI. One way we could do that is by defining compatibility ID for both of them that we provide using a single device property (like I guess DT does). Then based on that compatibility ID, xhci-plat.c can set the actual "static" quirk flags. That we could already do easily. How would that sound to you? > > You can also use something like naming convention if you are worried > > about confusion between devicetree properties and build-in only > > properties ("build-in:skip-phy-init" or whatever), and of course > > require that each of the build-in only property is documented clearly > > in drivers/usb/host/xhci-plat.c. But this in any case really can not > > be justification for a platform data blob just so you can avoid using > > the properties - honestly, it really should to be the other way > > around. > > > > Platform data is in practice always problematic. On top of the driver > > coupling, it creates maintenance burden, code duplication, etc. Please > > don't just accept it lightly. I'm telling you, for hacks like this, the > > build-in device properties is a much much safer bet. > > > > Fair enough. > > Any idea if there's a long term solution for platform devices? > Adding some type of "compatibility" id to platform devices in addition to name? We don't need to touch the platform devices if, and when, we have always the fwnode. The "compatibility" id should always be possible to pick from the fwnode. > Also just noticed cdns3 driver passes a function pointer via platform_data to > xhci-plat. This should be cleared up as well. Looks like a PM quirk. For that I would propose this: https://lore.kernel.org/linux-acpi/20201029105941.63410-2-heikki.krogerus@linux.intel.com/ So with that you can supply separate PM operations for your software fwnode. When ever the device suspends, the suspend quirk is also executed. xHCI core does not need to know about it. Yeah, I guess I need to finish that series... thanks, -- heikki ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-25 15:38 ` Heikki Krogerus @ 2022-03-29 9:18 ` Sandeep Maheswaram (Temp) 2022-03-29 9:28 ` Pavan Kondeti 2022-03-30 17:47 ` Mathias Nyman 0 siblings, 2 replies; 20+ messages in thread From: Sandeep Maheswaram (Temp) @ 2022-03-29 9:18 UTC (permalink / raw) To: Heikki Krogerus, Mathias Nyman Cc: Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap Hi Mathias,Heikki On 3/25/2022 9:08 PM, Heikki Krogerus wrote: > On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: >> On 25.3.2022 13.27, Heikki Krogerus wrote: >>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: >>>> On 24.3.2022 14.27, Heikki Krogerus wrote: >>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: >>>>>> Currently the phy init is done from dwc3 and also xhci which makes the >>>>>> runtime_usage value 2 for the phy which causes issue during runtime >>>>>> suspend. When we run the below command the runtime_status still shows >>>>>> active. >>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control >>>>>> >>>>>> dwc3 manages PHY by own DRD driver, so skip the management by >>>>>> HCD core by setting this quirk. >>>>>> >>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>>>>> --- >>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ >>>>>> 1 file changed, 13 insertions(+) >>>>>> >>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>>>> index eda8719..d4fcf06 100644 >>>>>> --- a/drivers/usb/dwc3/host.c >>>>>> +++ b/drivers/usb/dwc3/host.c >>>>>> @@ -13,6 +13,12 @@ >>>>>> #include <linux/platform_device.h> >>>>>> >>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) >>>>>> } >>>>>> } >>>>>> >>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, >>>>>> + sizeof(xhci_plat_dwc3_xhci)); >>>>>> + 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"); >>>>> I think you should just use device property: >>>>> >>>> This was suggested in an earlier series, but was rejected as it also added >>>> the property as a device tree parameter. >>>> >>>> I think adding more device properties can be messy in the long run, especially if we >>>> need to add them for many of the existing xhci quirks. >>>> We also end up with a mix where some device properties are listed as device tree >>>> parameters, and some not. >>>> >>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 >>>> allow those drivers to easily set any existing xhci quirk, or other possible optional >>>> callbacks. >>>> >>>> cdns3 driver is already doing this, but it includes the full xhci.h header. >>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and >>>> platform data structure. >>>> >>>> On the downside we add a couple xhci related header files to include/linux/usb/ >>>> Let me know if you see any other issues I missed with this approach. >>> The problem here is that these drivers are now coupled together, and >>> that should not be taken lightly. We have a dependency hell in our >>> hands with a lot of drivers, and the culprit is always platform data. >>> >>> Build-in device properties may be messy, but I would still say they >>> are less messy than those quirk flags - you got to admit, they are a >>> mess. The benefit from build-in properties is in any case the fact >>> that they remove the need to couple these drivers together. >> Agree, quirk bits are messy. Any suggestion that would work with >> PCI xHCI devices, devicetree, and "pure" platform devices? > I think xHCI driver should always be able to rely on being able to > read this kind of information from the fwnode. If there is no actual > firmware node (DT or ACPI), or if it's missing some information, the > glue driver needs to populate software node for the xHCI. > > Right now I just want to avoid having to pass the quirks using > platform data from drivers such as drivers/usb/cdns3/host.c and > drivers/usb/dwc3/host.c to xHCI. > > One way we could do that is by defining compatibility ID for both of > them that we provide using a single device property (like I guess DT > does). Then based on that compatibility ID, xhci-plat.c can set the > actual "static" quirk flags. That we could already do easily. How > would that sound to you? This was my previous patch where I was using device tree property. Should we go ahead with this approach? https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ Any further changes to this ? Regards Sandeep > >>> You can also use something like naming convention if you are worried >>> about confusion between devicetree properties and build-in only >>> properties ("build-in:skip-phy-init" or whatever), and of course >>> require that each of the build-in only property is documented clearly >>> in drivers/usb/host/xhci-plat.c. But this in any case really can not >>> be justification for a platform data blob just so you can avoid using >>> the properties - honestly, it really should to be the other way >>> around. >>> >>> Platform data is in practice always problematic. On top of the driver >>> coupling, it creates maintenance burden, code duplication, etc. Please >>> don't just accept it lightly. I'm telling you, for hacks like this, the >>> build-in device properties is a much much safer bet. >>> >> Fair enough. >> >> Any idea if there's a long term solution for platform devices? >> Adding some type of "compatibility" id to platform devices in addition to name? > We don't need to touch the platform devices if, and when, we have > always the fwnode. The "compatibility" id should always be possible to > pick from the fwnode. > >> Also just noticed cdns3 driver passes a function pointer via platform_data to >> xhci-plat. This should be cleared up as well. > Looks like a PM quirk. For that I would propose this: > https://lore.kernel.org/linux-acpi/20201029105941.63410-2-heikki.krogerus@linux.intel.com/ > > So with that you can supply separate PM operations for your software > fwnode. When ever the device suspends, the suspend quirk is also > executed. xHCI core does not need to know about it. > > Yeah, I guess I need to finish that series... > > thanks, > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-29 9:18 ` Sandeep Maheswaram (Temp) @ 2022-03-29 9:28 ` Pavan Kondeti 2022-03-30 17:47 ` Mathias Nyman 1 sibling, 0 replies; 20+ messages in thread From: Pavan Kondeti @ 2022-03-29 9:28 UTC (permalink / raw) To: Sandeep Maheswaram (Temp) Cc: Heikki Krogerus, Mathias Nyman, Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap On Tue, Mar 29, 2022 at 02:48:05PM +0530, Sandeep Maheswaram (Temp) wrote: > Hi Mathias,Heikki > > On 3/25/2022 9:08 PM, Heikki Krogerus wrote: > >On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: > >>On 25.3.2022 13.27, Heikki Krogerus wrote: > >>>On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > >>>>On 24.3.2022 14.27, Heikki Krogerus wrote: > >>>>>On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > >>>>>>Currently the phy init is done from dwc3 and also xhci which makes the > >>>>>>runtime_usage value 2 for the phy which causes issue during runtime > >>>>>>suspend. When we run the below command the runtime_status still shows > >>>>>>active. > >>>>>>echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > >>>>>> > >>>>>>dwc3 manages PHY by own DRD driver, so skip the management by > >>>>>>HCD core by setting this quirk. > >>>>>> > >>>>>>Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >>>>>>--- > >>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ > >>>>>> 1 file changed, 13 insertions(+) > >>>>>> > >>>>>>diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >>>>>>index eda8719..d4fcf06 100644 > >>>>>>--- a/drivers/usb/dwc3/host.c > >>>>>>+++ b/drivers/usb/dwc3/host.c > >>>>>>@@ -13,6 +13,12 @@ > >>>>>> #include <linux/platform_device.h> > >>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > >>>>>> } > >>>>>> } > >>>>>>+ ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > >>>>>>+ sizeof(xhci_plat_dwc3_xhci)); > >>>>>>+ 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"); > >>>>>I think you should just use device property: > >>>>> > >>>>This was suggested in an earlier series, but was rejected as it also added > >>>>the property as a device tree parameter. > >>>> > >>>>I think adding more device properties can be messy in the long run, especially if we > >>>>need to add them for many of the existing xhci quirks. > >>>>We also end up with a mix where some device properties are listed as device tree > >>>>parameters, and some not. > >>>> > >>>>Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > >>>>allow those drivers to easily set any existing xhci quirk, or other possible optional > >>>>callbacks. > >>>> > >>>>cdns3 driver is already doing this, but it includes the full xhci.h header. > >>>>This series cleans up that a bit so cdns3 will only include xhci quirk bits and > >>>>platform data structure. > >>>> > >>>>On the downside we add a couple xhci related header files to include/linux/usb/ > >>>>Let me know if you see any other issues I missed with this approach. > >>>The problem here is that these drivers are now coupled together, and > >>>that should not be taken lightly. We have a dependency hell in our > >>>hands with a lot of drivers, and the culprit is always platform data. > >>> > >>>Build-in device properties may be messy, but I would still say they > >>>are less messy than those quirk flags - you got to admit, they are a > >>>mess. The benefit from build-in properties is in any case the fact > >>>that they remove the need to couple these drivers together. > >>Agree, quirk bits are messy. Any suggestion that would work with > >>PCI xHCI devices, devicetree, and "pure" platform devices? > >I think xHCI driver should always be able to rely on being able to > >read this kind of information from the fwnode. If there is no actual > >firmware node (DT or ACPI), or if it's missing some information, the > >glue driver needs to populate software node for the xHCI. > > > >Right now I just want to avoid having to pass the quirks using > >platform data from drivers such as drivers/usb/cdns3/host.c and > >drivers/usb/dwc3/host.c to xHCI. > > > >One way we could do that is by defining compatibility ID for both of > >them that we provide using a single device property (like I guess DT > >does). Then based on that compatibility ID, xhci-plat.c can set the > >actual "static" quirk flags. That we could already do easily. How > >would that sound to you? > > This was my previous patch where I was using device tree property. Should we > go ahead with this approach? > > https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > > Any further changes to this ? > Actually the dT idea looks better than the platform data. Earlier, we decided to use if (of_device_is_compatible(dev->parent->of_node, "snps,dwc3")) check but I feel that we should just introduce a device tree param that specify this functionality (skipping phy init). we don't want xhci-plat to know that this is dwc3/xyz device. we expect it to be abstracted. Thanks, Pavan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-29 9:18 ` Sandeep Maheswaram (Temp) 2022-03-29 9:28 ` Pavan Kondeti @ 2022-03-30 17:47 ` Mathias Nyman 2022-03-31 11:16 ` Heikki Krogerus 1 sibling, 1 reply; 20+ messages in thread From: Mathias Nyman @ 2022-03-30 17:47 UTC (permalink / raw) To: Sandeep Maheswaram (Temp), Heikki Krogerus Cc: Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: > Hi Mathias,Heikki > > On 3/25/2022 9:08 PM, Heikki Krogerus wrote: >> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: >>> On 25.3.2022 13.27, Heikki Krogerus wrote: >>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: >>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: >>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: >>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the >>>>>>> runtime_usage value 2 for the phy which causes issue during runtime >>>>>>> suspend. When we run the below command the runtime_status still shows >>>>>>> active. >>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control >>>>>>> >>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by >>>>>>> HCD core by setting this quirk. >>>>>>> >>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>>>>>> --- >>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ >>>>>>> 1 file changed, 13 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>>>>> index eda8719..d4fcf06 100644 >>>>>>> --- a/drivers/usb/dwc3/host.c >>>>>>> +++ b/drivers/usb/dwc3/host.c >>>>>>> @@ -13,6 +13,12 @@ >>>>>>> #include <linux/platform_device.h> >>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) >>>>>>> } >>>>>>> } >>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, >>>>>>> + sizeof(xhci_plat_dwc3_xhci)); >>>>>>> + 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"); >>>>>> I think you should just use device property: >>>>>> >>>>> This was suggested in an earlier series, but was rejected as it also added >>>>> the property as a device tree parameter. >>>>> >>>>> I think adding more device properties can be messy in the long run, especially if we >>>>> need to add them for many of the existing xhci quirks. >>>>> We also end up with a mix where some device properties are listed as device tree >>>>> parameters, and some not. >>>>> >>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 >>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional >>>>> callbacks. >>>>> >>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. >>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and >>>>> platform data structure. >>>>> >>>>> On the downside we add a couple xhci related header files to include/linux/usb/ >>>>> Let me know if you see any other issues I missed with this approach. >>>> The problem here is that these drivers are now coupled together, and >>>> that should not be taken lightly. We have a dependency hell in our >>>> hands with a lot of drivers, and the culprit is always platform data. >>>> >>>> Build-in device properties may be messy, but I would still say they >>>> are less messy than those quirk flags - you got to admit, they are a >>>> mess. The benefit from build-in properties is in any case the fact >>>> that they remove the need to couple these drivers together. >>> Agree, quirk bits are messy. Any suggestion that would work with >>> PCI xHCI devices, devicetree, and "pure" platform devices? >> I think xHCI driver should always be able to rely on being able to >> read this kind of information from the fwnode. If there is no actual >> firmware node (DT or ACPI), or if it's missing some information, the >> glue driver needs to populate software node for the xHCI. >> >> Right now I just want to avoid having to pass the quirks using >> platform data from drivers such as drivers/usb/cdns3/host.c and >> drivers/usb/dwc3/host.c to xHCI. >> >> One way we could do that is by defining compatibility ID for both of >> them that we provide using a single device property (like I guess DT >> does). Then based on that compatibility ID, xhci-plat.c can set the >> actual "static" quirk flags. That we could already do easily. How >> would that sound to you? Sounds good. > > This was my previous patch where I was using device tree property. Should we go ahead with this approach? > > https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > > Any further changes to this ? By dropping the DT part of that series we get a similar built-in device property solution as Heikki initially suggested. How about adding the compatibility ID device property that was just suggested? Then matching the Id in xhci-plat.c against a static table containing Ids and xhci_plat_priv structures, with the needed quirks for dwc3. Thanks -Mathias ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-30 17:47 ` Mathias Nyman @ 2022-03-31 11:16 ` Heikki Krogerus 2022-04-04 8:25 ` Pavan Kondeti 0 siblings, 1 reply; 20+ messages in thread From: Heikki Krogerus @ 2022-03-31 11:16 UTC (permalink / raw) To: Mathias Nyman, Pavan Kondeti Cc: Sandeep Maheswaram (Temp), Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_pkondeti, quic_ppratap On Wed, Mar 30, 2022 at 08:47:34PM +0300, Mathias Nyman wrote: > On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: > > Hi Mathias,Heikki > > > > On 3/25/2022 9:08 PM, Heikki Krogerus wrote: > >> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: > >>> On 25.3.2022 13.27, Heikki Krogerus wrote: > >>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > >>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: > >>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > >>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the > >>>>>>> runtime_usage value 2 for the phy which causes issue during runtime > >>>>>>> suspend. When we run the below command the runtime_status still shows > >>>>>>> active. > >>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > >>>>>>> > >>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by > >>>>>>> HCD core by setting this quirk. > >>>>>>> > >>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >>>>>>> --- > >>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ > >>>>>>> 1 file changed, 13 insertions(+) > >>>>>>> > >>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >>>>>>> index eda8719..d4fcf06 100644 > >>>>>>> --- a/drivers/usb/dwc3/host.c > >>>>>>> +++ b/drivers/usb/dwc3/host.c > >>>>>>> @@ -13,6 +13,12 @@ > >>>>>>> #include <linux/platform_device.h> > >>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > >>>>>>> } > >>>>>>> } > >>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > >>>>>>> + sizeof(xhci_plat_dwc3_xhci)); > >>>>>>> + 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"); > >>>>>> I think you should just use device property: > >>>>>> > >>>>> This was suggested in an earlier series, but was rejected as it also added > >>>>> the property as a device tree parameter. > >>>>> > >>>>> I think adding more device properties can be messy in the long run, especially if we > >>>>> need to add them for many of the existing xhci quirks. > >>>>> We also end up with a mix where some device properties are listed as device tree > >>>>> parameters, and some not. > >>>>> > >>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > >>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional > >>>>> callbacks. > >>>>> > >>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. > >>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and > >>>>> platform data structure. > >>>>> > >>>>> On the downside we add a couple xhci related header files to include/linux/usb/ > >>>>> Let me know if you see any other issues I missed with this approach. > >>>> The problem here is that these drivers are now coupled together, and > >>>> that should not be taken lightly. We have a dependency hell in our > >>>> hands with a lot of drivers, and the culprit is always platform data. > >>>> > >>>> Build-in device properties may be messy, but I would still say they > >>>> are less messy than those quirk flags - you got to admit, they are a > >>>> mess. The benefit from build-in properties is in any case the fact > >>>> that they remove the need to couple these drivers together. > >>> Agree, quirk bits are messy. Any suggestion that would work with > >>> PCI xHCI devices, devicetree, and "pure" platform devices? > >> I think xHCI driver should always be able to rely on being able to > >> read this kind of information from the fwnode. If there is no actual > >> firmware node (DT or ACPI), or if it's missing some information, the > >> glue driver needs to populate software node for the xHCI. > >> > >> Right now I just want to avoid having to pass the quirks using > >> platform data from drivers such as drivers/usb/cdns3/host.c and > >> drivers/usb/dwc3/host.c to xHCI. > >> > >> One way we could do that is by defining compatibility ID for both of > >> them that we provide using a single device property (like I guess DT > >> does). Then based on that compatibility ID, xhci-plat.c can set the > >> actual "static" quirk flags. That we could already do easily. How > >> would that sound to you? > > Sounds good. > > > > > This was my previous patch where I was using device tree property. Should we go ahead with this approach? > > > > https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > > > > Any further changes to this ? > > By dropping the DT part of that series we get a similar built-in device property > solution as Heikki initially suggested. > > How about adding the compatibility ID device property that was just suggested? > Then matching the Id in xhci-plat.c against a static table containing Ids and > xhci_plat_priv structures, with the needed quirks for dwc3. There was a comment from Pavan. Is it still possible to get this detail from DT? I guess that would still be ideal, right? I have another question. Can't we now just assume that if the sysdev is the parent (or grandparent), then the phy initialization should always be skipped? In that case we could just do something like this: diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 649ffd861b44e..1018b33488046 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -212,8 +212,12 @@ static int xhci_plat_probe(struct platform_device *pdev) #endif } - if (!sysdev) + if (sysdev) { + if (sysdev != &pdev->dev) + hcd->skip_phy_initialization = 1; + } else { sysdev = &pdev->dev; + } if (WARN_ON(!sysdev->dma_mask)) /* Platform did not initialize dma_mask */ I did not go through all the drivers that carefully, so I may have missed something, but it looks like the only drivers that can have the sysdev as the parent or grandparent are cdns3 and dwc3. thanks, -- heikki ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-03-31 11:16 ` Heikki Krogerus @ 2022-04-04 8:25 ` Pavan Kondeti 2022-04-06 6:25 ` Pavan Kondeti 0 siblings, 1 reply; 20+ messages in thread From: Pavan Kondeti @ 2022-04-04 8:25 UTC (permalink / raw) To: Heikki Krogerus Cc: Mathias Nyman, Pavan Kondeti, Sandeep Maheswaram (Temp), Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap, quic_kriskura Hi Heikki, On Thu, Mar 31, 2022 at 02:16:53PM +0300, Heikki Krogerus wrote: > On Wed, Mar 30, 2022 at 08:47:34PM +0300, Mathias Nyman wrote: > > On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: > > > Hi Mathias,Heikki > > > > > > On 3/25/2022 9:08 PM, Heikki Krogerus wrote: > > >> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: > > >>> On 25.3.2022 13.27, Heikki Krogerus wrote: > > >>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > > >>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: > > >>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > > >>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the > > >>>>>>> runtime_usage value 2 for the phy which causes issue during runtime > > >>>>>>> suspend. When we run the below command the runtime_status still shows > > >>>>>>> active. > > >>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > > >>>>>>> > > >>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by > > >>>>>>> HCD core by setting this quirk. > > >>>>>>> > > >>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > >>>>>>> --- > > >>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ > > >>>>>>> 1 file changed, 13 insertions(+) > > >>>>>>> > > >>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > > >>>>>>> index eda8719..d4fcf06 100644 > > >>>>>>> --- a/drivers/usb/dwc3/host.c > > >>>>>>> +++ b/drivers/usb/dwc3/host.c > > >>>>>>> @@ -13,6 +13,12 @@ > > >>>>>>> #include <linux/platform_device.h> > > >>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > > >>>>>>> } > > >>>>>>> } > > >>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > > >>>>>>> + sizeof(xhci_plat_dwc3_xhci)); > > >>>>>>> + 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"); > > >>>>>> I think you should just use device property: > > >>>>>> > > >>>>> This was suggested in an earlier series, but was rejected as it also added > > >>>>> the property as a device tree parameter. > > >>>>> > > >>>>> I think adding more device properties can be messy in the long run, especially if we > > >>>>> need to add them for many of the existing xhci quirks. > > >>>>> We also end up with a mix where some device properties are listed as device tree > > >>>>> parameters, and some not. > > >>>>> > > >>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > > >>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional > > >>>>> callbacks. > > >>>>> > > >>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. > > >>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and > > >>>>> platform data structure. > > >>>>> > > >>>>> On the downside we add a couple xhci related header files to include/linux/usb/ > > >>>>> Let me know if you see any other issues I missed with this approach. > > >>>> The problem here is that these drivers are now coupled together, and > > >>>> that should not be taken lightly. We have a dependency hell in our > > >>>> hands with a lot of drivers, and the culprit is always platform data. > > >>>> > > >>>> Build-in device properties may be messy, but I would still say they > > >>>> are less messy than those quirk flags - you got to admit, they are a > > >>>> mess. The benefit from build-in properties is in any case the fact > > >>>> that they remove the need to couple these drivers together. > > >>> Agree, quirk bits are messy. Any suggestion that would work with > > >>> PCI xHCI devices, devicetree, and "pure" platform devices? > > >> I think xHCI driver should always be able to rely on being able to > > >> read this kind of information from the fwnode. If there is no actual > > >> firmware node (DT or ACPI), or if it's missing some information, the > > >> glue driver needs to populate software node for the xHCI. > > >> > > >> Right now I just want to avoid having to pass the quirks using > > >> platform data from drivers such as drivers/usb/cdns3/host.c and > > >> drivers/usb/dwc3/host.c to xHCI. > > >> > > >> One way we could do that is by defining compatibility ID for both of > > >> them that we provide using a single device property (like I guess DT > > >> does). Then based on that compatibility ID, xhci-plat.c can set the > > >> actual "static" quirk flags. That we could already do easily. How > > >> would that sound to you? > > > > Sounds good. > > > > > > > > This was my previous patch where I was using device tree property. Should we go ahead with this approach? > > > > > > https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > > > > > > Any further changes to this ? > > > > By dropping the DT part of that series we get a similar built-in device property > > solution as Heikki initially suggested. > > > > How about adding the compatibility ID device property that was just suggested? > > Then matching the Id in xhci-plat.c against a static table containing Ids and > > xhci_plat_priv structures, with the needed quirks for dwc3. > > There was a comment from Pavan. Is it still possible to get this > detail from DT? > I guess that would still be ideal, right? > I was suggesting if we can have device tree param like the patch sandeep pointed out. How would adding a compatible index to usb_xhci_of_match[] would work actually? I ask this because, dwc3/host.c creates platform device and it is not associated with any of_node, so of_driver_match_device() called from platform bus match method does not work. one way to achieve this would be by matching against sysdev. Something like below. Is it acceptible? diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 649ffd8..bd5d055 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -126,6 +126,10 @@ static const struct xhci_plat_priv xhci_plat_brcm = { .quirks = XHCI_RESET_ON_RESUME, }; +static const struct xhci_plat_priv xhci_plat_dwc3 = { + .quirks = XHCI_SKIP_PHY_INIT, +}; + static const struct of_device_id usb_xhci_of_match[] = { { .compatible = "generic-xhci", @@ -167,6 +171,9 @@ static const struct of_device_id usb_xhci_of_match[] = { }, { .compatible = "brcm,bcm7445-xhci", .data = &xhci_plat_brcm, + }, { + .compatible = "snps,dwc3", + .data = &xhci_plat_dwc3, }, {}, }; @@ -274,6 +281,15 @@ static int xhci_plat_probe(struct platform_device *pdev) else priv_match = dev_get_platdata(&pdev->dev); + /* allow private data mapping with the sysdev compatible */ + if (!priv_match) { + struct of_device_id *match; + + match = of_match_device(usb_xhci_of_match, sysdev); + if (match) + priv_match = match->data; + } + if (priv_match) { priv = hcd_to_xhci_priv(hcd); /* Just copy data for now */ > I have another question. Can't we now just assume that if the sysdev > is the parent (or grandparent), then the phy initialization should > always be skipped? In that case we could just do something like this: > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index 649ffd861b44e..1018b33488046 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -212,8 +212,12 @@ static int xhci_plat_probe(struct platform_device *pdev) > #endif > } > > - if (!sysdev) > + if (sysdev) { > + if (sysdev != &pdev->dev) > + hcd->skip_phy_initialization = 1; > + } else { > sysdev = &pdev->dev; > + } > > if (WARN_ON(!sysdev->dma_mask)) > /* Platform did not initialize dma_mask */ > > > I did not go through all the drivers that carefully, so I may have > missed something, but it looks like the only drivers that can have the > sysdev as the parent or grandparent are cdns3 and dwc3. > I cross checked and these are two drivers that are creating xhci-plat device. So this patch would definitely work. However I am not sure in future if any device created via device tree would want to use this feature. For now, it looks good. It Mathias, Do you see any problem with this approach? Thanks, Pavan ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-04-04 8:25 ` Pavan Kondeti @ 2022-04-06 6:25 ` Pavan Kondeti 2022-04-06 10:27 ` Heikki Krogerus 2022-04-06 10:52 ` Mathias Nyman 0 siblings, 2 replies; 20+ messages in thread From: Pavan Kondeti @ 2022-04-06 6:25 UTC (permalink / raw) To: Heikki Krogerus, Mathias Nyman Cc: Heikki Krogerus, Mathias Nyman, Sandeep Maheswaram (Temp), Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap, quic_kriskura, quic_pkondeti Hi Heikki/Mathias, On Mon, Apr 04, 2022 at 01:55:16PM +0530, Pavan Kondeti wrote: > Hi Heikki, > > On Thu, Mar 31, 2022 at 02:16:53PM +0300, Heikki Krogerus wrote: > > On Wed, Mar 30, 2022 at 08:47:34PM +0300, Mathias Nyman wrote: > > > On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: > > > > Hi Mathias,Heikki > > > > > > > > On 3/25/2022 9:08 PM, Heikki Krogerus wrote: > > > >> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: > > > >>> On 25.3.2022 13.27, Heikki Krogerus wrote: > > > >>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > > > >>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: > > > >>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > > > >>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the > > > >>>>>>> runtime_usage value 2 for the phy which causes issue during runtime > > > >>>>>>> suspend. When we run the below command the runtime_status still shows > > > >>>>>>> active. > > > >>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > > > >>>>>>> > > > >>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by > > > >>>>>>> HCD core by setting this quirk. > > > >>>>>>> > > > >>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > >>>>>>> --- > > > >>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ > > > >>>>>>> 1 file changed, 13 insertions(+) > > > >>>>>>> > > > >>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > > > >>>>>>> index eda8719..d4fcf06 100644 > > > >>>>>>> --- a/drivers/usb/dwc3/host.c > > > >>>>>>> +++ b/drivers/usb/dwc3/host.c > > > >>>>>>> @@ -13,6 +13,12 @@ > > > >>>>>>> #include <linux/platform_device.h> > > > >>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > > > >>>>>>> } > > > >>>>>>> } > > > >>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > > > >>>>>>> + sizeof(xhci_plat_dwc3_xhci)); > > > >>>>>>> + 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"); > > > >>>>>> I think you should just use device property: > > > >>>>>> > > > >>>>> This was suggested in an earlier series, but was rejected as it also added > > > >>>>> the property as a device tree parameter. > > > >>>>> > > > >>>>> I think adding more device properties can be messy in the long run, especially if we > > > >>>>> need to add them for many of the existing xhci quirks. > > > >>>>> We also end up with a mix where some device properties are listed as device tree > > > >>>>> parameters, and some not. > > > >>>>> > > > >>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > > > >>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional > > > >>>>> callbacks. > > > >>>>> > > > >>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. > > > >>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and > > > >>>>> platform data structure. > > > >>>>> > > > >>>>> On the downside we add a couple xhci related header files to include/linux/usb/ > > > >>>>> Let me know if you see any other issues I missed with this approach. > > > >>>> The problem here is that these drivers are now coupled together, and > > > >>>> that should not be taken lightly. We have a dependency hell in our > > > >>>> hands with a lot of drivers, and the culprit is always platform data. > > > >>>> > > > >>>> Build-in device properties may be messy, but I would still say they > > > >>>> are less messy than those quirk flags - you got to admit, they are a > > > >>>> mess. The benefit from build-in properties is in any case the fact > > > >>>> that they remove the need to couple these drivers together. > > > >>> Agree, quirk bits are messy. Any suggestion that would work with > > > >>> PCI xHCI devices, devicetree, and "pure" platform devices? > > > >> I think xHCI driver should always be able to rely on being able to > > > >> read this kind of information from the fwnode. If there is no actual > > > >> firmware node (DT or ACPI), or if it's missing some information, the > > > >> glue driver needs to populate software node for the xHCI. > > > >> > > > >> Right now I just want to avoid having to pass the quirks using > > > >> platform data from drivers such as drivers/usb/cdns3/host.c and > > > >> drivers/usb/dwc3/host.c to xHCI. > > > >> > > > >> One way we could do that is by defining compatibility ID for both of > > > >> them that we provide using a single device property (like I guess DT > > > >> does). Then based on that compatibility ID, xhci-plat.c can set the > > > >> actual "static" quirk flags. That we could already do easily. How > > > >> would that sound to you? > > > > > > Sounds good. > > > > > > > > > > > This was my previous patch where I was using device tree property. Should we go ahead with this approach? > > > > > > > > https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > > > > > > > > Any further changes to this ? > > > > > > By dropping the DT part of that series we get a similar built-in device property > > > solution as Heikki initially suggested. > > > > > > How about adding the compatibility ID device property that was just suggested? > > > Then matching the Id in xhci-plat.c against a static table containing Ids and > > > xhci_plat_priv structures, with the needed quirks for dwc3. > > > > There was a comment from Pavan. Is it still possible to get this > > detail from DT? > > I guess that would still be ideal, right? > > > I was suggesting if we can have device tree param like the patch sandeep > pointed out. > > How would adding a compatible index to usb_xhci_of_match[] would work > actually? I ask this because, dwc3/host.c creates platform device and > it is not associated with any of_node, so of_driver_match_device() called > from platform bus match method does not work. one way to achieve this would > be by matching against sysdev. Something like below. Is it acceptible? > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index 649ffd8..bd5d055 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -126,6 +126,10 @@ static const struct xhci_plat_priv xhci_plat_brcm = { > .quirks = XHCI_RESET_ON_RESUME, > }; > > +static const struct xhci_plat_priv xhci_plat_dwc3 = { > + .quirks = XHCI_SKIP_PHY_INIT, > +}; > + > static const struct of_device_id usb_xhci_of_match[] = { > { > .compatible = "generic-xhci", > @@ -167,6 +171,9 @@ static const struct of_device_id usb_xhci_of_match[] = { > }, { > .compatible = "brcm,bcm7445-xhci", > .data = &xhci_plat_brcm, > + }, { > + .compatible = "snps,dwc3", > + .data = &xhci_plat_dwc3, > }, > {}, > }; > @@ -274,6 +281,15 @@ static int xhci_plat_probe(struct platform_device *pdev) > else > priv_match = dev_get_platdata(&pdev->dev); > > + /* allow private data mapping with the sysdev compatible */ > + if (!priv_match) { > + struct of_device_id *match; > + > + match = of_match_device(usb_xhci_of_match, sysdev); > + if (match) > + priv_match = match->data; > + } > + > if (priv_match) { > priv = hcd_to_xhci_priv(hcd); > /* Just copy data for now */ > > > I have another question. Can't we now just assume that if the sysdev > > is the parent (or grandparent), then the phy initialization should > > always be skipped? In that case we could just do something like this: > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > index 649ffd861b44e..1018b33488046 100644 > > --- a/drivers/usb/host/xhci-plat.c > > +++ b/drivers/usb/host/xhci-plat.c > > @@ -212,8 +212,12 @@ static int xhci_plat_probe(struct platform_device *pdev) > > #endif > > } > > > > - if (!sysdev) > > + if (sysdev) { > > + if (sysdev != &pdev->dev) > > + hcd->skip_phy_initialization = 1; > > + } else { > > sysdev = &pdev->dev; > > + } > > > > if (WARN_ON(!sysdev->dma_mask)) > > /* Platform did not initialize dma_mask */ > > > > > > I did not go through all the drivers that carefully, so I may have > > missed something, but it looks like the only drivers that can have the > > sysdev as the parent or grandparent are cdns3 and dwc3. > > > I cross checked and these are two drivers that are creating xhci-plat device. > So this patch would definitely work. However I am not sure in future if any > device created via device tree would want to use this feature. For now, > it looks good. It Mathias, Do you see any problem with this approach? > Can you please provide your suggestions on this? We have discussed about 3 approaches here other than the whole platform data refactoring done. (1) Introduce a new dT property and expect dwc3/host.c to set this property to skip the phy initialization. (2) Allow platform private data match based on the sysdev. The diff I sent in the previous email. (3) Heikki's suggestion of relying on the fact that the users of phy skip init can be tested with sysdev != &pdev->dev check. Thanks, Pavan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-04-06 6:25 ` Pavan Kondeti @ 2022-04-06 10:27 ` Heikki Krogerus 2022-04-06 10:52 ` Mathias Nyman 1 sibling, 0 replies; 20+ messages in thread From: Heikki Krogerus @ 2022-04-06 10:27 UTC (permalink / raw) To: Pavan Kondeti Cc: Mathias Nyman, Sandeep Maheswaram (Temp), Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap, quic_kriskura On Wed, Apr 06, 2022 at 11:55:43AM +0530, Pavan Kondeti wrote: > Hi Heikki/Mathias, > > On Mon, Apr 04, 2022 at 01:55:16PM +0530, Pavan Kondeti wrote: > > Hi Heikki, > > > > On Thu, Mar 31, 2022 at 02:16:53PM +0300, Heikki Krogerus wrote: > > > On Wed, Mar 30, 2022 at 08:47:34PM +0300, Mathias Nyman wrote: > > > > On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: > > > > > Hi Mathias,Heikki > > > > > > > > > > On 3/25/2022 9:08 PM, Heikki Krogerus wrote: > > > > >> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: > > > > >>> On 25.3.2022 13.27, Heikki Krogerus wrote: > > > > >>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > > > > >>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: > > > > >>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > > > > >>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the > > > > >>>>>>> runtime_usage value 2 for the phy which causes issue during runtime > > > > >>>>>>> suspend. When we run the below command the runtime_status still shows > > > > >>>>>>> active. > > > > >>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > > > > >>>>>>> > > > > >>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by > > > > >>>>>>> HCD core by setting this quirk. > > > > >>>>>>> > > > > >>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > > >>>>>>> --- > > > > >>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ > > > > >>>>>>> 1 file changed, 13 insertions(+) > > > > >>>>>>> > > > > >>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > > > > >>>>>>> index eda8719..d4fcf06 100644 > > > > >>>>>>> --- a/drivers/usb/dwc3/host.c > > > > >>>>>>> +++ b/drivers/usb/dwc3/host.c > > > > >>>>>>> @@ -13,6 +13,12 @@ > > > > >>>>>>> #include <linux/platform_device.h> > > > > >>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > > > > >>>>>>> } > > > > >>>>>>> } > > > > >>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > > > > >>>>>>> + sizeof(xhci_plat_dwc3_xhci)); > > > > >>>>>>> + 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"); > > > > >>>>>> I think you should just use device property: > > > > >>>>>> > > > > >>>>> This was suggested in an earlier series, but was rejected as it also added > > > > >>>>> the property as a device tree parameter. > > > > >>>>> > > > > >>>>> I think adding more device properties can be messy in the long run, especially if we > > > > >>>>> need to add them for many of the existing xhci quirks. > > > > >>>>> We also end up with a mix where some device properties are listed as device tree > > > > >>>>> parameters, and some not. > > > > >>>>> > > > > >>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > > > > >>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional > > > > >>>>> callbacks. > > > > >>>>> > > > > >>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. > > > > >>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and > > > > >>>>> platform data structure. > > > > >>>>> > > > > >>>>> On the downside we add a couple xhci related header files to include/linux/usb/ > > > > >>>>> Let me know if you see any other issues I missed with this approach. > > > > >>>> The problem here is that these drivers are now coupled together, and > > > > >>>> that should not be taken lightly. We have a dependency hell in our > > > > >>>> hands with a lot of drivers, and the culprit is always platform data. > > > > >>>> > > > > >>>> Build-in device properties may be messy, but I would still say they > > > > >>>> are less messy than those quirk flags - you got to admit, they are a > > > > >>>> mess. The benefit from build-in properties is in any case the fact > > > > >>>> that they remove the need to couple these drivers together. > > > > >>> Agree, quirk bits are messy. Any suggestion that would work with > > > > >>> PCI xHCI devices, devicetree, and "pure" platform devices? > > > > >> I think xHCI driver should always be able to rely on being able to > > > > >> read this kind of information from the fwnode. If there is no actual > > > > >> firmware node (DT or ACPI), or if it's missing some information, the > > > > >> glue driver needs to populate software node for the xHCI. > > > > >> > > > > >> Right now I just want to avoid having to pass the quirks using > > > > >> platform data from drivers such as drivers/usb/cdns3/host.c and > > > > >> drivers/usb/dwc3/host.c to xHCI. > > > > >> > > > > >> One way we could do that is by defining compatibility ID for both of > > > > >> them that we provide using a single device property (like I guess DT > > > > >> does). Then based on that compatibility ID, xhci-plat.c can set the > > > > >> actual "static" quirk flags. That we could already do easily. How > > > > >> would that sound to you? > > > > > > > > Sounds good. > > > > > > > > > > > > > > This was my previous patch where I was using device tree property. Should we go ahead with this approach? > > > > > > > > > > https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > > > > > > > > > > Any further changes to this ? > > > > > > > > By dropping the DT part of that series we get a similar built-in device property > > > > solution as Heikki initially suggested. > > > > > > > > How about adding the compatibility ID device property that was just suggested? > > > > Then matching the Id in xhci-plat.c against a static table containing Ids and > > > > xhci_plat_priv structures, with the needed quirks for dwc3. > > > > > > There was a comment from Pavan. Is it still possible to get this > > > detail from DT? > > > I guess that would still be ideal, right? > > > > > I was suggesting if we can have device tree param like the patch sandeep > > pointed out. > > > > How would adding a compatible index to usb_xhci_of_match[] would work > > actually? I ask this because, dwc3/host.c creates platform device and > > it is not associated with any of_node, so of_driver_match_device() called > > from platform bus match method does not work. one way to achieve this would > > be by matching against sysdev. Something like below. Is it acceptible? > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > index 649ffd8..bd5d055 100644 > > --- a/drivers/usb/host/xhci-plat.c > > +++ b/drivers/usb/host/xhci-plat.c > > @@ -126,6 +126,10 @@ static const struct xhci_plat_priv xhci_plat_brcm = { > > .quirks = XHCI_RESET_ON_RESUME, > > }; > > > > +static const struct xhci_plat_priv xhci_plat_dwc3 = { > > + .quirks = XHCI_SKIP_PHY_INIT, > > +}; > > + > > static const struct of_device_id usb_xhci_of_match[] = { > > { > > .compatible = "generic-xhci", > > @@ -167,6 +171,9 @@ static const struct of_device_id usb_xhci_of_match[] = { > > }, { > > .compatible = "brcm,bcm7445-xhci", > > .data = &xhci_plat_brcm, > > + }, { > > + .compatible = "snps,dwc3", > > + .data = &xhci_plat_dwc3, > > }, > > {}, > > }; > > @@ -274,6 +281,15 @@ static int xhci_plat_probe(struct platform_device *pdev) > > else > > priv_match = dev_get_platdata(&pdev->dev); > > > > + /* allow private data mapping with the sysdev compatible */ > > + if (!priv_match) { > > + struct of_device_id *match; > > + > > + match = of_match_device(usb_xhci_of_match, sysdev); > > + if (match) > > + priv_match = match->data; > > + } > > + > > if (priv_match) { > > priv = hcd_to_xhci_priv(hcd); > > /* Just copy data for now */ > > > > > I have another question. Can't we now just assume that if the sysdev > > > is the parent (or grandparent), then the phy initialization should > > > always be skipped? In that case we could just do something like this: > > > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > > index 649ffd861b44e..1018b33488046 100644 > > > --- a/drivers/usb/host/xhci-plat.c > > > +++ b/drivers/usb/host/xhci-plat.c > > > @@ -212,8 +212,12 @@ static int xhci_plat_probe(struct platform_device *pdev) > > > #endif > > > } > > > > > > - if (!sysdev) > > > + if (sysdev) { > > > + if (sysdev != &pdev->dev) > > > + hcd->skip_phy_initialization = 1; > > > + } else { > > > sysdev = &pdev->dev; > > > + } > > > > > > if (WARN_ON(!sysdev->dma_mask)) > > > /* Platform did not initialize dma_mask */ > > > > > > > > > I did not go through all the drivers that carefully, so I may have > > > missed something, but it looks like the only drivers that can have the > > > sysdev as the parent or grandparent are cdns3 and dwc3. > > > > > I cross checked and these are two drivers that are creating xhci-plat device. > > So this patch would definitely work. However I am not sure in future if any > > device created via device tree would want to use this feature. For now, > > it looks good. It Mathias, Do you see any problem with this approach? > > > > Can you please provide your suggestions on this? We have discussed about > 3 approaches here other than the whole platform data refactoring done. > > (1) Introduce a new dT property and expect dwc3/host.c to set this property > to skip the phy initialization. > (2) Allow platform private data match based on the sysdev. The diff I sent > in the previous email. > (3) Heikki's suggestion of relying on the fact that the users of phy skip > init can be tested with sysdev != &pdev->dev check. My vote is for option 3. That would allow us to sort this out properly later. I will in any case look at these drivers when I send the software node PM operations patch series. thanks, -- heikki ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-04-06 6:25 ` Pavan Kondeti 2022-04-06 10:27 ` Heikki Krogerus @ 2022-04-06 10:52 ` Mathias Nyman 2022-04-06 11:01 ` Pavan Kondeti 1 sibling, 1 reply; 20+ messages in thread From: Mathias Nyman @ 2022-04-06 10:52 UTC (permalink / raw) To: Pavan Kondeti, Heikki Krogerus Cc: Sandeep Maheswaram (Temp), Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap, quic_kriskura Hi Sorry about the delayed response. On 6.4.2022 9.25, Pavan Kondeti wrote: > Hi Heikki/Mathias, > > On Mon, Apr 04, 2022 at 01:55:16PM +0530, Pavan Kondeti wrote: >> Hi Heikki, >> >> On Thu, Mar 31, 2022 at 02:16:53PM +0300, Heikki Krogerus wrote: >>> On Wed, Mar 30, 2022 at 08:47:34PM +0300, Mathias Nyman wrote: >>>> On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: >>>>> Hi Mathias,Heikki >>>>> >>>>> On 3/25/2022 9:08 PM, Heikki Krogerus wrote: >>>>>> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: >>>>>>> On 25.3.2022 13.27, Heikki Krogerus wrote: >>>>>>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: >>>>>>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: >>>>>>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: >>>>>>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the >>>>>>>>>>> runtime_usage value 2 for the phy which causes issue during runtime >>>>>>>>>>> suspend. When we run the below command the runtime_status still shows >>>>>>>>>>> active. >>>>>>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control >>>>>>>>>>> >>>>>>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by >>>>>>>>>>> HCD core by setting this quirk. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>>>>>>>>>> --- >>>>>>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ >>>>>>>>>>> 1 file changed, 13 insertions(+) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>>>>>>>>> index eda8719..d4fcf06 100644 >>>>>>>>>>> --- a/drivers/usb/dwc3/host.c >>>>>>>>>>> +++ b/drivers/usb/dwc3/host.c >>>>>>>>>>> @@ -13,6 +13,12 @@ >>>>>>>>>>> #include <linux/platform_device.h> >>>>>>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, >>>>>>>>>>> + sizeof(xhci_plat_dwc3_xhci)); >>>>>>>>>>> + 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"); >>>>>>>>>> I think you should just use device property: >>>>>>>>>> >>>>>>>>> This was suggested in an earlier series, but was rejected as it also added >>>>>>>>> the property as a device tree parameter. >>>>>>>>> >>>>>>>>> I think adding more device properties can be messy in the long run, especially if we >>>>>>>>> need to add them for many of the existing xhci quirks. >>>>>>>>> We also end up with a mix where some device properties are listed as device tree >>>>>>>>> parameters, and some not. >>>>>>>>> >>>>>>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 >>>>>>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional >>>>>>>>> callbacks. >>>>>>>>> >>>>>>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. >>>>>>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and >>>>>>>>> platform data structure. >>>>>>>>> >>>>>>>>> On the downside we add a couple xhci related header files to include/linux/usb/ >>>>>>>>> Let me know if you see any other issues I missed with this approach. >>>>>>>> The problem here is that these drivers are now coupled together, and >>>>>>>> that should not be taken lightly. We have a dependency hell in our >>>>>>>> hands with a lot of drivers, and the culprit is always platform data. >>>>>>>> >>>>>>>> Build-in device properties may be messy, but I would still say they >>>>>>>> are less messy than those quirk flags - you got to admit, they are a >>>>>>>> mess. The benefit from build-in properties is in any case the fact >>>>>>>> that they remove the need to couple these drivers together. >>>>>>> Agree, quirk bits are messy. Any suggestion that would work with >>>>>>> PCI xHCI devices, devicetree, and "pure" platform devices? >>>>>> I think xHCI driver should always be able to rely on being able to >>>>>> read this kind of information from the fwnode. If there is no actual >>>>>> firmware node (DT or ACPI), or if it's missing some information, the >>>>>> glue driver needs to populate software node for the xHCI. >>>>>> >>>>>> Right now I just want to avoid having to pass the quirks using >>>>>> platform data from drivers such as drivers/usb/cdns3/host.c and >>>>>> drivers/usb/dwc3/host.c to xHCI. >>>>>> >>>>>> One way we could do that is by defining compatibility ID for both of >>>>>> them that we provide using a single device property (like I guess DT >>>>>> does). Then based on that compatibility ID, xhci-plat.c can set the >>>>>> actual "static" quirk flags. That we could already do easily. How >>>>>> would that sound to you? >>>> >>>> Sounds good. >>>> >>>>> >>>>> This was my previous patch where I was using device tree property. Should we go ahead with this approach? >>>>> >>>>> https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ >>>>> >>>>> Any further changes to this ? >>>> >>>> By dropping the DT part of that series we get a similar built-in device property >>>> solution as Heikki initially suggested. >>>> >>>> How about adding the compatibility ID device property that was just suggested? >>>> Then matching the Id in xhci-plat.c against a static table containing Ids and >>>> xhci_plat_priv structures, with the needed quirks for dwc3. >>> >>> There was a comment from Pavan. Is it still possible to get this >>> detail from DT? >>> I guess that would still be ideal, right? >>> >> I was suggesting if we can have device tree param like the patch sandeep >> pointed out. >> >> How would adding a compatible index to usb_xhci_of_match[] would work >> actually? I ask this because, dwc3/host.c creates platform device and >> it is not associated with any of_node, so of_driver_match_device() called >> from platform bus match method does not work. one way to achieve this would >> be by matching against sysdev. Something like below. Is it acceptible? >> >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >> index 649ffd8..bd5d055 100644 >> --- a/drivers/usb/host/xhci-plat.c >> +++ b/drivers/usb/host/xhci-plat.c >> @@ -126,6 +126,10 @@ static const struct xhci_plat_priv xhci_plat_brcm = { >> .quirks = XHCI_RESET_ON_RESUME, >> }; >> >> +static const struct xhci_plat_priv xhci_plat_dwc3 = { >> + .quirks = XHCI_SKIP_PHY_INIT, >> +}; >> + >> static const struct of_device_id usb_xhci_of_match[] = { >> { >> .compatible = "generic-xhci", >> @@ -167,6 +171,9 @@ static const struct of_device_id usb_xhci_of_match[] = { >> }, { >> .compatible = "brcm,bcm7445-xhci", >> .data = &xhci_plat_brcm, >> + }, { >> + .compatible = "snps,dwc3", >> + .data = &xhci_plat_dwc3, >> }, Isn't there a risk that xhci-plat now binds to the parent dwc3 device? competing with the similar of_match_table entry created in drivers/usb/dwc3/core.c >> {}, >> }; >> @@ -274,6 +281,15 @@ static int xhci_plat_probe(struct platform_device *pdev) >> else >> priv_match = dev_get_platdata(&pdev->dev); >> >> + /* allow private data mapping with the sysdev compatible */ >> + if (!priv_match) { >> + struct of_device_id *match; >> + >> + match = of_match_device(usb_xhci_of_match, sysdev); >> + if (match) >> + priv_match = match->data; >> + } >> + >> if (priv_match) { >> priv = hcd_to_xhci_priv(hcd); >> /* Just copy data for now */ >> >>> I have another question. Can't we now just assume that if the sysdev >>> is the parent (or grandparent), then the phy initialization should >>> always be skipped? In that case we could just do something like this: >>> >>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >>> index 649ffd861b44e..1018b33488046 100644 >>> --- a/drivers/usb/host/xhci-plat.c >>> +++ b/drivers/usb/host/xhci-plat.c >>> @@ -212,8 +212,12 @@ static int xhci_plat_probe(struct platform_device *pdev) >>> #endif >>> } >>> >>> - if (!sysdev) >>> + if (sysdev) { >>> + if (sysdev != &pdev->dev) >>> + hcd->skip_phy_initialization = 1; >>> + } else { >>> sysdev = &pdev->dev; >>> + } >>> >>> if (WARN_ON(!sysdev->dma_mask)) >>> /* Platform did not initialize dma_mask */ >>> >>> >>> I did not go through all the drivers that carefully, so I may have >>> missed something, but it looks like the only drivers that can have the >>> sysdev as the parent or grandparent are cdns3 and dwc3. >>> >> I cross checked and these are two drivers that are creating xhci-plat device. >> So this patch would definitely work. However I am not sure in future if any >> device created via device tree would want to use this feature. For now, >> it looks good. It Mathias, Do you see any problem with this approach? >> Would work for now but seems like a risk to assume this would hold for all future xhci platform devices. > > Can you please provide your suggestions on this? We have discussed about > 3 approaches here other than the whole platform data refactoring done. > > (1) Introduce a new dT property and expect dwc3/host.c to set this property > to skip the phy initialization. Adding one more device property to swnode in dwc3/host.c starts to look like the best option for now even if it didn't appeal initially. The place creating the xhci platform device should have best info on what properties are needed for the platform device. So this would be just like Heikki's first suggestion, or Sandeep's patches 2/3 and 3/3 in: https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ but without the devicetree binding documentation part. > (2) Allow platform private data match based on the sysdev. The diff I sent > in the previous email -Possible risk of binding parent dwc3 to incorrect driver (xhci-plat), haven't looked at details. -Not working with ACPI dwc3 devices. Basic idea was similar to what I was thinking, but not matching with sysdev and misusing the of of_device_id table. Instead create a device property similar to "compatible". But looks like we would need to reinvent the wheel and create a some kind of swnode_device_id table, and custom match functions. So option 1 looks better to get this case fixed > (3) Heikki's suggestion of relying on the fact that the users of phy skip > init can be tested with sysdev != &pdev->dev check. Looks like a risky assumption for future, but could work as temporary solution So I think option 1 looks best, option 3 possible I also realize I don't know details of of_node, fwnode, swnode and device properties so all comments from Heikki and other who know more about this are welcome. Thanks -Mathias ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-04-06 10:52 ` Mathias Nyman @ 2022-04-06 11:01 ` Pavan Kondeti 2022-04-06 12:18 ` Mathias Nyman 0 siblings, 1 reply; 20+ messages in thread From: Pavan Kondeti @ 2022-04-06 11:01 UTC (permalink / raw) To: Mathias Nyman Cc: Pavan Kondeti, Heikki Krogerus, Sandeep Maheswaram (Temp), Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap, quic_kriskura Hi Mathias, On Wed, Apr 06, 2022 at 01:52:56PM +0300, Mathias Nyman wrote: > Hi > > Sorry about the delayed response. > > > On 6.4.2022 9.25, Pavan Kondeti wrote: > > Hi Heikki/Mathias, > > > > On Mon, Apr 04, 2022 at 01:55:16PM +0530, Pavan Kondeti wrote: > >> Hi Heikki, > >> > >> On Thu, Mar 31, 2022 at 02:16:53PM +0300, Heikki Krogerus wrote: > >>> On Wed, Mar 30, 2022 at 08:47:34PM +0300, Mathias Nyman wrote: > >>>> On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: > >>>>> Hi Mathias,Heikki > >>>>> > >>>>> On 3/25/2022 9:08 PM, Heikki Krogerus wrote: > >>>>>> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: > >>>>>>> On 25.3.2022 13.27, Heikki Krogerus wrote: > >>>>>>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > >>>>>>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: > >>>>>>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > >>>>>>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the > >>>>>>>>>>> runtime_usage value 2 for the phy which causes issue during runtime > >>>>>>>>>>> suspend. When we run the below command the runtime_status still shows > >>>>>>>>>>> active. > >>>>>>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > >>>>>>>>>>> > >>>>>>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by > >>>>>>>>>>> HCD core by setting this quirk. > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >>>>>>>>>>> --- > >>>>>>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ > >>>>>>>>>>> 1 file changed, 13 insertions(+) > >>>>>>>>>>> > >>>>>>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >>>>>>>>>>> index eda8719..d4fcf06 100644 > >>>>>>>>>>> --- a/drivers/usb/dwc3/host.c > >>>>>>>>>>> +++ b/drivers/usb/dwc3/host.c > >>>>>>>>>>> @@ -13,6 +13,12 @@ > >>>>>>>>>>> #include <linux/platform_device.h> > >>>>>>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > >>>>>>>>>>> } > >>>>>>>>>>> } > >>>>>>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > >>>>>>>>>>> + sizeof(xhci_plat_dwc3_xhci)); > >>>>>>>>>>> + 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"); > >>>>>>>>>> I think you should just use device property: > >>>>>>>>>> > >>>>>>>>> This was suggested in an earlier series, but was rejected as it also added > >>>>>>>>> the property as a device tree parameter. > >>>>>>>>> > >>>>>>>>> I think adding more device properties can be messy in the long run, especially if we > >>>>>>>>> need to add them for many of the existing xhci quirks. > >>>>>>>>> We also end up with a mix where some device properties are listed as device tree > >>>>>>>>> parameters, and some not. > >>>>>>>>> > >>>>>>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > >>>>>>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional > >>>>>>>>> callbacks. > >>>>>>>>> > >>>>>>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. > >>>>>>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and > >>>>>>>>> platform data structure. > >>>>>>>>> > >>>>>>>>> On the downside we add a couple xhci related header files to include/linux/usb/ > >>>>>>>>> Let me know if you see any other issues I missed with this approach. > >>>>>>>> The problem here is that these drivers are now coupled together, and > >>>>>>>> that should not be taken lightly. We have a dependency hell in our > >>>>>>>> hands with a lot of drivers, and the culprit is always platform data. > >>>>>>>> > >>>>>>>> Build-in device properties may be messy, but I would still say they > >>>>>>>> are less messy than those quirk flags - you got to admit, they are a > >>>>>>>> mess. The benefit from build-in properties is in any case the fact > >>>>>>>> that they remove the need to couple these drivers together. > >>>>>>> Agree, quirk bits are messy. Any suggestion that would work with > >>>>>>> PCI xHCI devices, devicetree, and "pure" platform devices? > >>>>>> I think xHCI driver should always be able to rely on being able to > >>>>>> read this kind of information from the fwnode. If there is no actual > >>>>>> firmware node (DT or ACPI), or if it's missing some information, the > >>>>>> glue driver needs to populate software node for the xHCI. > >>>>>> > >>>>>> Right now I just want to avoid having to pass the quirks using > >>>>>> platform data from drivers such as drivers/usb/cdns3/host.c and > >>>>>> drivers/usb/dwc3/host.c to xHCI. > >>>>>> > >>>>>> One way we could do that is by defining compatibility ID for both of > >>>>>> them that we provide using a single device property (like I guess DT > >>>>>> does). Then based on that compatibility ID, xhci-plat.c can set the > >>>>>> actual "static" quirk flags. That we could already do easily. How > >>>>>> would that sound to you? > >>>> > >>>> Sounds good. > >>>> > >>>>> > >>>>> This was my previous patch where I was using device tree property. Should we go ahead with this approach? > >>>>> > >>>>> https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > >>>>> > >>>>> Any further changes to this ? > >>>> > >>>> By dropping the DT part of that series we get a similar built-in device property > >>>> solution as Heikki initially suggested. > >>>> > >>>> How about adding the compatibility ID device property that was just suggested? > >>>> Then matching the Id in xhci-plat.c against a static table containing Ids and > >>>> xhci_plat_priv structures, with the needed quirks for dwc3. > >>> > >>> There was a comment from Pavan. Is it still possible to get this > >>> detail from DT? > >>> I guess that would still be ideal, right? > >>> > >> I was suggesting if we can have device tree param like the patch sandeep > >> pointed out. > >> > >> How would adding a compatible index to usb_xhci_of_match[] would work > >> actually? I ask this because, dwc3/host.c creates platform device and > >> it is not associated with any of_node, so of_driver_match_device() called > >> from platform bus match method does not work. one way to achieve this would > >> be by matching against sysdev. Something like below. Is it acceptible? > >> > >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > >> index 649ffd8..bd5d055 100644 > >> --- a/drivers/usb/host/xhci-plat.c > >> +++ b/drivers/usb/host/xhci-plat.c > >> @@ -126,6 +126,10 @@ static const struct xhci_plat_priv xhci_plat_brcm = { > >> .quirks = XHCI_RESET_ON_RESUME, > >> }; > >> > >> +static const struct xhci_plat_priv xhci_plat_dwc3 = { > >> + .quirks = XHCI_SKIP_PHY_INIT, > >> +}; > >> + > >> static const struct of_device_id usb_xhci_of_match[] = { > >> { > >> .compatible = "generic-xhci", > >> @@ -167,6 +171,9 @@ static const struct of_device_id usb_xhci_of_match[] = { > >> }, { > >> .compatible = "brcm,bcm7445-xhci", > >> .data = &xhci_plat_brcm, > >> + }, { > >> + .compatible = "snps,dwc3", > >> + .data = &xhci_plat_dwc3, > >> }, > > Isn't there a risk that xhci-plat now binds to the parent dwc3 device? > competing with the similar of_match_table entry created in drivers/usb/dwc3/core.c Sill of me. Yes, it does not work. Thanks for pointing it out. > > >> {}, > >> }; > >> @@ -274,6 +281,15 @@ static int xhci_plat_probe(struct platform_device *pdev) > >> else > >> priv_match = dev_get_platdata(&pdev->dev); > >> > >> + /* allow private data mapping with the sysdev compatible */ > >> + if (!priv_match) { > >> + struct of_device_id *match; > >> + > >> + match = of_match_device(usb_xhci_of_match, sysdev); > >> + if (match) > >> + priv_match = match->data; > >> + } > >> + > >> if (priv_match) { > >> priv = hcd_to_xhci_priv(hcd); > >> /* Just copy data for now */ > >> > >>> I have another question. Can't we now just assume that if the sysdev > >>> is the parent (or grandparent), then the phy initialization should > >>> always be skipped? In that case we could just do something like this: > >>> > >>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > >>> index 649ffd861b44e..1018b33488046 100644 > >>> --- a/drivers/usb/host/xhci-plat.c > >>> +++ b/drivers/usb/host/xhci-plat.c > >>> @@ -212,8 +212,12 @@ static int xhci_plat_probe(struct platform_device *pdev) > >>> #endif > >>> } > >>> > >>> - if (!sysdev) > >>> + if (sysdev) { > >>> + if (sysdev != &pdev->dev) > >>> + hcd->skip_phy_initialization = 1; > >>> + } else { > >>> sysdev = &pdev->dev; > >>> + } > >>> > >>> if (WARN_ON(!sysdev->dma_mask)) > >>> /* Platform did not initialize dma_mask */ > >>> > >>> > >>> I did not go through all the drivers that carefully, so I may have > >>> missed something, but it looks like the only drivers that can have the > >>> sysdev as the parent or grandparent are cdns3 and dwc3. > >>> > >> I cross checked and these are two drivers that are creating xhci-plat device. > >> So this patch would definitely work. However I am not sure in future if any > >> device created via device tree would want to use this feature. For now, > >> it looks good. It Mathias, Do you see any problem with this approach? > >> > > Would work for now but seems like a risk to assume this would hold for all future > xhci platform devices. > Agree that it may break in future for other drivers. > > > > Can you please provide your suggestions on this? We have discussed about > > 3 approaches here other than the whole platform data refactoring done. > > > > (1) Introduce a new dT property and expect dwc3/host.c to set this property > > to skip the phy initialization. > > Adding one more device property to swnode in dwc3/host.c starts > to look like the best option for now even if it didn't appeal initially. > > The place creating the xhci platform device should have best info on what properties > are needed for the platform device. > > So this would be just like Heikki's first suggestion, or Sandeep's patches 2/3 and 3/3 in: > https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > but without the devicetree binding documentation part. > Why do you say devicetree binding doc is not needed in this case? Possible that xhci-plat's device can be coming from dT and this param is passed to skip initialization of PHY. Thanks, Pavan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-04-06 11:01 ` Pavan Kondeti @ 2022-04-06 12:18 ` Mathias Nyman 2022-04-06 15:34 ` Pavan Kondeti 0 siblings, 1 reply; 20+ messages in thread From: Mathias Nyman @ 2022-04-06 12:18 UTC (permalink / raw) To: Pavan Kondeti Cc: Heikki Krogerus, Sandeep Maheswaram (Temp), Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap, quic_kriskura On 6.4.2022 14.01, Pavan Kondeti wrote: > Hi Mathias, > > On Wed, Apr 06, 2022 at 01:52:56PM +0300, Mathias Nyman wrote: >> Hi >> >> Sorry about the delayed response. >> >> >> On 6.4.2022 9.25, Pavan Kondeti wrote: >>> Hi Heikki/Mathias, >>> >>> On Mon, Apr 04, 2022 at 01:55:16PM +0530, Pavan Kondeti wrote: >>>> Hi Heikki, >>>> >>>> On Thu, Mar 31, 2022 at 02:16:53PM +0300, Heikki Krogerus wrote: >>>>> On Wed, Mar 30, 2022 at 08:47:34PM +0300, Mathias Nyman wrote: >>>>>> On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: >>>>>>> Hi Mathias,Heikki >>>>>>> >>>>>>> On 3/25/2022 9:08 PM, Heikki Krogerus wrote: >>>>>>>> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: >>>>>>>>> On 25.3.2022 13.27, Heikki Krogerus wrote: >>>>>>>>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: >>>>>>>>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: >>>>>>>>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: >>>>>>>>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the >>>>>>>>>>>>> runtime_usage value 2 for the phy which causes issue during runtime >>>>>>>>>>>>> suspend. When we run the below command the runtime_status still shows >>>>>>>>>>>>> active. >>>>>>>>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control >>>>>>>>>>>>> >>>>>>>>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by >>>>>>>>>>>>> HCD core by setting this quirk. >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>>>>>>>>>>>> --- >>>>>>>>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ >>>>>>>>>>>>> 1 file changed, 13 insertions(+) >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>>>>>>>>>>> index eda8719..d4fcf06 100644 >>>>>>>>>>>>> --- a/drivers/usb/dwc3/host.c >>>>>>>>>>>>> +++ b/drivers/usb/dwc3/host.c >>>>>>>>>>>>> @@ -13,6 +13,12 @@ >>>>>>>>>>>>> #include <linux/platform_device.h> >>>>>>>>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) >>>>>>>>>>>>> } >>>>>>>>>>>>> } >>>>>>>>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, >>>>>>>>>>>>> + sizeof(xhci_plat_dwc3_xhci)); >>>>>>>>>>>>> + 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"); >>>>>>>>>>>> I think you should just use device property: >>>>>>>>>>>> >>>>>>>>>>> This was suggested in an earlier series, but was rejected as it also added >>>>>>>>>>> the property as a device tree parameter. >>>>>>>>>>> >>>>>>>>>>> I think adding more device properties can be messy in the long run, especially if we >>>>>>>>>>> need to add them for many of the existing xhci quirks. >>>>>>>>>>> We also end up with a mix where some device properties are listed as device tree >>>>>>>>>>> parameters, and some not. >>>>>>>>>>> >>>>>>>>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 >>>>>>>>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional >>>>>>>>>>> callbacks. >>>>>>>>>>> >>>>>>>>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. >>>>>>>>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and >>>>>>>>>>> platform data structure. >>>>>>>>>>> >>>>>>>>>>> On the downside we add a couple xhci related header files to include/linux/usb/ >>>>>>>>>>> Let me know if you see any other issues I missed with this approach. >>>>>>>>>> The problem here is that these drivers are now coupled together, and >>>>>>>>>> that should not be taken lightly. We have a dependency hell in our >>>>>>>>>> hands with a lot of drivers, and the culprit is always platform data. >>>>>>>>>> >>>>>>>>>> Build-in device properties may be messy, but I would still say they >>>>>>>>>> are less messy than those quirk flags - you got to admit, they are a >>>>>>>>>> mess. The benefit from build-in properties is in any case the fact >>>>>>>>>> that they remove the need to couple these drivers together. >>>>>>>>> Agree, quirk bits are messy. Any suggestion that would work with >>>>>>>>> PCI xHCI devices, devicetree, and "pure" platform devices? >>>>>>>> I think xHCI driver should always be able to rely on being able to >>>>>>>> read this kind of information from the fwnode. If there is no actual >>>>>>>> firmware node (DT or ACPI), or if it's missing some information, the >>>>>>>> glue driver needs to populate software node for the xHCI. >>>>>>>> >>>>>>>> Right now I just want to avoid having to pass the quirks using >>>>>>>> platform data from drivers such as drivers/usb/cdns3/host.c and >>>>>>>> drivers/usb/dwc3/host.c to xHCI. >>>>>>>> >>>>>>>> One way we could do that is by defining compatibility ID for both of >>>>>>>> them that we provide using a single device property (like I guess DT >>>>>>>> does). Then based on that compatibility ID, xhci-plat.c can set the >>>>>>>> actual "static" quirk flags. That we could already do easily. How >>>>>>>> would that sound to you? >>>>>> >>>>>> Sounds good. >>>>>> >>>>>>> >>>>>>> This was my previous patch where I was using device tree property. Should we go ahead with this approach? >>>>>>> >>>>>>> https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ >>>>>>> >>>>>>> Any further changes to this ? >>>>>> >>>>>> By dropping the DT part of that series we get a similar built-in device property >>>>>> solution as Heikki initially suggested. >>>>>> >>>>>> How about adding the compatibility ID device property that was just suggested? >>>>>> Then matching the Id in xhci-plat.c against a static table containing Ids and >>>>>> xhci_plat_priv structures, with the needed quirks for dwc3. >>>>> >>>>> There was a comment from Pavan. Is it still possible to get this >>>>> detail from DT? >>>>> I guess that would still be ideal, right? >>>>> >>>> I was suggesting if we can have device tree param like the patch sandeep >>>> pointed out. >>>> >>>> How would adding a compatible index to usb_xhci_of_match[] would work >>>> actually? I ask this because, dwc3/host.c creates platform device and >>>> it is not associated with any of_node, so of_driver_match_device() called >>>> from platform bus match method does not work. one way to achieve this would >>>> be by matching against sysdev. Something like below. Is it acceptible? >>>> >>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >>>> index 649ffd8..bd5d055 100644 >>>> --- a/drivers/usb/host/xhci-plat.c >>>> +++ b/drivers/usb/host/xhci-plat.c >>>> @@ -126,6 +126,10 @@ static const struct xhci_plat_priv xhci_plat_brcm = { >>>> .quirks = XHCI_RESET_ON_RESUME, >>>> }; >>>> >>>> +static const struct xhci_plat_priv xhci_plat_dwc3 = { >>>> + .quirks = XHCI_SKIP_PHY_INIT, >>>> +}; >>>> + >>>> static const struct of_device_id usb_xhci_of_match[] = { >>>> { >>>> .compatible = "generic-xhci", >>>> @@ -167,6 +171,9 @@ static const struct of_device_id usb_xhci_of_match[] = { >>>> }, { >>>> .compatible = "brcm,bcm7445-xhci", >>>> .data = &xhci_plat_brcm, >>>> + }, { >>>> + .compatible = "snps,dwc3", >>>> + .data = &xhci_plat_dwc3, >>>> }, >> >> Isn't there a risk that xhci-plat now binds to the parent dwc3 device? >> competing with the similar of_match_table entry created in drivers/usb/dwc3/core.c > > Sill of me. Yes, it does not work. Thanks for pointing it out. > >> >>>> {}, >>>> }; >>>> @@ -274,6 +281,15 @@ static int xhci_plat_probe(struct platform_device *pdev) >>>> else >>>> priv_match = dev_get_platdata(&pdev->dev); >>>> >>>> + /* allow private data mapping with the sysdev compatible */ >>>> + if (!priv_match) { >>>> + struct of_device_id *match; >>>> + >>>> + match = of_match_device(usb_xhci_of_match, sysdev); >>>> + if (match) >>>> + priv_match = match->data; >>>> + } >>>> + >>>> if (priv_match) { >>>> priv = hcd_to_xhci_priv(hcd); >>>> /* Just copy data for now */ >>>> >>>>> I have another question. Can't we now just assume that if the sysdev >>>>> is the parent (or grandparent), then the phy initialization should >>>>> always be skipped? In that case we could just do something like this: >>>>> >>>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >>>>> index 649ffd861b44e..1018b33488046 100644 >>>>> --- a/drivers/usb/host/xhci-plat.c >>>>> +++ b/drivers/usb/host/xhci-plat.c >>>>> @@ -212,8 +212,12 @@ static int xhci_plat_probe(struct platform_device *pdev) >>>>> #endif >>>>> } >>>>> >>>>> - if (!sysdev) >>>>> + if (sysdev) { >>>>> + if (sysdev != &pdev->dev) >>>>> + hcd->skip_phy_initialization = 1; >>>>> + } else { >>>>> sysdev = &pdev->dev; >>>>> + } >>>>> >>>>> if (WARN_ON(!sysdev->dma_mask)) >>>>> /* Platform did not initialize dma_mask */ >>>>> >>>>> >>>>> I did not go through all the drivers that carefully, so I may have >>>>> missed something, but it looks like the only drivers that can have the >>>>> sysdev as the parent or grandparent are cdns3 and dwc3. >>>>> >>>> I cross checked and these are two drivers that are creating xhci-plat device. >>>> So this patch would definitely work. However I am not sure in future if any >>>> device created via device tree would want to use this feature. For now, >>>> it looks good. It Mathias, Do you see any problem with this approach? >>>> >> >> Would work for now but seems like a risk to assume this would hold for all future >> xhci platform devices. >> > Agree that it may break in future for other drivers. > >>> >>> Can you please provide your suggestions on this? We have discussed about >>> 3 approaches here other than the whole platform data refactoring done. >>> >>> (1) Introduce a new dT property and expect dwc3/host.c to set this property >>> to skip the phy initialization. >> >> Adding one more device property to swnode in dwc3/host.c starts >> to look like the best option for now even if it didn't appeal initially. >> >> The place creating the xhci platform device should have best info on what properties >> are needed for the platform device. >> >> So this would be just like Heikki's first suggestion, or Sandeep's patches 2/3 and 3/3 in: >> https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ >> but without the devicetree binding documentation part. >> > Why do you say devicetree binding doc is not needed in this case? Possible > that xhci-plat's device can be coming from dT and this param is passed to > skip initialization of PHY. Adding it is fine by me. It was rejected earlier by Rob Herring But we can live without it. In dwc3 and cdns3 "pure" platform case we add the device property when creating the device. In dT case we should have a compatible entry, and we can add a matching entry in usb_xhci_of_match[] which sets the needed quirk. Thanks -Mathias ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT 2022-04-06 12:18 ` Mathias Nyman @ 2022-04-06 15:34 ` Pavan Kondeti 0 siblings, 0 replies; 20+ messages in thread From: Pavan Kondeti @ 2022-04-06 15:34 UTC (permalink / raw) To: Mathias Nyman Cc: Pavan Kondeti, Heikki Krogerus, Sandeep Maheswaram (Temp), Greg Kroah-Hartman, Felipe Balbi, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Mathias Nyman, Peter Chen, Pawel Laszczak, Roger Quadros, Aswath Govindraju, linux-arm-msm, linux-usb, linux-kernel, quic_ppratap, quic_kriskura Hi Mathias, On Wed, Apr 06, 2022 at 03:18:17PM +0300, Mathias Nyman wrote: > On 6.4.2022 14.01, Pavan Kondeti wrote: > > Hi Mathias, > > > > On Wed, Apr 06, 2022 at 01:52:56PM +0300, Mathias Nyman wrote: > >> Hi > >> > >> Sorry about the delayed response. > >> > >> > >> On 6.4.2022 9.25, Pavan Kondeti wrote: > >>> Hi Heikki/Mathias, > >>> > >>> On Mon, Apr 04, 2022 at 01:55:16PM +0530, Pavan Kondeti wrote: > >>>> Hi Heikki, > >>>> > >>>> On Thu, Mar 31, 2022 at 02:16:53PM +0300, Heikki Krogerus wrote: > >>>>> On Wed, Mar 30, 2022 at 08:47:34PM +0300, Mathias Nyman wrote: > >>>>>> On 29.3.2022 12.18, Sandeep Maheswaram (Temp) wrote: > >>>>>>> Hi Mathias,Heikki > >>>>>>> > >>>>>>> On 3/25/2022 9:08 PM, Heikki Krogerus wrote: > >>>>>>>> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote: > >>>>>>>>> On 25.3.2022 13.27, Heikki Krogerus wrote: > >>>>>>>>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote: > >>>>>>>>>>> On 24.3.2022 14.27, Heikki Krogerus wrote: > >>>>>>>>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote: > >>>>>>>>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the > >>>>>>>>>>>>> runtime_usage value 2 for the phy which causes issue during runtime > >>>>>>>>>>>>> suspend. When we run the below command the runtime_status still shows > >>>>>>>>>>>>> active. > >>>>>>>>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control > >>>>>>>>>>>>> > >>>>>>>>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by > >>>>>>>>>>>>> HCD core by setting this quirk. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >>>>>>>>>>>>> --- > >>>>>>>>>>>>> drivers/usb/dwc3/host.c | 13 +++++++++++++ > >>>>>>>>>>>>> 1 file changed, 13 insertions(+) > >>>>>>>>>>>>> > >>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >>>>>>>>>>>>> index eda8719..d4fcf06 100644 > >>>>>>>>>>>>> --- a/drivers/usb/dwc3/host.c > >>>>>>>>>>>>> +++ b/drivers/usb/dwc3/host.c > >>>>>>>>>>>>> @@ -13,6 +13,12 @@ > >>>>>>>>>>>>> #include <linux/platform_device.h> > >>>>>>>>>>>>> #include "core.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 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc) > >>>>>>>>>>>>> } > >>>>>>>>>>>>> } > >>>>>>>>>>>>> + ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci, > >>>>>>>>>>>>> + sizeof(xhci_plat_dwc3_xhci)); > >>>>>>>>>>>>> + 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"); > >>>>>>>>>>>> I think you should just use device property: > >>>>>>>>>>>> > >>>>>>>>>>> This was suggested in an earlier series, but was rejected as it also added > >>>>>>>>>>> the property as a device tree parameter. > >>>>>>>>>>> > >>>>>>>>>>> I think adding more device properties can be messy in the long run, especially if we > >>>>>>>>>>> need to add them for many of the existing xhci quirks. > >>>>>>>>>>> We also end up with a mix where some device properties are listed as device tree > >>>>>>>>>>> parameters, and some not. > >>>>>>>>>>> > >>>>>>>>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3 > >>>>>>>>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional > >>>>>>>>>>> callbacks. > >>>>>>>>>>> > >>>>>>>>>>> cdns3 driver is already doing this, but it includes the full xhci.h header. > >>>>>>>>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and > >>>>>>>>>>> platform data structure. > >>>>>>>>>>> > >>>>>>>>>>> On the downside we add a couple xhci related header files to include/linux/usb/ > >>>>>>>>>>> Let me know if you see any other issues I missed with this approach. > >>>>>>>>>> The problem here is that these drivers are now coupled together, and > >>>>>>>>>> that should not be taken lightly. We have a dependency hell in our > >>>>>>>>>> hands with a lot of drivers, and the culprit is always platform data. > >>>>>>>>>> > >>>>>>>>>> Build-in device properties may be messy, but I would still say they > >>>>>>>>>> are less messy than those quirk flags - you got to admit, they are a > >>>>>>>>>> mess. The benefit from build-in properties is in any case the fact > >>>>>>>>>> that they remove the need to couple these drivers together. > >>>>>>>>> Agree, quirk bits are messy. Any suggestion that would work with > >>>>>>>>> PCI xHCI devices, devicetree, and "pure" platform devices? > >>>>>>>> I think xHCI driver should always be able to rely on being able to > >>>>>>>> read this kind of information from the fwnode. If there is no actual > >>>>>>>> firmware node (DT or ACPI), or if it's missing some information, the > >>>>>>>> glue driver needs to populate software node for the xHCI. > >>>>>>>> > >>>>>>>> Right now I just want to avoid having to pass the quirks using > >>>>>>>> platform data from drivers such as drivers/usb/cdns3/host.c and > >>>>>>>> drivers/usb/dwc3/host.c to xHCI. > >>>>>>>> > >>>>>>>> One way we could do that is by defining compatibility ID for both of > >>>>>>>> them that we provide using a single device property (like I guess DT > >>>>>>>> does). Then based on that compatibility ID, xhci-plat.c can set the > >>>>>>>> actual "static" quirk flags. That we could already do easily. How > >>>>>>>> would that sound to you? > >>>>>> > >>>>>> Sounds good. > >>>>>> > >>>>>>> > >>>>>>> This was my previous patch where I was using device tree property. Should we go ahead with this approach? > >>>>>>> > >>>>>>> https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > >>>>>>> > >>>>>>> Any further changes to this ? > >>>>>> > >>>>>> By dropping the DT part of that series we get a similar built-in device property > >>>>>> solution as Heikki initially suggested. > >>>>>> > >>>>>> How about adding the compatibility ID device property that was just suggested? > >>>>>> Then matching the Id in xhci-plat.c against a static table containing Ids and > >>>>>> xhci_plat_priv structures, with the needed quirks for dwc3. > >>>>> > >>>>> There was a comment from Pavan. Is it still possible to get this > >>>>> detail from DT? > >>>>> I guess that would still be ideal, right? > >>>>> > >>>> I was suggesting if we can have device tree param like the patch sandeep > >>>> pointed out. > >>>> > >>>> How would adding a compatible index to usb_xhci_of_match[] would work > >>>> actually? I ask this because, dwc3/host.c creates platform device and > >>>> it is not associated with any of_node, so of_driver_match_device() called > >>>> from platform bus match method does not work. one way to achieve this would > >>>> be by matching against sysdev. Something like below. Is it acceptible? > >>>> > >>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > >>>> index 649ffd8..bd5d055 100644 > >>>> --- a/drivers/usb/host/xhci-plat.c > >>>> +++ b/drivers/usb/host/xhci-plat.c > >>>> @@ -126,6 +126,10 @@ static const struct xhci_plat_priv xhci_plat_brcm = { > >>>> .quirks = XHCI_RESET_ON_RESUME, > >>>> }; > >>>> > >>>> +static const struct xhci_plat_priv xhci_plat_dwc3 = { > >>>> + .quirks = XHCI_SKIP_PHY_INIT, > >>>> +}; > >>>> + > >>>> static const struct of_device_id usb_xhci_of_match[] = { > >>>> { > >>>> .compatible = "generic-xhci", > >>>> @@ -167,6 +171,9 @@ static const struct of_device_id usb_xhci_of_match[] = { > >>>> }, { > >>>> .compatible = "brcm,bcm7445-xhci", > >>>> .data = &xhci_plat_brcm, > >>>> + }, { > >>>> + .compatible = "snps,dwc3", > >>>> + .data = &xhci_plat_dwc3, > >>>> }, > >> > >> Isn't there a risk that xhci-plat now binds to the parent dwc3 device? > >> competing with the similar of_match_table entry created in drivers/usb/dwc3/core.c > > > > Sill of me. Yes, it does not work. Thanks for pointing it out. > > > >> > >>>> {}, > >>>> }; > >>>> @@ -274,6 +281,15 @@ static int xhci_plat_probe(struct platform_device *pdev) > >>>> else > >>>> priv_match = dev_get_platdata(&pdev->dev); > >>>> > >>>> + /* allow private data mapping with the sysdev compatible */ > >>>> + if (!priv_match) { > >>>> + struct of_device_id *match; > >>>> + > >>>> + match = of_match_device(usb_xhci_of_match, sysdev); > >>>> + if (match) > >>>> + priv_match = match->data; > >>>> + } > >>>> + > >>>> if (priv_match) { > >>>> priv = hcd_to_xhci_priv(hcd); > >>>> /* Just copy data for now */ > >>>> > >>>>> I have another question. Can't we now just assume that if the sysdev > >>>>> is the parent (or grandparent), then the phy initialization should > >>>>> always be skipped? In that case we could just do something like this: > >>>>> > >>>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > >>>>> index 649ffd861b44e..1018b33488046 100644 > >>>>> --- a/drivers/usb/host/xhci-plat.c > >>>>> +++ b/drivers/usb/host/xhci-plat.c > >>>>> @@ -212,8 +212,12 @@ static int xhci_plat_probe(struct platform_device *pdev) > >>>>> #endif > >>>>> } > >>>>> > >>>>> - if (!sysdev) > >>>>> + if (sysdev) { > >>>>> + if (sysdev != &pdev->dev) > >>>>> + hcd->skip_phy_initialization = 1; > >>>>> + } else { > >>>>> sysdev = &pdev->dev; > >>>>> + } > >>>>> > >>>>> if (WARN_ON(!sysdev->dma_mask)) > >>>>> /* Platform did not initialize dma_mask */ > >>>>> > >>>>> > >>>>> I did not go through all the drivers that carefully, so I may have > >>>>> missed something, but it looks like the only drivers that can have the > >>>>> sysdev as the parent or grandparent are cdns3 and dwc3. > >>>>> > >>>> I cross checked and these are two drivers that are creating xhci-plat device. > >>>> So this patch would definitely work. However I am not sure in future if any > >>>> device created via device tree would want to use this feature. For now, > >>>> it looks good. It Mathias, Do you see any problem with this approach? > >>>> > >> > >> Would work for now but seems like a risk to assume this would hold for all future > >> xhci platform devices. > >> > > Agree that it may break in future for other drivers. > > > >>> > >>> Can you please provide your suggestions on this? We have discussed about > >>> 3 approaches here other than the whole platform data refactoring done. > >>> > >>> (1) Introduce a new dT property and expect dwc3/host.c to set this property > >>> to skip the phy initialization. > >> > >> Adding one more device property to swnode in dwc3/host.c starts > >> to look like the best option for now even if it didn't appeal initially. > >> > >> The place creating the xhci platform device should have best info on what properties > >> are needed for the platform device. > >> > >> So this would be just like Heikki's first suggestion, or Sandeep's patches 2/3 and 3/3 in: > >> https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/ > >> but without the devicetree binding documentation part. > >> > > Why do you say devicetree binding doc is not needed in this case? Possible > > that xhci-plat's device can be coming from dT and this param is passed to > > skip initialization of PHY. > > Adding it is fine by me. It was rejected earlier by Rob Herring > > But we can live without it. > In dwc3 and cdns3 "pure" platform case we add the device property when > creating the device. > > In dT case we should have a compatible entry, and we can add a matching > entry in usb_xhci_of_match[] which sets the needed quirk. > Got it. Thanks for the explanation. We can go with Sandeep's patches 2/3 and 3/3 which add dT param and can be set by the dwc3/host.c Thanks, Pavan ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-04-06 17:29 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-24 6:37 [PATCH v3 0/3] Refactor xhci quirks and plat private data Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 1/3] usb: xhci: refactor " Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 2/3] usb: xhci: Remove unwanted header inclusion Sandeep Maheswaram 2022-03-24 6:37 ` [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT Sandeep Maheswaram 2022-03-24 12:27 ` Heikki Krogerus 2022-03-24 22:36 ` Mathias Nyman 2022-03-25 11:27 ` Heikki Krogerus 2022-03-25 14:33 ` Mathias Nyman 2022-03-25 15:38 ` Heikki Krogerus 2022-03-29 9:18 ` Sandeep Maheswaram (Temp) 2022-03-29 9:28 ` Pavan Kondeti 2022-03-30 17:47 ` Mathias Nyman 2022-03-31 11:16 ` Heikki Krogerus 2022-04-04 8:25 ` Pavan Kondeti 2022-04-06 6:25 ` Pavan Kondeti 2022-04-06 10:27 ` Heikki Krogerus 2022-04-06 10:52 ` Mathias Nyman 2022-04-06 11:01 ` Pavan Kondeti 2022-04-06 12:18 ` Mathias Nyman 2022-04-06 15:34 ` Pavan Kondeti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).