[PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Voon Weifeng]

From: Kweh Hock Leong <hock.leong.kweh@intel.com>

DWMAC4 is capable to support clause 45 mdio communication.
This patch enable the feature on stmmac_mdio_write() and
stmmac_mdio_read() by following phy_write_mmd() and
phy_read_mmd() mdiobus read write implementation format.

Reviewed-by: Li, Yifan <yifan2.li@intel.com>
Signed-off-by: Kweh Hock Leong <hock.leong.kweh@intel.com>
Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
Signed-off-by: Weifeng Voon <weifeng.voon@intel.com>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 18cadf0b0d66..b9edb18769f2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -24,11 +24,27 @@
 
 #define MII_BUSY 0x00000001
 #define MII_WRITE 0x00000002
+#define MII_DATA_MASK GENMASK(15, 0)
 
 /* GMAC4 defines */
 #define MII_GMAC4_GOC_SHIFT		2
+#define MII_GMAC4_REG_ADDR_SHIFT	16
 #define MII_GMAC4_WRITE			(1 << MII_GMAC4_GOC_SHIFT)
 #define MII_GMAC4_READ			(3 << MII_GMAC4_GOC_SHIFT)
+#define MII_GMAC4_C45E			BIT(1)
+
+static void stmmac_mdio_c45_setup(struct stmmac_priv *priv, int phyreg,
+				  u32 *val, u32 *data)
+{
+	unsigned int reg_shift = priv->hw->mii.reg_shift;
+	unsigned int reg_mask = priv->hw->mii.reg_mask;
+
+	*val |= MII_GMAC4_C45E;
+	*val &= ~reg_mask;
+	*val |= ((phyreg >> MII_DEVADDR_C45_SHIFT) << reg_shift) & reg_mask;
+
+	*data |= (phyreg & MII_REGADDR_C45_MASK) << MII_GMAC4_REG_ADDR_SHIFT;
+}
 
 /* XGMAC defines */
 #define MII_XGMAC_SADDR			BIT(18)
@@ -155,22 +171,26 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
 	struct stmmac_priv *priv = netdev_priv(ndev);
 	unsigned int mii_address = priv->hw->mii.addr;
 	unsigned int mii_data = priv->hw->mii.data;
-	u32 v;
-	int data;
 	u32 value = MII_BUSY;
+	int data = 0;
+	u32 v;
 
 	value |= (phyaddr << priv->hw->mii.addr_shift)
 		& priv->hw->mii.addr_mask;
 	value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw->mii.reg_mask;
 	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
 		& priv->hw->mii.clk_csr_mask;
-	if (priv->plat->has_gmac4)
+	if (priv->plat->has_gmac4) {
 		value |= MII_GMAC4_READ;
+		if (phyreg & MII_ADDR_C45)
+			stmmac_mdio_c45_setup(priv, phyreg, &value, &data);
+	}
 
 	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
 			       100, 10000))
 		return -EBUSY;
 
+	writel(data, priv->ioaddr + mii_data);
 	writel(value, priv->ioaddr + mii_address);
 
 	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
@@ -178,7 +198,7 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
 		return -EBUSY;
 
 	/* Read the data from the MII data register */
-	data = (int)readl(priv->ioaddr + mii_data);
+	data = (int)readl(priv->ioaddr + mii_data) & MII_DATA_MASK;
 
 	return data;
 }
@@ -198,8 +218,9 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg,
 	struct stmmac_priv *priv = netdev_priv(ndev);
 	unsigned int mii_address = priv->hw->mii.addr;
 	unsigned int mii_data = priv->hw->mii.data;
-	u32 v;
 	u32 value = MII_BUSY;
+	int data = phydata;
+	u32 v;
 
 	value |= (phyaddr << priv->hw->mii.addr_shift)
 		& priv->hw->mii.addr_mask;
@@ -207,10 +228,13 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg,
 
 	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
 		& priv->hw->mii.clk_csr_mask;
-	if (priv->plat->has_gmac4)
+	if (priv->plat->has_gmac4) {
 		value |= MII_GMAC4_WRITE;
-	else
+		if (phyreg & MII_ADDR_C45)
+			stmmac_mdio_c45_setup(priv, phyreg, &value, &data);
+	} else {
 		value |= MII_WRITE;
+	}
 
 	/* Wait until any existing MII operation is complete */
 	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
@@ -218,7 +242,7 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg,
 		return -EBUSY;
 
 	/* Set the MII address register to write */
