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