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

[Phil Edworthy]

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 
> > > wrote:
> > > > This has been tested with a Marvell 88E1512 PHY.
> > > >
> > > > Signed-off-by: Phil Edworthy 
> > > > Reviewed-by: Stefan Roese 
> > > > ---
> > > > 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_RGMII0
> > >
> > > 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 m88es_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 m88es_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_SGMII1
> > > >  #define MIIM_88E151x_RESET_OFFS15
> > > >
> > > > @@ -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,
> > > 0x);
> > > >
> > > > -   /* 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Phil Edworthy]

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 
> > wrote:
> > > This has been tested with a Marvell 88E1512 PHY.
> > >
> > > Signed-off-by: Phil Edworthy 
> > > Reviewed-by: Stefan Roese 
> > > ---
> > > 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_RGMII0
> >
> > 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 m88es_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 m88es_config(phydev);
 }


> > >  #define MIIM_88E151x_MODE_SGMII1
> > >  #define MIIM_88E151x_RESET_OFFS15
> > >
> > > @@ -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,
> > 0x);
> > >
> > > -   /* 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Phil Edworthy]

Hi Joe,

On 09 December 2016 18:59, Joe Hershberger wrote:
> On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy 
> wrote:
> > This has been tested with a Marvell 88E1512 PHY.
> >
> > Signed-off-by: Phil Edworthy 
> > Reviewed-by: Stefan Roese 
> > ---
> > 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_RGMII0
> 
> 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_SGMII1
> >  #define MIIM_88E151x_RESET_OFFS15
> >
> > @@ -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,
> 0x);
> >
> > -   /* 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Joe Hershberger]

Hi Phil,

On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy  wrote:
> This has been tested with a Marvell 88E1512 PHY.
>
> Signed-off-by: Phil Edworthy 
> Reviewed-by: Stefan Roese 
> ---
> 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_RGMII0

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_SGMII1
>  #define MIIM_88E151x_RESET_OFFS15
>
> @@ -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, 0x);
>
> -   /* 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Phil Edworthy]

This has been tested with a Marvell 88E1512 PHY.

Signed-off-by: Phil Edworthy 
Reviewed-by: Stefan Roese 
---
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_RGMII0
 #define MIIM_88E151x_MODE_SGMII1
 #define MIIM_88E151x_RESET_OFFS15
 
@@ -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, 0x);
 
-   /* 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