[PATCH net 0/3] amd-xgbe: Miscellaneous fixes

[PATCH net 0/3] amd-xgbe: Miscellaneous fixes

[Raju Rangoju]

Here are some of the fixes for XGBE:

(1) Fix the rrc for Yellow carp devices. CDR workaround path
    is disabled for YC devices, receiver reset cycle is not
    needed in such cases.

(2) Enable PLL_CTL for fixed PHY modes only. Driver does not
    implement SW RRCM for Autoneg Off configuration, hence PLL
    is needed for fixed PHY modes only.

(3) Fix the SFP compliance codes check for DAC cables. Some of
    the passive cables have non-zero data at offset 3 and 6 in
    SFP EEPROM data. So, fix the sfp compliance codes check.

Raju Rangoju (3):
  amd-xgbe: Yellow carp devices do not need rrc
  amd-xgbe: enable PLL_CTL for fixed PHY modes only
  amd-xgbe: fix the SFP compliance codes check for DAC cables

 drivers/net/ethernet/amd/xgbe/xgbe-pci.c    |  5 ++++
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 33 ++++++++++++---------
 drivers/net/ethernet/amd/xgbe/xgbe.h        | 11 +++++++
 3 files changed, 35 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH net 1/3] amd-xgbe: Yellow carp devices do not need rrc

[Raju Rangoju]

Yellow carp devices disables the CDR workaround path,
receiver reset cycle is not needed in such cases.
Hence, avoid issuing rrc on Yellow carp platforms.

Fixes: 47f164deab22 ("amd-xgbe: Add PCI device support")
Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-pci.c    | 5 +++++
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 2 +-
 drivers/net/ethernet/amd/xgbe/xgbe.h        | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
index 2af3da4b2d05..f409d7bd1f1e 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
@@ -285,6 +285,9 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 		/* Yellow Carp devices do not need cdr workaround */
 		pdata->vdata->an_cdr_workaround = 0;
