diff mbox series

[net-next,2/2] net: phy: micrel: Add PHY Auto/MDI/MDI-X set driver for KSZ9131

Message ID 20221021055642.255413-3-Raju.Lakkaraju@microchip.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: lan743x: PCI11010 / PCI11414 devices Enhancements | 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 8 of 8 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/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 89 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Raju Lakkaraju - I30499 Oct. 21, 2022, 5:56 a.m. UTC 
Add support for MDI-X status and configuration for KSZ9131 chips

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

Comments

Horatiu Vultur Oct. 21, 2022, 7:27 a.m. UTC | #1 
The 10/21/2022 11:26, Raju Lakkaraju wrote:

Hi Raju,

> Add support for MDI-X status and configuration for KSZ9131 chips
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> ---
>  drivers/net/phy/micrel.c | 77 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 54a17b576eac..40aa52d442f8 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -1295,6 +1295,81 @@ static int ksz9131_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +#define MII_KSZPHY_AUTO_MDIX		0x1C
> +#define MII_KSZPHY_AUTO_MDI_SET_	BIT(7)
> +#define MII_KSZPHY_AUTO_MDIX_SWAP_OFF_	BIT(6)

Can you please drop the "_" from the end of the macros. The only
macros that have that suffix are those used by PTP.

Are these KSZPHY registers generic for all KSZ phys? Then the functions
'ksz9131_mdix_update' and 'ksz9131_config_mdix' shouldn't be rename to
something like 'kszphy_mdix_update' and 'kszphy_config_mdix'?
Otherwise shoudln't these register contain 9131 in their name?

I know that also lan8841 and lan8804 have the same layout for these
registers.

