| Message ID | 20231029042712.520010-2-cristian.ciocaltea@collabora.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Enable networking support for StarFive JH7100 SoC | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 29/10/2023 05:27, Cristian Ciocaltea wrote: > The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires > just the 'ahb' reset name, but the binding allows selecting it only in > conjunction with 'stmmaceth'. > > Fix the issue by permitting exclusive usage of the 'ahb' reset name. > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > index 5c2769dc689a..a4d7172ea701 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > @@ -146,7 +146,7 @@ properties: > reset-names: > minItems: 1 > items: > - - const: stmmaceth > + - enum: [stmmaceth, ahb] Your patch #3 says you have minimum two items. Here you claim you have only one reset. It's confusing. Best regards, Krzysztof
On 29/10/2023 05:27, Cristian Ciocaltea wrote: > The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires > just the 'ahb' reset name, but the binding allows selecting it only in > conjunction with 'stmmaceth'. > > Fix the issue by permitting exclusive usage of the 'ahb' reset name. > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > index 5c2769dc689a..a4d7172ea701 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > @@ -146,7 +146,7 @@ properties: > reset-names: > minItems: 1 > items: > - - const: stmmaceth > + - enum: [stmmaceth, ahb] Also, this makes sense only with patch #4, so this should be squashed there. Best regards, Krzysztof
On 10/29/23 13:21, Krzysztof Kozlowski wrote: > On 29/10/2023 05:27, Cristian Ciocaltea wrote: >> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >> just the 'ahb' reset name, but the binding allows selecting it only in >> conjunction with 'stmmaceth'. >> >> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >> --- >> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> index 5c2769dc689a..a4d7172ea701 100644 >> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> @@ -146,7 +146,7 @@ properties: >> reset-names: >> minItems: 1 >> items: >> - - const: stmmaceth >> + - enum: [stmmaceth, ahb] > > Your patch #3 says you have minimum two items. Here you claim you have > only one reset. It's confusing. At least the following use-cases need to be supported: - JH7110: reset-names = "stmmaceth", "ahb"; - JH7110: reset-names = "ahb"; - other: reset-names = "stmmaceth"; Since this is the schema which gets included later in other bindings, the property needs to be generic enough to cope with all the above. [added actual content here for more clarity] reset-names: minItems: 1 items: - enum: [stmmaceth, ahb] - const: ahb Therefore, only the lower limit (1) is enforced here, while starfive,jh7110-dwmac.yaml (which PATCH 3 relates to) adds further constraints (limiting to precisely two items): reset-names: items: - const: stmmaceth - const: ahb I understand the generic binding also allows now specifying 'ahb' twice, but I'm not sure if there's a convenient way to avoid that (e.g. without complicating excessively the schema). Thanks, Cristian
On 10/29/23 23:55, Cristian Ciocaltea wrote: > On 10/29/23 13:21, Krzysztof Kozlowski wrote: >> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>> just the 'ahb' reset name, but the binding allows selecting it only in >>> conjunction with 'stmmaceth'. >>> >>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>> >>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>> --- >>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> index 5c2769dc689a..a4d7172ea701 100644 >>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> @@ -146,7 +146,7 @@ properties: >>> reset-names: >>> minItems: 1 >>> items: >>> - - const: stmmaceth >>> + - enum: [stmmaceth, ahb] >> >> Your patch #3 says you have minimum two items. Here you claim you have >> only one reset. It's confusing. > > At least the following use-cases need to be supported: > > - JH7110: reset-names = "stmmaceth", "ahb"; > - JH7110: reset-names = "ahb"; I've just realized my mistake here - this is for JH7100, sorry for the confusion: - JH7100: reset-names = "ahb"; > - other: reset-names = "stmmaceth"; > > Since this is the schema which gets included later in other bindings, > the property needs to be generic enough to cope with all the above. > [added actual content here for more clarity] > > reset-names: > minItems: 1 > items: > - enum: [stmmaceth, ahb] > - const: ahb > > Therefore, only the lower limit (1) is enforced here, while > starfive,jh7110-dwmac.yaml (which PATCH 3 relates to) adds further > constraints (limiting to precisely two items): > > reset-names: > items: > - const: stmmaceth > - const: ahb > > I understand the generic binding also allows now specifying 'ahb' twice, > but I'm not sure if there's a convenient way to avoid that (e.g. without > complicating excessively the schema).
On 10/29/23 13:25, Krzysztof Kozlowski wrote: > On 29/10/2023 05:27, Cristian Ciocaltea wrote: >> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >> just the 'ahb' reset name, but the binding allows selecting it only in >> conjunction with 'stmmaceth'. >> >> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >> --- >> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> index 5c2769dc689a..a4d7172ea701 100644 >> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> @@ -146,7 +146,7 @@ properties: >> reset-names: >> minItems: 1 >> items: >> - - const: stmmaceth >> + - enum: [stmmaceth, ahb] > > Also, this makes sense only with patch #4, so this should be squashed there. I added this as a separate patch since it changes the generic schema which is included by many other bindings. JH7100 just happens to be the first use-case requiring this update. But I can squash the patch if that's not a good enough reason to keep it separately. Thanks, Cristian
On 29/10/2023 23:24, Cristian Ciocaltea wrote: > On 10/29/23 13:25, Krzysztof Kozlowski wrote: >> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>> just the 'ahb' reset name, but the binding allows selecting it only in >>> conjunction with 'stmmaceth'. >>> >>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>> >>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>> --- >>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> index 5c2769dc689a..a4d7172ea701 100644 >>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> @@ -146,7 +146,7 @@ properties: >>> reset-names: >>> minItems: 1 >>> items: >>> - - const: stmmaceth >>> + - enum: [stmmaceth, ahb] >> >> Also, this makes sense only with patch #4, so this should be squashed there. > > I added this as a separate patch since it changes the generic schema > which is included by many other bindings. JH7100 just happens to be the > first use-case requiring this update. But I can squash the patch if > that's not a good enough reason to keep it separately. If there is no single user of this, why changing this? I would even argue that it is not correct from existing bindings point of view - nothing allows and uses ahb as the only reset. Even the commit msg mentions your hardware from patch 4. Best regards, Krzysztof
On 10/30/23 09:26, Krzysztof Kozlowski wrote: > On 29/10/2023 23:24, Cristian Ciocaltea wrote: >> On 10/29/23 13:25, Krzysztof Kozlowski wrote: >>> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>>> just the 'ahb' reset name, but the binding allows selecting it only in >>>> conjunction with 'stmmaceth'. >>>> >>>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>>> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>> --- >>>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> index 5c2769dc689a..a4d7172ea701 100644 >>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> @@ -146,7 +146,7 @@ properties: >>>> reset-names: >>>> minItems: 1 >>>> items: >>>> - - const: stmmaceth >>>> + - enum: [stmmaceth, ahb] >>> >>> Also, this makes sense only with patch #4, so this should be squashed there. >> >> I added this as a separate patch since it changes the generic schema >> which is included by many other bindings. JH7100 just happens to be the >> first use-case requiring this update. But I can squash the patch if >> that's not a good enough reason to keep it separately. > > If there is no single user of this, why changing this? I would even > argue that it is not correct from existing bindings point of view - > nothing allows and uses ahb as the only reset. Even the commit msg > mentions your hardware from patch 4. Sorry, I'm not sure I follow. JH7100 is (or will be) the user of it and, as a matter of fact, something similar has been done recently while adding support for JH7110. In particular, commit [1] changed this binding before the JH7110 compatible was introduced in a subsequent patch. On a closer look that commit made a statement which is not entirely correct: "dwmac controller may require one (stmmaceth) or two (stmmaceth+ahb) reset signals" That's because stmmaceth is also optional in dwmac's driver, hence the correct message would have been: "[...] may require one (stmmaceth OR ahb) [...]" Hence, I think it makes sense to keep this patch, after adding the above details in the commit message. [1] 843f603762a5 ("dt-bindings: net: snps,dwmac: Add 'ahb' reset/reset-name") Thanks, Cristian
On 30/10/2023 20:07, Cristian Ciocaltea wrote: > On 10/30/23 09:26, Krzysztof Kozlowski wrote: >> On 29/10/2023 23:24, Cristian Ciocaltea wrote: >>> On 10/29/23 13:25, Krzysztof Kozlowski wrote: >>>> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>>>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>>>> just the 'ahb' reset name, but the binding allows selecting it only in >>>>> conjunction with 'stmmaceth'. >>>>> >>>>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>>>> >>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>>> --- >>>>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>> index 5c2769dc689a..a4d7172ea701 100644 >>>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>> @@ -146,7 +146,7 @@ properties: >>>>> reset-names: >>>>> minItems: 1 >>>>> items: >>>>> - - const: stmmaceth >>>>> + - enum: [stmmaceth, ahb] >>>> >>>> Also, this makes sense only with patch #4, so this should be squashed there. >>> >>> I added this as a separate patch since it changes the generic schema >>> which is included by many other bindings. JH7100 just happens to be the >>> first use-case requiring this update. But I can squash the patch if >>> that's not a good enough reason to keep it separately. >> >> If there is no single user of this, why changing this? I would even >> argue that it is not correct from existing bindings point of view - >> nothing allows and uses ahb as the only reset. Even the commit msg >> mentions your hardware from patch 4. > > Sorry, I'm not sure I follow. JH7100 is (or will be) the user of it and, > as a matter of fact, something similar has been done recently while > adding support for JH7110. Every patch should stand on its own and at this point nothing uses it. We apply this patch #1 and we add dead code, unused case. > > In particular, commit [1] changed this binding before the JH7110 > compatible was introduced in a subsequent patch. On a closer look that > commit made a statement which is not entirely correct: > > "dwmac controller may require one (stmmaceth) or two (stmmaceth+ahb) > reset signals" > > That's because stmmaceth is also optional in dwmac's driver, hence the > correct message would have been: > > "[...] may require one (stmmaceth OR ahb) [...]" Driver does not describe the hardware. The bindings do, so according to that description all supported hardware required MAC reset (stmmaceth). Otherwise please point me to any hardware which skips MAC reset and requires AHB reset instead (not future hardware, but current). > > Hence, I think it makes sense to keep this patch, after adding the above > details in the commit message. > > [1] 843f603762a5 ("dt-bindings: net: snps,dwmac: Add 'ahb' > reset/reset-name") > > Thanks, > Cristian Best regards, Krzysztof
On 10/31/23 07:48, Krzysztof Kozlowski wrote: > On 30/10/2023 20:07, Cristian Ciocaltea wrote: >> On 10/30/23 09:26, Krzysztof Kozlowski wrote: >>> On 29/10/2023 23:24, Cristian Ciocaltea wrote: >>>> On 10/29/23 13:25, Krzysztof Kozlowski wrote: >>>>> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>>>>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>>>>> just the 'ahb' reset name, but the binding allows selecting it only in >>>>>> conjunction with 'stmmaceth'. >>>>>> >>>>>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>>>>> >>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>>>> --- >>>>>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>>> index 5c2769dc689a..a4d7172ea701 100644 >>>>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>>> @@ -146,7 +146,7 @@ properties: >>>>>> reset-names: >>>>>> minItems: 1 >>>>>> items: >>>>>> - - const: stmmaceth >>>>>> + - enum: [stmmaceth, ahb] >>>>> >>>>> Also, this makes sense only with patch #4, so this should be squashed there. >>>> >>>> I added this as a separate patch since it changes the generic schema >>>> which is included by many other bindings. JH7100 just happens to be the >>>> first use-case requiring this update. But I can squash the patch if >>>> that's not a good enough reason to keep it separately. >>> >>> If there is no single user of this, why changing this? I would even >>> argue that it is not correct from existing bindings point of view - >>> nothing allows and uses ahb as the only reset. Even the commit msg >>> mentions your hardware from patch 4. >> >> Sorry, I'm not sure I follow. JH7100 is (or will be) the user of it and, >> as a matter of fact, something similar has been done recently while >> adding support for JH7110. > > Every patch should stand on its own and at this point nothing uses it. > We apply this patch #1 and we add dead code, unused case. > >> >> In particular, commit [1] changed this binding before the JH7110 >> compatible was introduced in a subsequent patch. On a closer look that >> commit made a statement which is not entirely correct: >> >> "dwmac controller may require one (stmmaceth) or two (stmmaceth+ahb) >> reset signals" >> >> That's because stmmaceth is also optional in dwmac's driver, hence the >> correct message would have been: >> >> "[...] may require one (stmmaceth OR ahb) [...]" > > Driver does not describe the hardware. The bindings do, so according to > that description all supported hardware required MAC reset (stmmaceth). > Otherwise please point me to any hardware which skips MAC reset and > requires AHB reset instead (not future hardware, but current). I might have found one (arch/arm/boot/dts/allwinner/sun6i-a31.dtsi): gmac: ethernet@1c30000 { compatible = "allwinner,sun7i-a20-gmac"; [...] resets = <&ccu RST_AHB1_EMAC>; reset-names = "stmmaceth"; [...] }; It's possible the 'stmmaceth' naming has been used instead of 'ahb' just to avoid updating the binding, but that's just an assumption looking at RST_AHB1_EMAC. I will proceed with the squash for v3. Thanks for clarifying, Cristian
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml index 5c2769dc689a..a4d7172ea701 100644 --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml @@ -146,7 +146,7 @@ properties: reset-names: minItems: 1 items: - - const: stmmaceth + - enum: [stmmaceth, ahb] - const: ahb power-domains:
The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires just the 'ahb' reset name, but the binding allows selecting it only in conjunction with 'stmmaceth'. Fix the issue by permitting exclusive usage of the 'ahb' reset name. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)