-	writel(phydata, priv->ioaddr + mii_data);
+	writel(data, priv->ioaddr + mii_data);
 	writel(value, priv->ioaddr + mii_address);
 
 	/* Wait until any existing MII operation is complete */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index d0af7d37fdf9..1739c6dc470e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -195,6 +195,8 @@ static inline const char *phy_modes(phy_interface_t interface)
 /* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit
    IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy chips. */
 #define MII_ADDR_C45 (1<<30)
+#define MII_DEVADDR_C45_SHIFT	16
+#define MII_REGADDR_C45_MASK	GENMASK(15, 0)
 
 struct device;
 struct phylink;
-- 
1.9.1

Re: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Andrew Lunn]

On Wed, Jul 03, 2019 at 05:50:04PM +0800, Voon Weifeng wrote:
> @@ -155,22 +171,26 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  	unsigned int mii_address = priv->hw->mii.addr;
>  	unsigned int mii_data = priv->hw->mii.data;
> -	u32 v;
> -	int data;
>  	u32 value = MII_BUSY;
> +	int data = 0;
> +	u32 v;
>  
>  	value |= (phyaddr << priv->hw->mii.addr_shift)
>  		& priv->hw->mii.addr_mask;
>  	value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw->mii.reg_mask;
>  	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
>  		& priv->hw->mii.clk_csr_mask;
> -	if (priv->plat->has_gmac4)
> +	if (priv->plat->has_gmac4) {
>  		value |= MII_GMAC4_READ;
> +		if (phyreg & MII_ADDR_C45)
> +			stmmac_mdio_c45_setup(priv, phyreg, &value, &data);
> +	}
>  
>  	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
>  			       100, 10000))
>  		return -EBUSY;
>  
> +	writel(data, priv->ioaddr + mii_data);

That looks odd. Could you explain why it is needed.

Thanks
	Andrew

RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Voon, Weifeng]

> > @@ -155,22 +171,26 @@ static int stmmac_mdio_read(struct mii_bus *bus,
> int phyaddr, int phyreg)
> >  	struct stmmac_priv *priv = netdev_priv(ndev);
> >  	unsigned int mii_address = priv->hw->mii.addr;
> >  	unsigned int mii_data = priv->hw->mii.data;
> > -	u32 v;
> > -	int data;
> >  	u32 value = MII_BUSY;
> > +	int data = 0;
> > +	u32 v;
> >
> >  	value |= (phyaddr << priv->hw->mii.addr_shift)
> >  		& priv->hw->mii.addr_mask;
> >  	value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw-
> >mii.reg_mask;
> >  	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> >  		& priv->hw->mii.clk_csr_mask;
> > -	if (priv->plat->has_gmac4)
> > +	if (priv->plat->has_gmac4) {
> >  		value |= MII_GMAC4_READ;
> > +		if (phyreg & MII_ADDR_C45)
> > +			stmmac_mdio_c45_setup(priv, phyreg, &value, &data);
> > +	}
> >
> >  	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v &
> MII_BUSY),
> >  			       100, 10000))
> >  		return -EBUSY;
> >
> > +	writel(data, priv->ioaddr + mii_data);
> 
> That looks odd. Could you explain why it is needed.
> 
> Thanks
> 	Andrew

Hi Andrew,
This mdio c45 support needed to access DWC xPCS which is a Clause-45
MDIO Manageable Device (MMD). This is discuss in:
https://patchwork.ozlabs.org/patch/1109776/
https://patchwork.ozlabs.org/patch/1109777/
Since the patch is still WIP to make the xPCS as PHY driver for SGMII
to RGMII converter. So i decided to upstream this patch first.

Biao Huang also needed this patch and tested pass with this patch on 
their platform.
https://patchwork.ozlabs.org/patch/1103861/

Biao Huang's patch on MDIO-c45 access 
https://patchwork.ozlabs.org/patch/1092436/ 

Regards,
Weifeng

Re: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Andrew Lunn]

