ARM: imx6: init enet MAC address

PATCH New Fugang Duan 4 participants 8 messages May 20, 2014 – May 22, 2014

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape

lore

Share & tools

lore

Fugang Duan

author

2014-05-20 06:11 UTC #0 lore raw

PATCH 1/1 ARM: imx6: init enet MAC address

Enet get MAC address order:

Quoted 1 line

From module parameters or kernel command line -> device tree ->

pfuse -> mac registers set by bootloader -> random mac address.

When there have no "fec.macaddr" parameters set in kernel command line, enet driver get MAC address from device tree. And then if the MAC address set in device tree and is valid, enet driver get MAC address from device tree. Otherwise, enet get MAC address from pfuse. So, in the condition, update the MAC address (read from pfuse) to device tree.

Signed-off-by Fugang Duan <[email protected]>

---
 arch/arm/mach-imx/common.h      |    1 +
 arch/arm/mach-imx/mach-imx6q.c  |   90 ++++++++++++++++++++++++++++++++++++++-
 arch/arm/mach-imx/mach-imx6sl.c |    8 +++-
 3 files changed, 96 insertions(+), 3 deletions(-)

arch/arm/mach-imx/common.h +1 -0

index 4facd01..5c69325 100644
@@ -145,6 +145,7 @@ void imx6sl_set_wait_clk(bool enter);
 
 void imx_cpu_die(unsigned int cpu);
 int imx_cpu_kill(unsigned int cpu);
+void imx6_enet_mac_init(const char *compatible);
 
 #ifdef CONFIG_SUSPEND
 void v7_cpu_resume(void);

arch/arm/mach-imx/mach-imx6q.c +88 -2

119 lines not on this page · raw message

arch/arm/mach-imx/mach-imx6sl.c +7 -1

index ad32338..cb001d4 100644
@@ -19,7 +19,7 @@
 #include "common.h"
 #include "cpuidle.h"
 
-static void __init imx6sl_fec_init(void)
+static void __init imx6sl_fec_clk_init(void)
 {
 	struct regmap *gpr;

@@ -35,6 +35,12 @@ static void __init imx6sl_fec_init(void)
 	}
 }
 
