[PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Peter Korsgaard]

When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
mode in U-Boot), nothing updates the ethernet hwaddr specified for the
CPSW slaves, causing the driver to use a random hwaddr, which is some times
troublesome.

The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
more sense to default to these rather than random ones. Add a fixup step
which adds mac-address dt properties using the efuse addresses if the DTB
didn't contain valid ones.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
Changes since v1:
- Use omap_arch_initcall as pointed out by Tony

 arch/arm/mach-omap2/Makefile      |    3 ++
 arch/arm/mach-omap2/am33xx-cpsw.c |   94 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/control.h     |    4 ++
 3 files changed, 101 insertions(+)
 create mode 100644 arch/arm/mach-omap2/am33xx-cpsw.c

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 65fb6fb..54fb2ee 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -298,4 +298,7 @@ endif
 emac-$(CONFIG_TI_DAVINCI_EMAC)		:= am35xx-emac.o
 obj-y					+= $(emac-m) $(emac-y)
 
+cpsw-$(CONFIG_TI_CPSW)			:= am33xx-cpsw.o
+obj-y					+= $(cpsw-m) $(cpsw-y)
+
 obj-y					+= common-board-devices.o twl-common.o dss-common.o
diff --git a/arch/arm/mach-omap2/am33xx-cpsw.c b/arch/arm/mach-omap2/am33xx-cpsw.c
new file mode 100644
index 0000000..eec29a4
--- /dev/null
+++ b/arch/arm/mach-omap2/am33xx-cpsw.c
@@ -0,0 +1,94 @@
+/*
+ * am335x specific cpsw dt fixups
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/etherdevice.h>
+#include <linux/of.h>
+#include <linux/of_net.h>
+
+#include "soc.h"
+#include "control.h"
+
+/**
+ * am33xx_dt_cpsw_set_mac_from_efuse - Add mac-address property using
+ * ethernet hwaddr from efuse
+ * @np:		Pointer to the cpsw slave to set mac address of
+ * @idx:	Mac address index to use from efuse
+ */
+static void am33xx_dt_cpsw_set_mac_from_efuse(struct device_node *np, int idx)
+{
+	struct property *prop;
+	u32 lo, hi;
+	u8 *mac;
+
+	switch (idx) {
+	case 0:
+		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_LOW);
+		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_HIGH);
+		break;
+
+	case 1:
+		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_LOW);
+		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_HIGH);
+		break;
+
+	default:
+		pr_err("cpsw.%d: too many slaves found\n", idx);
+		return;
+	}
+
+	prop = kzalloc(sizeof(*prop) + ETH_ALEN, GFP_KERNEL);
+	if (!prop)
+		return;
+
+	prop->value = prop + 1;
+	prop->length = ETH_ALEN;
+	prop->name = kstrdup("mac-address", GFP_KERNEL);
+	if (!prop->name) {
+		kfree(prop);
+		return;
+	}
+
+	mac = prop->value;
+
+	mac[0] = hi;
+	mac[1] = hi >> 8;
+	mac[2] = hi >> 16;
+	mac[3] = hi >> 24;
+	mac[4] = lo;
+	mac[5] = lo >> 8;
+
+	of_update_property(np, prop);
+
+	pr_info("cpsw.%d: No hwaddr in dt. Using %pM from efuse\n", idx, mac);
+}
+
+static int __init am33xx_dt_cpsw_mac_fixup(void)
+{
+	struct device_node *np, *slave;
+	int idx = 0;
+
+	if (!soc_is_am33xx())
+		return -ENODEV;
+
+	for_each_compatible_node(np, NULL, "ti,cpsw")
+		for_each_node_by_name(slave, "slave") {
+			if (!of_get_mac_address(slave))
+				am33xx_dt_cpsw_set_mac_from_efuse(slave, idx);
+			idx++;
+		}
+
+	return 0;
+}
+omap_arch_initcall(am33xx_dt_cpsw_mac_fixup);
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index e6c3281..266d512 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -352,6 +352,10 @@
 /* AM33XX CONTROL_STATUS register */
 #define AM33XX_CONTROL_STATUS		0x040
 #define AM33XX_CONTROL_SEC_CLK_CTRL	0x1bc
