[PATCH 0/3] Convert the Dreamplug Ethernet and SATA to Driver Model.

[PATCH 0/3] Convert the Dreamplug Ethernet and SATA to Driver Model.

[Tony Dinh]

- Enable DM SATA, removed IDE driver, add SATA MV driver.
- Use Ethernet PHY names from device tree.
- Replace the old Ethernet PHY addr lookup with a device tree parsing
lookup function.


Tony Dinh (3):
  arm: kirkwood: Dreamplug: Add DM Ethernet and DM SATA configs
  arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from
    device tree
  arm: kirkwood: Dreamplug: Add DM SATA and remove IDE configs

 board/Marvell/dreamplug/dreamplug.c | 62 ++++++++++++++++++++++-------
 configs/dreamplug_defconfig         |  4 ++
 include/configs/dreamplug.h         | 12 +++++-
 3 files changed, 62 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCH 1/3] arm: kirkwood: Dreamplug: Add DM Ethernet and DM SATA configs

[Tony Dinh]

Add DM_ETH, SATA_MV and associated configs to dreamplug_defconfig

Signed-off-by: Tony Dinh <mibodhi@gmail.com>
---

 configs/dreamplug_defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configs/dreamplug_defconfig b/configs/dreamplug_defconfig
index 8956d2f3c5..516e28752c 100644
--- a/configs/dreamplug_defconfig
+++ b/configs/dreamplug_defconfig
@@ -51,3 +51,7 @@ CONFIG_USB=y
 CONFIG_DM_USB=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_STORAGE=y
+CONFIG_DM_ETH=y
+CONFIG_NET_RANDOM_ETHADDR=y
+CONFIG_CMD_SATA=y
+CONFIG_SATA_MV=y
-- 
2.20.1

[PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Tony Dinh]

In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer valid,
so replace these with Ethernet PHY names from device tree. Also, read
Ethernet PHY address for each port from device tree.

Signed-off-by: Tony Dinh <mibodhi@gmail.com>
---

 board/Marvell/dreamplug/dreamplug.c | 62 ++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/board/Marvell/dreamplug/dreamplug.c b/board/Marvell/dreamplug/dreamplug.c
index e1c64b5224..d5b6b22ddf 100644
--- a/board/Marvell/dreamplug/dreamplug.c
+++ b/board/Marvell/dreamplug/dreamplug.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
+ * Copyright (C) 2021  Tony Dinh <mibodhi@gmail.com>
  * (C) Copyright 2011
  * Jason Cooper <u-boot@lakedaemon.net>
  *
@@ -97,42 +98,75 @@ int board_init(void)
 	return 0;
 }
 
+static int fdt_get_phy_addr(const char *path)
+{
+	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 mv_phy_88e1116_init(char *name)
+void mv_phy_88e1116_init(const char *name, const char *path)
 {
 	u16 reg;
-	u16 devadr;
+	int phyaddr;
 
 	if (miiphy_set_current_dev(name))
 		return;
 
-	/* command to read PHY dev address */
-	if (miiphy_read(name, 0xEE, 0xEE, (u16 *) &devadr)) {
-		printf("Err..%s could not read PHY dev address\n",
-			__func__);
+	phyaddr = fdt_get_phy_addr(path);
+	if (phyaddr < 0)
 		return;
-	}
 
 	/*
 	 * Enable RGMII delay on Tx and Rx for CPU port
 	 * Ref: sec 4.7.2 of chip datasheet
 	 */
-	miiphy_write(name, devadr, MV88E1116_PGADR_REG, 2);
-	miiphy_read(name, devadr, MV88E1116_MAC_CTRL2_REG, &reg);
+	miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 2);
+	miiphy_read(name, phyaddr, MV88E1116_MAC_CTRL2_REG, &reg);
 	reg |= (MV88E1116_RGMII_RXTM_CTRL | MV88E1116_RGMII_TXTM_CTRL);
-	miiphy_write(name, devadr, MV88E1116_MAC_CTRL2_REG, reg);
-	miiphy_write(name, devadr, MV88E1116_PGADR_REG, 0);
+	miiphy_write(name, phyaddr, MV88E1116_MAC_CTRL2_REG, reg);
+	miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 0);
 
 	/* reset the phy */
-	miiphy_reset(name, devadr);
+	miiphy_reset(name, phyaddr);
 
 	printf("88E1116 Initialized on %s\n", name);
 }
 
 void reset_phy(void)
 {
+	char *eth0_name = "ethernet-controller@72000";
+	char *eth0_path = "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
+	char *eth1_name = "ethernet-controller@76000";
+	char *eth1_path = "/ocp@f1000000/ethernet-controller@72000/ethernet1-port@0";
+
 	/* configure and initialize both PHY's */
-	mv_phy_88e1116_init("egiga0");
-	mv_phy_88e1116_init("egiga1");
+	mv_phy_88e1116_init(eth0_name, eth0_path);
+	mv_phy_88e1116_init(eth1_name, eth1_path);
 }
 #endif /* CONFIG_RESET_PHY_R */
-- 
2.20.1

[PATCH 3/3] arm: kirkwood: Dreamplug: Add DM SATA and remove IDE configs

[Tony Dinh]

- Enable DM SATA, removed IDE driver, and add SATA MV driver.
- Use ethernet PHY names from device tree in default boot command

Signed-off-by: Tony Dinh <mibodhi@gmail.com>
---

 include/configs/dreamplug.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/configs/dreamplug.h b/include/configs/dreamplug.h
index 9106203ebc..65962ee733 100644
--- a/include/configs/dreamplug.h
+++ b/include/configs/dreamplug.h
@@ -31,8 +31,8 @@
 /*
  * Default environment variables
  */
-#define CONFIG_BOOTCOMMAND		"setenv ethact egiga0; " \
-	"${x_bootcmd_ethernet}; setenv ethact egiga1; " \
+#define CONFIG_BOOTCOMMAND		"setenv ethact ethernet-controller@72000; " \
+	"${x_bootcmd_ethernet}; setenv ethact ethernet-controller@76000; " \
 	"${x_bootcmd_ethernet}; ${x_bootcmd_usb}; ${x_bootcmd_kernel}; "\
 	"setenv bootargs ${x_bootargs} ${x_bootargs_root}; "	\
 	"bootm 0x6400000;"
