diff mbox series

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

Message ID 20220111215504.2714643-3-robert.hancock@calian.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series at803x fiber/SFP support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 84 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Robert Hancock Jan. 11, 2022, 9:55 p.m. UTC 
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(-)

Comments

Andrew Lunn Jan. 12, 2022, 12:10 a.m. UTC | #1 
>  #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
Robert Hancock Jan. 12, 2022, 12:42 a.m. UTC | #2 
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
Andrew Lunn Jan. 12, 2022, 7:49 p.m. UTC | #3 
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
Robert Hancock Jan. 12, 2022, 11:56 p.m. UTC | #4 
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
diff mbox series

Patch

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.
 	 */