+#define AM33XX_CONTROL_MAC_ID0_LOW	0x630
+#define AM33XX_CONTROL_MAC_ID0_HIGH	0x634
+#define AM33XX_CONTROL_MAC_ID1_LOW	0x638
+#define AM33XX_CONTROL_MAC_ID1_HIGH	0x63c
 
 /* AM33XX CONTROL_STATUS bitfields (partial) */
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_SHIFT		22
-- 
1.7.10.4

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Mugunthan V N]

On 1/18/2013 3:48 AM, Peter Korsgaard wrote:
> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
> CPSW slaves, causing the driver to use a random hwaddr, which is some times
> troublesome.
>
> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
> more sense to default to these rather than random ones. Add a fixup step
> which adds mac-address dt properties using the efuse addresses if the DTB
> didn't contain valid ones.
>
> Signed-off-by: Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>
>

This implementation looks fine.
Acked-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>

Regards
Mugunthan V N

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Michal Bachraty]

[-- Attachment #1.1: Type: text/plain, Size: 1641 bytes --]

On Fri, Jan 18, 2013 at 6:14 AM, Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org> wrote:

> On 1/18/2013 3:48 AM, Peter Korsgaard wrote:
>
>> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
>> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
>> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
>> CPSW slaves, causing the driver to use a random hwaddr, which is some
>> times
>> troublesome.
>>
>> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it
>> makes
>> more sense to default to these rather than random ones. Add a fixup step
>> which adds mac-address dt properties using the efuse addresses if the DTB
>> didn't contain valid ones.
>>
>> Signed-off-by: Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>
>>
>>
> This implementation looks fine.
> Acked-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
>
> Regards
> Mugunthan V N
>

Thanks,
My approach for cpsw driver is posted here:
https://patchwork.kernel.org/patch/1966481/
I added DT option mac-address-source because there is still possiblility to
have mac address set by the u-boot in the future. Beaglebone have special
eeprom on board, were you can have stored user MAC and u-boot can read
eeprom
data. Maybe some users will want this feature.  Also in cpsw dt binding
documentation, there is mention  /* Filled in by U-Boot */.  For that
purpose,
code I proposed can easily deal with any driver exteds.

Reading CPU mac address registers with (my approach) or without (your
approach) DT should be more discussed. Also your way seems to be fine.

[-- Attachment #1.2: Type: text/html, Size: 2435 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Mark Jackson]

On 18/01/13 05:14, Mugunthan V N wrote:
> On 1/18/2013 3:48 AM, Peter Korsgaard wrote:
>> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
>> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
>> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
>> CPSW slaves, causing the driver to use a random hwaddr, which is some times
>> troublesome.
>>
>> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
>> more sense to default to these rather than random ones. Add a fixup step
>> which adds mac-address dt properties using the efuse addresses if the DTB
>> didn't contain valid ones.
>>
>> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
>>
> 
> This implementation looks fine.
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Mark Jackson]

On 08/07/13 13:42, Mark Jackson wrote:
> On 18/01/13 05:14, Mugunthan V N wrote:
>> On 1/18/2013 3:48 AM, Peter Korsgaard wrote:
>>> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
>>> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
>>> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
>>> CPSW slaves, causing the driver to use a random hwaddr, which is some times
>>> troublesome.
>>>
>>> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
>>> more sense to default to these rather than random ones. Add a fixup step
>>> which adds mac-address dt properties using the efuse addresses if the DTB
>>> didn't contain valid ones.
>>>
>>> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
>>>
>>
>> This implementation looks fine.
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> 
> Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>

Is this ever going to be put into the mainline code ?

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Peter Korsgaard]

>>>>> "Mark" == Mark Jackson <mpfj-list@newflow.co.uk> writes:

Hi,

 Mark> On 08/07/13 13:42, Mark Jackson wrote:
 >> On 18/01/13 05:14, Mugunthan V N wrote:
 >>> On 1/18/2013 3:48 AM, Peter Korsgaard wrote:
 >>>> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
 >>>> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
 >>>> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
 >>>> CPSW slaves, causing the driver to use a random hwaddr, which is some times
 >>>> troublesome.
 >>>> 
 >>>> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
 >>>> more sense to default to these rather than random ones. Add a fixup step
 >>>> which adds mac-address dt properties using the efuse addresses if the DTB
 >>>> didn't contain valid ones.
 >>>> 
 >>>> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
 >>> 
 >>> This implementation looks fine.
 >>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
 >> 
 >> Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>

 Mark> Is this ever going to be put into the mainline code ?

Good question. Tony, could you please pick this up? It has been pending
since January and has a number of acks.

Do you want me to resend?

-- 
Bye, Peter Korsgaard

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Koen Kooi]

Op 8 jul. 2013, om 14:42 heeft Mark Jackson <mpfj-list@newflow.co.uk> het volgende geschreven:

> On 18/01/13 05:14, Mugunthan V N wrote:
>> On 1/18/2013 3:48 AM, Peter Korsgaard wrote:
>>> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
>>> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
>>> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
>>> CPSW slaves, causing the driver to use a random hwaddr, which is some times
>>> troublesome.
>>> 
>>> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
>>> more sense to default to these rather than random ones. Add a fixup step
>>> which adds mac-address dt properties using the efuse addresses if the DTB
>>> didn't contain valid ones.
>>> 
>>> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
>>> 
>> 
>> This implementation looks fine.
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> 
> Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>

Tested-by: Koen Kooi <koen@dominion.thruhere.net>

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Matt Porter]

On Mon, Jul 15, 2013 at 07:31:18AM +0200, Peter Korsgaard wrote:
> >>>>> "Mark" == Mark Jackson <mpfj-list@newflow.co.uk> writes:
> 
> Hi,
> 
>  Mark> On 08/07/13 13:42, Mark Jackson wrote:
>  >> On 18/01/13 05:14, Mugunthan V N wrote:
>  >>> On 1/18/2013 3:48 AM, Peter Korsgaard wrote:
>  >>>> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
>  >>>> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
>  >>>> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
>  >>>> CPSW slaves, causing the driver to use a random hwaddr, which is some times
>  >>>> troublesome.
>  >>>> 
>  >>>> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
>  >>>> more sense to default to these rather than random ones. Add a fixup step
>  >>>> which adds mac-address dt properties using the efuse addresses if the DTB
>  >>>> didn't contain valid ones.
>  >>>> 
>  >>>> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
>  >>> 
>  >>> This implementation looks fine.
>  >>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>  >> 
>  >> Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>
> 
>  Mark> Is this ever going to be put into the mainline code ?
> 
> Good question. Tony, could you please pick this up? It has been pending
> since January and has a number of acks.
> 
> Do you want me to resend?

Also working nicely here on 3.11.

Tested-by: Matt Porter <matt.porter@linaro.org>

Kevin or Olof: can you apply? Seems to be continuing no response after
Ack back in January.

-Matt

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Olof Johansson]

On Thu, Sep 5, 2013 at 1:16 PM, Matt Porter <matt.porter@linaro.org> wrote:
> On Mon, Jul 15, 2013 at 07:31:18AM +0200, Peter Korsgaard wrote:
>> >>>>> "Mark" == Mark Jackson <mpfj-list@newflow.co.uk> writes:
>>
>> Hi,
>>
>>  Mark> On 08/07/13 13:42, Mark Jackson wrote:
>>  >> On 18/01/13 05:14, Mugunthan V N wrote:
>>  >>> On 1/18/2013 3:48 AM, Peter Korsgaard wrote:
>>  >>>> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
>>  >>>> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
>>  >>>> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
>>  >>>> CPSW slaves, causing the driver to use a random hwaddr, which is some times
>>  >>>> troublesome.
>>  >>>>
>>  >>>> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
>>  >>>> more sense to default to these rather than random ones. Add a fixup step
>>  >>>> which adds mac-address dt properties using the efuse addresses if the DTB
>>  >>>> didn't contain valid ones.
>>  >>>>
>>  >>>> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
>>  >>>
>>  >>> This implementation looks fine.
>>  >>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>>  >>
>>  >> Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>
>>
>>  Mark> Is this ever going to be put into the mainline code ?
>>
>> Good question. Tony, could you please pick this up? It has been pending
>> since January and has a number of acks.
>>
>> Do you want me to resend?
>
> Also working nicely here on 3.11.
>
> Tested-by: Matt Porter <matt.porter@linaro.org>
>
> Kevin or Olof: can you apply? Seems to be continuing no response after
> Ack back in January.

