Patchwork [1/2] ARM: kirkwood: proper retain MAC address workaround on DT ethernet

login
register
mail settings
Submitter Sebastian Hesselbarth
Date May 22, 2013, 8:04 p.m.
Message ID <1369253042-15082-1-git-send-email-sebastian.hesselbarth@gmail.com>
Download mbox | patch
Permalink /patch/245705/
State Not Applicable
Headers show

Comments

Sebastian Hesselbarth - May 22, 2013, 8:04 p.m.
Kirkwood ethernet controllers suffer from loosing MAC register content
on gated clocks. In the past this was prevented by not gating the ethernet
controller clocks. With DT support for mv643xx_eth and corresponding
nodes available, a different approach is more reasonable.

This patch replaces the former clock gating workaround by parsing the
ethernet controller nodes for *invalid* MAC addresses and overwrites
the local-mac-address property with MAC register contents early. The
clock can now properly gated in modular mv643xx_eth and DT agnostic
boot loader scenarios because mv643xx_eth will find the stored MAC
in the corresponding MAC address property.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Lennert Buytenhek <buytenh@wantstofly.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/mach-kirkwood/board-dt.c |   67 +++++++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 14 deletions(-)
David Miller - May 26, 2013, 4:04 a.m.
From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Wed, 22 May 2013 22:04:01 +0200

> +			memcpy((void *)p->value, reg, 6);

This cast is completely unnecessary, non-void to void pointer casts
are automatic.

If it is necessary, because p->value is const, then you are trying
to change something behind the OF layer's back and need to use
the appropriate interface to change the property contents.
Sebastian Hesselbarth - May 26, 2013, 8:06 p.m.
On 05/26/2013 06:04 AM, David Miller wrote:
> From: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
> Date: Wed, 22 May 2013 22:04:01 +0200
>
>> +			memcpy((void *)p->value, reg, 6);
>
> This cast is completely unnecessary, non-void to void pointer casts
> are automatic.
>
> If it is necessary, because p->value is const, then you are trying
> to change something behind the OF layer's back and need to use
> the appropriate interface to change the property contents.

David,

good you mention it. I added Grant on Cc and will give a short
sum-up why I casted the const from property->value away here.

Maybe I overlooked the API for modifying the DT property but as
far as I've seen - there is no API for modifying it. And yes,
you are right, it is kind of an abuse of DT here.

As Kirkwoods loose their MAC address on clock gating, I was looking
for a place to store it early. (a) DT property "local-mac-address"
looked as a good place as it will allow the driver to find it without
any extra code. Of course, I am doing severaly sanity checks if it is
safe to overwrite it, i.e. no other MAC set, property is there, long
enough.

If Grant also NACKs modifying the DT we basically have two more options
left for Kirkwood: (b) have MAC stored early in two global arrays in
board init and reference that from mv643xx_eth or (c) leave the clock
ungated unconditionally on all Kirkwoods.

I can live with all three, just name it and I prepare a final patch set.

Sebastian
David Miller - May 27, 2013, 9:23 a.m.
From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Sun, 26 May 2013 22:06:58 +0200

> good you mention it. I added Grant on Cc and will give a short
> sum-up why I casted the const from property->value away here.
> 
> Maybe I overlooked the API for modifying the DT property but as
> far as I've seen - there is no API for modifying it. And yes,
> you are right, it is kind of an abuse of DT here.

Sparc has an of_set_property(), it needs to become generic.
Benjamin Herrenschmidt - May 27, 2013, 9:38 a.m.
On Sun, 2013-05-26 at 22:06 +0200, Sebastian Hesselbarth wrote:

> good you mention it. I added Grant on Cc and will give a short
> sum-up why I casted the const from property->value away here.
> 
> Maybe I overlooked the API for modifying the DT property but as
> far as I've seen - there is no API for modifying it. And yes,
> you are right, it is kind of an abuse of DT here.

of_update_property(). That also makes sure that any notifiers
is called and proc/device-tree is updated in the case where the property
is new.

> As Kirkwoods loose their MAC address on clock gating, I was looking
> for a place to store it early. (a) DT property "local-mac-address"
> looked as a good place as it will allow the driver to find it without
> any extra code. Of course, I am doing severaly sanity checks if it is
> safe to overwrite it, i.e. no other MAC set, property is there, long
> enough.
> 
> If Grant also NACKs modifying the DT we basically have two more options
> left for Kirkwood: (b) have MAC stored early in two global arrays in
> board init and reference that from mv643xx_eth or (c) leave the clock
> ungated unconditionally on all Kirkwoods.
> 
> I can live with all three, just name it and I prepare a final patch set.

No, putting it in the DT makes sense, just use the right accessor.

Cheers,
Ben.

> Sebastian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Benjamin Herrenschmidt - May 27, 2013, 9:39 a.m.
On Mon, 2013-05-27 at 02:23 -0700, David Miller wrote:
> Sparc has an of_set_property(), it needs to become generic.

There is an of_update_property(), we could change the name though, yours
is nicer :-)

