[PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

Raju Lakkaraju posted 5 patches 2 months, 2 weeks ago
[PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
Posted by Raju Lakkaraju 2 months, 2 weeks ago
Register MDIO bus for PCS layer to use Synopsys designware XPCS, support
SGMII/1000Base-X/2500Base-X interfaces.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
============                                                                    
V2 : Include this new patch in the V2 series. 

 drivers/net/ethernet/microchip/Kconfig        |  1 +
 drivers/net/ethernet/microchip/lan743x_main.c | 72 ++++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  2 +
 3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index 3dacf39b49b4..793a20ef51fc 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -53,6 +53,7 @@ config LAN743X
 	select I2C_PCI1XXXX
 	select GP_PCI1XXXX
 	select SFP
+	select PCS_XPCS
 	help
 	  Support for the Microchip LAN743x and PCI11x1x families of PCI
 	  Express Ethernet devices
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index c1061e2972f9..ef76d0c1642f 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1143,6 +1143,28 @@ static int lan743x_get_lsd(int speed, int duplex, u8 mss)
 	return lsd;
 }
 
+static int pci11x1x_pcs_read(struct mii_bus *bus, int addr, int devnum,
+			     int regnum)
+{
+	struct lan743x_adapter *adapter = bus->priv;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	return lan743x_sgmii_read(adapter, devnum, regnum);
+}
+
+static int pci11x1x_pcs_write(struct mii_bus *bus, int addr, int devnum,
+			      int regnum, u16 val)
+{
+	struct lan743x_adapter *adapter = bus->priv;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	return lan743x_sgmii_write(adapter, devnum, regnum, val);
+}
+
 static int lan743x_sgmii_mpll_set(struct lan743x_adapter *adapter,
 				  u16 baud)
 {
@@ -3201,6 +3223,19 @@ void lan743x_mac_eee_enable(struct lan743x_adapter *adapter, bool enable)
 	lan743x_csr_write(adapter, MAC_CR, mac_cr);
 }
 
+static struct phylink_pcs *
+lan743x_phylink_mac_select_pcs(struct phylink_config *config,
+			       phy_interface_t interface)
+{
+	struct net_device *netdev = to_net_dev(config->dev);
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+	if (adapter->xpcs)
+		return &adapter->xpcs->pcs;
+
+	return NULL;
+}
+
 static void lan743x_phylink_mac_config(struct phylink_config *config,
 				       unsigned int link_an_mode,
 				       const struct phylink_link_state *state)
@@ -3302,6 +3337,7 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
 }
 
 static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
+	.mac_select_pcs = lan743x_phylink_mac_select_pcs,
 	.mac_config = lan743x_phylink_mac_config,
 	.mac_link_down = lan743x_phylink_mac_link_down,
 	.mac_link_up = lan743x_phylink_mac_link_up,
@@ -3654,6 +3690,9 @@ static void lan743x_hardware_cleanup(struct lan743x_adapter *adapter)
 
 static void lan743x_mdiobus_cleanup(struct lan743x_adapter *adapter)
 {
+	if (adapter->xpcs)
+		xpcs_destroy(adapter->xpcs);
+
 	mdiobus_unregister(adapter->mdiobus);
 }
 
@@ -3763,6 +3802,7 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 
 static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 {
+	struct dw_xpcs *xpcs;
 	u32 sgmii_ctl;
 	int ret;
 
@@ -3783,8 +3823,17 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 				  "SGMII operation\n");
 			adapter->mdiobus->read = lan743x_mdiobus_read_c22;
 			adapter->mdiobus->write = lan743x_mdiobus_write_c22;
-			adapter->mdiobus->read_c45 = lan743x_mdiobus_read_c45;
-			adapter->mdiobus->write_c45 = lan743x_mdiobus_write_c45;
+			if (adapter->is_sfp_support_en) {
+				adapter->mdiobus->read_c45 =
+					pci11x1x_pcs_read;
+				adapter->mdiobus->write_c45 =
+					pci11x1x_pcs_write;
+			} else {
+				adapter->mdiobus->read_c45 =
+					 lan743x_mdiobus_read_c45;
+				adapter->mdiobus->write_c45 =
+					 lan743x_mdiobus_write_c45;
+			}
 			adapter->mdiobus->name = "lan743x-mdiobus-c45";
 			netif_dbg(adapter, drv, adapter->netdev,
 				  "lan743x-mdiobus-c45\n");
@@ -3820,9 +3869,28 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 	ret = mdiobus_register(adapter->mdiobus);
 	if (ret < 0)
 		goto return_error;