On Thu, Jul 04, 2019 at 01:33:16AM +0000, Voon, Weifeng wrote:
> > > @@ -155,22 +171,26 @@ static int stmmac_mdio_read(struct mii_bus *bus,
> > int phyaddr, int phyreg)
> > >  	struct stmmac_priv *priv = netdev_priv(ndev);
> > >  	unsigned int mii_address = priv->hw->mii.addr;
> > >  	unsigned int mii_data = priv->hw->mii.data;
> > > -	u32 v;
> > > -	int data;
> > >  	u32 value = MII_BUSY;
> > > +	int data = 0;
> > > +	u32 v;
> > >
> > >  	value |= (phyaddr << priv->hw->mii.addr_shift)
> > >  		& priv->hw->mii.addr_mask;
> > >  	value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw-
> > >mii.reg_mask;
> > >  	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> > >  		& priv->hw->mii.clk_csr_mask;
> > > -	if (priv->plat->has_gmac4)
> > > +	if (priv->plat->has_gmac4) {
> > >  		value |= MII_GMAC4_READ;
> > > +		if (phyreg & MII_ADDR_C45)
> > > +			stmmac_mdio_c45_setup(priv, phyreg, &value, &data);
> > > +	}
> > >
> > >  	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v &
> > MII_BUSY),
> > >  			       100, 10000))
> > >  		return -EBUSY;
> > >
> > > +	writel(data, priv->ioaddr + mii_data);
> > 
> > That looks odd. Could you explain why it is needed.
> > 
> > Thanks
> > 	Andrew
> 
> Hi Andrew,
> This mdio c45 support needed to access DWC xPCS which is a Clause-45

I mean it looks odd doing a write to the data register in the middle
of stmmac_mdio_read().

   Andrew

RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Voon, Weifeng]

> > > > @@ -155,22 +171,26 @@ static int stmmac_mdio_read(struct mii_bus
> > > > *bus,
> > > int phyaddr, int phyreg)
> > > >  	struct stmmac_priv *priv = netdev_priv(ndev);
> > > >  	unsigned int mii_address = priv->hw->mii.addr;
> > > >  	unsigned int mii_data = priv->hw->mii.data;
> > > > -	u32 v;
> > > > -	int data;
> > > >  	u32 value = MII_BUSY;
> > > > +	int data = 0;
> > > > +	u32 v;
> > > >
> > > >  	value |= (phyaddr << priv->hw->mii.addr_shift)
> > > >  		& priv->hw->mii.addr_mask;
> > > >  	value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw-
> > > >mii.reg_mask;
> > > >  	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> > > >  		& priv->hw->mii.clk_csr_mask;
> > > > -	if (priv->plat->has_gmac4)
> > > > +	if (priv->plat->has_gmac4) {
> > > >  		value |= MII_GMAC4_READ;
> > > > +		if (phyreg & MII_ADDR_C45)
> > > > +			stmmac_mdio_c45_setup(priv, phyreg, &value, &data);
> > > > +	}
> > > >
> > > >  	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v &
> > > MII_BUSY),
> > > >  			       100, 10000))
> > > >  		return -EBUSY;
> > > >
> > > > +	writel(data, priv->ioaddr + mii_data);
> > >
> > > That looks odd. Could you explain why it is needed.
> > >
> > > Thanks
> > > 	Andrew
> >
> > Hi Andrew,
> > This mdio c45 support needed to access DWC xPCS which is a Clause-45
> 
> I mean it looks odd doing a write to the data register in the middle of
> stmmac_mdio_read().

MAC is using an indirect access to access mdio devices. In order to read,
the driver needs to write into both mii_data and mii_address to select 
c45, read/write command, phy address, address to read, and etc. 

Weifeng

Re: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Andrew Lunn]

On Thu, Jul 04, 2019 at 06:05:23AM +0000, Voon, Weifeng wrote:
> > > > > @@ -155,22 +171,26 @@ static int stmmac_mdio_read(struct mii_bus
> > > > > *bus,
> > > > int phyaddr, int phyreg)
> > > > >  	struct stmmac_priv *priv = netdev_priv(ndev);
> > > > >  	unsigned int mii_address = priv->hw->mii.addr;
> > > > >  	unsigned int mii_data = priv->hw->mii.data;
> > > > > -	u32 v;
> > > > > -	int data;
> > > > >  	u32 value = MII_BUSY;
> > > > > +	int data = 0;
> > > > > +	u32 v;
> > > > >
> > > > >  	value |= (phyaddr << priv->hw->mii.addr_shift)
> > > > >  		& priv->hw->mii.addr_mask;
> > > > >  	value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw-
> > > > >mii.reg_mask;
> > > > >  	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> > > > >  		& priv->hw->mii.clk_csr_mask;
> > > > > -	if (priv->plat->has_gmac4)
> > > > > +	if (priv->plat->has_gmac4) {
> > > > >  		value |= MII_GMAC4_READ;
> > > > > +		if (phyreg & MII_ADDR_C45)
> > > > > +			stmmac_mdio_c45_setup(priv, phyreg, &value, &data);
> > > > > +	}
> > > > >
> > > > >  	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v &
> > > > MII_BUSY),
> > > > >  			       100, 10000))
> > > > >  		return -EBUSY;
> > > > >
> > > > > +	writel(data, priv->ioaddr + mii_data);
> > > >
> > > > That looks odd. Could you explain why it is needed.
> > > >
> > > > Thanks
> > > > 	Andrew
> > >
> > > Hi Andrew,
> > > This mdio c45 support needed to access DWC xPCS which is a Clause-45
> > 
> > I mean it looks odd doing a write to the data register in the middle of
> > stmmac_mdio_read().
> 
> MAC is using an indirect access to access mdio devices. In order to read,
> the driver needs to write into both mii_data and mii_address to select 
> c45, read/write command, phy address, address to read, and etc. 