I have no idea what the patch is, it's no longer in the email. Can you
resend it, please?


-Olof

[PATCH RESEND] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Peter Korsgaard]

When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
mode in U-Boot), nothing updates the ethernet hwaddr specified for the
CPSW slaves, causing the driver to use a random hwaddr, which is some times
troublesome.

The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
more sense to default to these rather than random ones. Add a fixup step
which adds mac-address dt properties using the efuse addresses if the DTB
didn't contain valid ones.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>
Tested-by: Koen Kooi <koen@dominion.thruhere.net>
Tested-by: Matt Porter <matt.porter@linaro.org>
---
Resent without changes.

 arch/arm/mach-omap2/Makefile      |    3 ++
 arch/arm/mach-omap2/am33xx-cpsw.c |   94 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/control.h     |    4 ++
 3 files changed, 101 insertions(+)
 create mode 100644 arch/arm/mach-omap2/am33xx-cpsw.c

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index afb457c..0afee42 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -305,4 +305,7 @@ endif
 emac-$(CONFIG_TI_DAVINCI_EMAC)		:= am35xx-emac.o
 obj-y					+= $(emac-m) $(emac-y)
 
+cpsw-$(CONFIG_TI_CPSW)			:= am33xx-cpsw.o
+obj-y					+= $(cpsw-m) $(cpsw-y)
+
 obj-y					+= common-board-devices.o twl-common.o dss-common.o
diff --git a/arch/arm/mach-omap2/am33xx-cpsw.c b/arch/arm/mach-omap2/am33xx-cpsw.c
new file mode 100644
index 0000000..5a674d9
--- /dev/null
+++ b/arch/arm/mach-omap2/am33xx-cpsw.c
@@ -0,0 +1,94 @@
+/*
+ * am335x specific cpsw dt fixups
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/etherdevice.h>
+#include <linux/of.h>
+#include <linux/of_net.h>
+
+#include "soc.h"
+#include "control.h"
+
+/**
+ * am33xx_dt_cpsw_set_mac_from_efuse - Add mac-address property using
+ * ethernet hwaddr from efuse
+ * @np:		Pointer to the cpsw slave to set mac address of
+ * @idx:	Mac address index to use from efuse
+ */
+static void am33xx_dt_cpsw_set_mac_from_efuse(struct device_node *np, int idx)
+{
+	struct property *prop;
+	u32 lo, hi;
+	u8 *mac;
+
+	switch (idx) {
+	case 0:
+		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_LOW);
+		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_HIGH);
+		break;
+
+	case 1:
+		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_LOW);
+		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_HIGH);
+		break;
+
+	default:
+		pr_err("cpsw.%d: too many slaves found\n", idx);
+		return;
+	}
+
+	prop = kzalloc(sizeof(*prop) + ETH_ALEN, GFP_KERNEL);
+	if (!prop)
+		return;
+
+	prop->value = prop + 1;
+	prop->length = ETH_ALEN;
+	prop->name = kstrdup("mac-address", GFP_KERNEL);
+	if (!prop->name) {
+		kfree(prop);
+		return;
+	}
+
+	mac = prop->value;
+
+	mac[0] = hi;
+	mac[1] = hi >> 8;
+	mac[2] = hi >> 16;
+	mac[3] = hi >> 24;
+	mac[4] = lo;
+	mac[5] = lo >> 8;
+
+	of_update_property(np, prop);
+
+	pr_info("cpsw.%d: No hwaddr in dt. Using %pM from efuse\n", idx, mac);
+}
+
+static int __init am33xx_dt_cpsw_mac_fixup(void)
+{
+	struct device_node *np, *slave;
+	int idx = 0;
+
+	if (!soc_is_am33xx())
+		return -ENODEV;
+
+	for_each_compatible_node(np, NULL, "ti,cpsw")
+		for_each_node_by_name(slave, "slave") {
+			if (!of_get_mac_address(slave))
+				am33xx_dt_cpsw_set_mac_from_efuse(slave, idx);
+			idx++;
+		}
+
+	return 0;
+}
+arch_initcall(am33xx_dt_cpsw_mac_fixup);
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index f7d7c2e..5d684b7 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -352,6 +352,10 @@
 /* AM33XX CONTROL_STATUS register */
 #define AM33XX_CONTROL_STATUS		0x040
 #define AM33XX_CONTROL_SEC_CLK_CTRL	0x1bc