Cheers,
Ben.
Sebastian Hesselbarth - May 27, 2013, 10:24 a.m.
On 05/27/13 11:39, Benjamin Herrenschmidt wrote:
> On Mon, 2013-05-27 at 02:23 -0700, David Miller wrote:
>> Sparc has an of_set_property(), it needs to become generic.
>
> There is an of_update_property(), we could change the name though, yours
> is nicer :-)

Ben, David,

I had a quick look at sparc's of_set_property. Nothing special except it
depends on OF_DYNAMIC at some place. Using of_update_property instead
should be the correct function to use. Thanks for the hint, I'll update
the patches accordingly and send v5 hopefully by today.

Sebastian
Benjamin Herrenschmidt - May 27, 2013, 11:50 a.m.
On Mon, 2013-05-27 at 12:24 +0200, Sebastian Hesselbarth wrote:
> > There is an of_update_property(), we could change the name though,
> yours
> > is nicer :-)
> 
> Ben, David,
> 
> I had a quick look at sparc's of_set_property. Nothing special except
> it
> depends on OF_DYNAMIC at some place. Using of_update_property instead
> should be the correct function to use. Thanks for the hint, I'll
> update
> the patches accordingly and send v5 hopefully by today.

The only thing is that of_update_property() is a bit awkward to use,
requiring the caller to provide an allocated struct property with
associated allocated content. It also leaks the old property which
is annoying but we haven't sorted out how to deal with allocation
of properties and property content yet.

It would be handy to be able to just do something like

	of_set_property(node, name, ptr, len);

However, that wouldn't help much with the allocation/leak problem,
though at least it would be easier to use. It could also *try* to re-use
the current allocation if the new content is of smaller or equal size.

Cheers,
Ben.
Arnd Bergmann - May 27, 2013, 12:47 p.m.
On Monday 27 May 2013 21:50:04 Benjamin Herrenschmidt wrote:
> However, that wouldn't help much with the allocation/leak problem,
> though at least it would be easier to use. It could also *try* to re-use
> the current allocation if the new content is of smaller or equal size.

I thought that dtc tried to aggressively save space by folding identical
strings. If you tried to reuse a property that had its contents shared
with another one, you would get interesting results I guess.

	Arnd
David Miller - May 27, 2013, 8:18 p.m.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Mon, 27 May 2013 21:50:04 +1000

> It would be handy to be able to just do something like
> 
> 	of_set_property(node, name, ptr, len);
> 
> However, that wouldn't help much with the allocation/leak problem,
> though at least it would be easier to use. It could also *try* to re-use
> the current allocation if the new content is of smaller or equal size.

And this is so much better of an interface because it allows the
OF implementation to decide how to deal with memory allocation
and freeing.
Benjamin Herrenschmidt - May 27, 2013, 9:48 p.m.
On Mon, 2013-05-27 at 13:18 -0700, David Miller wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Mon, 27 May 2013 21:50:04 +1000
> 
> > It would be handy to be able to just do something like
> > 
> >       of_set_property(node, name, ptr, len);
> > 
> > However, that wouldn't help much with the allocation/leak problem,
> > though at least it would be easier to use. It could also *try* to re-use
> > the current allocation if the new content is of smaller or equal size.
> 
> And this is so much better of an interface because it allows the
> OF implementation to decide how to deal with memory allocation
> and freeing.

Absolutely, I'm not arguing that point.

Cheers,
Ben.
Benjamin Herrenschmidt - May 27, 2013, 9:50 p.m.
On Mon, 2013-05-27 at 14:47 +0200, Arnd Bergmann wrote:
> On Monday 27 May 2013 21:50:04 Benjamin Herrenschmidt wrote:
> > However, that wouldn't help much with the allocation/leak problem,
> > though at least it would be easier to use. It could also *try* to re-use
> > the current allocation if the new content is of smaller or equal size.
> 
> I thought that dtc tried to aggressively save space by folding identical
> strings. If you tried to reuse a property that had its contents shared
> with another one, you would get interesting results I guess.

It used to be only property names, unless that has changed in recent
dtc. But that's a good point, we probably want a flag in struct property
like we have for nodes, indicating whether it comes from the original
fdt data pool or not.

Cheers,
Ben.
Sebastian Hesselbarth - May 27, 2013, 10:12 p.m.
On 05/27/2013 11:50 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2013-05-27 at 14:47 +0200, Arnd Bergmann wrote:
>> On Monday 27 May 2013 21:50:04 Benjamin Herrenschmidt wrote:
>>> However, that wouldn't help much with the allocation/leak problem,
>>> though at least it would be easier to use. It could also *try* to re-use
>>> the current allocation if the new content is of smaller or equal size.
>>
>> I thought that dtc tried to aggressively save space by folding identical
>> strings. If you tried to reuse a property that had its contents shared
>> with another one, you would get interesting results I guess.
>
> It used to be only property names, unless that has changed in recent
> dtc. But that's a good point, we probably want a flag in struct property
> like we have for nodes, indicating whether it comes from the original
> fdt data pool or not.

