diff mbox series

[net-next] net: phy: mxl-gpy: Add PHY Auto/MDI/MDI-X set driver for GPY211 chips

Message ID 20221021100305.6576-1-Raju.Lakkaraju@microchip.com
State New
Headers
Series [net-next] net: phy: mxl-gpy: Add PHY Auto/MDI/MDI-X set driver for GPY211 chips |

Commit Message

Raju Lakkaraju Oct. 21, 2022, 10:03 a.m. UTC 
  Add support for MDI-X status and configuration for GPY211 chips

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
 drivers/net/phy/mxl-gpy.c | 79 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

Comments

Russell King (Oracle) Oct. 21, 2022, 11:40 a.m. UTC | #1 
Hi,

On Fri, Oct 21, 2022 at 03:33:05PM +0530, Raju Lakkaraju wrote:
> @@ -370,6 +415,38 @@ static int gpy_config_aneg(struct phy_device *phydev)
>  			      VSPEC1_SGMII_CTRL_ANRS, VSPEC1_SGMII_CTRL_ANRS);
>  }
>  
> +static void gpy_update_mdix(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, PHY_CTL1);
> +	if (ret < 0) {
> +		phydev_err(phydev, "Error: MDIO register access failed: %d\n",
> +			   ret);
> +		return;
> +	}
> +
> +	if (ret & PHY_CTL1_AMDIX)
> +		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> +	else
> +		if (ret & PHY_CTL1_MDICD || ret & PHY_CTL1_MDIAB)
> +			phydev->mdix_ctrl = ETH_TP_MDI_X;
> +		else
> +			phydev->mdix_ctrl = ETH_TP_MDI;

I think this would be better formatted as:

	if (...)
		...
	else if (...)
		...
	else
		...

We don't indent unless there's braces, and if there's braces, coding
style advises braces on both sides of the "else". So, much better to
use the formatting I suggest above.

Apart from that, nothing stands out as being wrong in this patch.

Thanks.
Andrew Lunn Oct. 21, 2022, 2:01 p.m. UTC | #2 
> +static void gpy_update_mdix(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, PHY_CTL1);
> +	if (ret < 0) {
> +		phydev_err(phydev, "Error: MDIO register access failed: %d\n",
> +			   ret);
> +		return;
> +	}

> @@ -413,6 +490,8 @@ static void gpy_update_interface(struct phy_device *phydev)
>  
>  	if (phydev->speed == SPEED_2500 || phydev->speed == SPEED_1000)
>  		genphy_read_master_slave(phydev);
> +
> +	gpy_update_mdix(phydev);

Do you know why gpy_update_interface() is a void function? It is
called from gpy_read_status() which does return error codes. And it
seems like gpy_read_status() would benefit from returning -EINVAL, etc.

      Andrew
Raju Lakkaraju Oct. 24, 2022, 7:24 a.m. UTC | #3 
Hi Andrew,

Thank you for review comments.

The 10/21/2022 16:01, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > +static void gpy_update_mdix(struct phy_device *phydev)
> > +{
> > +     int ret;
> > +
> > +     ret = phy_read(phydev, PHY_CTL1);
> > +     if (ret < 0) {
> > +             phydev_err(phydev, "Error: MDIO register access failed: %d\n",
> > +                        ret);
> > +             return;
> > +     }
> 
> > @@ -413,6 +490,8 @@ static void gpy_update_interface(struct phy_device *phydev)
> >
> >       if (phydev->speed == SPEED_2500 || phydev->speed == SPEED_1000)
> >               genphy_read_master_slave(phydev);
> > +
> > +     gpy_update_mdix(phydev);
> 
> Do you know why gpy_update_interface() is a void function? It is
> called from gpy_read_status() which does return error codes. And it
> seems like gpy_read_status() would benefit from returning -EINVAL, etc.

Do you want me to change gpy_update_interface() return type ?
Can I do those changes as part of this commit or need to fix on "net"
branch ?

> 
>       Andrew
Andrew Lunn Oct. 24, 2022, 11:54 a.m. UTC | #4 
> > Do you know why gpy_update_interface() is a void function? It is
> > called from gpy_read_status() which does return error codes. And it
> > seems like gpy_read_status() would benefit from returning -EINVAL, etc.
> 
> Do you want me to change gpy_update_interface() return type ?
> Can I do those changes as part of this commit or need to fix on "net"
> branch ?

I would say it is not a fix, so do it on net-next. But do it as a
separate patch please.

	 Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 24bae27eedef..a7b11a86bef5 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -29,6 +29,10 @@ 
 #define PHY_ID_GPY241BM		0x67C9DE80
 #define PHY_ID_GPY245B		0x67C9DEC0
 
+#define PHY_CTL1		0x13
+#define PHY_CTL1_MDICD		BIT(3)
+#define PHY_CTL1_MDIAB		BIT(2)
+#define PHY_CTL1_AMDIX		BIT(0)
 #define PHY_MIISTAT		0x18	/* MII state */
 #define PHY_IMASK		0x19	/* interrupt mask */
 #define PHY_ISTAT		0x1A	/* interrupt status */
@@ -59,6 +63,13 @@ 
 #define PHY_FWV_MAJOR_MASK	GENMASK(11, 8)
 #define PHY_FWV_MINOR_MASK	GENMASK(7, 0)
 
+#define PHY_PMA_MGBT_POLARITY	0x82
+#define PHY_MDI_MDI_X_MASK	GENMASK(1, 0)
+#define PHY_MDI_MDI_X_NORMAL	0x3
+#define PHY_MDI_MDI_X_AB	0x2
+#define PHY_MDI_MDI_X_CD	0x1
+#define PHY_MDI_MDI_X_CROSS	0x0
+
 /* SGMII */
 #define VSPEC1_SGMII_CTRL	0x08
 #define VSPEC1_SGMII_CTRL_ANEN	BIT(12)		/* Aneg enable */
@@ -289,6 +300,36 @@  static bool gpy_sgmii_aneg_en(struct phy_device *phydev)
 	return (ret & VSPEC1_SGMII_CTRL_ANEN) ? true : false;
 }
 