+#define AM33XX_CONTROL_MAC_ID0_LOW	0x630
+#define AM33XX_CONTROL_MAC_ID0_HIGH	0x634
+#define AM33XX_CONTROL_MAC_ID1_LOW	0x638
+#define AM33XX_CONTROL_MAC_ID1_HIGH	0x63c
 
 /* AM33XX CONTROL_STATUS bitfields (partial) */
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_SHIFT		22
-- 
1.7.10.4

Re: [PATCHv2] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Peter Korsgaard]

>>>>> "Olof" == Olof Johansson <olof@lixom.net> writes:

 >> Tested-by: Matt Porter <matt.porter@linaro.org>
 >> 
 >> Kevin or Olof: can you apply? Seems to be continuing no response after
 >> Ack back in January.

 Olof> I have no idea what the patch is, it's no longer in the email. Can you
 Olof> resend it, please?

Sure, one moment.

-- 
Bye, Peter Korsgaard

Re: [PATCH RESEND] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <jacmet@sunsite.dk> writes:

 Peter> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
 Peter> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
 Peter> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
 Peter> CPSW slaves, causing the driver to use a random hwaddr, which is some times
 Peter> troublesome.

 Peter> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
 Peter> more sense to default to these rather than random ones. Add a fixup step
 Peter> which adds mac-address dt properties using the efuse addresses if the DTB
 Peter> didn't contain valid ones.

 Peter> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
 Peter> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
 Peter> Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>
 Peter> Tested-by: Koen Kooi <koen@dominion.thruhere.net>
 Peter> Tested-by: Matt Porter <matt.porter@linaro.org>
 Peter> ---
 Peter> Resent without changes.

Argh, this was supposed to have been a resend of v2. Will resend that
one instead.

-- 
Bye, Peter Korsgaard

[PATCHv2 RESEND] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Peter Korsgaard]

When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
mode in U-Boot), nothing updates the ethernet hwaddr specified for the
CPSW slaves, causing the driver to use a random hwaddr, which is some times
troublesome.

The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
more sense to default to these rather than random ones. Add a fixup step
which adds mac-address dt properties using the efuse addresses if the DTB
didn't contain valid ones.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>
Tested-by: Koen Kooi <koen@dominion.thruhere.net>
Tested-by: Matt Porter <matt.porter@linaro.org>
---
Changes since v1:
- Use omap_arch_initcall as pointed out by Tony

 arch/arm/mach-omap2/Makefile      |    3 ++
 arch/arm/mach-omap2/am33xx-cpsw.c |   94 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/control.h     |    4 ++
 3 files changed, 101 insertions(+)
 create mode 100644 arch/arm/mach-omap2/am33xx-cpsw.c

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index afb457c..0afee42 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -305,4 +305,7 @@ endif
 emac-$(CONFIG_TI_DAVINCI_EMAC)		:= am35xx-emac.o
 obj-y					+= $(emac-m) $(emac-y)
 
+cpsw-$(CONFIG_TI_CPSW)			:= am33xx-cpsw.o
+obj-y					+= $(cpsw-m) $(cpsw-y)
+
 obj-y					+= common-board-devices.o twl-common.o dss-common.o
