Re: [PATCH RFC v5 6/6] usb: dwc3: dwc3-exynos: add host init

[Arnd Bergmann]

On Fri, May 6, 2022 at 8:31 AM Daehwan Jung <dh10.jung@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>  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