[RFC Patch] arm: kirkwood: nsa310s: Use Marvell uclass mvgbe and PHY driver for DM Ethernet
Hi all,
This is a work-in-progress patch that I'm working on, to clean up the DM Ethernet code for the Zyxel NSA310S board (Kirkwood 88F6702 A1).
This NSA310s board Ethernet has some quirks that it does not work quite the same way as other Kirwood boards with the similar network chip (MV88E1318), such as the Sheevaplug and the Dreamplug.
Currently, in the NSA310S board file we use CONFIG_RESET_PHY_R to execute reset_phy() during initialization. And the reset_phy() code in this board file is ad-hoc, does not involve Marvell PHY driver (which it should be). So I'm following the same pattern that I've done for the Sheevaplug: use the uclass drivers/net/mvgbe.c (CONFIG_MVGBE) to bring up Ethernet, removing the reset_phy() code, and enable CONFIG_PHY_MARVEL.
With this board, I could not get it to work the same way as for the Sheevaplug to bring up the PHY automatically. I had to insert the ad-hoc code to set RGMII delay in the __mvgbe_phy_init() function in mvgbe.c, so the phy_connect() call will find the PHY. The PHY Id is 1410e90, same as in the Sheevaplug and Dreamplug. Please see this in the patch below (it's only a hack to see the PHY can be brought up this way).
With the Sheevaplug and Dreamplug, there is no need to do this hack. The uclass mvgbe did all the work, by the time phy_connect() is executed, the PHY was already up and working fine. And then the Marvell PHY driver kicks in, setting up the rest.
Perhaps we need code in the uclass MVGBE that handles this in a generic way? These are the Marvell PHY driver functions invoked after phy_connect() was successful. This happens in all 3 boards (Sheevaplug, Dreamplug, and NSA310s).
m88e1310_config m88e1011s_startup
I would appreciate hearing some explanation and perhaps some suggestion/guidance about this topic.
Thanks, Tony
diff --git a/board/zyxel/nsa310s/nsa310s.c b/board/zyxel/nsa310s/nsa310s.c index b71de4e11f..78d472d56c 100644 --- a/board/zyxel/nsa310s/nsa310s.c +++ b/board/zyxel/nsa310s/nsa310s.c @@ -1,22 +1,49 @@ // SPDX-License-Identifier: GPL-2.0+ /* - * Copyright (C) 2015, 2021 Tony Dinh mibodhi@gmail.com + * Copyright (C) 2015, 2021-2022 Tony Dinh mibodhi@gmail.com * Copyright (C) 2015 Gerald Kerma dreagle@doukki.net */
#include <common.h> #include <init.h> -#include <miiphy.h> -#include <net.h> +#include <netdev.h> #include <asm/arch/cpu.h> #include <asm/arch/soc.h> #include <asm/arch/mpp.h> #include <asm/global_data.h> #include <asm/io.h> -#include "nsa310s.h" +#include <linux/bitops.h>
DECLARE_GLOBAL_DATA_PTR;
+/* + * low GPIO's + */ +#define HDD1_GREEN_LED BIT(16) +#define HDD1_RED_LED BIT(13) +#define USB_GREEN_LED BIT(15) +#define USB_POWER BIT(21) +#define SYS_GREEN_LED BIT(28) +#define SYS_ORANGE_LED BIT(29) + +#define COPY_GREEN_LED BIT(22) +#define COPY_RED_LED BIT(23) + +#define PIN_USB_GREEN_LED 15 +#define PIN_USB_POWER 21 + +#define NSA310S_OE_LOW (~(0)) +#define NSA310S_VAL_LOW (SYS_GREEN_LED | USB_POWER) + +/* + * high GPIO's +*/ +#define HDD2_GREEN_LED BIT(2) +#define HDD2_POWER BIT(1) + +#define NSA310S_OE_HIGH (~(0)) +#define NSA310S_VAL_HIGH (HDD2_POWER) + int board_early_init_f(void) { /* @@ -80,87 +107,7 @@ int board_init(void) return 0; }
-static int fdt_get_phy_addr(const char *path) +int board_eth_init(struct bd_info *bis) { - const void *fdt = gd->fdt_blob; - const u32 *reg; - const u32 *val; - int node, phandle, addr; - - /* Find the node by its full path */ - node = fdt_path_offset(fdt, path); - if (node >= 0) { - /* Look up phy-handle */ - val = fdt_getprop(fdt, node, "phy-handle", NULL); - if (val) { - phandle = fdt32_to_cpu(*val); - if (!phandle) - return -1; - /* Follow it to its node */ - node = fdt_node_offset_by_phandle(fdt, phandle); - if (node) { - /* Look up reg */ - reg = fdt_getprop(fdt, node, "reg", NULL); - if (reg) { - addr = fdt32_to_cpu(*reg); - return addr; - } - } - } - } - return -1; -} - -#ifdef CONFIG_RESET_PHY_R -void reset_phy(void) -{ - u16 reg; - u16 phyaddr; - char *name = "ethernet-controller@72000"; - char *eth0_path = "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0"; - - if (miiphy_set_current_dev(name)) - return; - - phyaddr = fdt_get_phy_addr(eth0_path); - if (phyaddr < 0) - return; - - /* set RGMII delay */ - miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, MV88E1318_MAC_CTRL_PG); - miiphy_read(name, phyaddr, MV88E1318_MAC_CTRL_REG, ®); - reg |= (MV88E1318_RGMII_RX_CTRL | MV88E1318_RGMII_TX_CTRL); - miiphy_write(name, phyaddr, MV88E1318_MAC_CTRL_REG, reg); - miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, 0); - - /* reset PHY */ - if (miiphy_reset(name, phyaddr)) - return; - - /* - * ZyXEL NSA310S uses the 88E1310S Alaska (interface identical to 88E1318) - * and has an MCU attached to the LED[2] via tristate interrupt - */ - - /* switch to LED register page */ - miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, MV88E1318_LED_PG); - /* read out LED polarity register */ - miiphy_read(name, phyaddr, MV88E1318_LED_POL_REG, ®); - /* clear 4, set 5 - LED2 low, tri-state */ - reg &= ~(MV88E1318_LED2_4); - reg |= (MV88E1318_LED2_5); - /* write back LED polarity register */ - miiphy_write(name, phyaddr, MV88E1318_LED_POL_REG, reg); - /* jump back to page 0, per the PHY chip documenation. */ - miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, 0); - - /* set PHY back to auto-negotiation mode */ - miiphy_write(name, phyaddr, 0x4, 0x1e1); - miiphy_write(name, phyaddr, 0x9, 0x300); - /* downshift */ - miiphy_write(name, phyaddr, 0x10, 0x3860); - miiphy_write(name, phyaddr, 0x0, 0x9140); - - printf("MV88E1318 PHY initialized on %s\n", name); + return cpu_eth_init(bis); } -#endif /* CONFIG_RESET_PHY_R */ diff --git a/configs/nsa310s_defconfig b/configs/nsa310s_defconfig index 05a6761854..07c96489c0 100644 --- a/configs/nsa310s_defconfig +++ b/configs/nsa310s_defconfig @@ -2,6 +2,7 @@ CONFIG_ARM=y CONFIG_SKIP_LOWLEVEL_INIT=y CONFIG_SYS_DCACHE_OFF=y CONFIG_ARCH_CPU_INIT=y +CONFIG_SYS_THUMB_BUILD=y CONFIG_ARCH_KIRKWOOD=y CONFIG_SYS_KWD_CONFIG="board/zyxel/nsa310s/kwbimage.cfg" CONFIG_SYS_TEXT_BASE=0x600000 @@ -31,6 +32,7 @@ CONFIG_CMD_PING=y CONFIG_CMD_EXT2=y CONFIG_CMD_FAT=y CONFIG_CMD_JFFS2=y +CONFIG_CMD_FS_GENERIC=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDPARTS_DEFAULT="mtdparts=orion_nand:0xe0000@0x0(uboot),0x20000@0xe0000(uboot_env),0x100000@0x100000(second_stage_uboot),-@0x200000(root)" CONFIG_CMD_UBI=y @@ -47,6 +49,7 @@ CONFIG_SYS_SATA_MAX_DEVICE=1 # CONFIG_MMC is not set CONFIG_MTD=y CONFIG_MTD_RAW_NAND=y +CONFIG_PHY_MARVELL=y CONFIG_DM_ETH=y CONFIG_MVGBE=y CONFIG_MII=y diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c index 954bf86121..9aff72bc74 100644 --- a/drivers/net/mvgbe.c +++ b/drivers/net/mvgbe.c @@ -726,6 +726,13 @@ static int mvgbe_recv(struct eth_device *dev) } #endif
+/* PHY related */ +#define MV88E1318_PGADR_REG 22 +#define MV88E1318_MAC_CTRL_PG 2 +#define MV88E1318_MAC_CTRL_REG 21 +#define MV88E1318_RGMII_TX_CTRL (1 << 4) +#define MV88E1318_RGMII_RX_CTRL (1 << 5) + #if defined(CONFIG_PHYLIB) || defined(CONFIG_DM_ETH) #if defined(CONFIG_DM_ETH) static struct phy_device *__mvgbe_phy_init(struct udevice *dev, @@ -740,11 +747,19 @@ static struct phy_device *__mvgbe_phy_init(struct eth_device *dev, #endif { struct phy_device *phydev; + int reg;
/* Set phy address of the port */ miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST, phyid);
+ /* Set RGMII delay */ + miiphy_write(dev->name, phyid, MV88E1318_PGADR_REG, MV88E1318_MAC_CTRL_PG); + miiphy_read(dev->name, phyid, MV88E1318_MAC_CTRL_REG, ®); + reg |= (MV88E1318_RGMII_RX_CTRL | MV88E1318_RGMII_TX_CTRL); + miiphy_write(dev->name, phyid, MV88E1318_MAC_CTRL_REG, reg); + miiphy_write(dev->name, phyid, MV88E1318_PGADR_REG, 0); + phydev = phy_connect(bus, phyid, dev, phy_interface); if (!phydev) { printf("phy_connect failed\n");
Hi Tony,
On 4/8/22 07:03, Tony Dinh wrote:
Hi all,
This is a work-in-progress patch that I'm working on, to clean up the DM Ethernet code for the Zyxel NSA310S board (Kirkwood 88F6702 A1).
This NSA310s board Ethernet has some quirks that it does not work quite the same way as other Kirwood boards with the similar network chip (MV88E1318), such as the Sheevaplug and the Dreamplug.
Currently, in the NSA310S board file we use CONFIG_RESET_PHY_R to execute reset_phy() during initialization. And the reset_phy() code in this board file is ad-hoc, does not involve Marvell PHY driver (which it should be). So I'm following the same pattern that I've done for the Sheevaplug: use the uclass drivers/net/mvgbe.c (CONFIG_MVGBE) to bring up Ethernet, removing the reset_phy() code, and enable CONFIG_PHY_MARVEL.
With this board, I could not get it to work the same way as for the Sheevaplug to bring up the PHY automatically. I had to insert the ad-hoc code to set RGMII delay in the __mvgbe_phy_init() function in mvgbe.c, so the phy_connect() call will find the PHY. The PHY Id is 1410e90, same as in the Sheevaplug and Dreamplug. Please see this in the patch below (it's only a hack to see the PHY can be brought up this way).
Looking at drivers/net/phy/marvell.c I see that m88e1310_config() does exactly this rx/tx delay configuration:
/* Marvell 88E1310 */ static int m88e1310_config(struct phy_device *phydev) { ... /* Set RGMII delay */ phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_PAGE, 0x0002); reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_RGMII_CTRL); reg |= 0x0030; phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_RGMII_CTRL, reg);
Are you sure that this functions is not called while probing / enabling the network interface?
With the Sheevaplug and Dreamplug, there is no need to do this hack. The uclass mvgbe did all the work, by the time phy_connect() is executed, the PHY was already up and working fine. And then the Marvell PHY driver kicks in, setting up the rest.
Perhaps we need code in the uclass MVGBE that handles this in a generic way? These are the Marvell PHY driver functions invoked after phy_connect() was successful. This happens in all 3 boards (Sheevaplug, Dreamplug, and NSA310s).
m88e1310_config m88e1011s_startup
Ah okay, It's executed.
I would appreciate hearing some explanation and perhaps some suggestion/guidance about this topic.
If all 3 boards use the same ethernet driver with the same configuration (e.g. CONFIG_PHYLIB, CONFIG_EM_ETH etc) and the same PHY, there should of course be no need to handle this differently for this Zyxel board. I suggest to continue debugging to see, why this really is necessary. Perhaps some soft-reset of the PHY clears this rx/tx delay again?
Additionally your patch seems to be indented incorrectly. This makes reviewing a bit harder.
Thanks, Stefan
Thanks, Tony
diff --git a/board/zyxel/nsa310s/nsa310s.c b/board/zyxel/nsa310s/nsa310s.c index b71de4e11f..78d472d56c 100644 --- a/board/zyxel/nsa310s/nsa310s.c +++ b/board/zyxel/nsa310s/nsa310s.c @@ -1,22 +1,49 @@ // SPDX-License-Identifier: GPL-2.0+ /*
- Copyright (C) 2015, 2021 Tony Dinh mibodhi@gmail.com
- Copyright (C) 2015, 2021-2022 Tony Dinh mibodhi@gmail.com
- Copyright (C) 2015 Gerald Kerma dreagle@doukki.net
*/
#include <common.h> #include <init.h>
-#include <miiphy.h> -#include <net.h> +#include <netdev.h> #include <asm/arch/cpu.h> #include <asm/arch/soc.h> #include <asm/arch/mpp.h> #include <asm/global_data.h> #include <asm/io.h> -#include "nsa310s.h" +#include <linux/bitops.h>
DECLARE_GLOBAL_DATA_PTR;
+/*
- low GPIO's
- */
+#define HDD1_GREEN_LED BIT(16) +#define HDD1_RED_LED BIT(13) +#define USB_GREEN_LED BIT(15) +#define USB_POWER BIT(21) +#define SYS_GREEN_LED BIT(28) +#define SYS_ORANGE_LED BIT(29)
+#define COPY_GREEN_LED BIT(22) +#define COPY_RED_LED BIT(23)
+#define PIN_USB_GREEN_LED 15 +#define PIN_USB_POWER 21
+#define NSA310S_OE_LOW (~(0)) +#define NSA310S_VAL_LOW (SYS_GREEN_LED | USB_POWER)
+/*
- high GPIO's
+*/ +#define HDD2_GREEN_LED BIT(2) +#define HDD2_POWER BIT(1)
+#define NSA310S_OE_HIGH (~(0)) +#define NSA310S_VAL_HIGH (HDD2_POWER)
- int board_early_init_f(void) { /*
@@ -80,87 +107,7 @@ int board_init(void) return 0; }
-static int fdt_get_phy_addr(const char *path) +int board_eth_init(struct bd_info *bis) {
- const void *fdt = gd->fdt_blob;
- const u32 *reg;
- const u32 *val;
- int node, phandle, addr;
- /* Find the node by its full path */
- node = fdt_path_offset(fdt, path);
- if (node >= 0) {
- /* Look up phy-handle */
- val = fdt_getprop(fdt, node, "phy-handle", NULL);
- if (val) {
- phandle = fdt32_to_cpu(*val);
- if (!phandle)
- return -1;
- /* Follow it to its node */
- node = fdt_node_offset_by_phandle(fdt, phandle);
- if (node) {
- /* Look up reg */
- reg = fdt_getprop(fdt, node, "reg", NULL);
- if (reg) {
- addr = fdt32_to_cpu(*reg);
- return addr;
- }
- }
- }
- }
- return -1;
-}
-#ifdef CONFIG_RESET_PHY_R -void reset_phy(void) -{
- u16 reg;
- u16 phyaddr;
- char *name = "ethernet-controller@72000";
- char *eth0_path = "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
- if (miiphy_set_current_dev(name))
- return;
- phyaddr = fdt_get_phy_addr(eth0_path);
- if (phyaddr < 0)
- return;
- /* set RGMII delay */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, MV88E1318_MAC_CTRL_PG);
- miiphy_read(name, phyaddr, MV88E1318_MAC_CTRL_REG, ®);
- reg |= (MV88E1318_RGMII_RX_CTRL | MV88E1318_RGMII_TX_CTRL);
- miiphy_write(name, phyaddr, MV88E1318_MAC_CTRL_REG, reg);
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, 0);
- /* reset PHY */
- if (miiphy_reset(name, phyaddr))
- return;
- /*
- ZyXEL NSA310S uses the 88E1310S Alaska (interface identical to 88E1318)
- and has an MCU attached to the LED[2] via tristate interrupt
- */
- /* switch to LED register page */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, MV88E1318_LED_PG);
- /* read out LED polarity register */
- miiphy_read(name, phyaddr, MV88E1318_LED_POL_REG, ®);
- /* clear 4, set 5 - LED2 low, tri-state */
- reg &= ~(MV88E1318_LED2_4);
- reg |= (MV88E1318_LED2_5);
- /* write back LED polarity register */
- miiphy_write(name, phyaddr, MV88E1318_LED_POL_REG, reg);
- /* jump back to page 0, per the PHY chip documenation. */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, 0);
- /* set PHY back to auto-negotiation mode */
- miiphy_write(name, phyaddr, 0x4, 0x1e1);
- miiphy_write(name, phyaddr, 0x9, 0x300);
- /* downshift */
- miiphy_write(name, phyaddr, 0x10, 0x3860);
- miiphy_write(name, phyaddr, 0x0, 0x9140);
- printf("MV88E1318 PHY initialized on %s\n", name);
- return cpu_eth_init(bis); }
-#endif /* CONFIG_RESET_PHY_R */ diff --git a/configs/nsa310s_defconfig b/configs/nsa310s_defconfig index 05a6761854..07c96489c0 100644 --- a/configs/nsa310s_defconfig +++ b/configs/nsa310s_defconfig @@ -2,6 +2,7 @@ CONFIG_ARM=y CONFIG_SKIP_LOWLEVEL_INIT=y CONFIG_SYS_DCACHE_OFF=y CONFIG_ARCH_CPU_INIT=y +CONFIG_SYS_THUMB_BUILD=y CONFIG_ARCH_KIRKWOOD=y CONFIG_SYS_KWD_CONFIG="board/zyxel/nsa310s/kwbimage.cfg" CONFIG_SYS_TEXT_BASE=0x600000 @@ -31,6 +32,7 @@ CONFIG_CMD_PING=y CONFIG_CMD_EXT2=y CONFIG_CMD_FAT=y CONFIG_CMD_JFFS2=y +CONFIG_CMD_FS_GENERIC=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDPARTS_DEFAULT="mtdparts=orion_nand:0xe0000@0x0(uboot),0x20000@0xe0000(uboot_env),0x100000@0x100000(second_stage_uboot),-@0x200000(root)" CONFIG_CMD_UBI=y @@ -47,6 +49,7 @@ CONFIG_SYS_SATA_MAX_DEVICE=1 # CONFIG_MMC is not set CONFIG_MTD=y CONFIG_MTD_RAW_NAND=y +CONFIG_PHY_MARVELL=y CONFIG_DM_ETH=y CONFIG_MVGBE=y CONFIG_MII=y diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c index 954bf86121..9aff72bc74 100644 --- a/drivers/net/mvgbe.c +++ b/drivers/net/mvgbe.c @@ -726,6 +726,13 @@ static int mvgbe_recv(struct eth_device *dev) } #endif
+/* PHY related */ +#define MV88E1318_PGADR_REG 22 +#define MV88E1318_MAC_CTRL_PG 2 +#define MV88E1318_MAC_CTRL_REG 21 +#define MV88E1318_RGMII_TX_CTRL (1 << 4) +#define MV88E1318_RGMII_RX_CTRL (1 << 5)
- #if defined(CONFIG_PHYLIB) || defined(CONFIG_DM_ETH) #if defined(CONFIG_DM_ETH) static struct phy_device *__mvgbe_phy_init(struct udevice *dev,
@@ -740,11 +747,19 @@ static struct phy_device *__mvgbe_phy_init(struct eth_device *dev, #endif { struct phy_device *phydev;
int reg;
/* Set phy address of the port */ miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST, phyid);
/* Set RGMII delay */
miiphy_write(dev->name, phyid, MV88E1318_PGADR_REG,MV88E1318_MAC_CTRL_PG);
miiphy_read(dev->name, phyid, MV88E1318_MAC_CTRL_REG, ®);reg |= (MV88E1318_RGMII_RX_CTRL | MV88E1318_RGMII_TX_CTRL);miiphy_write(dev->name, phyid, MV88E1318_MAC_CTRL_REG, reg);miiphy_write(dev->name, phyid, MV88E1318_PGADR_REG, 0);- phydev = phy_connect(bus, phyid, dev, phy_interface); if (!phydev) { printf("phy_connect failed\n");
Viele Grüße, Stefan Roese
On Thu, Apr 7, 2022 at 10:39 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 4/8/22 07:03, Tony Dinh wrote:
Hi all,
This is a work-in-progress patch that I'm working on, to clean up the DM Ethernet code for the Zyxel NSA310S board (Kirkwood 88F6702 A1).
This NSA310s board Ethernet has some quirks that it does not work quite the same way as other Kirwood boards with the similar network chip (MV88E1318), such as the Sheevaplug and the Dreamplug.
Currently, in the NSA310S board file we use CONFIG_RESET_PHY_R to execute reset_phy() during initialization. And the reset_phy() code in this board file is ad-hoc, does not involve Marvell PHY driver (which it should be). So I'm following the same pattern that I've done for the Sheevaplug: use the uclass drivers/net/mvgbe.c (CONFIG_MVGBE) to bring up Ethernet, removing the reset_phy() code, and enable CONFIG_PHY_MARVEL.
With this board, I could not get it to work the same way as for the Sheevaplug to bring up the PHY automatically. I had to insert the ad-hoc code to set RGMII delay in the __mvgbe_phy_init() function in mvgbe.c, so the phy_connect() call will find the PHY. The PHY Id is 1410e90, same as in the Sheevaplug and Dreamplug. Please see this in the patch below (it's only a hack to see the PHY can be brought up this way).
Looking at drivers/net/phy/marvell.c I see that m88e1310_config() does exactly this rx/tx delay configuration:
/* Marvell 88E1310 */ static int m88e1310_config(struct phy_device *phydev) { ... /* Set RGMII delay */ phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_PAGE, 0x0002); reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_RGMII_CTRL); reg |= 0x0030; phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_RGMII_CTRL, reg);
Are you sure that this functions is not called while probing / enabling the network interface?
With the Sheevaplug and Dreamplug, there is no need to do this hack. The uclass mvgbe did all the work, by the time phy_connect() is executed, the PHY was already up and working fine. And then the Marvell PHY driver kicks in, setting up the rest.
Perhaps we need code in the uclass MVGBE that handles this in a generic way? These are the Marvell PHY driver functions invoked after phy_connect() was successful. This happens in all 3 boards (Sheevaplug, Dreamplug, and NSA310s).
m88e1310_config m88e1011s_startup
Ah okay, It's executed.
I would appreciate hearing some explanation and perhaps some suggestion/guidance about this topic.
If all 3 boards use the same ethernet driver with the same configuration (e.g. CONFIG_PHYLIB, CONFIG_EM_ETH etc) and the same PHY, there should of course be no need to handle this differently for this Zyxel board. I suggest to continue debugging to see, why this really is necessary. Perhaps some soft-reset of the PHY clears this rx/tx delay again?
That's interesting. I recall trying miiphy_reset before phy_connect, but not phy_reset. I'll try that.
By the way, without doing something to make the PHY available at that point, the phy_connect would fail, so the Marvell PHY driver was never invoked later.
Additionally your patch seems to be indented incorrectly. This makes reviewing a bit harder.
Sorry about that! I forgot to run checkpatch since it is an RFC patch. Will send the V2 patch for this.
Thanks for looking at this, Tony
Thanks, Stefan
Thanks, Tony
diff --git a/board/zyxel/nsa310s/nsa310s.c b/board/zyxel/nsa310s/nsa310s.c index b71de4e11f..78d472d56c 100644 --- a/board/zyxel/nsa310s/nsa310s.c +++ b/board/zyxel/nsa310s/nsa310s.c @@ -1,22 +1,49 @@ // SPDX-License-Identifier: GPL-2.0+ /*
- Copyright (C) 2015, 2021 Tony Dinh mibodhi@gmail.com
- Copyright (C) 2015, 2021-2022 Tony Dinh mibodhi@gmail.com
- Copyright (C) 2015 Gerald Kerma dreagle@doukki.net
*/
#include <common.h> #include <init.h>
-#include <miiphy.h> -#include <net.h> +#include <netdev.h> #include <asm/arch/cpu.h> #include <asm/arch/soc.h> #include <asm/arch/mpp.h> #include <asm/global_data.h> #include <asm/io.h> -#include "nsa310s.h" +#include <linux/bitops.h>
DECLARE_GLOBAL_DATA_PTR;
+/*
- low GPIO's
- */
+#define HDD1_GREEN_LED BIT(16) +#define HDD1_RED_LED BIT(13) +#define USB_GREEN_LED BIT(15) +#define USB_POWER BIT(21) +#define SYS_GREEN_LED BIT(28) +#define SYS_ORANGE_LED BIT(29)
+#define COPY_GREEN_LED BIT(22) +#define COPY_RED_LED BIT(23)
+#define PIN_USB_GREEN_LED 15 +#define PIN_USB_POWER 21
+#define NSA310S_OE_LOW (~(0)) +#define NSA310S_VAL_LOW (SYS_GREEN_LED | USB_POWER)
+/*
- high GPIO's
+*/ +#define HDD2_GREEN_LED BIT(2) +#define HDD2_POWER BIT(1)
+#define NSA310S_OE_HIGH (~(0)) +#define NSA310S_VAL_HIGH (HDD2_POWER)
- int board_early_init_f(void) { /*
@@ -80,87 +107,7 @@ int board_init(void) return 0; }
-static int fdt_get_phy_addr(const char *path) +int board_eth_init(struct bd_info *bis) {
- const void *fdt = gd->fdt_blob;
- const u32 *reg;
- const u32 *val;
- int node, phandle, addr;
- /* Find the node by its full path */
- node = fdt_path_offset(fdt, path);
- if (node >= 0) {
- /* Look up phy-handle */
- val = fdt_getprop(fdt, node, "phy-handle", NULL);
- if (val) {
- phandle = fdt32_to_cpu(*val);
- if (!phandle)
- return -1;
- /* Follow it to its node */
- node = fdt_node_offset_by_phandle(fdt, phandle);
- if (node) {
- /* Look up reg */
- reg = fdt_getprop(fdt, node, "reg", NULL);
- if (reg) {
- addr = fdt32_to_cpu(*reg);
- return addr;
- }
- }
- }
- }
- return -1;
-}
-#ifdef CONFIG_RESET_PHY_R -void reset_phy(void) -{
- u16 reg;
- u16 phyaddr;
- char *name = "ethernet-controller@72000";
- char *eth0_path = "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
- if (miiphy_set_current_dev(name))
- return;
- phyaddr = fdt_get_phy_addr(eth0_path);
- if (phyaddr < 0)
- return;
- /* set RGMII delay */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, MV88E1318_MAC_CTRL_PG);
- miiphy_read(name, phyaddr, MV88E1318_MAC_CTRL_REG, ®);
- reg |= (MV88E1318_RGMII_RX_CTRL | MV88E1318_RGMII_TX_CTRL);
- miiphy_write(name, phyaddr, MV88E1318_MAC_CTRL_REG, reg);
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, 0);
- /* reset PHY */
- if (miiphy_reset(name, phyaddr))
- return;
- /*
- ZyXEL NSA310S uses the 88E1310S Alaska (interface identical to 88E1318)
- and has an MCU attached to the LED[2] via tristate interrupt
- */
- /* switch to LED register page */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, MV88E1318_LED_PG);
- /* read out LED polarity register */
- miiphy_read(name, phyaddr, MV88E1318_LED_POL_REG, ®);
- /* clear 4, set 5 - LED2 low, tri-state */
- reg &= ~(MV88E1318_LED2_4);
- reg |= (MV88E1318_LED2_5);
- /* write back LED polarity register */
- miiphy_write(name, phyaddr, MV88E1318_LED_POL_REG, reg);
- /* jump back to page 0, per the PHY chip documenation. */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, 0);
- /* set PHY back to auto-negotiation mode */
- miiphy_write(name, phyaddr, 0x4, 0x1e1);
- miiphy_write(name, phyaddr, 0x9, 0x300);
- /* downshift */
- miiphy_write(name, phyaddr, 0x10, 0x3860);
- miiphy_write(name, phyaddr, 0x0, 0x9140);
- printf("MV88E1318 PHY initialized on %s\n", name);
- return cpu_eth_init(bis); }
-#endif /* CONFIG_RESET_PHY_R */ diff --git a/configs/nsa310s_defconfig b/configs/nsa310s_defconfig index 05a6761854..07c96489c0 100644 --- a/configs/nsa310s_defconfig +++ b/configs/nsa310s_defconfig @@ -2,6 +2,7 @@ CONFIG_ARM=y CONFIG_SKIP_LOWLEVEL_INIT=y CONFIG_SYS_DCACHE_OFF=y CONFIG_ARCH_CPU_INIT=y +CONFIG_SYS_THUMB_BUILD=y CONFIG_ARCH_KIRKWOOD=y CONFIG_SYS_KWD_CONFIG="board/zyxel/nsa310s/kwbimage.cfg" CONFIG_SYS_TEXT_BASE=0x600000 @@ -31,6 +32,7 @@ CONFIG_CMD_PING=y CONFIG_CMD_EXT2=y CONFIG_CMD_FAT=y CONFIG_CMD_JFFS2=y +CONFIG_CMD_FS_GENERIC=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDPARTS_DEFAULT="mtdparts=orion_nand:0xe0000@0x0(uboot),0x20000@0xe0000(uboot_env),0x100000@0x100000(second_stage_uboot),-@0x200000(root)" CONFIG_CMD_UBI=y @@ -47,6 +49,7 @@ CONFIG_SYS_SATA_MAX_DEVICE=1 # CONFIG_MMC is not set CONFIG_MTD=y CONFIG_MTD_RAW_NAND=y +CONFIG_PHY_MARVELL=y CONFIG_DM_ETH=y CONFIG_MVGBE=y CONFIG_MII=y diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c index 954bf86121..9aff72bc74 100644 --- a/drivers/net/mvgbe.c +++ b/drivers/net/mvgbe.c @@ -726,6 +726,13 @@ static int mvgbe_recv(struct eth_device *dev) } #endif
+/* PHY related */ +#define MV88E1318_PGADR_REG 22 +#define MV88E1318_MAC_CTRL_PG 2 +#define MV88E1318_MAC_CTRL_REG 21 +#define MV88E1318_RGMII_TX_CTRL (1 << 4) +#define MV88E1318_RGMII_RX_CTRL (1 << 5)
- #if defined(CONFIG_PHYLIB) || defined(CONFIG_DM_ETH) #if defined(CONFIG_DM_ETH) static struct phy_device *__mvgbe_phy_init(struct udevice *dev,
@@ -740,11 +747,19 @@ static struct phy_device *__mvgbe_phy_init(struct eth_device *dev, #endif { struct phy_device *phydev;
int reg;
/* Set phy address of the port */ miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST, phyid);
/* Set RGMII delay */
miiphy_write(dev->name, phyid, MV88E1318_PGADR_REG,MV88E1318_MAC_CTRL_PG);
miiphy_read(dev->name, phyid, MV88E1318_MAC_CTRL_REG, ®);reg |= (MV88E1318_RGMII_RX_CTRL | MV88E1318_RGMII_TX_CTRL);miiphy_write(dev->name, phyid, MV88E1318_MAC_CTRL_REG, reg);miiphy_write(dev->name, phyid, MV88E1318_PGADR_REG, 0);- phydev = phy_connect(bus, phyid, dev, phy_interface); if (!phydev) { printf("phy_connect failed\n");
Viele Grüße, Stefan Roese
-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
Hi Stefan,
On Thu, Apr 7, 2022 at 10:55 PM Tony Dinh mibodhi@gmail.com wrote:
On Thu, Apr 7, 2022 at 10:39 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 4/8/22 07:03, Tony Dinh wrote:
Hi all,
This is a work-in-progress patch that I'm working on, to clean up the DM Ethernet code for the Zyxel NSA310S board (Kirkwood 88F6702 A1).
This NSA310s board Ethernet has some quirks that it does not work quite the same way as other Kirwood boards with the similar network chip (MV88E1318), such as the Sheevaplug and the Dreamplug.
Currently, in the NSA310S board file we use CONFIG_RESET_PHY_R to execute reset_phy() during initialization. And the reset_phy() code in this board file is ad-hoc, does not involve Marvell PHY driver (which it should be). So I'm following the same pattern that I've done for the Sheevaplug: use the uclass drivers/net/mvgbe.c (CONFIG_MVGBE) to bring up Ethernet, removing the reset_phy() code, and enable CONFIG_PHY_MARVEL.
With this board, I could not get it to work the same way as for the Sheevaplug to bring up the PHY automatically. I had to insert the ad-hoc code to set RGMII delay in the __mvgbe_phy_init() function in mvgbe.c, so the phy_connect() call will find the PHY. The PHY Id is 1410e90, same as in the Sheevaplug and Dreamplug. Please see this in the patch below (it's only a hack to see the PHY can be brought up this way).
Looking at drivers/net/phy/marvell.c I see that m88e1310_config() does exactly this rx/tx delay configuration:
/* Marvell 88E1310 */ static int m88e1310_config(struct phy_device *phydev) { ... /* Set RGMII delay */ phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_PAGE, 0x0002); reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_RGMII_CTRL); reg |= 0x0030; phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1310_PHY_RGMII_CTRL, reg);
Are you sure that this functions is not called while probing / enabling the network interface?
With the Sheevaplug and Dreamplug, there is no need to do this hack. The uclass mvgbe did all the work, by the time phy_connect() is executed, the PHY was already up and working fine. And then the Marvell PHY driver kicks in, setting up the rest.
Perhaps we need code in the uclass MVGBE that handles this in a generic way? These are the Marvell PHY driver functions invoked after phy_connect() was successful. This happens in all 3 boards (Sheevaplug, Dreamplug, and NSA310s).
m88e1310_config m88e1011s_startup
Ah okay, It's executed.
I would appreciate hearing some explanation and perhaps some suggestion/guidance about this topic.
If all 3 boards use the same ethernet driver with the same configuration (e.g. CONFIG_PHYLIB, CONFIG_EM_ETH etc) and the same PHY, there should of course be no need to handle this differently for this Zyxel board. I suggest to continue debugging to see, why this really is necessary. Perhaps some soft-reset of the PHY clears this rx/tx delay again?
That's interesting. I recall trying miiphy_reset before phy_connect, but not phy_reset. I'll try that.
By the way, without doing something to make the PHY available at that point, the phy_connect would fail, so the Marvell PHY driver was never invoked later.
I've tried the soft phy_reset (instead of the ad-hoc code for RGMII), the board just hangs, and I need to recycle the power to reboot.
Additionally your patch seems to be indented incorrectly. This makes reviewing a bit harder.
I will send in the V2 patch shortly.
Thanks, Tony
Sorry about that! I forgot to run checkpatch since it is an RFC patch. Will send the V2 patch for this.
Thanks for looking at this, Tony
Thanks, Stefan
Thanks, Tony
diff --git a/board/zyxel/nsa310s/nsa310s.c b/board/zyxel/nsa310s/nsa310s.c index b71de4e11f..78d472d56c 100644 --- a/board/zyxel/nsa310s/nsa310s.c +++ b/board/zyxel/nsa310s/nsa310s.c @@ -1,22 +1,49 @@ // SPDX-License-Identifier: GPL-2.0+ /*
- Copyright (C) 2015, 2021 Tony Dinh mibodhi@gmail.com
- Copyright (C) 2015, 2021-2022 Tony Dinh mibodhi@gmail.com
- Copyright (C) 2015 Gerald Kerma dreagle@doukki.net
*/
#include <common.h> #include <init.h>
-#include <miiphy.h> -#include <net.h> +#include <netdev.h> #include <asm/arch/cpu.h> #include <asm/arch/soc.h> #include <asm/arch/mpp.h> #include <asm/global_data.h> #include <asm/io.h> -#include "nsa310s.h" +#include <linux/bitops.h>
DECLARE_GLOBAL_DATA_PTR;
+/*
- low GPIO's
- */
+#define HDD1_GREEN_LED BIT(16) +#define HDD1_RED_LED BIT(13) +#define USB_GREEN_LED BIT(15) +#define USB_POWER BIT(21) +#define SYS_GREEN_LED BIT(28) +#define SYS_ORANGE_LED BIT(29)
+#define COPY_GREEN_LED BIT(22) +#define COPY_RED_LED BIT(23)
+#define PIN_USB_GREEN_LED 15 +#define PIN_USB_POWER 21
+#define NSA310S_OE_LOW (~(0)) +#define NSA310S_VAL_LOW (SYS_GREEN_LED | USB_POWER)
+/*
- high GPIO's
+*/ +#define HDD2_GREEN_LED BIT(2) +#define HDD2_POWER BIT(1)
+#define NSA310S_OE_HIGH (~(0)) +#define NSA310S_VAL_HIGH (HDD2_POWER)
- int board_early_init_f(void) { /*
@@ -80,87 +107,7 @@ int board_init(void) return 0; }
-static int fdt_get_phy_addr(const char *path) +int board_eth_init(struct bd_info *bis) {
- const void *fdt = gd->fdt_blob;
- const u32 *reg;
- const u32 *val;
- int node, phandle, addr;
- /* Find the node by its full path */
- node = fdt_path_offset(fdt, path);
- if (node >= 0) {
- /* Look up phy-handle */
- val = fdt_getprop(fdt, node, "phy-handle", NULL);
- if (val) {
- phandle = fdt32_to_cpu(*val);
- if (!phandle)
- return -1;
- /* Follow it to its node */
- node = fdt_node_offset_by_phandle(fdt, phandle);
- if (node) {
- /* Look up reg */
- reg = fdt_getprop(fdt, node, "reg", NULL);
- if (reg) {
- addr = fdt32_to_cpu(*reg);
- return addr;
- }
- }
- }
- }
- return -1;
-}
-#ifdef CONFIG_RESET_PHY_R -void reset_phy(void) -{
- u16 reg;
- u16 phyaddr;
- char *name = "ethernet-controller@72000";
- char *eth0_path = "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
- if (miiphy_set_current_dev(name))
- return;
- phyaddr = fdt_get_phy_addr(eth0_path);
- if (phyaddr < 0)
- return;
- /* set RGMII delay */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, MV88E1318_MAC_CTRL_PG);
- miiphy_read(name, phyaddr, MV88E1318_MAC_CTRL_REG, ®);
- reg |= (MV88E1318_RGMII_RX_CTRL | MV88E1318_RGMII_TX_CTRL);
- miiphy_write(name, phyaddr, MV88E1318_MAC_CTRL_REG, reg);
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, 0);
- /* reset PHY */
- if (miiphy_reset(name, phyaddr))
- return;
- /*
- ZyXEL NSA310S uses the 88E1310S Alaska (interface identical to 88E1318)
- and has an MCU attached to the LED[2] via tristate interrupt
- */
- /* switch to LED register page */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, MV88E1318_LED_PG);
- /* read out LED polarity register */
- miiphy_read(name, phyaddr, MV88E1318_LED_POL_REG, ®);
- /* clear 4, set 5 - LED2 low, tri-state */
- reg &= ~(MV88E1318_LED2_4);
- reg |= (MV88E1318_LED2_5);
- /* write back LED polarity register */
- miiphy_write(name, phyaddr, MV88E1318_LED_POL_REG, reg);
- /* jump back to page 0, per the PHY chip documenation. */
- miiphy_write(name, phyaddr, MV88E1318_PGADR_REG, 0);
- /* set PHY back to auto-negotiation mode */
- miiphy_write(name, phyaddr, 0x4, 0x1e1);
- miiphy_write(name, phyaddr, 0x9, 0x300);
- /* downshift */
- miiphy_write(name, phyaddr, 0x10, 0x3860);
- miiphy_write(name, phyaddr, 0x0, 0x9140);
- printf("MV88E1318 PHY initialized on %s\n", name);
- return cpu_eth_init(bis); }
-#endif /* CONFIG_RESET_PHY_R */ diff --git a/configs/nsa310s_defconfig b/configs/nsa310s_defconfig index 05a6761854..07c96489c0 100644 --- a/configs/nsa310s_defconfig +++ b/configs/nsa310s_defconfig @@ -2,6 +2,7 @@ CONFIG_ARM=y CONFIG_SKIP_LOWLEVEL_INIT=y CONFIG_SYS_DCACHE_OFF=y CONFIG_ARCH_CPU_INIT=y +CONFIG_SYS_THUMB_BUILD=y CONFIG_ARCH_KIRKWOOD=y CONFIG_SYS_KWD_CONFIG="board/zyxel/nsa310s/kwbimage.cfg" CONFIG_SYS_TEXT_BASE=0x600000 @@ -31,6 +32,7 @@ CONFIG_CMD_PING=y CONFIG_CMD_EXT2=y CONFIG_CMD_FAT=y CONFIG_CMD_JFFS2=y +CONFIG_CMD_FS_GENERIC=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDPARTS_DEFAULT="mtdparts=orion_nand:0xe0000@0x0(uboot),0x20000@0xe0000(uboot_env),0x100000@0x100000(second_stage_uboot),-@0x200000(root)" CONFIG_CMD_UBI=y @@ -47,6 +49,7 @@ CONFIG_SYS_SATA_MAX_DEVICE=1 # CONFIG_MMC is not set CONFIG_MTD=y CONFIG_MTD_RAW_NAND=y +CONFIG_PHY_MARVELL=y CONFIG_DM_ETH=y CONFIG_MVGBE=y CONFIG_MII=y diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c index 954bf86121..9aff72bc74 100644 --- a/drivers/net/mvgbe.c +++ b/drivers/net/mvgbe.c @@ -726,6 +726,13 @@ static int mvgbe_recv(struct eth_device *dev) } #endif
+/* PHY related */ +#define MV88E1318_PGADR_REG 22 +#define MV88E1318_MAC_CTRL_PG 2 +#define MV88E1318_MAC_CTRL_REG 21 +#define MV88E1318_RGMII_TX_CTRL (1 << 4) +#define MV88E1318_RGMII_RX_CTRL (1 << 5)
- #if defined(CONFIG_PHYLIB) || defined(CONFIG_DM_ETH) #if defined(CONFIG_DM_ETH) static struct phy_device *__mvgbe_phy_init(struct udevice *dev,
@@ -740,11 +747,19 @@ static struct phy_device *__mvgbe_phy_init(struct eth_device *dev, #endif { struct phy_device *phydev;
int reg;
/* Set phy address of the port */ miiphy_write(dev->name, MV_PHY_ADR_REQUEST, MV_PHY_ADR_REQUEST, phyid);
/* Set RGMII delay */
miiphy_write(dev->name, phyid, MV88E1318_PGADR_REG,MV88E1318_MAC_CTRL_PG);
miiphy_read(dev->name, phyid, MV88E1318_MAC_CTRL_REG, ®);reg |= (MV88E1318_RGMII_RX_CTRL | MV88E1318_RGMII_TX_CTRL);miiphy_write(dev->name, phyid, MV88E1318_MAC_CTRL_REG, reg);miiphy_write(dev->name, phyid, MV88E1318_PGADR_REG, 0);- phydev = phy_connect(bus, phyid, dev, phy_interface); if (!phydev) { printf("phy_connect failed\n");
Viele Grüße, Stefan Roese
-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
participants (2)
-
Stefan Roese -
Tony Dinh