[PATCH 0/3] usb: xhci: Add support for Google XHCI controller

[PATCH 0/3] usb: xhci: Add support for Google XHCI controller

[Puma Hsu]

In our SoC platform, we support allocating dedicated memory spaces
other than system memory for XHCI, which also requires IOMMU mapping.
The rest of driver probing and executing will use the generic
xhci-plat driver, so we introduce a Google XHCI glue driver.

Besides, we support USB dual roles and switch roles by generic dwc3
driver, but the dwc3 driver always probes xhci-plat driver by hardcode.
We introduce an alternative for probing a XHCI glue driver.

Puma Hsu (3):
  dt-bindings: usb: Add xhci glue driver support
  usb: xhci: Add support for Google XHCI controller
  MAINTAINERS: Add maintainer for Google USB XHCI driver

 .../devicetree/bindings/usb/usb-drd.yaml      |   7 +
 MAINTAINERS                                   |   6 +
 drivers/usb/dwc3/host.c                       |   8 +-
 drivers/usb/host/Kconfig                      |   6 +
 drivers/usb/host/Makefile                     |   1 +
 drivers/usb/host/xhci-goog.c                  | 154 ++++++++++++++++++
 6 files changed, 181 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/host/xhci-goog.c

-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 1/3] dt-bindings: usb: Add xhci glue driver support

[Puma Hsu]

Currently the dwc3 driver always probes xhci-plat driver
by hardcode in driver. Introduce a property to make this
flexible that a user can probe a xhci glue driver by the
generic dwc3 driver.

Signed-off-by: Puma Hsu <pumahsu@google.com>
---
 Documentation/devicetree/bindings/usb/usb-drd.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-drd.yaml b/Documentation/devicetree/bindings/usb/usb-drd.yaml
index 114fb5dc0498..215fb7f70054 100644
--- a/Documentation/devicetree/bindings/usb/usb-drd.yaml
+++ b/Documentation/devicetree/bindings/usb/usb-drd.yaml
@@ -62,6 +62,12 @@ properties:
     enum: [host, peripheral]
     default: peripheral
 
+  xhci-glue:
+    description:
+      Tell dwc3 core driver what xhci specific platform driver we want to probe.
+      The string should match to the name of device_driver of platform_driver
+      in the xhci specific platform driver.
+
 additionalProperties: true
 
 examples:
@@ -76,4 +82,5 @@ examples:
         phy_type = "utmi_wide";
         otg-rev = <0x0200>;
         adp-disable;
+        xhci-glue = "xhci-hcd-plat";
     };
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH 1/3] dt-bindings: usb: Add xhci glue driver support

[Krzysztof Kozlowski]

On 19/02/2024 07:10, Puma Hsu wrote:
> Currently the dwc3 driver always probes xhci-plat driver

Not a DT property, at least at first glance. NAK.

> by hardcode in driver. Introduce a property to make this
> flexible that a user can probe a xhci glue driver by the
> generic dwc3 driver.
> 
> Signed-off-by: Puma Hsu <pumahsu@google.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Best regards,
Krzysztof

[PATCH 2/3] usb: xhci: Add support for Google XHCI controller

[Puma Hsu]

In our SoC platform, we support allocating dedicated memory spaces
other than system memory for XHCI, which also requires IOMMU mapping.
The rest of driver probing and executing will use the generic
xhci-plat driver.

We support USB dual roles and switch roles by generic dwc3 driver,
the dwc3 driver always probes xhci-plat driver now, so we introduce
a device tree property to probe a XHCI glue driver.

Sample:
  xhci_dma: xhci_dma@99C0000 {
    compatible = "shared-dma-pool";
    reg = <0x00000000 0x99C0000 0x00000000 0x40000>;
    no-map;
  };

  dwc3: dwc3@c400000 {
    compatible = "snps,dwc3";
    reg = <0 0x0c400000  0 0x10000>;
    xhci-glue = "xhci-hcd-goog";
    memory-region = <&xhci_dma>;
    iommus = <&cpuacc_mmu 0x8100>;
  };

