Hi Stefan,

On Fri, Nov 30, 2018 at 2:44 PM Stefan Roese sr@denx.de wrote:

On 30.11.18 03:54, Aditya Prayoga wrote:

...

Similar to Clearfog rev 2.1, GPIO 19 also used to reset onboard ethernet PHY.

Signed-off-by: Aditya Prayoga aditya@kobol.io

v2:

• Use generic gpio_* API (Baruch Siach)

---

board/kobol/helios4/helios4.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)

diff --git a/board/kobol/helios4/helios4.c b/board/kobol/helios4/helios4.c index 37c46a5..e535d7b 100644 --- a/board/kobol/helios4/helios4.c +++ b/board/kobol/helios4/helios4.c @@ -8,6 +8,7 @@ #include <i2c.h> #include <miiphy.h> #include <netdev.h> +#include <asm/gpio.h> #include <asm/io.h> #include <asm/arch/cpu.h> #include <asm/arch/soc.h> @@ -111,9 +112,39 @@ int board_early_init_f(void)

int board_init(void) {

• struct udevice *gpio0;
  

• struct gpio_desc phy_reset;
  

• int ret;
  

• /* Address of boot parameters */
  gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
  
  

From here

...

• ret = uclass_get_device_by_name(UCLASS_GPIO,
  

•                                 "gpio@18100",
  

•                                 &gpio0);
  

• if (ret < 0) {
  

•         printf("Failed to find gpio@18100 node.\n");
  

•         return ret;
  

• }
  

• phy_reset.dev = gpio0;
  

• /* MPP19 controls the uSOM onboard phy reset */
  

• phy_reset.offset = 19;
  

up until this line can be replaced by a single line ret = dm_gpio_lookup_name("A19", &reset);

but gpio "A19" does not correlate with any documentation (datasheet, schematic, or device tree). I also got this warning "Device 'gpio@18100': seq 0 is in use by 'gpio-expander@20'"

...

• ret = dm_gpio_request(&phy_reset, "phy-reset");
  

• if (ret)
  

•         return ret;
  

• dm_gpio_set_dir_flags(&phy_reset,
  

•                       GPIOD_IS_OUT |
  

•                       GPIOD_ACTIVE_LOW |
  

•                       GPIOD_IS_OUT_ACTIVE);
  

• mdelay(10);
  

• dm_gpio_set_value(&phy_reset, 0);
  

• mdelay(10);
  

Hmm, this is a pretty complex and unusual way to use the GPIO. Please use the DT to describe the PHY reset GPIO instead. And since it seems to be a common issue to have such a PHY reset GPIO, it would be better to integrate this into the ethernet driver instead.

well, I followed minnowmax implementation (board/intel/minnowmax/minnowmax.c) and some other boards. Maybe I should use named gpios in the device tree.

Aditya

Here an example from the mvpp2 driver:

https://patchwork.ozlabs.org/patch/799654/

Could you try to integrate something similar into the mvneta driver?

Thanks, Stefan

Hi Stefan,

On Fri, Nov 30, 2018 at 3:25 PM Stefan Roese sr@denx.de wrote:

On 30.11.18 09:14, Aditya Prayoga wrote:

...

Hi Stefan,

On Fri, Nov 30, 2018 at 2:44 PM Stefan Roese sr@denx.de wrote:

...

On 30.11.18 03:54, Aditya Prayoga wrote:

...

Similar to Clearfog rev 2.1, GPIO 19 also used to reset onboard ethernet PHY.

Signed-off-by: Aditya Prayoga aditya@kobol.io

v2:

• Use generic gpio_* API (Baruch Siach)

---

board/kobol/helios4/helios4.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)

diff --git a/board/kobol/helios4/helios4.c b/board/kobol/helios4/helios4.c index 37c46a5..e535d7b 100644 --- a/board/kobol/helios4/helios4.c +++ b/board/kobol/helios4/helios4.c @@ -8,6 +8,7 @@ #include <i2c.h> #include <miiphy.h> #include <netdev.h> +#include <asm/gpio.h> #include <asm/io.h> #include <asm/arch/cpu.h> #include <asm/arch/soc.h> @@ -111,9 +112,39 @@ int board_early_init_f(void)

