Re: [PATCH v4 1/2] usb: dwc3: core: add support for RTK SoC custom's global register start address

[Thinh Nguyen]

On Tue, May 02, 2023, Stanley Chang wrote:
> The RTK DHC SoCs were designed, the global register address offset at
> 0x8100. The default address offset is constant at DWC3_GLOBALS_REGS_START
> (0xc100). Therefore, add the compatible name of device-tree to specify
> the SoC custom's global register start address.
> 
> Signed-off-by: Stanley Chang <stanley_chang@xxxxxxxxxxx>
> ---
>  v3 to v4 change:
> Use the compatible name to specify the global register address offset.
> If the compatible name is "snps,dwc3-rtk-soc", then the offset use 0x8100.
> Otherwise, the offset is default value 0xc100.
> 
>  v2 to v3 change:
> 1.  Fix the dtschema validation error.
> 
>  v1 to v2 change:
> 1. Change the name of the property "snps,global-regs-starting-offset".
> 2. Adjust the format of comment.
> 3. Add initial value of the global_regs_starting_offset
> 4. Remove the log of dev_info.
> ---
>  drivers/usb/dwc3/core.c | 18 +++++++++++++++---
>  drivers/usb/dwc3/core.h |  5 +++++
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 0beaab932e7d..4f69b26d7dab 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/acpi.h>
>  #include <linux/pinctrl/consumer.h>
> @@ -1793,12 +1794,17 @@ static int dwc3_probe(struct platform_device *pdev)
>  	dwc->xhci_resources[0].flags = res->flags;
>  	dwc->xhci_resources[0].name = res->name;
>  
> +	dwc->global_regs_starting_offset = (u32)(uintptr_t)
> +		    of_device_get_match_data(dev);
> +	if (!dwc->global_regs_starting_offset)
> +		dwc->global_regs_starting_offset = DWC3_GLOBALS_REGS_START;
> +
>  	/*
>  	 * Request memory region but exclude xHCI regs,
>  	 * since it will be requested by the xhci-plat driver.
>  	 */
>  	dwc_res = *res;
> -	dwc_res.start += DWC3_GLOBALS_REGS_START;
> +	dwc_res.start += dwc->global_regs_starting_offset;

I think you're overcomplicating things here.

Can we just match using compatible string as mentioned before? I believe
I suggested to use that before but I think you had issue we getting it
because it's from the parent device?

Did you try this?

	dwc_res.start = DWC3_RTK_ABC_GLOBAL_OFFSET;

	if (dev->of_node) {
		struct device_node *parent = of_get_parent(dev->of_node);

		if (of_device_is_compatible(parent, "your-compatible"))
			dwc_res.start = DWC3_RTK_ABC_GLOBAL_OFFSET;

		of_node_put(parent);
	}

>  
>  	regs = devm_ioremap_resource(dev, &dwc_res);
>  	if (IS_ERR(regs))
> @@ -2224,10 +2230,16 @@ static const struct dev_pm_ops dwc3_dev_pm_ops = {
>  #ifdef CONFIG_OF
>  static const struct of_device_id of_dwc3_match[] = {
>  	{
> -		.compatible = "snps,dwc3"
> +		.compatible = "snps,dwc3",
> +		.data = (void *)DWC3_GLOBALS_REGS_START,
> +	},
> +	{
> +		.compatible = "snps,dwc3-rtk-soc",
> +		.data = (void *)DWC3_GLOBALS_REGS_START_FOR_RTK,

Don't do this.

>  	},
>  	{
> -		.compatible = "synopsys,dwc3"
> +		.compatible = "synopsys,dwc3",
> +		.data = (void *)DWC3_GLOBALS_REGS_START,
>  	},
>  	{ },
>  };
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index d56457c02996..46557cf52f4b 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -84,6 +84,8 @@
>  #define DWC3_OTG_REGS_START		0xcc00
>  #define DWC3_OTG_REGS_END		0xccff
>  
> +#define DWC3_GLOBALS_REGS_START_FOR_RTK 0x8100
> +
>  /* Global Registers */
>  #define DWC3_GSBUSCFG0		0xc100
>  #define DWC3_GSBUSCFG1		0xc104
> @@ -1118,6 +1120,8 @@ struct dwc3_scratchpad_array {
>   * @wakeup_configured: set if the device is configured for remote wakeup.
>   * @imod_interval: set the interrupt moderation interval in 250ns
>   *			increments or 0 to disable.
> + * @global_regs_starting_offset: set the dwc3 global register start address
> + *	 and it is default at DWC3_GLOBALS_REGS_START (0xc100).
>   * @max_cfg_eps: current max number of IN eps used across all USB configs.
>   * @last_fifo_depth: last fifo depth used to determine next fifo ram start
>   *		     address.
> @@ -1334,6 +1338,7 @@ struct dwc3 {
>  	unsigned		wakeup_configured:1;
>  
>  	u16			imod_interval;
> +	u32			global_regs_starting_offset;
>  
>  	int			max_cfg_eps;
>  	int			last_fifo_depth;
> -- 
> 2.34.1
> 

Please note that this is very unique to Realtek, I'm not aware of any
other vendor that reconfigured the register offset that our IP
specified. So, it makes more sense to match using compatible string than
creating a separate property that you may be the only user that needs
it.

Thanks,
Thinh