Yes, that is all clear. The stmmac_mdio_c45_setup() does part of this
setup. There is also a write to mii_address which i snipped out when
replying. But why do you need to write to the data registers during a
read? C22 does not need this write. Are there some bits in the top of
the data register which are relevant to C45?

Thanks
    Andrew

RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Jose Abreu]

From: Andrew Lunn <andrew@lunn.ch>

> Yes, that is all clear. The stmmac_mdio_c45_setup() does part of this
> setup. There is also a write to mii_address which i snipped out when
> replying. But why do you need to write to the data registers during a
> read? C22 does not need this write. Are there some bits in the top of
> the data register which are relevant to C45?

Yes. The register is 32bits. 16 lower bits are for data and remaining 
for C45 register address.

RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Voon, Weifeng]

> > > > > > @@ -155,22 +171,26 @@ static int stmmac_mdio_read(struct
> > > > > > mii_bus *bus,
> > > > > int phyaddr, int phyreg)
> > > > > >  	struct stmmac_priv *priv = netdev_priv(ndev);
> > > > > >  	unsigned int mii_address = priv->hw->mii.addr;
> > > > > >  	unsigned int mii_data = priv->hw->mii.data;
> > > > > > -	u32 v;
> > > > > > -	int data;
> > > > > >  	u32 value = MII_BUSY;
> > > > > > +	int data = 0;
> > > > > > +	u32 v;
> > > > > >
> > > > > >  	value |= (phyaddr << priv->hw->mii.addr_shift)
> > > > > >  		& priv->hw->mii.addr_mask;
> > > > > >  	value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw-
> > > > > >mii.reg_mask;
> > > > > >  	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> > > > > >  		& priv->hw->mii.clk_csr_mask;
> > > > > > -	if (priv->plat->has_gmac4)
> > > > > > +	if (priv->plat->has_gmac4) {
> > > > > >  		value |= MII_GMAC4_READ;
> > > > > > +		if (phyreg & MII_ADDR_C45)
> > > > > > +			stmmac_mdio_c45_setup(priv, phyreg, &value,
> &data);
> > > > > > +	}
> > > > > >
> > > > > >  	if (readl_poll_timeout(priv->ioaddr + mii_address,
> v, !(v &
> > > > > MII_BUSY),
> > > > > >  			       100, 10000))
> > > > > >  		return -EBUSY;
> > > > > >
> > > > > > +	writel(data, priv->ioaddr + mii_data);
> > > > >
> > > > > That looks odd. Could you explain why it is needed.
> > > > >
> > > > > Thanks
> > > > > 	Andrew
> > > >
> > > > Hi Andrew,
> > > > This mdio c45 support needed to access DWC xPCS which is a
> > > > Clause-45
> > >
> > > I mean it looks odd doing a write to the data register in the middle
> > > of stmmac_mdio_read().
> >
> > MAC is using an indirect access to access mdio devices. In order to
> > read, the driver needs to write into both mii_data and mii_address to
> > select c45, read/write command, phy address, address to read, and etc.
> 
> Yes, that is all clear. The stmmac_mdio_c45_setup() does part of this
> setup. There is also a write to mii_address which i snipped out when
> replying. But why do you need to write to the data registers during a
> read? C22 does not need this write. Are there some bits in the top of
> the data register which are relevant to C45?
> 

Yes, the top 16 bit of the data register only valid when C45 is enable.
It contains the Register address which MDIO c45 frame intended for.

Re: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Andrew Lunn]

> Yes, the top 16 bit of the data register only valid when C45 is enable.
> It contains the Register address which MDIO c45 frame intended for.

I think there is too much passing variables around by reference than
by value, to make this code easy to understand.

Maybe a better structure would be

