| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <trinity-cda3b94f-8556-4b83-bc34-d2c215f93bcd-1651587032669@3c-app-gmx-bap25>
Date: Tue, 3 May 2022 16:10:32 +0200
From: Frank Wunderlich <frank-w@...lic-files.de>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Greg Ungerer <gerg@...nel.org>,
René van Dorst <opensource@...rst.com>,
Mauro Carvalho Chehab <mchehab+samsung@...nel.org>
Cc: Frank Wunderlich <linux@...web.de>, Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Sean Wang <sean.wang@...iatek.com>,
Landen Chao <Landen.Chao@...iatek.com>,
DENG Qingfang <dqfext@...il.com>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for
mediatek switches
Hi,
thank you for first review.
> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.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@...lic-files.de>
> >
> > Convert txt binding to yaml binding for Mediatek switches.
> >
> > Signed-off-by: Frank Wunderlich <frank-w@...lic-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.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/dsa/mediatek.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mediatek MT7530 Ethernet switch
> > +
> > +maintainers:
> > + - Sean Wang <sean.wang@...iatek.com>
> > + - Landen Chao <Landen.Chao@...iatek.com>
> > + - DENG Qingfang <dqfext@...il.com>
> > +
> > +description: |
> > + Port 5 of mt7530 and mt7621 switch is muxed between:
> > + 1. GMAC5: GMAC5 can interface with another external MAC or PHY.
> > + 2. PHY of port 0 or port 4: PHY interfaces with an external MAC like 2nd GMAC
> > + of the SOC. Used in many setups where port 0/4 becomes the WAN port.
> > + Note: On a MT7621 SOC with integrated switch: 2nd GMAC can only connected to
> > + GMAC5 when the gpios for RGMII2 (GPIO 22-33) are not used and not
> > + connected to external component!
> > +
> > + Port 5 modes/configurations:
> > + 1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
> > + GMAC of the SOC.
> > + In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
> > + GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
> > + 2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
> > + It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
> > + and RGMII delay.
> > + 3. Port 5 is muxed to GMAC5 and can interface to an external phy.
> > + Port 5 becomes an extra switch port.
> > + Only works on platform where external phy TX<->RX lines are swapped.
> > + Like in the Ubiquiti ER-X-SFP.
> > + 4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
> > + Currently a 2nd CPU port is not supported by DSA code.
> > +
> > + Depending on how the external PHY is wired:
> > + 1. normal: The PHY can only connect to 2nd GMAC but not to the switch
> > + 2. swapped: RGMII TX, RX are swapped; external phy interface with the switch as
> > + a ethernet port. But can't interface to the 2nd GMAC.
> > +
> > + Based on the DT the port 5 mode is configured.
> > +
> > + Driver tries to lookup the phy-handle of the 2nd GMAC of the master device.
> > + When phy-handle matches PHY of port 0 or 4 then port 5 set-up as mode 2.
> > + phy-mode must be set, see also example 2 below!
> > + * mt7621: phy-mode = "rgmii-txid";
> > + * mt7623: phy-mode = "rgmii";
> > +
> > + CPU-Ports need a phy-mode property:
> > + Allowed values on mt7530 and mt7621:
> > + - "rgmii"
> > + - "trgmii"
> > + On mt7531:
> > + - "1000base-x"
> > + - "2500base-x"
> > + - "sgmii"
> > +
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - mediatek,mt7530
> > + - mediatek,mt7531
> > + - mediatek,mt7621
> > +
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 0
> > +
> > + core-supply:
> > + description: |
> > + Phandle to the regulator node necessary for the core power.
> > +
> > + "#gpio-cells":
> > + description: |
> > + Must be 2 if gpio-controller is defined.
> > + const: 2
> > +
> > + gpio-controller:
> > + type: boolean
> > + description: |
> > + Boolean; if defined, MT7530's LED controller will run on
>
> No need to repeat Boolean.
ok, will change
> > + GPIO mode.
> > +
> > + "#interrupt-cells":
> > + const: 1
> > +
> > + interrupt-controller:
> > + type: boolean
> > + description: |
> > + Boolean; Enables the internal interrupt controller.
>
> Skip description.
ok
> > +
> > + interrupts:
> > + description: |
> > + Parent interrupt for the interrupt controller.
>
> Skip description.
ok
> > + maxItems: 1
> > +
> > + io-supply:
> > + description: |
> > + Phandle to the regulator node necessary for the I/O power.
> > + See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
> > + for details for the regulator setup on these boards.
> > +
> > + mediatek,mcm:
> > + type: boolean
> > + description: |
> > + Boolean;
>
> No need to repeat Boolean.
ack
> > 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.
> > + 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)?
> > + - 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"
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?
> > + 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)
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + port@0 {
> > + reg = <0>;
> > + label = "lan0";
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > + label = "lan1";
> > + };
> > +
> > + port@2 {
> > + reg = <2>;
> > + label = "lan2";
> > + };
> > +
> > + port@3 {
> > + reg = <3>;
> > + label = "lan3";
> > + };
> > +
> > + port@4 {
> > + reg = <4>;
> > + label = "wan";
> > + };
> > +
> > + port@6 {
> > + reg = <6>;
> > + label = "cpu";
> > + ethernet = <&gmac0>;
> > + phy-mode = "trgmii";
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + };
> > + };
> > + };
> > + };
> > + };
> > +
> > + - |
> > + //Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4.
> > +
> > + eth {
>
> s/eth/ethernet/
ok
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + gmac0: mac@0 {
> > + compatible = "mediatek,eth-mac";
> > + reg = <0>;
> > + phy-mode = "rgmii";
> > +
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + pause;
> > + };
> > + };
> > +
> > + gmac1: mac@1 {
> > + compatible = "mediatek,eth-mac";
> > + reg = <1>;
> > + phy-mode = "rgmii-txid";
> > + phy-handle = <&phy4>;
> > + };
> > +
> > + mdio: mdio-bus {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + /* Internal phy */
> > + phy4: ethernet-phy@4 {
> > + reg = <4>;
> > + };
> > +
> > + mt7530: switch@1f {
> > + compatible = "mediatek,mt7621";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + reg = <0x1f>;
> > + mediatek,mcm;
> > +
> > + resets = <&rstctrl 2>;
> > + reset-names = "mcm";
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > + label = "lan0";
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > + label = "lan1";
> > + };
> > +
> > + port@2 {
> > + reg = <2>;
> > + label = "lan2";
> > + };
> > +
> > + port@3 {
> > + reg = <3>;
> > + label = "lan3";
> > + };
> > +
> > + /* Commented out. Port 4 is handled by 2nd GMAC.
> > + port@4 {
> > + reg = <4>;
> > + label = "lan4";
> > + };
> > + */
>
> Messed up indentation
will fix it
> > +
> > + port@6 {
> > + reg = <6>;
> > + label = "cpu";
> > + ethernet = <&gmac0>;
> > + phy-mode = "rgmii";
> > +
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + pause;
> > + };
> > + };
> > + };
> > + };
> > + };
> > + };
> > +
> > + - |
> > + //Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
> > +
> > + eth {
>
> Also ethernet?
will do
> Best regards,
> Krzysztof
regards Frank
Powered by blists - more mailing lists