diff mbox series

[net-next,5/5] net: lan743x: Add Clause-45 MDIO access

Message ID 20220127173055.308918-6-Raju.Lakkaraju@microchip.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: lan743x: PCI1A011/PCI1A014 Enhancment | 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 warning 1 maintainers not CCed: bryan.whitehead@microchip.com
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/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, 178 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 Jan. 27, 2022, 5:30 p.m. UTC 
PCI1A011/PCI1A041 chip support the MDIO Clause-45 access

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

Comments

Andrew Lunn Jan. 27, 2022, 10:23 p.m. UTC | #1 
On Thu, Jan 27, 2022 at 11:00:55PM +0530, Raju Lakkaraju wrote:
> PCI1A011/PCI1A041 chip support the MDIO Clause-45 access
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> ---
>  drivers/net/ethernet/microchip/lan743x_main.c | 110 +++++++++++++++++-
>  drivers/net/ethernet/microchip/lan743x_main.h |  16 +++
>  2 files changed, 123 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 6f6655eb6438..98d346aaf627 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -793,6 +793,95 @@ static int lan743x_mdiobus_write(struct mii_bus *bus,
>  	return ret;
>  }
>  
> +static u32 lan743x_mac_mmd_access(int id, int index, int op, u8 freq)
> +{
> +	u16 dev_addr;
> +	u32 ret;
> +
> +	dev_addr = (index >> 16) & 0x1f;
> +	ret = (id << MAC_MII_ACC_PHY_ADDR_SHIFT_) &
> +		MAC_MII_ACC_PHY_ADDR_MASK_;
> +	ret |= (dev_addr << MAC_MII_ACC_MIIMMD_SHIFT_) &
> +		MAC_MII_ACC_MIIMMD_MASK_;
> +	if (freq)
> +		ret |= (freq << MAC_MII_ACC_MDC_CYCLE_SHIFT_) &
> +			MAC_MII_ACC_MDC_CYCLE_MASK_;

All callers of this function appear to pass freq as 0. So you can
remove this.

> +	if (op == 1)
> +		ret |= MAC_MII_ACC_MIICMD_WRITE_;
> +	else if (op == 2)
> +		ret |= MAC_MII_ACC_MIICMD_READ_;
> +	else if (op == 3)
> +		ret |= MAC_MII_ACC_MIICMD_READ_INC_;
> +	else
> +		ret |= MAC_MII_ACC_MIICMD_ADDR_;

> +		mmd_access = lan743x_mac_mmd_access(phy_id, index, 0, 0);

It is pretty opaque what the 0 means here. How about you actually pass
MAC_MII_ACC_MIICMD_ values?

lan743x_mac_mmd_access(phy_id, index, );

> +		if (adapter->mdiobus->probe_capabilities == MDIOBUS_C45)
> +			phydev->c45_ids.devices_in_package &= ~BIT(0);
>  	}

A MAC driver should not be modifying the phydev structure.

