[U-Boot] [PATCH v2 0/5] net: phy: Add Marvell M88E1512
These patches add support for the Marvell M88E1512 PHY.
Phil Edworthy (5): net: phy: Fix mask so that we can identify Marvell 88E1518 net: phy: Add support for Marvell M88E1512 net: phy: Marvell 88E151x: Add support for RGMII net: phy: Marvell 88E151x: Increase delay after init net: phy: Marvell: Use phy_interface_is_rgmii helper function
drivers/net/phy/marvell.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
The mask for the 88E1510 meant that the 88E1518 code would never be used.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com Reviewed-by: Stefan Roese sr@denx.de --- Note: This has only been tested on a board that uses a Marvell 88E1512 PHY, see subsequent patches.
v2: No changes --- drivers/net/phy/marvell.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index ac13a67..95d1492 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -578,7 +578,7 @@ static struct phy_driver M88E1149S_driver = { static struct phy_driver M88E1510_driver = { .name = "Marvell 88E1510", .uid = 0x1410dd0, - .mask = 0xffffff0, + .mask = 0xfffffff, .features = PHY_GBIT_FEATURES, .config = &m88e1510_config, .startup = &m88e1011s_startup, @@ -588,7 +588,7 @@ static struct phy_driver M88E1510_driver = { static struct phy_driver M88E1518_driver = { .name = "Marvell 88E1518", .uid = 0x1410dd1, - .mask = 0xffffff0, + .mask = 0xfffffff, .features = PHY_GBIT_FEATURES, .config = &m88e1518_config, .startup = &m88e1011s_startup,
On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy phil.edworthy@renesas.com wrote:
The mask for the 88E1510 meant that the 88E1518 code would never be used.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com Reviewed-by: Stefan Roese sr@denx.de
Acked-by: Joe Hershberger joe.hershberger@ni.com
This device also works with the 88E1518 code, so we just adjust the UID mask accordingly.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com --- v2: Don't add a new entry, just adjust the UID mask. --- drivers/net/phy/marvell.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 95d1492..5adfe7d 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -585,10 +585,14 @@ static struct phy_driver M88E1510_driver = { .shutdown = &genphy_shutdown, };
+/* This supports: + * 88E1518, uid 0x1410dd1 + * 88E1512, uid 0x1410dd4 + */ static struct phy_driver M88E1518_driver = { .name = "Marvell 88E1518", .uid = 0x1410dd1, - .mask = 0xfffffff, + .mask = 0xffffffb, .features = PHY_GBIT_FEATURES, .config = &m88e1518_config, .startup = &m88e1011s_startup,
On 09.12.2016 14:38, Phil Edworthy wrote:
This device also works with the 88E1518 code, so we just adjust the UID mask accordingly.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
v2: Don't add a new entry, just adjust the UID mask.
drivers/net/phy/marvell.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 95d1492..5adfe7d 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -585,10 +585,14 @@ static struct phy_driver M88E1510_driver = { .shutdown = &genphy_shutdown, };
+/* This supports:
- 88E1518, uid 0x1410dd1
- 88E1512, uid 0x1410dd4
- */
Nitpicking comment:
Please use this multiline comment style:
/* * This supports: * 88E1518, uid 0x1410dd1 * 88E1512, uid 0x1410dd4 */
static struct phy_driver M88E1518_driver = { .name = "Marvell 88E1518", .uid = 0x1410dd1,
- .mask = 0xfffffff,
- .mask = 0xffffffb, .features = PHY_GBIT_FEATURES, .config = &m88e1518_config, .startup = &m88e1011s_startup,
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
On Fri, Dec 9, 2016 at 7:43 AM, Stefan Roese sr@denx.de wrote:
On 09.12.2016 14:38, Phil Edworthy wrote:
This device also works with the 88E1518 code, so we just adjust the UID mask accordingly.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
v2: Don't add a new entry, just adjust the UID mask.
Alright. I assume we will see what other IDs they come out with.
Acked-by: Joe Hershberger joe.hershberger@ni.com
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 #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,
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;elsemode = 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
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;elsemode = 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
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;elsemode = 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
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;elsemode = 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
On Marvell 88E1512, the delay is not enough when connected to some external switches (e.g. Netgear GS108E).
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com Reviewed-by: Stefan Roese sr@denx.de --- v2: No changes --- drivers/net/phy/marvell.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 646b00d..b6c005f 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -340,7 +340,7 @@ static int m88e1518_config(struct phy_device *phydev) /* Reset page selection */ phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE, 0);
- udelay(100); + udelay(500); }
return m88e1111s_config(phydev);
On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy phil.edworthy@renesas.com wrote:
On Marvell 88E1512, the delay is not enough when connected to some external switches (e.g. Netgear GS108E).
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com Reviewed-by: Stefan Roese sr@denx.de
Acked-by: Joe Hershberger joe.hershberger@ni.com
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com Reviewed-by: Stefan Roese sr@denx.de --- v2: No changes --- drivers/net/phy/marvell.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index b6c005f..f640a81 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -193,10 +193,7 @@ static int m88e1111s_config(struct phy_device *phydev) { int reg;
- if ((phydev->interface == PHY_INTERFACE_MODE_RGMII) || - (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) || - (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) || - (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)) { + if (phy_interface_is_rgmii(phydev)) { reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_88E1111_PHY_EXT_CR); if ((phydev->interface == PHY_INTERFACE_MODE_RGMII) ||
On Fri, Dec 9, 2016 at 7:38 AM, Phil Edworthy phil.edworthy@renesas.com wrote:
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com Reviewed-by: Stefan Roese sr@denx.de
Acked-by: Joe Hershberger joe.hershberger@ni.com
participants (3)
-
Joe Hershberger -
Phil Edworthy -
Stefan Roese