diff --git a/arch/arm/mach-omap2/am33xx-cpsw.c b/arch/arm/mach-omap2/am33xx-cpsw.c
new file mode 100644
index 0000000..eec29a4
--- /dev/null
+++ b/arch/arm/mach-omap2/am33xx-cpsw.c
@@ -0,0 +1,94 @@
+/*
+ * am335x specific cpsw dt fixups
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/etherdevice.h>
+#include <linux/of.h>
+#include <linux/of_net.h>
+
+#include "soc.h"
+#include "control.h"
+
+/**
+ * am33xx_dt_cpsw_set_mac_from_efuse - Add mac-address property using
+ * ethernet hwaddr from efuse
+ * @np:		Pointer to the cpsw slave to set mac address of
+ * @idx:	Mac address index to use from efuse
+ */
+static void am33xx_dt_cpsw_set_mac_from_efuse(struct device_node *np, int idx)
+{
+	struct property *prop;
+	u32 lo, hi;
+	u8 *mac;
+
+	switch (idx) {
+	case 0:
+		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_LOW);
+		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_HIGH);
+		break;
+
+	case 1:
+		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_LOW);
+		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_HIGH);
+		break;
+
+	default:
+		pr_err("cpsw.%d: too many slaves found\n", idx);
+		return;
+	}
+
+	prop = kzalloc(sizeof(*prop) + ETH_ALEN, GFP_KERNEL);
+	if (!prop)
+		return;
+
+	prop->value = prop + 1;
+	prop->length = ETH_ALEN;
+	prop->name = kstrdup("mac-address", GFP_KERNEL);
+	if (!prop->name) {
+		kfree(prop);
+		return;
+	}
+
+	mac = prop->value;
+
+	mac[0] = hi;
+	mac[1] = hi >> 8;
+	mac[2] = hi >> 16;
+	mac[3] = hi >> 24;
+	mac[4] = lo;
+	mac[5] = lo >> 8;
+
+	of_update_property(np, prop);
+
+	pr_info("cpsw.%d: No hwaddr in dt. Using %pM from efuse\n", idx, mac);
+}
+
+static int __init am33xx_dt_cpsw_mac_fixup(void)
+{
+	struct device_node *np, *slave;
+	int idx = 0;
+
+	if (!soc_is_am33xx())
+		return -ENODEV;
+
+	for_each_compatible_node(np, NULL, "ti,cpsw")
+		for_each_node_by_name(slave, "slave") {
+			if (!of_get_mac_address(slave))
+				am33xx_dt_cpsw_set_mac_from_efuse(slave, idx);
+			idx++;
+		}
+
+	return 0;
+}
+omap_arch_initcall(am33xx_dt_cpsw_mac_fixup);
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index f7d7c2e..5d684b7 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -352,6 +352,10 @@
 /* AM33XX CONTROL_STATUS register */
 #define AM33XX_CONTROL_STATUS		0x040
 #define AM33XX_CONTROL_SEC_CLK_CTRL	0x1bc
+#define AM33XX_CONTROL_MAC_ID0_LOW	0x630
+#define AM33XX_CONTROL_MAC_ID0_HIGH	0x634
+#define AM33XX_CONTROL_MAC_ID1_LOW	0x638
+#define AM33XX_CONTROL_MAC_ID1_HIGH	0x63c
 
 /* AM33XX CONTROL_STATUS bitfields (partial) */
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_SHIFT		22
-- 
1.7.10.4

Re: [PATCHv2 RESEND] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Olof Johansson]

Hi,

