[RFC PATCH v2 0/3] support Samsung Exynos xHCI Controller

[RFC PATCH v2 0/3] support Samsung Exynos xHCI Controller

[Daehwan Jung]

This patchset is to support xHCI Controller on Samsung Exynos SOCs.
Thanks for all reviews on v1 and have tried to modify it well.
I again added "RFC" because I haven't solved below problem when checking dt bindings.

usb@4a000000: #size-cells:0:0: 0 was expected
Documentation/devicetree/bindings/usb/snps,dwc3.yaml

Changes in v2 :
- Rename subject of cover letter
    ([RFC,v1,0/2] add xhci-exynos to support Samsung Exynos SOC)
- Add Exynos compatible in xhci platform driver instead making new platform driver.
- Make xhci platform driver as child of dwc3 with DT schema.
- Override roothub ops for xhci platform driver.

Daehwan Jung (3):
  usb: support Samsung Exynos xHCI Controller
  dt-bindings: usb: generic-xhci: add Samsung Exynos compatible
  dt-bindings: usb: snps,dwc3: add generic-xhci as child

 .../devicetree/bindings/usb/generic-xhci.yaml |  2 +
 .../devicetree/bindings/usb/snps,dwc3.yaml    | 29 +++++++++
 drivers/usb/dwc3/drd.c                        |  7 +++
 drivers/usb/dwc3/host.c                       | 33 +++++++++-
 drivers/usb/host/xhci-plat.c                  | 60 ++++++++++++++++++-
 drivers/usb/host/xhci.c                       |  4 ++
 drivers/usb/host/xhci.h                       |  5 ++
 7 files changed, 137 insertions(+), 3 deletions(-)

-- 
2.31.1

[RFC PATCH v2 1/3] usb: support Samsung Exynos xHCI Controller

[Daehwan Jung]

Currently, dwc3 invokes just xhci platform driver without any data.
We add xhci node as child of dwc3 node in order to get data from
device tree. It populates "xhci" child by name during initialization
of host. This patch only effects if dwc3 node has a child named "xhci"
not to disturb original path.

We add "samsung,exynos-xhci" compatible in xhci platform driver
to support Exynos SOCs. We introduce roothub wakeup, which uses roothub
as system wakeup source. It needs xhci platform driver to override
roothub ops.

Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
---
 drivers/usb/dwc3/drd.c       |  7 +++++
 drivers/usb/dwc3/host.c      | 33 ++++++++++++++++++--
 drivers/usb/host/xhci-plat.c | 60 +++++++++++++++++++++++++++++++++++-
 drivers/usb/host/xhci.c      |  4 +++
 drivers/usb/host/xhci.h      |  5 +++
 5 files changed, 106 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 039bf241769a..7263fd7bdf4f 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -496,6 +496,8 @@ static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)
 static int dwc3_setup_role_switch(struct dwc3 *dwc)
 {
 	struct usb_role_switch_desc dwc3_role_switch = {NULL};
+	struct device_node *dwc3_np = dev_of_node(dwc->dev);
+	struct device_node *xhci_np;
 	u32 mode;
 
 	dwc->role_switch_default_mode = usb_get_role_switch_default_mode(dwc->dev);
@@ -514,6 +516,10 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 	if (IS_ERR(dwc->role_sw))
 		return PTR_ERR(dwc->role_sw);
 
+	xhci_np = of_get_child_by_name(dwc3_np, "xhci");
+	if (xhci_np)
+		goto populate_skip;
+
 	if (dwc->dev->of_node) {
 		/* populate connector entry */
 		int ret = devm_of_platform_populate(dwc->dev);
@@ -526,6 +532,7 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 		}
 	}
 
+populate_skip:
 	dwc3_set_mode(dwc, mode);
 	return 0;
 }
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f6f13e7f1ba1..8adfe45d9681 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -9,6 +9,7 @@
 
 #include <linux/irq.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 
 #include "core.h"
@@ -68,9 +69,22 @@ int dwc3_host_init(struct dwc3 *dwc)
 {
 	struct property_entry	props[4];
 	struct platform_device	*xhci;
+	struct device_node *dwc3_np = dev_of_node(dwc->dev);
+	struct device_node *xhci_np;
 	int			ret, irq;
 	int			prop_idx = 0;
 
+	xhci_np = of_get_child_by_name(dwc3_np, "xhci");
+	if (xhci_np) {
+		ret = of_platform_populate(dwc3_np, NULL, NULL, dwc->dev);
+		if (ret) {
+			dev_err(dwc->dev, "failed to register xhci - %d\n", ret);
+			goto err;
+		}
+		dwc->xhci = of_find_device_by_node(xhci_np);
+		goto populate_done;
+	}
+
 	irq = dwc3_host_get_irq(dwc);
 	if (irq < 0)
 		return irq;
@@ -126,14 +140,29 @@ int dwc3_host_init(struct dwc3 *dwc)
 		goto err;
 	}
 
+populate_done:
 	return 0;
+
 err:
