[PATCH net-next v2 1/1] net: phy: microchip: lan937x: add support for 100BaseTX PHY

[PATCH net-next v2 1/1] net: phy: microchip: lan937x: add support for 100BaseTX PHY

[Oleksij Rempel]

Add support of 100BaseTX PHY build in to LAN9371 and LAN9372 switches.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
---
changes v2:
- move LAN937X_TX code from microchip_t1.c to microchip.c
- add Reviewed-by tags
---
 drivers/net/phy/microchip.c | 75 +++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
index 0b88635f4fbca..b46d5d43e2585 100644
--- a/drivers/net/phy/microchip.c
+++ b/drivers/net/phy/microchip.c
@@ -12,6 +12,12 @@
 #include <linux/of.h>
 #include <dt-bindings/net/microchip-lan78xx.h>
 
+#define PHY_ID_LAN937X_TX			0x0007c190
+
+#define LAN937X_MODE_CTRL_STATUS_REG		0x11
+#define LAN937X_AUTOMDIX_EN			BIT(7)
+#define LAN937X_MDI_MODE			BIT(6)
+
 #define DRIVER_AUTHOR	"WOOJUNG HUH <woojung.huh@microchip.com>"
 #define DRIVER_DESC	"Microchip LAN88XX PHY driver"
 
@@ -373,6 +379,66 @@ static void lan88xx_link_change_notify(struct phy_device *phydev)
 	}
 }
 
+static int lan937x_tx_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_read_status(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_read(phydev, LAN937X_MODE_CTRL_STATUS_REG);
+	if (ret < 0)
+		return ret;
+
+	if (ret & LAN937X_AUTOMDIX_EN) {
+		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+		/* MDI/MDIX status is unknown */
+		phydev->mdix = ETH_TP_MDI_INVALID;
+	} else if (ret & LAN937X_MDI_MODE) {
+		phydev->mdix_ctrl = ETH_TP_MDI_X;
+		phydev->mdix = ETH_TP_MDI_X;
+	} else {
+		phydev->mdix_ctrl = ETH_TP_MDI;
+		phydev->mdix = ETH_TP_MDI;
+	}
+
+	return 0;
+}
+
+static int lan937x_tx_config_mdix(struct phy_device *phydev, u8 ctrl)
+{
+	u16 val;
+
+	switch (ctrl) {
+	case ETH_TP_MDI:
+		val = 0;
+		break;
+	case ETH_TP_MDI_X:
+		val = LAN937X_MDI_MODE;
+		break;
+	case ETH_TP_MDI_AUTO:
+		val = LAN937X_AUTOMDIX_EN;
+		break;
+	default:
+		return 0;
+	}
+
+	return phy_modify(phydev, LAN937X_MODE_CTRL_STATUS_REG,
+			  LAN937X_AUTOMDIX_EN | LAN937X_MDI_MODE, val);
+}
+
+static int lan937x_tx_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	return lan937x_tx_config_mdix(phydev, phydev->mdix_ctrl);
+}
+
 static struct phy_driver microchip_phy_driver[] = {
 {
 	.phy_id		= 0x0007c132,
@@ -400,12 +466,21 @@ static struct phy_driver microchip_phy_driver[] = {
 	.set_wol	= lan88xx_set_wol,
 	.read_page	= lan88xx_read_page,
 	.write_page	= lan88xx_write_page,
+},
+{
+	PHY_ID_MATCH_MODEL(PHY_ID_LAN937X_TX),
+	.name		= "Microchip LAN937x TX",
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
+	.config_aneg	= lan937x_tx_config_aneg,
+	.read_status	= lan937x_tx_read_status,
 } };
 
 module_phy_driver(microchip_phy_driver);
 
 static struct mdio_device_id __maybe_unused microchip_tbl[] = {
 	{ 0x0007c132, 0xfffffff2 },
+	{ PHY_ID_MATCH_MODEL(PHY_ID_LAN937X_TX) },
 	{ }
 };
 
-- 
2.39.2

Re: [PATCH net-next v2 1/1] net: phy: microchip: lan937x: add support for 100BaseTX PHY

[Arun.Ramadoss]

Hi Oleksij,
On Fri, 2024-07-05 at 10:55 +0200, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Add support of 100BaseTX PHY build in to LAN9371 and LAN9372
> switches.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
> ---
> changes v2:
> - move LAN937X_TX code from microchip_t1.c to microchip.c
> - add Reviewed-by tags
> ---
>  drivers/net/phy/microchip.c | 75
> +++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/drivers/net/phy/microchip.c
> b/drivers/net/phy/microchip.c
> index 0b88635f4fbca..b46d5d43e2585 100644
> --- a/drivers/net/phy/microchip.c
> +++ b/drivers/net/phy/microchip.c
> @@ -12,6 +12,12 @@
>  #include <linux/of.h>
>  #include <dt-bindings/net/microchip-lan78xx.h>
> 
> +#define PHY_ID_LAN937X_TX                      0x0007c190

0x0007c190 -> 0x0007C190

> +
> +#define LAN937X_MODE_CTRL_STATUS_REG           0x11
> +#define LAN937X_AUTOMDIX_EN                    BIT(7)
> +#define LAN937X_MDI_MODE                       BIT(6)
> +
>  #define DRIVER_AUTHOR  "WOOJUNG HUH <woojung.huh@microchip.com>"
>  #define DRIVER_DESC    "Microchip LAN88XX PHY driver"

nitpick:
It can be updated to include "Microchip LAN88XX/LAN937X TX PHY driver"
> 
> @@ -373,6 +379,66 @@ static void lan88xx_link_change_notify(struct
> phy_device *phydev)
>         }
>  }
> 