+static inline void imx6sl_fec_init(void)
+{
+	imx6sl_fec_clk_init();
+	imx6_enet_mac_init("fsl,imx6sl-fec");
+}
+
 static void __init imx6sl_init_late(void)
 {
 	/* imx6sl reuses imx6q cpufreq driver */

Signature · 1 line

1.7.8

Sascha Hauer

maintainer

2014-05-20 09:03 UTC #1 lore raw

PATCH 1/1 ARM: imx6: init enet MAC address

Quoted 11 lines · Fugang Duan

Enet get MAC address order:
From module parameters or kernel command line -> device tree -> pfuse -> mac registers set by bootloader -> random mac address.
When there have no "fec.macaddr" parameters set in kernel command line, enet driver get MAC address from device tree. And then if the MAC address set in device tree and is valid, enet driver get MAC address from device tree. Otherwise, enet get MAC address from pfuse. So, in the condition, update the MAC address (read from pfuse) to device tree.

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 170

Quoted 3 lines

arch/arm/mach-imx/mach-imx6q.c +3 -0

+#define OCOTP_MACn(n)		(0x00000620 + (n) * 0x10)
+#define IMX_MAX_ENET_NUM	2
+void __init imx6_enet_mac_init(const char *compatible)

static

Quoted 21 lines

arch/arm/mach-imx/mach-imx6q.c +21 -0

+{
+	struct device_node *ocotp_np, *enet_np, *from = NULL;
+	void __iomem *base;
+	struct property *newmac;
+	u32 macaddr0_low;
+	u32 macaddr0_high = 0;
+	u32 macaddr1_high = 0;
+	u8 *macaddr;
+	int i;
+
+	for (i = 0; i < IMX_MAX_ENET_NUM; i++) {
+		enet_np = of_find_compatible_node(from, NULL, compatible);
+		if (!enet_np)
+			return;
+
+		from = enet_np;
+
+		if (of_get_mac_address(enet_np))
+			goto put_enet_node;
+
+		ocotp_np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 201

Quoted 10 lines

arch/arm/mach-imx/mach-imx6q.c +10 -0

+		if (!ocotp_np) {
+			pr_warn("failed to find ocotp node\n");
+			goto put_enet_node;
+		}
+
+		base = of_iomap(ocotp_np, 0);
+		if (!base) {
+			pr_warn("failed to map ocotp\n");
+			goto put_ocotp_node;
+		}

Move this outside the loop.

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 207

Quoted 6 lines

arch/arm/mach-imx/mach-imx6q.c +6 -0

+
+		macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
+		if (i)
+			macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
+		else
+			macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));

You go over these register reads two times and read the very same registers two times. Argh! Could please look at your code before posting it?

Quoted 19 lines

arch/arm/mach-imx/mach-imx6q.c +19 -0

+
+		newmac = kzalloc(sizeof(*newmac) + 6, GFP_KERNEL);
+		if (!newmac)
+			goto put_ocotp_node;
+
+		newmac->value = newmac + 1;
+		newmac->length = 6;
+		newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
+		if (!newmac->name) {
+			kfree(newmac);
+			goto put_ocotp_node;
+		}
+
+		macaddr = newmac->value;
+		if (i) {
+			macaddr[0] = (macaddr1_high >> 24) & 0xff;
+			macaddr[1] = (macaddr1_high >> 16) & 0xff;
+			macaddr[2] = (macaddr1_high >> 8) & 0xff;
+			macaddr[3] = macaddr1_high & 0xff;

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 236

Quoted 10 lines

arch/arm/mach-imx/mach-imx6q.c +10 -0

+			macaddr[4] = (macaddr0_low >> 24) & 0xff;
+			macaddr[5] = (macaddr0_low >> 16) & 0xff;
+		} else {
+			macaddr[0] = (macaddr0_low >> 8) & 0xff;
+			macaddr[1] = macaddr0_low & 0xff;
+			macaddr[2] = (macaddr0_high >> 24) & 0xff;
+			macaddr[3] = (macaddr0_high >> 16) & 0xff;
+			macaddr[4] = (macaddr0_high >> 8) & 0xff;
+			macaddr[5] = macaddr0_high & 0xff;
+		}

It makes no sense to have a IMX_MAX_ENET_NUM define when the code assumes that the only valid value is 2.

All this is probably far more readable and less fishy when you move the fixup of a single enet device to a extra function which you call from the loop.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

fugang.duan at freescale.com

2014-05-20 09:24 UTC #2 lore raw

PATCH 1/1 ARM: imx6: init enet MAC address

From: Sascha Hauer <[email protected]>
Data: Tuesday, May 20, 2014 5:03 PM

Quoted 17 lines

To: Duan Fugang-B38611
Cc: Li Frank-B20596; Guo Shawn-R65073; linux-arm- kernel at lists.infradead.org; kernel at pengutronix.de
Subject: Re: [PATCH] ARM: imx6: init enet MAC address
Quoted 11 lines · Fugang Duan
Enet get MAC address order:
From module parameters or kernel command line -> device tree -> pfuse -> mac registers set by bootloader -> random mac address.
When there have no "fec.macaddr" parameters set in kernel command line, enet driver get MAC address from device tree. And then if the MAC address set in device tree and is valid, enet driver get MAC address from device tree. Otherwise, enet get MAC address from pfuse. So, in the condition, update the MAC address (read from pfuse) to device tree.

Quoted 6 lines

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 170
Quoted 3 lines
arch/arm/mach-imx/mach-imx6q.c +3 -0
+#define OCOTP_MACn(n)		(0x00000620 + (n) * 0x10)
+#define IMX_MAX_ENET_NUM	2
+void __init imx6_enet_mac_init(const char *compatible)
static

The .fun() will be called for imx6sl, imx6sx(the latest chip) machine files.

Quoted 22 lines

Quoted 11 lines

arch/arm/mach-imx/mach-imx6q.c +11 -0

+{
+	struct device_node *ocotp_np, *enet_np, *from = NULL;
+	void __iomem *base;
+	struct property *newmac;
+	u32 macaddr0_low;
+	u32 macaddr0_high = 0;
+	u32 macaddr1_high = 0;
+	u8 *macaddr;
+	int i;
+
+	for (i = 0; i < IMX_MAX_ENET_NUM; i++) {

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev)

Quoted 10 lines

arch/arm/mach-imx/mach-imx6q.c +10 -0

+		enet_np = of_find_compatible_node(from, NULL, compatible);
+		if (!enet_np)
+			return;
+
+		from = enet_np;
+
+		if (of_get_mac_address(enet_np))
+			goto put_enet_node;
+
+		ocotp_np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-

ocotp");

Quoted 13 lines

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 201
Quoted 10 lines
arch/arm/mach-imx/mach-imx6q.c +10 -0
+		if (!ocotp_np) {
+			pr_warn("failed to find ocotp node\n");
+			goto put_enet_node;
+		}
+
+		base = of_iomap(ocotp_np, 0);
+		if (!base) {
+			pr_warn("failed to map ocotp\n");
+			goto put_ocotp_node;
+		}
Move this outside the loop.

Agree, I will move the ocotp node from the loop.

Quoted 11 lines

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 207
Quoted 6 lines
arch/arm/mach-imx/mach-imx6q.c +6 -0
+
+		macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
+		if (i)
+			macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
+		else
+			macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));
You go over these register reads two times and read the very same registers two times. Argh! Could please look at your code before posting it?

