diff mbox

[U-Boot,v2,3/5] net: phy: Marvell 88E151x: Add support for RGMII

Message ID 1481290705-10401-4-git-send-email-phil.edworthy@renesas.com
State Rejected
Delegated to: Joe Hershberger
Headers show

Commit Message

Phil Edworthy Dec. 9, 2016, 1:38 p.m. UTC 
This has been tested with a Marvell 88E1512 PHY.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---
v2:
 Rebased on top of Joe's code to use macros
---
 drivers/net/phy/marvell.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Joe Hershberger Dec. 9, 2016, 6:59 p.m. UTC | #1 
Hi Phil,

On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote:
> This has been tested with a Marvell 88E1512 PHY.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> ---
> v2:
>  Rebased on top of Joe's code to use macros
> ---
>  drivers/net/phy/marvell.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 5adfe7d..646b00d 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -94,6 +94,7 @@
>  #define MIIM_88E151x_INT_EN_OFFS       7
>  /* Page 18 registers */
>  #define MIIM_88E151x_GENERAL_CTRL      20
> +#define MIIM_88E151x_MODE_RGMII                0

I'm not sure why this is needed. The HW reset value for the mode is
0x7 (RGMII auto detect mode). Would it not be sufficient to simply not
change it if the mode is RGMII?

>  #define MIIM_88E151x_MODE_SGMII                1
>  #define MIIM_88E151x_RESET_OFFS                15
>
> @@ -315,14 +316,22 @@ static int m88e1518_config(struct phy_device *phydev)
>         phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);
>         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE, 0x0000);
>
> -       /* SGMII-to-Copper mode initialization */
> -       if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
> +       /* SGMII/RGMII-to-Copper mode initialization */
> +       if ((phydev->interface == PHY_INTERFACE_MODE_SGMII) ||
> +                       phy_interface_is_rgmii(phydev)) {
> +               u16 mode;
> +
> +               if (phydev->interface == PHY_INTERFACE_MODE_SGMII)
> +                       mode = MIIM_88E151x_MODE_SGMII;
> +               else
> +                       mode = MIIM_88E151x_MODE_RGMII;
> +
>                 /* Select page 18 */
>                 phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE, 18);
>
> -               /* In reg 20, write MODE[2:0] = 0x1 (SGMII to Copper) */
> +               /* In reg 20, write MODE[2:0], SGMII or RGMII to Copper */
>                 m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,
> -                                      0, 3, MIIM_88E151x_MODE_SGMII);
> +                                      0, 3, mode);
>
>                 /* PHY reset is necessary after changing MODE[2:0] */
>                 m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Phil Edworthy Dec. 12, 2016, 9:25 a.m. UTC | #2 
Hi Joe,

On 09 December 2016 18:59, Joe Hershberger wrote:
> On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy <phil.edworthy@renesas.com>

> wrote:

> > This has been tested with a Marvell 88E1512 PHY.

> >

> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

> > Reviewed-by: Stefan Roese <sr@denx.de>

> > ---

> > v2:

> >  Rebased on top of Joe's code to use macros

> > ---

> >  drivers/net/phy/marvell.c | 17 +++++++++++++----

> >  1 file changed, 13 insertions(+), 4 deletions(-)

> >

> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c

> > index 5adfe7d..646b00d 100644

> > --- a/drivers/net/phy/marvell.c

> > +++ b/drivers/net/phy/marvell.c

> > @@ -94,6 +94,7 @@

> >  #define MIIM_88E151x_INT_EN_OFFS       7

> >  /* Page 18 registers */

> >  #define MIIM_88E151x_GENERAL_CTRL      20

> > +#define MIIM_88E151x_MODE_RGMII                0

> 

> I'm not sure why this is needed. The HW reset value for the mode is

> 0x7 (RGMII auto detect mode). Would it not be sufficient to simply not

> change it if the mode is RGMII?

I don’t have a manual for any Marvell devices, just some small bits
of information. All I can tell you is that without this patch, RGMII on
1512 doesn't work.


> >  #define MIIM_88E151x_MODE_SGMII                1

> >  #define MIIM_88E151x_RESET_OFFS                15

> >

> > @@ -315,14 +316,22 @@ static int m88e1518_config(struct phy_device

> *phydev)

> >         phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);

> >         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE,

> 0x0000);

> >

> > -       /* SGMII-to-Copper mode initialization */

> > -       if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {

> > +       /* SGMII/RGMII-to-Copper mode initialization */

> > +       if ((phydev->interface == PHY_INTERFACE_MODE_SGMII) ||

> > +                       phy_interface_is_rgmii(phydev)) {

> > +               u16 mode;

> > +

> > +               if (phydev->interface == PHY_INTERFACE_MODE_SGMII)

> > +                       mode = MIIM_88E151x_MODE_SGMII;

> > +               else

> > +                       mode = MIIM_88E151x_MODE_RGMII;

> > +

> >                 /* Select page 18 */

> >                 phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE,

> 18);

> >

> > -               /* In reg 20, write MODE[2:0] = 0x1 (SGMII to Copper) */

> > +               /* In reg 20, write MODE[2:0], SGMII or RGMII to Copper */

> >                 m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,

> > -                                      0, 3, MIIM_88E151x_MODE_SGMII);