@@ -52,4 +52,12 @@
 #define CONFIG_PHY_BASE_ADR	0
 #endif /* CONFIG_CMD_NET */
 
+/*
+ * SATA Driver configuration
+ */
+#ifdef CONFIG_SATA
+#define CONFIG_SYS_SATA_MAX_DEVICE	1
+#define CONFIG_LBA48
+#endif /* CONFIG_SATA */
+
 #endif /* _CONFIG_DREAMPLUG_H */
-- 
2.20.1

Re: [PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Stefan Roese]

On 26.07.21 08:01, Tony Dinh wrote:
> In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer valid,
> so replace these with Ethernet PHY names from device tree. Also, read
> Ethernet PHY address for each port from device tree.
> 
> Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> ---
> 
>   board/Marvell/dreamplug/dreamplug.c | 62 ++++++++++++++++++++++-------
>   1 file changed, 48 insertions(+), 14 deletions(-)
> 
> diff --git a/board/Marvell/dreamplug/dreamplug.c b/board/Marvell/dreamplug/dreamplug.c
> index e1c64b5224..d5b6b22ddf 100644
> --- a/board/Marvell/dreamplug/dreamplug.c
> +++ b/board/Marvell/dreamplug/dreamplug.c
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0+
>   /*
> + * Copyright (C) 2021  Tony Dinh <mibodhi@gmail.com>
>    * (C) Copyright 2011
>    * Jason Cooper <u-boot@lakedaemon.net>
>    *
> @@ -97,42 +98,75 @@ int board_init(void)
>   	return 0;
>   }
>   
> +static int fdt_get_phy_addr(const char *path)
> +{
> +	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;
> +}

You've added this exact some function now for the 3rd time IIRC. Could
please please consolidate this function into one of the fdt / dt common
functions instead? Perhaps there is already something similar for
reading PHY addresses?

Please do this in a follow-up patch, after I've pulled this one.

Other that this:

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

Thanks,
Stefan


> +
>   #ifdef CONFIG_RESET_PHY_R
> -void mv_phy_88e1116_init(char *name)
> +void mv_phy_88e1116_init(const char *name, const char *path)
>   {
>   	u16 reg;
> -	u16 devadr;
> +	int phyaddr;
>   
>   	if (miiphy_set_current_dev(name))
>   		return;
>   
> -	/* command to read PHY dev address */
> -	if (miiphy_read(name, 0xEE, 0xEE, (u16 *) &devadr)) {
> -		printf("Err..%s could not read PHY dev address\n",
> -			__func__);
> +	phyaddr = fdt_get_phy_addr(path);
> +	if (phyaddr < 0)
>   		return;
> -	}
>   
>   	/*
>   	 * Enable RGMII delay on Tx and Rx for CPU port
>   	 * Ref: sec 4.7.2 of chip datasheet
>   	 */
> -	miiphy_write(name, devadr, MV88E1116_PGADR_REG, 2);
> -	miiphy_read(name, devadr, MV88E1116_MAC_CTRL2_REG, &reg);
> +	miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 2);
> +	miiphy_read(name, phyaddr, MV88E1116_MAC_CTRL2_REG, &reg);
>   	reg |= (MV88E1116_RGMII_RXTM_CTRL | MV88E1116_RGMII_TXTM_CTRL);
> -	miiphy_write(name, devadr, MV88E1116_MAC_CTRL2_REG, reg);
> -	miiphy_write(name, devadr, MV88E1116_PGADR_REG, 0);
> +	miiphy_write(name, phyaddr, MV88E1116_MAC_CTRL2_REG, reg);
> +	miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 0);
>   
>   	/* reset the phy */
> -	miiphy_reset(name, devadr);
> +	miiphy_reset(name, phyaddr);
>   
>   	printf("88E1116 Initialized on %s\n", name);
>   }
>   
>   void reset_phy(void)
>   {
> +	char *eth0_name = "ethernet-controller@72000";
> +	char *eth0_path = "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
> +	char *eth1_name = "ethernet-controller@76000";
> +	char *eth1_path = "/ocp@f1000000/ethernet-controller@72000/ethernet1-port@0";
> +
>   	/* configure and initialize both PHY's */
> -	mv_phy_88e1116_init("egiga0");
> -	mv_phy_88e1116_init("egiga1");
> +	mv_phy_88e1116_init(eth0_name, eth0_path);
> +	mv_phy_88e1116_init(eth1_name, eth1_path);
>   }
>   #endif /* CONFIG_RESET_PHY_R */
> 


Viele Grüße,
Stefan

-- 
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

Re: [PATCH 1/3] arm: kirkwood: Dreamplug: Add DM Ethernet and DM SATA configs

[Stefan Roese]

On 26.07.21 08:01, Tony Dinh wrote:
> Add DM_ETH, SATA_MV and associated configs to dreamplug_defconfig
> 
> Signed-off-by: Tony Dinh <mibodhi@gmail.com>

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

Thanks,
Stefan

> ---
> 
>   configs/dreamplug_defconfig | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/configs/dreamplug_defconfig b/configs/dreamplug_defconfig
> index 8956d2f3c5..516e28752c 100644
> --- a/configs/dreamplug_defconfig
> +++ b/configs/dreamplug_defconfig
> @@ -51,3 +51,7 @@ CONFIG_USB=y
>   CONFIG_DM_USB=y
>   CONFIG_USB_EHCI_HCD=y
>   CONFIG_USB_STORAGE=y
> +CONFIG_DM_ETH=y
> +CONFIG_NET_RANDOM_ETHADDR=y
> +CONFIG_CMD_SATA=y
> +CONFIG_SATA_MV=y
> 

Re: [PATCH 3/3] arm: kirkwood: Dreamplug: Add DM SATA and remove IDE configs

[Stefan Roese]

On 26.07.21 08:01, Tony Dinh wrote:
> - Enable DM SATA, removed IDE driver, and add SATA MV driver.
> - Use ethernet PHY names from device tree in default boot command
> 
> Signed-off-by: Tony Dinh <mibodhi@gmail.com>

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

Thanks,
Stefan