-	platform_device_put(xhci);
+	if (xhci_np)
+		of_platform_depopulate(dwc->dev);
+	else
+		platform_device_put(xhci);
+
 	return ret;
 }
 
 void dwc3_host_exit(struct dwc3 *dwc)
 {
-	platform_device_unregister(dwc->xhci);
+	struct device_node *dwc3_np = dev_of_node(dwc->dev);
+	struct device_node *xhci_np;
+
+	xhci_np = of_get_child_by_name(dwc3_np, "xhci");
+
+	if (xhci_np)
+		of_platform_depopulate(dwc->dev);
+	else
+		platform_device_unregister(dwc->xhci);
+
 	dwc->xhci = NULL;
 }
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 5fb55bf19493..0eeaf306076a 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -29,11 +29,15 @@ static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
 static int xhci_plat_setup(struct usb_hcd *hcd);
 static int xhci_plat_start(struct usb_hcd *hcd);
+static int xhci_plat_bus_suspend(struct usb_hcd *hcd);
+static int xhci_plat_bus_resume(struct usb_hcd *hcd);
 
 static const struct xhci_driver_overrides xhci_plat_overrides __initconst = {
 	.extra_priv_size = sizeof(struct xhci_plat_priv),
 	.reset = xhci_plat_setup,
 	.start = xhci_plat_start,
+	.bus_suspend = xhci_plat_bus_suspend,
+	.bus_resume = xhci_plat_bus_resume,
 };
 
 static void xhci_priv_plat_start(struct usb_hcd *hcd)
@@ -86,6 +90,33 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 	xhci->quirks |= XHCI_PLAT | priv->quirks;
 }
 
