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

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(-)

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.