> > +                                      0, 3, mode);

> >

> >                 /* PHY reset is necessary after changing MODE[2:0] */

> >                 m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,

> > --

> > 2.7.4


Thanks
Phil
Phil Edworthy Dec. 12, 2016, 10:15 a.m. UTC | #3 
Hi Joe,

On 12 December 2016 09:25, Phil Edworthy wrote:
> On 09 December 2016 18:59, Joe Hershberger wrote:

> > On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy <phil.edworthy@renesas.com>

> > wrote:

> > > This has been tested with a Marvell 88E1512 PHY.

> > >

> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

> > > Reviewed-by: Stefan Roese <sr@denx.de>

> > > ---

> > > v2:

> > >  Rebased on top of Joe's code to use macros

> > > ---

> > >  drivers/net/phy/marvell.c | 17 +++++++++++++----

> > >  1 file changed, 13 insertions(+), 4 deletions(-)

> > >

> > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c

> > > index 5adfe7d..646b00d 100644

> > > --- a/drivers/net/phy/marvell.c

> > > +++ b/drivers/net/phy/marvell.c

> > > @@ -94,6 +94,7 @@

> > >  #define MIIM_88E151x_INT_EN_OFFS       7

> > >  /* Page 18 registers */

> > >  #define MIIM_88E151x_GENERAL_CTRL      20

> > > +#define MIIM_88E151x_MODE_RGMII                0

> >

> > I'm not sure why this is needed. The HW reset value for the mode is

> > 0x7 (RGMII auto detect mode). Would it not be sufficient to simply not

> > change it if the mode is RGMII?

> I don’t have a manual for any Marvell devices, just some small bits

> of information. All I can tell you is that without this patch, RGMII on

> 1512 doesn't work.

I messed around with the code a bit and found that I don’t need to
write to the MIIM_88E151x_GENERAL_CTRL register to set it to RGMII,
but I must do a soft reset of the PHY before calling m88e1111s_config().

Would you prefer something like this instead?
@@ -340,6 +333,9 @@ static int m88e1518_config(struct phy_device *phydev)
                udelay(100);
        }
 
+       if (phy_interface_is_rgmii(phydev))
+               phy_reset(phydev);
+
        return m88e1111s_config(phydev);
 }


> > >  #define MIIM_88E151x_MODE_SGMII                1

> > >  #define MIIM_88E151x_RESET_OFFS                15

> > >

> > > @@ -315,14 +316,22 @@ static int m88e1518_config(struct phy_device

> > *phydev)

> > >         phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);

> > >         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE,

> > 0x0000);

> > >

> > > -       /* SGMII-to-Copper mode initialization */

> > > -       if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {

> > > +       /* SGMII/RGMII-to-Copper mode initialization */

> > > +       if ((phydev->interface == PHY_INTERFACE_MODE_SGMII) ||

> > > +                       phy_interface_is_rgmii(phydev)) {

> > > +               u16 mode;

> > > +

> > > +               if (phydev->interface == PHY_INTERFACE_MODE_SGMII)

> > > +                       mode = MIIM_88E151x_MODE_SGMII;

> > > +               else

> > > +                       mode = MIIM_88E151x_MODE_RGMII;

> > > +

> > >                 /* Select page 18 */

> > >                 phy_write(phydev, MDIO_DEVAD_NONE,

> MIIM_88E1118_PHY_PAGE,

> > 18);

> > >

> > > -               /* In reg 20, write MODE[2:0] = 0x1 (SGMII to Copper) */

> > > +               /* In reg 20, write MODE[2:0], SGMII or RGMII to Copper */

> > >                 m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,

> > > -                                      0, 3, MIIM_88E151x_MODE_SGMII);

> > > +                                      0, 3, mode);

> > >

> > >                 /* PHY reset is necessary after changing MODE[2:0] */

> > >                 m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,

> > > --

> > > 2.7.4


Thanks
Phil
Phil Edworthy Dec. 12, 2016, 12:13 p.m. UTC | #4 
Hi Joe

On 12 December 2016 10:15, Phil Edworthy wrote:
> On 12 December 2016 09:25, Phil Edworthy wrote:

> > On 09 December 2016 18:59, Joe Hershberger wrote:

> > > On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy <phil.edworthy@renesas.com>

> > > wrote:

> > > > This has been tested with a Marvell 88E1512 PHY.

> > > >

> > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

> > > > Reviewed-by: Stefan Roese <sr@denx.de>

> > > > ---

> > > > v2:

> > > >  Rebased on top of Joe's code to use macros

> > > > ---

> > > >  drivers/net/phy/marvell.c | 17 +++++++++++++----

> > > >  1 file changed, 13 insertions(+), 4 deletions(-)

> > > >

> > > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c

> > > > index 5adfe7d..646b00d 100644

> > > > --- a/drivers/net/phy/marvell.c

> > > > +++ b/drivers/net/phy/marvell.c

> > > > @@ -94,6 +94,7 @@

> > > >  #define MIIM_88E151x_INT_EN_OFFS       7

