[PATCH net-next 0/2] net: lan743x: PCI11010 / PCI11414 devices Enhancements

[PATCH net-next 0/2] net: lan743x: PCI11010 / PCI11414 devices Enhancements

[Raju Lakkaraju]

This patch series continues with the addition of supported features for the 
Ethernet function of the PCI11010 / PCI11414 devices to the LAN743x driver.

Raju Lakkaraju (2):
  net: lan743x: Add support for get_pauseparam and set_pauseparam
  net: phy: micrel: Add PHY Auto/MDI/MDI-X set driver for KSZ9131

 .../net/ethernet/microchip/lan743x_ethtool.c  | 46 +++++++++++
 drivers/net/ethernet/microchip/lan743x_main.c |  4 +-
 drivers/net/ethernet/microchip/lan743x_main.h |  2 +
 drivers/net/phy/micrel.c                      | 77 +++++++++++++++++++
 4 files changed, 127 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH net-next 1/2] net: lan743x: Add support for get_pauseparam and set_pauseparam

[Raju Lakkaraju]

Add pause get and set functions

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
 .../net/ethernet/microchip/lan743x_ethtool.c  | 46 +++++++++++++++++++
 drivers/net/ethernet/microchip/lan743x_main.c |  4 +-
 drivers/net/ethernet/microchip/lan743x_main.h |  2 +
 3 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index c739d60ee17d..44c98715eb17 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1233,6 +1233,50 @@ static void lan743x_get_regs(struct net_device *dev,
 	lan743x_common_regs(dev, regs, p);
 }
 
+static void lan743x_get_pauseparam(struct net_device *dev,
+				   struct ethtool_pauseparam *pause)
+{
+	struct lan743x_adapter *adapter = netdev_priv(dev);
+	struct lan743x_phy *phy = &adapter->phy;
+
+	if (phy->fc_request_control & FLOW_CTRL_TX)
+		pause->tx_pause = 1;
+	if (phy->fc_request_control & FLOW_CTRL_RX)
+		pause->rx_pause = 1;
+	pause->autoneg = phy->fc_autoneg;
+}
+
+static int lan743x_set_pauseparam(struct net_device *dev,
+				  struct ethtool_pauseparam *pause)
+{
+	struct lan743x_adapter *adapter = netdev_priv(dev);
+	struct phy_device *phydev = dev->phydev;
+	struct lan743x_phy *phy = &adapter->phy;
+
+	if (!phydev)
+		return -ENODEV;
+
+	if (!phy_validate_pause(phydev, pause))
+		return -EINVAL;
+
+	phy->fc_request_control = 0;
+	if (pause->rx_pause)
+		phy->fc_request_control |= FLOW_CTRL_RX;
+
+	if (pause->tx_pause)
+		phy->fc_request_control |= FLOW_CTRL_TX;
+
+	phy->fc_autoneg = pause->autoneg;
+
+	phy_set_asym_pause(phydev, pause->rx_pause,  pause->tx_pause);
+
+	if (pause->autoneg == AUTONEG_DISABLE)
+		lan743x_mac_flow_ctrl_set_enables(adapter, pause->tx_pause,
+						  pause->rx_pause);
+
+	return 0;
+}
+
 const struct ethtool_ops lan743x_ethtool_ops = {
 	.get_drvinfo = lan743x_ethtool_get_drvinfo,
 	.get_msglevel = lan743x_ethtool_get_msglevel,
@@ -1259,6 +1303,8 @@ const struct ethtool_ops lan743x_ethtool_ops = {
 	.set_link_ksettings = phy_ethtool_set_link_ksettings,
 	.get_regs_len = lan743x_get_regs_len,
 	.get_regs = lan743x_get_regs,
+	.get_pauseparam = lan743x_get_pauseparam,
+	.set_pauseparam = lan743x_set_pauseparam,
 #ifdef CONFIG_PM
 	.get_wol = lan743x_ethtool_get_wol,
 	.set_wol = lan743x_ethtool_set_wol,
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 50eeecba1f18..c0f8ba601c01 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1326,8 +1326,8 @@ static void lan743x_mac_close(struct lan743x_adapter *adapter)
 				 1, 1000, 20000, 100);
 }
 
