| Message ID | 20221019085052.933385-3-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell10g: Add host speed setting by an ethernet driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Wed, 19 Oct 2022 17:50:51 +0900 Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > Add support for selecting host speed mode. For now, only support > 1000M bps. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c > index 383a9c9f36e5..daf3242c6078 100644 > --- a/drivers/net/phy/marvell10g.c > +++ b/drivers/net/phy/marvell10g.c > @@ -101,6 +101,10 @@ enum { > MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS = BIT(13), > MV_AN_21X0_SERDES_CTRL2_RUN_INIT = BIT(15), > > + MV_MOD_CONF = 0xf000, > + MV_MOD_CONF_SPEED_MASK = 0x00c0, > + MV_MOD_CONF_SPEED_1000 = BIT(7), > + Where did you get these values from? My documentation says: Mode Configuration Device 31, Register 0xF000 Bits 7:6 Reserved R/W 0x3 This must always be 11. > /* These registers appear at 0x800X and 0xa00X - the 0xa00X control > * registers appear to set themselves to the 0x800X when AN is > * restarted, but status registers appear readable from either. > @@ -147,6 +151,7 @@ struct mv3310_chip { > int (*get_mactype)(struct phy_device *phydev); > int (*set_mactype)(struct phy_device *phydev, int mactype); > int (*select_mactype)(unsigned long *interfaces); > + int (*set_macspeed)(struct phy_device *phydev, int macspeed); > int (*init_interface)(struct phy_device *phydev, int mactype); > > #ifdef CONFIG_HWMON > @@ -644,6 +649,16 @@ static int mv2110_select_mactype(unsigned long *interfaces) > return -1; > } > > +static int mv2110_set_macspeed(struct phy_device *phydev, int macspeed) > +{ > + if (macspeed != SPEED_1000) > + return -EOPNOTSUPP; > + > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_MOD_CONF, > + MV_MOD_CONF_SPEED_MASK, > + MV_MOD_CONF_SPEED_1000); > +} Why not also support other speeds, if we are doing this already? Marek
On Wed, 19 Oct 2022 12:48:39 +0200 Marek Behún <kabel@kernel.org> wrote: > On Wed, 19 Oct 2022 17:50:51 +0900 > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > > Add support for selecting host speed mode. For now, only support > > 1000M bps. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c > > index 383a9c9f36e5..daf3242c6078 100644 > > --- a/drivers/net/phy/marvell10g.c > > +++ b/drivers/net/phy/marvell10g.c > > @@ -101,6 +101,10 @@ enum { > > MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS = BIT(13), > > MV_AN_21X0_SERDES_CTRL2_RUN_INIT = BIT(15), > > > > + MV_MOD_CONF = 0xf000, > > + MV_MOD_CONF_SPEED_MASK = 0x00c0, > > + MV_MOD_CONF_SPEED_1000 = BIT(7), > > + > > Where did you get these values from? My documentation says: > Mode Configuration > Device 31, Register 0xF000 > Bits > 7:6 Reserved R/W 0x3 This must always be 11. Ah, I see. Probably from the MTD API sources... But the bits should be set to 0x3 after HW reset, which means 10 Gbps, and this should not interfere with 1000 Mbps SGMII operation. Do you really need to set this? Isn't setting the MACTYPE to SGMII sufficient? Marek
On Wed, Oct 19, 2022 at 12:48:39PM +0200, Marek Behún wrote: > On Wed, 19 Oct 2022 17:50:51 +0900 > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > > Add support for selecting host speed mode. For now, only support > > 1000M bps. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c > > index 383a9c9f36e5..daf3242c6078 100644 > > --- a/drivers/net/phy/marvell10g.c > > +++ b/drivers/net/phy/marvell10g.c > > @@ -101,6 +101,10 @@ enum { > > MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS = BIT(13), > > MV_AN_21X0_SERDES_CTRL2_RUN_INIT = BIT(15), > > > > + MV_MOD_CONF = 0xf000, > > + MV_MOD_CONF_SPEED_MASK = 0x00c0, > > + MV_MOD_CONF_SPEED_1000 = BIT(7), > > + > > Where did you get these values from? My documentation says: > Mode Configuration > Device 31, Register 0xF000 > Bits > 7:6 Reserved R/W 0x3 This must always be 11. The closest is from the 88x3310 documentation that indicates these are the default speed, which are used when the media side is down. There is a specific sequence to update these. However, as we seem to be talking about the 2110 here, that should be reflected in these definitions. Finally, using BIT() for definitions of a field which can be one of four possible values is not acceptable. BIT() is for single bits not for a multi-bit field which can take any possible value but just the value we're representing there just happens to have a single bit set.
Hi Marek, > From: Marek Behún, Sent: Wednesday, October 19, 2022 7:49 PM > > On Wed, 19 Oct 2022 17:50:51 +0900 > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > > Add support for selecting host speed mode. For now, only support > > 1000M bps. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c > > index 383a9c9f36e5..daf3242c6078 100644 > > --- a/drivers/net/phy/marvell10g.c > > +++ b/drivers/net/phy/marvell10g.c > > @@ -101,6 +101,10 @@ enum { > > MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS = BIT(13), > > MV_AN_21X0_SERDES_CTRL2_RUN_INIT = BIT(15), > > > > + MV_MOD_CONF = 0xf000, > > + MV_MOD_CONF_SPEED_MASK = 0x00c0, > > + MV_MOD_CONF_SPEED_1000 = BIT(7), > > + > > Where did you get these values from? My documentation says: > Mode Configuration > Device 31, Register 0xF000 > Bits > 7:6 Reserved R/W 0x3 This must always be 11. Hmm, that's true. The register description said that. But, "loopback control" in the functional description said "default MAC interface Speed". (I don't use loopback mode though...) > > /* These registers appear at 0x800X and 0xa00X - the 0xa00X control > > * registers appear to set themselves to the 0x800X when AN is > > * restarted, but status registers appear readable from either. > > @@ -147,6 +151,7 @@ struct mv3310_chip { > > int (*get_mactype)(struct phy_device *phydev); > > int (*set_mactype)(struct phy_device *phydev, int mactype); > > int (*select_mactype)(unsigned long *interfaces); > > + int (*set_macspeed)(struct phy_device *phydev, int macspeed); > > int (*init_interface)(struct phy_device *phydev, int mactype); > > > > #ifdef CONFIG_HWMON > > @@ -644,6 +649,16 @@ static int mv2110_select_mactype(unsigned long *interfaces) > > return -1; > > } > > > > +static int mv2110_set_macspeed(struct phy_device *phydev, int macspeed) > > +{ > > + if (macspeed != SPEED_1000) > > + return -EOPNOTSUPP; > > + > > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_MOD_CONF, > > + MV_MOD_CONF_SPEED_MASK, > > + MV_MOD_CONF_SPEED_1000); > > +} > > Why not also support other speeds, if we are doing this already? The register seems to support other speeds: 10, 100 Mbps and "speed controlled by other register". I'll update this function if acceptable... # I'm thinking this is not acceptable because the register description said "Reserved" # as you mentioned. Best regards, Yoshihiro Shimoda
Hi Russell, > From: Russell King, Sent: Wednesday, October 19, 2022 8:59 PM > > On Wed, Oct 19, 2022 at 12:48:39PM +0200, Marek Behún wrote: > > On Wed, 19 Oct 2022 17:50:51 +0900 > > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > > > > Add support for selecting host speed mode. For now, only support > > > 1000M bps. > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > --- > > > drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c > > > index 383a9c9f36e5..daf3242c6078 100644 > > > --- a/drivers/net/phy/marvell10g.c > > > +++ b/drivers/net/phy/marvell10g.c > > > @@ -101,6 +101,10 @@ enum { > > > MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS = BIT(13), > > > MV_AN_21X0_SERDES_CTRL2_RUN_INIT = BIT(15), > > > > > > + MV_MOD_CONF = 0xf000, > > > + MV_MOD_CONF_SPEED_MASK = 0x00c0, > > > + MV_MOD_CONF_SPEED_1000 = BIT(7), > > > + > > > > Where did you get these values from? My documentation says: > > Mode Configuration > > Device 31, Register 0xF000 > > Bits > > 7:6 Reserved R/W 0x3 This must always be 11. > > The closest is from the 88x3310 documentation that indicates these are > the default speed, which are used when the media side is down. There > is a specific sequence to update these. > > However, as we seem to be talking about the 2110 here, that should be > reflected in these definitions. Yes, this is about 2110. So, I'll find other way for my environment somehow. > Finally, using BIT() for definitions of a field which can be one of > four possible values is not acceptable. BIT() is for single bits > not for a multi-bit field which can take any possible value but just > the value we're representing there just happens to have a single bit > set. I understood it. Thanks! Best regards, Yoshihiro Shimoda > -- > RMK's Patch system:
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 383a9c9f36e5..daf3242c6078 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -101,6 +101,10 @@ enum { MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS = BIT(13), MV_AN_21X0_SERDES_CTRL2_RUN_INIT = BIT(15), + MV_MOD_CONF = 0xf000, + MV_MOD_CONF_SPEED_MASK = 0x00c0, + MV_MOD_CONF_SPEED_1000 = BIT(7), + /* These registers appear at 0x800X and 0xa00X - the 0xa00X control * registers appear to set themselves to the 0x800X when AN is * restarted, but status registers appear readable from either. @@ -147,6 +151,7 @@ struct mv3310_chip { int (*get_mactype)(struct phy_device *phydev); int (*set_mactype)(struct phy_device *phydev, int mactype); int (*select_mactype)(unsigned long *interfaces); + int (*set_macspeed)(struct phy_device *phydev, int macspeed); int (*init_interface)(struct phy_device *phydev, int mactype); #ifdef CONFIG_HWMON @@ -644,6 +649,16 @@ static int mv2110_select_mactype(unsigned long *interfaces) return -1; } +static int mv2110_set_macspeed(struct phy_device *phydev, int macspeed) +{ + if (macspeed != SPEED_1000) + return -EOPNOTSUPP; + + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_MOD_CONF, + MV_MOD_CONF_SPEED_MASK, + MV_MOD_CONF_SPEED_1000); +} + static int mv3310_get_mactype(struct phy_device *phydev) { int mactype; @@ -778,6 +793,13 @@ static int mv3310_config_init(struct phy_device *phydev) if (err) return err; + /* If host provided host mac speed, try to set the mac speed */ + if (phydev->host_speed && chip->set_macspeed) { + err = chip->set_macspeed(phydev, phydev->host_speed); + if (err) + return err; + } + /* If host provided host supported interface modes, try to select the * best one */ @@ -1181,6 +1203,7 @@ static const struct mv3310_chip mv2110_type = { .get_mactype = mv2110_get_mactype, .set_mactype = mv2110_set_mactype, .select_mactype = mv2110_select_mactype, + .set_macspeed = mv2110_set_macspeed, .init_interface = mv2110_init_interface, #ifdef CONFIG_HWMON
Add support for selecting host speed mode. For now, only support 1000M bps. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)