| Message ID | 20231218214451.2345691-3-cristian.ciocaltea@collabora.com |
|---|---|
| State | Superseded |
| 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 18/12/2023 22:44, Cristian Ciocaltea wrote: > The Synopsys DesignWare MAC found on StarFive JH7100 SoC is mostly > similar to the newer JH7110, but it requires only two interrupts and a > single reset line, which is 'ahb' instead of the commonly used > 'stmmaceth'. > > reg: > @@ -145,9 +146,13 @@ properties: > > reset-names: > minItems: 1 > - items: > - - const: stmmaceth > - - const: ahb > + maxItems: 2 min and maxItems should not be needed here. > + oneOf: > + - items: > + - enum: [stmmaceth, ahb] > + - items: > + - const: stmmaceth > + - const: ahb > > power-domains: > maxItems: 1 > diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml > index d90cb82c1424..f5f0bff5be0f 100644 > --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml > @@ -16,16 +16,20 @@ select: > compatible: > contains: > enum: > + - starfive,jh7100-dwmac > - starfive,jh7110-dwmac > required: > - compatible > > properties: > compatible: > - items: > - - enum: > - - starfive,jh7110-dwmac > - - const: snps,dwmac-5.20 > + oneOf: > + - items: > + - const: starfive,jh7100-dwmac > + - const: snps,dwmac > + - items: > + - const: starfive,jh7110-dwmac > + - const: snps,dwmac-5.20 > > reg: > maxItems: 1 > @@ -46,23 +50,6 @@ properties: > - const: tx > - const: gtx > > - interrupts: > - minItems: 3 > - maxItems: 3 > - > - interrupt-names: > - minItems: 3 > - maxItems: 3 > - > - resets: > - minItems: 2 > - maxItems: 2 What is the point of your previous patch if you immediately remove it? It is a no-op. Just mention in this commit msg, that both resets and reset-names are coming from snps,dwmac so they can be removed from top-level entirely. Best regards, Krzysztof
On 12/19/23 09:27, Krzysztof Kozlowski wrote: > On 18/12/2023 22:44, Cristian Ciocaltea wrote: >> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is mostly >> similar to the newer JH7110, but it requires only two interrupts and a >> single reset line, which is 'ahb' instead of the commonly used >> 'stmmaceth'. >> > >> reg: >> @@ -145,9 +146,13 @@ properties: >> >> reset-names: >> minItems: 1 >> - items: >> - - const: stmmaceth >> - - const: ahb >> + maxItems: 2 > > min and maxItems should not be needed here. Indeed, I will drop them. >> + oneOf: >> + - items: >> + - enum: [stmmaceth, ahb] >> + - items: >> + - const: stmmaceth >> + - const: ahb >> >> power-domains: >> maxItems: 1 >> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >> index d90cb82c1424..f5f0bff5be0f 100644 >> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >> @@ -16,16 +16,20 @@ select: >> compatible: >> contains: >> enum: >> + - starfive,jh7100-dwmac >> - starfive,jh7110-dwmac >> required: >> - compatible >> >> properties: >> compatible: >> - items: >> - - enum: >> - - starfive,jh7110-dwmac >> - - const: snps,dwmac-5.20 >> + oneOf: >> + - items: >> + - const: starfive,jh7100-dwmac >> + - const: snps,dwmac >> + - items: >> + - const: starfive,jh7110-dwmac >> + - const: snps,dwmac-5.20 >> >> reg: >> maxItems: 1 >> @@ -46,23 +50,6 @@ properties: >> - const: tx >> - const: gtx >> >> - interrupts: >> - minItems: 3 >> - maxItems: 3 >> - >> - interrupt-names: >> - minItems: 3 >> - maxItems: 3 >> - >> - resets: >> - minItems: 2 >> - maxItems: 2 > > What is the point of your previous patch if you immediately remove it? > It is a no-op. Just mention in this commit msg, that both resets and > reset-names are coming from snps,dwmac so they can be removed from > top-level entirely. This has been discussed during v2 review [1], where I also provided the rational behind not updating reset-names. So the code was not deleted, but moved under an if clause. Thanks for reviewing, Cristian [1]: https://lore.kernel.org/lkml/f4d0b216-5bdc-4559-aabb-8af638d33721@collabora.com/
On 19/12/2023 13:49, Cristian Ciocaltea wrote: >>> reg: >>> maxItems: 1 >>> @@ -46,23 +50,6 @@ properties: >>> - const: tx >>> - const: gtx >>> >>> - interrupts: >>> - minItems: 3 >>> - maxItems: 3 >>> - >>> - interrupt-names: >>> - minItems: 3 >>> - maxItems: 3 >>> - >>> - resets: >>> - minItems: 2 >>> - maxItems: 2 >> >> What is the point of your previous patch if you immediately remove it? >> It is a no-op. Just mention in this commit msg, that both resets and >> reset-names are coming from snps,dwmac so they can be removed from >> top-level entirely. > > This has been discussed during v2 review [1], where I also provided the > rational behind not updating reset-names. So the code was not deleted, > but moved under an if clause. > > Thanks for reviewing, > Cristian > > [1]: https://lore.kernel.org/lkml/f4d0b216-5bdc-4559-aabb-8af638d33721@collabora.com/ I don't see it being addressed: https://lore.kernel.org/lkml/35556392-3b9a-4997-b482-082dc2f9121f@linaro.org/ Repeating the same and the same :/ Best regards, Krzysztof
On 12/19/23 15:19, Krzysztof Kozlowski wrote: > On 19/12/2023 13:49, Cristian Ciocaltea wrote: >>>> reg: >>>> maxItems: 1 >>>> @@ -46,23 +50,6 @@ properties: >>>> - const: tx >>>> - const: gtx >>>> >>>> - interrupts: >>>> - minItems: 3 >>>> - maxItems: 3 >>>> - >>>> - interrupt-names: >>>> - minItems: 3 >>>> - maxItems: 3 >>>> - >>>> - resets: >>>> - minItems: 2 >>>> - maxItems: 2 >>> >>> What is the point of your previous patch if you immediately remove it? >>> It is a no-op. Just mention in this commit msg, that both resets and >>> reset-names are coming from snps,dwmac so they can be removed from >>> top-level entirely. >> >> This has been discussed during v2 review [1], where I also provided the >> rational behind not updating reset-names. So the code was not deleted, >> but moved under an if clause. >> >> Thanks for reviewing, >> Cristian >> >> [1]: https://lore.kernel.org/lkml/f4d0b216-5bdc-4559-aabb-8af638d33721@collabora.com/ > > I don't see it being addressed: > > https://lore.kernel.org/lkml/35556392-3b9a-4997-b482-082dc2f9121f@linaro.org/ > > Repeating the same and the same :/ I think this was just a misunderstanding, sorry for the confusion. I kept two sets of changes which were not directly related into separate patches, so I'm going to squash them in v5. Thanks, Cristian
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml index 5c2769dc689a..4052b355ec2c 100644 --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml @@ -95,6 +95,7 @@ properties: - snps,dwmac-5.20 - snps,dwxgmac - snps,dwxgmac-2.10 + - starfive,jh7100-dwmac - starfive,jh7110-dwmac reg: @@ -145,9 +146,13 @@ properties: reset-names: minItems: 1 - items: - - const: stmmaceth - - const: ahb + maxItems: 2 + oneOf: + - items: + - enum: [stmmaceth, ahb] + - items: + - const: stmmaceth + - const: ahb power-domains: maxItems: 1 diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml index d90cb82c1424..f5f0bff5be0f 100644 --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml @@ -16,16 +16,20 @@ select: compatible: contains: enum: + - starfive,jh7100-dwmac - starfive,jh7110-dwmac required: - compatible properties: compatible: - items: - - enum: - - starfive,jh7110-dwmac - - const: snps,dwmac-5.20 + oneOf: + - items: + - const: starfive,jh7100-dwmac + - const: snps,dwmac + - items: + - const: starfive,jh7110-dwmac + - const: snps,dwmac-5.20 reg: maxItems: 1 @@ -46,23 +50,6 @@ properties: - const: tx - const: gtx - interrupts: - minItems: 3 - maxItems: 3 - - interrupt-names: - minItems: 3 - maxItems: 3 - - resets: - minItems: 2 - maxItems: 2 - - reset-names: - items: - - const: stmmaceth - - const: ahb - starfive,tx-use-rgmii-clk: description: Tx clock is provided by external rgmii clock. @@ -93,6 +80,51 @@ required: allOf: - $ref: snps,dwmac.yaml# + - if: + properties: + compatible: + contains: + const: starfive,jh7100-dwmac + then: + properties: + interrupts: + minItems: 2 + maxItems: 2 + + interrupt-names: + minItems: 2 + maxItems: 2 + + resets: + maxItems: 1 + + reset-names: + const: ahb + + - if: + properties: + compatible: + contains: + const: starfive,jh7110-dwmac + then: + properties: + interrupts: + minItems: 3 + maxItems: 3 + + interrupt-names: + minItems: 3 + maxItems: 3 + + resets: + minItems: 2 + maxItems: 2 + + reset-names: + items: + - const: stmmaceth + - const: ahb + unevaluatedProperties: false examples:
The Synopsys DesignWare MAC found on StarFive JH7100 SoC is mostly similar to the newer JH7110, but it requires only two interrupts and a single reset line, which is 'ahb' instead of the commonly used 'stmmaceth'. Since the common binding 'snps,dwmac' allows selecting 'ahb' only in conjunction with 'stmmaceth', extend the logic to also permit exclusive usage of the 'ahb' reset name. This ensures the following use cases are supported: JH7110: reset-names = "stmmaceth", "ahb"; JH7100: reset-names = "ahb"; other: reset-names = "stmmaceth"; Also note the need to use a different dwmac fallback, as v5.20 applies to JH7110 only, while JH7100 relies on v3.7x. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- .../devicetree/bindings/net/snps,dwmac.yaml | 11 ++- .../bindings/net/starfive,jh7110-dwmac.yaml | 74 +++++++++++++------ 2 files changed, 61 insertions(+), 24 deletions(-)