| Message ID | 20240219061008.1761102-3-pumahsu@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: xhci: Add support for Google XHCI controller | expand |
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
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 >
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
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 >
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");
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