RE: [PATCH v2 2/2] dt-bindings: usb: snps,dwc3: Add 'snps,global-regs-starting-offset' quirk

[Stanley Chang[昌育德]]

> On 13/04/2023 04:53, Stanley Chang[昌育德] wrote:
> >
> >>
> >> On Tue, Apr 11, 2023 at 10:30 PM Stanley Chang
> >> <stanley_chang@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>> ---
> >>>  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;