Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <CGME20220506063340epcas2p4c9d88670f9be952f3637e3a545a7d1da@epcas2p4.samsung.com>
 <1651818679-10594-1-git-send-email-dh10.jung@samsung.com> <1651818679-10594-7-git-send-email-dh10.jung@samsung.com>
In-Reply-To: <1651818679-10594-7-git-send-email-dh10.jung@samsung.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 6 May 2022 09:51:46 +0200
Message-ID: <CAK8P3a22wWExGymOC__KxgMLsh4AMdnRRKUdgD=qqmj6JQZ1KA@mail.gmail.com>
Subject: Re: [PATCH RFC v5 6/6] usb: dwc3: dwc3-exynos: add host init
To:     Daehwan Jung <dh10.jung@samsung.com>
Cc:     Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Mathias Nyman <mathias.nyman@intel.com>,
        Lukas Bulwahn <lukas.bulwahn@gmail.com>,
        Tony Lindgren <tony@atomide.com>,
        Juergen Gross <jgross@suse.com>, Arnd Bergmann <arnd@arndb.de>,
        Matthias Kaehlcke <mka@chromium.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:DESIGNWARE USB3 DRD IP DRIVER" <linux-usb@vger.kernel.org>,
        "moderated list:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM ARCHITECTURES" 
        <linux-arm-kernel@lists.infradead.org>,
        "open list:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM ARCHITECTURES" 
        <linux-samsung-soc@vger.kernel.org>, sc.suh@samsung.com,
        taehyun cho <taehyun.cho@samsung.com>, jh0801.jung@samsung.com,
        eomji.oh@samsung.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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