On Thu, Sep 05, 2013 at 11:42:02PM +0200, Peter Korsgaard wrote:
> When booting with CONFIG_ARM_APPENDED_DTB (either because of using an old
> U-Boot, not wanting the hassle of 2 files or when using Falcon fast boot
> mode in U-Boot), nothing updates the ethernet hwaddr specified for the
> CPSW slaves, causing the driver to use a random hwaddr, which is some times
> troublesome.
> 
> The am33xx has unique ethernet hwaddrs programmed in the efuse, so it makes
> more sense to default to these rather than random ones. Add a fixup step
> which adds mac-address dt properties using the efuse addresses if the DTB
> didn't contain valid ones.
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> Tested-by: Mark Jackson <mpfj-list@newflow.co.uk>
> Tested-by: Koen Kooi <koen@dominion.thruhere.net>
> Tested-by: Matt Porter <matt.porter@linaro.org>
> ---
> diff --git a/arch/arm/mach-omap2/am33xx-cpsw.c b/arch/arm/mach-omap2/am33xx-cpsw.c
> new file mode 100644
> index 0000000..eec29a4
> --- /dev/null
> +++ b/arch/arm/mach-omap2/am33xx-cpsw.c
> @@ -0,0 +1,94 @@
> +/*
> + * am335x specific cpsw dt fixups
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/etherdevice.h>
> +#include <linux/of.h>
> +#include <linux/of_net.h>
> +
> +#include "soc.h"
> +#include "control.h"
> +
> +/**
> + * am33xx_dt_cpsw_set_mac_from_efuse - Add mac-address property using
> + * ethernet hwaddr from efuse
> + * @np:		Pointer to the cpsw slave to set mac address of
> + * @idx:	Mac address index to use from efuse
> + */
> +static void am33xx_dt_cpsw_set_mac_from_efuse(struct device_node *np, int idx)
> +{
> +	struct property *prop;
> +	u32 lo, hi;
> +	u8 *mac;
> +
> +	switch (idx) {
> +	case 0:
> +		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_LOW);
> +		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_HIGH);
> +		break;
> +
> +	case 1:
> +		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_LOW);
> +		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_HIGH);
> +		break;
> +
> +	default:
> +		pr_err("cpsw.%d: too many slaves found\n", idx);
> +		return;
> +	}
> +
> +	prop = kzalloc(sizeof(*prop) + ETH_ALEN, GFP_KERNEL);
> +	if (!prop)
> +		return;
> +
> +	prop->value = prop + 1;
> +	prop->length = ETH_ALEN;
> +	prop->name = kstrdup("mac-address", GFP_KERNEL);

Hmm. Do we want this to be mac-address or local-mac-address? If it's
mac-address, then this will override anything set by u-boot (by means
of priority in of_net). I don't think that's desireable. So it should
probably set local-mac-address instead.

> +	if (!prop->name) {
> +		kfree(prop);
> +		return;
> +	}
> +
> +	mac = prop->value;
> +
> +	mac[0] = hi;
> +	mac[1] = hi >> 8;
> +	mac[2] = hi >> 16;
> +	mac[3] = hi >> 24;
> +	mac[4] = lo;
> +	mac[5] = lo >> 8;
> +
> +	of_update_property(np, prop);

mach-mxs does this too, I wonder if it's worth creating a small helper for it.

Doesn't have to be done as part of this patch but it's still worth doing.