But isn't that what current sparc implementation of of_set_property does
when it marks the property as dynamic?

Anyway, this definitely exceeds my knowledge of OF API for sure, so what
do I do about the MAC workaround now?

Prepare the patch with global arrays and switch to some of_set_property
as soon as it is available?

Sebastian
David Miller - May 27, 2013, 10:17 p.m.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Tue, 28 May 2013 07:50:06 +1000

> On Mon, 2013-05-27 at 14:47 +0200, Arnd Bergmann wrote:
>> On Monday 27 May 2013 21:50:04 Benjamin Herrenschmidt wrote:
>> > However, that wouldn't help much with the allocation/leak problem,
>> > though at least it would be easier to use. It could also *try* to re-use
>> > the current allocation if the new content is of smaller or equal size.
>> 
>> I thought that dtc tried to aggressively save space by folding identical
>> strings. If you tried to reuse a property that had its contents shared
>> with another one, you would get interesting results I guess.
> 
> It used to be only property names, unless that has changed in recent
> dtc. But that's a good point, we probably want a flag in struct property
> like we have for nodes, indicating whether it comes from the original
> fdt data pool or not.

This is similar to what the "OF_IS_DYNAMIC()" thing on sparc
indicates.

Patch

diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index a86b41c..0aad9f7 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -13,6 +13,8 @@ 
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_net.h>
 #include <linux/of_platform.h>
 #include <linux/clk-provider.h>
 #include <linux/clk/mvebu.h>
@@ -31,6 +33,56 @@  static struct of_device_id kirkwood_dt_match_table[] __initdata = {
 };
 
 /*
+ * Kirkwood ethernet controllers suffer from loosing the MAC address
+ * register content on gated clocks. Rather than always ungate the
+ * clocks, we get the MAC address early and put it into DT for those
+ * boot loaders that don't provide a valid MAC address property.
+ */
+#define ETH_MAC_ADDR_L(n)	(0x400 + ((n) * 0x400) + 0x14)
+#define ETH_MAC_ADDR_H(n)	(0x400 + ((n) * 0x400) + 0x18)
+
+static void __init kirkwood_dt_eth_quirk(void)
+{
+	struct device_node *np;
+
+	for_each_compatible_node(np, NULL, "marvell,orion-eth") {
+		struct device_node *pnp;
+		void __iomem *base;
+
+		if (!of_device_is_available(np))
+			continue;
+
+		base = of_iomap(np, 0);
+		if (!base)
+			continue;
+
+		for_each_available_child_of_node(np, pnp) {
+			const void *mac_addr;
+			struct property *p;
+			u32 n, reg[2];
+
+			mac_addr = of_get_mac_address(pnp);
+			if (mac_addr)
+				continue;
+
+			p = of_find_property(pnp, "local-mac-address", NULL);
+			if (!p || p->length != 6)
+				continue;
+
+			if (of_property_read_u32(pnp, "reg", &n))
+				continue;
+
+			reg[0] = cpu_to_be32(readl(base + ETH_MAC_ADDR_H(n)));
+			reg[1] = cpu_to_be32(readl(base +
+						   ETH_MAC_ADDR_L(n)) << 16);
+			memcpy((void *)p->value, reg, 6);
+		}
+
+		iounmap(base);
+	}
+}
+
+/*
  * There are still devices that doesn't know about DT yet.  Get clock
  * gates here and add a clock lookup alias, so that old platform
  * devices still work.
@@ -42,7 +94,6 @@  static void __init kirkwood_legacy_clk_init(void)
 	struct device_node *np = of_find_compatible_node(
 		NULL, NULL, "marvell,kirkwood-gating-clock");
 	struct of_phandle_args clkspec;
-	struct clk *clk;
 
 	clkspec.np = np;
 	clkspec.args_count = 1;
@@ -58,19 +109,6 @@  static void __init kirkwood_legacy_clk_init(void)
 	clkspec.args[0] = CGC_BIT_SDIO;
 	orion_clkdev_add(NULL, "mvsdio",
 			 of_clk_get_from_provider(&clkspec));
-
-	/*
-	 * The ethernet interfaces forget the MAC address assigned by
-	 * u-boot if the clocks are turned off. Until proper DT support
-	 * is available we always enable them for now.
-	 */
-	clkspec.args[0] = CGC_BIT_GE0;
-	clk = of_clk_get_from_provider(&clkspec);
-	clk_prepare_enable(clk);
-
-	clkspec.args[0] = CGC_BIT_GE1;
-	clk = of_clk_get_from_provider(&clkspec);
-	clk_prepare_enable(clk);
 }
 
 static void __init kirkwood_of_clk_init(void)
@@ -97,6 +135,7 @@  static void __init kirkwood_dt_init(void)
 
 	/* Setup root of clk tree */
 	kirkwood_of_clk_init();
+	kirkwood_dt_eth_quirk();
 
 	kirkwood_cpuidle_init();