+static int xhci_plat_bus_suspend(struct usb_hcd *hcd)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+
+	if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) {
+		if (hcd == xhci->main_hcd)
+			__pm_relax(xhci->main_wakelock);
+		else
+			__pm_relax(xhci->shared_wakelock);
+	}
+
+	return xhci_bus_suspend(hcd);
+}
+
+static int xhci_plat_bus_resume(struct usb_hcd *hcd)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+
+	if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) {
+		if (hcd == xhci->main_hcd)
+			__pm_stay_awake(xhci->main_wakelock);
+		else
+			__pm_stay_awake(xhci->shared_wakelock);
+	}
+	return xhci_bus_resume(hcd);
+}
+
 /* called during probe() after chip reset completes */
 static int xhci_plat_setup(struct usb_hcd *hcd)
 {
@@ -126,6 +157,10 @@ static const struct xhci_plat_priv xhci_plat_brcm = {
 	.quirks = XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS,
 };
 
+static const struct xhci_plat_priv xhci_plat_exynos = {
+	.quirks = XHCI_ROOTHUB_WAKEUP,
+};
+
 static const struct of_device_id usb_xhci_of_match[] = {
 	{
 		.compatible = "generic-xhci",
@@ -167,6 +202,9 @@ static const struct of_device_id usb_xhci_of_match[] = {
 	}, {
 		.compatible = "brcm,bcm7445-xhci",
 		.data = &xhci_plat_brcm,
+	}, {
+		.compatible = "samsung,exynos-xhci",
+		.data = &xhci_plat_exynos,
 	},
 	{},
 };
@@ -185,7 +223,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	int			irq;
 	struct xhci_plat_priv	*priv = NULL;
 
-
 	if (usb_disabled())
 		return -ENODEV;
 
@@ -350,6 +387,21 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			goto put_usb3_hcd;
 	}
 
+	if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) {
+		/* Initialization wakelock for HCD */
+		xhci->main_wakelock = wakeup_source_register(&pdev->dev,
+							dev_name(&pdev->dev));
+		__pm_stay_awake(xhci->main_wakelock);
+		device_set_wakeup_enable(&xhci->main_hcd->self.root_hub->dev, true);
+
+		if (xhci->shared_hcd) {
+			xhci->shared_wakelock = wakeup_source_register(&pdev->dev,
+								dev_name(&pdev->dev));
+			__pm_stay_awake(xhci->shared_wakelock);
+			device_set_wakeup_enable(&xhci->shared_hcd->self.root_hub->dev, true);
+		}
+	}
+
 	device_enable_async_suspend(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
 
@@ -398,6 +450,12 @@ static int xhci_plat_remove(struct platform_device *dev)
 	pm_runtime_get_sync(&dev->dev);
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
 
+	if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) {
+		wakeup_source_unregister(xhci->main_wakelock);
+		if (xhci->shared_wakelock)
+			wakeup_source_unregister(xhci->shared_wakelock);
+	}
+
 	if (shared_hcd) {
 		usb_remove_hcd(shared_hcd);
 		xhci->shared_hcd = NULL;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 79d7931c048a..693495054001 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5502,6 +5502,10 @@ void xhci_init_driver(struct hc_driver *drv,
 			drv->check_bandwidth = over->check_bandwidth;
 		if (over->reset_bandwidth)
 			drv->reset_bandwidth = over->reset_bandwidth;
+		if (over->bus_suspend)
+			drv->bus_suspend = over->bus_suspend;
+		if (over->bus_resume)
+			drv->bus_resume = over->bus_resume;
 	}
 }
 EXPORT_SYMBOL_GPL(xhci_init_driver);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index c9f06c5e4e9d..cb9c54a6a22c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1752,6 +1752,8 @@ struct xhci_hub {
 struct xhci_hcd {
 	struct usb_hcd *main_hcd;
 	struct usb_hcd *shared_hcd;
+	struct wakeup_source *main_wakelock;
+	struct wakeup_source *shared_wakelock;
 	/* glue to PCI and HCD framework */
 	struct xhci_cap_regs __iomem *cap_regs;
 	struct xhci_op_regs __iomem *op_regs;
@@ -1898,6 +1900,7 @@ struct xhci_hcd {
 #define XHCI_EP_CTX_BROKEN_DCS	BIT_ULL(42)
 #define XHCI_SUSPEND_RESUME_CLKS	BIT_ULL(43)
 #define XHCI_RESET_TO_DEFAULT	BIT_ULL(44)
+#define XHCI_ROOTHUB_WAKEUP	BIT_ULL(45)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
@@ -1943,6 +1946,8 @@ struct xhci_driver_overrides {
 			     struct usb_host_endpoint *ep);
 	int (*check_bandwidth)(struct usb_hcd *, struct usb_device *);
 	void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
+	int (*bus_suspend)(struct usb_hcd *hcd);
+	int (*bus_resume)(struct usb_hcd *hcd);
 };
 
 #define	XHCI_CFC_DELAY		10
-- 
2.31.1

[RFC PATCH v2 2/3] dt-bindings: usb: generic-xhci: add Samsung Exynos compatible

[Daehwan Jung]

Add compatible for Samsung Exynos SOCs

Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
---
 Documentation/devicetree/bindings/usb/generic-xhci.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/generic-xhci.yaml b/Documentation/devicetree/bindings/usb/generic-xhci.yaml
index db841589fc33..f54aff477637 100644
--- a/Documentation/devicetree/bindings/usb/generic-xhci.yaml
+++ b/Documentation/devicetree/bindings/usb/generic-xhci.yaml
@@ -29,6 +29,8 @@ properties:
         enum:
           - brcm,xhci-brcm-v2
           - brcm,bcm7445-xhci
+      - description: Samsung Exynos SoCs with xHCI
+        const: samsung,exynos-xhci
       - description: Generic xHCI device
         const: xhci-platform
         deprecated: true
-- 
2.31.1

[RFC PATCH v2 3/3] dt-bindings: usb: snps,dwc3: add generic-xhci as child

[Daehwan Jung]

Currently, dwc3 invokes just xhci platform driver(generic-xhci) without
DT schema even though xhci works as child of dwc3. It makes sense to add
xhci as child of dwc3 with DT schema. It also supports to use another
compatible in xhci platform driver.

Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
---
 .../devicetree/bindings/usb/snps,dwc3.yaml    | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 6d78048c4613..83ed7c526dba 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -360,8 +360,22 @@ properties:
     description:
       Enable USB remote wakeup.
 
+  "#address-cells":
+    enum: [ 1, 2 ]
+
+  "#size-cells":
+    enum: [ 1, 2 ]
+
+  ranges: true
+
 unevaluatedProperties: false
 
+# Required child node:
+patternProperties:
+  "^usb@[0-9a-f]+$":
+    $ref: generic-xhci.yaml#
+    description: Required child node
+
 required:
   - compatible
   - reg
@@ -388,4 +402,19 @@ examples:
       snps,dis_u2_susphy_quirk;
       snps,dis_enblslpm_quirk;
     };
+  - |
+    usb@4a200000 {
+      compatible = "snps,dwc3";
+      reg = <0x4a200000 0xcfff>;
+      interrupts = <0 92 4>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      ranges;
+
+      xhci: usb@4a200000 {
+        compatible = "generic-xhci";
+        reg = <0x4a200000 0x7fff>;
+        interrupts = <0 92 4>;
+      };
+    };
 ...
-- 
2.31.1

Re: [RFC PATCH v2 2/3] dt-bindings: usb: generic-xhci: add Samsung Exynos compatible

[Krzysztof Kozlowski]

On 29/12/2022 10:57, Daehwan Jung wrote:
> Add compatible for Samsung Exynos SOCs

Missing full stop. Please explain here in details the hardware.
Otherwise it looks it is not for any hardware and patch should be dropped.

Also, missing DTS. I am going to keep NAK-ing this till you provide the
user.

NAK.

> 
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> ---
>  Documentation/devicetree/bindings/usb/generic-xhci.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/generic-xhci.yaml b/Documentation/devicetree/bindings/usb/generic-xhci.yaml
> index db841589fc33..f54aff477637 100644
> --- a/Documentation/devicetree/bindings/usb/generic-xhci.yaml
> +++ b/Documentation/devicetree/bindings/usb/generic-xhci.yaml
> @@ -29,6 +29,8 @@ properties:
>          enum:
>            - brcm,xhci-brcm-v2
>            - brcm,bcm7445-xhci
> +      - description: Samsung Exynos SoCs with xHCI
> +        const: samsung,exynos-xhci

Missing fallback.

>        - description: Generic xHCI device>          const: xhci-platform
>          deprecated: true

Best regards,
Krzysztof

Re: [RFC PATCH v2 3/3] dt-bindings: usb: snps,dwc3: add generic-xhci as child

[Krzysztof Kozlowski]

On 29/12/2022 10:57, Daehwan Jung wrote:
> Currently, dwc3 invokes just xhci platform driver(generic-xhci) without
> DT schema even though xhci works as child of dwc3. It makes sense to add
> xhci as child of dwc3 with DT schema. It also supports to use another
> compatible in xhci platform driver.

You use some driver as an argument for hardware description, which is
not what we need. Describe the hardware.

> 
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml    | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index 6d78048c4613..83ed7c526dba 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -360,8 +360,22 @@ properties:
>      description:
>        Enable USB remote wakeup.
>  
> +  "#address-cells":
> +    enum: [ 1, 2 ]
> +
> +  "#size-cells":
> +    enum: [ 1, 2 ]
> +
> +  ranges: true
> +
>  unevaluatedProperties: false
>  
> +# Required child node:
> +patternProperties:
> +  "^usb@[0-9a-f]+$":
> +    $ref: generic-xhci.yaml#
> +    description: Required child node

DWC does not have another piece of controller as child... DWC is the
controller. Not mentioning that you now affect several other devices
without describing the total hardware picture (just some drivers which
is not that relevant).

> +
>  required:
>    - compatible
>    - reg
> @@ -388,4 +402,19 @@ examples:
>        snps,dis_u2_susphy_quirk;
>        snps,dis_enblslpm_quirk;
>      };
> +  - |
> +    usb@4a200000 {
> +      compatible = "snps,dwc3";
> +      reg = <0x4a200000 0xcfff>;
> +      interrupts = <0 92 4>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      ranges;
> +
> +      xhci: usb@4a200000 {
> +        compatible = "generic-xhci";

There are no such device...

> +        reg = <0x4a200000 0x7fff>;
> +        interrupts = <0 92 4>;
> +      };
> +    };
>  ...

