| Message ID | 1369253042-15082-1-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
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
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.
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/
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.
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
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.
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
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.
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.
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.
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
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.
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();
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(-)