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

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

[Stanley Chang[昌育德]]

> On 12/04/2023 05:30, Stanley Chang 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>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people and lists
> to CC.  It might happen, that command when run on an older kernel, gives
> you outdated entries.  Therefore please be sure you base your patches on
> recent Linux kernel.
> 
> Since you skipped lists used in automated check, that's
> unfortunately: NAK
> 
> 
> > ---
> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> Best regards,
> Krzysztof
> 
> 
CC more maintainers by using scripts/get_maintainers.pl

Thanks,
Stanley

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

[Krzysztof Kozlowski]

On 12/04/2023 13:11, Stanley Chang[昌育德] wrote:
>> On 12/04/2023 05:30, Stanley Chang 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>
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people and lists
>> to CC.  It might happen, that command when run on an older kernel, gives
>> you outdated entries.  Therefore please be sure you base your patches on
>> recent Linux kernel.
>>
>> Since you skipped lists used in automated check, that's
>> unfortunately: NAK
>>
>>
>>> ---
>>>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>> Best regards,
>> Krzysztof
>>
>>
> CC more maintainers by using scripts/get_maintainers.pl

This does not work like this. It has to appear in the patchwork and be
tested by automated tools. Please resend.

Best regards,
Krzysztof

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

[Stanley Chang[昌育德]]

> 
> 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.
The dwc3/core is a general driver for every dwc3 IP of SoCs,
and vendor's definition and compatible should specify on its parent.
If we add a specific compatible to dwc3/core driver, then it will break this rule.
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.

Thanks,
Stanley

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

[Stanley Chang]

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>
---
 v1 to v2 change:
1. Change the name of the property "snps,global-regs-starting-offset".
---
 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.
+
   port:
     $ref: /schemas/graph.yaml#/properties/port
     description:
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 13/04/2023 06:25, Stanley Chang 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>
> ---
>  v1 to v2 change:
> 1. Change the name of the property "snps,global-regs-starting-offset".
> ---

Didn't you got already comment for this patch? How did you implement it?

Also, I asked you multiple times:

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

I don't understand why you ignore this.

NAK, patch is not correct.

Best regards,
Krzysztof

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

[Rob Herring]

On Thu, 13 Apr 2023 12:25:03 +0800, Stanley Chang 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>
> ---
>  v1 to v2 change:
> 1. Change the name of the property "snps,global-regs-starting-offset".
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,global-regs-starting-offset: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,global-regs-starting-offset: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,global-regs-starting-offset: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230413042503.4047-1-stanley_chang@realtek.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Stanley Chang[昌育德]]

> On 13/04/2023 06:25, Stanley Chang 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>
> > ---
> >  v1 to v2 change:
> > 1. Change the name of the property "snps,global-regs-starting-offset".
> > ---
> 
> Didn't you got already comment for this patch? How did you implement it?
> 
> Also, I asked you multiple times:
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people and lists
> to CC.  It might happen, that command when run on an older kernel, gives
> you outdated entries.  Therefore please be sure you base your patches on
> recent Linux kernel.
> 
> I don't understand why you ignore this.
> 
> NAK, patch is not correct.
> 
> Best regards,
> Krzysztof
> 

Thank you for your patient guidance.
Because I'm not familiar with the review process and didn't use scripts/get_maintainers.pl properly in the initial email thread.
Therefore, this series of errors was caused. Sorry for the confusion.
Now I know how to use the script properly.
After correcting the maintainer's suggestion, I'll restart a new email thread and review again.

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

[Krzysztof Kozlowski]

On 13/04/2023 16:58, Stanley Chang[昌育德] wrote:
>> On 13/04/2023 06:25, Stanley Chang 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>
>>> ---
>>>  v1 to v2 change:
>>> 1. Change the name of the property "snps,global-regs-starting-offset".
>>> ---
>>
>> Didn't you got already comment for this patch? How did you implement it?
>>
>> Also, I asked you multiple times:
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people and lists
>> to CC.  It might happen, that command when run on an older kernel, gives
>> you outdated entries.  Therefore please be sure you base your patches on
>> recent Linux kernel.
>>
>> I don't understand why you ignore this.
>>
>> NAK, patch is not correct.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Thank you for your patient guidance.
> Because I'm not familiar with the review process and didn't use scripts/get_maintainers.pl properly in the initial email thread.
> Therefore, this series of errors was caused. Sorry for the confusion.
> Now I know how to use the script properly.
> After correcting the maintainer's suggestion, I'll restart a new email thread and review again.

Did you respond to feedback you got about the property? Did reviewer
agreed on your view after your feedback?

If not, then why resending this patch?

Best regards,
Krzysztof

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

[Stanley Chang[昌育德]]

> >> Didn't you got already comment for this patch? How did you implement it?
> >>
> >> Also, I asked you multiple times:
> >>
> >> Please use scripts/get_maintainers.pl to get a list of necessary
> >> people and lists to CC.  It might happen, that command when run on an
> >> older kernel, gives you outdated entries.  Therefore please be sure
> >> you base your patches on recent Linux kernel.
> >>
> >> I don't understand why you ignore this.
> >>
> >> NAK, patch is not correct.
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >
> > Thank you for your patient guidance.
> > Because I'm not familiar with the review process and didn't use
> scripts/get_maintainers.pl properly in the initial email thread.
> > Therefore, this series of errors was caused. Sorry for the confusion.
> > Now I know how to use the script properly.
> > After correcting the maintainer's suggestion, I'll restart a new email thread
> and review again.
> 
> Did you respond to feedback you got about the property? Did reviewer agreed
> on your view after your feedback?
> 
> If not, then why resending this patch?
> 

1. Because you said, "This patch is incorrect". And I won't be cc'ing the proper maintainer.
I think I need to restart a new review process.
2. Modify the previous reviewer's comments and fix the dtschema validation error.

Am I misunderstanding what you mean?
Can I keep reviewing this patch on this email thread until consensus is reached with the reviewers?

Thanks,
Stanley

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

[Krzysztof Kozlowski]

On 14/04/2023 04:12, Stanley Chang[昌育德] wrote:
> 
>>>> Didn't you got already comment for this patch? How did you implement it?
>>>>
>>>> Also, I asked you multiple times:
>>>>
>>>> Please use scripts/get_maintainers.pl to get a list of necessary
>>>> people and lists to CC.  It might happen, that command when run on an
>>>> older kernel, gives you outdated entries.  Therefore please be sure
>>>> you base your patches on recent Linux kernel.
>>>>
>>>> I don't understand why you ignore this.
>>>>
>>>> NAK, patch is not correct.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Thank you for your patient guidance.
>>> Because I'm not familiar with the review process and didn't use
>> scripts/get_maintainers.pl properly in the initial email thread.
>>> Therefore, this series of errors was caused. Sorry for the confusion.
>>> Now I know how to use the script properly.
>>> After correcting the maintainer's suggestion, I'll restart a new email thread
>> and review again.
>>
>> Did you respond to feedback you got about the property? Did reviewer agreed
>> on your view after your feedback?
>>
>> If not, then why resending this patch?
>>
> 
> 1. Because you said, "This patch is incorrect". And I won't be cc'ing the proper maintainer.
> I think I need to restart a new review process.
> 2. Modify the previous reviewer's comments and fix the dtschema validation error.
> 
> Am I misunderstanding what you mean?
> Can I keep reviewing this patch on this email thread until consensus is reached with the reviewers?

I guess confusion is because you never received response from Rob. I'll
reply there.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

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@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;