[2/3] net: socionext: Add dummy PHY register read in phy_write()

Message ID 20181019010843.3605-3-masahisa.kojima@linaro.org
State Superseded
Headers show
Series
  • Bugfix for the netsec driver
Related show

Commit Message

Masahisa Kojima Oct. 19, 2018, 1:08 a.m.
From: Masahisa Kojima <masahisa.kojima@linaro.org>


There is a compatibility issue between RTL8211E implemented
in Developerbox and netsec network controller IP(F_GMAC4).

RTL8211E expects MDC clock must be kept toggling for several
clock cycle with MDIO high before entering the IDLE state.
To meet this requirement, netsec driver needs to issue dummy
read(e.g. read PHYID1(offset 0x2) register) right after write.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>

---
 drivers/net/ethernet/socionext/netsec.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

-- 
2.14.2

Comments

Florian Fainelli Oct. 19, 2018, 3:01 a.m. | #1
On 10/18/2018 6:08 PM, masahisa.kojima@linaro.org wrote:
> From: Masahisa Kojima <masahisa.kojima@linaro.org>

> 

> There is a compatibility issue between RTL8211E implemented

> in Developerbox and netsec network controller IP(F_GMAC4).

> 

> RTL8211E expects MDC clock must be kept toggling for several

> clock cycle with MDIO high before entering the IDLE state.

> To meet this requirement, netsec driver needs to issue dummy

> read(e.g. read PHYID1(offset 0x2) register) right after write.

> 

> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

> Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>

> ---

>  drivers/net/ethernet/socionext/netsec.c | 18 ++++++++++++++++--

>  1 file changed, 16 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c

> index 273cc5fc07e0..e7faaf8be99e 100644

> --- a/drivers/net/ethernet/socionext/netsec.c

> +++ b/drivers/net/ethernet/socionext/netsec.c

> @@ -431,9 +431,12 @@ static int netsec_mac_update_to_phy_state(struct netsec_priv *priv)

>  	return 0;

>  }

>  

> +static int netsec_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr);

> +

>  static int netsec_phy_write(struct mii_bus *bus,

>  			    int phy_addr, int reg, u16 val)

>  {

> +	int status;

>  	struct netsec_priv *priv = bus->priv;

>  

>  	if (netsec_mac_write(priv, GMAC_REG_GDR, val))

> @@ -446,8 +449,19 @@ static int netsec_phy_write(struct mii_bus *bus,

>  			      GMAC_REG_SHIFT_CR_GAR)))

>  		return -ETIMEDOUT;

>  

> -	return netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,

> -					  NETSEC_GMAC_GAR_REG_GB);

> +	status = netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,

> +					    NETSEC_GMAC_GAR_REG_GB);

> +

> +	/* Developerbox implements RTL8211E PHY and there is

> +	 * a compatibility problem with F_GMAC4.

> +	 * RTL8211E expects MDC clock must be kept toggling for several

> +	 * clock cycle with MDIO high before entering the IDLE state.

> +	 * To meet this requirement, netsec driver needs to issue dummy

> +	 * read(e.g. read PHYID1(offset 0x2) register) right after write.

> +	 */

> +	netsec_phy_read(bus, phy_addr, 2);


MII_PHYSID1 instead of 0x2 would be preferable. It is not clear to me
from your commit message if this is a problem specific to your MDIO
controller implementation and the RTL8211E PHY or if this is a general
problem of the PHY irrespective of the MDIO controller it is interface
with. If the latter, then we should seek a solution at a different layer
such that other systems don't have that same problem.

Thank you!
-- 
Florian
Masahisa Kojima Oct. 19, 2018, 6:44 a.m. | #2
On Fri, 19 Oct 2018 at 12:01, Florian Fainelli <f.fainelli@gmail.com> wrote:
>

>

>

> On 10/18/2018 6:08 PM, masahisa.kojima@linaro.org wrote:

> > From: Masahisa Kojima <masahisa.kojima@linaro.org>

> >

> > There is a compatibility issue between RTL8211E implemented

> > in Developerbox and netsec network controller IP(F_GMAC4).

> >