>  	/* MAC doesn't support 1000T Half */
> @@ -2822,12 +2914,14 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  			sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
>  			lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
>  			netif_info(adapter, drv, adapter->netdev, "SGMII operation\n");
> +			adapter->mdiobus->probe_capabilities = MDIOBUS_C22_C45;
>  		} else {
>  			sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
>  			sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
>  			sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
>  			lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
>  			netif_info(adapter, drv, adapter->netdev, "GMII operation\n");
> +			adapter->mdiobus->probe_capabilities = MDIOBUS_C22;
>  		}
>  	} else {
>  		chip_ver = lan743x_csr_read(adapter, STRAP_READ);
> @@ -2839,19 +2933,29 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  			sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
>  			lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
>  			netif_info(adapter, drv, adapter->netdev, "SGMII operation\n");
> +			adapter->mdiobus->probe_capabilities = MDIOBUS_C22_C45;
>  		} else {
>  			sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
>  			sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
>  			sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
>  			lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
>  			netif_info(adapter, drv, adapter->netdev, "GMII operation\n");
> +			adapter->mdiobus->probe_capabilities = MDIOBUS_C22;

This manipulation of adapter->mdiobus->probe_capabilities based on
SGMII vs RGMII makes no sense. It should be set based on what the bus
master can actually do. I assume the PCI1A011/PCI1A041 can do both C22
and C45. So it should look at the reg value and either do a C45
transaction, or a C22 transaction. Do the older chips not support C45?
In that case, return -EOPNOTSUPP if asked to do a C45 transaction.

	Andrew
Raju Lakkaraju - I30499 Jan. 31, 2022, 6:14 p.m. UTC | #2 
Hi Andrew,

Thank you for review comments.

The 01/27/2022 23:23, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Jan 27, 2022 at 11:00:55PM +0530, Raju Lakkaraju wrote:
> > PCI1A011/PCI1A041 chip support the MDIO Clause-45 access
> >
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> > ---
> >  drivers/net/ethernet/microchip/lan743x_main.c | 110 +++++++++++++++++-
> >  drivers/net/ethernet/microchip/lan743x_main.h |  16 +++
> >  2 files changed, 123 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> > index 6f6655eb6438..98d346aaf627 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> > @@ -793,6 +793,95 @@ static int lan743x_mdiobus_write(struct mii_bus *bus,
> >       return ret;
> >  }
> >
> > +static u32 lan743x_mac_mmd_access(int id, int index, int op, u8 freq)
> > +{
> > +     u16 dev_addr;
> > +     u32 ret;
> > +
> > +     dev_addr = (index >> 16) & 0x1f;
> > +     ret = (id << MAC_MII_ACC_PHY_ADDR_SHIFT_) &
> > +             MAC_MII_ACC_PHY_ADDR_MASK_;
> > +     ret |= (dev_addr << MAC_MII_ACC_MIIMMD_SHIFT_) &
> > +             MAC_MII_ACC_MIIMMD_MASK_;
> > +     if (freq)
> > +             ret |= (freq << MAC_MII_ACC_MDC_CYCLE_SHIFT_) &
> > +                     MAC_MII_ACC_MDC_CYCLE_MASK_;
> 
> All callers of this function appear to pass freq as 0. So you can
> remove this.
> 

Accepted.
Yes. Currently frequency is not programming.
I will change.

> > +     if (op == 1)
> > +             ret |= MAC_MII_ACC_MIICMD_WRITE_;
> > +     else if (op == 2)
> > +             ret |= MAC_MII_ACC_MIICMD_READ_;
> > +     else if (op == 3)
> > +             ret |= MAC_MII_ACC_MIICMD_READ_INC_;
> > +     else
> > +             ret |= MAC_MII_ACC_MIICMD_ADDR_;
> 
> > +             mmd_access = lan743x_mac_mmd_access(phy_id, index, 0, 0);
> 
> It is pretty opaque what the 0 means here. How about you actually pass
> MAC_MII_ACC_MIICMD_ values?
> 
> lan743x_mac_mmd_access(phy_id, index, );
> 

Accepted. I will change

> > +             if (adapter->mdiobus->probe_capabilities == MDIOBUS_C45)
> > +                     phydev->c45_ids.devices_in_package &= ~BIT(0);
> >       }
> 
> A MAC driver should not be modifying the phydev structure.
> 

Accepted. I will remove this change.

> >       /* MAC doesn't support 1000T Half */
> > @@ -2822,12 +2914,14 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >                       sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
> >                       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >                       netif_info(adapter, drv, adapter->netdev, "SGMII operation\n");
> > +                     adapter->mdiobus->probe_capabilities = MDIOBUS_C22_C45;
> >               } else {
> >                       sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
> >                       sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
> >                       sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
> >                       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >                       netif_info(adapter, drv, adapter->netdev, "GMII operation\n");
> > +                     adapter->mdiobus->probe_capabilities = MDIOBUS_C22;
> >               }
> >       } else {
> >               chip_ver = lan743x_csr_read(adapter, STRAP_READ);
> > @@ -2839,19 +2933,29 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >                       sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
> >                       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >                       netif_info(adapter, drv, adapter->netdev, "SGMII operation\n");
> > +                     adapter->mdiobus->probe_capabilities = MDIOBUS_C22_C45;
> >               } else {
> >                       sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
> >                       sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
> >                       sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
> >                       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >                       netif_info(adapter, drv, adapter->netdev, "GMII operation\n");
> > +                     adapter->mdiobus->probe_capabilities = MDIOBUS_C22;
> 
> This manipulation of adapter->mdiobus->probe_capabilities based on
> SGMII vs RGMII makes no sense. It should be set based on what the bus
> master can actually do. I assume the PCI1A011/PCI1A041 can do both C22
> and C45. So it should look at the reg value and either do a C45
> transaction, or a C22 transaction. Do the older chips not support C45?
> In that case, return -EOPNOTSUPP if asked to do a C45 transaction.
> 

Yes, Older chip does not suuport C45.
I will change code such that without upate the
"adapter->mdiobus->probe_capabilities" variable, assign the read/write
functions based chip id.

>         Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 6f6655eb6438..98d346aaf627 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -793,6 +793,95 @@  static int lan743x_mdiobus_write(struct mii_bus *bus,
 	return ret;
 }
 
+static u32 lan743x_mac_mmd_access(int id, int index, int op, u8 freq)
+{
+	u16 dev_addr;
+	u32 ret;
+
+	dev_addr = (index >> 16) & 0x1f;
+	ret = (id << MAC_MII_ACC_PHY_ADDR_SHIFT_) &
+		MAC_MII_ACC_PHY_ADDR_MASK_;
+	ret |= (dev_addr << MAC_MII_ACC_MIIMMD_SHIFT_) &
+		MAC_MII_ACC_MIIMMD_MASK_;
+	if (freq)
+		ret |= (freq << MAC_MII_ACC_MDC_CYCLE_SHIFT_) &
+			MAC_MII_ACC_MDC_CYCLE_MASK_;
+	if (op == 1)
+		ret |= MAC_MII_ACC_MIICMD_WRITE_;
+	else if (op == 2)
+		ret |= MAC_MII_ACC_MIICMD_READ_;
+	else if (op == 3)
+		ret |= MAC_MII_ACC_MIICMD_READ_INC_;
+	else
+		ret |= MAC_MII_ACC_MIICMD_ADDR_;
+	ret |= (MAC_MII_ACC_MII_BUSY_ | MAC_MII_ACC_MIICL45_);
+
+	return ret;
+}
+
+static int lan743x_mdiobus_c45_read(struct mii_bus *bus, int phy_id, int index)
+{
+	struct lan743x_adapter *adapter = bus->priv;
+	u32 mmd_access;
+	int ret;
+
+	/* comfirm MII not busy */
+	ret = lan743x_mac_mii_wait_till_not_busy(adapter);
+	if (ret < 0)
+		return ret;
+	if (index & MII_ADDR_C45) {
+		/* Load Register Address */
+		lan743x_csr_write(adapter, MAC_MII_DATA, (u32)(index & 0xffff));
+		mmd_access = lan743x_mac_mmd_access(phy_id, index, 0, 0);
+		lan743x_csr_write(adapter, MAC_MII_ACC, mmd_access);
+		ret = lan743x_mac_mii_wait_till_not_busy(adapter);
+		if (ret < 0)
+			return ret;
+		/* Read Data */
+		mmd_access = lan743x_mac_mmd_access(phy_id, index, 2, 0);
+		lan743x_csr_write(adapter, MAC_MII_ACC, mmd_access);
+		ret = lan743x_mac_mii_wait_till_not_busy(adapter);
+		if (ret < 0)
+			return ret;
+		ret = lan743x_csr_read(adapter, MAC_MII_DATA);
+		return (int)(ret & 0xFFFF);
+	}
+
+	ret = lan743x_mdiobus_read(bus, phy_id, index);
+	return ret;
+}
+
+static int lan743x_mdiobus_c45_write(struct mii_bus *bus,
+				     int phy_id, int index, u16 regval)
+{
+	struct lan743x_adapter *adapter = bus->priv;
+	u32 mmd_access;
+	int ret;
+
+	/* confirm MII not busy */
+	ret = lan743x_mac_mii_wait_till_not_busy(adapter);
+	if (ret < 0)
+		return ret;
+	if (index & MII_ADDR_C45) {
+		/* Load Register Address */
+		lan743x_csr_write(adapter, MAC_MII_DATA, (u32)(index & 0xffff));
+		mmd_access = lan743x_mac_mmd_access(phy_id, index, 0, 0);
+		lan743x_csr_write(adapter, MAC_MII_ACC, mmd_access);
+		ret = lan743x_mac_mii_wait_till_not_busy(adapter);
+		if (ret < 0)
+			return ret;
+		/* Write Data */
+		lan743x_csr_write(adapter, MAC_MII_DATA, (u32)regval);
+		mmd_access = lan743x_mac_mmd_access(phy_id, index, 1, 0);
+		lan743x_csr_write(adapter, MAC_MII_ACC, mmd_access);
+		ret = lan743x_mac_mii_wait_till_not_busy(adapter);
+	} else {
+		ret = lan743x_mdiobus_write(bus, phy_id, index, regval);
+	}
+
+	return ret;
+}
+
 static void lan743x_mac_set_address(struct lan743x_adapter *adapter,
 				    u8 *addr)
 {
@@ -1053,6 +1142,9 @@  static int lan743x_phy_open(struct lan743x_adapter *adapter)
 					 PHY_INTERFACE_MODE_GMII);
 		if (ret)
 			goto return_error;
+
+		if (adapter->mdiobus->probe_capabilities == MDIOBUS_C45)
+			phydev->c45_ids.devices_in_package &= ~BIT(0);
 	}
 
 	/* MAC doesn't support 1000T Half */
