[PATCH net-next v2 0/3] at803x fiber/SFP support

[PATCH net-next v2 0/3] at803x fiber/SFP support

[Robert Hancock]

Add support for 1000Base-X fiber modes to the at803x PHY driver, as
well as support for connecting a downstream SFP cage.

Changes since v1:
-moved page selection to config_init so it is handled properly
after suspend/resume
-added explicit check for empty sfp_support bitmask

Robert Hancock (3):
  net: phy: at803x: move page selection fix to config_init
  net: phy: at803x: add fiber support
  net: phy: at803x: Support downstream SFP cage

 drivers/net/phy/at803x.c | 146 +++++++++++++++++++++++++++++++++------
 1 file changed, 126 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH net-next v2 1/3] net: phy: at803x: move page selection fix to config_init

[Robert Hancock]

The fix to select the copper page on AR8031 was being done in the probe
function rather than config_init, so it would not be redone after resume
from suspend. Move this to config_init so it is always redone when
needed.

Fixes: c329e5afb42f ("net: phy: at803x: select correct page on config init")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/net/phy/at803x.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index dae95d9a07e8..23d6f2e5f48b 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -784,25 +784,7 @@ static int at803x_probe(struct phy_device *phydev)
 			return ret;
 	}
 
-	/* Some bootloaders leave the fiber page selected.
-	 * Switch to the copper page, as otherwise we read
-	 * the PHY capabilities from the fiber side.
-	 */
-	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
-		phy_lock_mdio_bus(phydev);
-		ret = at803x_write_page(phydev, AT803X_PAGE_COPPER);
-		phy_unlock_mdio_bus(phydev);
-		if (ret)
-			goto err;
-	}
-
 	return 0;
-
-err:
-	if (priv->vddio)
-		regulator_disable(priv->vddio);
-
-	return ret;
 }
 
 static void at803x_remove(struct phy_device *phydev)
