linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Wong Vee Khee <vee.khee.wong@linux.intel.com>
Cc: "David S . Miller" <davem@davemloft.net>,
	Jose Abreu <Jose.Abreu@synopsys.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Vladimir Oltean <vladimir.oltean@nxp.com>,
	Wong Vee Khee <veekhee@gmail.com>
Subject: Re: [PATCH net v3 1/1] net: pcs: xpcs: fix incorrect CL37 AN sequence
Date: Thu, 30 Sep 2021 10:24:39 +0100	[thread overview]
Message-ID: <YVWCV1Vaq5SAJflX@shell.armlinux.org.uk> (raw)
In-Reply-To: <20210930044421.1309538-1-vee.khee.wong@linux.intel.com>

On Thu, Sep 30, 2021 at 12:44:21PM +0800, Wong Vee Khee wrote:
> According to Synopsys DesignWare Cores Ethernet PCS databook, it is
> required to disable Clause 37 auto-negotiation by programming bit-12
> (AN_ENABLE) to 0 if it is already enabled, before programming various
> fields of VR_MII_AN_CTRL registers.
> 
> After all these programming are done, it is then required to enable
> Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1.
> 
> Fixes: b97b5331b8ab ("net: pcs: add C37 SGMII AN support for intel mGbE controller")
> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Wong Vee Khee <vee.khee.wong@linux.intel.com>
> ---
> v2 -> v3:
>  - Added error handling after xpcs_write().
>  - Added 'changed' flag.
>  - Added fixes tag.
> v1 -> v2:
>  - Removed use of xpcs_modify() helper function.
>  - Add conditional check on inband auto-negotiation.
> ---
>  drivers/net/pcs/pcs-xpcs.c | 41 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index fb0a83dc09ac..d2126f5d5016 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -697,14 +697,18 @@ EXPORT_SYMBOL_GPL(xpcs_config_eee);
>  
>  static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
>  {
> -	int ret;
> +	int ret, reg_val;
> +	int changed = 0;
>  
>  	/* For AN for C37 SGMII mode, the settings are :-
> -	 * 1) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN)
> -	 * 2) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII)
> +	 * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable SGMII AN in case
> +	      it is already enabled)
> +	 * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN)
> +	 * 3) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII)
>  	 *    DW xPCS used with DW EQoS MAC is always MAC side SGMII.
> -	 * 3) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic
> +	 * 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic
>  	 *    speed/duplex mode change by HW after SGMII AN complete)
> +	 * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN)
>  	 *
>  	 * Note: Since it is MAC side SGMII, there is no need to set
>  	 *	 SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
> @@ -712,6 +716,19 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
>  	 *	 between PHY and Link Partner. There is also no need to
>  	 *	 trigger AN restart for MAC-side SGMII.
>  	 */
> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & AN_CL37_EN) {
> +		changed = 1;
> +		reg_val = ret & ~AN_CL37_EN;
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> +				 reg_val);
> +		if (ret < 0)
> +			return ret;
> +	}

I don't think you need to record "changed" here - just maintain a
local copy of the current state of the register, e.g:

+	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
+	if (ret < 0)
+		return ret;
+
+	reg_val = ret;
+	if (reg_val & AN_CL37_EN) {
+		reg_val &= ~AN_CL37_EN;
...

What you're wanting to do is ensure this bit is clear before you do the
register changes, and once complete, set the bit back to one. If the bit
was previously clear, you can omit this write, otherwise the write was
needed - as you have the code. However, the point is that once you're
past this code, the state of the register in the device will have the
AN_CL37_EN clear. See below for the rest of my comments on this...

> @@ -736,7 +753,21 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
>  	else
>  		ret &= ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
>  
> -	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret);
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (changed) {
> +		if (phylink_autoneg_inband(mode))
> +			reg_val |= AN_CL37_EN;
> +		else
> +			reg_val &= ~AN_CL37_EN;
> +
> +		return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> +				  reg_val);
> +	}

As I say above, here, you are _guaranteed_ that the AN_CL27_EN bit is
clear in the register due to the effects of your change above. You say
in the commit text:

  After all these programming are done, it is then required to enable
  Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1.

So that makes me think that you _always_ need to write back a value
with AN_CL27_EN set. So:

	reg_val |= AN_CL37_EN;
	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, reg_val);

If that is not correct, the commit message is misleading and needs
fixing.

Lastly, I would recommend a much better name for this variable rather
than the bland "reg_val" since you're needing the value preserved over
a chunk of code. "ctrl" maybe?

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

  reply	other threads:[~2021-09-30  9:25 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-30  4:44 [PATCH net v3 1/1] net: pcs: xpcs: fix incorrect CL37 AN sequence Wong Vee Khee
2021-09-30  9:24 ` Russell King (Oracle) [this message]
2021-10-01  0:19   ` Vladimir Oltean
2021-10-01  8:35     ` Wong Vee Khee
2021-10-01  9:57     ` Russell King (Oracle)
2021-10-01 10:12       ` Vladimir Oltean
2021-10-01  8:53   ` Wong Vee Khee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YVWCV1Vaq5SAJflX@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=Jose.Abreu@synopsys.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vee.khee.wong@linux.intel.com \
    --cc=veekhee@gmail.com \
    --cc=vladimir.oltean@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).