> ---
> 
>   include/configs/dreamplug.h | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/configs/dreamplug.h b/include/configs/dreamplug.h
> index 9106203ebc..65962ee733 100644
> --- a/include/configs/dreamplug.h
> +++ b/include/configs/dreamplug.h
> @@ -31,8 +31,8 @@
>   /*
>    * Default environment variables
>    */
> -#define CONFIG_BOOTCOMMAND		"setenv ethact egiga0; " \
> -	"${x_bootcmd_ethernet}; setenv ethact egiga1; " \
> +#define CONFIG_BOOTCOMMAND		"setenv ethact ethernet-controller@72000; " \
> +	"${x_bootcmd_ethernet}; setenv ethact ethernet-controller@76000; " \
>   	"${x_bootcmd_ethernet}; ${x_bootcmd_usb}; ${x_bootcmd_kernel}; "\
>   	"setenv bootargs ${x_bootargs} ${x_bootargs_root}; "	\
>   	"bootm 0x6400000;"
> @@ -52,4 +52,12 @@
>   #define CONFIG_PHY_BASE_ADR	0
>   #endif /* CONFIG_CMD_NET */
>   
> +/*
> + * SATA Driver configuration
> + */
> +#ifdef CONFIG_SATA
> +#define CONFIG_SYS_SATA_MAX_DEVICE	1
> +#define CONFIG_LBA48
> +#endif /* CONFIG_SATA */
> +
>   #endif /* _CONFIG_DREAMPLUG_H */
> 


Viele Grüße,
Stefan

-- 
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

Re: [PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Tony Dinh]

Hi Stefan,

On Sat, Jul 31, 2021 at 12:41 AM Stefan Roese <sr@denx.de> wrote:
>
> On 26.07.21 08:01, Tony Dinh wrote:
> > In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer valid,
> > so replace these with Ethernet PHY names from device tree. Also, read
> > Ethernet PHY address for each port from device tree.
> >
> > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > ---
> >
> >   board/Marvell/dreamplug/dreamplug.c | 62 ++++++++++++++++++++++-------
> >   1 file changed, 48 insertions(+), 14 deletions(-)
> >
> > diff --git a/board/Marvell/dreamplug/dreamplug.c b/board/Marvell/dreamplug/dreamplug.c
> > index e1c64b5224..d5b6b22ddf 100644
> > --- a/board/Marvell/dreamplug/dreamplug.c
> > +++ b/board/Marvell/dreamplug/dreamplug.c
> > @@ -1,5 +1,6 @@
> >   // SPDX-License-Identifier: GPL-2.0+
> >   /*
> > + * Copyright (C) 2021  Tony Dinh <mibodhi@gmail.com>
> >    * (C) Copyright 2011
> >    * Jason Cooper <u-boot@lakedaemon.net>
> >    *
> > @@ -97,42 +98,75 @@ int board_init(void)
> >       return 0;
> >   }
> >
> > +static int fdt_get_phy_addr(const char *path)
> > +{
> > +     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;
> > +}
>
> You've added this exact some function now for the 3rd time IIRC. Could
> please please consolidate this function into one of the fdt / dt common
> functions instead? Perhaps there is already something similar for
> reading PHY addresses?
>
> Please do this in a follow-up patch, after I've pulled this one.

Yes, I realized this function needs consolidation too, and mentioned
it in the patch for the Sheevaplug. Currently, it is hard to find a
home for this. ./common/fdt_support.c is one level of abstraction
below this fdt_get_phy_addr(). So it is not appropriate for this
function to be in that file.

I've looked at various Armada 37x/38x SoC DTS, the binding is a bit
different, i.e. the "phy-handle" is not defined as consistently as in
Kirkwood DTSs, so this function would not work for Marvell SoCs, in
general.

How about if we put this function in a new area in
arch/arm/mach-kirkwood/? such as arch/arm/mach-kirkwood/fdt/ ?

I'll send in a separate patch for this, which will apply to the
Kirkwood boards which have already been converted to DM Ethernet.

Thanks,
Tony

> Other that this:
>
> Reviewed-by: Stefan Roese <sr@denx.de>
>
> Thanks,
> Stefan
>
>
> > +
> >   #ifdef CONFIG_RESET_PHY_R
> > -void mv_phy_88e1116_init(char *name)
> > +void mv_phy_88e1116_init(const char *name, const char *path)
> >   {
> >       u16 reg;
> > -     u16 devadr;
> > +     int phyaddr;
> >
> >       if (miiphy_set_current_dev(name))
> >               return;
> >
> > -     /* command to read PHY dev address */
> > -     if (miiphy_read(name, 0xEE, 0xEE, (u16 *) &devadr)) {
> > -             printf("Err..%s could not read PHY dev address\n",
> > -                     __func__);
> > +     phyaddr = fdt_get_phy_addr(path);
> > +     if (phyaddr < 0)
> >               return;
> > -     }
> >
> >       /*
> >        * Enable RGMII delay on Tx and Rx for CPU port
> >        * Ref: sec 4.7.2 of chip datasheet
> >        */
> > -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 2);
> > -     miiphy_read(name, devadr, MV88E1116_MAC_CTRL2_REG, &reg);
> > +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 2);
> > +     miiphy_read(name, phyaddr, MV88E1116_MAC_CTRL2_REG, &reg);
> >       reg |= (MV88E1116_RGMII_RXTM_CTRL | MV88E1116_RGMII_TXTM_CTRL);
> > -     miiphy_write(name, devadr, MV88E1116_MAC_CTRL2_REG, reg);
> > -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 0);
> > +     miiphy_write(name, phyaddr, MV88E1116_MAC_CTRL2_REG, reg);
> > +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 0);
> >
> >       /* reset the phy */
> > -     miiphy_reset(name, devadr);
> > +     miiphy_reset(name, phyaddr);
> >
> >       printf("88E1116 Initialized on %s\n", name);
> >   }
> >
> >   void reset_phy(void)
> >   {
> > +     char *eth0_name = "ethernet-controller@72000";
> > +     char *eth0_path = "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
> > +     char *eth1_name = "ethernet-controller@76000";
> > +     char *eth1_path = "/ocp@f1000000/ethernet-controller@72000/ethernet1-port@0";
> > +
> >       /* configure and initialize both PHY's */
> > -     mv_phy_88e1116_init("egiga0");
> > -     mv_phy_88e1116_init("egiga1");
> > +     mv_phy_88e1116_init(eth0_name, eth0_path);
> > +     mv_phy_88e1116_init(eth1_name, eth1_path);
> >   }
> >   #endif /* CONFIG_RESET_PHY_R */
> >
>
>
> Viele Grüße,
> Stefan
>
> --
> 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