@@ -912,6 +894,22 @@ static int at803x_config_init(struct phy_device *phydev)
 {
 	int ret;
 
+	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
+	       /* Some bootloaders leave the fiber page selected.
+		* Switch to the copper page, as otherwise we read
+		* the PHY capabilities from the fiber side.
+		*/
+		phy_lock_mdio_bus(phydev);
+		ret = at803x_write_page(phydev, AT803X_PAGE_COPPER);
+		phy_unlock_mdio_bus(phydev);
+		if (ret)
+			return ret;
+
+		ret = at8031_pll_config(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* The RX and TX delay default is:
 	 *   after HW reset: RX delay enabled and TX delay disabled
 	 *   after SW reset: RX delay enabled, while TX delay retains the
@@ -941,12 +939,6 @@ static int at803x_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
-		ret = at8031_pll_config(phydev);
-		if (ret < 0)
-			return ret;
-	}
-
 	/* Ar803x extended next page bit is enabled by default. Cisco
 	 * multigig switches read this bit and attempt to negotiate 10Gbps
 	 * rates even if the next page bit is disabled. This is incorrect
-- 
2.31.1

[PATCH net-next v2 2/3] net: phy: at803x: add fiber support

[Robert Hancock]

Previously this driver always forced the copper page to be selected,
however for AR8031 in 100Base-FX or 1000Base-X modes, the fiber page
needs to be selected. Set the appropriate mode based on the hardware
mode_cfg strap selection.

Enable the appropriate interrupt bits to detect fiber-side link up
or down events.

Update config_aneg and read_status methods to use the appropriate
Clause 37 calls when fiber mode is in use.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/net/phy/at803x.c | 80 ++++++++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 23d6f2e5f48b..63d84eb2eddb 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -51,6 +51,8 @@
 #define AT803X_INTR_ENABLE_PAGE_RECEIVED	BIT(12)
 #define AT803X_INTR_ENABLE_LINK_FAIL		BIT(11)
 #define AT803X_INTR_ENABLE_LINK_SUCCESS		BIT(10)
+#define AT803X_INTR_ENABLE_LINK_FAIL_BX		BIT(8)
+#define AT803X_INTR_ENABLE_LINK_SUCCESS_BX	BIT(7)
 #define AT803X_INTR_ENABLE_WIRESPEED_DOWNGRADE	BIT(5)
 #define AT803X_INTR_ENABLE_POLARITY_CHANGED	BIT(1)
 #define AT803X_INTR_ENABLE_WOL			BIT(0)
@@ -85,7 +87,17 @@
 #define AT803X_DEBUG_DATA			0x1E
 
 #define AT803X_MODE_CFG_MASK			0x0F
-#define AT803X_MODE_CFG_SGMII			0x01
+#define AT803X_MODE_CFG_BASET_RGMII		0x00
+#define AT803X_MODE_CFG_BASET_SGMII		0x01
+#define AT803X_MODE_CFG_BX1000_RGMII_50		0x02
+#define AT803X_MODE_CFG_BX1000_RGMII_75		0x03
+#define AT803X_MODE_CFG_BX1000_CONV_50		0x04
+#define AT803X_MODE_CFG_BX1000_CONV_75		0x05
+#define AT803X_MODE_CFG_FX100_RGMII_50		0x06
+#define AT803X_MODE_CFG_FX100_CONV_50		0x07
+#define AT803X_MODE_CFG_RGMII_AUTO_MDET		0x0B
+#define AT803X_MODE_CFG_FX100_RGMII_75		0x0E
+#define AT803X_MODE_CFG_FX100_CONV_75		0x0F
 
 #define AT803X_PSSR				0x11	/*PHY-Specific Status Register*/
 #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
@@ -283,6 +295,8 @@ struct at803x_priv {
 	u16 clk_25m_mask;
 	u8 smarteee_lpi_tw_1g;
 	u8 smarteee_lpi_tw_100m;
+	bool is_fiber;
+	bool is_1000basex;
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
 	struct regulator *vddio;
@@ -784,7 +798,33 @@ static int at803x_probe(struct phy_device *phydev)
 			return ret;
 	}
 
+	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
+		int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
+		int mode_cfg;
+
+		if (ccr < 0)
+			goto err;
+		mode_cfg = ccr & AT803X_MODE_CFG_MASK;
+
+		switch (mode_cfg) {
+		case AT803X_MODE_CFG_BX1000_RGMII_50:
+		case AT803X_MODE_CFG_BX1000_RGMII_75:
+			priv->is_1000basex = true;
+			fallthrough;
+		case AT803X_MODE_CFG_FX100_RGMII_50:
+		case AT803X_MODE_CFG_FX100_RGMII_75:
+			priv->is_fiber = true;
+			break;
+		}
+	}
+
 	return 0;
+
+err:
+	if (priv->vddio)
+		regulator_disable(priv->vddio);
+
+	return ret;
 }
 
 static void at803x_remove(struct phy_device *phydev)
@@ -797,6 +837,7 @@ static void at803x_remove(struct phy_device *phydev)
 
 static int at803x_get_features(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int err;
 
 	err = genphy_read_abilities(phydev);
@@ -823,12 +864,13 @@ static int at803x_get_features(struct phy_device *phydev)
 	 * As a result of that, ESTATUS_1000_XFULL is set
 	 * to 1 even when operating in copper TP mode.
 	 *
-	 * Remove this mode from the supported link modes,
-	 * as this driver currently only supports copper
-	 * operation.
+	 * Remove this mode from the supported link modes
+	 * when not operating in 1000BaseX mode.
 	 */
-	linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
-			   phydev->supported);
+	if (!priv->is_1000basex)
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+				   phydev->supported);
+
 	return 0;
 }
 