> +
> +static int ksz9131_mdix_update(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, MII_KSZPHY_AUTO_MDIX);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & MII_KSZPHY_AUTO_MDIX_SWAP_OFF_) {
> +		if (ret & MII_KSZPHY_AUTO_MDI_SET_)
> +			phydev->mdix_ctrl = ETH_TP_MDI;
> +		else
> +			phydev->mdix_ctrl = ETH_TP_MDI_X;
> +	} else {
> +		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> +	}
> +
> +	if (ret & MII_KSZPHY_AUTO_MDI_SET_)
> +		phydev->mdix = ETH_TP_MDI;
> +	else
> +		phydev->mdix = ETH_TP_MDI_X;
> +
> +	return 0;
> +}
> +
> +static int ksz9131_config_mdix(struct phy_device *phydev, u8 ctrl)
> +{
> +	u16 val;
> +
> +	switch (ctrl) {
> +	case ETH_TP_MDI:
> +		val = MII_KSZPHY_AUTO_MDIX_SWAP_OFF_ |
> +		      MII_KSZPHY_AUTO_MDI_SET_;
> +		break;
> +	case ETH_TP_MDI_X:
> +		val = MII_KSZPHY_AUTO_MDIX_SWAP_OFF_;
> +		break;
> +	case ETH_TP_MDI_AUTO:
> +		val = 0;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return phy_modify(phydev, MII_KSZPHY_AUTO_MDIX,
> +			  MII_KSZPHY_AUTO_MDIX_SWAP_OFF_ |
> +			  MII_KSZPHY_AUTO_MDI_SET_, val);
> +}
> +
> +static int ksz9131_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = ksz9131_mdix_update(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	return genphy_read_status(phydev);
> +}
> +
> +static int ksz9131_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_config_aneg(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return ksz9131_config_mdix(phydev, phydev->mdix_ctrl);
> +}
> +
>  #define KSZ8873MLL_GLOBAL_CONTROL_4	0x06
>  #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX	BIT(6)
>  #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED	BIT(4)
> @@ -3304,6 +3379,8 @@ static struct phy_driver ksphy_driver[] = {
>  	.probe		= kszphy_probe,
>  	.config_init	= ksz9131_config_init,
>  	.config_intr	= kszphy_config_intr,
> +	.config_aneg	= ksz9131_config_aneg,
> +	.read_status	= ksz9131_read_status,
>  	.handle_interrupt = kszphy_handle_interrupt,
>  	.get_sset_count = kszphy_get_sset_count,
>  	.get_strings	= kszphy_get_strings,
> -- 
> 2.25.1
>
Raju Lakkaraju - I30499 Oct. 21, 2022, 10:26 a.m. UTC | #2 
Hi Horatiu,

Thank you for review comments.

The 10/21/2022 09:27, Horatiu Vultur wrote:
> The 10/21/2022 11:26, Raju Lakkaraju wrote:
> 
> Hi Raju,
> 
> > Add support for MDI-X status and configuration for KSZ9131 chips
> > 
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> > ---
> >  drivers/net/phy/micrel.c | 77 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> > 
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index 54a17b576eac..40aa52d442f8 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -1295,6 +1295,81 @@ static int ksz9131_config_init(struct phy_device *phydev)
> >  	return 0;
> >  }
> >  
> > +#define MII_KSZPHY_AUTO_MDIX		0x1C
> > +#define MII_KSZPHY_AUTO_MDI_SET_	BIT(7)
> > +#define MII_KSZPHY_AUTO_MDIX_SWAP_OFF_	BIT(6)
> 
> Can you please drop the "_" from the end of the macros. The only
> macros that have that suffix are those used by PTP.

Ok. I will remove '_' 

> 
> Are these KSZPHY registers generic for all KSZ phys? Then the functions

No. 
MDI-X register address is different for KSZ PHYs (Micerl PHYs)
KSZ8081 PHY MDI-X configuration register is 0x1f
KSZ886X PHY MDI-X configuration register is 0x00
KSZ9131 PHY MDI-X configuration register is 0x1c
 
.

> 'ksz9131_mdix_update' and 'ksz9131_config_mdix' shouldn't be rename to
> something like 'kszphy_mdix_update' and 'kszphy_config_mdix'?

No.
ksz9131_mdix_* specific to KSZ 9131 PHYs

> Otherwise shoudln't these register contain 9131 in their name?

Ok. I will add '9131' label in micro definitionis

> 
> I know that also lan8841 and lan8804 have the same layout for these
> registers.

O.k.

> 
> > +
> > +static int ksz9131_mdix_update(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	ret = phy_read(phydev, MII_KSZPHY_AUTO_MDIX);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (ret & MII_KSZPHY_AUTO_MDIX_SWAP_OFF_) {
> > +		if (ret & MII_KSZPHY_AUTO_MDI_SET_)
> > +			phydev->mdix_ctrl = ETH_TP_MDI;
> > +		else
> > +			phydev->mdix_ctrl = ETH_TP_MDI_X;
> > +	} else {
> > +		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> > +	}
> > +
> > +	if (ret & MII_KSZPHY_AUTO_MDI_SET_)
> > +		phydev->mdix = ETH_TP_MDI;
> > +	else
> > +		phydev->mdix = ETH_TP_MDI_X;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ksz9131_config_mdix(struct phy_device *phydev, u8 ctrl)
> > +{
> > +	u16 val;
> > +
> > +	switch (ctrl) {
> > +	case ETH_TP_MDI:
> > +		val = MII_KSZPHY_AUTO_MDIX_SWAP_OFF_ |
> > +		      MII_KSZPHY_AUTO_MDI_SET_;
> > +		break;
> > +	case ETH_TP_MDI_X:
> > +		val = MII_KSZPHY_AUTO_MDIX_SWAP_OFF_;
> > +		break;
> > +	case ETH_TP_MDI_AUTO:
> > +		val = 0;
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +	return phy_modify(phydev, MII_KSZPHY_AUTO_MDIX,
> > +			  MII_KSZPHY_AUTO_MDIX_SWAP_OFF_ |
> > +			  MII_KSZPHY_AUTO_MDI_SET_, val);
> > +}
> > +
> > +static int ksz9131_read_status(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	ret = ksz9131_mdix_update(phydev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return genphy_read_status(phydev);
> > +}
> > +
> > +static int ksz9131_config_aneg(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	ret = genphy_config_aneg(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ksz9131_config_mdix(phydev, phydev->mdix_ctrl);
> > +}
> > +
> >  #define KSZ8873MLL_GLOBAL_CONTROL_4	0x06
> >  #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX	BIT(6)
> >  #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED	BIT(4)
> > @@ -3304,6 +3379,8 @@ static struct phy_driver ksphy_driver[] = {
> >  	.probe		= kszphy_probe,
> >  	.config_init	= ksz9131_config_init,
> >  	.config_intr	= kszphy_config_intr,
> > +	.config_aneg	= ksz9131_config_aneg,
> > +	.read_status	= ksz9131_read_status,
> >  	.handle_interrupt = kszphy_handle_interrupt,
> >  	.get_sset_count = kszphy_get_sset_count,
> >  	.get_strings	= kszphy_get_strings,
> > -- 
> > 2.25.1
> > 
> 
> -- 
> /Horatiu

--------
Thanks,
Raju
diff mbox series

Patch

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 54a17b576eac..40aa52d442f8 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1295,6 +1295,81 @@  static int ksz9131_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+#define MII_KSZPHY_AUTO_MDIX		0x1C
+#define MII_KSZPHY_AUTO_MDI_SET_	BIT(7)
+#define MII_KSZPHY_AUTO_MDIX_SWAP_OFF_	BIT(6)
+
+static int ksz9131_mdix_update(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_KSZPHY_AUTO_MDIX);
+	if (ret < 0)
+		return ret;
+
+	if (ret & MII_KSZPHY_AUTO_MDIX_SWAP_OFF_) {
+		if (ret & MII_KSZPHY_AUTO_MDI_SET_)
+			phydev->mdix_ctrl = ETH_TP_MDI;
+		else
+			phydev->mdix_ctrl = ETH_TP_MDI_X;
+	} else {
+		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+	}
+
+	if (ret & MII_KSZPHY_AUTO_MDI_SET_)
+		phydev->mdix = ETH_TP_MDI;
+	else
+		phydev->mdix = ETH_TP_MDI_X;
+
+	return 0;
+}
+
+static int ksz9131_config_mdix(struct phy_device *phydev, u8 ctrl)
+{
+	u16 val;
+
+	switch (ctrl) {
+	case ETH_TP_MDI:
+		val = MII_KSZPHY_AUTO_MDIX_SWAP_OFF_ |
+		      MII_KSZPHY_AUTO_MDI_SET_;
+		break;
+	case ETH_TP_MDI_X:
+		val = MII_KSZPHY_AUTO_MDIX_SWAP_OFF_;
+		break;
+	case ETH_TP_MDI_AUTO:
+		val = 0;
+		break;
+	default:
+		return 0;
+	}
+
+	return phy_modify(phydev, MII_KSZPHY_AUTO_MDIX,
+			  MII_KSZPHY_AUTO_MDIX_SWAP_OFF_ |
+			  MII_KSZPHY_AUTO_MDI_SET_, val);
+}
+
+static int ksz9131_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = ksz9131_mdix_update(phydev);
+	if (ret < 0)
+		return ret;
+
+	return genphy_read_status(phydev);
+}
+
+static int ksz9131_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	return ksz9131_config_mdix(phydev, phydev->mdix_ctrl);
+}
+
 #define KSZ8873MLL_GLOBAL_CONTROL_4	0x06
 #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX	BIT(6)
 #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED	BIT(4)
@@ -3304,6 +3379,8 @@  static struct phy_driver ksphy_driver[] = {
 	.probe		= kszphy_probe,
 	.config_init	= ksz9131_config_init,
 	.config_intr	= kszphy_config_intr,
+	.config_aneg	= ksz9131_config_aneg,
+	.read_status	= ksz9131_read_status,
 	.handle_interrupt = kszphy_handle_interrupt,
 	.get_sset_count = kszphy_get_sset_count,
 	.get_strings	= kszphy_get_strings,