Best regards,
Krzysztof

Re: [RFC PATCH v2 1/3] usb: support Samsung Exynos xHCI Controller

[Krzysztof Kozlowski]

On 29/12/2022 10:57, Daehwan Jung wrote:
> Currently, dwc3 invokes just xhci platform driver without any data.
> We add xhci node as child of dwc3 node in order to get data from
> device tree. It populates "xhci" child by name during initialization
> of host. This patch only effects if dwc3 node has a child named "xhci"
> not to disturb original path.
> 
> We add "samsung,exynos-xhci" compatible in xhci platform driver

Where? It is not documented.

> to support Exynos SOCs. 

That's so not true. You do nothing to support Exynos SoC here. Please
stop pasting incorrect and misleading commit msgs.

> We introduce roothub wakeup, which uses roothub
> as system wakeup source. It needs xhci platform driver to override
> roothub ops.

You did not explain why you introduced wakelocks...


(...)

>  	if (shared_hcd) {
>  		usb_remove_hcd(shared_hcd);
>  		xhci->shared_hcd = NULL;
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 79d7931c048a..693495054001 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -5502,6 +5502,10 @@ void xhci_init_driver(struct hc_driver *drv,
>  			drv->check_bandwidth = over->check_bandwidth;
>  		if (over->reset_bandwidth)
>  			drv->reset_bandwidth = over->reset_bandwidth;
> +		if (over->bus_suspend)
> +			drv->bus_suspend = over->bus_suspend;
> +		if (over->bus_resume)
> +			drv->bus_resume = over->bus_resume;
>  	}
>  }
>  EXPORT_SYMBOL_GPL(xhci_init_driver);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index c9f06c5e4e9d..cb9c54a6a22c 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1752,6 +1752,8 @@ struct xhci_hub {
>  struct xhci_hcd {
>  	struct usb_hcd *main_hcd;
>  	struct usb_hcd *shared_hcd;
> +	struct wakeup_source *main_wakelock;
> +	struct wakeup_source *shared_wakelock;

Drop wakelocks. This is not related to USB and not needed here. Do you
see anywhere else in core kernel code usage of the wakelocks?

You got this comment already, didn't you? So why you do not address it?

Best regards,
Krzysztof

Re: [RFC PATCH v2 1/3] usb: support Samsung Exynos xHCI Controller

[Arnd Bergmann]

On Thu, Dec 29, 2022, at 10:57, Daehwan Jung wrote:
> Currently, dwc3 invokes just xhci platform driver without any data.
> We add xhci node as child of dwc3 node in order to get data from
> device tree. It populates "xhci" child by name during initialization
> of host. This patch only effects if dwc3 node has a child named "xhci"
> not to disturb original path.

Using child nodes is not the normal way of abstracting a soc specific
variant of a device, though there are some USB host drivers that
do this. Just use the node itself and add whatever samsung specific
properties are needed based on the compatible string.

> @@ -86,6 +90,33 @@ static void xhci_plat_quirks(struct device *dev, 
> struct xhci_hcd *xhci)
>  	xhci->quirks |= XHCI_PLAT | priv->quirks;
>  }
> 
> +static int xhci_plat_bus_suspend(struct usb_hcd *hcd)
> +{
> +	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +
> +	if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) {
> +		if (hcd == xhci->main_hcd)
> +			__pm_relax(xhci->main_wakelock);
> +		else
> +			__pm_relax(xhci->shared_wakelock);
> +	}
> +
> +	return xhci_bus_suspend(hcd);
> +}
> +
> +static int xhci_plat_bus_resume(struct usb_hcd *hcd)
> +{
> +	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +
> +	if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) {
> +		if (hcd == xhci->main_hcd)
> +			__pm_stay_awake(xhci->main_wakelock);
> +		else
> +			__pm_stay_awake(xhci->shared_wakelock);
> +	}
> +	return xhci_bus_resume(hcd);
> +}