Re: [PATCH 0/3] Convert the Dreamplug Ethernet and SATA to Driver Model.

[Stefan Roese]

On 26.07.21 08:01, Tony Dinh wrote:
> 
> - Enable DM SATA, removed IDE driver, add SATA MV driver.
> - Use Ethernet PHY names from device tree.
> - Replace the old Ethernet PHY addr lookup with a device tree parsing
> lookup function.
> 
> 
> Tony Dinh (3):
>    arm: kirkwood: Dreamplug: Add DM Ethernet and DM SATA configs
>    arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from
>      device tree
>    arm: kirkwood: Dreamplug: Add DM SATA and remove IDE configs
> 
>   board/Marvell/dreamplug/dreamplug.c | 62 ++++++++++++++++++++++-------
>   configs/dreamplug_defconfig         |  4 ++
>   include/configs/dreamplug.h         | 12 +++++-
>   3 files changed, 62 insertions(+), 16 deletions(-)

Applied to u-boot-marvell/master

Thanks,
Stefan

Re: [PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Stefan Roese]

Hi Tony,

(added Joe & Ramon as network custodians)

On 31.07.21 11:55, Tony Dinh wrote:
> Hi Stefan,
> 
> On Sat, Jul 31, 2021 at 12:41 AM Stefan Roese <sr@denx.de> wrote:
>>
>> On 26.07.21 08:01, Tony Dinh wrote:
>>> In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer valid,
>>> so replace these with Ethernet PHY names from device tree. Also, read
>>> Ethernet PHY address for each port from device tree.
>>>
>>> Signed-off-by: Tony Dinh <mibodhi@gmail.com>
>>> ---
>>>
>>>    board/Marvell/dreamplug/dreamplug.c | 62 ++++++++++++++++++++++-------
>>>    1 file changed, 48 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/board/Marvell/dreamplug/dreamplug.c b/board/Marvell/dreamplug/dreamplug.c
>>> index e1c64b5224..d5b6b22ddf 100644
>>> --- a/board/Marvell/dreamplug/dreamplug.c
>>> +++ b/board/Marvell/dreamplug/dreamplug.c
>>> @@ -1,5 +1,6 @@
>>>    // SPDX-License-Identifier: GPL-2.0+
>>>    /*
>>> + * Copyright (C) 2021  Tony Dinh <mibodhi@gmail.com>
>>>     * (C) Copyright 2011
>>>     * Jason Cooper <u-boot@lakedaemon.net>
>>>     *
>>> @@ -97,42 +98,75 @@ int board_init(void)
>>>        return 0;
>>>    }
>>>
>>> +static int fdt_get_phy_addr(const char *path)
>>> +{
>>> +     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;
>>> +}
>>
>> You've added this exact some function now for the 3rd time IIRC. Could
>> please please consolidate this function into one of the fdt / dt common
>> functions instead? Perhaps there is already something similar for
>> reading PHY addresses?
>>
>> Please do this in a follow-up patch, after I've pulled this one.
> 
> Yes, I realized this function needs consolidation too, and mentioned
> it in the patch for the Sheevaplug. Currently, it is hard to find a
> home for this. ./common/fdt_support.c is one level of abstraction
> below this fdt_get_phy_addr(). So it is not appropriate for this
> function to be in that file.

Joe, Ramon, do you know of any DT / FDT helper function already
available to read the PHY address from the device-tree? Or if not,
where would you like to have this new function being placed in the
U-Boot source tree?

> I've looked at various Armada 37x/38x SoC DTS, the binding is a bit
> different, i.e. the "phy-handle" is not defined as consistently as in
> Kirkwood DTSs, so this function would not work for Marvell SoCs, in
> general.

Not sure here (sorry for not looking deeper into the docs), but what is
the difference between the "phy-handle" and "phy" property here in the
ethernet DT nodes? Do we perhaps need to handle both properties?

> How about if we put this function in a new area in
> arch/arm/mach-kirkwood/? such as arch/arm/mach-kirkwood/fdt/ ?

I'm not a fan of using a kirkwood specific place here. This sounds like
a common (platform independent) issue and should be able to be used
by all boards.

Thanks,
Stefan

> I'll send in a separate patch for this, which will apply to the
> Kirkwood boards which have already been converted to DM Ethernet.
> 
> Thanks,
> Tony
> 
>> Other that this:
>>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>>
>> Thanks,
>> Stefan
>>
>>
>>> +
>>>    #ifdef CONFIG_RESET_PHY_R
>>> -void mv_phy_88e1116_init(char *name)
>>> +void mv_phy_88e1116_init(const char *name, const char *path)
>>>    {
>>>        u16 reg;
>>> -     u16 devadr;
>>> +     int phyaddr;
>>>
>>>        if (miiphy_set_current_dev(name))
>>>                return;
>>>
>>> -     /* command to read PHY dev address */
>>> -     if (miiphy_read(name, 0xEE, 0xEE, (u16 *) &devadr)) {
>>> -             printf("Err..%s could not read PHY dev address\n",
>>> -                     __func__);
>>> +     phyaddr = fdt_get_phy_addr(path);
>>> +     if (phyaddr < 0)
>>>                return;
>>> -     }
>>>
>>>        /*
>>>         * Enable RGMII delay on Tx and Rx for CPU port
>>>         * Ref: sec 4.7.2 of chip datasheet
>>>         */
>>> -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 2);
>>> -     miiphy_read(name, devadr, MV88E1116_MAC_CTRL2_REG, &reg);
>>> +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 2);
>>> +     miiphy_read(name, phyaddr, MV88E1116_MAC_CTRL2_REG, &reg);
>>>        reg |= (MV88E1116_RGMII_RXTM_CTRL | MV88E1116_RGMII_TXTM_CTRL);
>>> -     miiphy_write(name, devadr, MV88E1116_MAC_CTRL2_REG, reg);
>>> -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 0);
>>> +     miiphy_write(name, phyaddr, MV88E1116_MAC_CTRL2_REG, reg);
>>> +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 0);
>>>
>>>        /* reset the phy */
>>> -     miiphy_reset(name, devadr);
>>> +     miiphy_reset(name, phyaddr);
>>>
>>>        printf("88E1116 Initialized on %s\n", name);
>>>    }
>>>
>>>    void reset_phy(void)
>>>    {
>>> +     char *eth0_name = "ethernet-controller@72000";
>>> +     char *eth0_path = "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
>>> +     char *eth1_name = "ethernet-controller@76000";
>>> +     char *eth1_path = "/ocp@f1000000/ethernet-controller@72000/ethernet1-port@0";
>>> +
>>>        /* configure and initialize both PHY's */
>>> -     mv_phy_88e1116_init("egiga0");
>>> -     mv_phy_88e1116_init("egiga1");
>>> +     mv_phy_88e1116_init(eth0_name, eth0_path);
>>> +     mv_phy_88e1116_init(eth1_name, eth1_path);
>>>    }
>>>    #endif /* CONFIG_RESET_PHY_R */
>>>
>>
>>
>> Viele Grüße,
>> Stefan
>>
>> --
>> 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


