| Message ID | 20211228072645.32341-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dt-bindings: net: dsa: realtek-smi: convert to YAML schema | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Dec 28, 2021 at 8:27 AM Luiz Angelo Daros de Luca <luizluca@gmail.com> wrote: > Schema changes: > > - "interrupt-controller" was not added as a required property. It might > still work polling the ports when missing > - "interrupt" property was mentioned but never used. According to its > description, it was assumed it was really "interrupt-parent" > > Examples changes: > > - renamed "switch_intc" to make it unique between examples > - removed "dsa-mdio" from mdio compatible property > - renamed phy@0 to ethernet-phy@0 (not tested with real HW) > phy@ requires #phy-cells > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> Thanks for doing this! Very nice! > +maintainers: > + - Linus Walleij <linus.walleij@linaro.org> You can add yourself too (if you want) > + description: | > + realtek,rtl8365mb: 4+1 ports > + realtek,rtl8366: > + realtek,rtl8366rb: 4 + 1 ports With that fix: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Thanks Linus! > > + description: | > > + realtek,rtl8365mb: 4+1 ports > > + realtek,rtl8366: > > + realtek,rtl8366rb: There is some confusion with the n+m port description. Some 4+1 means 4 lan + 1 wan while in other cases it means 4 user + 1 ext port, even in Realtek documentation. The last digit in realtek product numbers is the port number (0 means 10) and it is the sum of user ports and external ports. From what I investigated, the last digit numbers normally mean: 3: 2 user + 1 ext port 4: 2 user + 2 ext port 5: 4 user + 1 ext port 6: 5 user + 1 ext port 7: 5 user + 2 ext port 0: 8 user + 2 ext port. The description in YAML was from the TXT version but it is a good time to improve it. BTW, I couldn't find a datasheet for rtl8366rb. The commit message says it is from a DIR-685 but wikidevi days that device has a RTL8366SR, which is described as "SINGLE-CHIP 5+1-PORT 10/100/1000 MBPS SWITCH CONTROLLER WITH DUAL MAC INTERFACES". Do you have any suggestions? Regards, Luiz
On Tue, Jan 04, 2022 at 08:44:37PM -0300, Luiz Angelo Daros de Luca wrote: > Thanks Linus! > > > > + description: | > > > + realtek,rtl8365mb: 4+1 ports > > > + realtek,rtl8366: > > > + realtek,rtl8366rb: > Why have you removed Linus' comment? > There is some confusion with the n+m port description. Some 4+1 means > 4 lan + 1 wan while in other cases it means 4 user + 1 ext port, even > in Realtek documentation. The last digit in realtek product numbers is > the port number (0 means 10) and it is the sum of user ports and > external ports. From what I investigated, the last digit numbers > normally mean: > > 3: 2 user + 1 ext port > 4: 2 user + 2 ext port > 5: 4 user + 1 ext port > 6: 5 user + 1 ext port > 7: 5 user + 2 ext port > 0: 8 user + 2 ext port. > > The description in YAML was from the TXT version but it is a good time > to improve it. > > BTW, I couldn't find a datasheet for rtl8366rb. The commit message > says it is from a DIR-685 but wikidevi days that device has a > RTL8366SR, which is described as "SINGLE-CHIP 5+1-PORT 10/100/1000 > MBPS SWITCH CONTROLLER WITH DUAL MAC INTERFACES". > > Do you have any suggestions? I think Linus just meant to add spaces around the '+'. Rob
On Tue, Dec 28, 2021 at 04:26:45AM -0300, Luiz Angelo Daros de Luca wrote: > Schema changes: > > - "interrupt-controller" was not added as a required property. It might > still work polling the ports when missing > - "interrupt" property was mentioned but never used. According to its > description, it was assumed it was really "interrupt-parent" > > Examples changes: > > - renamed "switch_intc" to make it unique between examples > - removed "dsa-mdio" from mdio compatible property > - renamed phy@0 to ethernet-phy@0 (not tested with real HW) > phy@ requires #phy-cells > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > .../bindings/net/dsa/realtek-smi.txt | 240 -------------- > .../bindings/net/dsa/realtek-smi.yaml | 310 ++++++++++++++++++ > 2 files changed, 310 insertions(+), 240 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/net/dsa/realtek-smi.txt > create mode 100644 Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml > diff --git a/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml b/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml > new file mode 100644 > index 000000000000..c4cd0038f092 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml > @@ -0,0 +1,310 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/dsa/realtek-smi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Realtek SMI-based Switches > + > +allOf: > + - $ref: dsa.yaml# > + > +maintainers: > + - Linus Walleij <linus.walleij@linaro.org> > + > +description: > + The SMI "Simple Management Interface" is a two-wire protocol using > + bit-banged GPIO that while it reuses the MDIO lines MCK and MDIO does > + not use the MDIO protocol. This binding defines how to specify the > + SMI-based Realtek devices. The realtek-smi driver is a platform driver > + and it must be inserted inside a platform node. > + > +properties: > + compatible: > + oneOf: > + - enum: Don't need oneOf when there is only 1 entry. > + - realtek,rtl8365mb > + - realtek,rtl8366 > + - realtek,rtl8366rb > + - realtek,rtl8366s > + - realtek,rtl8367 > + - realtek,rtl8367b > + - realtek,rtl8368s > + - realtek,rtl8369 > + - realtek,rtl8370 > + description: | > + realtek,rtl8365mb: 4+1 ports > + realtek,rtl8366: > + realtek,rtl8366rb: > + realtek,rtl8366s: 4+1 ports > + realtek,rtl8367: > + realtek,rtl8367b: > + realtek,rtl8368s: 8 ports > + realtek,rtl8369: > + realtek,rtl8370: 8+2 ports > + reg: > + maxItems: 1 > + > + mdc-gpios: > + description: GPIO line for the MDC clock line. > + maxItems: 1 > + > + mdio-gpios: > + description: GPIO line for the MDIO data line. > + maxItems: 1 > + > + reset-gpios: > + description: GPIO to be used to reset the whole device > + maxItems: 1 > + > + realtek,disable-leds: > + type: boolean > + description: | > + if the LED drivers are not used in the > + hardware design this will disable them so they are not turned on > + and wasting power. > + > + interrupt-controller: > + type: object > + description: | > + This defines an interrupt controller with an IRQ line (typically > + a GPIO) that will demultiplex and handle the interrupt from the single > + interrupt line coming out of one of the SMI-based chips. It most > + importantly provides link up/down interrupts to the PHY blocks inside > + the ASIC. > + > + properties: > + > + interrupt-controller: > + description: see interrupt-controller/interrupts.txt Don't need generic descriptions. Just 'true' here is fine. > + > + interrupts: > + description: TODO You have to define how many interrupts and what they are. > + > + '#address-cells': > + const: 0 > + > + '#interrupt-cells': > + const: 1 > + > + required: > + - interrupt-parent 'interrupt-parent' is never required. It's valid for the 'interrupt-parent' to be in any parent node. > + - interrupt-controller > + - '#address-cells' > + - '#interrupt-cells' > + > + mdio: > + type: object > + description: > + This defines the internal MDIO bus of the SMI device, mostly for the > + purpose of being able to hook the interrupts to the right PHY and > + the right PHY to the corresponding port. > + > + properties: > + compatible: > + const: "realtek,smi-mdio" Don't need quotes. blank line between properties. > + '#address-cells': > + const: 1 blank line. > + '#size-cells': > + const: 0 > + > + patternProperties: > + "^(ethernet-)?phy@[0-4]$": > + type: object > + > + allOf: > + - $ref: "http://devicetree.org/schemas/net/mdio.yaml#" This is applied to the wrong level. It should be applied to 'mdio' node. You also need to drop 'http://devicetree.org'. With that, you can drop most of the above. IOW, just this: mdio: $ref: /schemas/net/mdio.yaml# unevaluatedProperties: false properties: compatible: const: realtek,smi-mdio > + > + properties: > + reg: > + maxItems: 1 > + > + required: > + - reg > + > +required: > + - compatible > + - mdc-gpios > + - mdio-gpios > + - reset-gpios > + > +additionalProperties: true No. 'true' is only allowed for common, incomplete schemas referenced by other schemas. 'unevaluatedProperties: false' is what you need here. > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + switch { > + compatible = "realtek,rtl8366rb"; > + /* 22 = MDIO (has input reads), 21 = MDC (clock, output only) */ > + mdc-gpios = <&gpio0 21 GPIO_ACTIVE_HIGH>; > + mdio-gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio0 14 GPIO_ACTIVE_LOW>; > + > + switch_intc1: interrupt-controller { > + /* GPIO 15 provides the interrupt */ > + interrupt-parent = <&gpio0>; > + interrupts = <15 IRQ_TYPE_LEVEL_LOW>; > + interrupt-controller; > + #address-cells = <0>; > + #interrupt-cells = <1>; > + }; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + label = "lan0"; > + phy-handle = <&phy0>; > + }; > + port@1 { > + reg = <1>; > + label = "lan1"; > + phy-handle = <&phy1>; > + }; > + port@2 { > + reg = <2>; > + label = "lan2"; > + phy-handle = <&phy2>; > + }; > + port@3 { > + reg = <3>; > + label = "lan3"; > + phy-handle = <&phy3>; > + }; > + port@4 { > + reg = <4>; > + label = "wan"; > + phy-handle = <&phy4>; > + }; > + port@5 { > + reg = <5>; > + label = "cpu"; > + ethernet = <&gmac0>; > + phy-mode = "rgmii"; > + fixed-link { > + speed = <1000>; > + full-duplex; > + }; > + }; > + }; > + > + mdio { > + compatible = "realtek,smi-mdio"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + phy0: ethernet-phy@0 { > + reg = <0>; > + interrupt-parent = <&switch_intc1>; > + interrupts = <0>; > + }; > + phy1: ethernet-phy@1 { > + reg = <1>; > + interrupt-parent = <&switch_intc1>; > + interrupts = <1>; > + }; > + phy2: ethernet-phy@2 { > + reg = <2>; > + interrupt-parent = <&switch_intc1>; > + interrupts = <2>; > + }; > + phy3: ethernet-phy@3 { > + reg = <3>; > + interrupt-parent = <&switch_intc1>; > + interrupts = <3>; > + }; > + phy4: ethernet-phy@4 { > + reg = <4>; > + interrupt-parent = <&switch_intc1>; > + interrupts = <12>; > + }; > + }; > + }; > + > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + switch { > + compatible = "realtek,rtl8365mb"; > + mdc-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; > + mdio-gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > + > + switch_intc2: interrupt-controller { > + interrupt-parent = <&gpio5>; > + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; > + interrupt-controller; > + #address-cells = <0>; > + #interrupt-cells = <1>; > + }; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + label = "swp0"; > + phy-handle = <ðphy0>; > + }; > + port@1 { > + reg = <1>; > + label = "swp1"; > + phy-handle = <ðphy1>; > + }; > + port@2 { > + reg = <2>; > + label = "swp2"; > + phy-handle = <ðphy2>; > + }; > + port@3 { > + reg = <3>; > + label = "swp3"; > + phy-handle = <ðphy3>; > + }; > + port@6 { > + reg = <6>; > + label = "cpu"; > + ethernet = <&fec1>; > + phy-mode = "rgmii"; > + tx-internal-delay-ps = <2000>; > + rx-internal-delay-ps = <2000>; > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + pause; > + }; > + }; > + }; > + > + mdio { > + compatible = "realtek,smi-mdio"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethphy0: ethernet-phy@0 { > + reg = <0>; > + interrupt-parent = <&switch_intc2>; > + interrupts = <0>; > + }; > + ethphy1: ethernet-phy@1 { > + reg = <1>; > + interrupt-parent = <&switch_intc2>; > + interrupts = <1>; > + }; > + ethphy2: ethernet-phy@2 { > + reg = <2>; > + interrupt-parent = <&switch_intc2>; > + interrupts = <2>; > + }; > + ethphy3: ethernet-phy@3 { > + reg = <3>; > + interrupt-parent = <&switch_intc2>; > + interrupts = <3>; > + }; > + }; > + }; > -- > 2.34.0 > >
On Wed, Jan 5, 2022 at 12:44 AM Luiz Angelo Daros de Luca <luizluca@gmail.com> wrote: > BTW, I couldn't find a datasheet for rtl8366rb. There is none... all I have is a code dump from realtek. The custom header had to be reverse engineered. > The commit message > says it is from a DIR-685 but wikidevi days that device has a > RTL8366SR, which is described as "SINGLE-CHIP 5+1-PORT 10/100/1000 > MBPS SWITCH CONTROLLER WITH DUAL MAC INTERFACES". The device most definitely has the RTL8366RB, as can be seen in the PCB photo here: https://www.redeszone.net/app/uploads-redeszone.net/d-link_dir-685_analisis_15.jpg > Do you have any suggestions? The DIR-685 has WAN + 4 x LAN and the WAN port is handled in a separate register from the LAN ports (suggesting it can also do an optical line) so I think it's 4 + 1. Yours, Linus Walleij
Thanks Rob, now that the code side is merged, I'm back to docs. > > + interrupt-controller: > > + description: see interrupt-controller/interrupts.txt > > Don't need generic descriptions. Just 'true' here is fine. Do you really mean quoted true, like in "description: 'true' "? Without quotes it will fail > > > + > > + interrupts: > > + description: TODO > > You have to define how many interrupts and what they are. I didn't write the interruption code and Linus and Alvin might help here. The switch has a single interrupt pin that signals an interruption happened. The code reads a register to multiplex to these interruptions: INT_TYPE_LINK_STATUS = 0, INT_TYPE_METER_EXCEED, INT_TYPE_LEARN_LIMIT, INT_TYPE_LINK_SPEED, INT_TYPE_CONGEST, INT_TYPE_GREEN_FEATURE, INT_TYPE_LOOP_DETECT, INT_TYPE_8051, INT_TYPE_CABLE_DIAG, INT_TYPE_ACL, INT_TYPE_RESERVED, /* Unused */ INT_TYPE_SLIENT, And most of them, but not all, multiplex again to each port. However, the linux driver today does not care about any of these interruptions but INT_TYPE_LINK_STATUS. So it simply multiplex only this the interruption to each port, in a n-cell map (n being number of ports). I don't know what to describe here as device-tree should be something independent of a particular OS or driver. Anyway, I doubt someone might want to plug one of these interruptions outside the switch driver. Could it be simple as this: interrupts: minItems: 3 maxItems: 10 description: interrupt mapping one per switch port Once realtek-smi.yaml settles, I'll also send the realtek-mdio.yaml. Regards, Luiz
On 29/01/2022 19:02, Luiz Angelo Daros de Luca wrote: > Thanks Rob, now that the code side is merged, I'm back to docs. > > >>> + interrupt-controller: >>> + description: see interrupt-controller/interrupts.txt >> >> Don't need generic descriptions. Just 'true' here is fine. > > Do you really mean quoted true, like in "description: 'true' "? > Without quotes it will fail >> >>> + >>> + interrupts: >>> + description: TODO >> >> You have to define how many interrupts and what they are. > > I didn't write the interruption code and Linus and Alvin might help here. > > The switch has a single interrupt pin that signals an interruption happened. > The code reads a register to multiplex to these interruptions: > > INT_TYPE_LINK_STATUS = 0, > INT_TYPE_METER_EXCEED, > INT_TYPE_LEARN_LIMIT, > INT_TYPE_LINK_SPEED, > INT_TYPE_CONGEST, > INT_TYPE_GREEN_FEATURE, > INT_TYPE_LOOP_DETECT, > INT_TYPE_8051, > INT_TYPE_CABLE_DIAG, > INT_TYPE_ACL, > INT_TYPE_RESERVED, /* Unused */ > INT_TYPE_SLIENT, > > And most of them, but not all, multiplex again to each port. > > However, the linux driver today does not care about any of these > interruptions but INT_TYPE_LINK_STATUS. So it simply multiplex only > this the interruption to each port, in a n-cell map (n being number of > ports). > I don't know what to describe here as device-tree should be something > independent of a particular OS or driver. > > Anyway, I doubt someone might want to plug one of these interruptions > outside the switch driver. Could it be simple as this: > > interrupts: > minItems: 3 > maxItems: 10 > description: > interrupt mapping one per switch port > > Once realtek-smi.yaml settles, I'll also send the realtek-mdio.yaml. Why not turn realtek-smi.yaml into realtek.yaml which would also contain information for the mdio interface? The things different with using MDIO are that we don't use the [mdc,mdio,reset]-gpios properties and don't handle the PHYs to the DSA ports. Couldn't you present these differences on a single YAML file? Arınç
> Why not turn realtek-smi.yaml into realtek.yaml which would also contain > information for the mdio interface? The things different with using MDIO > are that we don't use the [mdc,mdio,reset]-gpios properties and don't > handle the PHYs to the DSA ports. Couldn't you present these differences > on a single YAML file? Hello, Arinç realtek-mdio is an mdio driver with a couple of less properties. They do share a lot of stuff. But I don't know if I can fit the schema validation into a single file. YAML files are not simply documentation. They are used to validate DTS files. But that's still off-topic. Let's finish SMI version first and then discuss if the MDIO version should be standalone or merged with SMI. Regards,
On 1/29/2022 12:52 PM, Luiz Angelo Daros de Luca wrote: >> Why not turn realtek-smi.yaml into realtek.yaml which would also contain >> information for the mdio interface? The things different with using MDIO >> are that we don't use the [mdc,mdio,reset]-gpios properties and don't >> handle the PHYs to the DSA ports. Couldn't you present these differences >> on a single YAML file? > > Hello, Arinç > > realtek-mdio is an mdio driver with a couple of less properties. They > do share a lot of stuff. But I don't know if I can fit the schema > validation into a single file. > YAML files are not simply documentation. They are used to validate DTS > files. But that's still off-topic. Let's finish SMI version first and > then discuss > if the MDIO version should be standalone or merged with SMI. Your YAML file can cover both types of electrical bus, what you are defining is the layout and the properties of the Ethernet switch Device Tree node which is exactly the same whether the switch is the children of a SPI controller or the children of a MDIO bus controller. If there are properties that only apply to SPI or MDIO, you can make use of conditionals within the YAML file to enforce those. Having a single binding file would be very helpful to make sure all eggs are in the same basket.
> Your YAML file can cover both types of electrical bus, what you are > defining is the layout and the properties of the Ethernet switch Device > Tree node which is exactly the same whether the switch is the children > of a SPI controller or the children of a MDIO bus controller. If there > are properties that only apply to SPI or MDIO, you can make use of > conditionals within the YAML file to enforce those. Having a single > binding file would be very helpful to make sure all eggs are in the same > basket. If you say it is possible, I'll give it a try. I'll just need a hand with the interruption section. Luiz
> If there > are properties that only apply to SPI or MDIO, you can make use of > conditionals within the YAML file to enforce those. Having a single > binding file would be very helpful to make sure all eggs are in the same > basket. Sorry Florian but I failed to find a way to test the parent node (platform or mdio) and conditionally offer properties. What I did was to guess if it is an mdio driver or not by checking the "reg" property. Is there a better way to solve it? Luiz PS: I might post the merged v2 doc soon.
On Sat, Jan 29, 2022 at 10:02 AM Luiz Angelo Daros de Luca <luizluca@gmail.com> wrote: > > Thanks Rob, now that the code side is merged, I'm back to docs. Sigh, bindings are supposed to be accepted first... > > > > > + interrupt-controller: > > > + description: see interrupt-controller/interrupts.txt > > > > Don't need generic descriptions. Just 'true' here is fine. > > Do you really mean quoted true, like in "description: 'true' "? > Without quotes it will fail interrupt-controller: true > > > > > + > > > + interrupts: > > > + description: TODO > > > > You have to define how many interrupts and what they are. > > I didn't write the interruption code and Linus and Alvin might help here. > > The switch has a single interrupt pin that signals an interruption happened. Then it's 1 interrupt? > The code reads a register to multiplex to these interruptions: > > INT_TYPE_LINK_STATUS = 0, > INT_TYPE_METER_EXCEED, > INT_TYPE_LEARN_LIMIT, > INT_TYPE_LINK_SPEED, > INT_TYPE_CONGEST, > INT_TYPE_GREEN_FEATURE, > INT_TYPE_LOOP_DETECT, > INT_TYPE_8051, > INT_TYPE_CABLE_DIAG, > INT_TYPE_ACL, > INT_TYPE_RESERVED, /* Unused */ > INT_TYPE_SLIENT, Unless the DT needs to route all these interrupts to multiple nodes, then the switch needs to be an interrupt-controller. > > And most of them, but not all, multiplex again to each port. Now I'm lost. So it's 1 per port, not 1 for the switch? > However, the linux driver today does not care about any of these > interruptions but INT_TYPE_LINK_STATUS. So it simply multiplex only > this the interruption to each port, in a n-cell map (n being number of > ports). > I don't know what to describe here as device-tree should be something > independent of a particular OS or driver. You shouldn't need to know what Linux does to figure this out. > > Anyway, I doubt someone might want to plug one of these interruptions > outside the switch driver. Could it be simple as this: > > interrupts: > minItems: 3 > maxItems: 10 > description: > interrupt mapping one per switch port > > Once realtek-smi.yaml settles, I'll also send the realtek-mdio.yaml. > > Regards, > > Luiz
Hi, Rob Herring <robh@kernel.org> writes: >> > >> > > + >> > > + interrupts: >> > > + description: TODO >> > >> > You have to define how many interrupts and what they are. >> >> I didn't write the interruption code and Linus and Alvin might help here. >> >> The switch has a single interrupt pin that signals an interruption happened. > > Then it's 1 interrupt? Correct. The switch has one physcical interrupt signal. > >> The code reads a register to multiplex to these interruptions: >> >> INT_TYPE_LINK_STATUS = 0, >> INT_TYPE_METER_EXCEED, >> INT_TYPE_LEARN_LIMIT, >> INT_TYPE_LINK_SPEED, >> INT_TYPE_CONGEST, >> INT_TYPE_GREEN_FEATURE, >> INT_TYPE_LOOP_DETECT, >> INT_TYPE_8051, >> INT_TYPE_CABLE_DIAG, >> INT_TYPE_ACL, >> INT_TYPE_RESERVED, /* Unused */ >> INT_TYPE_SLIENT, > > Unless the DT needs to route all these interrupts to multiple nodes, > then the switch needs to be an interrupt-controller. Yes, it is an interrupt-controller, and an interrupt-parent to the phy nodes. > >> >> And most of them, but not all, multiplex again to each port. > > Now I'm lost. So it's 1 per port, not 1 for the switch? There is one physical interrupt signal for these switches. In the switch driver IRQ handler, some registers are inspected to decide what type of event it is. Luiz pasted the enum of possible interrupt types in his mail above. Most of these events are ignored, or are otherwise internal to the switch driver. But in some cases, the interrupt is actually for one of the embedded PHYs in the switch. That's why we make the switch an interrupt-controller. So, in summary: - one interrupt for the switch - the switch is an interrupt-controller - ... and is the interrupt-parent for the phy nodes. The rest is internal details and shouldn't matter, as far as my understanding of device tree bindings goes. Happy to be corrected though. Kind regards, Alvin
> So, in summary: > > - one interrupt for the switch > - the switch is an interrupt-controller > - ... and is the interrupt-parent for the phy nodes. This pattern is pretty common for DSA switches, which have internal PHYs. You can see this in the mv88e6xxx binding for example. Andrew
> > However, the linux driver today does not care about any of these > > interruptions but INT_TYPE_LINK_STATUS. So it simply multiplex only > > this the interruption to each port, in a n-cell map (n being number of > > ports). > > I don't know what to describe here as device-tree should be something > > independent of a particular OS or driver. > > You shouldn't need to know what Linux does to figure this out. The Linux driver is masquerading all those interruptions into a single "link status changed". If interrupts property is about what the HW sends to us, it is a single pin. interrupt-controller: type: object description: | This defines an interrupt controller with an IRQ line (typically a GPIO) that will demultiplex and handle the interrupt from the single interrupt line coming out of one of the Realtek switch chips. It most importantly provides link up/down interrupts to the PHY blocks inside the ASIC. properties: interrupt-controller: true interrupts: maxItems: 1 description: A single IRQ line from the switch, either active LOW or HIGH '#address-cells': const: 0 '#interrupt-cells': const: 1 required: - interrupt-controller - '#address-cells' - '#interrupt-cells' Now as it is also an interrupt-controller, shouldn't I document what it emits? I've just sent the new version and we can discuss it there. > > - one interrupt for the switch > > - the switch is an interrupt-controller > > - ... and is the interrupt-parent for the phy nodes. > > This pattern is pretty common for DSA switches, which have internal > PHYs. You can see this in the mv88e6xxx binding for example. The issue is that those similar devices are still not in yaml format. > Andrew
On 2/9/22 10:43 AM, Luiz Angelo Daros de Luca wrote: >>> However, the linux driver today does not care about any of these >>> interruptions but INT_TYPE_LINK_STATUS. So it simply multiplex only >>> this the interruption to each port, in a n-cell map (n being number of >>> ports). >>> I don't know what to describe here as device-tree should be something >>> independent of a particular OS or driver. >> >> You shouldn't need to know what Linux does to figure this out. > > The Linux driver is masquerading all those interruptions into a single > "link status changed". If interrupts property is about what the HW > sends to us, it is a single pin. > > interrupt-controller: > type: object > description: | > This defines an interrupt controller with an IRQ line (typically > a GPIO) that will demultiplex and handle the interrupt from the single > interrupt line coming out of one of the Realtek switch chips. It most > importantly provides link up/down interrupts to the PHY blocks inside > the ASIC. The de-multiplexing is a software construct/operation, in fact, what the GPIO line does is multiplex since the line is used as an output to the next level interrupt controller it connects to. > > properties: > > interrupt-controller: true > > interrupts: > maxItems: 1 > description: > A single IRQ line from the switch, either active LOW or HIGH > > '#address-cells': > const: 0 > > '#interrupt-cells': > const: 1 > > required: > - interrupt-controller > - '#address-cells' > - '#interrupt-cells' > > Now as it is also an interrupt-controller, shouldn't I document what it emits? The interrupt controller emits a single output towards the next level, and you documented that already with these properties. If you want to go ahead and define what the various interrupt bits map to within the switch's interrupt controller, you can do that in an include/dt-bindings/ header file, or you can just open code the numbers. Up to you. > I've just sent the new version and we can discuss it there. > >>> - one interrupt for the switch >>> - the switch is an interrupt-controller >>> - ... and is the interrupt-parent for the phy nodes. >> >> This pattern is pretty common for DSA switches, which have internal >> PHYs. You can see this in the mv88e6xxx binding for example. > > The issue is that those similar devices are still not in yaml format. That does not mean we could not update dsa.yaml to list the 'interrupts', 'interrupt-controller' and '#interrupt-cells' properties and just leave it to the individual YAML bindings to specify the shape and size of those properties so they don't have to repeat them.
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt b/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt deleted file mode 100644 index 7959ec237983..000000000000 --- a/Documentation/devicetree/bindings/net/dsa/realtek-smi.txt +++ /dev/null @@ -1,240 +0,0 @@ -Realtek SMI-based Switches -========================== - -The SMI "Simple Management Interface" is a two-wire protocol using -bit-banged GPIO that while it reuses the MDIO lines MCK and MDIO does -not use the MDIO protocol. This binding defines how to specify the -SMI-based Realtek devices. - -Required properties: - -- compatible: must be exactly one of: - "realtek,rtl8365mb" (4+1 ports) - "realtek,rtl8366" - "realtek,rtl8366rb" (4+1 ports) - "realtek,rtl8366s" (4+1 ports) - "realtek,rtl8367" - "realtek,rtl8367b" - "realtek,rtl8368s" (8 port) - "realtek,rtl8369" - "realtek,rtl8370" (8 port) - -Required properties: -- mdc-gpios: GPIO line for the MDC clock line. -- mdio-gpios: GPIO line for the MDIO data line. -- reset-gpios: GPIO line for the reset signal. - -Optional properties: -- realtek,disable-leds: if the LED drivers are not used in the - hardware design this will disable them so they are not turned on - and wasting power. - -Required subnodes: - -- interrupt-controller - - This defines an interrupt controller with an IRQ line (typically - a GPIO) that will demultiplex and handle the interrupt from the single - interrupt line coming out of one of the SMI-based chips. It most - importantly provides link up/down interrupts to the PHY blocks inside - the ASIC. - -Required properties of interrupt-controller: - -- interrupt: parent interrupt, see interrupt-controller/interrupts.txt -- interrupt-controller: see interrupt-controller/interrupts.txt -- #address-cells: should be <0> -- #interrupt-cells: should be <1> - -- mdio - - This defines the internal MDIO bus of the SMI device, mostly for the - purpose of being able to hook the interrupts to the right PHY and - the right PHY to the corresponding port. - -Required properties of mdio: - -- compatible: should be set to "realtek,smi-mdio" for all SMI devices - -See net/mdio.txt for additional MDIO bus properties. - -See net/dsa/dsa.txt for a list of additional required and optional properties -and subnodes of DSA switches. - -Examples: - -An example for the RTL8366RB: - -switch { - compatible = "realtek,rtl8366rb"; - /* 22 = MDIO (has input reads), 21 = MDC (clock, output only) */ - mdc-gpios = <&gpio0 21 GPIO_ACTIVE_HIGH>; - mdio-gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>; - reset-gpios = <&gpio0 14 GPIO_ACTIVE_LOW>; - - switch_intc: interrupt-controller { - /* GPIO 15 provides the interrupt */ - interrupt-parent = <&gpio0>; - interrupts = <15 IRQ_TYPE_LEVEL_LOW>; - interrupt-controller; - #address-cells = <0>; - #interrupt-cells = <1>; - }; - - ports { - #address-cells = <1>; - #size-cells = <0>; - reg = <0>; - port@0 { - reg = <0>; - label = "lan0"; - phy-handle = <&phy0>; - }; - port@1 { - reg = <1>; - label = "lan1"; - phy-handle = <&phy1>; - }; - port@2 { - reg = <2>; - label = "lan2"; - phy-handle = <&phy2>; - }; - port@3 { - reg = <3>; - label = "lan3"; - phy-handle = <&phy3>; - }; - port@4 { - reg = <4>; - label = "wan"; - phy-handle = <&phy4>; - }; - port@5 { - reg = <5>; - label = "cpu"; - ethernet = <&gmac0>; - phy-mode = "rgmii"; - fixed-link { - speed = <1000>; - full-duplex; - }; - }; - }; - - mdio { - compatible = "realtek,smi-mdio", "dsa-mdio"; - #address-cells = <1>; - #size-cells = <0>; - - phy0: phy@0 { - reg = <0>; - interrupt-parent = <&switch_intc>; - interrupts = <0>; - }; - phy1: phy@1 { - reg = <1>; - interrupt-parent = <&switch_intc>; - interrupts = <1>; - }; - phy2: phy@2 { - reg = <2>; - interrupt-parent = <&switch_intc>; - interrupts = <2>; - }; - phy3: phy@3 { - reg = <3>; - interrupt-parent = <&switch_intc>; - interrupts = <3>; - }; - phy4: phy@4 { - reg = <4>; - interrupt-parent = <&switch_intc>; - interrupts = <12>; - }; - }; -}; - -An example for the RTL8365MB-VC: - -switch { - compatible = "realtek,rtl8365mb"; - mdc-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; - mdio-gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>; - reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; - - switch_intc: interrupt-controller { - interrupt-parent = <&gpio5>; - interrupts = <1 IRQ_TYPE_LEVEL_LOW>; - interrupt-controller; - #address-cells = <0>; - #interrupt-cells = <1>; - }; - - ports { - #address-cells = <1>; - #size-cells = <0>; - reg = <0>; - port@0 { - reg = <0>; - label = "swp0"; - phy-handle = <ðphy0>; - }; - port@1 { - reg = <1>; - label = "swp1"; - phy-handle = <ðphy1>; - }; - port@2 { - reg = <2>; - label = "swp2"; - phy-handle = <ðphy2>; - }; - port@3 { - reg = <3>; - label = "swp3"; - phy-handle = <ðphy3>; - }; - port@6 { - reg = <6>; - label = "cpu"; - ethernet = <&fec1>; - phy-mode = "rgmii"; - tx-internal-delay-ps = <2000>; - rx-internal-delay-ps = <2000>; - - fixed-link { - speed = <1000>; - full-duplex; - pause; - }; - }; - }; - - mdio { - compatible = "realtek,smi-mdio"; - #address-cells = <1>; - #size-cells = <0>; - - ethphy0: phy@0 { - reg = <0>; - interrupt-parent = <&switch_intc>; - interrupts = <0>; - }; - ethphy1: phy@1 { - reg = <1>; - interrupt-parent = <&switch_intc>; - interrupts = <1>; - }; - ethphy2: phy@2 { - reg = <2>; - interrupt-parent = <&switch_intc>; - interrupts = <2>; - }; - ethphy3: phy@3 { - reg = <3>; - interrupt-parent = <&switch_intc>; - interrupts = <3>; - }; - }; -}; diff --git a/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml b/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml new file mode 100644 index 000000000000..c4cd0038f092 --- /dev/null +++ b/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml @@ -0,0 +1,310 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/dsa/realtek-smi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Realtek SMI-based Switches + +allOf: + - $ref: dsa.yaml# + +maintainers: + - Linus Walleij <linus.walleij@linaro.org> + +description: + The SMI "Simple Management Interface" is a two-wire protocol using + bit-banged GPIO that while it reuses the MDIO lines MCK and MDIO does + not use the MDIO protocol. This binding defines how to specify the + SMI-based Realtek devices. The realtek-smi driver is a platform driver + and it must be inserted inside a platform node. + +properties: + compatible: + oneOf: + - enum: + - realtek,rtl8365mb + - realtek,rtl8366 + - realtek,rtl8366rb + - realtek,rtl8366s + - realtek,rtl8367 + - realtek,rtl8367b + - realtek,rtl8368s + - realtek,rtl8369 + - realtek,rtl8370 + description: | + realtek,rtl8365mb: 4+1 ports + realtek,rtl8366: + realtek,rtl8366rb: + realtek,rtl8366s: 4+1 ports + realtek,rtl8367: + realtek,rtl8367b: + realtek,rtl8368s: 8 ports + realtek,rtl8369: + realtek,rtl8370: 8+2 ports + reg: + maxItems: 1 + + mdc-gpios: + description: GPIO line for the MDC clock line. + maxItems: 1 + + mdio-gpios: + description: GPIO line for the MDIO data line. + maxItems: 1 + + reset-gpios: + description: GPIO to be used to reset the whole device + maxItems: 1 + + realtek,disable-leds: + type: boolean + description: | + if the LED drivers are not used in the + hardware design this will disable them so they are not turned on + and wasting power. + + interrupt-controller: + type: object + description: | + This defines an interrupt controller with an IRQ line (typically + a GPIO) that will demultiplex and handle the interrupt from the single + interrupt line coming out of one of the SMI-based chips. It most + importantly provides link up/down interrupts to the PHY blocks inside + the ASIC. + + properties: + + interrupt-controller: + description: see interrupt-controller/interrupts.txt + + interrupts: + description: TODO + + '#address-cells': + const: 0 + + '#interrupt-cells': + const: 1 + + required: + - interrupt-parent + - interrupt-controller + - '#address-cells' + - '#interrupt-cells' + + mdio: + type: object + description: + This defines the internal MDIO bus of the SMI device, mostly for the + purpose of being able to hook the interrupts to the right PHY and + the right PHY to the corresponding port. + + properties: + compatible: + const: "realtek,smi-mdio" + '#address-cells': + const: 1 + '#size-cells': + const: 0 + + patternProperties: + "^(ethernet-)?phy@[0-4]$": + type: object + + allOf: + - $ref: "http://devicetree.org/schemas/net/mdio.yaml#" + + properties: + reg: + maxItems: 1 + + required: + - reg + +required: + - compatible + - mdc-gpios + - mdio-gpios + - reset-gpios + +additionalProperties: true + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + switch { + compatible = "realtek,rtl8366rb"; + /* 22 = MDIO (has input reads), 21 = MDC (clock, output only) */ + mdc-gpios = <&gpio0 21 GPIO_ACTIVE_HIGH>; + mdio-gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio0 14 GPIO_ACTIVE_LOW>; + + switch_intc1: interrupt-controller { + /* GPIO 15 provides the interrupt */ + interrupt-parent = <&gpio0>; + interrupts = <15 IRQ_TYPE_LEVEL_LOW>; + interrupt-controller; + #address-cells = <0>; + #interrupt-cells = <1>; + }; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + label = "lan0"; + phy-handle = <&phy0>; + }; + port@1 { + reg = <1>; + label = "lan1"; + phy-handle = <&phy1>; + }; + port@2 { + reg = <2>; + label = "lan2"; + phy-handle = <&phy2>; + }; + port@3 { + reg = <3>; + label = "lan3"; + phy-handle = <&phy3>; + }; + port@4 { + reg = <4>; + label = "wan"; + phy-handle = <&phy4>; + }; + port@5 { + reg = <5>; + label = "cpu"; + ethernet = <&gmac0>; + phy-mode = "rgmii"; + fixed-link { + speed = <1000>; + full-duplex; + }; + }; + }; + + mdio { + compatible = "realtek,smi-mdio"; + #address-cells = <1>; + #size-cells = <0>; + + phy0: ethernet-phy@0 { + reg = <0>; + interrupt-parent = <&switch_intc1>; + interrupts = <0>; + }; + phy1: ethernet-phy@1 { + reg = <1>; + interrupt-parent = <&switch_intc1>; + interrupts = <1>; + }; + phy2: ethernet-phy@2 { + reg = <2>; + interrupt-parent = <&switch_intc1>; + interrupts = <2>; + }; + phy3: ethernet-phy@3 { + reg = <3>; + interrupt-parent = <&switch_intc1>; + interrupts = <3>; + }; + phy4: ethernet-phy@4 { + reg = <4>; + interrupt-parent = <&switch_intc1>; + interrupts = <12>; + }; + }; + }; + + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + switch { + compatible = "realtek,rtl8365mb"; + mdc-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; + mdio-gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; + + switch_intc2: interrupt-controller { + interrupt-parent = <&gpio5>; + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; + interrupt-controller; + #address-cells = <0>; + #interrupt-cells = <1>; + }; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + label = "swp0"; + phy-handle = <ðphy0>; + }; + port@1 { + reg = <1>; + label = "swp1"; + phy-handle = <ðphy1>; + }; + port@2 { + reg = <2>; + label = "swp2"; + phy-handle = <ðphy2>; + }; + port@3 { + reg = <3>; + label = "swp3"; + phy-handle = <ðphy3>; + }; + port@6 { + reg = <6>; + label = "cpu"; + ethernet = <&fec1>; + phy-mode = "rgmii"; + tx-internal-delay-ps = <2000>; + rx-internal-delay-ps = <2000>; + + fixed-link { + speed = <1000>; + full-duplex; + pause; + }; + }; + }; + + mdio { + compatible = "realtek,smi-mdio"; + #address-cells = <1>; + #size-cells = <0>; + + ethphy0: ethernet-phy@0 { + reg = <0>; + interrupt-parent = <&switch_intc2>; + interrupts = <0>; + }; + ethphy1: ethernet-phy@1 { + reg = <1>; + interrupt-parent = <&switch_intc2>; + interrupts = <1>; + }; + ethphy2: ethernet-phy@2 { + reg = <2>; + interrupt-parent = <&switch_intc2>; + interrupts = <2>; + }; + ethphy3: ethernet-phy@3 { + reg = <3>; + interrupt-parent = <&switch_intc2>; + interrupts = <3>; + }; + }; + };
Schema changes: - "interrupt-controller" was not added as a required property. It might still work polling the ports when missing - "interrupt" property was mentioned but never used. According to its description, it was assumed it was really "interrupt-parent" Examples changes: - renamed "switch_intc" to make it unique between examples - removed "dsa-mdio" from mdio compatible property - renamed phy@0 to ethernet-phy@0 (not tested with real HW) phy@ requires #phy-cells Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- .../bindings/net/dsa/realtek-smi.txt | 240 -------------- .../bindings/net/dsa/realtek-smi.yaml | 310 ++++++++++++++++++ 2 files changed, 310 insertions(+), 240 deletions(-) delete mode 100644 Documentation/devicetree/bindings/net/dsa/realtek-smi.txt create mode 100644 Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml