[v3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers

[PATCH net-next V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers

Posted by Raju Lakkaraju 2 years, 1 month ago

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(+)

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
-- 
2.34.1

Re: [PATCH net-next V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers

Posted by patchwork-bot+netdevbpf@kernel.org 2 years, 1 month ago

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers

Posted by Serge Semin 2 years, 1 month ago

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
>

Re: [PATCH net-next V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers

Posted by Russell King (Oracle) 2 years, 1 month ago

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers

Posted by Serge Semin 2 years, 1 month ago

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!