| Message ID | 20190222201242.GA20889@lvlogina.cadence.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Cover letter: Add support for high speed MAC in Cadence controller driver | expand |
On Fri, Feb 22, 2019 at 08:12:42PM +0000, Parshuram Thombare wrote: > This patch modify MDIO read/write functions to support > communication with C45 PHY in Cadence ethernet controller driver. Hi Parshuram Are all versions of the MDIO controller capable of doing C45? Andrew
Regards, Parshuram Thombare >-----Original Message----- >From: Andrew Lunn <andrew@lunn.ch> >Sent: Saturday, February 23, 2019 3:12 AM >To: Parshuram Raju Thombare <pthombar@cadence.com> >Cc: nicolas.ferre@microchip.com; davem@davemloft.net; >netdev@vger.kernel.org; f.fainelli@gmail.com; hkallweit1@gmail.com; linux- >kernel@vger.kernel.org; Rafal Ciepiela <rafalc@cadence.com>; Piotr Sroka ><piotrs@cadence.com>; Jan Kotas <jank@cadence.com> >Subject: Re: [PATCH 2/3] net: ethernet: add c45 PHY support in MDIO read/write >functions. > >EXTERNAL MAIL > > >On Fri, Feb 22, 2019 at 08:12:42PM +0000, Parshuram Thombare wrote: >> This patch modify MDIO read/write functions to support communication >> with C45 PHY in Cadence ethernet controller driver. > >Hi Parshuram > >Are all versions of the MDIO controller capable of doing C45? > > Andrew Now driver support c22 and c45 PHY. Are you suggesting to add check for C45 PHY using is_c45 in phydev ? Regards, Parshuram Thombare
> >On Fri, Feb 22, 2019 at 08:12:42PM +0000, Parshuram Thombare wrote: > >> This patch modify MDIO read/write functions to support communication > >> with C45 PHY in Cadence ethernet controller driver. > > > >Hi Parshuram > > > >Are all versions of the MDIO controller capable of doing C45? > > > > Andrew > Now driver support c22 and c45 PHY. > Are you suggesting to add check for C45 PHY using is_c45 in phydev ? You are unconditionally supporting C45. Are there versions of the hardware which don't actually support C45? You have this endless loop: + /* wait for end of transfer */ + while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) + cpu_relax(); If there is hardware which does not support C45, will this loop forever? Andrew
On Fri, Feb 22, 2019 at 08:12:42PM +0000, Parshuram Thombare wrote: > This patch modify MDIO read/write functions to support > communication with C45 PHY in Cadence ethernet controller driver. > > Signed-off-by: Parshuram Thombare <pthombar@cadence.com> > --- > drivers/net/ethernet/cadence/macb.h | 15 +++++-- > drivers/net/ethernet/cadence/macb_main.c | 61 ++++++++++++++++++++++++----- > 2 files changed, 61 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > index bed4ded..59c23e0 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -636,10 +636,17 @@ > #define GEM_CLK_DIV96 5 > > /* Constants for MAN register */ > -#define MACB_MAN_SOF 1 > -#define MACB_MAN_WRITE 1 > -#define MACB_MAN_READ 2 > -#define MACB_MAN_CODE 2 > +#define MACB_MAN_C22_SOF 1 > +#define MACB_MAN_C22_WRITE 1 > +#define MACB_MAN_C22_READ 2 > +#define MACB_MAN_C22_CODE 2 > + > +#define MACB_MAN_C45_SOF 0 > +#define MACB_MAN_C45_ADDR 0 > +#define MACB_MAN_C45_WRITE 1 > +#define MACB_MAN_C45_POST_READ_INCR 2 > +#define MACB_MAN_C45_READ 3 > +#define MACB_MAN_C45_CODE 2 > > /* Capability mask bits */ > #define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x00000001 > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 4f4f8e5..2494abf 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -323,11 +323,30 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum) > struct macb *bp = bus->priv; > int value; > > - macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF) > - | MACB_BF(RW, MACB_MAN_READ) > - | MACB_BF(PHYA, mii_id) > - | MACB_BF(REGA, regnum) > - | MACB_BF(CODE, MACB_MAN_CODE))); > + if (regnum & MII_ADDR_C45) { > + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF) > + | MACB_BF(RW, MACB_MAN_C45_ADDR) > + | MACB_BF(PHYA, mii_id) > + | MACB_BF(REGA, (regnum >> 16) & 0x1F) > + | MACB_BF(DATA, regnum & 0xFFFF) > + | MACB_BF(CODE, MACB_MAN_C45_CODE))); > + > + /* wait for end of transfer */ > + while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) > + cpu_relax(); You need a timeout here, and anywhere you wait for the hardware to complete. Try to make use of readx_poll_timeout() variants. Andrew
>> >On Fri, Feb 22, 2019 at 08:12:42PM +0000, Parshuram Thombare wrote: >> >> This patch modify MDIO read/write functions to support >> >> communication with C45 PHY in Cadence ethernet controller driver. >> > >> >Hi Parshuram >> > >> >Are all versions of the MDIO controller capable of doing C45? >> > >> > Andrew >> Now driver support c22 and c45 PHY. >> Are you suggesting to add check for C45 PHY using is_c45 in phydev ? > >You are unconditionally supporting C45. Are there versions of the hardware which >don't actually support C45? You have this endless loop: There is controller which don't support C45. I will add check for that using is_c45. > >+ /* wait for end of transfer */ >+ while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) >+ cpu_relax(); > >If there is hardware which does not support C45, will this loop forever? > > Andrew Yes, this bit is supposed to be set. I will add timeout here. Regards, Parshuram Thombare
>On Fri, Feb 22, 2019 at 08:12:42PM +0000, Parshuram Thombare wrote: >> This patch modify MDIO read/write functions to support communication >> with C45 PHY in Cadence ethernet controller driver. >> >> Signed-off-by: Parshuram Thombare <pthombar@cadence.com> >> --- >> drivers/net/ethernet/cadence/macb.h | 15 +++++-- >> drivers/net/ethernet/cadence/macb_main.c | 61 >++++++++++++++++++++++++----- >> 2 files changed, 61 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb.h >> b/drivers/net/ethernet/cadence/macb.h >> index bed4ded..59c23e0 100644 >> --- a/drivers/net/ethernet/cadence/macb.h >> +++ b/drivers/net/ethernet/cadence/macb.h >> @@ -636,10 +636,17 @@ >> #define GEM_CLK_DIV96 5 >> >> /* Constants for MAN register */ >> -#define MACB_MAN_SOF 1 >> -#define MACB_MAN_WRITE 1 >> -#define MACB_MAN_READ 2 >> -#define MACB_MAN_CODE 2 >> +#define MACB_MAN_C22_SOF 1 >> +#define MACB_MAN_C22_WRITE 1 >> +#define MACB_MAN_C22_READ 2 >> +#define MACB_MAN_C22_CODE 2 >> + >> +#define MACB_MAN_C45_SOF 0 >> +#define MACB_MAN_C45_ADDR 0 >> +#define MACB_MAN_C45_WRITE 1 >> +#define MACB_MAN_C45_POST_READ_INCR 2 >> +#define MACB_MAN_C45_READ 3 >> +#define MACB_MAN_C45_CODE 2 >> >> /* Capability mask bits */ >> #define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x00000001 >> diff --git a/drivers/net/ethernet/cadence/macb_main.c >> b/drivers/net/ethernet/cadence/macb_main.c >> index 4f4f8e5..2494abf 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -323,11 +323,30 @@ static int macb_mdio_read(struct mii_bus *bus, int >mii_id, int regnum) >> struct macb *bp = bus->priv; >> int value; >> >> - macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF) >> - | MACB_BF(RW, MACB_MAN_READ) >> - | MACB_BF(PHYA, mii_id) >> - | MACB_BF(REGA, regnum) >> - | MACB_BF(CODE, MACB_MAN_CODE))); >> + if (regnum & MII_ADDR_C45) { >> + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF) >> + | MACB_BF(RW, MACB_MAN_C45_ADDR) >> + | MACB_BF(PHYA, mii_id) >> + | MACB_BF(REGA, (regnum >> 16) & 0x1F) >> + | MACB_BF(DATA, regnum & 0xFFFF) >> + | MACB_BF(CODE, MACB_MAN_C45_CODE))); >> + >> + /* wait for end of transfer */ >> + while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) >> + cpu_relax(); > >You need a timeout here, and anywhere you wait for the hardware to complete. >Try to make use of readx_poll_timeout() variants. > > Andrew Yes, I will add timeout here. Regards, Parshuram Thombare
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index bed4ded..59c23e0 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -636,10 +636,17 @@ #define GEM_CLK_DIV96 5 /* Constants for MAN register */ -#define MACB_MAN_SOF 1 -#define MACB_MAN_WRITE 1 -#define MACB_MAN_READ 2 -#define MACB_MAN_CODE 2 +#define MACB_MAN_C22_SOF 1 +#define MACB_MAN_C22_WRITE 1 +#define MACB_MAN_C22_READ 2 +#define MACB_MAN_C22_CODE 2 + +#define MACB_MAN_C45_SOF 0 +#define MACB_MAN_C45_ADDR 0 +#define MACB_MAN_C45_WRITE 1 +#define MACB_MAN_C45_POST_READ_INCR 2 +#define MACB_MAN_C45_READ 3 +#define MACB_MAN_C45_CODE 2 /* Capability mask bits */ #define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x00000001 diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 4f4f8e5..2494abf 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -323,11 +323,30 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum) struct macb *bp = bus->priv; int value; - macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF) - | MACB_BF(RW, MACB_MAN_READ) - | MACB_BF(PHYA, mii_id) - | MACB_BF(REGA, regnum) - | MACB_BF(CODE, MACB_MAN_CODE))); + if (regnum & MII_ADDR_C45) { + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF) + | MACB_BF(RW, MACB_MAN_C45_ADDR) + | MACB_BF(PHYA, mii_id) + | MACB_BF(REGA, (regnum >> 16) & 0x1F) + | MACB_BF(DATA, regnum & 0xFFFF) + | MACB_BF(CODE, MACB_MAN_C45_CODE))); + + /* wait for end of transfer */ + while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) + cpu_relax(); + + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF) + | MACB_BF(RW, MACB_MAN_C45_READ) + | MACB_BF(PHYA, mii_id) + | MACB_BF(REGA, (regnum >> 16) & 0x1F) + | MACB_BF(CODE, MACB_MAN_C45_CODE))); + } else { + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C22_SOF) + | MACB_BF(RW, MACB_MAN_C22_READ) + | MACB_BF(PHYA, mii_id) + | MACB_BF(REGA, regnum) + | MACB_BF(CODE, MACB_MAN_C22_CODE))); + } /* wait for end of transfer */ while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) @@ -343,12 +362,32 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum, { struct macb *bp = bus->priv; - macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF) - | MACB_BF(RW, MACB_MAN_WRITE) - | MACB_BF(PHYA, mii_id) - | MACB_BF(REGA, regnum) - | MACB_BF(CODE, MACB_MAN_CODE) - | MACB_BF(DATA, value))); + if (regnum & MII_ADDR_C45) { + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF) + | MACB_BF(RW, MACB_MAN_C45_ADDR) + | MACB_BF(PHYA, mii_id) + | MACB_BF(REGA, (regnum >> 16) & 0x1F) + | MACB_BF(DATA, regnum & 0xFFFF) + | MACB_BF(CODE, MACB_MAN_C45_CODE))); + + /* wait for end of transfer */ + while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) + cpu_relax(); + + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF) + | MACB_BF(RW, MACB_MAN_C45_WRITE) + | MACB_BF(PHYA, mii_id) + | MACB_BF(REGA, (regnum >> 16) & 0x1F) + | MACB_BF(CODE, MACB_MAN_C45_CODE) + | MACB_BF(DATA, value))); //Data + } else { + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C22_SOF) + | MACB_BF(RW, MACB_MAN_C22_WRITE) + | MACB_BF(PHYA, mii_id) + | MACB_BF(REGA, regnum) + | MACB_BF(CODE, MACB_MAN_C22_CODE) + | MACB_BF(DATA, value))); + } /* wait for end of transfer */ while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
This patch modify MDIO read/write functions to support communication with C45 PHY in Cadence ethernet controller driver. Signed-off-by: Parshuram Thombare <pthombar@cadence.com> --- drivers/net/ethernet/cadence/macb.h | 15 +++++-- drivers/net/ethernet/cadence/macb_main.c | 61 ++++++++++++++++++++++++----- 2 files changed, 61 insertions(+), 15 deletions(-)