It looks like these are no longer tied to the Samsung
device type, which would be a step in the right direction,
but I think adding this should be a separate patch since
it is not a hardware specific change but a new feature.

    Arnd

Re: [RFC PATCH v2 3/3] dt-bindings: usb: snps,dwc3: add generic-xhci as child

[Rob Herring]

On Thu, 29 Dec 2022 18:57:46 +0900, Daehwan Jung wrote:
> Currently, dwc3 invokes just xhci platform driver(generic-xhci) without
> DT schema even though xhci works as child of dwc3. It makes sense to add
> xhci as child of dwc3 with DT schema. It also supports to use another
> compatible in xhci platform driver.
> 
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml    | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.example.dtb: usb@4a200000: #size-cells:0:0: 0 was expected
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1672307866-25839-4-git-send-email-dh10.jung@samsung.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [RFC PATCH v2 2/3] dt-bindings: usb: generic-xhci: add Samsung Exynos compatible

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

On Thu, Dec 29, 2022 at 11:19:09AM +0100, Krzysztof Kozlowski wrote:
> On 29/12/2022 10:57, Daehwan Jung wrote:
> > Add compatible for Samsung Exynos SOCs
> 
> Missing full stop. Please explain here in details the hardware.
> Otherwise it looks it is not for any hardware and patch should be dropped.
> 

I got it. This patch may be for new feature of generic xhci not for exynos.
I will add hardware description on next submission.

> Also, missing DTS. I am going to keep NAK-ing this till you provide the
> user.
> 
> NAK.
> 

I've added a example and checked bindings following below guides.

https://docs.kernel.org/devicetree/bindings/submitting-patches.html
https://docs.kernel.org/devicetree/bindings/writing-schema.html

I have no idea that I have to also submit DTS.
I will submit it on next submission.

> > 
> > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> > ---
> >  Documentation/devicetree/bindings/usb/generic-xhci.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/generic-xhci.yaml b/Documentation/devicetree/bindings/usb/generic-xhci.yaml
> > index db841589fc33..f54aff477637 100644
> > --- a/Documentation/devicetree/bindings/usb/generic-xhci.yaml
> > +++ b/Documentation/devicetree/bindings/usb/generic-xhci.yaml
> > @@ -29,6 +29,8 @@ properties:
> >          enum:
> >            - brcm,xhci-brcm-v2
> >            - brcm,bcm7445-xhci
> > +      - description: Samsung Exynos SoCs with xHCI
> > +        const: samsung,exynos-xhci
> 
> Missing fallback.

Modifying it like below is OK?

decription: Samsung Exynos SoCs with xHCI
        items:
            - const: samsung,exynos-xhci
            - const: generic-xhci

Best Regards,
Jung Daehwan

> 
> >        - description: Generic xHCI device>          const: xhci-platform
> >          deprecated: true
> 
> Best regards,
> Krzysztof
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v2 3/3] dt-bindings: usb: snps,dwc3: add generic-xhci as child

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 2639 bytes --]

On Thu, Dec 29, 2022 at 11:23:02AM +0100, Krzysztof Kozlowski wrote:
> On 29/12/2022 10:57, Daehwan Jung wrote:
> > Currently, dwc3 invokes just xhci platform driver(generic-xhci) without
> > DT schema even though xhci works as child of dwc3. It makes sense to add
> > xhci as child of dwc3 with DT schema. It also supports to use another
> > compatible in xhci platform driver.
> 
> You use some driver as an argument for hardware description, which is
> not what we need. Describe the hardware.
> 

OK. I will it on next submission.

> > 
> > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> > ---
> >  .../devicetree/bindings/usb/snps,dwc3.yaml    | 29 +++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > index 6d78048c4613..83ed7c526dba 100644
> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > @@ -360,8 +360,22 @@ properties:
> >      description:
> >        Enable USB remote wakeup.
> >  
> > +  "#address-cells":
> > +    enum: [ 1, 2 ]
> > +
> > +  "#size-cells":
> > +    enum: [ 1, 2 ]
> > +
> > +  ranges: true
> > +
> >  unevaluatedProperties: false
> >  
> > +# Required child node:
> > +patternProperties:
> > +  "^usb@[0-9a-f]+$":
> > +    $ref: generic-xhci.yaml#
> > +    description: Required child node
> 
> DWC does not have another piece of controller as child... DWC is the
> controller. Not mentioning that you now affect several other devices
> without describing the total hardware picture (just some drivers which
> is not that relevant).
> 