Viele Grüße,
Stefan

-- 
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

Re: [PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Stefan Roese]

On 31.07.21 12:27, Stefan Roese wrote:
> Hi Tony,
> 
> (added Joe & Ramon as network custodians)
> 
> On 31.07.21 11:55, Tony Dinh wrote:
>> Hi Stefan,
>>
>> On Sat, Jul 31, 2021 at 12:41 AM Stefan Roese <sr@denx.de> wrote:
>>>
>>> On 26.07.21 08:01, Tony Dinh wrote:
>>>> In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer 
>>>> valid,
>>>> so replace these with Ethernet PHY names from device tree. Also, read
>>>> Ethernet PHY address for each port from device tree.
>>>>
>>>> Signed-off-by: Tony Dinh <mibodhi@gmail.com>
>>>> ---
>>>>
>>>>    board/Marvell/dreamplug/dreamplug.c | 62 
>>>> ++++++++++++++++++++++-------
>>>>    1 file changed, 48 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/board/Marvell/dreamplug/dreamplug.c 
>>>> b/board/Marvell/dreamplug/dreamplug.c
>>>> index e1c64b5224..d5b6b22ddf 100644
>>>> --- a/board/Marvell/dreamplug/dreamplug.c
>>>> +++ b/board/Marvell/dreamplug/dreamplug.c
>>>> @@ -1,5 +1,6 @@
>>>>    // SPDX-License-Identifier: GPL-2.0+
>>>>    /*
>>>> + * Copyright (C) 2021  Tony Dinh <mibodhi@gmail.com>
>>>>     * (C) Copyright 2011
>>>>     * Jason Cooper <u-boot@lakedaemon.net>
>>>>     *
>>>> @@ -97,42 +98,75 @@ int board_init(void)
>>>>        return 0;
>>>>    }
>>>>
>>>> +static int fdt_get_phy_addr(const char *path)
>>>> +{
>>>> +     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;
>>>> +}
>>>
>>> You've added this exact some function now for the 3rd time IIRC. Could
>>> please please consolidate this function into one of the fdt / dt common
>>> functions instead? Perhaps there is already something similar for
>>> reading PHY addresses?
>>>
>>> Please do this in a follow-up patch, after I've pulled this one.
>>
>> Yes, I realized this function needs consolidation too, and mentioned
>> it in the patch for the Sheevaplug. Currently, it is hard to find a
>> home for this. ./common/fdt_support.c is one level of abstraction
>> below this fdt_get_phy_addr(). So it is not appropriate for this
>> function to be in that file.
> 
> Joe, Ramon, do you know of any DT / FDT helper function already
> available to read the PHY address from the device-tree?

It just now came to my mind that the "normal" functions for base address
reading like dev_read_addr(), fdt_get_base_address() and friends can
most likely be used for PHY MDIO address reading as well.

Thanks,
Stefan

> Or if not,
> where would you like to have this new function being placed in the
> U-Boot source tree?
> 
>> I've looked at various Armada 37x/38x SoC DTS, the binding is a bit
>> different, i.e. the "phy-handle" is not defined as consistently as in
>> Kirkwood DTSs, so this function would not work for Marvell SoCs, in
>> general.
> 
> Not sure here (sorry for not looking deeper into the docs), but what is
> the difference between the "phy-handle" and "phy" property here in the
> ethernet DT nodes? Do we perhaps need to handle both properties?
> 
>> How about if we put this function in a new area in
>> arch/arm/mach-kirkwood/? such as arch/arm/mach-kirkwood/fdt/ ?
> 
> I'm not a fan of using a kirkwood specific place here. This sounds like
> a common (platform independent) issue and should be able to be used
> by all boards.
> 
> Thanks,
> Stefan
> 
>> I'll send in a separate patch for this, which will apply to the
>> Kirkwood boards which have already been converted to DM Ethernet.
>>
>> Thanks,
>> Tony
>>
>>> Other that this:
>>>
>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>>
>>> Thanks,
>>> Stefan
>>>
>>>
>>>> +
>>>>    #ifdef CONFIG_RESET_PHY_R
>>>> -void mv_phy_88e1116_init(char *name)
>>>> +void mv_phy_88e1116_init(const char *name, const char *path)
>>>>    {
>>>>        u16 reg;
>>>> -     u16 devadr;
>>>> +     int phyaddr;
>>>>
>>>>        if (miiphy_set_current_dev(name))
>>>>                return;
>>>>
>>>> -     /* command to read PHY dev address */
>>>> -     if (miiphy_read(name, 0xEE, 0xEE, (u16 *) &devadr)) {
>>>> -             printf("Err..%s could not read PHY dev address\n",
>>>> -                     __func__);
>>>> +     phyaddr = fdt_get_phy_addr(path);
>>>> +     if (phyaddr < 0)
>>>>                return;
>>>> -     }
>>>>
>>>>        /*
>>>>         * Enable RGMII delay on Tx and Rx for CPU port
>>>>         * Ref: sec 4.7.2 of chip datasheet
>>>>         */
>>>> -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 2);
>>>> -     miiphy_read(name, devadr, MV88E1116_MAC_CTRL2_REG, &reg);
>>>> +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 2);
>>>> +     miiphy_read(name, phyaddr, MV88E1116_MAC_CTRL2_REG, &reg);
>>>>        reg |= (MV88E1116_RGMII_RXTM_CTRL | MV88E1116_RGMII_TXTM_CTRL);
>>>> -     miiphy_write(name, devadr, MV88E1116_MAC_CTRL2_REG, reg);
>>>> -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 0);
>>>> +     miiphy_write(name, phyaddr, MV88E1116_MAC_CTRL2_REG, reg);
>>>> +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 0);
>>>>
>>>>        /* reset the phy */
>>>> -     miiphy_reset(name, devadr);
>>>> +     miiphy_reset(name, phyaddr);
>>>>
>>>>        printf("88E1116 Initialized on %s\n", name);
>>>>    }
>>>>
>>>>    void reset_phy(void)
>>>>    {
>>>> +     char *eth0_name = "ethernet-controller@72000";
>>>> +     char *eth0_path = 
>>>> "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
>>>> +     char *eth1_name = "ethernet-controller@76000";
>>>> +     char *eth1_path = 
>>>> "/ocp@f1000000/ethernet-controller@72000/ethernet1-port@0";
>>>> +
>>>>        /* configure and initialize both PHY's */
>>>> -     mv_phy_88e1116_init("egiga0");
>>>> -     mv_phy_88e1116_init("egiga1");
>>>> +     mv_phy_88e1116_init(eth0_name, eth0_path);
>>>> +     mv_phy_88e1116_init(eth1_name, eth1_path);
>>>>    }
>>>>    #endif /* CONFIG_RESET_PHY_R */
>>>>
>>>
>>>
>>> Viele Grüße,
>>> Stefan
>>>
>>> -- 
>>> 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
> 
> 
> Viele Grüße,
> Stefan
> 