For some other platform like imx6sx has two enet MACs, so "i" is 0, 1. You means OCOTP_MACn(1) is read twice, I will change it. Thanks.

Quoted 24 lines

arch/arm/mach-imx/mach-imx6q.c +24 -0

+
+		newmac = kzalloc(sizeof(*newmac) + 6, GFP_KERNEL);
+		if (!newmac)
+			goto put_ocotp_node;
+
+		newmac->value = newmac + 1;
+		newmac->length = 6;
+		newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
+		if (!newmac->name) {
+			kfree(newmac);
+			goto put_ocotp_node;
+		}
+
+		macaddr = newmac->value;
+		if (i) {
+			macaddr[0] = (macaddr1_high >> 24) & 0xff;
+			macaddr[1] = (macaddr1_high >> 16) & 0xff;
+			macaddr[2] = (macaddr1_high >> 8) & 0xff;
+			macaddr[3] = macaddr1_high & 0xff;
+			macaddr[4] = (macaddr0_low >> 24) & 0xff;
+			macaddr[5] = (macaddr0_low >> 16) & 0xff;
+		} else {
+			macaddr[0] = (macaddr0_low >> 8) & 0xff;
+			macaddr[1] = macaddr0_low & 0xff;

Quoted 9 lines

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 236
Quoted 5 lines
arch/arm/mach-imx/mach-imx6q.c +5 -0
+			macaddr[2] = (macaddr0_high >> 24) & 0xff;
+			macaddr[3] = (macaddr0_high >> 16) & 0xff;
+			macaddr[4] = (macaddr0_high >> 8) & 0xff;
+			macaddr[5] = macaddr0_high & 0xff;
+		}
It makes no sense to have a IMX_MAX_ENET_NUM define when the code assumes that the only valid value is 2.

You mean remove the "IMX_MAX_ENET_NUM" ?

Quoted 4 lines

All this is probably far more readable and less fishy when you move the fixup of a single enet device to a extra function which you call from the loop.