DWC controller supports USB Host mode and it uses same resource and
really works as a child. I guess it's same on many SOCs, especially
mobile..

I just want to modify it to work with DT schema (dwc3 -> xhci-plat).
I think it needs to dicuss more..

Best Regards,
Jung Daehwan.

> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -388,4 +402,19 @@ examples:
> >        snps,dis_u2_susphy_quirk;
> >        snps,dis_enblslpm_quirk;
> >      };
> > +  - |
> > +    usb@4a200000 {
> > +      compatible = "snps,dwc3";
> > +      reg = <0x4a200000 0xcfff>;
> > +      interrupts = <0 92 4>;
> > +      #address-cells = <1>;
> > +      #size-cells = <1>;
> > +      ranges;
> > +
> > +      xhci: usb@4a200000 {
> > +        compatible = "generic-xhci";
> 
> There are no such device...
> 

> > +        reg = <0x4a200000 0x7fff>;
> > +        interrupts = <0 92 4>;
> > +      };
> > +    };
> >  ...
> 
> Best regards,
> Krzysztof
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v2 1/3] usb: support Samsung Exynos xHCI Controller

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 2813 bytes --]

On Thu, Dec 29, 2022 at 11:25:58AM +0100, Krzysztof Kozlowski wrote:
> On 29/12/2022 10:57, Daehwan Jung wrote:
> > Currently, dwc3 invokes just xhci platform driver without any data.
> > We add xhci node as child of dwc3 node in order to get data from
> > device tree. It populates "xhci" child by name during initialization
> > of host. This patch only effects if dwc3 node has a child named "xhci"
> > not to disturb original path.
> > 
> > We add "samsung,exynos-xhci" compatible in xhci platform driver
> 
> Where? It is not documented.

I submitted the patch of dt bindings on same patchset.
Is there any missing documentation?

> 
> > to support Exynos SOCs. 
> 
> That's so not true. You do nothing to support Exynos SoC here. Please
> stop pasting incorrect and misleading commit msgs.

I agree misleading commit msgs. I will modify it.

> 
> > We introduce roothub wakeup, which uses roothub
> > as system wakeup source. It needs xhci platform driver to override
> > roothub ops.
> 
> You did not explain why you introduced wakelocks...
> 

I'm sorry I didn't write description enough.
I add it below.

> 
> (...)
> 
> >  	if (shared_hcd) {
> >  		usb_remove_hcd(shared_hcd);
> >  		xhci->shared_hcd = NULL;
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 79d7931c048a..693495054001 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -5502,6 +5502,10 @@ void xhci_init_driver(struct hc_driver *drv,
> >  			drv->check_bandwidth = over->check_bandwidth;
> >  		if (over->reset_bandwidth)
> >  			drv->reset_bandwidth = over->reset_bandwidth;
> > +		if (over->bus_suspend)
> > +			drv->bus_suspend = over->bus_suspend;
> > +		if (over->bus_resume)
> > +			drv->bus_resume = over->bus_resume;
> >  	}
> >  }
> >  EXPORT_SYMBOL_GPL(xhci_init_driver);
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index c9f06c5e4e9d..cb9c54a6a22c 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -1752,6 +1752,8 @@ struct xhci_hub {
> >  struct xhci_hcd {
> >  	struct usb_hcd *main_hcd;
> >  	struct usb_hcd *shared_hcd;
> > +	struct wakeup_source *main_wakelock;
> > +	struct wakeup_source *shared_wakelock;
> 
> Drop wakelocks. This is not related to USB and not needed here. Do you
> see anywhere else in core kernel code usage of the wakelocks?
> 
> You got this comment already, didn't you? So why you do not address it?
> 

I want to add a new feature in xhci platform driver. I want to make it
possible to enter system sleep while usb host connected like USB Mouse.
It gets system enter sleep only if there's no usb transaction at all.
Deciding if there's tranaction or not is in root hub because it's parent
of all child usb devices.

Best Regards,
Jung Daehwan

> Best regards,
> Krzysztof
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v2 1/3] usb: support Samsung Exynos xHCI Controller

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 2392 bytes --]

On Thu, Dec 29, 2022 at 03:44:02PM +0100, Arnd Bergmann wrote:
> On Thu, Dec 29, 2022, at 10:57, Daehwan Jung wrote:
> > Currently, dwc3 invokes just xhci platform driver without any data.
> > We add xhci node as child of dwc3 node in order to get data from
> > device tree. It populates "xhci" child by name during initialization
> > of host. This patch only effects if dwc3 node has a child named "xhci"
> > not to disturb original path.
> 
> Using child nodes is not the normal way of abstracting a soc specific
> variant of a device, though there are some USB host drivers that
> do this. Just use the node itself and add whatever samsung specific
> properties are needed based on the compatible string.
> 

