Re: [PATCH RFC v5 5/6] usb: host: add xhci-exynos driver

[Krzysztof Kozlowski]

On 06/05/2022 08:31, Daehwan Jung wrote:
> This driver is for Samsung Exynos xHCI host conroller. It works based on

https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> xhci platform driver and extends functions by xhci hooks and overrides.
> Vendor ops(xhci hooks) should be mapped before probing driver.
> It overrides functions of hc driver on vendor init.
> 
> It supports USB Audio offload with Co-processor. It only cares DCBAA,
> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
> on specific address with xhci hooks. Co-processor could use them directly
> without xhci driver after then.
> 
> Signed-off-by: Daehwan Jung <dh10.jung@xxxxxxxxxxx>
> ---
>  drivers/usb/host/Kconfig       |   8 +
>  drivers/usb/host/Makefile      |   1 +
>  drivers/usb/host/xhci-exynos.c | 775 +++++++++++++++++++++++++++++++++

This is your fifth version and *it still does not compile*. Can you
compile your changes before sending them? It saves reviewer's time.

/usr/bin/aarch64-linux-gnu-ld: drivers/usb/dwc3/dwc3-exynos.o: in
function `dwc3_exynos_probe':

dwc3-exynos.c:(.text+0x470): undefined reference to
`xhci_exynos_register_vendor_ops'



>  3 files changed, 784 insertions(+)
>  create mode 100644 drivers/usb/host/xhci-exynos.c
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 682b3d2da623..ccafcd9b4212 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -104,6 +104,14 @@ config USB_XHCI_TEGRA
>  	  Say 'Y' to enable the support for the xHCI host controller
>  	  found in NVIDIA Tegra124 and later SoCs.
>  
> +config USB_XHCI_EXYNOS
> +	tristate "xHCI support for Samsung Exynos SoC Series"

XHCI was supported before, wasn't it? If yes, this title does not make
really sense.

You need to provide proper title explaining this option.

> +	depends on USB_XHCI_PLATFORM
> +	depends on ARCH_EXYNOS || COMPILE_TEST
> +	help
> +	  Say 'Y' to enable the support for the xHCI host controller
> +	  found in Samsung Exynos SoCs.

The same.

> +
>  endif # USB_XHCI_HCD
>  
>  config USB_EHCI_BRCMSTB
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index 2948983618fb..300f22b6eb1b 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -86,3 +86,4 @@ obj-$(CONFIG_USB_HCD_SSB)	+= ssb-hcd.o
>  obj-$(CONFIG_USB_FOTG210_HCD)	+= fotg210-hcd.o
>  obj-$(CONFIG_USB_MAX3421_HCD)	+= max3421-hcd.o
>  obj-$(CONFIG_USB_XEN_HCD)	+= xen-hcd.o
> +obj-$(CONFIG_USB_XHCI_EXYNOS)	+= xhci-exynos.o
> diff --git a/drivers/usb/host/xhci-exynos.c b/drivers/usb/host/xhci-exynos.c
> new file mode 100644
> index 000000000000..5318a51ac5ee
> --- /dev/null
> +++ b/drivers/usb/host/xhci-exynos.c
> @@ -0,0 +1,775 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * xhci-exynos.c - xHCI host controller driver platform Bus Glue for Exynos.
> + *
> + * Copyright (C) 2022 Samsung Electronics Incorporated - http://www.samsung.com
> + * Author: Daehwan Jung <dh10.jung@xxxxxxxxxxx>
> + *
> + * A lot of code borrowed from the Linux xHCI driver.

Then please keep original copyrights, as a derivative work.

> + */
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +
> +#include "xhci.h"
> +#include "xhci-plat.h"
> +
> +/* EXYNOS uram memory map */
> +#define EXYNOS_URAM_ABOX_EVT_RING_ADDR	0x02a00000

Are these SoC memory map addresses? If yes, they should not be
hard-coded in the driver.

> +#define EXYNOS_URAM_ISOC_OUT_RING_ADDR	0x02a01000
> +#define EXYNOS_URAM_ISOC_IN_RING_ADDR	0x02a02000
> +#define EXYNOS_URAM_DEVICE_CTX_ADDR	0x02a03000
> +#define EXYNOS_URAM_DCBAA_ADDR		0x02a03880
> +#define EXYNOS_URAM_ABOX_ERST_SEG_ADDR	0x02a03C80
> +#define EXYNOS_URAM_CTX_SIZE		2112
> +
> +int xhci_exynos_register_vendor_ops(void);
> +
> +struct xhci_hcd_exynos {
> +	struct	xhci_intr_reg __iomem *ir_set_audio;
> +
> +	struct xhci_ring	*event_ring_audio;
> +	struct xhci_erst	erst_audio;

Why "xHCI support for Samsung Exynos SoC Series" comes specific to
audio? Isn't XHCI related to USB, so a Universal use? Cannot XHCI driver
support mass storage?

> +
> +	struct device		*dev;
> +	struct usb_hcd		*hcd;
> +	struct usb_hcd		*shared_hcd;
> +
> +	struct wakeup_source *main_wakelock; /* Wakelock for HS HCD */
> +	struct wakeup_source *shared_wakelock; /* Wakelock for SS HCD */

None of other USB drivers use wakeloks so why is this one special?

> +
> +	u32 in_ep;
> +	u32 out_ep;
> +	u32 in_deq;
> +	u32 out_deq;
> +
> +	/* This flag is used to check first allocation for URAM */
> +	bool			exynos_uram_ctx_alloc;
> +	bool			exynos_uram_isoc_out_alloc;
> +	bool			exynos_uram_isoc_in_alloc;

This indentation is really troubling me - just few lines above, you
don't indent variables. Here you indent. You need to clean up your
driver before submitting. Run checkpatch --strict and fix all the
issues. Add const to all static variables and most of pointed memory.
Remove any inconsistencies. Remove double blank lines. Fix indentation.

Best regards,
Krzysztof