@@ -2822,12 +2914,14 @@  static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 			sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
 			lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
 			netif_info(adapter, drv, adapter->netdev, "SGMII operation\n");
+			adapter->mdiobus->probe_capabilities = MDIOBUS_C22_C45;
 		} else {
 			sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
 			sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
 			sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
 			lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
 			netif_info(adapter, drv, adapter->netdev, "GMII operation\n");
+			adapter->mdiobus->probe_capabilities = MDIOBUS_C22;
 		}
 	} else {
 		chip_ver = lan743x_csr_read(adapter, STRAP_READ);
@@ -2839,19 +2933,29 @@  static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 			sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
 			lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
 			netif_info(adapter, drv, adapter->netdev, "SGMII operation\n");
+			adapter->mdiobus->probe_capabilities = MDIOBUS_C22_C45;
 		} else {
 			sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
 			sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
 			sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
 			lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
 			netif_info(adapter, drv, adapter->netdev, "GMII operation\n");
+			adapter->mdiobus->probe_capabilities = MDIOBUS_C22;
 		}
 	}
 
 	adapter->mdiobus->priv = (void *)adapter;
-	adapter->mdiobus->read = lan743x_mdiobus_read;
-	adapter->mdiobus->write = lan743x_mdiobus_write;
-	adapter->mdiobus->name = "lan743x-mdiobus";
+	if (adapter->mdiobus->probe_capabilities == MDIOBUS_C45 ||
+	    adapter->mdiobus->probe_capabilities == MDIOBUS_C22_C45) {
+		adapter->mdiobus->read = lan743x_mdiobus_c45_read;
+		adapter->mdiobus->write = lan743x_mdiobus_c45_write;
+		adapter->mdiobus->name = "lan743x-mdiobus-c45";
+	} else {
+		adapter->mdiobus->read = lan743x_mdiobus_read;
+		adapter->mdiobus->write = lan743x_mdiobus_write;
+		adapter->mdiobus->name = "lan743x-mdiobus";
+	}
+
 	snprintf(adapter->mdiobus->id, MII_BUS_ID_SIZE,
 		 "pci-%s", pci_name(adapter->pdev));
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 233555dd5464..12facf905eaa 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -149,6 +149,13 @@ 
 #define MAC_RX_ADDRL			(0x11C)
 
 #define MAC_MII_ACC			(0x120)