Adding function description will be good.

> +static int lan937x_tx_config_mdix(struct phy_device *phydev, u8
> ctrl)
> +{
> +       u16 val;
> +
> +       switch (ctrl) {
> +       case ETH_TP_MDI:
> +               val = 0;
> +               break;
> +       case ETH_TP_MDI_X:
> +               val = LAN937X_MDI_MODE;
> +               break;
> +       case ETH_TP_MDI_AUTO:
> +               val = LAN937X_AUTOMDIX_EN;
> +               break;
> +       default:
> +               return 0;
> +       }
> +
> +       return phy_modify(phydev, LAN937X_MODE_CTRL_STATUS_REG,
> +                         LAN937X_AUTOMDIX_EN | LAN937X_MDI_MODE,
> val);
> +}
> +
> +static int lan937x_tx_config_aneg(struct phy_device *phydev)
> +{
> +       int ret;
> +
> +       ret = genphy_config_aneg(phydev);
> +       if (ret)

Is this if( ret < 0) ?

> +               return ret;
> +
> +       return lan937x_tx_config_mdix(phydev, phydev->mdix_ctrl);

why we need to pass argument phydev->mdix_ctrl, since already phydev is
passed. Also IMO, this two function can be combined together if
lan937x_tx_config_mdix is not used by other functions. 

> +}
> +
>  static struct phy_driver microchip_phy_driver[] = {
>  {
>         .phy_id         = 0x0007c132,
> @@ -400,12 +466,21 @@ static struct phy_driver microchip_phy_driver[]
> = {
>         .set_wol        = lan88xx_set_wol,
>         .read_page      = lan88xx_read_page,
>         .write_page     = lan88xx_write_page,
> +},
> +{
> +       PHY_ID_MATCH_MODEL(PHY_ID_LAN937X_TX),
> +       .name           = "Microchip LAN937x TX",
> +       .suspend        = genphy_suspend,
> +       .resume         = genphy_resume,
> +       .config_aneg    = lan937x_tx_config_aneg,
> +       .read_status    = lan937x_tx_read_status,

Do we need to add genphy_suspend/resume, .features?
>  } };
> 
>  module_phy_driver(microchip_phy_driver);
> 
>  static struct mdio_device_id __maybe_unused microchip_tbl[] = {
>         { 0x0007c132, 0xfffffff2 },
> +       { PHY_ID_MATCH_MODEL(PHY_ID_LAN937X_TX) },
>         { }
>  };
> 
> --
> 2.39.2
> 

Re: [PATCH net-next v2 1/1] net: phy: microchip: lan937x: add support for 100BaseTX PHY

[Oleksij Rempel]

On Fri, Jul 05, 2024 at 03:15:36PM +0000, Arun.Ramadoss@microchip.com wrote:
> Hi Oleksij,
> On Fri, 2024-07-05 at 10:55 +0200, Oleksij Rempel wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > Add support of 100BaseTX PHY build in to LAN9371 and LAN9372
> > switches.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
> > ---
> > changes v2:
> > - move LAN937X_TX code from microchip_t1.c to microchip.c
> > - add Reviewed-by tags
> > ---
> >  drivers/net/phy/microchip.c | 75
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> > 
> > diff --git a/drivers/net/phy/microchip.c
> > b/drivers/net/phy/microchip.c
> > index 0b88635f4fbca..b46d5d43e2585 100644
> > --- a/drivers/net/phy/microchip.c
> > +++ b/drivers/net/phy/microchip.c
> > @@ -12,6 +12,12 @@
> >  #include <linux/of.h>
> >  #include <dt-bindings/net/microchip-lan78xx.h>
> > 
> > +#define PHY_ID_LAN937X_TX                      0x0007c190
> 
> 0x0007c190 -> 0x0007C190

Why? 

I wrote a python script to gather stats in the drivers/net/phy:

Uppercase hex digits count:
E: 83
F: 216
C: 130
A: 148
B: 65
D: 74

Lowercase hex digits count:
b: 218
a: 337
d: 190
e: 238
f: 2560
c: 368

Sum of uppercase A-F: 716
Sum of lowercase a-f: 3911

> > +#define LAN937X_MODE_CTRL_STATUS_REG           0x11
> > +#define LAN937X_AUTOMDIX_EN                    BIT(7)
> > +#define LAN937X_MDI_MODE                       BIT(6)
> > +
> >  #define DRIVER_AUTHOR  "WOOJUNG HUH <woojung.huh@microchip.com>"
> >  #define DRIVER_DESC    "Microchip LAN88XX PHY driver"
> 
> nitpick:
> It can be updated to include "Microchip LAN88XX/LAN937X TX PHY driver"

ack

> > @@ -373,6 +379,66 @@ static void lan88xx_link_change_notify(struct
> > phy_device *phydev)
> >         }
> >  }
> > 
> 
> Adding function description will be good.

