diff mbox series

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

Message ID 20231027044306.291250-1-Raju.Lakkaraju@microchip.com
State New
Headers
Series [net-next,V3] net: pcs: xpcs: Add 2500BASE-X case in get state for XPCS drivers |

Commit Message

Raju Lakkaraju Oct. 27, 2023, 4:43 a.m. UTC 
  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(+)

Comments

Serge Semin Oct. 27, 2023, 11:04 a.m. UTC | #1 
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
>
Russell King (Oracle) Oct. 27, 2023, 11:54 a.m. UTC | #2 
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.
Serge Semin Oct. 27, 2023, 12:06 p.m. UTC | #3 
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!
Russell King (Oracle) Oct. 27, 2023, 12:40 p.m. UTC | #4 
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.
patchwork-bot+netdevbpf@kernel.org Oct. 27, 2023, 11:10 p.m. UTC | #5 
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!
Serge Semin Oct. 31, 2023, 12:56 a.m. UTC | #6 
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 mbox series

Patch

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