+
+	if (adapter->is_sfp_support_en) {
+		if (!adapter->phy_interface)
+			lan743x_phy_interface_select(adapter);
+
+		xpcs = xpcs_create_mdiodev(adapter->mdiobus, 0,
+					   adapter->phy_interface);
+		if (IS_ERR(xpcs)) {
+			netdev_err(adapter->netdev, "failed to create xpcs\n");
+			ret = PTR_ERR(xpcs);
+			goto err_destroy_xpcs;
+		}
+		adapter->xpcs = xpcs;
+	}
+
 	return 0;
 
+err_destroy_xpcs:
+	xpcs_destroy(xpcs);
+
 return_error:
+	mdiobus_free(adapter->mdiobus);
 	return ret;
 }
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index c303a69c3bea..f7480a401a27 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -10,6 +10,7 @@
 #include <linux/i2c.h>
 #include <linux/gpio/machine.h>
 #include <linux/auxiliary_bus.h>
+#include <linux/pcs/pcs-xpcs.h>
 #include "lan743x_ptp.h"
 
 #define DRIVER_AUTHOR   "Bryan Whitehead <Bryan.Whitehead@microchip.com>"
@@ -1130,6 +1131,7 @@ struct lan743x_adapter {
 	struct lan743x_sw_nodes	*nodes;
 	struct i2c_adapter	*i2c_adap;
 	struct platform_device	*sfp_dev;
+	struct dw_xpcs		*xpcs;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
-- 
2.34.1
Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
Posted by Andrew Lunn 2 months, 2 weeks ago
> +static int pci11x1x_pcs_read(struct mii_bus *bus, int addr, int devnum,
> +			     int regnum)
> +{
> +	struct lan743x_adapter *adapter = bus->priv;
> +
> +	if (addr)
> +		return -EOPNOTSUPP;
> +
> +	return lan743x_sgmii_read(adapter, devnum, regnum);
> +}

>  static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  {
> +	struct dw_xpcs *xpcs;
>  	u32 sgmii_ctl;
>  	int ret;
>  
> @@ -3783,8 +3823,17 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  				  "SGMII operation\n");
>  			adapter->mdiobus->read = lan743x_mdiobus_read_c22;
>  			adapter->mdiobus->write = lan743x_mdiobus_write_c22;
> -			adapter->mdiobus->read_c45 = lan743x_mdiobus_read_c45;
> -			adapter->mdiobus->write_c45 = lan743x_mdiobus_write_c45;
> +			if (adapter->is_sfp_support_en) {
> +				adapter->mdiobus->read_c45 =
> +					pci11x1x_pcs_read;
> +				adapter->mdiobus->write_c45 =
> +					pci11x1x_pcs_write;

As you can see, the naming convention is to put the bus transaction
type on the end. So please name these pci11x1x_pcs_read_c45...

Also, am i reading this correct. C22 transfers will go out a
completely different bus to C45 transfers when there is an SFP?

If there are two physical MDIO busses, please instantiate two Linux
MDIO busses.

    Andrew

---
pw-bot: cr
Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
Posted by Raju Lakkaraju 2 months, 2 weeks ago
Hi Andrew,

The 09/11/2024 19:26, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > +static int pci11x1x_pcs_read(struct mii_bus *bus, int addr, int devnum,
> > +                          int regnum)
> > +{
> > +     struct lan743x_adapter *adapter = bus->priv;
> > +
> > +     if (addr)
> > +             return -EOPNOTSUPP;
> > +
> > +     return lan743x_sgmii_read(adapter, devnum, regnum);
> > +}
> 
> >  static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >  {
> > +     struct dw_xpcs *xpcs;
> >       u32 sgmii_ctl;
> >       int ret;
> >
> > @@ -3783,8 +3823,17 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >                                 "SGMII operation\n");
> >                       adapter->mdiobus->read = lan743x_mdiobus_read_c22;
> >                       adapter->mdiobus->write = lan743x_mdiobus_write_c22;
> > -                     adapter->mdiobus->read_c45 = lan743x_mdiobus_read_c45;
> > -                     adapter->mdiobus->write_c45 = lan743x_mdiobus_write_c45;
> > +                     if (adapter->is_sfp_support_en) {
> > +                             adapter->mdiobus->read_c45 =
> > +                                     pci11x1x_pcs_read;
> > +                             adapter->mdiobus->write_c45 =
> > +                                     pci11x1x_pcs_write;
> 
> As you can see, the naming convention is to put the bus transaction
> type on the end. So please name these pci11x1x_pcs_read_c45...

Accpeted. I will fix

> 
> Also, am i reading this correct. C22 transfers will go out a
> completely different bus to C45 transfers when there is an SFP?

No. You are correct.
This LAN743x driver support following chips
1. LAN7430 - C22 only with GMII/RGMII I/F
2. LAN7431 - C22 only with MII I/F
3. PCI11010/PCI11414 - C45 with RGMII or SGMII/1000Base-X/2500Base-X
   If SFP enable, then XPCS's C45 PCS access
   If SGMII only enable, then SGMII (PCS) C45 access

> 
> If there are two physical MDIO busses, please instantiate two Linux
> MDIO busses.
> 

XPCS driver is doing.
Am i miss anything there?

>     Andrew
> 
> ---
> pw-bot: cr

-- 
Thanks,                                                                         
Raju
Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
Posted by Andrew Lunn 2 months, 2 weeks ago
> > Also, am i reading this correct. C22 transfers will go out a
> > completely different bus to C45 transfers when there is an SFP?
> 
> No. You are correct.
> This LAN743x driver support following chips
> 1. LAN7430 - C22 only with GMII/RGMII I/F
> 2. LAN7431 - C22 only with MII I/F

Fine, simple, not a problem.

> 3. PCI11010/PCI11414 - C45 with RGMII or SGMII/1000Base-X/2500Base-X
>    If SFP enable, then XPCS's C45 PCS access
>    If SGMII only enable, then SGMII (PCS) C45 access

Physically, there are two MDIO busses? There is an external MDIO bus
with two pins along side the RGMII/SGMII pins? And internally, there
is an MDIO bus to the PCS block?

Some designs do have only one bus, the internal PCS uses address X on
the bus and you are simply not allowed to put an external device at
that address.

But from my reading of the code, you have two MDIO busses, so you need
two Linux MDIO busses.

	Andrew
Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
Posted by Maxime Chevallier 2 months, 2 weeks ago
Hello Raju,

On Wed, 11 Sep 2024 21:40:53 +0530
Raju Lakkaraju <Raju.Lakkaraju@microchip.com> wrote:

[...]

> @@ -3820,9 +3869,28 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  	ret = mdiobus_register(adapter->mdiobus);
>  	if (ret < 0)
>  		goto return_error;
> +
> +	if (adapter->is_sfp_support_en) {
> +		if (!adapter->phy_interface)
> +			lan743x_phy_interface_select(adapter);
> +
> +		xpcs = xpcs_create_mdiodev(adapter->mdiobus, 0,
> +					   adapter->phy_interface);
> +		if (IS_ERR(xpcs)) {
> +			netdev_err(adapter->netdev, "failed to create xpcs\n");
> +			ret = PTR_ERR(xpcs);
> +			goto err_destroy_xpcs;
> +		}
> +		adapter->xpcs = xpcs;
> +	}
> +
>  	return 0;
>  
> +err_destroy_xpcs:
> +	xpcs_destroy(xpcs);

It looks like here, you're destroying the xpcs only when the xpcs
couln't be created in the first place, so no need to destroy it :)

Best regards,

Maxime
Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
Posted by Raju Lakkaraju 2 months, 2 weeks ago
Hi Maxime,

Thank you for review the patches.

The 09/11/2024 19:24, Maxime Chevallier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello Raju,
> 
> On Wed, 11 Sep 2024 21:40:53 +0530
> Raju Lakkaraju <Raju.Lakkaraju@microchip.com> wrote:
> 
> [...]
> 
> > @@ -3820,9 +3869,28 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >       ret = mdiobus_register(adapter->mdiobus);
> >       if (ret < 0)
> >               goto return_error;
> > +
> > +     if (adapter->is_sfp_support_en) {
> > +             if (!adapter->phy_interface)
> > +                     lan743x_phy_interface_select(adapter);
> > +
> > +             xpcs = xpcs_create_mdiodev(adapter->mdiobus, 0,
> > +                                        adapter->phy_interface);
> > +             if (IS_ERR(xpcs)) {
> > +                     netdev_err(adapter->netdev, "failed to create xpcs\n");
> > +                     ret = PTR_ERR(xpcs);
> > +                     goto err_destroy_xpcs;
> > +             }
> > +             adapter->xpcs = xpcs;
> > +     }
> > +
> >       return 0;
> >
> > +err_destroy_xpcs:
> > +     xpcs_destroy(xpcs);
> 
> It looks like here, you're destroying the xpcs only when the xpcs
> couln't be created in the first place, so no need to destroy it :)

Ok. I will remove

But i was little bit confusion here.

In xpcs_create_mdiodev( ) function, inside the mdio_device_create( ) function 
allocate memory for mdio_device

Then, in xpcs_create( ) function to create data by calling xpcs_create_data( )
function, create dw_xpcs memory.

It's reason, for safe side, I updte xpcs destroy

> 
> Best regards,
> 
> Maxime

-- 
Thanks,                                                                         
Raju