diff mbox series

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

Message ID 20240705085550.86678-1-o.rempel@pengutronix.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2,1/1] net: phy: microchip: lan937x: add support for 100BaseTX PHY | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 816 this patch: 816
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 821 this patch: 821
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 821 this patch: 821
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 99 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-07-05--15-00 (tests: 694)

Commit Message

Oleksij Rempel July 5, 2024, 8:55 a.m. UTC 
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(+)

Comments

Arun Ramadoss July 5, 2024, 3:15 p.m. UTC | #1 
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
>
Oleksij Rempel July 5, 2024, 3:52 p.m. UTC | #2 
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
Russell King (Oracle) July 29, 2024, 8:14 a.m. UTC | #3 
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?
Oleksij Rempel Aug. 2, 2024, 8:02 a.m. UTC | #4 
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
diff mbox series

Patch

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) },
 	{ }
 };