ack

> > +static int lan937x_tx_config_mdix(struct phy_device *phydev, u8
> > ctrl)
> > +{
> > +       u16 val;
> > +
> > +       switch (ctrl) {
> > +       case ETH_TP_MDI:
> > +               val = 0;
> > +               break;
> > +       case ETH_TP_MDI_X:
> > +               val = LAN937X_MDI_MODE;
> > +               break;
> > +       case ETH_TP_MDI_AUTO:
> > +               val = LAN937X_AUTOMDIX_EN;
> > +               break;
> > +       default:
> > +               return 0;
> > +       }
> > +
> > +       return phy_modify(phydev, LAN937X_MODE_CTRL_STATUS_REG,
> > +                         LAN937X_AUTOMDIX_EN | LAN937X_MDI_MODE,
> > val);
> > +}
> > +
> > +static int lan937x_tx_config_aneg(struct phy_device *phydev)
> > +{
> > +       int ret;
> > +
> > +       ret = genphy_config_aneg(phydev);
> > +       if (ret)
> 
> Is this if( ret < 0) ?

ack

> > +               return ret;
> > +
> > +       return lan937x_tx_config_mdix(phydev, phydev->mdix_ctrl);
> 
> why we need to pass argument phydev->mdix_ctrl, since already phydev is
> passed.

good point.

> Also IMO, this two function can be combined together if
> lan937x_tx_config_mdix is not used by other functions. 

I disagree here.

> > +{
> > +       PHY_ID_MATCH_MODEL(PHY_ID_LAN937X_TX),
> > +       .name           = "Microchip LAN937x TX",
> > +       .suspend        = genphy_suspend,
> > +       .resume         = genphy_resume,
> > +       .config_aneg    = lan937x_tx_config_aneg,
> > +       .read_status    = lan937x_tx_read_status,
> 
> Do we need to add genphy_suspend/resume, .features?

From PHY driver perspective - yes, otherwise to suspend or resume will be
called.
From internal PHY perspective - i do not know. Will the MAC disable PHY
automatically?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v2 1/1] net: phy: microchip: lan937x: add support for 100BaseTX PHY

[Russell King (Oracle)]

On Fri, Jul 05, 2024 at 10:55:50AM +0200, Oleksij Rempel wrote:
> +static int lan937x_tx_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_config_aneg(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return lan937x_tx_config_mdix(phydev, phydev->mdix_ctrl);

Should the MDIX configuration be changed _after_ aneg has possibly
been restarted (by genphy_config_aneg()) or should the MDIX config
be done before?

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

Re: [PATCH net-next v2 1/1] net: phy: microchip: lan937x: add support for 100BaseTX PHY

[Oleksij Rempel]

On Mon, Jul 29, 2024 at 09:14:47AM +0100, Russell King (Oracle) wrote:
> On Fri, Jul 05, 2024 at 10:55:50AM +0200, Oleksij Rempel wrote:
> > +static int lan937x_tx_config_aneg(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	ret = genphy_config_aneg(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return lan937x_tx_config_mdix(phydev, phydev->mdix_ctrl);
> 
> Should the MDIX configuration be changed _after_ aneg has possibly
> been restarted (by genphy_config_aneg()) or should the MDIX config
> be done before?

Hm, good question. I'm sure it was working, but now i'm starting to
doubt. I'll retest it as soon I'll continue to work on this HW.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |