| Message ID | 20231027044306.291250-1-Raju.Lakkaraju@microchip.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f1c73396133cb3d913e2075298005644ee8dfade |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers | expand |
Cc += Russell * It's a good practice to add all the reviewers to Cc in the new patch * revisions. On Fri, Oct 27, 2023 at 10:13:06AM +0530, Raju Lakkaraju wrote: > Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> With a nitpick below clarified, feel free to add: Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > --- > drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++++++ > drivers/net/pcs/pcs-xpcs.h | 2 ++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > index 4dbc21f604f2..31f0beba638a 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -1090,6 +1090,28 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs, > return 0; > } > > +static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs, > + struct phylink_link_state *state) > +{ > + int ret; > + > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS); > + if (ret < 0) { > + state->link = 0; > + return ret; > + } > + > + state->link = !!(ret & DW_VR_MII_MMD_STS_LINK_STS); > + if (!state->link) > + return 0; > + > + state->speed = SPEED_2500; > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; Why is it '|=' instead of just '='? Is it possible to have the 'pause' field having some additional flags set which would be required to preserve? -Serge(y) > + state->duplex = DUPLEX_FULL; > + > + return 0; > +} > + > static void xpcs_get_state(struct phylink_pcs *pcs, > struct phylink_link_state *state) > { > @@ -1127,6 +1149,13 @@ static void xpcs_get_state(struct phylink_pcs *pcs, > ERR_PTR(ret)); > } > break; > + case DW_2500BASEX: > + ret = xpcs_get_state_2500basex(xpcs, state); > + if (ret) { > + pr_err("xpcs_get_state_2500basex returned %pe\n", > + ERR_PTR(ret)); > + } > + break; > default: > return; > } > diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h > index 39a90417e535..96c36b32ca99 100644 > --- a/drivers/net/pcs/pcs-xpcs.h > +++ b/drivers/net/pcs/pcs-xpcs.h > @@ -55,6 +55,8 @@ > /* Clause 37 Defines */ > /* VR MII MMD registers offsets */ > #define DW_VR_MII_MMD_CTRL 0x0000 > +#define DW_VR_MII_MMD_STS 0x0001 > +#define DW_VR_MII_MMD_STS_LINK_STS BIT(2) > #define DW_VR_MII_DIG_CTRL1 0x8000 > #define DW_VR_MII_AN_CTRL 0x8001 > #define DW_VR_MII_AN_INTR_STS 0x8002 > -- > 2.34.1 >
On Fri, Oct 27, 2023 at 02:04:15PM +0300, Serge Semin wrote: > Cc += Russell > > * It's a good practice to add all the reviewers to Cc in the new patch > * revisions. > > On Fri, Oct 27, 2023 at 10:13:06AM +0530, Raju Lakkaraju wrote: > > Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause > > > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> > > With a nitpick below clarified, feel free to add: > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > --- > > drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++++++ > > drivers/net/pcs/pcs-xpcs.h | 2 ++ > > 2 files changed, 31 insertions(+) > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > index 4dbc21f604f2..31f0beba638a 100644 > > --- a/drivers/net/pcs/pcs-xpcs.c > > +++ b/drivers/net/pcs/pcs-xpcs.c > > @@ -1090,6 +1090,28 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs, > > return 0; > > } > > > > +static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs, > > + struct phylink_link_state *state) > > +{ > > + int ret; > > + > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS); > > + if (ret < 0) { > > + state->link = 0; > > + return ret; > > + } > > + > > + state->link = !!(ret & DW_VR_MII_MMD_STS_LINK_STS); > > + if (!state->link) > > + return 0; > > + > > + state->speed = SPEED_2500; > > > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; > > Why is it '|=' instead of just '='? Is it possible to have the 'pause' > field having some additional flags set which would be required to > preserve? The code is correct. There are other flags on state->pause other than these, and phylink initialises state->pause prior to calling the function. The only flags that should be modified here are these two bits that the code is setting. Phylink will initialise it to MLO_PAUSE_NONE if expecting autoneg, or the configured values if autoneg on the link is disabled.
Hi Russell On Fri, Oct 27, 2023 at 12:54:36PM +0100, Russell King (Oracle) wrote: > On Fri, Oct 27, 2023 at 02:04:15PM +0300, Serge Semin wrote: > > Cc += Russell > > > > * It's a good practice to add all the reviewers to Cc in the new patch > > * revisions. > > > > On Fri, Oct 27, 2023 at 10:13:06AM +0530, Raju Lakkaraju wrote: > > > Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause > > > > > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> > > > > With a nitpick below clarified, feel free to add: > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > --- > > > drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++++++ > > > drivers/net/pcs/pcs-xpcs.h | 2 ++ > > > 2 files changed, 31 insertions(+) > > > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > > index 4dbc21f604f2..31f0beba638a 100644 > > > --- a/drivers/net/pcs/pcs-xpcs.c > > > +++ b/drivers/net/pcs/pcs-xpcs.c > > > @@ -1090,6 +1090,28 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs, > > > return 0; > > > } > > > > > > +static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs, > > > + struct phylink_link_state *state) > > > +{ > > > + int ret; > > > + > > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS); > > > + if (ret < 0) { > > > + state->link = 0; > > > + return ret; > > > + } > > > + > > > + state->link = !!(ret & DW_VR_MII_MMD_STS_LINK_STS); > > > + if (!state->link) > > > + return 0; > > > + > > > + state->speed = SPEED_2500; > > > > > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; > > > > Why is it '|=' instead of just '='? Is it possible to have the 'pause' > > field having some additional flags set which would be required to > > preserve? > > The code is correct. There are other flags on state->pause other than > these, and phylink initialises state->pause prior to calling the > function. The only flags that should be modified here are these two > bits that the code is setting. > > Phylink will initialise it to MLO_PAUSE_NONE if expecting autoneg, or > the configured values if autoneg on the link is disabled. Thanks for clarification. Then no more comments from my side in this patch regard. Regarding the XPCS driver in general. Based on what you said the rest of the XPCS state getters are wrong in fully re-writing the 'pause' field. Right? -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Oct 27, 2023 at 03:06:19PM +0300, Serge Semin wrote: > Hi Russell > > On Fri, Oct 27, 2023 at 12:54:36PM +0100, Russell King (Oracle) wrote: > > On Fri, Oct 27, 2023 at 02:04:15PM +0300, Serge Semin wrote: > > > Cc += Russell > > > > > > * It's a good practice to add all the reviewers to Cc in the new patch > > > * revisions. > > > > > > On Fri, Oct 27, 2023 at 10:13:06AM +0530, Raju Lakkaraju wrote: > > > > Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause > > > > > > > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> > > > > > > With a nitpick below clarified, feel free to add: > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > --- > > > > drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++++++ > > > > drivers/net/pcs/pcs-xpcs.h | 2 ++ > > > > 2 files changed, 31 insertions(+) > > > > > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > > > index 4dbc21f604f2..31f0beba638a 100644 > > > > --- a/drivers/net/pcs/pcs-xpcs.c > > > > +++ b/drivers/net/pcs/pcs-xpcs.c > > > > @@ -1090,6 +1090,28 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs, > > > > return 0; > > > > } > > > > > > > > +static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs, > > > > + struct phylink_link_state *state) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS); > > > > + if (ret < 0) { > > > > + state->link = 0; > > > > + return ret; > > > > + } > > > > + > > > > + state->link = !!(ret & DW_VR_MII_MMD_STS_LINK_STS); > > > > + if (!state->link) > > > > + return 0; > > > > + > > > > + state->speed = SPEED_2500; > > > > > > > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; > > > > > > Why is it '|=' instead of just '='? Is it possible to have the 'pause' > > > field having some additional flags set which would be required to > > > preserve? > > > > The code is correct. There are other flags on state->pause other than > > these, and phylink initialises state->pause prior to calling the > > function. The only flags that should be modified here are these two > > bits that the code is setting. > > > > Phylink will initialise it to MLO_PAUSE_NONE if expecting autoneg, or > > the configured values if autoneg on the link is disabled. > > Thanks for clarification. Then no more comments from my side in this > patch regard. > > Regarding the XPCS driver in general. Based on what you said the rest > of the XPCS state getters are wrong in fully re-writing the 'pause' > field. Right? Yes. xpcs_resolve_pma: state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; xpcs_get_state_c37_sgmii: state->pause = 0; are both incorrect. The former should be |=, the latter is totally unnecessary. Documentation: * pcs_get_state() - Read the current inband link state from the hardware * @pcs: a pointer to a &struct phylink_pcs. * @state: a pointer to a &struct phylink_link_state. * * Read the current inband link state from the MAC PCS, reporting the * current speed in @state->speed, duplex mode in @state->duplex, pause ^^^^^ * mode in @state->pause using the %MLO_PAUSE_RX and %MLO_PAUSE_TX bits, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I guess I need to make that more explicit that pcs_get_state() methods are only expected to _set_ these two bits as appropriate, leaving all other bits as-is.
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 27 Oct 2023 10:13:06 +0530 you wrote: > Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> > --- > drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++++++ > drivers/net/pcs/pcs-xpcs.h | 2 ++ > 2 files changed, 31 insertions(+) Here is the summary with links: - [net-next,V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers https://git.kernel.org/netdev/net-next/c/f1c73396133c You are awesome, thank you!
On Fri, Oct 27, 2023 at 01:40:34PM +0100, Russell King (Oracle) wrote: > On Fri, Oct 27, 2023 at 03:06:19PM +0300, Serge Semin wrote: > > Hi Russell > > > > On Fri, Oct 27, 2023 at 12:54:36PM +0100, Russell King (Oracle) wrote: > > > On Fri, Oct 27, 2023 at 02:04:15PM +0300, Serge Semin wrote: > > > > Cc += Russell > > > > > > > > * It's a good practice to add all the reviewers to Cc in the new patch > > > > * revisions. > > > > > > > > On Fri, Oct 27, 2023 at 10:13:06AM +0530, Raju Lakkaraju wrote: > > > > > Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause > > > > > > > > > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> > > > > > > > > With a nitpick below clarified, feel free to add: > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > > > --- > > > > > drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++++++ > > > > > drivers/net/pcs/pcs-xpcs.h | 2 ++ > > > > > 2 files changed, 31 insertions(+) > > > > > > > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > > > > index 4dbc21f604f2..31f0beba638a 100644 > > > > > --- a/drivers/net/pcs/pcs-xpcs.c > > > > > +++ b/drivers/net/pcs/pcs-xpcs.c > > > > > @@ -1090,6 +1090,28 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs, > > > > > return 0; > > > > > } > > > > > > > > > > +static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs, > > > > > + struct phylink_link_state *state) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS); > > > > > + if (ret < 0) { > > > > > + state->link = 0; > > > > > + return ret; > > > > > + } > > > > > + > > > > > + state->link = !!(ret & DW_VR_MII_MMD_STS_LINK_STS); > > > > > + if (!state->link) > > > > > + return 0; > > > > > + > > > > > + state->speed = SPEED_2500; > > > > > > > > > + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; > > > > > > > > Why is it '|=' instead of just '='? Is it possible to have the 'pause' > > > > field having some additional flags set which would be required to > > > > preserve? > > > > > > The code is correct. There are other flags on state->pause other than > > > these, and phylink initialises state->pause prior to calling the > > > function. The only flags that should be modified here are these two > > > bits that the code is setting. > > > > > > Phylink will initialise it to MLO_PAUSE_NONE if expecting autoneg, or > > > the configured values if autoneg on the link is disabled. > > > > Thanks for clarification. Then no more comments from my side in this > > patch regard. > > > > Regarding the XPCS driver in general. Based on what you said the rest > > of the XPCS state getters are wrong in fully re-writing the 'pause' > > field. Right? > > Yes. > > xpcs_resolve_pma: > state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; > > xpcs_get_state_c37_sgmii: > state->pause = 0; > > are both incorrect. The former should be |=, the latter is totally > unnecessary. > > Documentation: > * pcs_get_state() - Read the current inband link state from the hardware > * @pcs: a pointer to a &struct phylink_pcs. > * @state: a pointer to a &struct phylink_link_state. > * > * Read the current inband link state from the MAC PCS, reporting the > * current speed in @state->speed, duplex mode in @state->duplex, pause > ^^^^^ > * mode in @state->pause using the %MLO_PAUSE_RX and %MLO_PAUSE_TX bits, > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > I guess I need to make that more explicit that pcs_get_state() methods > are only expected to _set_ these two bits as appropriate, leaving all > other bits as-is. Thanks for the detailed response. I'll send fixup patches for the denoted problems as soon as I get some free time for it. Hopefully within a month if nobody does it sooner. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index 4dbc21f604f2..31f0beba638a 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -1090,6 +1090,28 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs, return 0; } +static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs, + struct phylink_link_state *state) +{ + int ret; + + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS); + if (ret < 0) { + state->link = 0; + return ret; + } + + state->link = !!(ret & DW_VR_MII_MMD_STS_LINK_STS); + if (!state->link) + return 0; + + state->speed = SPEED_2500; + state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX; + state->duplex = DUPLEX_FULL; + + return 0; +} + static void xpcs_get_state(struct phylink_pcs *pcs, struct phylink_link_state *state) { @@ -1127,6 +1149,13 @@ static void xpcs_get_state(struct phylink_pcs *pcs, ERR_PTR(ret)); } break; + case DW_2500BASEX: + ret = xpcs_get_state_2500basex(xpcs, state); + if (ret) { + pr_err("xpcs_get_state_2500basex returned %pe\n", + ERR_PTR(ret)); + } + break; default: return; } diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h index 39a90417e535..96c36b32ca99 100644 --- a/drivers/net/pcs/pcs-xpcs.h +++ b/drivers/net/pcs/pcs-xpcs.h @@ -55,6 +55,8 @@ /* Clause 37 Defines */ /* VR MII MMD registers offsets */ #define DW_VR_MII_MMD_CTRL 0x0000 +#define DW_VR_MII_MMD_STS 0x0001 +#define DW_VR_MII_MMD_STS_LINK_STS BIT(2) #define DW_VR_MII_DIG_CTRL1 0x8000 #define DW_VR_MII_AN_CTRL 0x8001 #define DW_VR_MII_AN_INTR_STS 0x8002
Add DW_2500BASEX case in xpcs_get_state( ) to update speed, duplex and pause Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> --- drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++++++ drivers/net/pcs/pcs-xpcs.h | 2 ++ 2 files changed, 31 insertions(+)