Because imx6sx has two enet MACs, and the function is called for imx6qdl/imx6sl/imx6sx, so add the loop in here. For imx6qdl/imx6sl, there just have one MAC, the second loop don't go through the flow.
Do you have other suggestion ?

Thanks!

Sascha Hauer

maintainer

2014-05-20 09:57 UTC #3 lore raw

PATCH 1/1 ARM: imx6: init enet MAC address

Quoted 76 lines · fugang.duan at freescale.com

From: Sascha Hauer <[email protected]>
Data: Tuesday, May 20, 2014 5:03 PM
Quoted 17 lines
To: Duan Fugang-B38611
Cc: Li Frank-B20596; Guo Shawn-R65073; linux-arm- kernel at lists.infradead.org; kernel at pengutronix.de
Subject: Re: [PATCH] ARM: imx6: init enet MAC address
Quoted 11 lines · Fugang Duan
Enet get MAC address order:
From module parameters or kernel command line -> device tree -> pfuse -> mac registers set by bootloader -> random mac address.
When there have no "fec.macaddr" parameters set in kernel command line, enet driver get MAC address from device tree. And then if the MAC address set in device tree and is valid, enet driver get MAC address from device tree. Otherwise, enet get MAC address from pfuse. So, in the condition, update the MAC address (read from pfuse) to device tree.
Quoted 6 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 170
Quoted 3 lines
arch/arm/mach-imx/mach-imx6q.c +3 -0
+#define OCOTP_MACn(n)		(0x00000620 + (n) * 0x10)
+#define IMX_MAX_ENET_NUM	2
+void __init imx6_enet_mac_init(const char *compatible)
static
The .fun() will be called for imx6sl, imx6sx(the latest chip) machine files.
Quoted 22 lines
Quoted 11 lines
arch/arm/mach-imx/mach-imx6q.c +11 -0
+{
+	struct device_node *ocotp_np, *enet_np, *from = NULL;
+	void __iomem *base;
+	struct property *newmac;
+	u32 macaddr0_low;
+	u32 macaddr0_high = 0;
+	u32 macaddr1_high = 0;
+	u8 *macaddr;
+	int i;
+
+	for (i = 0; i < IMX_MAX_ENET_NUM; i++) {
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev)
Quoted 10 lines
arch/arm/mach-imx/mach-imx6q.c +10 -0
+		enet_np = of_find_compatible_node(from, NULL, compatible);
+		if (!enet_np)
+			return;
+
+		from = enet_np;
+
+		if (of_get_mac_address(enet_np))
+			goto put_enet_node;
+
+		ocotp_np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-
ocotp");
Quoted 13 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 201
Quoted 10 lines
arch/arm/mach-imx/mach-imx6q.c +10 -0
+		if (!ocotp_np) {
+			pr_warn("failed to find ocotp node\n");
+			goto put_enet_node;
+		}
+
+		base = of_iomap(ocotp_np, 0);
+		if (!base) {
+			pr_warn("failed to map ocotp\n");
+			goto put_ocotp_node;
+		}
Move this outside the loop.
Agree, I will move the ocotp node from the loop.
Quoted 11 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 207
Quoted 6 lines
arch/arm/mach-imx/mach-imx6q.c +6 -0
+
+		macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
+		if (i)
+			macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
+		else
+			macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));
You go over these register reads two times and read the very same registers two times. Argh! Could please look at your code before posting it?

Quoted 2 lines

For some other platform like imx6sx has two enet MACs, so "i" is 0, 1. You means OCOTP_MACn(1) is read twice, I will change it. Thanks.

What you do is:

	for (i = 0; i < IMX_MAX_ENET_NUM; i++) {
		...
		macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
		if (i)
			macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
		else
			macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));
		...
	}

What you should do is:

	macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
	macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
	macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));

	for (i = 0; i < IMX_MAX_ENET_NUM; i++) {
		...
	}

Quoted 36 lines

Quoted 25 lines