> > > >  /* Page 18 registers */

> > > >  #define MIIM_88E151x_GENERAL_CTRL      20

> > > > +#define MIIM_88E151x_MODE_RGMII                0

> > >

> > > I'm not sure why this is needed. The HW reset value for the mode is

> > > 0x7 (RGMII auto detect mode). Would it not be sufficient to simply not

> > > change it if the mode is RGMII?

> > I don’t have a manual for any Marvell devices, just some small bits

> > of information. All I can tell you is that without this patch, RGMII on

> > 1512 doesn't work.

> I messed around with the code a bit and found that I don’t need to

> write to the MIIM_88E151x_GENERAL_CTRL register to set it to RGMII,

> but I must do a soft reset of the PHY before calling m88e1111s_config().

> 

> Would you prefer something like this instead?

> @@ -340,6 +333,9 @@ static int m88e1518_config(struct phy_device *phydev)

>                 udelay(100);

>         }

> 

> +       if (phy_interface_is_rgmii(phydev))

> +               phy_reset(phydev);

> +

>         return m88e1111s_config(phydev);

>  }


Sometimes I hate PHYs! My v2 patch for detecting the 1512 is wrong; the
PHY still worked to some extent since there was still power on the board.
I _thought_ the power switch was cutting power to the PHY, but no it's
not.

After fixing up the 1512 detection again, there doesn't appear to be a
problem with detecting RGMII. The PHY was only gets a 10M link,  I was
only getting a 1Gb link by forcing MIIM_88E151x_MODE bits to 0, as per
this patch. And the reason for the wrong speed is that I used the wrong
PHY mode; RGMII instead of RGMII_ID.

So the upshot of all this is that this patch is not needed at all... I'll send
a new patch that changes the uid/mask to support the 88E1512.


> > > >  #define MIIM_88E151x_MODE_SGMII                1

> > > >  #define MIIM_88E151x_RESET_OFFS                15

> > > >

> > > > @@ -315,14 +316,22 @@ static int m88e1518_config(struct phy_device

> > > *phydev)

> > > >         phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);

> > > >         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE,

> > > 0x0000);

> > > >

> > > > -       /* SGMII-to-Copper mode initialization */

> > > > -       if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {

> > > > +       /* SGMII/RGMII-to-Copper mode initialization */

> > > > +       if ((phydev->interface == PHY_INTERFACE_MODE_SGMII) ||

> > > > +                       phy_interface_is_rgmii(phydev)) {

> > > > +               u16 mode;

> > > > +

> > > > +               if (phydev->interface == PHY_INTERFACE_MODE_SGMII)

> > > > +                       mode = MIIM_88E151x_MODE_SGMII;

> > > > +               else

> > > > +                       mode = MIIM_88E151x_MODE_RGMII;

> > > > +

> > > >                 /* Select page 18 */

> > > >                 phy_write(phydev, MDIO_DEVAD_NONE,

> > MIIM_88E1118_PHY_PAGE,

> > > 18);

> > > >

> > > > -               /* In reg 20, write MODE[2:0] = 0x1 (SGMII to Copper) */

> > > > +               /* In reg 20, write MODE[2:0], SGMII or RGMII to Copper */

> > > >                 m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,

> > > > -                                      0, 3, MIIM_88E151x_MODE_SGMII);

> > > > +                                      0, 3, mode);

> > > >

> > > >                 /* PHY reset is necessary after changing MODE[2:0] */

> > > >                 m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,

> > > > --

> > > > 2.7.4


Thanks
Phil
diff mbox

Patch

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 5adfe7d..646b00d 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -94,6 +94,7 @@ 
 #define MIIM_88E151x_INT_EN_OFFS	7
 /* Page 18 registers */
 #define MIIM_88E151x_GENERAL_CTRL	20
+#define MIIM_88E151x_MODE_RGMII		0
 #define MIIM_88E151x_MODE_SGMII		1
 #define MIIM_88E151x_RESET_OFFS		15
 
@@ -315,14 +316,22 @@  static int m88e1518_config(struct phy_device *phydev)
 	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);
 	phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE, 0x0000);
 
-	/* SGMII-to-Copper mode initialization */
-	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+	/* SGMII/RGMII-to-Copper mode initialization */
+	if ((phydev->interface == PHY_INTERFACE_MODE_SGMII) ||
+			phy_interface_is_rgmii(phydev)) {
+		u16 mode;
+
+		if (phydev->interface == PHY_INTERFACE_MODE_SGMII)
+			mode = MIIM_88E151x_MODE_SGMII;
+		else
+			mode = MIIM_88E151x_MODE_RGMII;
+
 		/* Select page 18 */
 		phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE, 18);
 
-		/* In reg 20, write MODE[2:0] = 0x1 (SGMII to Copper) */
+		/* In reg 20, write MODE[2:0], SGMII or RGMII to Copper */
 		m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,
-				       0, 3, MIIM_88E151x_MODE_SGMII);
+				       0, 3, mode);
 
 		/* PHY reset is necessary after changing MODE[2:0] */
 		m88e1518_phy_writebits(phydev, MIIM_88E151x_GENERAL_CTRL,