Signed-off-by: Puma Hsu <pumahsu@google.com>
---
 drivers/usb/dwc3/host.c      |   8 +-
 drivers/usb/host/Kconfig     |   6 ++
 drivers/usb/host/Makefile    |   1 +
 drivers/usb/host/xhci-goog.c | 154 +++++++++++++++++++++++++++++++++++
 4 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/host/xhci-goog.c

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index ae189b7a4f8b..45114c0fc38d 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -109,6 +109,7 @@ int dwc3_host_init(struct dwc3 *dwc)
 	struct platform_device	*xhci;
 	int			ret, irq;
 	int			prop_idx = 0;
+	const char		*xhci_glue;
 
 	/*
 	 * Some platforms need to power off all Root hub ports immediately after DWC3 set to host
@@ -121,7 +122,12 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (irq < 0)
 		return irq;
 
-	xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
+	device_property_read_string(dwc->dev, "xhci-glue", &xhci_glue);
+	if (xhci_glue)
+		xhci = platform_device_alloc(xhci_glue, PLATFORM_DEVID_AUTO);
+	else
+		xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
+
 	if (!xhci) {
 		dev_err(dwc->dev, "couldn't allocate xHCI device\n");
 		return -ENOMEM;
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 4448d0ab06f0..1c1613c548d9 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -61,6 +61,12 @@ config USB_XHCI_PLATFORM
 
 	  If unsure, say N.
 
+config USB_XHCI_GOOG
+	tristate "xHCI support for Google Tensor SoCs"
+	help
+	  Say 'Y' to enable the support for the xHCI host controller
+	  found in Google Tensor SoCs.
+	  If unsure, say N.
+
 config USB_XHCI_HISTB
 	tristate "xHCI support for HiSilicon STB SoCs"
 	depends on USB_XHCI_PLATFORM && (ARCH_HISI || COMPILE_TEST)
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index be4e5245c52f..76f315a1aa76 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -85,3 +85,4 @@ obj-$(CONFIG_USB_HCD_BCMA)	+= bcma-hcd.o
 obj-$(CONFIG_USB_HCD_SSB)	+= ssb-hcd.o
 obj-$(CONFIG_USB_MAX3421_HCD)	+= max3421-hcd.o
 obj-$(CONFIG_USB_XEN_HCD)	+= xen-hcd.o
+obj-$(CONFIG_USB_XHCI_GOOG)	+= xhci-goog.o
diff --git a/drivers/usb/host/xhci-goog.c b/drivers/usb/host/xhci-goog.c
new file mode 100644
index 000000000000..db027a5866db
--- /dev/null
+++ b/drivers/usb/host/xhci-goog.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * xhci-goog.c - xHCI host controller driver platform Bus Glue.
+ *
+ * Copyright (c) 2024 Google LLC
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/usb/phy.h>
+#include <linux/usb/of.h>
+
+#include "xhci.h"
+#include "xhci-plat.h"
+
+
+static int xhci_goog_probe(struct platform_device *pdev)
+{
+	const struct xhci_plat_priv *priv_match;
+	struct device *sysdev;
+	int ret;
+	struct device_node	*np;
+	struct iommu_domain	*domain;
+	struct reserved_mem	*rmem;
+	unsigned long           iova;
+	phys_addr_t             paddr;
+
+	for (sysdev = &pdev->dev; sysdev; sysdev = sysdev->parent) {
+		if (is_of_node(sysdev->fwnode))
+			break;
+	}
+
+	np = of_parse_phandle(sysdev->of_node, "memory-region", 0);
+	if (np) {
+		ret = of_reserved_mem_device_init(sysdev);
+		if (ret) {
+			dev_err(sysdev, "Could not get reserved memory\n");
+			return ret;
+		}
+
+		domain = iommu_get_domain_for_dev(sysdev);
+		if (domain) {
+			rmem = of_reserved_mem_lookup(np);
+			if (!rmem) {
+				dev_err(sysdev, "reserved memory lookup failed\n");
+				ret = -ENOMEM;
+				goto release_reserved_mem;
+			}
+
+			/* We do a direct mapping */
+			paddr = rmem->base;
+			iova = paddr;
+
+			dev_dbg(sysdev, "map: iova: 0x%lx, pa: %pa, size: 0x%llx\n", iova, &paddr,
+				rmem->size);
+
+			ret = iommu_map(domain, iova, paddr, rmem->size,
+					IOMMU_READ | IOMMU_WRITE, GFP_KERNEL);
+			if (ret < 0) {
+				dev_err(sysdev, "iommu map error: %d\n", ret);
+				goto release_reserved_mem;
+			}
+		}
+	}
+
+	if (WARN_ON(!sysdev->dma_mask)) {
+		/* Platform did not initialize dma_mask */
+		ret = dma_coerce_mask_and_coherent(sysdev, DMA_BIT_MASK(64));
+		if (ret)
+			goto unmap_iommu;
+	}
+
+	if (pdev->dev.of_node)
+		priv_match = of_device_get_match_data(&pdev->dev);
+	else
+		priv_match = dev_get_platdata(&pdev->dev);
+
+	ret = xhci_plat_probe(pdev, sysdev, priv_match);
+	if (ret) {
+		dev_err(&pdev->dev, "xhci plat probe failed: %d\n", ret);
+		goto unmap_iommu;
+	}
+
+	return 0;
+
+unmap_iommu:
+	iommu_unmap(domain, rmem->base, rmem->size);
+
+release_reserved_mem:
+	of_reserved_mem_device_release(sysdev);
+
+	return ret;
+}
+
+static int xhci_goog_remove(struct platform_device *dev)
+{
+	struct usb_hcd	*hcd = platform_get_drvdata(dev);
+	struct device *sysdev = hcd->self.sysdev;
+	struct iommu_domain *domain;
+	struct device_node *np;
+	struct reserved_mem *rmem;
+
+	xhci_plat_remove(dev);
+
+	domain = iommu_get_domain_for_dev(sysdev);
+	if (domain) {
+		np = of_parse_phandle(sysdev->of_node, "memory-region", 0);
+		rmem = of_reserved_mem_lookup(np);
+		if (rmem)
+			iommu_unmap(domain, rmem->base, rmem->size);
+	}
+
+	of_reserved_mem_device_release(sysdev);
+
+	return 0;
+}
+
+static void xhci_goog_shutdown(struct platform_device *dev)
+{
+	usb_hcd_platform_shutdown(dev);
+}
+
+static struct platform_driver usb_goog_xhci_driver = {
+	.probe	= xhci_goog_probe,
+	.remove	= xhci_goog_remove,
+	.shutdown = xhci_goog_shutdown,
+	.driver	= {
+		.name = "xhci-hcd-goog",
+		.pm = &xhci_plat_pm_ops,
+	},
+};
+MODULE_ALIAS("platform:xhci-hcd-goog");
+
+static int __init xhci_goog_init(void)
+{
+	return platform_driver_register(&usb_goog_xhci_driver);
+}
+module_init(xhci_goog_init);
+
+static void __exit xhci_goog_exit(void)
+{
+	platform_driver_unregister(&usb_goog_xhci_driver);
+}
+module_exit(xhci_goog_exit);
+
+MODULE_DESCRIPTION("Google xHCI Platform Host Controller Driver");
+MODULE_LICENSE("GPL");
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH 2/3] usb: xhci: Add support for Google XHCI controller

