| Message ID | E1qTkRn-003NBG-FH@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: dsa: mv88e6060: add phylink_get_caps implementation | expand |
Hi Russell, On Wed, Aug 09, 2023 at 03:46:03PM +0100, Russell King (Oracle) wrote: > Add a phylink_get_caps implementation for Marvell 88e6060 DSA switch. > This is a fast ethernet switch, with internal PHYs for ports 0 through > 4. Port 4 also supports MII, REVMII, REVRMII and SNI. Port 5 supports > MII, REVMII, REVRMII and SNI without an internal PHY. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > Sergei, > > Would it be possible for you to check that this patch works with your > setup please? > > Thanks! > > drivers/net/dsa/mv88e6060.c | 46 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c > index fdda62d6eb16..0e776be5e941 100644 > --- a/drivers/net/dsa/mv88e6060.c > +++ b/drivers/net/dsa/mv88e6060.c > @@ -247,11 +247,57 @@ mv88e6060_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val) > return reg_write(priv, addr, regnum, val); > } > > +static void mv88e6060_phylink_get_caps(struct dsa_switch *ds, int port, > + struct phylink_config *config) > +{ > + unsigned long *interfaces = config->supported_interfaces; > + struct mv88e6060_priv *priv = ds->priv; > + int addr = REG_PORT(port); > + int ret; > + > + ret = reg_read(priv, addr, PORT_STATUS); > + if (ret < 0) { > + dev_err(ds->dev, > + "port %d: unable to read status register: %pe\n", > + port, ERR_PTR(ret)); > + return; > + } > + > + if (!(ret & PORT_STATUS_PORTMODE)) { > + /* Port configured in SNI mode (acts as a 10Mbps PHY) */ > + config->mac_capabilities = MAC_10 | MAC_SYM_PAUSE; > + /* I don't think SNI is SMII - SMII has a sync signal, and > + * SNI doesn't. > + */ > + __set_bit(PHY_INTERFACE_MODE_SMII, interfaces); I don't think that SNI == SMII either. From what I could gather (datasheets of implementations in the wild, rather than any official spec): KSZ8895: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8895MQX-RQX-FQX-MLX-Integrated-5-Port-10-100-Managed-Ethernet-Switch-with-MII-RMII-Interface-DS00002246C.pdf DP83910A: https://www.ti.com/lit/ds/symlink/dp83910a.pdf RTL8201BL: https://file.elecfans.com/web1/M00/9A/9F/pIYBAF0fDAuANJTlAAqoYDastvw919.pdf SNI (7-wire Serial Network Interface) has the same structural pin layout as the parallel MII/Rev-MII, except for the fact that RXD[3:0] and TXD[3:0] became RXD0 and TXD0, resulting in an effectively "serial" interface and reducing the baud rate of the 100 Mbps MII to a quarter, and the TX_CLK and RX_CLK signals also operate at a reduced 10 MHz rather than MII's 25 MHz, to provide a further 2.5x baud rate reduction down to 10 Mbps. It was a once popular (in the 1990s) interface mode between a MAC and a 10Mbps-only PHY. If we compare that to SMII (Serial MII), I could only find this document here: https://opencores.org/ocsvn/smii/smii/trunk/doc/SMII.pdf but it appears to be quite different. SMII seems to be a gasket/encapsulation module which serializes both the data and control signals of up to 4 10/100 Mbps MACs, which can be connected to a quad SMII PHY. The resulting (cumulated, or individual) bandwidth is much larger than that of SNI, too. The pinout of SMII is: - one RX and one TX signal for each MAC. Data transfer consists of segments (10 serial bits on these lines). The bits in each segment are: RX direction: CRS, RX_DV, RXD0, RXD1, RXD2, RXD3, RXD4, RXD5, RXD6, RXD7 TX direction: TX_ER, TX_EN, TXD0, TXD1, TXD2, TXD3, TXD4, TXD5, TXD6, TXD7 - SYNC: denotes the beginning of a new segment - CLK: denotes the beginning of a new bit So, I guess we have to introduce PHY_INTERFACE_MODE_SNI rather than pretend it is the same as SMII. The rest looks ok (but, as SNI is a 10Mbps only interface, you could, in v2, populate config->mac_capabilities in a common code path to MAC_100 | MAC_10, and let phylink_get_capabilities() reduce it). pw-bot: cr > + return; > + } > + > + config->mac_capabilities = MAC_100 | MAC_10 | MAC_SYM_PAUSE; > + > + if (port >= 4) { > + /* Ports 4 and 5 can support MII, REVMII and REVRMII modes */ > + __set_bit(PHY_INTERFACE_MODE_MII, interfaces); > + __set_bit(PHY_INTERFACE_MODE_REVMII, interfaces); > + __set_bit(PHY_INTERFACE_MODE_REVRMII, interfaces); > + } > + if (port <= 4) { > + /* Ports 0 to 3 have internal PHYs, and port 4 can optionally > + * use an internal PHY. > + */ > + /* Internal PHY */ > + __set_bit(PHY_INTERFACE_MODE_INTERNAL, interfaces); > + /* Default phylib interface mode */ > + __set_bit(PHY_INTERFACE_MODE_GMII, interfaces); > + } > +} > + > static const struct dsa_switch_ops mv88e6060_switch_ops = { > .get_tag_protocol = mv88e6060_get_tag_protocol, > .setup = mv88e6060_setup, > .phy_read = mv88e6060_phy_read, > .phy_write = mv88e6060_phy_write, > + .phylink_get_caps = mv88e6060_phylink_get_caps, > }; > > static int mv88e6060_probe(struct mdio_device *mdiodev) > -- > 2.30.2 >
On Thu, Aug 10, 2023 at 07:44:41PM +0300, Vladimir Oltean wrote: > Hi Russell, > > On Wed, Aug 09, 2023 at 03:46:03PM +0100, Russell King (Oracle) wrote: > > Add a phylink_get_caps implementation for Marvell 88e6060 DSA switch. > > This is a fast ethernet switch, with internal PHYs for ports 0 through > > 4. Port 4 also supports MII, REVMII, REVRMII and SNI. Port 5 supports > > MII, REVMII, REVRMII and SNI without an internal PHY. > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > Sergei, > > > > Would it be possible for you to check that this patch works with your > > setup please? > > > > Thanks! > > > > drivers/net/dsa/mv88e6060.c | 46 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > > diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c > > index fdda62d6eb16..0e776be5e941 100644 > > --- a/drivers/net/dsa/mv88e6060.c > > +++ b/drivers/net/dsa/mv88e6060.c > > @@ -247,11 +247,57 @@ mv88e6060_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val) > > return reg_write(priv, addr, regnum, val); > > } > > > > +static void mv88e6060_phylink_get_caps(struct dsa_switch *ds, int port, > > + struct phylink_config *config) > > +{ > > + unsigned long *interfaces = config->supported_interfaces; > > + struct mv88e6060_priv *priv = ds->priv; > > + int addr = REG_PORT(port); > > + int ret; > > + > > + ret = reg_read(priv, addr, PORT_STATUS); > > + if (ret < 0) { > > + dev_err(ds->dev, > > + "port %d: unable to read status register: %pe\n", > > + port, ERR_PTR(ret)); > > + return; > > + } > > + > > + if (!(ret & PORT_STATUS_PORTMODE)) { > > + /* Port configured in SNI mode (acts as a 10Mbps PHY) */ > > + config->mac_capabilities = MAC_10 | MAC_SYM_PAUSE; > > + /* I don't think SNI is SMII - SMII has a sync signal, and > > + * SNI doesn't. > > + */ > > + __set_bit(PHY_INTERFACE_MODE_SMII, interfaces); > > I don't think that SNI == SMII either. > > From what I could gather (datasheets of implementations in the wild, > rather than any official spec): > KSZ8895: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8895MQX-RQX-FQX-MLX-Integrated-5-Port-10-100-Managed-Ethernet-Switch-with-MII-RMII-Interface-DS00002246C.pdf > DP83910A: https://www.ti.com/lit/ds/symlink/dp83910a.pdf > RTL8201BL: https://file.elecfans.com/web1/M00/9A/9F/pIYBAF0fDAuANJTlAAqoYDastvw919.pdf > > SNI (7-wire Serial Network Interface) has the same structural pin layout > as the parallel MII/Rev-MII, except for the fact that RXD[3:0] and > TXD[3:0] became RXD0 and TXD0, resulting in an effectively "serial" > interface and reducing the baud rate of the 100 Mbps MII to a quarter, > and the TX_CLK and RX_CLK signals also operate at a reduced 10 MHz > rather than MII's 25 MHz, to provide a further 2.5x baud rate reduction > down to 10 Mbps. It was a once popular (in the 1990s) interface mode > between a MAC and a 10Mbps-only PHY. > > If we compare that to SMII (Serial MII), I could only find this document here: > https://opencores.org/ocsvn/smii/smii/trunk/doc/SMII.pdf > > but it appears to be quite different. SMII seems to be a gasket/encapsulation > module which serializes both the data and control signals of up to 4 10/100 > Mbps MACs, which can be connected to a quad SMII PHY. The resulting > (cumulated, or individual) bandwidth is much larger than that of SNI, too. > > The pinout of SMII is: > - one RX and one TX signal for each MAC. Data transfer consists of > segments (10 serial bits on these lines). The bits in each segment are: > RX direction: CRS, RX_DV, RXD0, RXD1, RXD2, RXD3, RXD4, RXD5, RXD6, RXD7 > TX direction: TX_ER, TX_EN, TXD0, TXD1, TXD2, TXD3, TXD4, TXD5, TXD6, TXD7 > - SYNC: denotes the beginning of a new segment > - CLK: denotes the beginning of a new bit > > So, I guess we have to introduce PHY_INTERFACE_MODE_SNI rather than > pretend it is the same as SMII. I wonder whether we have any implementation using SNI mode. I couldn't find anything in the in-kernel dts files for this driver, the only dts we have is one that was posted on-list recently, and that was using MII at 100Mbps: https://lore.kernel.org/r/CABikg9zfGVEJsWf7eq=K5oKQozt86LLn-rzMaVmycekXkQEa8Q@mail.gmail.com No one would be able to specify "sni" in their dts, so maybe for the sake of simplicity, we shouldn't detect whether it's in SNI mode, and just use MII, and limit the speed to just 10Mbps?
On Thu, Aug 10, 2023 at 05:52:41PM +0100, Russell King (Oracle) wrote: > I wonder whether we have any implementation using SNI mode. I couldn't > find anything in the in-kernel dts files for this driver, the only > dts we have is one that was posted on-list recently, and that was using > MII at 100Mbps: > > https://lore.kernel.org/r/CABikg9zfGVEJsWf7eq=K5oKQozt86LLn-rzMaVmycekXkQEa8Q@mail.gmail.com > > No one would be able to specify "sni" in their dts, so maybe for the > sake of simplicity, we shouldn't detect whether it's in SNI mode, and > just use MII, and limit the speed to just 10Mbps? Based on the fact that "marvell,mv88e6060" is in dsa_switches_apply_workarounds[], it is technically possible that there exist boards which use the SNI mode but have no phy-mode and other phylink properties on the CPU port, and thus they work fine while skipping phylink. Of course, "possible" != "real". What I would like is to not discourage the board user to set phy-mode = "sni" in the device tree if that's what PortMode in the Port Status Register says that the port is strapped for. I'm afraid that by intentionally ignoring that bit and putting PHY_INTERFACE_MODE_REVMII in supported_interfaces, we're kind of suggesting to that person that this is what is correct, as that is the only thing that would work without modifying the kernel? Maybe if we don't want to introduce PHY_INTERFACE_MODE_SNI for fear of a lack of real users, we could at least detect PortMode=0, and not populate supported_interfaces, leading to an intentional validation failure and a comment above that check, stating that phy-mode = "sni" is not yet implemented?
On Thu, Aug 10, 2023 at 08:11:00PM +0300, Vladimir Oltean wrote: > On Thu, Aug 10, 2023 at 05:52:41PM +0100, Russell King (Oracle) wrote: > > I wonder whether we have any implementation using SNI mode. I couldn't > > find anything in the in-kernel dts files for this driver, the only > > dts we have is one that was posted on-list recently, and that was using > > MII at 100Mbps: > > > > https://lore.kernel.org/r/CABikg9zfGVEJsWf7eq=K5oKQozt86LLn-rzMaVmycekXkQEa8Q@mail.gmail.com > > > > No one would be able to specify "sni" in their dts, so maybe for the > > sake of simplicity, we shouldn't detect whether it's in SNI mode, and > > just use MII, and limit the speed to just 10Mbps? > > Based on the fact that "marvell,mv88e6060" is in > dsa_switches_apply_workarounds[], it is technically possible that there > exist boards which use the SNI mode but have no phy-mode and other > phylink properties on the CPU port, and thus they work fine while > skipping phylink. Of course, "possible" != "real". What I meant is that there are no in-tree users of the Marvell 88E6060 DSA driver. It looks like it was contributed in 2008. Whether it had users between the date that it was contributed and today I don't know. All that I can see is that the only users of it are out-of-tree users, which means we have the maintenance burden from the driver but no apparent platforms that make use of it, and no way to test it (other than if one of those out-of-tree users pops up, such as like last month.) I know that Arnd tends to strip out code that a platform uses when the platform is removed, was there a reason that this got left behind, assuming that it was used by a board? > Maybe if we don't want to introduce PHY_INTERFACE_MODE_SNI for fear of a > lack of real users, we could at least detect PortMode=0, and not > populate supported_interfaces, leading to an intentional validation > failure and a comment above that check, stating that phy-mode = "sni" is > not yet implemented? It would probably be better for mv88e6060_phylink_get_caps() to detect it and print the warning, leaving supported_interfaces empty - which will then cause phylink_create() to fail. Maybe that's what you meant, but I interpreted it as modifying the check in phylink_create().
On Wed, 9 Aug 2023 at 17:46, Russell King (Oracle) <rmk+kernel@armlinux.org.uk> wrote: > > Add a phylink_get_caps implementation for Marvell 88e6060 DSA switch. > This is a fast ethernet switch, with internal PHYs for ports 0 through > 4. Port 4 also supports MII, REVMII, REVRMII and SNI. Port 5 supports > MII, REVMII, REVRMII and SNI without an internal PHY. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > Sergei, > > Would it be possible for you to check that this patch works with your > setup please? I have tested it. First I reverted this commit: 9945c1fb03a3c9f7e0dcf9aa17041a70e551387a net: dsa: fix older DSA drivers using phylink So I could see the issue reproduce. Then I applied this patch and - yes, this patch resolves the issue. I haven't studied the code carefully, but because of testing: Tested-by: Sergei Antonov <saproj@gmail.com> Thanks!
On Thu, 10 Aug 2023 at 20:38, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Thu, Aug 10, 2023 at 08:11:00PM +0300, Vladimir Oltean wrote: > > On Thu, Aug 10, 2023 at 05:52:41PM +0100, Russell King (Oracle) wrote: > > > I wonder whether we have any implementation using SNI mode. I couldn't > > > find anything in the in-kernel dts files for this driver, the only > > > dts we have is one that was posted on-list recently, and that was using > > > MII at 100Mbps: > > > > > > https://lore.kernel.org/r/CABikg9zfGVEJsWf7eq=K5oKQozt86LLn-rzMaVmycekXkQEa8Q@mail.gmail.com > > > > > > No one would be able to specify "sni" in their dts, so maybe for the > > > sake of simplicity, we shouldn't detect whether it's in SNI mode, and > > > just use MII, and limit the speed to just 10Mbps? > > > > Based on the fact that "marvell,mv88e6060" is in > > dsa_switches_apply_workarounds[], it is technically possible that there > > exist boards which use the SNI mode but have no phy-mode and other > > phylink properties on the CPU port, and thus they work fine while > > skipping phylink. Of course, "possible" != "real". > > What I meant is that there are no in-tree users of the Marvell 88E6060 > DSA driver. It looks like it was contributed in 2008. Whether it had > users between the date that it was contributed and today I don't know. > > All that I can see is that the only users of it are out-of-tree users, > which means we have the maintenance burden from the driver but no > apparent platforms that make use of it, and no way to test it (other > than if one of those out-of-tree users pops up, such as like last > month.) I am planning to submit a platform using "marvell,mv88e6060". For the next release cycle hopefully. Our should I rather try to move MV88E6060 support to /mv88e6xxx?
On Thu, Aug 10, 2023 at 06:38:29PM +0100, Russell King (Oracle) wrote: > What I meant is that there are no in-tree users of the Marvell 88E6060 > DSA driver. It looks like it was contributed in 2008. Whether it had > users between the date that it was contributed and today I don't know. > > All that I can see is that the only users of it are out-of-tree users, > which means we have the maintenance burden from the driver but no > apparent platforms that make use of it, and no way to test it (other > than if one of those out-of-tree users pops up, such as like last > month.) > > I know that Arnd tends to strip out code that a platform uses when the > platform is removed, was there a reason that this got left behind, > assuming that it was used by a board? I also have reasons to believe that this driver is seeing very little use, based on the number of reports relative to the severity and age of the bugs/performance limitations found. But, since it hasn't really bothered the general DSA maintenance all that much up until now, we just kept it, not knowing what state it is in. If you're saying that's starting to change, I suppose we can start issuing ultimatums, and make a list of what's being blocked because of the lack of activity. But as long as Sergei is responding, maybe the mv88e6060 can live another day. > > Maybe if we don't want to introduce PHY_INTERFACE_MODE_SNI for fear of a > > lack of real users, we could at least detect PortMode=0, and not > > populate supported_interfaces, leading to an intentional validation > > failure and a comment above that check, stating that phy-mode = "sni" is > > not yet implemented? > > It would probably be better for mv88e6060_phylink_get_caps() to detect > it and print the warning, leaving supported_interfaces empty - which > will then cause phylink_create() to fail. Maybe that's what you meant, > but I interpreted it as modifying the check in phylink_create(). Yes, this is what I meant. I didn't say anything about phylink_create(). diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c index 0e776be5e941..23cc6b01a1c4 100644 --- a/drivers/net/dsa/mv88e6060.c +++ b/drivers/net/dsa/mv88e6060.c @@ -263,15 +263,12 @@ static void mv88e6060_phylink_get_caps(struct dsa_switch *ds, int port, return; } - if (!(ret & PORT_STATUS_PORTMODE)) { - /* Port configured in SNI mode (acts as a 10Mbps PHY) */ - config->mac_capabilities = MAC_10 | MAC_SYM_PAUSE; - /* I don't think SNI is SMII - SMII has a sync signal, and - * SNI doesn't. - */ - __set_bit(PHY_INTERFACE_MODE_SMII, interfaces); + /* If the port is configured in SNI mode (acts as a 10Mbps PHY), + * it should have phy-mode = "sni", but that doesn't yet exist, so + * forcibly fail validation until the need arises to introduce it. + */ + if (!(ret & PORT_STATUS_PORTMODE)) return; - } config->mac_capabilities = MAC_100 | MAC_10 | MAC_SYM_PAUSE;
On Thu, Aug 10, 2023 at 09:17:33PM +0300, Sergei Antonov wrote: > I am planning to submit a platform using "marvell,mv88e6060". For the > next release cycle hopefully. > Our should I rather try to move MV88E6060 support to /mv88e6xxx? Device tree bindings and the driver implementation should be seen as separate things. You can submit a platform for mv88e6060 regardless of what driver it's using, and if the driver is merged into mv88e6xxx, that needs to support the existing platforms as well.
diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c index fdda62d6eb16..0e776be5e941 100644 --- a/drivers/net/dsa/mv88e6060.c +++ b/drivers/net/dsa/mv88e6060.c @@ -247,11 +247,57 @@ mv88e6060_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val) return reg_write(priv, addr, regnum, val); } +static void mv88e6060_phylink_get_caps(struct dsa_switch *ds, int port, + struct phylink_config *config) +{ + unsigned long *interfaces = config->supported_interfaces; + struct mv88e6060_priv *priv = ds->priv; + int addr = REG_PORT(port); + int ret; + + ret = reg_read(priv, addr, PORT_STATUS); + if (ret < 0) { + dev_err(ds->dev, + "port %d: unable to read status register: %pe\n", + port, ERR_PTR(ret)); + return; + } + + if (!(ret & PORT_STATUS_PORTMODE)) { + /* Port configured in SNI mode (acts as a 10Mbps PHY) */ + config->mac_capabilities = MAC_10 | MAC_SYM_PAUSE; + /* I don't think SNI is SMII - SMII has a sync signal, and + * SNI doesn't. + */ + __set_bit(PHY_INTERFACE_MODE_SMII, interfaces); + return; + } + + config->mac_capabilities = MAC_100 | MAC_10 | MAC_SYM_PAUSE; + + if (port >= 4) { + /* Ports 4 and 5 can support MII, REVMII and REVRMII modes */ + __set_bit(PHY_INTERFACE_MODE_MII, interfaces); + __set_bit(PHY_INTERFACE_MODE_REVMII, interfaces); + __set_bit(PHY_INTERFACE_MODE_REVRMII, interfaces); + } + if (port <= 4) { + /* Ports 0 to 3 have internal PHYs, and port 4 can optionally + * use an internal PHY. + */ + /* Internal PHY */ + __set_bit(PHY_INTERFACE_MODE_INTERNAL, interfaces); + /* Default phylib interface mode */ + __set_bit(PHY_INTERFACE_MODE_GMII, interfaces); + } +} + static const struct dsa_switch_ops mv88e6060_switch_ops = { .get_tag_protocol = mv88e6060_get_tag_protocol, .setup = mv88e6060_setup, .phy_read = mv88e6060_phy_read, .phy_write = mv88e6060_phy_write, + .phylink_get_caps = mv88e6060_phylink_get_caps, }; static int mv88e6060_probe(struct mdio_device *mdiodev)