> > RTL8211E expects MDC clock must be kept toggling for several

> > clock cycle with MDIO high before entering the IDLE state.

> > To meet this requirement, netsec driver needs to issue dummy

> > read(e.g. read PHYID1(offset 0x2) register) right after write.

> >

> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

> > Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>

> > ---

> >  drivers/net/ethernet/socionext/netsec.c | 18 ++++++++++++++++--

> >  1 file changed, 16 insertions(+), 2 deletions(-)

> >

> > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c

> > index 273cc5fc07e0..e7faaf8be99e 100644

> > --- a/drivers/net/ethernet/socionext/netsec.c

> > +++ b/drivers/net/ethernet/socionext/netsec.c

> > @@ -431,9 +431,12 @@ static int netsec_mac_update_to_phy_state(struct netsec_priv *priv)

> >       return 0;

> >  }

> >

> > +static int netsec_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr);

> > +

> >  static int netsec_phy_write(struct mii_bus *bus,

> >                           int phy_addr, int reg, u16 val)

> >  {

> > +     int status;

> >       struct netsec_priv *priv = bus->priv;

> >

> >       if (netsec_mac_write(priv, GMAC_REG_GDR, val))

> > @@ -446,8 +449,19 @@ static int netsec_phy_write(struct mii_bus *bus,

> >                             GMAC_REG_SHIFT_CR_GAR)))

> >               return -ETIMEDOUT;

> >

> > -     return netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,

> > -                                       NETSEC_GMAC_GAR_REG_GB);

> > +     status = netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,

> > +                                         NETSEC_GMAC_GAR_REG_GB);

> > +

> > +     /* Developerbox implements RTL8211E PHY and there is

> > +      * a compatibility problem with F_GMAC4.

> > +      * RTL8211E expects MDC clock must be kept toggling for several

> > +      * clock cycle with MDIO high before entering the IDLE state.

> > +      * To meet this requirement, netsec driver needs to issue dummy

> > +      * read(e.g. read PHYID1(offset 0x2) register) right after write.

> > +      */

> > +     netsec_phy_read(bus, phy_addr, 2);

>

> MII_PHYSID1 instead of 0x2 would be preferable. It is not clear to me

> from your commit message if this is a problem specific to your MDIO

> controller implementation and the RTL8211E PHY or if this is a general

> problem of the PHY irrespective of the MDIO controller it is interface

> with. If the latter, then we should seek a solution at a different layer

> such that other systems don't have that same problem.

>

> Thank you!

> --

> Florian


I will replace direct value with MACROs, also for other instances.

Our MDIO controller stops MDC clock right after the write access,
but RTL8211E PHY expects to keep several clock cycle.
So it is depending on our MDIO controller implementation.

Patch

diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 273cc5fc07e0..e7faaf8be99e 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -431,9 +431,12 @@  static int netsec_mac_update_to_phy_state(struct netsec_priv *priv)
 	return 0;
 }
 
+static int netsec_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr);
+
 static int netsec_phy_write(struct mii_bus *bus,
 			    int phy_addr, int reg, u16 val)
 {
+	int status;
 	struct netsec_priv *priv = bus->priv;
 
 	if (netsec_mac_write(priv, GMAC_REG_GDR, val))
@@ -446,8 +449,19 @@  static int netsec_phy_write(struct mii_bus *bus,
 			      GMAC_REG_SHIFT_CR_GAR)))
 		return -ETIMEDOUT;
 
-	return netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,
-					  NETSEC_GMAC_GAR_REG_GB);
+	status = netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,
+					    NETSEC_GMAC_GAR_REG_GB);
+
+	/* Developerbox implements RTL8211E PHY and there is
+	 * a compatibility problem with F_GMAC4.
+	 * RTL8211E expects MDC clock must be kept toggling for several
+	 * clock cycle with MDIO high before entering the IDLE state.
+	 * To meet this requirement, netsec driver needs to issue dummy
+	 * read(e.g. read PHYID1(offset 0x2) register) right after write.
+	 */
+	netsec_phy_read(bus, phy_addr, 2);
+
+	return status;
 }
 
 static int netsec_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr)