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