+
+		/* Yellow Carp devices do not need rrc */
+		pdata->vdata->enable_rrc = 0;
 	} else {
 		pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
 		pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
@@ -483,6 +486,7 @@ static struct xgbe_version_data xgbe_v2a = {
 	.tx_desc_prefetch		= 5,
 	.rx_desc_prefetch		= 5,
 	.an_cdr_workaround		= 1,
+	.enable_rrc			= 1,
 };
 
 static struct xgbe_version_data xgbe_v2b = {
@@ -498,6 +502,7 @@ static struct xgbe_version_data xgbe_v2b = {
 	.tx_desc_prefetch		= 5,
 	.rx_desc_prefetch		= 5,
 	.an_cdr_workaround		= 1,
+	.enable_rrc			= 1,
 };
 
 static const struct pci_device_id xgbe_pci_table[] = {
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 2156600641b6..19b943eba560 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -2640,7 +2640,7 @@ static int xgbe_phy_link_status(struct xgbe_prv_data *pdata, int *an_restart)
 	}
 
 	/* No link, attempt a receiver reset cycle */
-	if (phy_data->rrc_count++ > XGBE_RRC_FREQUENCY) {
+	if (pdata->vdata->enable_rrc && phy_data->rrc_count++ > XGBE_RRC_FREQUENCY) {
 		phy_data->rrc_count = 0;
 		xgbe_phy_rrc(pdata);
 	}
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
index b875c430222e..49d23abce73d 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
@@ -1013,6 +1013,7 @@ struct xgbe_version_data {
 	unsigned int tx_desc_prefetch;
 	unsigned int rx_desc_prefetch;
 	unsigned int an_cdr_workaround;
+	unsigned int enable_rrc;
 };
 
 struct xgbe_prv_data {
-- 
2.25.1

Re: [PATCH net 1/3] amd-xgbe: Yellow carp devices do not need rrc

[Tom Lendacky]

On 10/6/22 08:54, Raju Rangoju wrote:
> Yellow carp devices disables the CDR workaround path,
> receiver reset cycle is not needed in such cases.
> Hence, avoid issuing rrc on Yellow carp platforms.
> 
> Fixes: 47f164deab22 ("amd-xgbe: Add PCI device support")

That is the wrong Fixes: tag. Yellow Carp support was added with commit

dbb6c58b5a61 ("net: amd-xgbe: Add Support for Yellow Carp Ethernet device")

However, the changes to allow updating the version data were made with

6f60ecf233f9 ("net: amd-xgbe: Disable the CDR workaround path for Yellow Carp Devices")

so that is the tag most likely needed should you want this to be able to
go to stable.

With a change to the Fixes: tag:

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
> ---
>   drivers/net/ethernet/amd/xgbe/xgbe-pci.c    | 5 +++++
>   drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 2 +-
>   drivers/net/ethernet/amd/xgbe/xgbe.h        | 1 +
>   3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> index 2af3da4b2d05..f409d7bd1f1e 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> @@ -285,6 +285,9 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   
>   		/* Yellow Carp devices do not need cdr workaround */
>   		pdata->vdata->an_cdr_workaround = 0;
> +
> +		/* Yellow Carp devices do not need rrc */
> +		pdata->vdata->enable_rrc = 0;
>   	} else {
>   		pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>   		pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
> @@ -483,6 +486,7 @@ static struct xgbe_version_data xgbe_v2a = {
>   	.tx_desc_prefetch		= 5,
>   	.rx_desc_prefetch		= 5,
>   	.an_cdr_workaround		= 1,
> +	.enable_rrc			= 1,
>   };
>   
>   static struct xgbe_version_data xgbe_v2b = {
> @@ -498,6 +502,7 @@ static struct xgbe_version_data xgbe_v2b = {
>   	.tx_desc_prefetch		= 5,
>   	.rx_desc_prefetch		= 5,
>   	.an_cdr_workaround		= 1,
> +	.enable_rrc			= 1,
>   };
>   
>   static const struct pci_device_id xgbe_pci_table[] = {
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> index 2156600641b6..19b943eba560 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> @@ -2640,7 +2640,7 @@ static int xgbe_phy_link_status(struct xgbe_prv_data *pdata, int *an_restart)
>   	}
>   
>   	/* No link, attempt a receiver reset cycle */
> -	if (phy_data->rrc_count++ > XGBE_RRC_FREQUENCY) {
> +	if (pdata->vdata->enable_rrc && phy_data->rrc_count++ > XGBE_RRC_FREQUENCY) {
>   		phy_data->rrc_count = 0;
>   		xgbe_phy_rrc(pdata);
>   	}
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
> index b875c430222e..49d23abce73d 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
> @@ -1013,6 +1013,7 @@ struct xgbe_version_data {
>   	unsigned int tx_desc_prefetch;
>   	unsigned int rx_desc_prefetch;
>   	unsigned int an_cdr_workaround;
> +	unsigned int enable_rrc;
>   };
>   
>   struct xgbe_prv_data {

Re: [PATCH net 1/3] amd-xgbe: Yellow carp devices do not need rrc

[Jakub Kicinski]

On Thu, 6 Oct 2022 09:32:34 -0500 Tom Lendacky wrote:
> On 10/6/22 08:54, Raju Rangoju wrote:
> > Yellow carp devices disables the CDR workaround path,
> > receiver reset cycle is not needed in such cases.
> > Hence, avoid issuing rrc on Yellow carp platforms.

Not entirely clear why this is a Fix, TBH.

What harm comes from doing the reset? You need to describe 
the user-observable harm if the patch is a fix.

> > Fixes: 47f164deab22 ("amd-xgbe: Add PCI device support")  
> 
> That is the wrong Fixes: tag. Yellow Carp support was added with commit
> 
> dbb6c58b5a61 ("net: amd-xgbe: Add Support for Yellow Carp Ethernet device")
> 
> However, the changes to allow updating the version data were made with
> 
> 6f60ecf233f9 ("net: amd-xgbe: Disable the CDR workaround path for Yellow Carp Devices")
> 
> so that is the tag most likely needed should you want this to be able to
> go to stable.

FWIW the Fixes tag should point to the commit where the bug is
introduced, not where the patch will apply. The automation will
figure out where it applies.

> With a change to the Fixes: tag:
> 
> Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

These devices are only present on SoCs? Changing global data during
probe looks odd.

Re: [PATCH net 1/3] amd-xgbe: Yellow carp devices do not need rrc

[Raju Rangoju]

On 10/7/2022 5:56 AM, Jakub Kicinski wrote:
> On Thu, 6 Oct 2022 09:32:34 -0500 Tom Lendacky wrote:
>> On 10/6/22 08:54, Raju Rangoju wrote:
>>> Yellow carp devices disables the CDR workaround path,
>>> receiver reset cycle is not needed in such cases.
>>> Hence, avoid issuing rrc on Yellow carp platforms.
> 
> Not entirely clear why this is a Fix, TBH.
> 
> What harm comes from doing the reset? You need to describe
> the user-observable harm if the patch is a fix.

Link stability issues are noticed if RRC is issued on Yellow carp 
platforms. Since the CDR workaround is disabled on these platforms, the 
Receiver Reset Cycle is not needed.

> 
>>> Fixes: 47f164deab22 ("amd-xgbe: Add PCI device support")
>>
>> That is the wrong Fixes: tag. Yellow Carp support was added with commit
>>
>> dbb6c58b5a61 ("net: amd-xgbe: Add Support for Yellow Carp Ethernet device")
>>
>> However, the changes to allow updating the version data were made with
>>
>> 6f60ecf233f9 ("net: amd-xgbe: Disable the CDR workaround path for Yellow Carp Devices")
>>
>> so that is the tag most likely needed should you want this to be able to
>> go to stable.
> 
> FWIW the Fixes tag should point to the commit where the bug is
> introduced, not where the patch will apply. The automation will
> figure out where it applies.
> 
>> With a change to the Fixes: tag:
>>
>> Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
> These devices are only present on SoCs? Changing global data during
> probe looks odd.
> 

[PATCH net 2/3] amd-xgbe: enable PLL_CTL for fixed PHY modes only

[Raju Rangoju]

PLL control setting(HW RRCM) is needed only in fixed PHY configuration
to fix the peer-peer issues. Without the PLL control setting, the link
up takes longer time in a fixed phy configuration.

Driver implements SW RRCM for Autoneg On configuration, hence PLL
control setting (HW RRCM) is not needed for AN On configuration, and
can be skipped.

Also, PLL re-initialization is not needed for PHY Power Off and RRCM
commands. Otherwise, they lead to mailbox errors. Added the changes
accordingly.

Fixes: daf182d360e5 ("net: amd-xgbe: Toggle PLL settings during rate change")
Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 21 +++++++++++++--------
 drivers/net/ethernet/amd/xgbe/xgbe.h        | 10 ++++++++++
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 19b943eba560..23fbd89a29df 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -1979,13 +1979,16 @@ static void xgbe_phy_rx_reset(struct xgbe_prv_data *pdata)
 
 static void xgbe_phy_pll_ctrl(struct xgbe_prv_data *pdata, bool enable)
 {
-	XMDIO_WRITE_BITS(pdata, MDIO_MMD_PMAPMD, MDIO_VEND2_PMA_MISC_CTRL0,
-			 XGBE_PMA_PLL_CTRL_MASK,
-			 enable ? XGBE_PMA_PLL_CTRL_ENABLE
-				: XGBE_PMA_PLL_CTRL_DISABLE);
+	/* PLL_CTRL feature needs to be enabled for fixed PHY modes (Non-Autoneg) only */
+	if (pdata->phy.autoneg == AUTONEG_DISABLE) {
+		XMDIO_WRITE_BITS(pdata, MDIO_MMD_PMAPMD, MDIO_VEND2_PMA_MISC_CTRL0,
+				 XGBE_PMA_PLL_CTRL_MASK,
+				 enable ? XGBE_PMA_PLL_CTRL_ENABLE
+					: XGBE_PMA_PLL_CTRL_DISABLE);
 
-	/* Wait for command to complete */
-	usleep_range(100, 200);
+		/* Wait for command to complete */
+		usleep_range(100, 200);
+	}
 }
 
 static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
@@ -2029,8 +2032,10 @@ static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
 	xgbe_phy_rx_reset(pdata);
 
 reenable_pll:
-	/* Enable PLL re-initialization */
-	xgbe_phy_pll_ctrl(pdata, true);
+	/* Enable PLL re-initialization, not needed for PHY Power Off cmd */
+	if (cmd != XGBE_MAILBOX_CMD_POWER_OFF &&
+	    cmd != XGBE_MAILBOX_CMD_RRCM)
+		xgbe_phy_pll_ctrl(pdata, true);
 }
 
 static void xgbe_phy_rrc(struct xgbe_prv_data *pdata)
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
index 49d23abce73d..c7865681790c 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
@@ -611,6 +611,16 @@ enum xgbe_mdio_mode {
 	XGBE_MDIO_MODE_CL45,
 };
 
+enum XGBE_MAILBOX_CMD {
+	XGBE_MAILBOX_CMD_POWER_OFF	= 0,
+	XGBE_MAILBOX_CMD_SET_1G		= 1,
+	XGBE_MAILBOX_CMD_SET_2_5G	= 2,
+	XGBE_MAILBOX_CMD_SET_10G_SFI	= 3,
+	XGBE_MAILBOX_CMD_SET_10G_KR	= 4,
+	XGBE_MAILBOX_CMD_RRCM		= 5,
+	XGBE_MAILBOX_CMD_UNKNOWN	= 6
+};
+
 struct xgbe_phy {
 	struct ethtool_link_ksettings lks;
 
-- 
2.25.1

Re: [PATCH net 2/3] amd-xgbe: enable PLL_CTL for fixed PHY modes only

[Tom Lendacky]

On 10/6/22 08:54, Raju Rangoju wrote:
> PLL control setting(HW RRCM) is needed only in fixed PHY configuration
> to fix the peer-peer issues. Without the PLL control setting, the link
> up takes longer time in a fixed phy configuration.
> 
> Driver implements SW RRCM for Autoneg On configuration, hence PLL
> control setting (HW RRCM) is not needed for AN On configuration, and
> can be skipped.
> 
> Also, PLL re-initialization is not needed for PHY Power Off and RRCM
> commands. Otherwise, they lead to mailbox errors. Added the changes
> accordingly.
> 
> Fixes: daf182d360e5 ("net: amd-xgbe: Toggle PLL settings during rate change")
> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
> ---
>   drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 21 +++++++++++++--------
>   drivers/net/ethernet/amd/xgbe/xgbe.h        | 10 ++++++++++
>   2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> index 19b943eba560..23fbd89a29df 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> @@ -1979,13 +1979,16 @@ static void xgbe_phy_rx_reset(struct xgbe_prv_data *pdata)
>   
>   static void xgbe_phy_pll_ctrl(struct xgbe_prv_data *pdata, bool enable)
>   {
> -	XMDIO_WRITE_BITS(pdata, MDIO_MMD_PMAPMD, MDIO_VEND2_PMA_MISC_CTRL0,
> -			 XGBE_PMA_PLL_CTRL_MASK,
> -			 enable ? XGBE_PMA_PLL_CTRL_ENABLE
> -				: XGBE_PMA_PLL_CTRL_DISABLE);
> +	/* PLL_CTRL feature needs to be enabled for fixed PHY modes (Non-Autoneg) only */
> +	if (pdata->phy.autoneg == AUTONEG_DISABLE) {
> +		XMDIO_WRITE_BITS(pdata, MDIO_MMD_PMAPMD, MDIO_VEND2_PMA_MISC_CTRL0,
> +				 XGBE_PMA_PLL_CTRL_MASK,
> +				 enable ? XGBE_PMA_PLL_CTRL_ENABLE
> +					: XGBE_PMA_PLL_CTRL_DISABLE);
>   
> -	/* Wait for command to complete */
> -	usleep_range(100, 200);
> +		/* Wait for command to complete */
> +		usleep_range(100, 200);
> +	}

Rather than indent all this, just add an if that returns at the beginning
of the function, e.g.:

	/* PLL_CTRL feature needs to be enabled for fixed PHY modes (Non-Autoneg) only */
	if (pdata->phy.autoneg != AUTONEG_DISABLE)
		return;

Now a general question... is this going to force Autoneg ON to end up
always going through the RRC path now where it may not have before? In
other words, there's now an auto-negotiation delay?

>   }
>   
>   static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
> @@ -2029,8 +2032,10 @@ static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
>   	xgbe_phy_rx_reset(pdata);
>   
>   reenable_pll:
> -	/* Enable PLL re-initialization */
> -	xgbe_phy_pll_ctrl(pdata, true);
> +	/* Enable PLL re-initialization, not needed for PHY Power Off cmd */

Comment should also include the RRC command...

> +	if (cmd != XGBE_MAILBOX_CMD_POWER_OFF &&
> +	    cmd != XGBE_MAILBOX_CMD_RRCM)
> +		xgbe_phy_pll_ctrl(pdata, true);
>   }
>   
>   static void xgbe_phy_rrc(struct xgbe_prv_data *pdata)
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
> index 49d23abce73d..c7865681790c 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
> @@ -611,6 +611,16 @@ enum xgbe_mdio_mode {
>   	XGBE_MDIO_MODE_CL45,
>   };
>   
> +enum XGBE_MAILBOX_CMD {
> +	XGBE_MAILBOX_CMD_POWER_OFF	= 0,
> +	XGBE_MAILBOX_CMD_SET_1G		= 1,
> +	XGBE_MAILBOX_CMD_SET_2_5G	= 2,
> +	XGBE_MAILBOX_CMD_SET_10G_SFI	= 3,
> +	XGBE_MAILBOX_CMD_SET_10G_KR	= 4,
> +	XGBE_MAILBOX_CMD_RRCM		= 5,
> +	XGBE_MAILBOX_CMD_UNKNOWN	= 6
> +};

If you're going to add an enum for the commands, then you should apply
them everywhere. Creating this enum and updating all the command locations
should be a pre-patch to this patch.

Thanks,
Tom

> +
>   struct xgbe_phy {
>   	struct ethtool_link_ksettings lks;
>   

Re: [PATCH net 2/3] amd-xgbe: enable PLL_CTL for fixed PHY modes only

[Raju Rangoju]

Hi Tom,

On 10/6/2022 8:17 PM, Tom Lendacky wrote:
> On 10/6/22 08:54, Raju Rangoju wrote:
>> PLL control setting(HW RRCM) is needed only in fixed PHY configuration
>> to fix the peer-peer issues. Without the PLL control setting, the link
>> up takes longer time in a fixed phy configuration.
>>
>> Driver implements SW RRCM for Autoneg On configuration, hence PLL
>> control setting (HW RRCM) is not needed for AN On configuration, and
>> can be skipped.
>>
>> Also, PLL re-initialization is not needed for PHY Power Off and RRCM
>> commands. Otherwise, they lead to mailbox errors. Added the changes
>> accordingly.
>>
>> Fixes: daf182d360e5 ("net: amd-xgbe: Toggle PLL settings during rate 
>> change")
>> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
>> ---
>>   drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 21 +++++++++++++--------
>>   drivers/net/ethernet/amd/xgbe/xgbe.h        | 10 ++++++++++
>>   2 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c 
>> b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>> index 19b943eba560..23fbd89a29df 100644
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>> @@ -1979,13 +1979,16 @@ static void xgbe_phy_rx_reset(struct 
>> xgbe_prv_data *pdata)
>>   static void xgbe_phy_pll_ctrl(struct xgbe_prv_data *pdata, bool enable)
>>   {
>> -    XMDIO_WRITE_BITS(pdata, MDIO_MMD_PMAPMD, MDIO_VEND2_PMA_MISC_CTRL0,
>> -             XGBE_PMA_PLL_CTRL_MASK,
>> -             enable ? XGBE_PMA_PLL_CTRL_ENABLE
>> -                : XGBE_PMA_PLL_CTRL_DISABLE);
>> +    /* PLL_CTRL feature needs to be enabled for fixed PHY modes 
>> (Non-Autoneg) only */
>> +    if (pdata->phy.autoneg == AUTONEG_DISABLE) {
>> +        XMDIO_WRITE_BITS(pdata, MDIO_MMD_PMAPMD, 
>> MDIO_VEND2_PMA_MISC_CTRL0,
>> +                 XGBE_PMA_PLL_CTRL_MASK,
>> +                 enable ? XGBE_PMA_PLL_CTRL_ENABLE
>> +                    : XGBE_PMA_PLL_CTRL_DISABLE);
>> -    /* Wait for command to complete */
>> -    usleep_range(100, 200);
>> +        /* Wait for command to complete */
>> +        usleep_range(100, 200);
>> +    }
> 
> Rather than indent all this, just add an if that returns at the beginning
> of the function, e.g.:
> 
>      /* PLL_CTRL feature needs to be enabled for fixed PHY modes 
> (Non-Autoneg) only */
>      if (pdata->phy.autoneg != AUTONEG_DISABLE)
>          return;

Sure, I'll add the above changes.

> 
> Now a general question... is this going to force Autoneg ON to end up
> always going through the RRC path now where it may not have before? In
> other words, there's now an auto-negotiation delay?

As per the databook, Receiver power state change (HW RRCM) is not 
allowed during the CL73 / CL72. FW is already implementing SW RRCM for 
Autoneg ON, and no additional changes are needed in driver for AN ON 
path. However, HW RRCM(PLL_CTRL) is needed in Fixed PHY configs.

> 
>>   }
>>   static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
>> @@ -2029,8 +2032,10 @@ static void xgbe_phy_perform_ratechange(struct 
>> xgbe_prv_data *pdata,
>>       xgbe_phy_rx_reset(pdata);
>>   reenable_pll:
>> -    /* Enable PLL re-initialization */
>> -    xgbe_phy_pll_ctrl(pdata, true);
>> +    /* Enable PLL re-initialization, not needed for PHY Power Off cmd */
> 
> Comment should also include the RRC command...
> 
>> +    if (cmd != XGBE_MAILBOX_CMD_POWER_OFF &&
>> +        cmd != XGBE_MAILBOX_CMD_RRCM)
>> +        xgbe_phy_pll_ctrl(pdata, true);
>>   }
>>   static void xgbe_phy_rrc(struct xgbe_prv_data *pdata)
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h 
>> b/drivers/net/ethernet/amd/xgbe/xgbe.h
>> index 49d23abce73d..c7865681790c 100644
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
>> @@ -611,6 +611,16 @@ enum xgbe_mdio_mode {
>>       XGBE_MDIO_MODE_CL45,
>>   };
>> +enum XGBE_MAILBOX_CMD {
>> +    XGBE_MAILBOX_CMD_POWER_OFF    = 0,
>> +    XGBE_MAILBOX_CMD_SET_1G        = 1,
>> +    XGBE_MAILBOX_CMD_SET_2_5G    = 2,
>> +    XGBE_MAILBOX_CMD_SET_10G_SFI    = 3,
>> +    XGBE_MAILBOX_CMD_SET_10G_KR    = 4,
>> +    XGBE_MAILBOX_CMD_RRCM        = 5,
>> +    XGBE_MAILBOX_CMD_UNKNOWN    = 6
>> +};
> 
> If you're going to add an enum for the commands, then you should apply
> them everywhere. Creating this enum and updating all the command locations
> should be a pre-patch to this patch.
> 
> Thanks,
> Tom
> 
>> +
>>   struct xgbe_phy {
>>       struct ethtool_link_ksettings lks;

[PATCH net 3/3] amd-xgbe: fix the SFP compliance codes check for DAC cables

[Raju Rangoju]

The current XGBE code assumes that offset 3 and 6 of EEPROM SFP DAC
(passive) cables are NULL. It also assumes the offset 12 is in the
range 0x64 to 0x68. However, some of the cables (the 5 meter and 7 meter
molex passive cables have non-zero data at offset 3 and 6, also a value
0x78 at offset 12. So, fix the sfp compliance codes check to ignore
those offsets. Also extend the macro XGBE_SFP_BASE_BR_10GBE range to 0x78.

Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules")
Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 23fbd89a29df..0387e691be68 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -238,7 +238,7 @@ enum xgbe_sfp_speed {
 #define XGBE_SFP_BASE_BR_1GBE_MIN		0x0a
 #define XGBE_SFP_BASE_BR_1GBE_MAX		0x0d
 #define XGBE_SFP_BASE_BR_10GBE_MIN		0x64
-#define XGBE_SFP_BASE_BR_10GBE_MAX		0x68
+#define XGBE_SFP_BASE_BR_10GBE_MAX		0x78
 
 #define XGBE_SFP_BASE_CU_CABLE_LEN		18
 
@@ -1151,7 +1151,10 @@ static void xgbe_phy_sfp_parse_eeprom(struct xgbe_prv_data *pdata)
 	}
 
 	/* Determine the type of SFP */
-	if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & XGBE_SFP_BASE_10GBE_CC_SR)
+	if (phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE &&
+	    xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
+		phy_data->sfp_base = XGBE_SFP_BASE_10000_CR;
+	else if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & XGBE_SFP_BASE_10GBE_CC_SR)
 		phy_data->sfp_base = XGBE_SFP_BASE_10000_SR;
 	else if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & XGBE_SFP_BASE_10GBE_CC_LR)
 		phy_data->sfp_base = XGBE_SFP_BASE_10000_LR;
@@ -1167,9 +1170,6 @@ static void xgbe_phy_sfp_parse_eeprom(struct xgbe_prv_data *pdata)
 		phy_data->sfp_base = XGBE_SFP_BASE_1000_CX;
 	else if (sfp_base[XGBE_SFP_BASE_1GBE_CC] & XGBE_SFP_BASE_1GBE_CC_T)
 		phy_data->sfp_base = XGBE_SFP_BASE_1000_T;
-	else if ((phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE) &&
-		 xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
-		phy_data->sfp_base = XGBE_SFP_BASE_10000_CR;
 
 	switch (phy_data->sfp_base) {
 	case XGBE_SFP_BASE_1000_T:
-- 
2.25.1

Re: [PATCH net 3/3] amd-xgbe: fix the SFP compliance codes check for DAC cables

[Tom Lendacky]

On 10/6/22 08:54, Raju Rangoju wrote:
> The current XGBE code assumes that offset 3 and 6 of EEPROM SFP DAC
> (passive) cables are NULL. It also assumes the offset 12 is in the
> range 0x64 to 0x68. However, some of the cables (the 5 meter and 7 meter
> molex passive cables have non-zero data at offset 3 and 6, also a value
> 0x78 at offset 12. So, fix the sfp compliance codes check to ignore
> those offsets. Also extend the macro XGBE_SFP_BASE_BR_10GBE range to 0x78.

So are these cables going against the specification? Should they be quirks 
instead of changing the way code is currently operating? How many 
different cables have you found that do this?

Why would a passive cable be setting any bit other than passive in byte 3? 
Why would byte 6 also have a non-zero value?

As for the range, 0x78 puts the cable at 12gbps which kind of seems 
outside the normal range of what a 10gbps cable should be reporting.

I guess I'm not opposed to the ordering of the SFP checks (moving the 
passive check up as the first check), but the reasons seem odd, hence my 
question of whether this should be a quirk.

Thanks,
Tom

> 
> Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules")
> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
> ---
>   drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> index 23fbd89a29df..0387e691be68 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> @@ -238,7 +238,7 @@ enum xgbe_sfp_speed {
>   #define XGBE_SFP_BASE_BR_1GBE_MIN		0x0a
>   #define XGBE_SFP_BASE_BR_1GBE_MAX		0x0d
>   #define XGBE_SFP_BASE_BR_10GBE_MIN		0x64
> -#define XGBE_SFP_BASE_BR_10GBE_MAX		0x68
> +#define XGBE_SFP_BASE_BR_10GBE_MAX		0x78
>   
>   #define XGBE_SFP_BASE_CU_CABLE_LEN		18
>   
> @@ -1151,7 +1151,10 @@ static void xgbe_phy_sfp_parse_eeprom(struct xgbe_prv_data *pdata)
>   	}
>   
>   	/* Determine the type of SFP */
> -	if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & XGBE_SFP_BASE_10GBE_CC_SR)
> +	if (phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE &&
> +	    xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
> +		phy_data->sfp_base = XGBE_SFP_BASE_10000_CR;
> +	else if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & XGBE_SFP_BASE_10GBE_CC_SR)
>   		phy_data->sfp_base = XGBE_SFP_BASE_10000_SR;
>   	else if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & XGBE_SFP_BASE_10GBE_CC_LR)
>   		phy_data->sfp_base = XGBE_SFP_BASE_10000_LR;
> @@ -1167,9 +1170,6 @@ static void xgbe_phy_sfp_parse_eeprom(struct xgbe_prv_data *pdata)
>   		phy_data->sfp_base = XGBE_SFP_BASE_1000_CX;
>   	else if (sfp_base[XGBE_SFP_BASE_1GBE_CC] & XGBE_SFP_BASE_1GBE_CC_T)
>   		phy_data->sfp_base = XGBE_SFP_BASE_1000_T;
> -	else if ((phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE) &&
> -		 xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
> -		phy_data->sfp_base = XGBE_SFP_BASE_10000_CR;
>   
>   	switch (phy_data->sfp_base) {
>   	case XGBE_SFP_BASE_1000_T:

Re: [PATCH net 3/3] amd-xgbe: fix the SFP compliance codes check for DAC cables

[Tom Lendacky]

On 10/6/22 12:37, Raju Rangoju wrote:
> On 10/6/2022 8:30 PM, Tom Lendacky wrote:
>> On 10/6/22 08:54, Raju Rangoju wrote:
>>> The current XGBE code assumes that offset 3 and 6 of EEPROM SFP DAC
>>> (passive) cables are NULL. It also assumes the offset 12 is in the
>>> range 0x64 to 0x68. However, some of the cables (the 5 meter and 7 meter
>>> molex passive cables have non-zero data at offset 3 and 6, also a value
>>> 0x78 at offset 12. So, fix the sfp compliance codes check to ignore
>>> those offsets. Also extend the macro XGBE_SFP_BASE_BR_10GBE range to 0x78.
>>
>> So are these cables going against the specification? Should they be 
>> quirks instead of changing the way code is currently operating? How many 
>> different cables have you found that do this?
>>
>> Why would a passive cable be setting any bit other than passive in byte 
>> 3? Why would byte 6 also have a non-zero value?
>>
>> As for the range, 0x78 puts the cable at 12gbps which kind of seems 
>> outside the normal range of what a 10gbps cable should be reporting.
>>
> 
> For the passive cables, the current SFP checks in driver are not expecting 
> any data at offset 3 and 6. Also, the offset 12 is expected to be in the 
> range 0x64 - 0x68. This was holding good for Fiber store cables so far. 
> However, the 5 and 7 meter Molex passive cables have non-zero data at 
> offset 3 and 6, and also a value 0x78 at offset 12.

The 0x64 - 0x68 BR range was holding well for the various passive cables 
that I tested with. What is the BR value for their other length cables?

> 
> Here is the feedback from cable Vendor when asked about the SFP standard 
> for passive cables:
> 
> "For DAC cables –The Ethernet code compliance code standard for passive 
> cabling, Offset 3 is “0x0” other offsets  4 and 5 - none of the standards 
> are applicable .

Ok, so it's not offset 3 that is the issue as none of the bits are set and 
won't trigger on the 10G Ethernet Compliance Codes.

> Offset 6 – refers to 1000base-cx which is supported .

Ok, that makes sense and argues for moving the passive check first, 
although the code doesn't support being able to switch to 1000Base-CX.


> For passive cable , there is no separate bit to define the compliance code 
> for 10G as per the SFF standard. Please modify your SW accordingly.
> "

This doesn't answer the question about the BR range. 0x78 seems excessive 
to me (12,000 mbps) and so I'm not sure what effect increasing the range 
will have in general vs restricting the change to just the vendor/part 
having the issue.

Thanks,
Tom

> 
>> I guess I'm not opposed to the ordering of the SFP checks (moving the 
>> passive check up as the first check), but the reasons seem odd, hence my 
>> question of whether this should be a quirk.
>>
>> Thanks,
>> Tom
>>
>>>
>>> Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules")
>>> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
>>> ---
>>>   drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c 
>>> b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>> index 23fbd89a29df..0387e691be68 100644
>>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
>>> @@ -238,7 +238,7 @@ enum xgbe_sfp_speed {
>>>   #define XGBE_SFP_BASE_BR_1GBE_MIN        0x0a
>>>   #define XGBE_SFP_BASE_BR_1GBE_MAX        0x0d
>>>   #define XGBE_SFP_BASE_BR_10GBE_MIN        0x64
>>> -#define XGBE_SFP_BASE_BR_10GBE_MAX        0x68
>>> +#define XGBE_SFP_BASE_BR_10GBE_MAX        0x78
>>>   #define XGBE_SFP_BASE_CU_CABLE_LEN        18
>>> @@ -1151,7 +1151,10 @@ static void xgbe_phy_sfp_parse_eeprom(struct 
>>> xgbe_prv_data *pdata)
>>>       }
>>>       /* Determine the type of SFP */
>>> -    if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & XGBE_SFP_BASE_10GBE_CC_SR)
>>> +    if (phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE &&
>>> +        xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
>>> +        phy_data->sfp_base = XGBE_SFP_BASE_10000_CR;
>>> +    else if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & 
>>> XGBE_SFP_BASE_10GBE_CC_SR)
>>>           phy_data->sfp_base = XGBE_SFP_BASE_10000_SR;
>>>       else if (sfp_base[XGBE_SFP_BASE_10GBE_CC] & 
>>> XGBE_SFP_BASE_10GBE_CC_LR)
>>>           phy_data->sfp_base = XGBE_SFP_BASE_10000_LR;
>>> @@ -1167,9 +1170,6 @@ static void xgbe_phy_sfp_parse_eeprom(struct 
>>> xgbe_prv_data *pdata)
>>>           phy_data->sfp_base = XGBE_SFP_BASE_1000_CX;
>>>       else if (sfp_base[XGBE_SFP_BASE_1GBE_CC] & XGBE_SFP_BASE_1GBE_CC_T)
>>>           phy_data->sfp_base = XGBE_SFP_BASE_1000_T;
>>> -    else if ((phy_data->sfp_cable == XGBE_SFP_CABLE_PASSIVE) &&
>>> -         xgbe_phy_sfp_bit_rate(sfp_eeprom, XGBE_SFP_SPEED_10000))
>>> -        phy_data->sfp_base = XGBE_SFP_BASE_10000_CR;
>>>       switch (phy_data->sfp_base) {
>>>       case XGBE_SFP_BASE_1000_T:

Re: [PATCH net 3/3] amd-xgbe: fix the SFP compliance codes check for DAC cables

[Tom Lendacky]

On 10/7/22 07:25, Raju Rangoju wrote:
> On 10/7/2022 12:34 AM, Tom Lendacky wrote:
>> On 10/6/22 12:37, Raju Rangoju wrote:
>>> On 10/6/2022 8:30 PM, Tom Lendacky wrote:
>>>> On 10/6/22 08:54, Raju Rangoju wrote:
>>>>> The current XGBE code assumes that offset 3 and 6 of EEPROM SFP DAC
>>>>> (passive) cables are NULL. It also assumes the offset 12 is in the
>>>>> range 0x64 to 0x68. However, some of the cables (the 5 meter and 7 meter
>>>>> molex passive cables have non-zero data at offset 3 and 6, also a value
>>>>> 0x78 at offset 12. So, fix the sfp compliance codes check to ignore
>>>>> those offsets. Also extend the macro XGBE_SFP_BASE_BR_10GBE range to 
>>>>> 0x78.
>>>>
>>>> So are these cables going against the specification? Should they be 
>>>> quirks instead of changing the way code is currently operating? How 
>>>> many different cables have you found that do this?
>>>>
>>>> Why would a passive cable be setting any bit other than passive in 
>>>> byte 3? Why would byte 6 also have a non-zero value?
>>>>
>>>> As for the range, 0x78 puts the cable at 12gbps which kind of seems 
>>>> outside the normal range of what a 10gbps cable should be reporting.
>>>>
>>>
>>> For the passive cables, the current SFP checks in driver are not 
>>> expecting any data at offset 3 and 6. Also, the offset 12 is expected 
>>> to be in the range 0x64 - 0x68. This was holding good for Fiber store 
>>> cables so far. However, the 5 and 7 meter Molex passive cables have 
>>> non-zero data at offset 3 and 6, and also a value 0x78 at offset 12.
>>
>> The 0x64 - 0x68 BR range was holding well for the various passive cables 
>> that I tested with. What is the BR value for their other length cables?
> 
> The 1m and 3m cables have the BR value within the range 0x64 - 0x68.

That certainly seems like 5 and 7 meter cables have an inconsistent value 
then, no? A quirk for Molex vendor or the specific part series, e.g. 
74752- or such in xgbe_phy_sfp_bit_rate() might be a better solution here 
than expanding the range for all cable vendors.

Not sure if others with more SFP+ experience could respond and indicate if 
0x78 is really valid for a 10Gbps cable.

> 
>>
>>>
>>> Here is the feedback from cable Vendor when asked about the SFP 
>>> standard for passive cables:
>>>
>>> "For DAC cables –The Ethernet code compliance code standard for passive 
>>> cabling, Offset 3 is “0x0” other offsets  4 and 5 - none of the 
>>> standards are applicable .
>>
>> Ok, so it's not offset 3 that is the issue as none of the bits are set 
>> and won't trigger on the 10G Ethernet Compliance Codes.
> 
> As per the Transceiver compliance codes, offset 3 bits 4,5,6,7 represent 
> 10Gbps speed. However, 5m and 7m transceivers have none of those bits set.

Right, and passive cables shouldn't. Those bits are:

   Bit 7: 10G Base-ER
   Bit 6: 10G Base-LRM
   Bit 5: 10G Base-LR
   Bit 4: 10G Base-SR

which are all Fiber standards.

> On the other hand, offset 6 is set, so the driver incorrectly assumes the 
> transceiver as a 1Gbps cable.

Correct, 1000Base-CX is copper, e.g., passive. So, as stated in my 
previous email, I think moving the passive check up as the first check is 
reasonable.

Thanks,
Tom

> 
> Transceiver details:
> EEPROM sfp_base offset 0-12:
> 0:  0x3   1: 0x4   2: 0x21  3: 0x1   4: 0x0   5: 0x0   6: 0x4   7: 0x41
> 8:  0x84  9: 0x80 10: 0xd5 11: 0x0  12: 0x78
> 
> enp7s0f2:   vendor:         Molex Inc.
> enp7s0f2:   part number:    74752-1701
> enp7s0f2:   revision level:
> enp7s0f2:   serial number:  726341570
> 
>>> Offset 6 – refers to 1000base-cx which is supported .
>>
>> Ok, that makes sense and argues for moving the passive check first, 
>> although the code doesn't support being able to switch to 1000Base-CX.
>>
>>