int board_init(void) {

• struct udevice *gpio0;
  

• struct gpio_desc phy_reset;
  

• int ret;
  

•  /* Address of boot parameters */
   gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
  
  

From here

...

...

• ret = uclass_get_device_by_name(UCLASS_GPIO,
  

•                                 "gpio@18100",
  

•                                 &gpio0);
  

• if (ret < 0) {
  

•         printf("Failed to find gpio@18100 node.\n");
  

•         return ret;
  

• }
  

• phy_reset.dev = gpio0;
  

• /* MPP19 controls the uSOM onboard phy reset */
  

• phy_reset.offset = 19;
  

up until this line can be replaced by a single line ret = dm_gpio_lookup_name("A19", &reset);

but gpio "A19" does not correlate with any documentation (datasheet, schematic, or device tree). I also got this warning "Device 'gpio@18100': seq 0 is in use by 'gpio-expander@20'"

...

...

• ret = dm_gpio_request(&phy_reset, "phy-reset");
  

• if (ret)
  

•         return ret;
  

• dm_gpio_set_dir_flags(&phy_reset,
  

•                       GPIOD_IS_OUT |
  

•                       GPIOD_ACTIVE_LOW |
  

•                       GPIOD_IS_OUT_ACTIVE);
  

• mdelay(10);
  

• dm_gpio_set_value(&phy_reset, 0);
  

• mdelay(10);
  

Hmm, this is a pretty complex and unusual way to use the GPIO. Please use the DT to describe the PHY reset GPIO instead. And since it seems to be a common issue to have such a PHY reset GPIO, it would be better to integrate this into the ethernet driver instead.

well, I followed minnowmax implementation (board/intel/minnowmax/minnowmax.c) and some other boards.

This seems not to be a good example. It's pretty outdated, AFAICT.

...

Maybe I should use named gpios in the device tree.

Yes. As mentioned above, please look at the mvpp2 driver. And e.g. at the armada-8040-clearfog-gt-8k.dts DT file:

/* 1G SGMII */ &cps_eth1 { status = "okay"; phy-mode = "sgmii"; phy = <&phy0>; phy-reset-gpios = <&cpm_gpio1 11 GPIO_ACTIVE_LOW>; };

Here you see, how the GPIO is defined in the DT. It should not be too hard to get this implemented in the mvneta driver as well.

Yes, what I meant is to put "phy-reset-gpios" in the device tree but it would still part of the system reset so the reset would be asserted in board_init.

If the reset implemented in driver (mvneta), the reset would only asserted when u-boot going to use the network interface. This is not desirable.

Regards, Aditya

Thanks, Stefan

Hi Baruch,

On Tue, Dec 4, 2018 at 9:40 PM Baruch Siach baruch@tkos.co.il wrote:

Hi Aditya,

On Mon, Dec 03, 2018 at 09:39:56AM +0700, Aditya Prayoga wrote:

...

On Fri, Nov 30, 2018 at 3:25 PM Stefan Roese sr@denx.de wrote:

...

On 30.11.18 09:14, Aditya Prayoga wrote:

...

On Fri, Nov 30, 2018 at 2:44 PM Stefan Roese sr@denx.de wrote:

...

On 30.11.18 03:54, Aditya Prayoga wrote:

...

Similar to Clearfog rev 2.1, GPIO 19 also used to reset onboard ethernet PHY.

Signed-off-by: Aditya Prayoga aditya@kobol.io

v2:

• Use generic gpio_* API (Baruch Siach)

---

board/kobol/helios4/helios4.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)

diff --git a/board/kobol/helios4/helios4.c b/board/kobol/helios4/helios4.c index 37c46a5..e535d7b 100644 --- a/board/kobol/helios4/helios4.c +++ b/board/kobol/helios4/helios4.c @@ -8,6 +8,7 @@ #include <i2c.h> #include <miiphy.h> #include <netdev.h> +#include <asm/gpio.h> #include <asm/io.h> #include <asm/arch/cpu.h> #include <asm/arch/soc.h> @@ -111,9 +112,39 @@ int board_early_init_f(void)

