[PATCH v1 1/2] usb: dwc3: core: add support for remapping global register start address

[PATCH v1 1/2] usb: dwc3: core: add support for remapping global register start address

[Stanley Chang]

The RTK DHC SoCs were designed the global register address offset at 0x8100.
The default address is at DWC3_GLOBALS_REGS_START (0xc100).
Therefore, add the property of device-tree to adjust this start address.

Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
---
 drivers/usb/dwc3/core.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 476b63618511..771b35449376 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1785,6 +1785,23 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc_res = *res;
 	dwc_res.start += DWC3_GLOBALS_REGS_START;
 
+	/* For some dwc3 controller, the dwc3 global register start address is
+	 * not at DWC3_GLOBALS_REGS_START (0xc100).
+	 */
+	if (dev) {
+		int fixed_dwc3_globals_regs_start;
+
+		device_property_read_u32(dev, "snps,fixed_dwc3_globals_regs_start",
+				 &fixed_dwc3_globals_regs_start);
+		if (fixed_dwc3_globals_regs_start) {
+			dwc_res.start -= DWC3_GLOBALS_REGS_START;
+			dwc_res.start += fixed_dwc3_globals_regs_start;
+			dev_info(dev,
+			    "fixed dwc3 globals register start address from 0x%x to end 0x%x\n",
+			    (int)dwc_res.start, (int)dwc_res.end);
+		}
+	}
+
 	regs = devm_ioremap_resource(dev, &dwc_res);
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
-- 
2.34.1

[PATCH v1 2/2] dt-bindings: usb: snps,dwc3: Add 'snps,fixed_dwc3_globals_regs_start' quirk

[Stanley Chang]

Add a new 'snps,fixed_dwc3_globals_regs_start' DT to dwc3 core to remap
the global register start address

The RTK DHC SoCs were designed the global register address offset at 0x8100.
The default address is at DWC3_GLOBALS_REGS_START (0xc100).
Therefore, add the property of device-tree to adjust this start address.

Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index be36956af53b..a5599d977db6 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -359,6 +359,13 @@ properties:
     items:
       enum: [1, 4, 8, 16, 32, 64, 128, 256]
 
+  snps,fixed_dwc3_globals_regs_start:
+    description:
+      value for remapping global register start address. For some dwc3
+      controller, the dwc3 global register start address is not at
+      default DWC3_GLOBALS_REGS_START (0xc100). This property is added to
+      adjust the address.
+
   port:
     $ref: /schemas/graph.yaml#/properties/port
     description:
-- 
2.34.1

Re: [PATCH v1 1/2] usb: dwc3: core: add support for remapping global register start address

[Thinh Nguyen]

On Fri, Apr 07, 2023, Stanley Chang wrote:
> The RTK DHC SoCs were designed the global register address offset at 0x8100.
> The default address is at DWC3_GLOBALS_REGS_START (0xc100).
> Therefore, add the property of device-tree to adjust this start address.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
>  drivers/usb/dwc3/core.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..771b35449376 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1785,6 +1785,23 @@ static int dwc3_probe(struct platform_device *pdev)
>  	dwc_res = *res;
>  	dwc_res.start += DWC3_GLOBALS_REGS_START;
>  
> +	/* For some dwc3 controller, the dwc3 global register start address is
> +	 * not at DWC3_GLOBALS_REGS_START (0xc100).
> +	 */

Please use this comment style block:
 /*
  * a b c
  * d e f
  */

> +	if (dev) {

Why do we need this if (dev) check? When would this not be the case?

> +		int fixed_dwc3_globals_regs_start;

Need to initialize this in case you get bogus value when the property is
not defined.

> +
> +		device_property_read_u32(dev, "snps,fixed_dwc3_globals_regs_start",

Property name should be using "-" instead of "_". Also can we rename it
to "snps,global-regs-starting-offset"

Thanks,
Thinh

> +				 &fixed_dwc3_globals_regs_start);
> +		if (fixed_dwc3_globals_regs_start) {
> +			dwc_res.start -= DWC3_GLOBALS_REGS_START;
> +			dwc_res.start += fixed_dwc3_globals_regs_start;
> +			dev_info(dev,
> +			    "fixed dwc3 globals register start address from 0x%x to end 0x%x\n",
> +			    (int)dwc_res.start, (int)dwc_res.end);
> +		}
> +	}
> +
>  	regs = devm_ioremap_resource(dev, &dwc_res);
>  	if (IS_ERR(regs))
>  		return PTR_ERR(regs);
> -- 
> 2.34.1
> 

Re: [PATCH v1 2/2] dt-bindings: usb: snps,dwc3: Add 'snps,fixed_dwc3_globals_regs_start' quirk

[Thinh Nguyen]

On Fri, Apr 07, 2023, Stanley Chang wrote:
> Add a new 'snps,fixed_dwc3_globals_regs_start' DT to dwc3 core to remap
> the global register start address
> 
> The RTK DHC SoCs were designed the global register address offset at 0x8100.
> The default address is at DWC3_GLOBALS_REGS_START (0xc100).
> Therefore, add the property of device-tree to adjust this start address.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index be36956af53b..a5599d977db6 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -359,6 +359,13 @@ properties:
>      items:
>        enum: [1, 4, 8, 16, 32, 64, 128, 256]
>  
> +  snps,fixed_dwc3_globals_regs_start:
> +    description:
> +      value for remapping global register start address. For some dwc3
> +      controller, the dwc3 global register start address is not at
> +      default DWC3_GLOBALS_REGS_START (0xc100). This property is added to
> +      adjust the address.
> +
>    port:
>      $ref: /schemas/graph.yaml#/properties/port
>      description:
> -- 
> 2.34.1
> 