arch/arm/mach-imx/mach-imx6q.c +24 -0

+
+		newmac = kzalloc(sizeof(*newmac) + 6, GFP_KERNEL);
+		if (!newmac)
+			goto put_ocotp_node;
+
+		newmac->value = newmac + 1;
+		newmac->length = 6;
+		newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
+		if (!newmac->name) {
+			kfree(newmac);
+			goto put_ocotp_node;
+		}
+
+		macaddr = newmac->value;
+		if (i) {
+			macaddr[0] = (macaddr1_high >> 24) & 0xff;
+			macaddr[1] = (macaddr1_high >> 16) & 0xff;
+			macaddr[2] = (macaddr1_high >> 8) & 0xff;
+			macaddr[3] = macaddr1_high & 0xff;
+			macaddr[4] = (macaddr0_low >> 24) & 0xff;
+			macaddr[5] = (macaddr0_low >> 16) & 0xff;
+		} else {
+			macaddr[0] = (macaddr0_low >> 8) & 0xff;
+			macaddr[1] = macaddr0_low & 0xff;

Quoted 9 lines

arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 236
Quoted 5 lines
arch/arm/mach-imx/mach-imx6q.c +5 -0
+			macaddr[2] = (macaddr0_high >> 24) & 0xff;
+			macaddr[3] = (macaddr0_high >> 16) & 0xff;
+			macaddr[4] = (macaddr0_high >> 8) & 0xff;
+			macaddr[5] = macaddr0_high & 0xff;
+		}
It makes no sense to have a IMX_MAX_ENET_NUM define when the code assumes that the only valid value is 2.

You mean remove the "IMX_MAX_ENET_NUM" ?

Quoted 7 lines

Quoted 4 lines
All this is probably far more readable and less fishy when you move the fixup of a single enet device to a extra function which you call from the loop.
Because imx6sx has two enet MACs, and the function is called for imx6qdl/imx6sl/imx6sx, so add the loop in here. For imx6qdl/imx6sl, there just have one MAC, the second loop don't go through the flow.
Do you have other suggestion ?

What I want to say is: When you have a loop and the loop body gets to complicated, then it often improves readability to move the loop body to an extra function, like this:

static int imx6_enet_mac_init_one(struct device_node *enet, const u8 *mac)

{
	struct property *newmac;
	u8 mac[6];

	if (of_get_mac_address(enet))
		return 0;

	newmac = kzalloc(sizeof(*newmac) + 6, GFP_KERNEL);
	if (!newmac)
		return -ENOMEM;

	newmac->length = 6;
	newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
	newmac->value = kmemdup(mac, 6, GFP_KERNEL);

	of_update_property(enet_np, newmac);

	release_stuff();
}

int imx6_enet_mac_init(const char *compatible)

{
	struct device_node *from = NULL;
	u8 mac[6];

	macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
	macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
	macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));

	mac[0] = ...;

	enet_np = of_find_compatible_node(NULL, NULL, compatible);
	if (!enet_np)
		return 0;

	imx6_enet_mac_init_one(enet_np, mac);

	enet_np = of_find_compatible_node(enet_np, NULL, compatible);
	if (!enet_np)
		return 0;

	mac[0] = ...;

	imx6_enet_mac_init_one(enet_no, mac);
}

(here I removed the loop which makes the code even more straight forward)

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

fugang.duan at freescale.com

2014-05-20 10:23 UTC #4 lore raw

PATCH 1/1 ARM: imx6: init enet MAC address

From: Sascha Hauer <[email protected]>
Data: Tuesday, May 20, 2014 5:57 PM

Quoted 208 lines