@@ -892,15 +934,18 @@ static int at8031_pll_config(struct phy_device *phydev)
 
 static int at803x_config_init(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int ret;
 
 	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
-	       /* Some bootloaders leave the fiber page selected.
-		* Switch to the copper page, as otherwise we read
-		* the PHY capabilities from the fiber side.
-		*/
+		/* Some bootloaders leave the fiber page selected.
+		 * Switch to the appropriate page (fiber or copper), as otherwise we
+		 * read the PHY capabilities from the wrong page.
+		 */
 		phy_lock_mdio_bus(phydev);
-		ret = at803x_write_page(phydev, AT803X_PAGE_COPPER);
+		ret = at803x_write_page(phydev,
+					priv->is_fiber ? AT803X_PAGE_FIBER :
+							 AT803X_PAGE_COPPER);
 		phy_unlock_mdio_bus(phydev);
 		if (ret)
 			return ret;
@@ -959,6 +1004,7 @@ static int at803x_ack_interrupt(struct phy_device *phydev)
 
 static int at803x_config_intr(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int err;
 	int value;
 
@@ -975,6 +1021,10 @@ static int at803x_config_intr(struct phy_device *phydev)
 		value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
 		value |= AT803X_INTR_ENABLE_LINK_FAIL;
 		value |= AT803X_INTR_ENABLE_LINK_SUCCESS;
+		if (priv->is_fiber) {
+			value |= AT803X_INTR_ENABLE_LINK_FAIL_BX;
+			value |= AT803X_INTR_ENABLE_LINK_SUCCESS_BX;
+		}
 
 		err = phy_write(phydev, AT803X_INTR_ENABLE, value);
 	} else {
@@ -1107,8 +1157,12 @@ static int at803x_read_specific_status(struct phy_device *phydev)
 
 static int at803x_read_status(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int err, old_link = phydev->link;
 
+	if (priv->is_1000basex)
+		return genphy_c37_read_status(phydev);
+
 	/* Update the link, but return if there was an error */
 	err = genphy_update_link(phydev);
 	if (err)
@@ -1162,6 +1216,7 @@ static int at803x_config_mdix(struct phy_device *phydev, u8 ctrl)
 
 static int at803x_config_aneg(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int ret;
 
 	ret = at803x_config_mdix(phydev, phydev->mdix_ctrl);
@@ -1178,6 +1233,9 @@ static int at803x_config_aneg(struct phy_device *phydev)
 			return ret;
 	}
 
+	if (priv->is_1000basex)
+		return genphy_c37_config_aneg(phydev);
+
 	/* Do not restart auto-negotiation by setting ret to 0 defautly,
 	 * when calling __genphy_config_aneg later.
 	 */
-- 
2.31.1

[PATCH net-next v2 3/3] net: phy: at803x: Support downstream SFP cage

[Robert Hancock]

Add support for downstream SFP cages for AR8031 and AR8033. This is
primarily intended for fiber modules or direct-attach cables, however
copper modules which work in 1000Base-X mode may also function. Such
modules are allowed with a warning.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/net/phy/at803x.c | 56 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 63d84eb2eddb..c4e87c76edcb 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -19,6 +19,8 @@
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/consumer.h>
+#include <linux/phylink.h>
+#include <linux/sfp.h>
 #include <dt-bindings/net/qca-ar803x.h>
 
 #define AT803X_SPECIFIC_FUNCTION_CONTROL	0x10
@@ -664,6 +666,55 @@ static int at8031_register_regulators(struct phy_device *phydev)
 	return 0;
 }
 
+static int at803x_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+{
+	struct phy_device *phydev = upstream;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_support);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
+	phy_interface_t iface;
+
+	linkmode_zero(phy_support);
+	phylink_set(phy_support, 1000baseX_Full);
+	phylink_set(phy_support, 1000baseT_Full);
+	phylink_set(phy_support, Autoneg);
+	phylink_set(phy_support, Pause);
+	phylink_set(phy_support, Asym_Pause);
+
+	linkmode_zero(sfp_support);
+	sfp_parse_support(phydev->sfp_bus, id, sfp_support);
+	/* Some modules support 10G modes as well as others we support.
+	 * Mask out non-supported modes so the correct interface is picked.
+	 */
+	linkmode_and(sfp_support, phy_support, sfp_support);
+
+	if (linkmode_empty(sfp_support)) {
+		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
+		return -EINVAL;
+	}
+
+	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
+
+	/* Only 1000Base-X is supported by AR8031/8033 as the downstream SerDes
+	 * interface for use with SFP modules.
+	 * However, some copper modules detected as having a preferred SGMII
+	 * interface do default to and function in 1000Base-X mode, so just
+	 * print a warning and allow such modules, as they may have some chance
+	 * of working.
+	 */
+	if (iface == PHY_INTERFACE_MODE_SGMII)
+		dev_warn(&phydev->mdio.dev, "module may not function if 1000Base-X not supported\n");
+	else if (iface != PHY_INTERFACE_MODE_1000BASEX)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct sfp_upstream_ops at803x_sfp_ops = {
+	.attach = phy_sfp_attach,
+	.detach = phy_sfp_detach,
+	.module_insert = at803x_sfp_insert,
+};
+
 static int at803x_parse_dt(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
@@ -771,6 +822,11 @@ static int at803x_parse_dt(struct phy_device *phydev)
 			phydev_err(phydev, "failed to get VDDIO regulator\n");
 			return PTR_ERR(priv->vddio);
 		}
+
+		/* Only AR8031/8033 support 1000Base-X for SFP modules */
+		ret = phy_sfp_probe(phydev, &at803x_sfp_ops);
+		if (ret < 0)
+			return ret;
 	}
 
 	return 0;
-- 
2.31.1

Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support

[Andrew Lunn]

>  #define AT803X_MODE_CFG_MASK			0x0F
> -#define AT803X_MODE_CFG_SGMII			0x01
> +#define AT803X_MODE_CFG_BASET_RGMII		0x00
> +#define AT803X_MODE_CFG_BASET_SGMII		0x01
> +#define AT803X_MODE_CFG_BX1000_RGMII_50		0x02
> +#define AT803X_MODE_CFG_BX1000_RGMII_75		0x03
> +#define AT803X_MODE_CFG_BX1000_CONV_50		0x04
> +#define AT803X_MODE_CFG_BX1000_CONV_75		0x05
> +#define AT803X_MODE_CFG_FX100_RGMII_50		0x06
> +#define AT803X_MODE_CFG_FX100_CONV_50		0x07
> +#define AT803X_MODE_CFG_RGMII_AUTO_MDET		0x0B
> +#define AT803X_MODE_CFG_FX100_RGMII_75		0x0E
> +#define AT803X_MODE_CFG_FX100_CONV_75		0x0F

Hi Robert

What do these _50, and _75 mean?

 
>  #define AT803X_PSSR				0x11	/*PHY-Specific Status Register*/
>  #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
> @@ -283,6 +295,8 @@ struct at803x_priv {
>  	u16 clk_25m_mask;
>  	u8 smarteee_lpi_tw_1g;
>  	u8 smarteee_lpi_tw_100m;
> +	bool is_fiber;

Is maybe is_100basefx a better name? It makes it clearer it represents
a link mode?

> +	bool is_1000basex;
>  	struct regulator_dev *vddio_rdev;
>  	struct regulator_dev *vddh_rdev;
>  	struct regulator *vddio;
> @@ -784,7 +798,33 @@ static int at803x_probe(struct phy_device *phydev)
>  			return ret;
>  	}
>  
> +	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> +		int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> +		int mode_cfg;
> +
> +		if (ccr < 0)
> +			goto err;
> +		mode_cfg = ccr & AT803X_MODE_CFG_MASK;
> +
> +		switch (mode_cfg) {
> +		case AT803X_MODE_CFG_BX1000_RGMII_50:
> +		case AT803X_MODE_CFG_BX1000_RGMII_75:
> +			priv->is_1000basex = true;
> +			fallthrough;
> +		case AT803X_MODE_CFG_FX100_RGMII_50:
> +		case AT803X_MODE_CFG_FX100_RGMII_75:
> +			priv->is_fiber = true;

O.K, now i'm wondering what AT803X_MODE_CFG_FX100_* actually means. I
was thinking it indicated 100BaseFX? But the fall through suggests
otherwise.

>  static int at803x_config_init(struct phy_device *phydev)
>  {
> +	struct at803x_priv *priv = phydev->priv;
>  	int ret;
>  
>  	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> -	       /* Some bootloaders leave the fiber page selected.
> -		* Switch to the copper page, as otherwise we read
> -		* the PHY capabilities from the fiber side.
> -		*/
> +		/* Some bootloaders leave the fiber page selected.

Looks like you have a tab vs space problem with the previous patch?
Otherwise this first line should not of changed.

	  Andrew

Re: [PATCH net-next v2 3/3] net: phy: at803x: Support downstream SFP cage

[Andrew Lunn]

On Tue, Jan 11, 2022 at 03:55:04PM -0600, Robert Hancock wrote:
> Add support for downstream SFP cages for AR8031 and AR8033. This is
> primarily intended for fiber modules or direct-attach cables, however
> copper modules which work in 1000Base-X mode may also function. Such
> modules are allowed with a warning.

The previous patch added:

AT803X_MODE_CFG_BASET_SGMII

So it seems it has some support for SGMII? Cannot it be used?

   Andrew

Re: [PATCH net-next v2 3/3] net: phy: at803x: Support downstream SFP cage

[Robert Hancock]

On Wed, 2022-01-12 at 01:14 +0100, Andrew Lunn wrote:
> On Tue, Jan 11, 2022 at 03:55:04PM -0600, Robert Hancock wrote:
> > Add support for downstream SFP cages for AR8031 and AR8033. This is
> > primarily intended for fiber modules or direct-attach cables, however
> > copper modules which work in 1000Base-X mode may also function. Such
> > modules are allowed with a warning.
> 
> The previous patch added:
> 
> AT803X_MODE_CFG_BASET_SGMII
> 
> So it seems it has some support for SGMII? Cannot it be used?

According to Qualcomm, the AR8031 PHY has one SERDES block which can either be
used in SGMII mode on the MAC side, or in 1000Base-X mode on the line side, but
not in SGMII mode on the line side, so only 1000Base-X mode can be used there.
So that means no SGMII support for SFP modules unfortunately.

In practice, it does seem to work with most of the copper modules we have
tried, though in some cases (like where the module defaults to SGMII mode, or
1000Base-X mode with auto-negotiation disabled) we need to disable auto-
negotiation on the interface to get the link to come up (the module still does
its own auto-negotiation on the copper side regardless). Of course without
SGMII, 100 or 10 Mbps speeds won't work.

> 
>    Andrew
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support

[Robert Hancock]

On Wed, 2022-01-12 at 01:10 +0100, Andrew Lunn wrote:
> >  #define AT803X_MODE_CFG_MASK			0x0F
> > -#define AT803X_MODE_CFG_SGMII			0x01
> > +#define AT803X_MODE_CFG_BASET_RGMII		0x00
> > +#define AT803X_MODE_CFG_BASET_SGMII		0x01
> > +#define AT803X_MODE_CFG_BX1000_RGMII_50		0x02
> > +#define AT803X_MODE_CFG_BX1000_RGMII_75		0x03
> > +#define AT803X_MODE_CFG_BX1000_CONV_50		0x04
> > +#define AT803X_MODE_CFG_BX1000_CONV_75		0x05
> > +#define AT803X_MODE_CFG_FX100_RGMII_50		0x06
> > +#define AT803X_MODE_CFG_FX100_CONV_50		0x07
> > +#define AT803X_MODE_CFG_RGMII_AUTO_MDET		0x0B
> > +#define AT803X_MODE_CFG_FX100_RGMII_75		0x0E
> > +#define AT803X_MODE_CFG_FX100_CONV_75		0x0F
> 
> Hi Robert
> 
> What do these _50, and _75 mean?

50 or 75 ohm impedance. Can refer to page 82 of the datasheet at 
https://www.digikey.ca/en/datasheets/qualcomm/qualcommar8031dsatherosrev10aug2011
 - these names were chosen to match what it uses.

> 
>  
> >  #define AT803X_PSSR				0x11	/*PHY-
> > Specific Status Register*/
> >  #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
> > @@ -283,6 +295,8 @@ struct at803x_priv {
> >  	u16 clk_25m_mask;
> >  	u8 smarteee_lpi_tw_1g;
> >  	u8 smarteee_lpi_tw_100m;
> > +	bool is_fiber;
> 
> Is maybe is_100basefx a better name? It makes it clearer it represents
> a link mode?

This is meant to indicate the chip is set for any fiber mode (100Base-FX or
1000Base-X).

> 
> > +	bool is_1000basex;
> >  	struct regulator_dev *vddio_rdev;
> >  	struct regulator_dev *vddh_rdev;
> >  	struct regulator *vddio;
> > @@ -784,7 +798,33 @@ static int at803x_probe(struct phy_device *phydev)
> >  			return ret;
> >  	}
> >  
> > +	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > +		int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> > +		int mode_cfg;
> > +
> > +		if (ccr < 0)
> > +			goto err;
> > +		mode_cfg = ccr & AT803X_MODE_CFG_MASK;
> > +
> > +		switch (mode_cfg) {
> > +		case AT803X_MODE_CFG_BX1000_RGMII_50:
> > +		case AT803X_MODE_CFG_BX1000_RGMII_75:
> > +			priv->is_1000basex = true;
> > +			fallthrough;
> > +		case AT803X_MODE_CFG_FX100_RGMII_50:
> > +		case AT803X_MODE_CFG_FX100_RGMII_75:
> > +			priv->is_fiber = true;
> 
> O.K, now i'm wondering what AT803X_MODE_CFG_FX100_* actually means. I
> was thinking it indicated 100BaseFX? But the fall through suggests
> otherwise.

is_1000basex is a subset of is_fiber here, so 1000Base-X sets both flags,
100Base-FX sets only is_fiber.

> 
> >  static int at803x_config_init(struct phy_device *phydev)
> >  {
> > +	struct at803x_priv *priv = phydev->priv;
> >  	int ret;
> >  
> >  	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > -	       /* Some bootloaders leave the fiber page selected.
> > -		* Switch to the copper page, as otherwise we read
> > -		* the PHY capabilities from the fiber side.
> > -		*/
> > +		/* Some bootloaders leave the fiber page selected.
> 
> Looks like you have a tab vs space problem with the previous patch?
> Otherwise this first line should not of changed.

Indeed, looks like patch 1 replaced some tabs with spaces when the code was
moved, and this patch removed them. Odd that checkpatch didn't pick that up.
Can fix that up in a new rev..

> 
> 	  Andrew
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH net-next v2 1/3] net: phy: at803x: move page selection fix to config_init

[Jakub Kicinski]

On Tue, 11 Jan 2022 15:55:02 -0600 Robert Hancock wrote:
> The fix to select the copper page on AR8031 was being done in the probe
> function rather than config_init, so it would not be redone after resume
> from suspend. Move this to config_init so it is always redone when
> needed.
> 
> Fixes: c329e5afb42f ("net: phy: at803x: select correct page on config init")

Please make sure to CC authors of patches in the fixes tags, they are
usually good reviewers.

Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support

[Andrew Lunn]

On Wed, Jan 12, 2022 at 12:42:00AM +0000, Robert Hancock wrote:
> On Wed, 2022-01-12 at 01:10 +0100, Andrew Lunn wrote:
> > >  #define AT803X_MODE_CFG_MASK			0x0F
> > > -#define AT803X_MODE_CFG_SGMII			0x01
> > > +#define AT803X_MODE_CFG_BASET_RGMII		0x00
> > > +#define AT803X_MODE_CFG_BASET_SGMII		0x01
> > > +#define AT803X_MODE_CFG_BX1000_RGMII_50		0x02
> > > +#define AT803X_MODE_CFG_BX1000_RGMII_75		0x03
> > > +#define AT803X_MODE_CFG_BX1000_CONV_50		0x04
> > > +#define AT803X_MODE_CFG_BX1000_CONV_75		0x05
> > > +#define AT803X_MODE_CFG_FX100_RGMII_50		0x06
> > > +#define AT803X_MODE_CFG_FX100_CONV_50		0x07
> > > +#define AT803X_MODE_CFG_RGMII_AUTO_MDET		0x0B
> > > +#define AT803X_MODE_CFG_FX100_RGMII_75		0x0E
> > > +#define AT803X_MODE_CFG_FX100_CONV_75		0x0F
> > 
> > Hi Robert
> > 
> > What do these _50, and _75 mean?
> 
> 50 or 75 ohm impedance. Can refer to page 82 of the datasheet at 
> https://www.digikey.ca/en/datasheets/qualcomm/qualcommar8031dsatherosrev10aug2011
>  - these names were chosen to match what it uses.

I know they are getting long, but maybe add OHM to the end?

> > >  #define AT803X_PSSR				0x11	/*PHY-
> > > Specific Status Register*/
> > >  #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
> > > @@ -283,6 +295,8 @@ struct at803x_priv {
> > >  	u16 clk_25m_mask;
> > >  	u8 smarteee_lpi_tw_1g;
> > >  	u8 smarteee_lpi_tw_100m;
> > > +	bool is_fiber;
> > 
> > Is maybe is_100basefx a better name? It makes it clearer it represents
> > a link mode?
> 
> This is meant to indicate the chip is set for any fiber mode (100Base-FX or
> 1000Base-X).

O.K, then is_fibre is O.K.

I noticed code removing the link mode 1000BaseX in the case of
is_fibre && !is_100basex. Does 100BaseFX need removing for !is_fibre?

	Andrew

Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support

[Robert Hancock]

On Wed, 2022-01-12 at 20:49 +0100, Andrew Lunn wrote:
> On Wed, Jan 12, 2022 at 12:42:00AM +0000, Robert Hancock wrote:
> > On Wed, 2022-01-12 at 01:10 +0100, Andrew Lunn wrote:
> > > >  #define AT803X_MODE_CFG_MASK			0x0F
> > > > -#define AT803X_MODE_CFG_SGMII			0x01
> > > > +#define AT803X_MODE_CFG_BASET_RGMII		0x00
> > > > +#define AT803X_MODE_CFG_BASET_SGMII		0x01
> > > > +#define AT803X_MODE_CFG_BX1000_RGMII_50		0x02
> > > > +#define AT803X_MODE_CFG_BX1000_RGMII_75		0x03
> > > > +#define AT803X_MODE_CFG_BX1000_CONV_50		0x04
> > > > +#define AT803X_MODE_CFG_BX1000_CONV_75		0x05
> > > > +#define AT803X_MODE_CFG_FX100_RGMII_50		0x06
> > > > +#define AT803X_MODE_CFG_FX100_CONV_50		0x07
> > > > +#define AT803X_MODE_CFG_RGMII_AUTO_MDET		0x0B
> > > > +#define AT803X_MODE_CFG_FX100_RGMII_75		0x0E
> > > > +#define AT803X_MODE_CFG_FX100_CONV_75		0x0F
> > > 
> > > Hi Robert
> > > 
> > > What do these _50, and _75 mean?
> > 
> > 50 or 75 ohm impedance. Can refer to page 82 of the datasheet at 
> > https://urldefense.com/v3/__https://www.digikey.ca/en/datasheets/qualcomm/qualcommar8031dsatherosrev10aug2011__;!!IOGos0k!xrZprIiCK5nnfhnmAdxtCxOHGIP9149yaVxZRjOIrvXZOnmeXDVFmRH8RB9Mv_rmqdI$ 
> >  - these names were chosen to match what it uses.
> 
> I know they are getting long, but maybe add OHM to the end?

Could probably do that, yeah..

> 
> > > >  #define AT803X_PSSR				0x11	/*PHY-
> > > > Specific Status Register*/
> > > >  #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
> > > > @@ -283,6 +295,8 @@ struct at803x_priv {
> > > >  	u16 clk_25m_mask;
> > > >  	u8 smarteee_lpi_tw_1g;
> > > >  	u8 smarteee_lpi_tw_100m;
> > > > +	bool is_fiber;
> > > 
> > > Is maybe is_100basefx a better name? It makes it clearer it represents
> > > a link mode?
> > 
> > This is meant to indicate the chip is set for any fiber mode (100Base-FX or
> > 1000Base-X).
> 
> O.K, then is_fibre is O.K.
> 
> I noticed code removing the link mode 1000BaseX in the case of
> is_fibre && !is_100basex. Does 100BaseFX need removing for !is_fibre?

That 1000Base-X link mode was coming from what genphy_read_abilities was
parsing out of the device's status registers. That function can't actually
detect 100Base-FX mode so it can't end up in there to need removing at this
point.

> 
> 	Andrew