-static void lan743x_mac_flow_ctrl_set_enables(struct lan743x_adapter *adapter,
-					      bool tx_enable, bool rx_enable)
+void lan743x_mac_flow_ctrl_set_enables(struct lan743x_adapter *adapter,
+				       bool tx_enable, bool rx_enable)
 {
 	u32 flow_setting = 0;
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 67877d3b6dd9..bc5eea4c7b40 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -1159,5 +1159,7 @@ u32 lan743x_csr_read(struct lan743x_adapter *adapter, int offset);
 void lan743x_csr_write(struct lan743x_adapter *adapter, int offset, u32 data);
 int lan743x_hs_syslock_acquire(struct lan743x_adapter *adapter, u16 timeout);
 void lan743x_hs_syslock_release(struct lan743x_adapter *adapter);
+void lan743x_mac_flow_ctrl_set_enables(struct lan743x_adapter *adapter,
+				       bool tx_enable, bool rx_enable);
 
 #endif /* _LAN743X_H */
-- 
2.25.1

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

[Raju Lakkaraju]

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)
+
+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

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

[Horatiu Vultur]

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
> 

-- 
/Horatiu

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

[Raju Lakkaraju]

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

Re: [PATCH net-next 1/2] net: lan743x: Add support for get_pauseparam and set_pauseparam

[Andrew Lunn]

> +static int lan743x_set_pauseparam(struct net_device *dev,
> +				  struct ethtool_pauseparam *pause)
> +{
> +	struct lan743x_adapter *adapter = netdev_priv(dev);
> +	struct phy_device *phydev = dev->phydev;
> +	struct lan743x_phy *phy = &adapter->phy;
> +
> +	if (!phydev)
> +		return -ENODEV;
> +
> +	if (!phy_validate_pause(phydev, pause))
> +		return -EINVAL;
> +
> +	phy->fc_request_control = 0;
> +	if (pause->rx_pause)
> +		phy->fc_request_control |= FLOW_CTRL_RX;
> +
> +	if (pause->tx_pause)
> +		phy->fc_request_control |= FLOW_CTRL_TX;
> +
> +	phy->fc_autoneg = pause->autoneg;
> +
> +	phy_set_asym_pause(phydev, pause->rx_pause,  pause->tx_pause);
> +
> +	if (pause->autoneg == AUTONEG_DISABLE)
> +		lan743x_mac_flow_ctrl_set_enables(adapter, pause->tx_pause,
> +						  pause->rx_pause);

pause is not too well defined. But i think phy_set_asym_pause() should
be in an else clause. If pause autoneg is off, you directly set it in
the MAC and ignore what is negotiated. If it is enabled, you
negotiate. As far as i understand, you don't modify your negotiation
when pause autoneg is off.

	Andrew

Re: [PATCH net-next 1/2] net: lan743x: Add support for get_pauseparam and set_pauseparam

[Raju Lakkaraju]

Hi Andrew,

Thank you for review comments.

The 10/21/2022 15:46, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > +static int lan743x_set_pauseparam(struct net_device *dev,
> > +                               struct ethtool_pauseparam *pause)
> > +{
> > +     struct lan743x_adapter *adapter = netdev_priv(dev);
> > +     struct phy_device *phydev = dev->phydev;
> > +     struct lan743x_phy *phy = &adapter->phy;
> > +
> > +     if (!phydev)
> > +             return -ENODEV;
> > +
> > +     if (!phy_validate_pause(phydev, pause))
> > +             return -EINVAL;
> > +
> > +     phy->fc_request_control = 0;
> > +     if (pause->rx_pause)
> > +             phy->fc_request_control |= FLOW_CTRL_RX;
> > +
> > +     if (pause->tx_pause)
> > +             phy->fc_request_control |= FLOW_CTRL_TX;
> > +
> > +     phy->fc_autoneg = pause->autoneg;
> > +
> > +     phy_set_asym_pause(phydev, pause->rx_pause,  pause->tx_pause);
> > +
> > +     if (pause->autoneg == AUTONEG_DISABLE)
> > +             lan743x_mac_flow_ctrl_set_enables(adapter, pause->tx_pause,
> > +                                               pause->rx_pause);
> 
> pause is not too well defined. But i think phy_set_asym_pause() should
> be in an else clause. If pause autoneg is off, you directly set it in
> the MAC and ignore what is negotiated. If it is enabled, you
> negotiate. As far as i understand, you don't modify your negotiation
> when pause autoneg is off.

O.K. I will change.

> 
>         Andrew

--------
Thanks,
Raju