From: Kweh Hock Leong <[email protected]>
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 <[email protected]>
Signed-off-by: Kweh Hock Leong <[email protected]>
Signed-off-by: Ong Boon Leong <[email protected]>
Signed-off-by: Weifeng Voon <[email protected]>
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
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
> > @@ -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
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
> > > > @@ -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
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
> 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
From: Andrew Lunn <[email protected]>
> 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.
> > > > > > @@ -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.
> 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
> 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
> > 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