+static int gpy_config_mdix(struct phy_device *phydev, u8 ctrl)
+{
+	int ret;
+	u16 val;
+
+	switch (ctrl) {
+	case ETH_TP_MDI_AUTO:
+		val = PHY_CTL1_AMDIX;
+		break;
+	case ETH_TP_MDI_X:
+		val = (PHY_CTL1_MDIAB | PHY_CTL1_MDICD);
+		break;
+	case ETH_TP_MDI:
+		val = 0;
+		break;
+	default:
+		return 0;
+	}
+
+	ret =  phy_modify(phydev, PHY_CTL1, PHY_CTL1_AMDIX | PHY_CTL1_MDIAB |
+			  PHY_CTL1_MDICD, val);
+	if (ret < 0) {
+		phydev_err(phydev, "Error: MMD register access failed: %d\n",
+			   ret);
+		return ret;
+	}
+
+	return genphy_c45_restart_aneg(phydev);
+}
+
 static int gpy_config_aneg(struct phy_device *phydev)
 {
 	bool changed = false;
@@ -304,6 +345,10 @@  static int gpy_config_aneg(struct phy_device *phydev)
 			: genphy_c45_pma_setup_forced(phydev);
 	}
 
+	ret = gpy_config_mdix(phydev,  phydev->mdix_ctrl);
+	if (ret < 0)
+		return ret;
+
 	ret = genphy_c45_an_config_aneg(phydev);
 	if (ret < 0)
 		return ret;
@@ -370,6 +415,38 @@  static int gpy_config_aneg(struct phy_device *phydev)
 			      VSPEC1_SGMII_CTRL_ANRS, VSPEC1_SGMII_CTRL_ANRS);
 }
 
+static void gpy_update_mdix(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, PHY_CTL1);
+	if (ret < 0) {
+		phydev_err(phydev, "Error: MDIO register access failed: %d\n",
+			   ret);
+		return;
+	}
+
+	if (ret & PHY_CTL1_AMDIX)
+		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+	else
+		if (ret & PHY_CTL1_MDICD || ret & PHY_CTL1_MDIAB)
+			phydev->mdix_ctrl = ETH_TP_MDI_X;
+		else
+			phydev->mdix_ctrl = ETH_TP_MDI;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, PHY_PMA_MGBT_POLARITY);
+	if (ret < 0) {
+		phydev_err(phydev, "Error: MMD register access failed: %d\n",
+			   ret);
+		return;
+	}
+
+	if ((ret & PHY_MDI_MDI_X_MASK) < PHY_MDI_MDI_X_NORMAL)
+		phydev->mdix = ETH_TP_MDI_X;
+	else
+		phydev->mdix = ETH_TP_MDI;
+}
+
 static void gpy_update_interface(struct phy_device *phydev)
 {
 	int ret;
@@ -413,6 +490,8 @@  static void gpy_update_interface(struct phy_device *phydev)
 
 	if (phydev->speed == SPEED_2500 || phydev->speed == SPEED_1000)
 		genphy_read_master_slave(phydev);
+
+	gpy_update_mdix(phydev);
 }
 
 static int gpy_read_status(struct phy_device *phydev)