[Greg KH]

On Mon, Feb 19, 2024 at 02:10:07PM +0800, Puma Hsu wrote:
> diff --git a/drivers/usb/host/xhci-goog.c b/drivers/usb/host/xhci-goog.c
> new file mode 100644
> index 000000000000..db027a5866db
> --- /dev/null
> +++ b/drivers/usb/host/xhci-goog.c
> @@ -0,0 +1,154 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * xhci-goog.c - xHCI host controller driver platform Bus Glue.
> + *
> + * Copyright (c) 2024 Google LLC

This code is older than 2024, right?  :)

> +	if (WARN_ON(!sysdev->dma_mask)) {

If this triggers, you just rebooted your device (as you run with
panic-on-warn enabled), so please, if this is something that can
actually happen, handle it properly and move on, don't reboot a device.

> +MODULE_ALIAS("platform:xhci-hcd-goog");

Why is this alias needed?  I thought that was only for older-style
platform devices

> +static int __init xhci_goog_init(void)
> +{
> +	return platform_driver_register(&usb_goog_xhci_driver);
> +}
> +module_init(xhci_goog_init);
> +
> +static void __exit xhci_goog_exit(void)
> +{
> +	platform_driver_unregister(&usb_goog_xhci_driver);
> +}
> +module_exit(xhci_goog_exit);

module_platform_driver()?

thanks,

greg k-h

Re: [PATCH 2/3] usb: xhci: Add support for Google XHCI controller

[Puma Hsu]

On Mon, Feb 19, 2024 at 2:30 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Feb 19, 2024 at 02:10:07PM +0800, Puma Hsu wrote:
> > diff --git a/drivers/usb/host/xhci-goog.c b/drivers/usb/host/xhci-goog.c
> > new file mode 100644
> > index 000000000000..db027a5866db
> > --- /dev/null
> > +++ b/drivers/usb/host/xhci-goog.c
> > @@ -0,0 +1,154 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * xhci-goog.c - xHCI host controller driver platform Bus Glue.
> > + *
> > + * Copyright (c) 2024 Google LLC
>
> This code is older than 2024, right?  :)

