| Message ID | 1400566266-22746-1-git-send-email-b38611@freescale.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 20, 2014 at 02:11:06PM +0800, Fugang Duan wrote: > 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. > > +#define OCOTP_MACn(n) (0x00000620 + (n) * 0x10) > +#define IMX_MAX_ENET_NUM 2 > +void __init imx6_enet_mac_init(const char *compatible) static > +{ > + 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"); > + 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. > + > + 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? > + > + 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; > + 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
From: Sascha Hauer <s.hauer@pengutronix.de> Data: Tuesday, May 20, 2014 5:03 PM >To: Duan Fugang-B38611 >Cc: Li Frank-B20596; Guo Shawn-R65073; linux-arm- >kernel@lists.infradead.org; kernel@pengutronix.de >Subject: Re: [PATCH] ARM: imx6: init enet MAC address > >On Tue, May 20, 2014 at 02:11:06PM +0800, Fugang Duan wrote: >> 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. >> >> +#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. >> +{ >> + 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"); >> + 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. >> + >> + 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. >> + >> + 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; >> + 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" ? >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!
On Tue, May 20, 2014 at 09:24:01AM +0000, fugang.duan@freescale.com wrote: > From: Sascha Hauer <s.hauer@pengutronix.de> > Data: Tuesday, May 20, 2014 5:03 PM > > >To: Duan Fugang-B38611 > >Cc: Li Frank-B20596; Guo Shawn-R65073; linux-arm- > >kernel@lists.infradead.org; kernel@pengutronix.de > >Subject: Re: [PATCH] ARM: imx6: init enet MAC address > > > >On Tue, May 20, 2014 at 02:11:06PM +0800, Fugang Duan wrote: > >> 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. > >> > >> +#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. > > >> +{ > >> + 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"); > >> + 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. > > >> + > >> + 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. 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++) { ... } > > >> + > >> + 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; > >> + 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" ? > > >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
From: Sascha Hauer <s.hauer@pengutronix.de> Data: Tuesday, May 20, 2014 5:57 PM >To: Duan Fugang-B38611 >Cc: Li Frank-B20596; Guo Shawn-R65073; linux-arm- >kernel@lists.infradead.org; kernel@pengutronix.de >Subject: Re: [PATCH] ARM: imx6: init enet MAC address > >On Tue, May 20, 2014 at 09:24:01AM +0000, fugang.duan@freescale.com wrote: >> From: Sascha Hauer <s.hauer@pengutronix.de> >> Data: Tuesday, May 20, 2014 5:03 PM >> >> >To: Duan Fugang-B38611 >> >Cc: Li Frank-B20596; Guo Shawn-R65073; linux-arm- >> >kernel@lists.infradead.org; kernel@pengutronix.de >> >Subject: Re: [PATCH] ARM: imx6: init enet MAC address >> > >> >On Tue, May 20, 2014 at 02:11:06PM +0800, Fugang Duan wrote: >> >> 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. >> >> >> >> +#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. >> >> >> +{ >> >> + 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"); >> >> + 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. >> >> >> + >> >> + 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. > >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++) { > ... > } >> >> >> + >> >> + 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; >> >> + 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" ? >> >> >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 > Sascha, thanks for your suggestion ! It is more readable. I will send out the patch V2.
On Tue, May 20, 2014 at 05:24:01PM +0800, Duan Fugang-B38611 wrote: > >> +#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
On Tue, May 20, 2014 at 02:11:06PM +0800, Fugang Duan wrote: > 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. > > Signed-off-by: Fugang Duan <B38611@freescale.com> 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
From: Shawn Guo <shawn.guo@freescale.com> Data: Thursday, May 22, 2014 2:13 PM >To: Duan Fugang-B38611 >Cc: Li Frank-B20596; linux-arm-kernel@lists.infradead.org; >kernel@pengutronix.de >Subject: Re: [PATCH] ARM: imx6: init enet MAC address > >On Tue, May 20, 2014 at 02:11:06PM +0800, Fugang Duan wrote: >> 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. >> >> Signed-off-by: Fugang Duan <B38611@freescale.com> > >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 Shawn, if so, we don't need to upstream the patch in kernel. Thanks for your information. Andy
diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h index 4facd01..5c69325 100644 --- a/arch/arm/mach-imx/common.h +++ b/arch/arm/mach-imx/common.h @@ -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); diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index e60456d..2bec13d 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -22,6 +22,7 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <linux/of_net.h> #include <linux/of_platform.h> #include <linux/pm_opp.h> #include <linux/pci.h> @@ -164,6 +165,85 @@ static int ar8035_phy_fixup(struct phy_device *dev) return 0; } +#define OCOTP_MACn(n) (0x00000620 + (n) * 0x10) +#define IMX_MAX_ENET_NUM 2 +void __init imx6_enet_mac_init(const char *compatible) +{ + 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"); + 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; + } + + 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)); + + 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; + macaddr[2] = (macaddr0_high >> 24) & 0xff; + macaddr[3] = (macaddr0_high >> 16) & 0xff; + macaddr[4] = (macaddr0_high >> 8) & 0xff; + macaddr[5] = macaddr0_high & 0xff; + } + + of_update_property(enet_np, newmac); + +put_ocotp_node: + of_node_put(ocotp_np); +put_enet_node: + of_node_put(enet_np); + } +} + #define PHY_ID_AR8035 0x004dd072 static void __init imx6q_enet_phy_init(void) @@ -228,6 +308,13 @@ put_node: of_node_put(np); } +static inline void imx6q_enet_init(void) +{ + imx6_enet_mac_init("fsl,imx6q-fec"); + imx6q_enet_phy_init(); + imx6q_1588_init(); +} + static void __init imx6q_axi_init(void) { struct regmap *gpr; @@ -274,13 +361,12 @@ static void __init imx6q_init_machine(void) if (parent == NULL) pr_warn("failed to initialize soc device\n"); - imx6q_enet_phy_init(); of_platform_populate(NULL, of_default_bus_match_table, NULL, parent); + imx6q_enet_init(); imx_anatop_init(); cpu_is_imx6q() ? imx6q_pm_init() : imx6dl_pm_init(); - imx6q_1588_init(); imx6q_axi_init(); } diff --git a/arch/arm/mach-imx/mach-imx6sl.c b/arch/arm/mach-imx/mach-imx6sl.c index ad32338..cb001d4 100644 --- a/arch/arm/mach-imx/mach-imx6sl.c +++ b/arch/arm/mach-imx/mach-imx6sl.c @@ -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 */