static int stmmac_mdion_c45_read(struct stmmac_priv *priv, int phyaddr, int phyreg)
{

	unsigned int reg_shift = priv->hw->mii.reg_shift;
	unsigned int reg_mask = priv->hw->mii.reg_mask;
	u32 mii_addr_val, mii_data_val;

	mii_addr_val = MII_GMAC4_C45E |
                       ((phyreg >> MII_DEVADDR_C45_SHIFT) << reg_shift) & reg_mask;
        mii_data_val = (phyreg & MII_REGADDR_C45_MASK) << MII_GMAC4_REG_ADDR_SHIFT;

	writel(mii_data_val, priv->ioaddr + priv->hw->mii_data);
	writel(mii_addr_val, priv->ioaddr + priv->hw->mii_addrress);

	return (int)readl(priv->ioaddr + mii_data) & MII_DATA_MASK;
}		   

static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
{

...
	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
 	   		      100, 10000))
 		return -EBUSY;

      if (priv->plat->has_gmac4 && phyreg & MII_ADDR_C45)
      	return stmmac_mdio_c45_read(priv, phyaddr, phyreg);

	Andrew

RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Voon, Weifeng]

> I think there is too much passing variables around by reference than by
> value, to make this code easy to understand.
> 
> Maybe a better structure would be
> 
> static int stmmac_mdion_c45_read(struct stmmac_priv *priv, int phyaddr,
> int phyreg) {
> 
> 	unsigned int reg_shift = priv->hw->mii.reg_shift;
> 	unsigned int reg_mask = priv->hw->mii.reg_mask;
> 	u32 mii_addr_val, mii_data_val;
> 
> 	mii_addr_val = MII_GMAC4_C45E |
>                        ((phyreg >> MII_DEVADDR_C45_SHIFT) << reg_shift)
> & reg_mask;
>         mii_data_val = (phyreg & MII_REGADDR_C45_MASK) <<
> MII_GMAC4_REG_ADDR_SHIFT;
> 
> 	writel(mii_data_val, priv->ioaddr + priv->hw->mii_data);
> 	writel(mii_addr_val, priv->ioaddr + priv->hw->mii_addrress);
> 
> 	return (int)readl(priv->ioaddr + mii_data) & MII_DATA_MASK;
> }
> 
> static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
> {
> 
> ...
> 	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v &
> MII_BUSY),
>  	   		      100, 10000))
>  		return -EBUSY;
> 
>       if (priv->plat->has_gmac4 && phyreg & MII_ADDR_C45)
>       	return stmmac_mdio_c45_read(priv, phyaddr, phyreg);
> 
> 	Andrew

Both c45 read/write needs to set c45 enable bit(MII_ADDR_C45) in mii_adrress
and set the register address in mii_data. Besides this, the whole programming
flow will be the same as c22. With the current design, user can easily know
that the different between c22 and c45 is just in stmmac_mdio_c45_setup(). 

If the community prefers readability, I will suggest to do the c45 setup in
both stmmac_mdio_read() and stmmac_mdio_write() 's if(C45) condition rather
than splitting into 2 new c45_read() and c45_write() functions.     

static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
{

...
	if (phyreg & MII_ADDR_C45)
       *val |= MII_GMAC4_C45E;
       *val &= ~reg_mask;
       *val |= ((phyreg >> MII_DEVADDR_C45_SHIFT) << reg_shift) & reg_mask;

       *data |= (phyreg & MII_REGADDR_C45_MASK) << MII_GMAC4_REG_ADDR_SHIFT;

Weifeng

Re: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Andrew Lunn]

> If the community prefers readability

Readability nearly always comes first. There is nothing performance
critical here, MDIO is a slow bus. So the code should be readable,
simple to understand.


, I will suggest to do the c45 setup in
> both stmmac_mdio_read() and stmmac_mdio_write() 's if(C45) condition rather
> than splitting into 2 new c45_read() and c45_write() functions.     

Fine.

	Andrew

RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

[Voon, Weifeng]

> > If the community prefers readability
> 
> Readability nearly always comes first. There is nothing performance
> critical here, MDIO is a slow bus. So the code should be readable,
> simple to understand.
> 
Noted and thanks for the comments.

> 
> , I will suggest to do the c45 setup in
> > both stmmac_mdio_read() and stmmac_mdio_write() 's if(C45) condition
> rather
> > than splitting into 2 new c45_read() and c45_write() functions.
> 
> Fine.
> 
> 	Andrew

I will start preparing v2. Thanks.  

Weifeng