From: Frank Wunderlich <frank-w@public-files.de>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: "Greg Ungerer" <gerg@kernel.org>,
"René van Dorst" <opensource@vdorst.com>,
"Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>,
"Frank Wunderlich" <linux@fw-web.de>,
"Andrew Lunn" <andrew@lunn.ch>,
"Vivien Didelot" <vivien.didelot@gmail.com>,
"Florian Fainelli" <f.fainelli@gmail.com>,
"Vladimir Oltean" <olteanv@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
"Jakub Kicinski" <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>,
"Rob Herring" <robh+dt@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
"Sean Wang" <sean.wang@mediatek.com>,
"Landen Chao" <Landen.Chao@mediatek.com>,
"DENG Qingfang" <dqfext@gmail.com>,
netdev@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org
Subject: Aw: Re: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
Date: Tue, 3 May 2022 17:03:17 +0200 [thread overview]
Message-ID: <trinity-213ab6b1-ccff-4429-b76c-623c529f6f73-1651590197578@3c-app-gmx-bap25> (raw)
In-Reply-To: <10770ff5-c9b1-7364-4276-05fa0c393d3b@linaro.org>
Hi,
> Gesendet: Dienstag, 03. Mai 2022 um 16:40 Uhr
> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
> Betreff: Re: Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
>
> On 03/05/2022 16:10, Frank Wunderlich wrote:
> > Hi,
> >
> > thank you for first review.
> >
> >> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
> >> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
> >> Betreff: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
> >>
> >> On 02/05/2022 17:32, Frank Wunderlich wrote:
> >>> From: Frank Wunderlich <frank-w@public-files.de>
> >>>
> >>> Convert txt binding to yaml binding for Mediatek switches.
> >>>
> >>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> >>> ---
> >>> .../devicetree/bindings/net/dsa/mediatek.yaml | 435 ++++++++++++++++++
> >>> .../devicetree/bindings/net/dsa/mt7530.txt | 327 -------------
> >>> 2 files changed, 435 insertions(+), 327 deletions(-)
> >>> create mode 100644 Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> >>> delete mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> >>> new file mode 100644
> >>> index 000000000000..c1724809d34e
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
> >>
> >> Specific name please, so previous (with vendor prefix) was better:
> >> mediatek,mt7530.yaml
> >
> > ok, named it mediatek only because mt7530 is only one possible chip and driver handles 3 different "variants".
> >
> >>> @@ -0,0 +1,435 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>
> >> You should CC previous contributors and get their acks on this. You
> >> copied here a lot of description.
> >
> > added 3 Persons that made commits to txt before to let them know about this change
> >
> > and yes, i tried to define at least the phy-mode requirement as yaml-depency, but failed because i cannot match
> > compatible in subnode.
>
> I don't remember such syntax.
>
> (...)
have not posted this version as it was failing in dtbs_check, this was how i tried:
https://github.com/frank-w/BPI-R2-4.14/blob/8f2033eb6fcae273580263c3f0b31f0d48821740/Documentation/devicetree/bindings/net/dsa/mediatek.yaml#L177
> >>> if defined, indicates that either MT7530 is the part
> >>> + on multi-chip module belong to MT7623A has or the remotely standalone
> >>> + chip as the function MT7623N reference board provided for.
> >>> +
> >>> + reset-gpios:
> >>> + description: |
> >>> + Should be a gpio specifier for a reset line.
> >>> + maxItems: 1
> >>> +
> >>> + reset-names:
> >>> + description: |
> >>> + Should be set to "mcm".
> >>> + const: mcm
> >>> +
> >>> + resets:
> >>> + description: |
> >>> + Phandle pointing to the system reset controller with
> >>> + line index for the ethsys.
> >>> + maxItems: 1
> >>> +
> >>> +required:
> >>> + - compatible
> >>> + - reg
> >>
> >> What about address/size cells?
> >
> > you're right even if they are const to a value they need to be set
> >
> >>> +
> >>> +allOf:
> >>> + - $ref: "dsa.yaml#"
> >>> + - if:
> >>> + required:
> >>> + - mediatek,mcm
> >>
> >> Original bindings had this reversed.
> >
> > i know, but i think it is better readable and i will drop the else-part later.
> > Driver supports optional reset ("mediatek,mcm" unset and without reset-gpios)
> > as this is needed if there is a shared reset-line for gmac and switch like on R2 Pro.
> >
> > i left this as separate commit to be posted later to have a nearly 1:1 conversion here.
>
> Ah, I missed that actually your syntax is better. No need to
> reverse/negate and the changes do not have to be strict 1:1.
yes, but a conversion implies same meaning, so changing things later ;)
> >>> + then:
> >>> + required:
> >>> + - resets
> >>> + - reset-names
> >>> + else:
> >>> + required:
> >>> + - reset-gpios
> >>> +
> >>> + - if:
> >>> + required:
> >>> + - interrupt-controller
> >>> + then:
> >>> + required:
> >>> + - "#interrupt-cells"
> >>
> >> This should come from dt schema already...
> >
> > so i should drop (complete block for interrupt controller)?
>
> The interrupts you need. What I mean, you can skip requirement of cells.
ok, i drop only the #interrupt-cells
> >>> + - interrupts
> >>> +
> >>> + - if:
> >>> + properties:
> >>> + compatible:
> >>> + items:
> >>> + - const: mediatek,mt7530
> >>> + then:
> >>> + required:
> >>> + - core-supply
> >>> + - io-supply
> >>> +
> >>> +
> >>> +patternProperties:
> >>> + "^ports$":
> >>
> >> It''s not a pattern, so put it under properties, like regular property.
> >
> > can i then make the subnodes match? so the full block will move above required between "mediatek,mcm" and "reset-gpios"
>
> Yes, subnodes stay with patternProperties.
>
> >
> > ports:
> > type: object
> >
> > patternProperties:
> > "^port@[0-9]+$":
> > type: object
> > description: Ethernet switch ports
> >
> > properties:
> > reg:
> > description: |
> > Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports.
> >
> > unevaluatedProperties: false
> >
> > allOf:
> > - $ref: dsa-port.yaml#
> > - if:
> > ....
> >
> > basicly this "ports"-property should be required too, right?
>
> Previous binding did not enforce it, I think, but it is reasonable to
> require ports.
basicly it is required in dsa.yaml, so it will be redundant here
https://elixir.bootlin.com/linux/v5.18-rc5/source/Documentation/devicetree/bindings/net/dsa/dsa.yaml#L55
this defines it as pattern "^(ethernet-)?ports$" and should be processed by dsa-core. so maybe changing it to same pattern instead of moving up as normal property?
> >>> + type: object
> >>> +
> >>> + patternProperties:
> >>> + "^port@[0-9]+$":
> >>> + type: object
> >>> + description: Ethernet switch ports
> >>> +
> >>> + $ref: dsa-port.yaml#
> >>
> >> This should go to allOf below.
> >
> > see above
> >
> >>> +
> >>> + properties:
> >>> + reg:
> >>> + description: |
> >>> + Port address described must be 6 for CPU port and from 0 to 5 for user ports.
> >>> +
> >>> + unevaluatedProperties: false
> >>> +
> >>> + allOf:
> >>> + - if:
> >>> + properties:
> >>> + label:
> >>> + items:
> >>> + - const: cpu
> >>> + then:
> >>> + required:
> >>> + - reg
> >>> + - phy-mode
> >>> +
> >>> +unevaluatedProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + mdio0 {
> >>
> >> Just mdio
> >
> > ok
> >
> >>> + #address-cells = <1>;
> >>> + #size-cells = <0>;
> >>> + switch@0 {
> >>> + compatible = "mediatek,mt7530";
> >>> + #address-cells = <1>;
> >>> + #size-cells = <0>;
> >>> + reg = <0>;
> >>> +
> >>> + core-supply = <&mt6323_vpa_reg>;
> >>> + io-supply = <&mt6323_vemc3v3_reg>;
> >>> + reset-gpios = <&pio 33 0>;
> >>
> >> Use GPIO flag define/constant.
> >
> > this example seems to be taken from bpi-r2 (i had taken it from the txt). In dts for this board there are no
> > constants too.
> >
> > i guess
> > include/dt-bindings/gpio/gpio.h:14:#define GPIO_ACTIVE_HIGH 0
> >
> > for 33 there seem no constant..all other references to pio node are with numbers too and there seem no binding
> > header defining the gpio pins (only functions in include/dt-bindings/pinctrl/mt7623-pinfunc.h)
>
> ok, then my comment
you mean adding a comment to the example that GPIO-flags/constants should be used instead of magic numbers?
> Best regards,
> Krzysztof
this is how it looks like without the port-property-change:
https://github.com/frank-w/BPI-R2-4.14/blob/5.18-mt7531-mainline/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
regards Frank
next prev parent reply other threads:[~2022-05-03 15:05 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-02 15:32 [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches Frank Wunderlich
2022-05-03 12:05 ` Krzysztof Kozlowski
2022-05-03 14:10 ` Aw: " Frank Wunderlich
2022-05-03 14:40 ` Krzysztof Kozlowski
2022-05-03 15:03 ` Frank Wunderlich [this message]
2022-05-04 6:51 ` Aw: " Krzysztof Kozlowski
2022-05-04 7:44 ` Frank Wunderlich
2022-05-04 7:46 ` Krzysztof Kozlowski
2022-05-04 10:21 ` Aw: " Frank Wunderlich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=trinity-213ab6b1-ccff-4429-b76c-623c529f6f73-1651590197578@3c-app-gmx-bap25 \
--to=frank-w@public-files.de \
--cc=Landen.Chao@mediatek.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=dqfext@gmail.com \
--cc=f.fainelli@gmail.com \
--cc=gerg@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=krzysztof.kozlowski@linaro.org \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux@fw-web.de \
--cc=matthias.bgg@gmail.com \
--cc=mchehab+samsung@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=opensource@vdorst.com \
--cc=pabeni@redhat.com \
--cc=robh+dt@kernel.org \
--cc=sean.wang@mediatek.com \
--cc=vivien.didelot@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).