Yes, this actually started from 2023, will fix it.

>
> > +     if (WARN_ON(!sysdev->dma_mask)) {
>
> If this triggers, you just rebooted your device (as you run with
> panic-on-warn enabled), so please, if this is something that can
> actually happen, handle it properly and move on, don't reboot a device.

Thank you for advising. Will review and handle it properly.

>
> > +MODULE_ALIAS("platform:xhci-hcd-goog");
>
> Why is this alias needed?  I thought that was only for older-style
> platform devices

I will change to module_platform_driver(). Thank you for advising.

>
> > +static int __init xhci_goog_init(void)
> > +{
> > +     return platform_driver_register(&usb_goog_xhci_driver);
> > +}
> > +module_init(xhci_goog_init);
> > +
> > +static void __exit xhci_goog_exit(void)
> > +{
> > +     platform_driver_unregister(&usb_goog_xhci_driver);
> > +}
> > +module_exit(xhci_goog_exit);
>
> module_platform_driver()?
>
> thanks,
>
> greg k-h

Re: [PATCH 2/3] usb: xhci: Add support for Google XHCI controller

[Krzysztof Kozlowski]

On 19/02/2024 07:10, Puma Hsu wrote:
> In our SoC platform, we support allocating dedicated memory spaces
> other than system memory for XHCI, which also requires IOMMU mapping.
> The rest of driver probing and executing will use the generic
> xhci-plat driver.
> 
> We support USB dual roles and switch roles by generic dwc3 driver,
> the dwc3 driver always probes xhci-plat driver now, so we introduce
> a device tree property to probe a XHCI glue driver.
> 
> Sample:
>   xhci_dma: xhci_dma@99C0000 {
>     compatible = "shared-dma-pool";
>     reg = <0x00000000 0x99C0000 0x00000000 0x40000>;
>     no-map;
>   };
> 
>   dwc3: dwc3@c400000 {
>     compatible = "snps,dwc3";
>     reg = <0 0x0c400000  0 0x10000>;
>     xhci-glue = "xhci-hcd-goog";

NAK, that's not DWC3 hardware in such case.

...

>  		return -ENOMEM;
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 4448d0ab06f0..1c1613c548d9 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -61,6 +61,12 @@ config USB_XHCI_PLATFORM
>  
>  	  If unsure, say N.
>  
> +config USB_XHCI_GOOG
> +	tristate "xHCI support for Google Tensor SoCs"
> +	help

Please always Cc Google Tensor SoC maintainers and Samsung SoC
maintainers on your contributions around Google Tensor SoC.

Anyway you just tried to push vendor code to upstream without aligning
it to usptream code style and to proper driver model. That's not good.
Please work with your colleagues in Google to explain how to upstream
vendor code. There were many, many trainings and presentations. One
coming from Dmitry will be in EOSS24 in two months.

Best regards,
Krzysztof

Re: [PATCH 2/3] usb: xhci: Add support for Google XHCI controller

[Puma Hsu]

On Mon, Feb 19, 2024 at 8:22 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 19/02/2024 07:10, Puma Hsu wrote:
> > In our SoC platform, we support allocating dedicated memory spaces
> > other than system memory for XHCI, which also requires IOMMU mapping.
> > The rest of driver probing and executing will use the generic
> > xhci-plat driver.
> >
> > We support USB dual roles and switch roles by generic dwc3 driver,
> > the dwc3 driver always probes xhci-plat driver now, so we introduce
> > a device tree property to probe a XHCI glue driver.
> >
> > Sample:
> >   xhci_dma: xhci_dma@99C0000 {
> >     compatible = "shared-dma-pool";
> >     reg = <0x00000000 0x99C0000 0x00000000 0x40000>;
> >     no-map;
> >   };
> >
> >   dwc3: dwc3@c400000 {
> >     compatible = "snps,dwc3";
> >     reg = <0 0x0c400000  0 0x10000>;
> >     xhci-glue = "xhci-hcd-goog";
>
> NAK, that's not DWC3 hardware in such case.

By introducing this property, users can specify the name of their
dedicated driver in the device tree. The generic dwc3 driver will
read this property to initiate the probing of the dedicated driver.
The motivation behind this is that we have dedicated things
(described in commit message) to do for the XHCI driver in our
device. BTW, I put this property here because currently there is
no xhci node, xhci related properties are put under dwc3 node.
It will be appreciated if there are alternative or more appropriate
approaches, we welcome discussion to explore the best possible
solution. Thanks.

>
> ...
>
> >               return -ENOMEM;
> > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > index 4448d0ab06f0..1c1613c548d9 100644
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -61,6 +61,12 @@ config USB_XHCI_PLATFORM
> >
> >         If unsure, say N.
> >
> > +config USB_XHCI_GOOG
> > +     tristate "xHCI support for Google Tensor SoCs"
> > +     help
>
> Please always Cc Google Tensor SoC maintainers and Samsung SoC
> maintainers on your contributions around Google Tensor SoC.
>
> Anyway you just tried to push vendor code to upstream without aligning
> it to usptream code style and to proper driver model. That's not good.
> Please work with your colleagues in Google to explain how to upstream
> vendor code. There were many, many trainings and presentations. One
> coming from Dmitry will be in EOSS24 in two months.

Thank you for advising. I will find the training and study it first, and will cc
the related maintainers in future code uploading.

>
> Best regards,
> Krzysztof
>

Re: [PATCH 2/3] usb: xhci: Add support for Google XHCI controller

[Krzysztof Kozlowski]

On 21/02/2024 10:31, Puma Hsu wrote:
> On Mon, Feb 19, 2024 at 8:22 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 19/02/2024 07:10, Puma Hsu wrote:
>>> In our SoC platform, we support allocating dedicated memory spaces
>>> other than system memory for XHCI, which also requires IOMMU mapping.
>>> The rest of driver probing and executing will use the generic
>>> xhci-plat driver.
>>>
>>> We support USB dual roles and switch roles by generic dwc3 driver,
>>> the dwc3 driver always probes xhci-plat driver now, so we introduce
>>> a device tree property to probe a XHCI glue driver.
>>>
>>> Sample:
>>>   xhci_dma: xhci_dma@99C0000 {
>>>     compatible = "shared-dma-pool";
>>>     reg = <0x00000000 0x99C0000 0x00000000 0x40000>;
>>>     no-map;
>>>   };
>>>
>>>   dwc3: dwc3@c400000 {
>>>     compatible = "snps,dwc3";
>>>     reg = <0 0x0c400000  0 0x10000>;
>>>     xhci-glue = "xhci-hcd-goog";
>>
>> NAK, that's not DWC3 hardware in such case.
> 
> By introducing this property, users can specify the name of their
> dedicated driver in the device tree. The generic dwc3 driver will

DT is not a place for driver stuff.


> read this property to initiate the probing of the dedicated driver.

I know, but it is not a reason to add stuff to DT.

> The motivation behind this is that we have dedicated things
> (described in commit message) to do for the XHCI driver in our
> device. BTW, I put this property here because currently there is
> no xhci node, xhci related properties are put under dwc3 node.

Sorry, you miss the point. Either you have pure DWC3 hardware or not.
You claim now you do not have pure hardware, which is reasonable,
because it is always customized per-vendor. In such case you cannot
claim this is a pure DWC3. You must provide bindings for your hardware.

Now, if you claim you have a pure DWC3 hardware without need for any
vendor customizations, then entire patchset is fake try to upstream your
Android vendor stuff. We talked about such stuff many times on mailing
list, so for obvious reasons I won't repeat it. Trying to push vendor
hooks and vendor quirks is one of the most common mistakes, so several
talks already say: don't do this.

> It will be appreciated if there are alternative or more appropriate
> approaches, we welcome discussion to explore the best possible
> solution. Thanks.

And what's wrong with all previous feedbacks for similar
Google/Samsung/Artpec/Tensor vendor hacks? Once or twice per year some
folks around Google or Samsung try to push such, they all receive the
same feedback and they disappear, so I have to repeat the same feedback
to the next person... Please go through previous patches from
@samsung.com for various subsystems.

Documentation/devicetree/bindings/submitting-patches.rst
Documentation/devicetree/bindings/writing-bindings.rst
+other people or my talks on Devicetree

Summarizing: Devicetree is for hardware, not for your driver
hooks/quirks/needs. Describe properly and fully the hardware, not your
driver.


Best regards,
Krzysztof

Re: [PATCH 2/3] usb: xhci: Add support for Google XHCI controller

[Puma Hsu]

On Wed, Feb 21, 2024 at 5:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 21/02/2024 10:31, Puma Hsu wrote:
> > On Mon, Feb 19, 2024 at 8:22 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 19/02/2024 07:10, Puma Hsu wrote:
> >>> In our SoC platform, we support allocating dedicated memory spaces
> >>> other than system memory for XHCI, which also requires IOMMU mapping.
> >>> The rest of driver probing and executing will use the generic
> >>> xhci-plat driver.
> >>>
> >>> We support USB dual roles and switch roles by generic dwc3 driver,
> >>> the dwc3 driver always probes xhci-plat driver now, so we introduce
> >>> a device tree property to probe a XHCI glue driver.
> >>>
> >>> Sample:
> >>>   xhci_dma: xhci_dma@99C0000 {
> >>>     compatible = "shared-dma-pool";
> >>>     reg = <0x00000000 0x99C0000 0x00000000 0x40000>;
> >>>     no-map;
> >>>   };
> >>>
> >>>   dwc3: dwc3@c400000 {
> >>>     compatible = "snps,dwc3";
> >>>     reg = <0 0x0c400000  0 0x10000>;
> >>>     xhci-glue = "xhci-hcd-goog";
> >>
> >> NAK, that's not DWC3 hardware in such case.
> >
> > By introducing this property, users can specify the name of their
> > dedicated driver in the device tree. The generic dwc3 driver will
>
> DT is not a place for driver stuff.
>
>
> > read this property to initiate the probing of the dedicated driver.
>
> I know, but it is not a reason to add stuff to DT.
>
> > The motivation behind this is that we have dedicated things
> > (described in commit message) to do for the XHCI driver in our
> > device. BTW, I put this property here because currently there is
> > no xhci node, xhci related properties are put under dwc3 node.
>
> Sorry, you miss the point. Either you have pure DWC3 hardware or not.
> You claim now you do not have pure hardware, which is reasonable,
> because it is always customized per-vendor. In such case you cannot
> claim this is a pure DWC3. You must provide bindings for your hardware.
>
> Now, if you claim you have a pure DWC3 hardware without need for any
> vendor customizations, then entire patchset is fake try to upstream your
> Android vendor stuff. We talked about such stuff many times on mailing
> list, so for obvious reasons I won't repeat it. Trying to push vendor
> hooks and vendor quirks is one of the most common mistakes, so several
> talks already say: don't do this.
>
> > It will be appreciated if there are alternative or more appropriate
> > approaches, we welcome discussion to explore the best possible
> > solution. Thanks.
>
> And what's wrong with all previous feedbacks for similar
> Google/Samsung/Artpec/Tensor vendor hacks? Once or twice per year some
> folks around Google or Samsung try to push such, they all receive the
> same feedback and they disappear, so I have to repeat the same feedback
> to the next person... Please go through previous patches from
> @samsung.com for various subsystems.
>
> Documentation/devicetree/bindings/submitting-patches.rst
> Documentation/devicetree/bindings/writing-bindings.rst
> +other people or my talks on Devicetree
>
> Summarizing: Devicetree is for hardware, not for your driver
> hooks/quirks/needs. Describe properly and fully the hardware, not your
> driver.

Thank you Krzysztof for the explanation. I will study and explore
the possibility of integrating the stuff we want into the generic driver.

>
> Best regards,
> Krzysztof
>

[PATCH 3/3] MAINTAINERS: Add maintainer for Google USB XHCI driver

[Puma Hsu]

Add Google USB XHCI driver and maintainer.

Signed-off-by: Puma Hsu <pumahsu@google.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 960512bec428..dc0e32a3c250 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9081,6 +9081,12 @@ F:	arch/arm64/boot/dts/exynos/google/
 F:	drivers/clk/samsung/clk-gs101.c
 F:	include/dt-bindings/clock/google,gs101.h
 
+GOOGLE USB XHCI DRIVER
+M:	Puma Hsu <pumahsu@google.com>
+L:	linux-usb@vger.kernel.org
+S:	Maintained
+F:	drivers/usb/host/xhci-goog.c
+
 GPD POCKET FAN DRIVER
 M:	Hans de Goede <hdegoede@redhat.com>
 L:	platform-driver-x86@vger.kernel.org
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH 3/3] MAINTAINERS: Add maintainer for Google USB XHCI driver

[Greg KH]

On Mon, Feb 19, 2024 at 02:10:08PM +0800, Puma Hsu wrote:
> Add Google USB XHCI driver and maintainer.
> 
> Signed-off-by: Puma Hsu <pumahsu@google.com>
> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 960512bec428..dc0e32a3c250 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9081,6 +9081,12 @@ F:	arch/arm64/boot/dts/exynos/google/
>  F:	drivers/clk/samsung/clk-gs101.c
>  F:	include/dt-bindings/clock/google,gs101.h
>  
> +GOOGLE USB XHCI DRIVER
> +M:	Puma Hsu <pumahsu@google.com>
> +L:	linux-usb@vger.kernel.org
> +S:	Maintained
> +F:	drivers/usb/host/xhci-goog.c

You are not paid to look after this?  That sounds odd, can you work with
your managers to do this, otherwise this is going to be tough to do over
time, right?

thanks,

greg k-h

Re: [PATCH 3/3] MAINTAINERS: Add maintainer for Google USB XHCI driver

[Puma Hsu]

On Mon, Feb 19, 2024 at 2:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Feb 19, 2024 at 02:10:08PM +0800, Puma Hsu wrote:
> > Add Google USB XHCI driver and maintainer.
> >
> > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > ---
> >  MAINTAINERS | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 960512bec428..dc0e32a3c250 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9081,6 +9081,12 @@ F:     arch/arm64/boot/dts/exynos/google/
> >  F:   drivers/clk/samsung/clk-gs101.c
> >  F:   include/dt-bindings/clock/google,gs101.h
> >
> > +GOOGLE USB XHCI DRIVER
> > +M:   Puma Hsu <pumahsu@google.com>
> > +L:   linux-usb@vger.kernel.org
> > +S:   Maintained
> > +F:   drivers/usb/host/xhci-goog.c
>
> You are not paid to look after this?  That sounds odd, can you work with
> your managers to do this, otherwise this is going to be tough to do over
> time, right?
>
> thanks,
>
> greg k-h

I misunderstand the definitions between Supported and Maintained,
will update to Supported in next revision. Thanks for advising.