| Message ID | 1651818679-10594-7-git-send-email-dh10.jung@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add xhci-exynos for Exynos SOC | expand |
On 06/05/2022 08:31, Daehwan Jung wrote: > This is for xHCI Host Controller driver on Exynos SOC. https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > It registers vendor ops before loading xhci platform driver. It does not explain why do you need it, why do you do it, what is this going to achieve or give us. > > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> > --- > drivers/usb/dwc3/dwc3-exynos.c | 100 ++++++++++++++++++++++++++++++++- > 1 file changed, 99 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c > index 0ecf20eeceee..c22ea5cd6ab0 100644 > --- a/drivers/usb/dwc3/dwc3-exynos.c > +++ b/drivers/usb/dwc3/dwc3-exynos.c > @@ -17,6 +17,12 @@ > #include <linux/of_platform.h> > #include <linux/regulator/consumer.h> > > +#include "core.h" > + > +#if IS_ENABLED(CONFIG_USB_XHCI_EXYNOS) This symbol does not exist at this point, so your patch does not look like correctly ordered. > +int xhci_exynos_register_vendor_ops(void); > +#endif > + > #define DWC3_EXYNOS_MAX_CLOCKS 4 > > struct dwc3_exynos_driverdata { > @@ -27,6 +33,7 @@ struct dwc3_exynos_driverdata { > > struct dwc3_exynos { > struct device *dev; > + struct dwc3 *dwc; > > const char **clk_names; > struct clk *clks[DWC3_EXYNOS_MAX_CLOCKS]; > @@ -46,12 +53,81 @@ static int dwc3_exynos_remove_child(struct device *dev, void *unused) > return 0; > } > > +#if IS_ENABLED(CONFIG_USB_XHCI_EXYNOS) > +static int dwc3_exynos_host_init(struct dwc3_exynos *exynos) > +{ > + struct dwc3 *dwc = exynos->dwc; > + struct device *dev = exynos->dev; > + struct platform_device *xhci; > + struct resource *res; > + struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); > + int ret = 0; > + > + /* Configuration xhci resources */ > + xhci_exynos_register_vendor_ops(); Why this is always being called? Runtime features should not be added like that. > + > + res = platform_get_resource(dwc3_pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(dev, "missing memory resource\n"); > + return -ENODEV; > + } > + dwc->xhci_resources[0].start = res->start; > + dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > + DWC3_XHCI_REGS_END; > + dwc->xhci_resources[0].flags = res->flags; > + dwc->xhci_resources[0].name = res->name; > + > + res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ, 0); > + if (!res) { > + dev_err(dev, "missing irq resource\n"); > + return -ENODEV; > + } > + > + dwc->xhci_resources[1].start = dwc->irq_gadget; > + dwc->xhci_resources[1].end = dwc->irq_gadget; > + dwc->xhci_resources[1].flags = res->flags; > + dwc->xhci_resources[1].name = res->name; > + > + xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO); > + if (!xhci) { > + dev_err(dwc->dev, "couldn't allocate xHCI device\n"); > + return -ENOMEM; > + } > + > + xhci->dev.parent = dwc->dev; Remove any duplicates spaces/tabs which should not be in the code (no need for indenting '='). > + ret = dma_set_mask_and_coherent(&xhci->dev, DMA_BIT_MASK(36)); > + if (ret) { > + pr_err("xhci dma set mask ret = %d\n", ret); > + return ret; > + } > + > + ret = platform_device_add_resources(xhci, dwc->xhci_resources, > + DWC3_XHCI_RESOURCES_NUM); But this should be properly indented, how checkpatch asks. > + if (ret) { > + dev_err(dwc->dev, "couldn't add resources to xHCI device\n"); > + goto err; > + } > + > + ret = platform_device_add(xhci); > + if (ret) { > + dev_err(dwc->dev, "couldn't add xHCI device\n"); > + goto err; > + } > + > + return 0; > +err: > + platform_device_put(xhci); > + return ret; > +} > +#endif > + > static int dwc3_exynos_probe(struct platform_device *pdev) > { > struct dwc3_exynos *exynos; > struct device *dev = &pdev->dev; > - struct device_node *node = dev->of_node; > + struct device_node *node = dev->of_node, *dwc3_np; > const struct dwc3_exynos_driverdata *driver_data; > + struct platform_device *dwc3_pdev; > int i, ret; > > exynos = devm_kzalloc(dev, sizeof(*exynos), GFP_KERNEL); > @@ -109,6 +185,12 @@ static int dwc3_exynos_probe(struct platform_device *pdev) > goto vdd10_err; > } > > + dwc3_np = of_get_compatible_child(node, "snps,dwc3"); > + if (!dwc3_np) { > + dev_err(dev, "failed to find dwc3 core child!\n"); Please keep messages consistent with other, so start with capital letter and do not shout. > + goto vdd33_err; > + } > + > if (node) { > ret = of_platform_populate(node, NULL, NULL, dev); > if (ret) { > @@ -121,6 +203,22 @@ static int dwc3_exynos_probe(struct platform_device *pdev) > goto populate_err; > } > > + dwc3_pdev = of_find_device_by_node(dwc3_np); > + exynos->dwc = platform_get_drvdata(dwc3_pdev); Driver should not poke into its child. You violate device layering here. No, no. This is a glue driver, not a "let's do something inside DWC3" driver. > + if (!exynos->dwc) { > + ret = -EPROBE_DEFER; > + dev_err(dev, "failed to get dwc3 core node!\n"); Again no reason for shouting. > + goto populate_err; > + } > + > +#if IS_ENABLED(CONFIG_USB_XHCI_EXYNOS) > + /* USB host initialization. */ > + ret = dwc3_exynos_host_init(exynos); > + if (ret) { > + dev_err(dev, "USB host pre-initialization fail!\n"); > + goto populate_err; > + } > +#endif > return 0; > > populate_err: Best regards, Krzysztof
On Fri, May 6, 2022 at 8:31 AM Daehwan Jung <dh10.jung@samsung.com> wrote: > > This is for xHCI Host Controller driver on Exynos SOC. > It registers vendor ops before loading xhci platform driver. > > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> > --- > drivers/usb/dwc3/dwc3-exynos.c | 100 ++++++++++++++++++++++++++++++++- > 1 file changed, 99 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c > index 0ecf20eeceee..c22ea5cd6ab0 100644 > --- a/drivers/usb/dwc3/dwc3-exynos.c > +++ b/drivers/usb/dwc3/dwc3-exynos.c > @@ -17,6 +17,12 @@ > #include <linux/of_platform.h> > #include <linux/regulator/consumer.h> > > +#include "core.h" > + > +#if IS_ENABLED(CONFIG_USB_XHCI_EXYNOS) > +int xhci_exynos_register_vendor_ops(void); > +#endif Function declarations should always be in a header file, and not guarded by an #ifdef. This particular one is probably not needed anyway if the driver is done correctly though, see below. > @@ -46,12 +53,81 @@ static int dwc3_exynos_remove_child(struct device *dev, void *unused) > return 0; > } > > +#if IS_ENABLED(CONFIG_USB_XHCI_EXYNOS) > +static int dwc3_exynos_host_init(struct dwc3_exynos *exynos) > +{ > + struct dwc3 *dwc = exynos->dwc; > + struct device *dev = exynos->dev; > + struct platform_device *xhci; > + struct resource *res; > + struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); > + int ret = 0; > + > + /* Configuration xhci resources */ > + xhci_exynos_register_vendor_ops(); > + > + res = platform_get_resource(dwc3_pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(dev, "missing memory resource\n"); > + return -ENODEV; > + } > + dwc->xhci_resources[0].start = res->start; > + dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > + DWC3_XHCI_REGS_END; > + dwc->xhci_resources[0].flags = res->flags; > + dwc->xhci_resources[0].name = res->name; > + > + res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ, 0); > + if (!res) { > + dev_err(dev, "missing irq resource\n"); > + return -ENODEV; > + } > + > + dwc->xhci_resources[1].start = dwc->irq_gadget; > + dwc->xhci_resources[1].end = dwc->irq_gadget; > + dwc->xhci_resources[1].flags = res->flags; > + dwc->xhci_resources[1].name = res->name; > + > + xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO); > + if (!xhci) { > + dev_err(dwc->dev, "couldn't allocate xHCI device\n"); > + return -ENOMEM; > + } > + > + xhci->dev.parent = dwc->dev; > + ret = dma_set_mask_and_coherent(&xhci->dev, DMA_BIT_MASK(36)); > + if (ret) { > + pr_err("xhci dma set mask ret = %d\n", ret); > + return ret; > + } This looks like you have the abstraction backwards from what normal drivers do. If you need a specialization of a driver that already exists, create a new driver module with a platform_driver that matches the specialized of_device_id, and have it call into the more general driver, do avoid having the general driver know about the specializations. Allocating a platform_device and making it DMA capable doesn't generally work correctly, and misses the IOMMU setup, so make sure you have a device node for it instead and probe it from DT. Arnd
On Fri, May 06, 2022 at 08:45:39AM +0200, Krzysztof Kozlowski wrote: > On 06/05/2022 08:31, Daehwan Jung wrote: > > This is for xHCI Host Controller driver on Exynos SOC. > > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > > It registers vendor ops before loading xhci platform driver. > > It does not explain why do you need it, why do you do it, what is this > going to achieve or give us. Nver mind that the whole concept if "vendor" ops doesnt make any sense whatsoever, as I've repeatedly pointed out. The "vendor" is completely irrelevant for a Linux driver.
diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c index 0ecf20eeceee..c22ea5cd6ab0 100644 --- a/drivers/usb/dwc3/dwc3-exynos.c +++ b/drivers/usb/dwc3/dwc3-exynos.c @@ -17,6 +17,12 @@ #include <linux/of_platform.h> #include <linux/regulator/consumer.h> +#include "core.h" + +#if IS_ENABLED(CONFIG_USB_XHCI_EXYNOS) +int xhci_exynos_register_vendor_ops(void); +#endif + #define DWC3_EXYNOS_MAX_CLOCKS 4 struct dwc3_exynos_driverdata { @@ -27,6 +33,7 @@ struct dwc3_exynos_driverdata { struct dwc3_exynos { struct device *dev; + struct dwc3 *dwc; const char **clk_names; struct clk *clks[DWC3_EXYNOS_MAX_CLOCKS]; @@ -46,12 +53,81 @@ static int dwc3_exynos_remove_child(struct device *dev, void *unused) return 0; } +#if IS_ENABLED(CONFIG_USB_XHCI_EXYNOS) +static int dwc3_exynos_host_init(struct dwc3_exynos *exynos) +{ + struct dwc3 *dwc = exynos->dwc; + struct device *dev = exynos->dev; + struct platform_device *xhci; + struct resource *res; + struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); + int ret = 0; + + /* Configuration xhci resources */ + xhci_exynos_register_vendor_ops(); + + res = platform_get_resource(dwc3_pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(dev, "missing memory resource\n"); + return -ENODEV; + } + dwc->xhci_resources[0].start = res->start; + dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + + DWC3_XHCI_REGS_END; + dwc->xhci_resources[0].flags = res->flags; + dwc->xhci_resources[0].name = res->name; + + res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ, 0); + if (!res) { + dev_err(dev, "missing irq resource\n"); + return -ENODEV; + } + + dwc->xhci_resources[1].start = dwc->irq_gadget; + dwc->xhci_resources[1].end = dwc->irq_gadget; + dwc->xhci_resources[1].flags = res->flags; + dwc->xhci_resources[1].name = res->name; + + xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO); + if (!xhci) { + dev_err(dwc->dev, "couldn't allocate xHCI device\n"); + return -ENOMEM; + } + + xhci->dev.parent = dwc->dev; + ret = dma_set_mask_and_coherent(&xhci->dev, DMA_BIT_MASK(36)); + if (ret) { + pr_err("xhci dma set mask ret = %d\n", ret); + return ret; + } + + ret = platform_device_add_resources(xhci, dwc->xhci_resources, + DWC3_XHCI_RESOURCES_NUM); + if (ret) { + dev_err(dwc->dev, "couldn't add resources to xHCI device\n"); + goto err; + } + + ret = platform_device_add(xhci); + if (ret) { + dev_err(dwc->dev, "couldn't add xHCI device\n"); + goto err; + } + + return 0; +err: + platform_device_put(xhci); + return ret; +} +#endif + static int dwc3_exynos_probe(struct platform_device *pdev) { struct dwc3_exynos *exynos; struct device *dev = &pdev->dev; - struct device_node *node = dev->of_node; + struct device_node *node = dev->of_node, *dwc3_np; const struct dwc3_exynos_driverdata *driver_data; + struct platform_device *dwc3_pdev; int i, ret; exynos = devm_kzalloc(dev, sizeof(*exynos), GFP_KERNEL); @@ -109,6 +185,12 @@ static int dwc3_exynos_probe(struct platform_device *pdev) goto vdd10_err; } + dwc3_np = of_get_compatible_child(node, "snps,dwc3"); + if (!dwc3_np) { + dev_err(dev, "failed to find dwc3 core child!\n"); + goto vdd33_err; + } + if (node) { ret = of_platform_populate(node, NULL, NULL, dev); if (ret) { @@ -121,6 +203,22 @@ static int dwc3_exynos_probe(struct platform_device *pdev) goto populate_err; } + dwc3_pdev = of_find_device_by_node(dwc3_np); + exynos->dwc = platform_get_drvdata(dwc3_pdev); + if (!exynos->dwc) { + ret = -EPROBE_DEFER; + dev_err(dev, "failed to get dwc3 core node!\n"); + goto populate_err; + } + +#if IS_ENABLED(CONFIG_USB_XHCI_EXYNOS) + /* USB host initialization. */ + ret = dwc3_exynos_host_init(exynos); + if (ret) { + dev_err(dev, "USB host pre-initialization fail!\n"); + goto populate_err; + } +#endif return 0; populate_err:
This is for xHCI Host Controller driver on Exynos SOC. It registers vendor ops before loading xhci platform driver. Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> --- drivers/usb/dwc3/dwc3-exynos.c | 100 ++++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 1 deletion(-)