I've tried to use existing platform drivers(dwc3, xhci-plat).
Dwc3 has host mode and I think it's necessary to have child node.
Currently we can't use compatible string but can just use xhci platform driver
itself. That's why I modified to have a xhci child. It makes us to use
specific properties.

I don't find a better way even if it's not the normal way.
I'm going to talk with other maintainers(dwc3, xhci-plat) to solve the
problem.

> > @@ -86,6 +90,33 @@ static void xhci_plat_quirks(struct device *dev, 
> > struct xhci_hcd *xhci)
> >  	xhci->quirks |= XHCI_PLAT | priv->quirks;
> >  }
> > 
> > +static int xhci_plat_bus_suspend(struct usb_hcd *hcd)
> > +{
> > +	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> > +
> > +	if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) {
> > +		if (hcd == xhci->main_hcd)
> > +			__pm_relax(xhci->main_wakelock);
> > +		else
> > +			__pm_relax(xhci->shared_wakelock);
> > +	}
> > +
> > +	return xhci_bus_suspend(hcd);
> > +}
> > +
> > +static int xhci_plat_bus_resume(struct usb_hcd *hcd)
> > +{
> > +	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> > +
> > +	if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) {
> > +		if (hcd == xhci->main_hcd)
> > +			__pm_stay_awake(xhci->main_wakelock);
> > +		else
> > +			__pm_stay_awake(xhci->shared_wakelock);
> > +	}
> > +	return xhci_bus_resume(hcd);
> > +}
> 
> It looks like these are no longer tied to the Samsung
> device type, which would be a step in the right direction,
> but I think adding this should be a separate patch since
> it is not a hardware specific change but a new feature.
> 
>     Arnd
> 

Thanks for the comment. I will separate and fix commit msg on next
submission.

Best Regards,
Jung Deahwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v2 2/3] dt-bindings: usb: generic-xhci: add Samsung Exynos compatible

[Krzysztof Kozlowski]

On 02/01/2023 06:30, Jung Daehwan wrote:
> On Thu, Dec 29, 2022 at 11:19:09AM +0100, Krzysztof Kozlowski wrote:
>> On 29/12/2022 10:57, Daehwan Jung wrote:
>>> Add compatible for Samsung Exynos SOCs
>>
>> Missing full stop. Please explain here in details the hardware.
>> Otherwise it looks it is not for any hardware and patch should be dropped.
>>
> 
> I got it. This patch may be for new feature of generic xhci not for exynos.
> I will add hardware description on next submission.
> 
>> Also, missing DTS. I am going to keep NAK-ing this till you provide the
>> user.
>>
>> NAK.
>>
> 
> I've added a example and checked bindings following below guides.
> 
> https://docs.kernel.org/devicetree/bindings/submitting-patches.html
> https://docs.kernel.org/devicetree/bindings/writing-schema.html
> 
> I have no idea that I have to also submit DTS.
> I will submit it on next submission.

I have doubts that this accurate description of hardware, therefore I
want the DTS user of these bindings which will show entire picture.

> 
>>>
>>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
>>> ---
>>>  Documentation/devicetree/bindings/usb/generic-xhci.yaml | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/generic-xhci.yaml b/Documentation/devicetree/bindings/usb/generic-xhci.yaml
>>> index db841589fc33..f54aff477637 100644
>>> --- a/Documentation/devicetree/bindings/usb/generic-xhci.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/generic-xhci.yaml
>>> @@ -29,6 +29,8 @@ properties:
>>>          enum:
>>>            - brcm,xhci-brcm-v2
>>>            - brcm,bcm7445-xhci
>>> +      - description: Samsung Exynos SoCs with xHCI
>>> +        const: samsung,exynos-xhci
>>
>> Missing fallback.
> 
> Modifying it like below is OK?
> 
> decription: Samsung Exynos SoCs with xHCI
>         items:
>             - const: samsung,exynos-xhci
>             - const: generic-xhci

To this comment yes, but in general this does not solve my concerns that
it does not look like real hardware at all.


Best regards,
Krzysztof

Re: [RFC PATCH v2 1/3] usb: support Samsung Exynos xHCI Controller

[Krzysztof Kozlowski]

On 02/01/2023 07:24, Jung Daehwan wrote:
> On Thu, Dec 29, 2022 at 11:25:58AM +0100, Krzysztof Kozlowski wrote:
>> On 29/12/2022 10:57, Daehwan Jung wrote:
>>> Currently, dwc3 invokes just xhci platform driver without any data.
>>> We add xhci node as child of dwc3 node in order to get data from
>>> device tree. It populates "xhci" child by name during initialization
>>> of host. This patch only effects if dwc3 node has a child named "xhci"
>>> not to disturb original path.
>>>
>>> We add "samsung,exynos-xhci" compatible in xhci platform driver
>>
>> Where? It is not documented.
> 
> I submitted the patch of dt bindings on same patchset.
> Is there any missing documentation?