To: Duan Fugang-B38611
Cc: Li Frank-B20596; Guo Shawn-R65073; linux-arm- kernel at lists.infradead.org; kernel at pengutronix.de
Subject: Re: [PATCH] ARM: imx6: init enet MAC address
Quoted 21 lines · fugang.duan at freescale.com
From: Sascha Hauer <[email protected]>
Data: Tuesday, May 20, 2014 5:03 PM
Quoted 18 lines
To: Duan Fugang-B38611
Cc: Li Frank-B20596; Guo Shawn-R65073; linux-arm- kernel at lists.infradead.org; kernel at pengutronix.de
Subject: Re: [PATCH] ARM: imx6: init enet MAC address
Quoted 12 lines · Fugang Duan
Enet get MAC address order:
From module parameters or kernel command line -> device tree -> pfuse
-> mac registers set by bootloader -> random mac address.
When there have no "fec.macaddr" parameters set in kernel command line, enet driver get MAC address from device tree. And then if the MAC address set in device tree and is valid, enet driver get MAC address from device tree. Otherwise, enet get MAC address from pfuse. So, in the condition, update the MAC address (read from pfuse) to device tree.
Quoted 7 lines
Quoted 6 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 170
Quoted 3 lines
arch/arm/mach-imx/mach-imx6q.c +3 -0
+#define OCOTP_MACn(n)		(0x00000620 + (n) * 0x10)
+#define IMX_MAX_ENET_NUM	2
+void __init imx6_enet_mac_init(const char *compatible)
static
The .fun() will be called for imx6sl, imx6sx(the latest chip) machine
files.
Quoted 13 lines
Quoted 13 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev)
Quoted 13 lines
arch/arm/mach-imx/mach-imx6q.c +12 -0
+{
+	struct device_node *ocotp_np, *enet_np, *from = NULL;
+	void __iomem *base;
+	struct property *newmac;
+	u32 macaddr0_low;
+	u32 macaddr0_high = 0;
+	u32 macaddr1_high = 0;
+	u8 *macaddr;
+	int i;
+
+	for (i = 0; i < IMX_MAX_ENET_NUM; i++) {
+		enet_np = of_find_compatible_node(from, NULL,
compatible);
Quoted 9 lines
Quoted 9 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev)
Quoted 9 lines
arch/arm/mach-imx/mach-imx6q.c +9 -0
+		if (!enet_np)
+			return;
+
+		from = enet_np;
+
+		if (of_get_mac_address(enet_np))
+			goto put_enet_node;
+
+		ocotp_np = of_find_compatible_node(NULL, NULL,
"fsl,imx6q-
Quoted 14 lines
Quoted 14 lines
ocotp");
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 201
Quoted 10 lines
arch/arm/mach-imx/mach-imx6q.c +10 -0
+		if (!ocotp_np) {
+			pr_warn("failed to find ocotp node\n");
+			goto put_enet_node;
+		}
+
+		base = of_iomap(ocotp_np, 0);
+		if (!base) {
+			pr_warn("failed to map ocotp\n");
+			goto put_ocotp_node;
+		}
Move this outside the loop.
Quoted 6 lines
Agree, I will move the ocotp node from the loop.
Quoted 4 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev)
Quoted 4 lines
arch/arm/mach-imx/mach-imx6q.c +4 -0
+
+		macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
+		if (i)
+			macaddr1_high = readl_relaxed(base +
OCOTP_MACn(2));
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		else
+			macaddr0_high = readl_relaxed(base +
OCOTP_MACn(0));
Quoted 7 lines
Quoted 5 lines
You go over these register reads two times and read the very same registers two times. Argh! Could please look at your code before posting it?
For some other platform like imx6sx has two enet MACs, so "i" is 0, 1. You means OCOTP_MACn(1) is read twice, I will change it. Thanks.
What you do is:
	for (i = 0; i < IMX_MAX_ENET_NUM; i++) {
		...
		macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
		if (i)
			macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
		else
			macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));
		...
	}
What you should do is:
	macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
	macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
	macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));

	for (i = 0; i < IMX_MAX_ENET_NUM; i++) {
		...
	}
