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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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