This is your first patch in the series and in this patch there is no
such bindings. Re-order the patches to have proper order.

> 
>>
>>> to support Exynos SOCs. 
>>
>> That's so not true. You do nothing to support Exynos SoC here. Please
>> stop pasting incorrect and misleading commit msgs.
> 
> I agree misleading commit msgs. I will modify it.
> 
>>
>>> We introduce roothub wakeup, which uses roothub
>>> as system wakeup source. It needs xhci platform driver to override
>>> roothub ops.
>>
>> You did not explain why you introduced wakelocks...
>>
> 
> I'm sorry I didn't write description enough.
> I add it below.
> 
>>
>> (...)
>>
>>>  	if (shared_hcd) {
>>>  		usb_remove_hcd(shared_hcd);
>>>  		xhci->shared_hcd = NULL;
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index 79d7931c048a..693495054001 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -5502,6 +5502,10 @@ void xhci_init_driver(struct hc_driver *drv,
>>>  			drv->check_bandwidth = over->check_bandwidth;
>>>  		if (over->reset_bandwidth)
>>>  			drv->reset_bandwidth = over->reset_bandwidth;
>>> +		if (over->bus_suspend)
>>> +			drv->bus_suspend = over->bus_suspend;
>>> +		if (over->bus_resume)
>>> +			drv->bus_resume = over->bus_resume;
>>>  	}
>>>  }
>>>  EXPORT_SYMBOL_GPL(xhci_init_driver);
>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>> index c9f06c5e4e9d..cb9c54a6a22c 100644
>>> --- a/drivers/usb/host/xhci.h
>>> +++ b/drivers/usb/host/xhci.h
>>> @@ -1752,6 +1752,8 @@ struct xhci_hub {
>>>  struct xhci_hcd {
>>>  	struct usb_hcd *main_hcd;
>>>  	struct usb_hcd *shared_hcd;
>>> +	struct wakeup_source *main_wakelock;
>>> +	struct wakeup_source *shared_wakelock;
>>
>> Drop wakelocks. This is not related to USB and not needed here. Do you
>> see anywhere else in core kernel code usage of the wakelocks?
>>
>> You got this comment already, didn't you? So why you do not address it?
>>
> 
> I want to add a new feature in xhci platform driver. I want to make it
> possible to enter system sleep while usb host connected like USB Mouse.
> It gets system enter sleep only if there's no usb transaction at all.
> Deciding if there's tranaction or not is in root hub because it's parent
> of all child usb devices.


I have USB mouse connected to my system and the system enters suspend,
thus I don't think this patch solves this particular issue.

Best regards,
Krzysztof

Re: [RFC PATCH v2 3/3] dt-bindings: usb: snps,dwc3: add generic-xhci as child

[Rob Herring]

On Mon, Jan 02, 2023 at 02:45:17PM +0900, Jung Daehwan wrote:
> On Thu, Dec 29, 2022 at 11:23:02AM +0100, Krzysztof Kozlowski wrote:
> > On 29/12/2022 10:57, Daehwan Jung wrote:
> > > Currently, dwc3 invokes just xhci platform driver(generic-xhci) without
> > > DT schema even though xhci works as child of dwc3. It makes sense to add
> > > xhci as child of dwc3 with DT schema. It also supports to use another
> > > compatible in xhci platform driver.
> > 
> > You use some driver as an argument for hardware description, which is
> > not what we need. Describe the hardware.
> > 
> 
> OK. I will it on next submission.
> 
> > > 
> > > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> > > ---
> > >  .../devicetree/bindings/usb/snps,dwc3.yaml    | 29 +++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > index 6d78048c4613..83ed7c526dba 100644
> > > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > @@ -360,8 +360,22 @@ properties:
> > >      description:
> > >        Enable USB remote wakeup.
> > >  
> > > +  "#address-cells":
> > > +    enum: [ 1, 2 ]
> > > +
> > > +  "#size-cells":
> > > +    enum: [ 1, 2 ]
> > > +
> > > +  ranges: true
> > > +
> > >  unevaluatedProperties: false
> > >  
> > > +# Required child node:
> > > +patternProperties:
> > > +  "^usb@[0-9a-f]+$":
> > > +    $ref: generic-xhci.yaml#
> > > +    description: Required child node
> > 
> > DWC does not have another piece of controller as child... DWC is the
> > controller. Not mentioning that you now affect several other devices
> > without describing the total hardware picture (just some drivers which
> > is not that relevant).
> > 
> 
> DWC controller supports USB Host mode and it uses same resource and
> really works as a child. I guess it's same on many SOCs, especially
> mobile..

Yes, and we already support all those platforms just fine without this 
child node. Adding it means we have to then support *both* ways in the 
driver.


> I just want to modify it to work with DT schema (dwc3 -> xhci-plat).
> I think it needs to dicuss more..

Why doesn't it work with the schema?

It's convenient when DT nodes == device drivers, but hardware is messy 
sometimes. Linux (and other OSs) have to deal with that. We can't write 
the DT to reflect the current (and evolving) needs of a particular OS. 

Rob