Quoted 34 lines
Quoted 25 lines
Quoted 25 lines
arch/arm/mach-imx/mach-imx6q.c +24 -0
+
+		newmac = kzalloc(sizeof(*newmac) + 6, GFP_KERNEL);
+		if (!newmac)
+			goto put_ocotp_node;
+
+		newmac->value = newmac + 1;
+		newmac->length = 6;
+		newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
+		if (!newmac->name) {
+			kfree(newmac);
+			goto put_ocotp_node;
+		}
+
+		macaddr = newmac->value;
+		if (i) {
+			macaddr[0] = (macaddr1_high >> 24) & 0xff;
+			macaddr[1] = (macaddr1_high >> 16) & 0xff;
+			macaddr[2] = (macaddr1_high >> 8) & 0xff;
+			macaddr[3] = macaddr1_high & 0xff;
+			macaddr[4] = (macaddr0_low >> 24) & 0xff;
+			macaddr[5] = (macaddr0_low >> 16) & 0xff;
+		} else {
+			macaddr[0] = (macaddr0_low >> 8) & 0xff;
+			macaddr[1] = macaddr0_low & 0xff;
Quoted 9 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 236
Quoted 5 lines
arch/arm/mach-imx/mach-imx6q.c +5 -0
+			macaddr[2] = (macaddr0_high >> 24) & 0xff;
+			macaddr[3] = (macaddr0_high >> 16) & 0xff;
+			macaddr[4] = (macaddr0_high >> 8) & 0xff;
+			macaddr[5] = macaddr0_high & 0xff;
+		}
It makes no sense to have a IMX_MAX_ENET_NUM define when the code assumes that the only valid value is 2.
Quoted 7 lines
You mean remove the "IMX_MAX_ENET_NUM" ?
Quoted 4 lines
All this is probably far more readable and less fishy when you move the fixup of a single enet device to a extra function which you call from the loop.
Because imx6sx has two enet MACs, and the function is called for
imx6qdl/imx6sl/imx6sx, so add the loop in here.
Quoted 1 line
For imx6qdl/imx6sl, there just have one MAC, the second loop don't go
through the flow.
Quoted 1 line
Do you have other suggestion ?
What I want to say is: When you have a loop and the loop body gets to complicated, then it often improves readability to move the loop body to an extra function, like this:
static int imx6_enet_mac_init_one(struct device_node *enet, const u8 *mac)
{
	struct property *newmac;
	u8 mac[6];

	if (of_get_mac_address(enet))
		return 0;

	newmac = kzalloc(sizeof(*newmac) + 6, GFP_KERNEL);
	if (!newmac)
		return -ENOMEM;

	newmac->length = 6;
	newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
	newmac->value = kmemdup(mac, 6, GFP_KERNEL);

	of_update_property(enet_np, newmac);

	release_stuff();
}
int imx6_enet_mac_init(const char *compatible) {
	struct device_node *from = NULL;
	u8 mac[6];

	macaddr0_low = readl_relaxed(base + OCOTP_MACn(1));
	macaddr1_high = readl_relaxed(base + OCOTP_MACn(2));
	macaddr0_high = readl_relaxed(base + OCOTP_MACn(0));

	mac[0] = ...;

	enet_np = of_find_compatible_node(NULL, NULL, compatible);
	if (!enet_np)
		return 0;

	imx6_enet_mac_init_one(enet_np, mac);

	enet_np = of_find_compatible_node(enet_np, NULL, compatible);
	if (!enet_np)
		return 0;

	mac[0] = ...;

	imx6_enet_mac_init_one(enet_no, mac);
}

Quoted 5 lines

(here I removed the loop which makes the code even more straight forward)
Sascha

Sascha, thanks for your suggestion ! It is more readable. I will send out the patch V2.

Shawn Guo

2014-05-21 06:46 UTC #5 lore raw

PATCH 1/1 ARM: imx6: init enet MAC address

In reply to fugang.duan at freescale.com