int board_init(void) {

• struct udevice *gpio0;
  

• struct gpio_desc phy_reset;
  

• int ret;
  

•  /* Address of boot parameters */
   gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
  
  

From here

...

...

• ret = uclass_get_device_by_name(UCLASS_GPIO,
  

•                                 "gpio@18100",
  

•                                 &gpio0);
  

• if (ret < 0) {
  

•         printf("Failed to find gpio@18100 node.\n");
  

•         return ret;
  

• }
  

• phy_reset.dev = gpio0;
  

• /* MPP19 controls the uSOM onboard phy reset */
  

• phy_reset.offset = 19;
  

up until this line can be replaced by a single line ret = dm_gpio_lookup_name("A19", &reset);

but gpio "A19" does not correlate with any documentation (datasheet, schematic, or device tree). I also got this warning "Device 'gpio@18100': seq 0 is in use by 'gpio-expander@20'"

...

...

• ret = dm_gpio_request(&phy_reset, "phy-reset");
  

• if (ret)
  

•         return ret;
  

• dm_gpio_set_dir_flags(&phy_reset,
  

•                       GPIOD_IS_OUT |
  

•                       GPIOD_ACTIVE_LOW |
  

•                       GPIOD_IS_OUT_ACTIVE);
  

• mdelay(10);
  

• dm_gpio_set_value(&phy_reset, 0);
  

• mdelay(10);
  

Hmm, this is a pretty complex and unusual way to use the GPIO. Please use the DT to describe the PHY reset GPIO instead. And since it seems to be a common issue to have such a PHY reset GPIO, it would be better to integrate this into the ethernet driver instead.

well, I followed minnowmax implementation (board/intel/minnowmax/minnowmax.c) and some other boards.

This seems not to be a good example. It's pretty outdated, AFAICT.

...

Maybe I should use named gpios in the device tree.

Yes. As mentioned above, please look at the mvpp2 driver. And e.g. at the armada-8040-clearfog-gt-8k.dts DT file:

/* 1G SGMII */ &cps_eth1 { status = "okay"; phy-mode = "sgmii"; phy = <&phy0>; phy-reset-gpios = <&cpm_gpio1 11 GPIO_ACTIVE_LOW>; };

Here you see, how the GPIO is defined in the DT. It should not be too hard to get this implemented in the mvneta driver as well.

Yes, what I meant is to put "phy-reset-gpios" in the device tree but it would still part of the system reset so the reset would be asserted in board_init.

If the reset implemented in driver (mvneta), the reset would only asserted when u-boot going to use the network interface. This is not desirable.

Network device probe routine is always called even when not used. See this comment in eth_initialize():

    /*
     * Devices need to write the hwaddr even if not started so that Linux
     * will have access to the hwaddr that u-boot stored for the device.
     * This is accomplished by attempting to probe each device and calling
     * their write_hwaddr() operation.
     */

That is what I thought but that is not what I observed. I tried to port that mvpp2 changes to mvneta --- diff --git a/drivers/net/mvneta.c b/drivers/net/mvneta.c index 8cb04b5..34f191d 100644 --- a/drivers/net/mvneta.c +++ b/drivers/net/mvneta.c @@ -27,6 +27,7 @@ #include <asm/arch/soc.h> #include <linux/compat.h> #include <linux/mbus.h> +#include <asm-generic/gpio.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -274,6 +275,9 @@ struct mvneta_port { int init; int phyaddr; struct phy_device *phydev; +#ifdef CONFIG_DM_GPIO + struct gpio_desc phy_reset_gpio; +#endif struct mii_dev *bus; };

@@ -1733,6 +1737,17 @@ static int mvneta_probe(struct udevice *dev) pp->phyaddr = fdtdec_get_int(blob, addr, "reg", 0); }

+#ifdef CONFIG_DM_GPIO + gpio_request_by_name(dev, "phy-reset-gpios", 0, + &pp->phy_reset_gpio, GPIOD_IS_OUT); + + if (dm_gpio_is_valid(&pp->phy_reset_gpio)) { + dm_gpio_set_value(&pp->phy_reset_gpio, 1); + mdelay(3000); + dm_gpio_set_value(&pp->phy_reset_gpio, 0); + } +#endif + bus = mdio_alloc(); if (!bus) { printf("Failed to allocate MDIO bus\n"); --- I intentionally put that 3 seconds delay so i can observe the RJ45 LED. After pressing the reset button, the LED never turned off. It turned off only when i access the network, for example tftpboot.