Viele Grüße,
Stefan

-- 
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

Re: [PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Tony Dinh]

Hi Stefan,

On Sat, Jul 31, 2021 at 4:50 AM Stefan Roese <sr@denx.de> wrote:
>
> On 31.07.21 12:27, Stefan Roese wrote:
> > Hi Tony,
> >
> > (added Joe & Ramon as network custodians)
> >
> > On 31.07.21 11:55, Tony Dinh wrote:
> >> Hi Stefan,
> >>
> >> On Sat, Jul 31, 2021 at 12:41 AM Stefan Roese <sr@denx.de> wrote:
> >>>
> >>> On 26.07.21 08:01, Tony Dinh wrote:
> >>>> In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer
> >>>> valid,
> >>>> so replace these with Ethernet PHY names from device tree. Also, read
> >>>> Ethernet PHY address for each port from device tree.
> >>>>
> >>>> Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> >>>> ---
> >>>>
> >>>>    board/Marvell/dreamplug/dreamplug.c | 62
> >>>> ++++++++++++++++++++++-------
> >>>>    1 file changed, 48 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/board/Marvell/dreamplug/dreamplug.c
> >>>> b/board/Marvell/dreamplug/dreamplug.c
> >>>> index e1c64b5224..d5b6b22ddf 100644
> >>>> --- a/board/Marvell/dreamplug/dreamplug.c
> >>>> +++ b/board/Marvell/dreamplug/dreamplug.c
> >>>> @@ -1,5 +1,6 @@
> >>>>    // SPDX-License-Identifier: GPL-2.0+
> >>>>    /*
> >>>> + * Copyright (C) 2021  Tony Dinh <mibodhi@gmail.com>
> >>>>     * (C) Copyright 2011
> >>>>     * Jason Cooper <u-boot@lakedaemon.net>
> >>>>     *
> >>>> @@ -97,42 +98,75 @@ int board_init(void)
> >>>>        return 0;
> >>>>    }
> >>>>
> >>>> +static int fdt_get_phy_addr(const char *path)
> >>>> +{
> >>>> +     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;
> >>>> +}
> >>>
> >>> You've added this exact some function now for the 3rd time IIRC. Could
> >>> please please consolidate this function into one of the fdt / dt common
> >>> functions instead? Perhaps there is already something similar for
> >>> reading PHY addresses?
> >>>
> >>> Please do this in a follow-up patch, after I've pulled this one.
> >>
> >> Yes, I realized this function needs consolidation too, and mentioned
> >> it in the patch for the Sheevaplug. Currently, it is hard to find a
> >> home for this. ./common/fdt_support.c is one level of abstraction
> >> below this fdt_get_phy_addr(). So it is not appropriate for this
> >> function to be in that file.
> >
> > Joe, Ramon, do you know of any DT / FDT helper function already
> > available to read the PHY address from the device-tree?
>
> It just now came to my mind that the "normal" functions for base address
> reading like dev_read_addr(), fdt_get_base_address() and friends can
> most likely be used for PHY MDIO address reading as well.

Thanks for the suggestion. I agree that would work, too. I did
consider going down the MDIO path and looking up "reg" a bit quicker
because we know what the PHY Base address is. But in the scenario that
we have 2 or more active Ethernet ports (eg. Dreamplug board has 2),
the lookup function is not as elegant.  It seems more straightforward
if we go down the Eth0 or Eth1 path from the DTSI and then follow the
"phy-handle" to the MDIO ethernet-phy node.

Regarding your previous question about  "phy" vs. "phy-handle",
according to the current device tree guideline, "phy" is deprecated
and "phy-handle" is the one we should use. And there are some Armada
37x/38x boards that use the deprecated name "phy" currently.

Thanks,
Tony




> Thanks,
> Stefan
>
> > Or if not,
> > where would you like to have this new function being placed in the
> > U-Boot source tree?
> >
> >> I've looked at various Armada 37x/38x SoC DTS, the binding is a bit
> >> different, i.e. the "phy-handle" is not defined as consistently as in
> >> Kirkwood DTSs, so this function would not work for Marvell SoCs, in
> >> general.
> >
> > Not sure here (sorry for not looking deeper into the docs), but what is
> > the difference between the "phy-handle" and "phy" property here in the
> > ethernet DT nodes? Do we perhaps need to handle both properties?
> >
> >> How about if we put this function in a new area in
> >> arch/arm/mach-kirkwood/? such as arch/arm/mach-kirkwood/fdt/ ?
> >
> > I'm not a fan of using a kirkwood specific place here. This sounds like
> > a common (platform independent) issue and should be able to be used
> > by all boards.
> >
> > Thanks,
> > Stefan
> >
> >> I'll send in a separate patch for this, which will apply to the
> >> Kirkwood boards which have already been converted to DM Ethernet.
> >>
> >> Thanks,
> >> Tony
> >>
> >>> Other that this:
> >>>
> >>> Reviewed-by: Stefan Roese <sr@denx.de>
> >>>
> >>> Thanks,
> >>> Stefan
> >>>
> >>>
> >>>> +
> >>>>    #ifdef CONFIG_RESET_PHY_R
> >>>> -void mv_phy_88e1116_init(char *name)
> >>>> +void mv_phy_88e1116_init(const char *name, const char *path)
> >>>>    {
> >>>>        u16 reg;
> >>>> -     u16 devadr;
> >>>> +     int phyaddr;
> >>>>
> >>>>        if (miiphy_set_current_dev(name))
> >>>>                return;
> >>>>
> >>>> -     /* command to read PHY dev address */
> >>>> -     if (miiphy_read(name, 0xEE, 0xEE, (u16 *) &devadr)) {
> >>>> -             printf("Err..%s could not read PHY dev address\n",
> >>>> -                     __func__);
> >>>> +     phyaddr = fdt_get_phy_addr(path);
> >>>> +     if (phyaddr < 0)
> >>>>                return;
> >>>> -     }
> >>>>
> >>>>        /*
> >>>>         * Enable RGMII delay on Tx and Rx for CPU port
> >>>>         * Ref: sec 4.7.2 of chip datasheet
> >>>>         */
> >>>> -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 2);
> >>>> -     miiphy_read(name, devadr, MV88E1116_MAC_CTRL2_REG, &reg);
> >>>> +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 2);
> >>>> +     miiphy_read(name, phyaddr, MV88E1116_MAC_CTRL2_REG, &reg);
> >>>>        reg |= (MV88E1116_RGMII_RXTM_CTRL | MV88E1116_RGMII_TXTM_CTRL);
> >>>> -     miiphy_write(name, devadr, MV88E1116_MAC_CTRL2_REG, reg);
> >>>> -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 0);
> >>>> +     miiphy_write(name, phyaddr, MV88E1116_MAC_CTRL2_REG, reg);
> >>>> +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 0);
> >>>>
> >>>>        /* reset the phy */
> >>>> -     miiphy_reset(name, devadr);
> >>>> +     miiphy_reset(name, phyaddr);
> >>>>
> >>>>        printf("88E1116 Initialized on %s\n", name);
> >>>>    }
> >>>>
> >>>>    void reset_phy(void)
> >>>>    {
> >>>> +     char *eth0_name = "ethernet-controller@72000";
> >>>> +     char *eth0_path =
> >>>> "/ocp@f1000000/ethernet-controller@72000/ethernet0-port@0";
> >>>> +     char *eth1_name = "ethernet-controller@76000";
> >>>> +     char *eth1_path =
> >>>> "/ocp@f1000000/ethernet-controller@72000/ethernet1-port@0";
> >>>> +
> >>>>        /* configure and initialize both PHY's */
> >>>> -     mv_phy_88e1116_init("egiga0");
> >>>> -     mv_phy_88e1116_init("egiga1");
> >>>> +     mv_phy_88e1116_init(eth0_name, eth0_path);
> >>>> +     mv_phy_88e1116_init(eth1_name, eth1_path);
> >>>>    }
> >>>>    #endif /* CONFIG_RESET_PHY_R */
> >>>>
> >>>
> >>>
> >>> Viele Grüße,
> >>> Stefan
> >>>
> >>> --
> >>> 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
> >
> >
> > Viele Grüße,
> > Stefan
> >
>
>
> Viele Grüße,
> Stefan
>
> --
> 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