This isn't common. Can we check through compatible string instead?

Thanks,
Thinh

RE: [PATCH v1 2/2] dt-bindings: usb: snps,dwc3: Add 'snps,fixed_dwc3_globals_regs_start' quirk

[Stanley Chang[昌育德]]

> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > index be36956af53b..a5599d977db6 100644
> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > @@ -359,6 +359,13 @@ properties:
> >      items:
> >        enum: [1, 4, 8, 16, 32, 64, 128, 256]
> >
> > +  snps,fixed_dwc3_globals_regs_start:
> > +    description:
> > +      value for remapping global register start address. For some dwc3
> > +      controller, the dwc3 global register start address is not at
> > +      default DWC3_GLOBALS_REGS_START (0xc100). This property is
> added to
> > +      adjust the address.
> > +
> >    port:
> >      $ref: /schemas/graph.yaml#/properties/port
> >      description:
> > --
> > 2.34.1
> >
> 
> This isn't common. Can we check through compatible string instead?
> 
> Thanks,
> Thinh

I revise this string in next version.
Thanks,
Stanley

RE: [PATCH v1 1/2] usb: dwc3: core: add support for remapping global register start address

[Stanley Chang[昌育德]]

> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > 476b63618511..771b35449376 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1785,6 +1785,23 @@ static int dwc3_probe(struct platform_device
> *pdev)
> >       dwc_res = *res;
> >       dwc_res.start += DWC3_GLOBALS_REGS_START;
> >
> > +     /* For some dwc3 controller, the dwc3 global register start address is
> > +      * not at DWC3_GLOBALS_REGS_START (0xc100).
> > +      */
> 
> Please use this comment style block:
>  /*
>   * a b c
>   * d e f
>   */
> 

I will follow this rule.

> > +     if (dev) {
> 
> Why do we need this if (dev) check? When would this not be the case?

I want the variable "fixed_dwc3_globals_regs_start" to be a local variable. 
So I added an if statement.
I can modify it to "if (dev->of_node)" which looks more make sense.

> 
> > +             int fixed_dwc3_globals_regs_start;
> 
> Need to initialize this in case you get bogus value when the property is not
> defined.

Thanks. I will add the initial value.

> 
> > +
> > +             device_property_read_u32(dev,
> > + "snps,fixed_dwc3_globals_regs_start",
> 
> Property name should be using "-" instead of "_". Also can we rename it to
> "snps,global-regs-starting-offset"
> 
> Thanks,
> Thinh
> 

I will rename as "snps,global-regs-starting-offset" in next version.

Thanks a lot of.
Stanley

Re: [PATCH v1 1/2] usb: dwc3: core: add support for remapping global register start address

[Thinh Nguyen]

On Mon, Apr 10, 2023, Stanley Chang[昌育德] wrote:
> 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > > 476b63618511..771b35449376 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1785,6 +1785,23 @@ static int dwc3_probe(struct platform_device
> > *pdev)
> > >       dwc_res = *res;
> > >       dwc_res.start += DWC3_GLOBALS_REGS_START;
> > >
> > > +     /* For some dwc3 controller, the dwc3 global register start address is
> > > +      * not at DWC3_GLOBALS_REGS_START (0xc100).
> > > +      */
> > 
> > Please use this comment style block:
> >  /*
> >   * a b c
> >   * d e f
> >   */
> > 
> 
> I will follow this rule.
> 
> > > +     if (dev) {
> > 
> > Why do we need this if (dev) check? When would this not be the case?
> 
> I want the variable "fixed_dwc3_globals_regs_start" to be a local variable. 
> So I added an if statement.
> I can modify it to "if (dev->of_node)" which looks more make sense.

Why? If it's within this function, it's a local variable. Don't create
arbitrary condition just to limit the scope of the variable.

Thanks,
Thinh

> 
> > 
> > > +             int fixed_dwc3_globals_regs_start;
> > 
> > Need to initialize this in case you get bogus value when the property is not
> > defined.
> 
> Thanks. I will add the initial value.
> 
> > 
> > > +
> > > +             device_property_read_u32(dev,
> > > + "snps,fixed_dwc3_globals_regs_start",
> > 
> > Property name should be using "-" instead of "_". Also can we rename it to
> > "snps,global-regs-starting-offset"
> > 
> > Thanks,
> > Thinh
> > 
> 
> I will rename as "snps,global-regs-starting-offset" in next version.
> 
> Thanks a lot of.
> Stanley

RE: [PATCH v1 1/2] usb: dwc3: core: add support for remapping global register start address

[Stanley Chang[昌育德]]

> > > > +     if (dev) {
> > >
> > > Why do we need this if (dev) check? When would this not be the case?
> >
> > I want the variable "fixed_dwc3_globals_regs_start" to be a local variable.
> > So I added an if statement.
> > I can modify it to "if (dev->of_node)" which looks more make sense.
> 
> Why? If it's within this function, it's a local variable. Don't create arbitrary
> condition just to limit the scope of the variable.
> 
> Thanks,
> Thinh
> 

I agree that don't create arbitrary conditions just to limit the scope of variables.
The property "snps,global-regs-starting-offset" is in the device tree,
so checking dev->of_node exists is a must.

Thanks,
Stanley