here are snippet from the serial log ---- Net: Warning: ethernet@70000 (eth1) using random MAC address - ae:c1:5a:4e:ba:00 eth1: ethernet@70000 Hit any key to stop autoboot: 0 => tftpboot ethernet@70000 Waiting for PHY auto negotiation to complete....... done *** ERROR: `serverip' not set => ---- The LED turned off during "Waiting for PHY auto negotiation"

Regards, Aditya

baruch

-- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{=

• baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Hi Stefan,

On Tue, Dec 4, 2018 at 10:51 PM Stefan Roese sr@denx.de wrote:

On 04.12.18 16:37, Aditya Prayoga wrote:

<snip>

...

...

...

...

Yes. As mentioned above, please look at the mvpp2 driver. And e.g. at the armada-8040-clearfog-gt-8k.dts DT file:

/* 1G SGMII */ &cps_eth1 { status = "okay"; phy-mode = "sgmii"; phy = <&phy0>; phy-reset-gpios = <&cpm_gpio1 11 GPIO_ACTIVE_LOW>; };

Here you see, how the GPIO is defined in the DT. It should not be too hard to get this implemented in the mvneta driver as well.

Yes, what I meant is to put "phy-reset-gpios" in the device tree but it would still part of the system reset so the reset would be asserted in board_init.

If the reset implemented in driver (mvneta), the reset would only asserted when u-boot going to use the network interface. This is not desirable.

Network device probe routine is always called even when not used. See this

This is not correct. The probe function is *not* always called - at least not in newer DM drivers.

...

...

comment in eth_initialize():

     /*
      * Devices need to write the hwaddr even if not started so that Linux
      * will have access to the hwaddr that u-boot stored for the device.
      * This is accomplished by attempting to probe each device and calling
      * their write_hwaddr() operation.
      */

These are special hooks that are always called to write the MAC address, even when the device is not used in U-Boot. IIRC, the bind() function is also always called.

...

That is what I thought but that is not what I observed. I tried to port that mvpp2 changes to mvneta

---

diff --git a/drivers/net/mvneta.c b/drivers/net/mvneta.c index 8cb04b5..34f191d 100644 --- a/drivers/net/mvneta.c +++ b/drivers/net/mvneta.c @@ -27,6 +27,7 @@ #include <asm/arch/soc.h> #include <linux/compat.h> #include <linux/mbus.h> +#include <asm-generic/gpio.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -274,6 +275,9 @@ struct mvneta_port { int init; int phyaddr; struct phy_device *phydev; +#ifdef CONFIG_DM_GPIO

• struct gpio_desc phy_reset_gpio;

+#endif struct mii_dev *bus; };

@@ -1733,6 +1737,17 @@ static int mvneta_probe(struct udevice *dev) pp->phyaddr = fdtdec_get_int(blob, addr, "reg", 0); }

+#ifdef CONFIG_DM_GPIO

• gpio_request_by_name(dev, "phy-reset-gpios", 0,

•             &pp->phy_reset_gpio, GPIOD_IS_OUT);
  

• if (dm_gpio_is_valid(&pp->phy_reset_gpio)) {

•    dm_gpio_set_value(&pp->phy_reset_gpio, 1);
  

•    mdelay(3000);
  

•    dm_gpio_set_value(&pp->phy_reset_gpio, 0);
  

• }

+#endif

• bus = mdio_alloc();
  if (!bus) {
      printf("Failed to allocate MDIO bus\n");
  

---

I intentionally put that 3 seconds delay so i can observe the RJ45 LED. After pressing the reset button, the LED never turned off. It turned off only when i access the network, for example tftpboot.

here are snippet from the serial log

Net: Warning: ethernet@70000 (eth1) using random MAC address - ae:c1:5a:4e:ba:00 eth1: ethernet@70000 Hit any key to stop autoboot: 0 => tftpboot ethernet@70000 Waiting for PHY auto negotiation to complete....... done *** ERROR: `serverip' not set =>

---

The LED turned off during "Waiting for PHY auto negotiation"

This should work. Did you correctly add the GPIO to your DT? Is the GPIO correctly referenced? Did you check this in the GPIO driver?

You're right. My fault, I put the gpio under phy node not the ethernet node. After move the gpio into ethernet node, it works. I will update this series and send the mvneta changes as separate patch.

Regards, Aditya

Thanks, Stefan