+#define MAC_MII_ACC_MDC_CYCLE_SHIFT_	(16)
+#define MAC_MII_ACC_MDC_CYCLE_MASK_	(0x00070000)
+#define MAC_MII_ACC_MDC_CYCLE_2_5MHZ_	(0)
+#define MAC_MII_ACC_MDC_CYCLE_5MHZ_	(1)
+#define MAC_MII_ACC_MDC_CYCLE_12_5MHZ_	(2)
+#define MAC_MII_ACC_MDC_CYCLE_25MHZ_	(3)
+#define MAC_MII_ACC_MDC_CYCLE_1_25MHZ_	(4)
 #define MAC_MII_ACC_PHY_ADDR_SHIFT_	(11)
 #define MAC_MII_ACC_PHY_ADDR_MASK_	(0x0000F800)
 #define MAC_MII_ACC_MIIRINDA_SHIFT_	(6)
@@ -157,6 +164,15 @@ 
 #define MAC_MII_ACC_MII_WRITE_		(0x00000002)
 #define MAC_MII_ACC_MII_BUSY_		BIT(0)
 
+#define MAC_MII_ACC_MIIMMD_SHIFT_	(6)
+#define MAC_MII_ACC_MIIMMD_MASK_	(0x000007C0)
+#define MAC_MII_ACC_MIICL45_		BIT(3)
+#define MAC_MII_ACC_MIICMD_MASK_	(0x00000006)
+#define MAC_MII_ACC_MIICMD_ADDR_	(0x00000000)
+#define MAC_MII_ACC_MIICMD_WRITE_	(0x00000002)
+#define MAC_MII_ACC_MIICMD_READ_	(0x00000004)
+#define MAC_MII_ACC_MIICMD_READ_INC_	(0x00000006)
+
 #define MAC_MII_DATA			(0x124)
 
 #define MAC_EEE_TX_LPI_REQ_DLY_CNT		(0x130)