Re: [PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Stefan Roese]

Hi Tony,

On 01.08.21 00:37, Tony Dinh wrote:
> Hi Stefan,
> 
> On Sat, Jul 31, 2021 at 4:50 AM Stefan Roese <sr@denx.de> wrote:
>>
>> On 31.07.21 12:27, Stefan Roese wrote:
>>> Hi Tony,
>>>
>>> (added Joe & Ramon as network custodians)
>>>
>>> On 31.07.21 11:55, Tony Dinh wrote:
>>>> Hi Stefan,
>>>>
>>>> On Sat, Jul 31, 2021 at 12:41 AM Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> On 26.07.21 08:01, Tony Dinh wrote:
>>>>>> In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer
>>>>>> valid,
>>>>>> so replace these with Ethernet PHY names from device tree. Also, read
>>>>>> Ethernet PHY address for each port from device tree.
>>>>>>
>>>>>> Signed-off-by: Tony Dinh <mibodhi@gmail.com>
>>>>>> ---
>>>>>>
>>>>>>     board/Marvell/dreamplug/dreamplug.c | 62
>>>>>> ++++++++++++++++++++++-------
>>>>>>     1 file changed, 48 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/board/Marvell/dreamplug/dreamplug.c
>>>>>> b/board/Marvell/dreamplug/dreamplug.c
>>>>>> index e1c64b5224..d5b6b22ddf 100644
>>>>>> --- a/board/Marvell/dreamplug/dreamplug.c
>>>>>> +++ b/board/Marvell/dreamplug/dreamplug.c
>>>>>> @@ -1,5 +1,6 @@
>>>>>>     // SPDX-License-Identifier: GPL-2.0+
>>>>>>     /*
>>>>>> + * Copyright (C) 2021  Tony Dinh <mibodhi@gmail.com>
>>>>>>      * (C) Copyright 2011
>>>>>>      * Jason Cooper <u-boot@lakedaemon.net>
>>>>>>      *
>>>>>> @@ -97,42 +98,75 @@ int board_init(void)
>>>>>>         return 0;
>>>>>>     }
>>>>>>
>>>>>> +static int fdt_get_phy_addr(const char *path)
>>>>>> +{
>>>>>> +     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;
>>>>>> +}
>>>>>
>>>>> You've added this exact some function now for the 3rd time IIRC. Could
>>>>> please please consolidate this function into one of the fdt / dt common
>>>>> functions instead? Perhaps there is already something similar for
>>>>> reading PHY addresses?
>>>>>
>>>>> Please do this in a follow-up patch, after I've pulled this one.
>>>>
>>>> Yes, I realized this function needs consolidation too, and mentioned
>>>> it in the patch for the Sheevaplug. Currently, it is hard to find a
>>>> home for this. ./common/fdt_support.c is one level of abstraction
>>>> below this fdt_get_phy_addr(). So it is not appropriate for this
>>>> function to be in that file.
>>>
>>> Joe, Ramon, do you know of any DT / FDT helper function already
>>> available to read the PHY address from the device-tree?
>>
>> It just now came to my mind that the "normal" functions for base address
>> reading like dev_read_addr(), fdt_get_base_address() and friends can
>> most likely be used for PHY MDIO address reading as well.
> 
> Thanks for the suggestion. I agree that would work, too. I did
> consider going down the MDIO path and looking up "reg" a bit quicker
> because we know what the PHY Base address is. But in the scenario that
> we have 2 or more active Ethernet ports (eg. Dreamplug board has 2),
> the lookup function is not as elegant.  It seems more straightforward
> if we go down the Eth0 or Eth1 path from the DTSI and then follow the
> "phy-handle" to the MDIO ethernet-phy node.

... and also "phy" to make this compatible with this deprecated property
as well?

But you could still use the already available functions to read the
"reg" property instead of coding it once again in this new function?

> Regarding your previous question about  "phy" vs. "phy-handle",
> according to the current device tree guideline, "phy" is deprecated
> and "phy-handle" is the one we should use. And there are some Armada
> 37x/38x boards that use the deprecated name "phy" currently.

Thanks. Please see my comment above.

Thanks,
Stefan

Re: [PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Tony Dinh]

Hi Stefan,

On Sun, Aug 1, 2021 at 11:39 PM Stefan Roese <sr@denx.de> wrote:
>
> Hi Tony,
>
> On 01.08.21 00:37, Tony Dinh wrote:
> > Hi Stefan,
> >
> > On Sat, Jul 31, 2021 at 4:50 AM Stefan Roese <sr@denx.de> wrote:
> >>
> >> On 31.07.21 12:27, Stefan Roese wrote:
> >>> Hi Tony,
> >>>
> >>> (added Joe & Ramon as network custodians)
> >>>
> >>> On 31.07.21 11:55, Tony Dinh wrote:
> >>>> Hi Stefan,
> >>>>
> >>>> On Sat, Jul 31, 2021 at 12:41 AM Stefan Roese <sr@denx.de> wrote:
> >>>>>
> >>>>> On 26.07.21 08:01, Tony Dinh wrote:
> >>>>>> In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer
> >>>>>> valid,
> >>>>>> so replace these with Ethernet PHY names from device tree. Also, read
> >>>>>> Ethernet PHY address for each port from device tree.
> >>>>>>
> >>>>>> Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> >>>>>> ---
> >>>>>>
> >>>>>>     board/Marvell/dreamplug/dreamplug.c | 62
> >>>>>> ++++++++++++++++++++++-------
> >>>>>>     1 file changed, 48 insertions(+), 14 deletions(-)
> >>>>>>
> >>>>>> diff --git a/board/Marvell/dreamplug/dreamplug.c
> >>>>>> b/board/Marvell/dreamplug/dreamplug.c
> >>>>>> index e1c64b5224..d5b6b22ddf 100644
> >>>>>> --- a/board/Marvell/dreamplug/dreamplug.c
> >>>>>> +++ b/board/Marvell/dreamplug/dreamplug.c
> >>>>>> @@ -1,5 +1,6 @@
> >>>>>>     // SPDX-License-Identifier: GPL-2.0+
> >>>>>>     /*
> >>>>>> + * Copyright (C) 2021  Tony Dinh <mibodhi@gmail.com>
> >>>>>>      * (C) Copyright 2011
> >>>>>>      * Jason Cooper <u-boot@lakedaemon.net>
> >>>>>>      *
> >>>>>> @@ -97,42 +98,75 @@ int board_init(void)
> >>>>>>         return 0;
> >>>>>>     }
> >>>>>>
> >>>>>> +static int fdt_get_phy_addr(const char *path)
> >>>>>> +{
> >>>>>> +     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;
> >>>>>> +}
> >>>>>
> >>>>> You've added this exact some function now for the 3rd time IIRC. Could
> >>>>> please please consolidate this function into one of the fdt / dt common
> >>>>> functions instead? Perhaps there is already something similar for
> >>>>> reading PHY addresses?
> >>>>>
> >>>>> Please do this in a follow-up patch, after I've pulled this one.
> >>>>
> >>>> Yes, I realized this function needs consolidation too, and mentioned
> >>>> it in the patch for the Sheevaplug. Currently, it is hard to find a
> >>>> home for this. ./common/fdt_support.c is one level of abstraction
> >>>> below this fdt_get_phy_addr(). So it is not appropriate for this
> >>>> function to be in that file.
> >>>
> >>> Joe, Ramon, do you know of any DT / FDT helper function already
> >>> available to read the PHY address from the device-tree?
> >>
> >> It just now came to my mind that the "normal" functions for base address
> >> reading like dev_read_addr(), fdt_get_base_address() and friends can
> >> most likely be used for PHY MDIO address reading as well.
> >
> > Thanks for the suggestion. I agree that would work, too. I did
> > consider going down the MDIO path and looking up "reg" a bit quicker
> > because we know what the PHY Base address is. But in the scenario that
> > we have 2 or more active Ethernet ports (eg. Dreamplug board has 2),
> > the lookup function is not as elegant.  It seems more straightforward
> > if we go down the Eth0 or Eth1 path from the DTSI and then follow the
> > "phy-handle" to the MDIO ethernet-phy node.
>
> ... and also "phy" to make this compatible with this deprecated property
> as well?
>
> But you could still use the already available functions to read the
> "reg" property instead of coding it once again in this new function?
>
> > Regarding your previous question about  "phy" vs. "phy-handle",
> > according to the current device tree guideline, "phy" is deprecated
> > and "phy-handle" is the one we should use. And there are some Armada
> > 37x/38x boards that use the deprecated name "phy" currently.
>
> Thanks. Please see my comment above.

Thanks for your review! I'll send in a patch to propose a new home (in
/common) for this function, and also update it to look up for the
deprecated "phy" also. Once we have that done, I'll go back and submit
patches for the Kirkwood boards that have this function hardcoded in.

Thanks,
Tony

>
> Thanks,
> Stefan