> +
> +	pr_info("cpsw.%d: No hwaddr in dt. Using %pM from efuse\n", idx, mac);
> +}
> +
> +static int __init am33xx_dt_cpsw_mac_fixup(void)
> +{
> +	struct device_node *np, *slave;
> +	int idx = 0;
> +
> +	if (!soc_is_am33xx())
> +		return -ENODEV;
> +
> +	for_each_compatible_node(np, NULL, "ti,cpsw")
> +		for_each_node_by_name(slave, "slave") {

The binding doesn't enforce "slave" node names for the subnodes, so you
should probably just iterate over children. Right now they are named
slave@0 and slave@1, but lack reg properties (and a notion of what said
reg property would symbolize).

> +			if (!of_get_mac_address(slave))
> +				am33xx_dt_cpsw_set_mac_from_efuse(slave, idx);
> +			idx++;

You're assuming that you get the children in order here, which is not
guaranteed.

Maybe best is to adjust the binding, to make "reg" mean something (i.e. the MAC
index for this hardware) and use the <reg> property of the childnode to tell
which efuse to read.


-Olof

Re: [PATCHv2 RESEND] am33xx: cpsw: default to ethernet hwaddr from efuse if not defined in dt

[Peter Korsgaard]

>>>>> "Olof" == Olof Johansson <olof@lixom.net> writes:

Hi Olof,

Thanks for the review!

 >> +/**
 >> + * am33xx_dt_cpsw_set_mac_from_efuse - Add mac-address property using
 >> + * ethernet hwaddr from efuse
 >> + * @np:		Pointer to the cpsw slave to set mac address of
 >> + * @idx:	Mac address index to use from efuse
 >> + */
 >> +static void am33xx_dt_cpsw_set_mac_from_efuse(struct device_node *np, int idx)
 >> +{
 >> +	struct property *prop;
 >> +	u32 lo, hi;
 >> +	u8 *mac;
 >> +
 >> +	switch (idx) {
 >> +	case 0:
 >> +		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_LOW);
 >> +		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID0_HIGH);
 >> +		break;
 >> +
 >> +	case 1:
 >> +		lo = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_LOW);
 >> +		hi = omap_ctrl_readl(AM33XX_CONTROL_MAC_ID1_HIGH);
 >> +		break;
 >> +
 >> +	default:
 >> +		pr_err("cpsw.%d: too many slaves found\n", idx);
 >> +		return;
 >> +	}
 >> +
 >> +	prop = kzalloc(sizeof(*prop) + ETH_ALEN, GFP_KERNEL);
 >> +	if (!prop)
 >> +		return;
 >> +
 >> +	prop->value = prop + 1;
 >> +	prop->length = ETH_ALEN;
 >> +	prop->name = kstrdup("mac-address", GFP_KERNEL);

 Olof> Hmm. Do we want this to be mac-address or local-mac-address? If it's
 Olof> mac-address, then this will override anything set by u-boot (by means
 Olof> of priority in of_net). I don't think that's desireable. So it should
 Olof> probably set local-mac-address instead.

It doesn't really matter as this only gets called if the DTB doesn't
contain any valid mac-address / local-mac-address properties (the
if (!of_get_mac_address(slave)) check).



 >> +	if (!prop->name) {
 >> +		kfree(prop);
 >> +		return;
 >> +	}
 >> +
 >> +	mac = prop->value;
 >> +
 >> +	mac[0] = hi;
 >> +	mac[1] = hi >> 8;
 >> +	mac[2] = hi >> 16;
 >> +	mac[3] = hi >> 24;
 >> +	mac[4] = lo;
 >> +	mac[5] = lo >> 8;
 >> +
 >> +	of_update_property(np, prop);

 Olof> mach-mxs does this too, I wonder if it's worth creating a small helper for it.

Yeah, guess where I got the inspiration to do this from? ;)


 Olof> Doesn't have to be done as part of this patch but it's still worth doing.


Ok, I'll send a patch adding of_set_mac_address() and change am335x /
mxs to use it once this is in.


 >> +
 >> +	pr_info("cpsw.%d: No hwaddr in dt. Using %pM from efuse\n", idx, mac);
 >> +}
 >> +
 >> +static int __init am33xx_dt_cpsw_mac_fixup(void)
 >> +{
 >> +	struct device_node *np, *slave;
 >> +	int idx = 0;
 >> +
 >> +	if (!soc_is_am33xx())
 >> +		return -ENODEV;
 >> +
 >> +	for_each_compatible_node(np, NULL, "ti,cpsw")
 >> +		for_each_node_by_name(slave, "slave") {

 Olof> The binding doesn't enforce "slave" node names for the subnodes, so you
 Olof> should probably just iterate over children. Right now they are named
 Olof> slave@0 and slave@1, but lack reg properties (and a notion of what said
 Olof> reg property would symbolize).

True. This just follows what the cpsw driver does. I would prefer to
change the driver, this file, am33xx.dtsi and the bindings documentation
in one go rather than doing something else than the driver here.


 >> +			if (!of_get_mac_address(slave))
 >> +				am33xx_dt_cpsw_set_mac_from_efuse(slave, idx);
 >> +			idx++;

 Olof> You're assuming that you get the children in order here, which is not
 Olof> guaranteed.

Again, this is in line with cpsw.c. Once we add a reg property we can
use that as idx.


 Olof> Maybe best is to adjust the binding, to make "reg" mean something
 Olof> (i.e. the MAC index for this hardware) and use the <reg> property
 Olof> of the childnode to tell which efuse to read.

Indeed.

-- 
Bye, Peter Korsgaard