Quoted 7 lines · On Tue, May 20, 2014 at 05:24:01PM +0800, Duan Fugang-B38611 wrote:

Quoted 6 lines
arch/arm/mach-imx/mach-imx6q.c static int ar8035_phy_fixup(struct phy_device *dev) · line 170
Quoted 3 lines
arch/arm/mach-imx/mach-imx6q.c +3 -0
+#define OCOTP_MACn(n)		(0x00000620 + (n) * 0x10)
+#define IMX_MAX_ENET_NUM	2
+void __init imx6_enet_mac_init(const char *compatible)
static
The .fun() will be called for imx6sl, imx6sx(the latest chip) machine files.

Then, it shouldn't be put in mach-imx6q.c which will be built only when CONFIG_SOC_IMX6Q is enabled.

Shawn

Shawn Guo

2014-05-22 06:12 UTC #6 lore raw

PATCH 1/1 ARM: imx6: init enet MAC address

In reply to Fugang Duan

Quoted 4 lines · Fugang Duan

Enet get MAC address order:
From module parameters or kernel command line -> device tree -> pfuse -> mac registers set by bootloader -> random mac address.

Quoted 8 lines

When there have no "fec.macaddr" parameters set in kernel command line, enet driver get MAC address from device tree. And then if the MAC address set in device tree and is valid, enet driver get MAC address from device tree. Otherwise, enet get MAC address from pfuse. So, in the condition, update the MAC address (read from pfuse) to device tree.
Signed-off-by Fugang Duan <[email protected]>

I would think this is a board level configuration than SoC one. Though the register name in Reference Manual, OCOTP_MAC0 and OCOTP_MAC1, recommends that these fuse bits can be used to store MAC address, it's really up to board designers how to use these fuse bits, e.g. how the MAC bytes will be stored in these 64 fuse bits, or on some board designs which do not have Ethernet interface at all, these bits can totally be used for other purpose.

So to me, it's inappropriate to assume that these fuse bits always hold MAC address, and do this setup in kernel unconditionally for every board. I think it's more reasonable to do this setup in bootloader, which should be more board specific and can understand these fuse bits better.

Shawn

fugang.duan at freescale.com

2014-05-22 07:34 UTC #7 lore raw

PATCH 1/1 ARM: imx6: init enet MAC address

From: Shawn Guo <[email protected]>
Data: Thursday, May 22, 2014 2:13 PM

Quoted 27 lines

To: Duan Fugang-B38611
Cc: Li Frank-B20596; linux-arm-kernel at lists.infradead.org; kernel at pengutronix.de
Subject: Re: [PATCH] ARM: imx6: init enet MAC address
Quoted 4 lines · Fugang Duan
Enet get MAC address order:
From module parameters or kernel command line -> device tree -> pfuse -> mac registers set by bootloader -> random mac address.
Quoted 8 lines
When there have no "fec.macaddr" parameters set in kernel command line, enet driver get MAC address from device tree. And then if the MAC address set in device tree and is valid, enet driver get MAC address from device tree. Otherwise, enet get MAC address from pfuse. So, in the condition, update the MAC address (read from pfuse) to device tree.
Signed-off-by Fugang Duan <[email protected]>
I would think this is a board level configuration than SoC one. Though the register name in Reference Manual, OCOTP_MAC0 and OCOTP_MAC1, recommends that these fuse bits can be used to store MAC address, it's really up to board designers how to use these fuse bits, e.g. how the MAC bytes will be stored in these 64 fuse bits, or on some board designs which do not have Ethernet interface at all, these bits can totally be used for other purpose.

Quoted 6 lines

So to me, it's inappropriate to assume that these fuse bits always hold MAC address, and do this setup in kernel unconditionally for every board. I think it's more reasonable to do this setup in bootloader, which should be more board specific and can understand these fuse bits better.
Shawn

Shawn, if so, we don't need to upstream the patch in kernel. Thanks for your information.

Andy