| Message ID | 20221108082330.2086671-4-lukma@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: Add support for mv88e6020 and mv88e6071 | expand |
On Tue, Nov 08, 2022 at 09:23:24AM +0100, Lukasz Majewski wrote: > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > Avoid the need to specify a PHY for each physical port in the device tree > when phy_base_addr is not 0 (6250 and 6341 families). > > This change should be backwards-compatible with existing device trees, > as it only adds sensible defaults where explicit definitions were > required before. > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> Needs your Signed-off-by tag as well. > --- Would it be possible to do like armada-3720-turris-mox.dts does, and put the phy-handle in the device tree, avoiding the need for so many PHY address translation quirks? If you're going to have U-Boot support for this switch as well, the phy-handle mechanism is the only thing that U-Boot supports, so device trees written in this way will work for both (and can be passed by U-Boot to Linux): switch1@11 { compatible = "marvell,mv88e6190"; reg = <0x11>; dsa,member = <0 1>; interrupt-parent = <&moxtet>; interrupts = <MOXTET_IRQ_PERIDOT(1)>; status = "disabled"; mdio { #address-cells = <1>; #size-cells = <0>; switch1phy1: switch1phy1@1 { reg = <0x1>; }; switch1phy2: switch1phy2@2 { reg = <0x2>; }; switch1phy3: switch1phy3@3 { reg = <0x3>; }; switch1phy4: switch1phy4@4 { reg = <0x4>; }; switch1phy5: switch1phy5@5 { reg = <0x5>; }; switch1phy6: switch1phy6@6 { reg = <0x6>; }; switch1phy7: switch1phy7@7 { reg = <0x7>; }; switch1phy8: switch1phy8@8 { reg = <0x8>; }; }; ports { #address-cells = <1>; #size-cells = <0>; port@1 { reg = <0x1>; label = "lan9"; phy-handle = <&switch1phy1>; }; port@2 { reg = <0x2>; label = "lan10"; phy-handle = <&switch1phy2>; }; port@3 { reg = <0x3>; label = "lan11"; phy-handle = <&switch1phy3>; }; port@4 { reg = <0x4>; label = "lan12"; phy-handle = <&switch1phy4>; }; port@5 { reg = <0x5>; label = "lan13"; phy-handle = <&switch1phy5>; }; port@6 { reg = <0x6>; label = "lan14"; phy-handle = <&switch1phy6>; }; port@7 { reg = <0x7>; label = "lan15"; phy-handle = <&switch1phy7>; }; port@8 { reg = <0x8>; label = "lan16"; phy-handle = <&switch1phy8>; }; switch1port9: port@9 { reg = <0x9>; label = "dsa"; phy-mode = "2500base-x"; managed = "in-band-status"; link = <&switch0port10>; }; switch1port10: port@a { reg = <0xa>; label = "dsa"; phy-mode = "2500base-x"; managed = "in-band-status"; link = <&switch2port9>; status = "disabled"; }; port-sfp@a { reg = <0xa>; label = "sfp"; sfp = <&sfp>; phy-mode = "sgmii"; managed = "in-band-status"; status = "disabled"; }; }; };
> Would it be possible to do like armada-3720-turris-mox.dts does, and put > the phy-handle in the device tree, avoiding the need for so many PHY > address translation quirks? > > If you're going to have U-Boot support for this switch as well, the > phy-handle mechanism is the only thing that U-Boot supports, so device > trees written in this way will work for both (and can be passed by > U-Boot to Linux): This is how i expect any board using the MV88E6141 and MV88E6341 work. It has the same issue that it is not a 1:1 mapping. Portability with U-boot is an interesting argument. Maybe there are patches to u-boot to add the same sort of quirks? Andrew
Hi Vladimir, > On Tue, Nov 08, 2022 at 09:23:24AM +0100, Lukasz Majewski wrote: > > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > > > Avoid the need to specify a PHY for each physical port in the > > device tree when phy_base_addr is not 0 (6250 and 6341 families). > > > > This change should be backwards-compatible with existing device > > trees, as it only adds sensible defaults where explicit definitions > > were required before. > > > > Signed-off-by: Matthias Schiffer > > <matthias.schiffer@ew.tq-group.com> > > Needs your Signed-off-by tag as well. Ok. > > > --- > > Would it be possible to do like armada-3720-turris-mox.dts does, and > put the phy-handle in the device tree, avoiding the need for so many > PHY address translation quirks? As far as I can tell - the mv88e6xxx driver (./drivers/net/dsa/mv88e6xxx) now uses hardcoded values for each member of mv88e6xxx_info struct. Those values are "port_base_addr" and "phy_base_addr". Those values could be read from DTS description as pasted below. > > If you're going to have U-Boot support for this switch as well, the > phy-handle mechanism is the only thing that U-Boot supports, so device > trees written in this way will work for both (and can be passed by > U-Boot to Linux): > > switch1@11 { > compatible = "marvell,mv88e6190"; > reg = <0x11>; > dsa,member = <0 1>; > interrupt-parent = <&moxtet>; > interrupts = <MOXTET_IRQ_PERIDOT(1)>; > status = "disabled"; > > mdio { > #address-cells = <1>; > #size-cells = <0>; > > switch1phy1: switch1phy1@1 { > reg = <0x1>; > }; > > switch1phy2: switch1phy2@2 { > reg = <0x2>; > }; > > switch1phy3: switch1phy3@3 { > reg = <0x3>; > }; > > switch1phy4: switch1phy4@4 { > reg = <0x4>; > }; > > switch1phy5: switch1phy5@5 { > reg = <0x5>; > }; > > switch1phy6: switch1phy6@6 { > reg = <0x6>; > }; > > switch1phy7: switch1phy7@7 { > reg = <0x7>; > }; > > switch1phy8: switch1phy8@8 { > reg = <0x8>; > }; > }; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@1 { > reg = <0x1>; > label = "lan9"; > phy-handle = <&switch1phy1>; > }; > > port@2 { > reg = <0x2>; > label = "lan10"; > phy-handle = <&switch1phy2>; > }; > > port@3 { > reg = <0x3>; > label = "lan11"; > phy-handle = <&switch1phy3>; > }; > > port@4 { > reg = <0x4>; > label = "lan12"; > phy-handle = <&switch1phy4>; > }; > > port@5 { > reg = <0x5>; > label = "lan13"; > phy-handle = <&switch1phy5>; > }; > > port@6 { > reg = <0x6>; > label = "lan14"; > phy-handle = <&switch1phy6>; > }; > > port@7 { > reg = <0x7>; > label = "lan15"; > phy-handle = <&switch1phy7>; > }; > > port@8 { > reg = <0x8>; > label = "lan16"; > phy-handle = <&switch1phy8>; > }; > > switch1port9: port@9 { > reg = <0x9>; > label = "dsa"; > phy-mode = "2500base-x"; > managed = "in-band-status"; > link = <&switch0port10>; > }; > > switch1port10: port@a { > reg = <0xa>; > label = "dsa"; > phy-mode = "2500base-x"; > managed = "in-band-status"; > link = <&switch2port9>; > status = "disabled"; > }; > > port-sfp@a { > reg = <0xa>; > label = "sfp"; > sfp = <&sfp>; > phy-mode = "sgmii"; > managed = "in-band-status"; > status = "disabled"; > }; > }; > }; The u-boot mailine has basic support for mv88e6071 and mv88e6020 (and also some 'extension' patches which are floating around [1]). For the current code - I'm using: mdio { #address-cells = <1>; #size-cells = <0>; switch@0 { compatible = "marvell,mv88e6250"; reg = <0x00>; interrupt-parent = <&gpio2>; interrupts = <20 IRQ_TYPE_LEVEL_LOW>; interrupt-controller; #interrupt-cells = <2>; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "lan1"; }; port@1 { reg = <1>; label = "lan2"; }; port@2 { reg = <2>; label = "lan3"; }; port@5 { reg = <5>; label = "cpu"; phy-mode = "rgmii-id"; ethernet = <&fec1>; fixed-link { speed = <100>; full-duplex; }; }; }; The only "hack" which I see from time to time is the replacement of 'switch@0' with 'switch@8' to take into account the R0_LED/ADDRES4 bootstrap pin value (to shift up ports addresses). Links: [1] - https://lists.denx.de/pipermail/u-boot/2021-March/444827.html Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Andrew, > > Would it be possible to do like armada-3720-turris-mox.dts does, > > and put the phy-handle in the device tree, avoiding the need for so > > many PHY address translation quirks? > > > > If you're going to have U-Boot support for this switch as well, the > > phy-handle mechanism is the only thing that U-Boot supports, Maybe in the generic case of PHY, yes (via Driver Model). However, when you delve into the mv88e6xxx driver [1] - you would find that this is not supporting it yet ... > > so > > device trees written in this way will work for both (and can be > > passed by U-Boot to Linux): > > This is how i expect any board using the MV88E6141 and MV88E6341 work. > It has the same issue that it is not a 1:1 mapping. Please be aware that there is also majority of DTS entries, which use the "old" switch description ... > > Portability with U-boot is an interesting argument. Maybe there are > patches to u-boot to add the same sort of quirks? As fair as I know - for the driver [1] - there was no ongoing effort recently. > > Andrew Links: [1] - https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/mv88e61xx.c Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Thu, Nov 10, 2022 at 06:00:53PM +0100, Lukasz Majewski wrote: > Maybe in the generic case of PHY, yes (via Driver Model). > > However, when you delve into the mv88e6xxx driver [1] - you would find > that this is not supporting it yet ... > > As fair as I know - for the driver [1] - there was no ongoing effort > recently. > > Links: > [1] - > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/mv88e61xx.c U-Boot mailing list is moving a bit slower than netdev (and the patch set is not yet applied despite being ready), but I was talking about this: https://patchwork.ozlabs.org/project/uboot/list/?series=324983 If you study DM_DSA, you'll see that only supporting PHY connection via phy-handle (even if the PHY is internal) (or fixed-link, in lack of a PHY) was an absolutely deliberate decision.
Hi Vladimir, > On Thu, Nov 10, 2022 at 06:00:53PM +0100, Lukasz Majewski wrote: > > Maybe in the generic case of PHY, yes (via Driver Model). > > > > However, when you delve into the mv88e6xxx driver [1] - you would > > find that this is not supporting it yet ... > > > > As fair as I know - for the driver [1] - there was no ongoing effort > > recently. > > > > Links: > > [1] - > > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/mv88e61xx.c > > > > U-Boot mailing list is moving a bit slower than netdev (and the patch > set is not yet applied despite being ready), but I was talking about > this: > https://patchwork.ozlabs.org/project/uboot/list/?series=324983 > Thanks for sharing this link. It looks like this driver also supports switchnig addresses for Marvell ICs. (This was the goal for mine previous patches). > If you study DM_DSA, you'll see that only supporting PHY connection > via phy-handle (even if the PHY is internal) (or fixed-link, in lack > of a PHY) was an absolutely deliberate decision. Ok. I do agree now - will adjust the code accordingly. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 2479be3a1e35..d51fd1966be9 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -6362,6 +6362,13 @@ static enum dsa_tag_protocol mv88e6xxx_get_tag_protocol(struct dsa_switch *ds, return chip->tag_protocol; } +static int mv88e6xxx_get_phy_address(struct dsa_switch *ds, int port) +{ + struct mv88e6xxx_chip *chip = ds->priv; + + return chip->phy_base_addr + port; +} + static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds, enum dsa_tag_protocol proto) { @@ -6887,6 +6894,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = { .phylink_mac_link_up = mv88e6xxx_mac_link_up, .get_strings = mv88e6xxx_get_strings, .get_ethtool_stats = mv88e6xxx_get_ethtool_stats, + .get_phy_address = mv88e6xxx_get_phy_address, .get_sset_count = mv88e6xxx_get_sset_count, .port_enable = mv88e6xxx_port_enable, .port_disable = mv88e6xxx_port_disable,