[v2,1/2] usb: dwc3: core: add support for remapping 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>
---
 drivers/usb/dwc3/core.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

On Wed, Apr 12, 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>
> ---

Please note what changed in v2 after the --- line. Also why did you
split the previous series in 2 now? It'll be confusing for us
maintainers.

>  drivers/usb/dwc3/core.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..96d3e634ebbf 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1785,6 +1785,24 @@ 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->of_node) {
> +		int global_regs_starting_offset = 0;
> +
> +		device_property_read_u32(dev, "snps,global-regs-starting-offset",
> +				 &global_regs_starting_offset);

I suggested to use compatible string instead since this isn't common and
only unique to your platform. Any reason we shouldn't do that?

ie. something like this:

if (dev->of_node && of_device_is_compatiable(dev->of_node, "your-platform"))
	dwc_res.start += your_platform_offset;
else
	dwc_res.start = DWC3_GLOBALS_REGS_START;

> +		if (global_regs_starting_offset) {
> +			dwc_res.start -= DWC3_GLOBALS_REGS_START;
> +			dwc_res.start += global_regs_starting_offset;
> +			dev_info(dev,
> +			    "dwc3 global register start address from 0x%x to end 0x%x\n",
> +			    (int)dwc_res.start, (int)dwc_res.end);

Don't use dev_info when it's not needed. What you have here is for your
debug purpose. Please remove.

> +		}
> +	}
> +
>  	regs = devm_ioremap_resource(dev, &dwc_res);
>  	if (IS_ERR(regs))
>  		return PTR_ERR(regs);
> -- 
> 2.34.1
> 

Thanks,
Thinh

> 
> On Wed, Apr 12, 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>
> > ---
> 
> Please note what changed in v2 after the --- line. Also why did you split the
> previous series in 2 now? It'll be confusing for us maintainers.
> 
Do you mean that to split the part of "remapping global register start address and
the part of " snps,parkmode-disable-hs-quirk"?

They are different series. I just submit them at the same time.

> >  drivers/usb/dwc3/core.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > 476b63618511..96d3e634ebbf 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1785,6 +1785,24 @@ 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->of_node) {
> > +             int global_regs_starting_offset = 0;
> > +
> > +             device_property_read_u32(dev,
> "snps,global-regs-starting-offset",
> > +                              &global_regs_starting_offset);
> 
> I suggested to use compatible string instead since this isn't common and only
> unique to your platform. Any reason we shouldn't do that?
> 
> ie. something like this:
> 
> if (dev->of_node && of_device_is_compatiable(dev->of_node,
> "your-platform"))
>         dwc_res.start += your_platform_offset; else
>         dwc_res.start = DWC3_GLOBALS_REGS_START;
> 

I will try this suggestion.

Thanks,
Stanley

On 13/04/2023 04:53, Stanley Chang[昌育德] wrote:
> 
>>
>> On Tue, Apr 11, 2023 at 10:30 PM Stanley Chang
>> <stanley_chang@realtek.com> wrote:
>>>
>>> Add a new 'snps,global-regs-starting-offset' 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..5cbf3b7ded04 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,global-regs-starting-offset:
>>> +    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.
>>
>> We already have 'reg' or using a specific compatible to handle differences. Use
>> one of those, not a custom property. Generally, properties should be used for
>> things that vary per board, not fixed in a given SoC.
>>
>> Rob
>>
> 
> The default offset is fixed by macro DWC3_GLOBALS_REGS_START, and it is not specified by reg.

Are you saying that reg points to XHCI registers and the gap between
them and DWC3_GLOBALS_REGS_START is different on some implementations of
this IP?

> The dwc3/core is a general driver for every dwc3 IP of SoCs,
> and vendor's definition and compatible should specify on its parent.

Not entirely. It's how currently it is written, but not necessarily
correct representation. The parent is only glue part which for some
non-IP resources.

> If we add a specific compatible to dwc3/core driver, then it will break this rule.

What rule? The rule is to have specific compatibles, so now you are
breaking it.

> Therefore, I use a property to adjust this offset. 
> If no define this property, it will use default offset. So it will not affect other board.


Best regards,
Krzysztof

> On 13/04/2023 04:53, Stanley Chang[昌育德] wrote:
> >
> >>
> >> On Tue, Apr 11, 2023 at 10:30 PM Stanley Chang
> >> <stanley_chang@realtek.com> wrote:
> >>>
> >>> Add a new 'snps,global-regs-starting-offset' 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..5cbf3b7ded04 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,global-regs-starting-offset:
> >>> +    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.
> >>
> >> We already have 'reg' or using a specific compatible to handle
> >> differences. Use one of those, not a custom property. Generally,
> >> properties should be used for things that vary per board, not fixed in a given
> SoC.
> >>
> >> Rob
> >>
> >
> > The default offset is fixed by macro DWC3_GLOBALS_REGS_START, and it is
> not specified by reg.
> 
> Are you saying that reg points to XHCI registers and the gap between them and
> DWC3_GLOBALS_REGS_START is different on some implementations of this IP?

Yes.

> > The dwc3/core is a general driver for every dwc3 IP of SoCs, and
> > vendor's definition and compatible should specify on its parent.
> 
> Not entirely. It's how currently it is written, but not necessarily correct
> representation. The parent is only glue part which for some non-IP resources.

I agree. 
I think this offset belongs to the IP resource.
But it is fixed value on dwc3/core driver.
Therefore, I had to add this patch to adjust it.

> > If we add a specific compatible to dwc3/core driver, then it will break this
> rule.
> 
> What rule? The rule is to have specific compatibles, so now you are breaking it.
> 
I just don't want dwc3/core to look like a specific Realtek driver.
If I add compatible to our platform, then apply this offset.
For example,
@@ -2046,6 +2046,9 @@ static const struct of_device_id of_dwc3_match[] = {
        {
                .compatible = "synopsys,dwc3"
        },
+       {
+               .compatible = "realtek,dwc3"
+       },
        { },
 };

Why not use a property of the device tree to specify this offset?
It will be more general. Other vendor IPs can also use this option if desired.
For example,
@@ -123,7 +123,7 @@ port0_dwc3: dwc3_u2drd@98020000 {
                        compatible = "synopsys,dwc3", "syscon";
                        reg = <0x98020000 0x9000>;
                        interrupts = <0 95 4>;
+                       snps,global-regs-starting-offset = <0x8100>;
                        usb-phy = <&dwc3_u2drd_usb2phy>;
                        dr_mode = "host";
                        snps,dis_u2_susphy_quirk;