| Message ID | 20240416155054.3650354-1-matthias.schiffer@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: mv88e6xx: fix supported_interfaces setup in mv88e6250_phylink_get_caps() | expand |
On Tue, Apr 16, 2024 at 05:50:54PM +0200, Matthias Schiffer wrote: > +int mv88e6250_port_get_mode(struct mv88e6xxx_chip *chip, int port, > + phy_interface_t *mode) > +{ > + int err; > + u16 reg; > + > + if (port < 5) { > + *mode = PHY_INTERFACE_MODE_INTERNAL; > + return 0; > + } Note that if mv88e6xxx_phy_is_internal() returns TRUE for the port, then this will be handled automatically. > + > + err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, ®); > + if (err) > + return err; > + > + switch (reg & MV88E6250_PORT_STS_PORTMODE_MASK) { > + case MV88E6250_PORT_STS_PORTMODE_MII_10_HALF_PHY: > + case MV88E6250_PORT_STS_PORTMODE_MII_100_HALF_PHY: > + case MV88E6250_PORT_STS_PORTMODE_MII_10_FULL_PHY: > + case MV88E6250_PORT_STS_PORTMODE_MII_100_FULL_PHY: > + *mode = PHY_INTERFACE_MODE_REVMII; > + break; > + > + case MV88E6250_PORT_STS_PORTMODE_MII_HALF: > + case MV88E6250_PORT_STS_PORTMODE_MII_FULL: > + *mode = PHY_INTERFACE_MODE_MII; > + break; > + > + case MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL_PHY: > + case MV88E6250_PORT_STS_PORTMODE_MII_200_RMII_FULL_PHY: > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_HALF_PHY: > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL_PHY: > + *mode = PHY_INTERFACE_MODE_REVRMII; > + break; > + > + case MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL: > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL: > + *mode = PHY_INTERFACE_MODE_RMII; > + break; > + > + case MV88E6250_PORT_STS_PORTMODE_MII_100_RGMII: > + *mode = PHY_INTERFACE_MODE_RGMII; > + break; > + > + default: > + *mode = PHY_INTERFACE_MODE_NA; What does this mean? I don't allow PHY_INTERFACE_MODE_NA to be set in the list of supported interfaces because it isn't an interface mode. If it's invalid, then it's probably best to return an error. I wonder whether it would just be better to pass the supported_interfaces bitmap into this function and have it set the appropriate bit itself, renaming the function to something more better suited to that purpose. Thanks.
On Tue, 2024-04-16 at 17:56 +0100, Russell King (Oracle) wrote: > On Tue, Apr 16, 2024 at 05:50:54PM +0200, Matthias Schiffer wrote: > > +int mv88e6250_port_get_mode(struct mv88e6xxx_chip *chip, int port, > > + phy_interface_t *mode) > > +{ > > + int err; > > + u16 reg; > > + > > + if (port < 5) { > > + *mode = PHY_INTERFACE_MODE_INTERNAL; > > + return 0; > > + } > > Note that if mv88e6xxx_phy_is_internal() returns TRUE for the port, > then this will be handled automatically. > > > + > > + err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, ®); > > + if (err) > > + return err; > > + > > + switch (reg & MV88E6250_PORT_STS_PORTMODE_MASK) { > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_HALF_PHY: > > + case MV88E6250_PORT_STS_PORTMODE_MII_100_HALF_PHY: > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_FULL_PHY: > > + case MV88E6250_PORT_STS_PORTMODE_MII_100_FULL_PHY: > > + *mode = PHY_INTERFACE_MODE_REVMII; > > + break; > > + > > + case MV88E6250_PORT_STS_PORTMODE_MII_HALF: > > + case MV88E6250_PORT_STS_PORTMODE_MII_FULL: > > + *mode = PHY_INTERFACE_MODE_MII; > > + break; > > + > > + case MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL_PHY: > > + case MV88E6250_PORT_STS_PORTMODE_MII_200_RMII_FULL_PHY: > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_HALF_PHY: > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL_PHY: > > + *mode = PHY_INTERFACE_MODE_REVRMII; > > + break; > > + > > + case MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL: > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL: > > + *mode = PHY_INTERFACE_MODE_RMII; > > + break; > > + > > + case MV88E6250_PORT_STS_PORTMODE_MII_100_RGMII: > > + *mode = PHY_INTERFACE_MODE_RGMII; > > + break; > > + > > + default: > > + *mode = PHY_INTERFACE_MODE_NA; > > What does this mean? I don't allow PHY_INTERFACE_MODE_NA to be set in > the list of supported interfaces because it isn't an interface mode. > If it's invalid, then it's probably best to return an error. > > I wonder whether it would just be better to pass the > supported_interfaces bitmap into this function and have it set the > appropriate bit itself, renaming the function to something more > better suited to that purpose. > > Thanks. I'm explicitly checking for PHY_INTERFACE_MODE_NA in the caller to handle the "this interface isn't useable" case. Passing supported_interfaces into the function handling the port modes is fine with me, too - will send a v2 later. Best regards, Matthias >
On Wed, 2024-04-17 at 09:13 +0200, Matthias Schiffer wrote: > On Tue, 2024-04-16 at 17:56 +0100, Russell King (Oracle) wrote: > > On Tue, Apr 16, 2024 at 05:50:54PM +0200, Matthias Schiffer wrote: > > > +int mv88e6250_port_get_mode(struct mv88e6xxx_chip *chip, int port, > > > + phy_interface_t *mode) > > > +{ > > > + int err; > > > + u16 reg; > > > + > > > + if (port < 5) { > > > + *mode = PHY_INTERFACE_MODE_INTERNAL; > > > + return 0; > > > + } > > > > Note that if mv88e6xxx_phy_is_internal() returns TRUE for the port, > > then this will be handled automatically. > > > > > + > > > + err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, ®); > > > + if (err) > > > + return err; > > > + > > > + switch (reg & MV88E6250_PORT_STS_PORTMODE_MASK) { > > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_HALF_PHY: > > > + case MV88E6250_PORT_STS_PORTMODE_MII_100_HALF_PHY: > > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_FULL_PHY: > > > + case MV88E6250_PORT_STS_PORTMODE_MII_100_FULL_PHY: > > > + *mode = PHY_INTERFACE_MODE_REVMII; > > > + break; > > > + > > > + case MV88E6250_PORT_STS_PORTMODE_MII_HALF: > > > + case MV88E6250_PORT_STS_PORTMODE_MII_FULL: > > > + *mode = PHY_INTERFACE_MODE_MII; > > > + break; > > > + > > > + case MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL_PHY: > > > + case MV88E6250_PORT_STS_PORTMODE_MII_200_RMII_FULL_PHY: > > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_HALF_PHY: > > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL_PHY: > > > + *mode = PHY_INTERFACE_MODE_REVRMII; > > > + break; > > > + > > > + case MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL: > > > + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL: > > > + *mode = PHY_INTERFACE_MODE_RMII; > > > + break; > > > + > > > + case MV88E6250_PORT_STS_PORTMODE_MII_100_RGMII: > > > + *mode = PHY_INTERFACE_MODE_RGMII; > > > + break; > > > + > > > + default: > > > + *mode = PHY_INTERFACE_MODE_NA; > > > > What does this mean? I don't allow PHY_INTERFACE_MODE_NA to be set in > > the list of supported interfaces because it isn't an interface mode. > > If it's invalid, then it's probably best to return an error. > > > > I wonder whether it would just be better to pass the > > supported_interfaces bitmap into this function and have it set the > > appropriate bit itself, renaming the function to something more > > better suited to that purpose. > > > > Thanks. > > I'm explicitly checking for PHY_INTERFACE_MODE_NA in the caller to handle the "this interface isn't > useable" case. Passing supported_interfaces into the function handling the port modes is fine with > me, too - will send a v2 later. > > Best regards, > Matthias ... and I realize I don't even check for PHY_INTERFACE_MODE_NA in the caller in the version of the patch I submitted. Oh well, time for v2. > > > > >
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index c95787cb90867..76040b63a5c33 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -570,9 +570,16 @@ static void mv88e6250_phylink_get_caps(struct mv88e6xxx_chip *chip, int port, struct phylink_config *config) { unsigned long *supported = config->supported_interfaces; + phy_interface_t mode; + int err; - /* Translate the default cmode */ - mv88e6xxx_translate_cmode(chip->ports[port].cmode, supported); + err = mv88e6250_port_get_mode(chip, port, &mode); + if (err) { + dev_err(chip->dev, "p%d: failed to get port mode\n", port); + return; + } + + __set_bit(mode, supported); config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100; } diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index 5394a8cf7bf1d..d58060e94250e 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -740,6 +740,57 @@ int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode) return 0; } +int mv88e6250_port_get_mode(struct mv88e6xxx_chip *chip, int port, + phy_interface_t *mode) +{ + int err; + u16 reg; + + if (port < 5) { + *mode = PHY_INTERFACE_MODE_INTERNAL; + return 0; + } + + err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, ®); + if (err) + return err; + + switch (reg & MV88E6250_PORT_STS_PORTMODE_MASK) { + case MV88E6250_PORT_STS_PORTMODE_MII_10_HALF_PHY: + case MV88E6250_PORT_STS_PORTMODE_MII_100_HALF_PHY: + case MV88E6250_PORT_STS_PORTMODE_MII_10_FULL_PHY: + case MV88E6250_PORT_STS_PORTMODE_MII_100_FULL_PHY: + *mode = PHY_INTERFACE_MODE_REVMII; + break; + + case MV88E6250_PORT_STS_PORTMODE_MII_HALF: + case MV88E6250_PORT_STS_PORTMODE_MII_FULL: + *mode = PHY_INTERFACE_MODE_MII; + break; + + case MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL_PHY: + case MV88E6250_PORT_STS_PORTMODE_MII_200_RMII_FULL_PHY: + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_HALF_PHY: + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL_PHY: + *mode = PHY_INTERFACE_MODE_REVRMII; + break; + + case MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL: + case MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL: + *mode = PHY_INTERFACE_MODE_RMII; + break; + + case MV88E6250_PORT_STS_PORTMODE_MII_100_RGMII: + *mode = PHY_INTERFACE_MODE_RGMII; + break; + + default: + *mode = PHY_INTERFACE_MODE_NA; + } + + return 0; +} + /* Offset 0x02: Jamming Control * * Do not limit the period of time that this port can be paused for by diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h index 86deeb347cbc1..43d69c8f74a8b 100644 --- a/drivers/net/dsa/mv88e6xxx/port.h +++ b/drivers/net/dsa/mv88e6xxx/port.h @@ -25,10 +25,25 @@ #define MV88E6250_PORT_STS_PORTMODE_PHY_100_HALF 0x0900 #define MV88E6250_PORT_STS_PORTMODE_PHY_10_FULL 0x0a00 #define MV88E6250_PORT_STS_PORTMODE_PHY_100_FULL 0x0b00 -#define MV88E6250_PORT_STS_PORTMODE_MII_10_HALF 0x0c00 -#define MV88E6250_PORT_STS_PORTMODE_MII_100_HALF 0x0d00 -#define MV88E6250_PORT_STS_PORTMODE_MII_10_FULL 0x0e00 -#define MV88E6250_PORT_STS_PORTMODE_MII_100_FULL 0x0f00 +/* - Modes with PHY suffix use output instead of input clock + * - Modes without RMII or RGMII use MII + * - Modes without speed do not have a fixed speed specified in the manual + * ("DC to x MHz" - variable clock support?) + */ +#define MV88E6250_PORT_STS_PORTMODE_MII_DISABLED 0x0000 +#define MV88E6250_PORT_STS_PORTMODE_MII_100_RGMII 0x0100 +#define MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL_PHY 0x0200 +#define MV88E6250_PORT_STS_PORTMODE_MII_200_RMII_FULL_PHY 0x0400 +#define MV88E6250_PORT_STS_PORTMODE_MII_DUAL_100_RMII_FULL 0x0600 +#define MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL 0x0700 +#define MV88E6250_PORT_STS_PORTMODE_MII_HALF 0x0800 +#define MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_HALF_PHY 0x0900 +#define MV88E6250_PORT_STS_PORTMODE_MII_FULL 0x0a00 +#define MV88E6250_PORT_STS_PORTMODE_MII_10_100_RMII_FULL_PHY 0x0b00 +#define MV88E6250_PORT_STS_PORTMODE_MII_10_HALF_PHY 0x0c00 +#define MV88E6250_PORT_STS_PORTMODE_MII_100_HALF_PHY 0x0d00 +#define MV88E6250_PORT_STS_PORTMODE_MII_10_FULL_PHY 0x0e00 +#define MV88E6250_PORT_STS_PORTMODE_MII_100_FULL_PHY 0x0f00 #define MV88E6XXX_PORT_STS_LINK 0x0800 #define MV88E6XXX_PORT_STS_DUPLEX 0x0400 #define MV88E6XXX_PORT_STS_SPEED_MASK 0x0300 @@ -442,6 +457,8 @@ int mv88e6393x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, phy_interface_t mode); int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); +int mv88e6250_port_get_mode(struct mv88e6xxx_chip *chip, int port, + phy_interface_t *mode); int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, bool drop_untagged); int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map);
With the recent PHYLINK changes requiring supported_interfaces to be set, MV88E6250 family switches like the 88E6020 fail to probe - cmode is never initialized on these devices, so mv88e6250_phylink_get_caps() does not set any supported_interfaces flags. Instead of a cmode, on 88E6250 we have a read-only port mode value that encodes similar information. There is no reason to bother mapping port mode to the cmodes of other switch models; instead we introduce a mv88e6250_port_get_mode() that is called directly from mv88e6250_phylink_get_caps(). Fixes: de5c9bf40c45 ("net: phylink: require supported_interfaces to be filled") Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 11 +++++-- drivers/net/dsa/mv88e6xxx/port.c | 51 ++++++++++++++++++++++++++++++++ drivers/net/dsa/mv88e6xxx/port.h | 25 +++++++++++++--- 3 files changed, 81 insertions(+), 6 deletions(-)