| Message ID | 20230601100005.2216345-3-lukma@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | Provide support for mv88e6020 Marvell switch | expand |
On 6/1/23 12:00, Lukasz Majewski wrote: > After this change PHY ports are able to pass packets between them (and > also to CPU port). > > The Kconfig variable - CONFIG_MV88E61XX_PHY_PORTS - is used to get the > PHY ports of the switch and generate proper mask. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com> Was there a V0 before ? Or where did these RB tags come from ? > --- > > drivers/net/phy/mv88e61xx.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c > index 31f9b57456d6..4aee83551beb 100644 > --- a/drivers/net/phy/mv88e61xx.c > +++ b/drivers/net/phy/mv88e61xx.c > @@ -865,14 +865,19 @@ static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy) > > static int mv88e61xx_phy_config_port(struct phy_device *phydev, u8 phy) > { > + struct mv88e61xx_phy_priv *priv = phydev->priv; > + u16 port_mask; > int val; > > val = mv88e61xx_port_enable(phydev, phy); > if (val < 0) > return val; > > - val = mv88e61xx_port_set_vlan(phydev, phy, > - 1 << CONFIG_MV88E61XX_CPU_PORT); > + port_mask = PORT_MASK(priv->port_count) & CONFIG_MV88E61XX_PHY_PORTS; > + port_mask &= ~(1 << phy); > + port_mask |= (1 << CONFIG_MV88E61XX_CPU_PORT); BIT() ?
Hi Marek, > On 6/1/23 12:00, Lukasz Majewski wrote: > > After this change PHY ports are able to pass packets between them > > (and also to CPU port). > > > > The Kconfig variable - CONFIG_MV88E61XX_PHY_PORTS - is used to get > > the PHY ports of the switch and generate proper mask. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Reviewed-by: Ramon Fried <rfried.dev@gmail.com> > > Was there a V0 before ? Or where did these RB tags come from ? > There was v1 in 2021 - The patman is not allowing "v1 RESEND" tag adding. > > --- > > > > drivers/net/phy/mv88e61xx.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/phy/mv88e61xx.c > > b/drivers/net/phy/mv88e61xx.c index 31f9b57456d6..4aee83551beb > > 100644 --- a/drivers/net/phy/mv88e61xx.c > > +++ b/drivers/net/phy/mv88e61xx.c > > @@ -865,14 +865,19 @@ static int mv88e61xx_phy_setup(struct > > phy_device *phydev, u8 phy) > > static int mv88e61xx_phy_config_port(struct phy_device *phydev, > > u8 phy) { > > + struct mv88e61xx_phy_priv *priv = phydev->priv; > > + u16 port_mask; > > int val; > > > > val = mv88e61xx_port_enable(phydev, phy); > > if (val < 0) > > return val; > > > > - val = mv88e61xx_port_set_vlan(phydev, phy, > > - 1 << CONFIG_MV88E61XX_CPU_PORT); > > + port_mask = PORT_MASK(priv->port_count) & > > CONFIG_MV88E61XX_PHY_PORTS; > > + port_mask &= ~(1 << phy); > > + port_mask |= (1 << CONFIG_MV88E61XX_CPU_PORT); > > BIT() ? I've kept the "style" from the rest of the file. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter 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 6/1/23 13:02, Lukasz Majewski wrote: > Hi Marek, > >> On 6/1/23 12:00, Lukasz Majewski wrote: >>> After this change PHY ports are able to pass packets between them >>> (and also to CPU port). >>> >>> The Kconfig variable - CONFIG_MV88E61XX_PHY_PORTS - is used to get >>> the PHY ports of the switch and generate proper mask. >>> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>> Reviewed-by: Ramon Fried <rfried.dev@gmail.com> >> >> Was there a V0 before ? Or where did these RB tags come from ? >> > > There was v1 in 2021 - The patman is not allowing "v1 RESEND" tag > adding. I think after two years, it would be good to drop the RB tags and do another round of reviews.
Hi Lukasz, On Thu, Jun 01, 2023 at 01:44:30PM +0200, Marek Vasut wrote: > I think after two years, it would be good to drop the RB tags and do another > round of reviews. To expand on Marek's point. In those past 2 years, Tim Harvey has put in a considerable amount of effort to add another driver for mv88e6xxx that uses DM_DSA. I believe the current "PHY" driver for the same hardware should be considered obsolete until all platforms are converted to DM_DSA, then it can be deleted. So, no new features for it. Then, there's also the question of the sanity of the proposed change itself. I believe that we need to be humble enough to recognize that the U-Boot network stack is not competent enough to handle the switching capabilities of a switch, not even enough for it to be safe. It doesn't handle STP (Spanning Tree Protocol), for one thing. So it will never be capable of detecting switching loops, such as to block one of its ports in order to not kill the network. In principle, I would say: as long as there is no plan to handle STP, there should be no plan to allow autonomous packet forwarding from U-Boot. The U-Boot network stack is there so that you can TFTP a kernel and boot it, which is also the only use case behind DM_DSA. But you may say: I'm never going to allow packet forwarding from U-Boot in a network with loops! Okay, but your patch suggests otherwise. Which ports allow forwarding is a compile-time option, which... is by definition contrary to any runtime network topology determinations. Maybe enabling forwarding between switch ports through a CLI command that communicates with DM_DSA would be tolerable - assuming that users are smart enough to not use it in a network with STP. But again, I'm not really sure what's the use case.
Hi Vladimir, > Hi Lukasz, > > On Thu, Jun 01, 2023 at 01:44:30PM +0200, Marek Vasut wrote: > > I think after two years, it would be good to drop the RB tags and > > do another round of reviews. > > To expand on Marek's point. > > In those past 2 years, Tim Harvey has put in a considerable amount of > effort to add another driver for mv88e6xxx that uses DM_DSA. I believe > the current "PHY" driver for the same hardware should be considered > obsolete until all platforms are converted to DM_DSA, then it can be > deleted. So, no new features for it. Ok. Thanks for the clarification. > > Then, there's also the question of the sanity of the proposed change > itself. > > I believe that we need to be humble enough to recognize that the > U-Boot network stack is not competent enough to handle the switching > capabilities of a switch, not even enough for it to be safe. It > doesn't handle STP (Spanning Tree Protocol), for one thing. So it > will never be capable of detecting switching loops, such as to block > one of its ports in order to not kill the network. > > In principle, I would say: as long as there is no plan to handle STP, > there should be no plan to allow autonomous packet forwarding from > U-Boot. The U-Boot network stack is there so that you can TFTP a > kernel and boot it, which is also the only use case behind DM_DSA. > > But you may say: I'm never going to allow packet forwarding from > U-Boot in a network with loops! > > Okay, but your patch suggests otherwise. Which ports allow forwarding > is a compile-time option, which... is by definition contrary to any > runtime network topology determinations. > > Maybe enabling forwarding between switch ports through a CLI command > that communicates with DM_DSA would be tolerable - assuming that users > are smart enough to not use it in a network with STP. No - adding extra command line option is not planned. > But again, I'm > not really sure what's the use case. This is a simple use case - the board on which I do work has two LAN ports, so the device is "attached" to the single LAN cable. When booting the device (even in u-boot) - packet's shall be forwarded between LAN ports to keep the ETH connection. However, I don't want to add STP for the u-boot network stack. I've also looked to Tim's patch set [1] (also in mainline), and I do believe that some extra features for this driver can be added; like ADDR4 (so the address is 'shifted' to 0x10) and !NO_CPU bootstraps (so ports need to be reset at power up). Those features are common for all mv88e6xxx devices. Links: [1] - https://patchwork.ozlabs.org/project/uboot/cover/20221130174251.82087-1-tharvey@gateworks.com/ Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter 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/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c index 31f9b57456d6..4aee83551beb 100644 --- a/drivers/net/phy/mv88e61xx.c +++ b/drivers/net/phy/mv88e61xx.c @@ -865,14 +865,19 @@ static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy) static int mv88e61xx_phy_config_port(struct phy_device *phydev, u8 phy) { + struct mv88e61xx_phy_priv *priv = phydev->priv; + u16 port_mask; int val; val = mv88e61xx_port_enable(phydev, phy); if (val < 0) return val; - val = mv88e61xx_port_set_vlan(phydev, phy, - 1 << CONFIG_MV88E61XX_CPU_PORT); + port_mask = PORT_MASK(priv->port_count) & CONFIG_MV88E61XX_PHY_PORTS; + port_mask &= ~(1 << phy); + port_mask |= (1 << CONFIG_MV88E61XX_CPU_PORT); + + val = mv88e61xx_port_set_vlan(phydev, phy, port_mask); if (val < 0) return val;