[PATCH 0/4] Kirkwood: minor cleanups

[PATCH 0/4] Kirkwood: minor cleanups

[Ezequiel Garcia]

This is just a small patchset containing minor cleanups for Kirkwood SoC
as a result of a review of the mach-dependent DT initialization code.

The first patch is a trivial cleanup, with no dependencies.

The second patch is also trivial, but given I don't have any boards
with PCIe to test, I would appreciate if someone could give it a try.

The third and fourth patch are related: the latter strictly depends on
the former. The idea behind these two patches is basically to convert
the "keep the ethernet clocks enabled by calling clk_enable()" hack
into a simpler and cleaner usage of the CLK_IGNORE_UNUSED flags which
prevent these clocks from being ever gated.

I've tested these two by building the ethernet driver as a module
and checking the MAC address is properly retained.

Any comments welcome.

Ezequiel Garcia (4):
  ARM: kirkwood: Remove unneeded MBus initialization
  ARM: kirkwood: Remove unneeded PCIe clock adding
  clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks
  ARM: kirkwood: Remove redundant ethernet clock enable

 arch/arm/mach-kirkwood/board-dt.c | 50 ---------------------------------------
 drivers/clk/mvebu/kirkwood.c      |  9 +++++--
 2 files changed, 7 insertions(+), 52 deletions(-)

-- 
1.8.1.5

[PATCH 1/4] ARM: kirkwood: Remove unneeded MBus initialization

[Ezequiel Garcia]

Since the MBus is initialized from the DT, it's not necessary to
call the legacy initialization.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-kirkwood/board-dt.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index 82d3ad8..ee6297e 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -72,13 +72,6 @@ static void __init kirkwood_dt_time_init(void)
 	clocksource_of_init();
 }
 
-static void __init kirkwood_dt_init_early(void)
-{
-	mvebu_mbus_init("marvell,kirkwood-mbus",
-			BRIDGE_WINS_BASE, BRIDGE_WINS_SZ,
-			DDR_WINDOW_CPU_BASE, DDR_WINDOW_CPU_SZ);
-}
-
 static void __init kirkwood_dt_init(void)
 {
 	pr_info("Kirkwood: %s, TCLK=%d.\n", kirkwood_id(), kirkwood_tclk);
@@ -121,7 +114,6 @@ static const char * const kirkwood_dt_board_compat[] = {
 DT_MACHINE_START(KIRKWOOD_DT, "Marvell Kirkwood (Flattened Device Tree)")
 	/* Maintainer: Jason Cooper <jason@lakedaemon.net> */
 	.map_io		= kirkwood_map_io,
-	.init_early	= kirkwood_dt_init_early,
 	.init_time	= kirkwood_dt_time_init,
 	.init_machine	= kirkwood_dt_init,
 	.restart	= kirkwood_restart,
-- 
1.8.1.5

[PATCH 1/4] ARM: kirkwood: Remove unneeded MBus initialization

[Jason Cooper]

On Mon, Sep 30, 2013 at 06:03:27PM -0300, Ezequiel Garcia wrote:
> Since the MBus is initialized from the DT, it's not necessary to
> call the legacy initialization.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  arch/arm/mach-kirkwood/board-dt.c | 8 --------
>  1 file changed, 8 deletions(-)

Applied to mvebu/soc

thx,

Jason.

[PATCH 2/4] ARM: kirkwood: Remove unneeded PCIe clock adding

[Ezequiel Garcia]

Since the PCIe devices is properly initialized from the DT, the clocks
are now referenced in the device tree nodes, and it's not needed
to have this hack to add them explicitly.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-kirkwood/board-dt.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index ee6297e..1bbced8 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -44,14 +44,6 @@ static void __init kirkwood_legacy_clk_init(void)
 	clkspec.np = np;
 	clkspec.args_count = 1;
 
-	clkspec.args[0] = CGC_BIT_PEX0;
-	orion_clkdev_add("0", "pcie",
-			 of_clk_get_from_provider(&clkspec));
-
-	clkspec.args[0] = CGC_BIT_PEX1;
-	orion_clkdev_add("1", "pcie",
-			 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
-- 
1.8.1.5

[PATCH 2/4] ARM: kirkwood: Remove unneeded PCIe clock adding

[Ezequiel Garcia]

On Mon, Sep 30, 2013 at 06:03:28PM -0300, Ezequiel Garcia wrote:
> Since the PCIe devices is properly initialized from the DT, the clocks
> are now referenced in the device tree nodes, and it's not needed
> to have this hack to add them explicitly.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  arch/arm/mach-kirkwood/board-dt.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
> index ee6297e..1bbced8 100644
> --- a/arch/arm/mach-kirkwood/board-dt.c
> +++ b/arch/arm/mach-kirkwood/board-dt.c
> @@ -44,14 +44,6 @@ static void __init kirkwood_legacy_clk_init(void)
>  	clkspec.np = np;
>  	clkspec.args_count = 1;
>  
> -	clkspec.args[0] = CGC_BIT_PEX0;
> -	orion_clkdev_add("0", "pcie",
> -			 of_clk_get_from_provider(&clkspec));
> -
> -	clkspec.args[0] = CGC_BIT_PEX1;
> -	orion_clkdev_add("1", "pcie",
> -			 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
> -- 
> 1.8.1.5
> 

If anyone with a PCIe Kirkwood board can give this patch a try
it would be great to have some Tested-by's.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 2/4] ARM: kirkwood: Remove unneeded PCIe clock adding

[Jason Gunthorpe]

On Tue, Oct 01, 2013 at 01:01:36PM -0300, Ezequiel Garcia wrote:
> If anyone with a PCIe Kirkwood board can give this patch a try
> it would be great to have some Tested-by's.

Worked here..

Tested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Jason

[PATCH 2/4] ARM: kirkwood: Remove unneeded PCIe clock adding

[Arnaud Ebalard]

Hi Ezequiel,

Ezequiel Garcia <ezequiel.garcia@free-electrons.com> writes:

> If anyone with a PCIe Kirkwood board can give this patch a try
> it would be great to have some Tested-by's.

Works on my ReadyNAS Duo v2 (88F6282), i.e. I can see and use the USB3.0
controller attached to the PCIe bus so ...

Tested-by: Arnaud Ebalard <arno@natisbad.org>

Cheers,

a+

[PATCH 2/4] ARM: kirkwood: Remove unneeded PCIe clock adding

[Jason Cooper]

On Mon, Sep 30, 2013 at 06:03:28PM -0300, Ezequiel Garcia wrote:
> Since the PCIe devices is properly initialized from the DT, the clocks
> are now referenced in the device tree nodes, and it's not needed
> to have this hack to add them explicitly.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  arch/arm/mach-kirkwood/board-dt.c | 8 --------
>  1 file changed, 8 deletions(-)

Applied to mvebu/soc

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Ezequiel Garcia]

These clocks should never be gated, since the ethernet interfaces forget
the assigned MAC address assigned if their clock source is turned off.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
Cc: Mike Turquette <mturquette@linaro.org>

 drivers/clk/mvebu/kirkwood.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/kirkwood.c b/drivers/clk/mvebu/kirkwood.c
index 2636a55..69d9603 100644
--- a/drivers/clk/mvebu/kirkwood.c
+++ b/drivers/clk/mvebu/kirkwood.c
@@ -220,7 +220,12 @@ CLK_OF_DECLARE(mv88f6180_core_clk, "marvell,mv88f6180-core-clock",
  */
 
 static const struct clk_gating_soc_desc kirkwood_gating_desc[] __initconst = {
-	{ "ge0", NULL, 0, 0 },
+	/*
+	 * The ethernet interfaces forget their assigned MAC address
+	 * if the clocks are turned off. Until proper DT support
+	 * is available we prevent ge0 and ge1 gating for now.
+	 */
+	{ "ge0", NULL, 0, CLK_IGNORE_UNUSED },
 	{ "pex0", NULL, 2, 0 },
 	{ "usb0", NULL, 3, 0 },
 	{ "sdio", NULL, 4, 0 },
@@ -234,7 +239,7 @@ static const struct clk_gating_soc_desc kirkwood_gating_desc[] __initconst = {
 	{ "xor1", NULL, 16, 0 },
 	{ "crypto", NULL, 17, 0 },
 	{ "pex1", NULL, 18, 0 },
-	{ "ge1", NULL, 19, 0 },
+	{ "ge1", NULL, 19, CLK_IGNORE_UNUSED },
 	{ "tdm", NULL, 20, 0 },
 	{ }
 };
-- 
1.8.1.5

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Gunthorpe]

On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:

> These clocks should never be gated, since the ethernet interfaces
> forget the assigned MAC address assigned if their clock source is
> turned off.

Hrm, this hack is only needed to support the hack of passing the MAC
address from the bootloader to the kernel via the ethernet registers.

If local-mac-address is present in the DT (or fixed up via ATAGS) then
none of this is needed.

Boards that have well formed DT's don't need this.

The major downside to this patch is we never get power savings by
disabling the 2nd GE. On my boards it is not connected so we always
want to see it powered down.

Our bootloader gates the power and turns off the clock, having Linux
turn the clock back on without knowing how to control the power seems
like a possible problem.

I don't have code, but when this was first brought up it was suggested
to add local-mac-address to the DT nodes if it isn't present in the
common code and don't mess with the clocks.

Psudeo code:

void marvell_dt_ethernet_fixup(void)
{
 for_each_compatible_node(np, "marvell,kirkwood-eth-port")
 {
   if (of_get_mac_address(np) != NULL)
      continue;
   regs = ioremap(of_regs(np->parent,0));
   address = (readl(regs + MAC_ADDR_HIGH) << 32) | 
              readl(regs + MAC_ADDR_LOW))
   iounmap(regs);
   of_add_property(np, {"local-mac-address" = address});
 }
}

Jason

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Andrew Lunn]

Hi Jason

> Our bootloader gates the power and turns off the clock, having Linux
> turn the clock back on without knowing how to control the power seems
> like a possible problem.

This patch won't do that. This patch will stop it getting turn off. It
will never turn it on. The only thing that will turn it on is the
Ethernet driver. So if the bootloader turned it off, it should stay
off.

> I don't have code, but when this was first brought up it was suggested
> to add local-mac-address to the DT nodes if it isn't present in the
> common code and don't mess with the clocks.
> 
> Psudeo code:
> 
> void marvell_dt_ethernet_fixup(void)
> {
>  for_each_compatible_node(np, "marvell,kirkwood-eth-port")
>  {
>    if (of_get_mac_address(np) != NULL)
>       continue;
>    regs = ioremap(of_regs(np->parent,0));
>    address = (readl(regs + MAC_ADDR_HIGH) << 32) | 
>               readl(regs + MAC_ADDR_LOW))
>    iounmap(regs);
>    of_add_property(np, {"local-mac-address" = address});
>  }
> }

The pseudo code looks nice, but i think in reality, the
of_add_property(np, {"local-mac-address" = address}); is not so
simple. Sebastian took a look at this.

	Andrew

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Gunthorpe]

On Mon, Sep 30, 2013 at 11:34:37PM +0200, Andrew Lunn wrote:

> > Our bootloader gates the power and turns off the clock, having Linux
> > turn the clock back on without knowing how to control the power seems
> > like a possible problem.
> 
> This patch won't do that. This patch will stop it getting turn off. It
> will never turn it on. The only thing that will turn it on is the
> Ethernet driver. So if the bootloader turned it off, it should stay
> off.

Ok, that sounds good.

> The pseudo code looks nice, but i think in reality, the
> of_add_property(np, {"local-mac-address" = address}); is not so
> simple. Sebastian took a look at this.

Oh? I though it was just this:

struct Tmp
{
    struct property prop;
    u8 mac[6];
};

struct Tmp *prop = kzalloc(sizeof(struct Tmp));
prop->prop.name = "local-mac-address";
prop->prop.length = sizeof(prop->mac);
prop->prop.value = prop->mac;
prop->mac = (...);
of_add_property(np, &prop->prop);

That is basically what other stuff in the kernel does, at least.

Jason

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Mon, Sep 30, 2013 at 03:31:31PM -0600, Jason Gunthorpe wrote:
> On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
> 
> > These clocks should never be gated, since the ethernet interfaces
> > forget the assigned MAC address assigned if their clock source is
> > turned off.
> 
> Hrm, this hack is only needed to support the hack of passing the MAC
> address from the bootloader to the kernel via the ethernet registers.
> 
> If local-mac-address is present in the DT (or fixed up via ATAGS) then
> none of this is needed.
> 
> Boards that have well formed DT's don't need this.
> 
> The major downside to this patch is we never get power savings by
> disabling the 2nd GE. On my boards it is not connected so we always
> want to see it powered down.
> 
> Our bootloader gates the power and turns off the clock, having Linux
> turn the clock back on without knowing how to control the power seems
> like a possible problem.
> 
> I don't have code, but when this was first brought up it was suggested
> to add local-mac-address to the DT nodes if it isn't present in the
> common code and don't mess with the clocks.
> 
> Psudeo code:
> 
> void marvell_dt_ethernet_fixup(void)
> {
>  for_each_compatible_node(np, "marvell,kirkwood-eth-port")
>  {
>    if (of_get_mac_address(np) != NULL)
>       continue;
>    regs = ioremap(of_regs(np->parent,0));
>    address = (readl(regs + MAC_ADDR_HIGH) << 32) | 
>               readl(regs + MAC_ADDR_LOW))
>    iounmap(regs);
>    of_add_property(np, {"local-mac-address" = address});
>  }
> }

Or, could we test for the mac address in the driver's suspend routine
and set the clock's IGNORE_UNUSED accordingly?  That wouldn't cover the
case of the modules being loaded after boot, but it would get us power
savings when we can.

It also has the benefit of being easily discoverable by future users
wondering why the clock isn't gated when attempting to conserve power.

To that end, we could set the bit as Ezequiel has done, and then change
it in the eth driver's probe if there's a mac address in the DT.


thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
> These clocks should never be gated, since the ethernet interfaces forget
> the assigned MAC address assigned if their clock source is turned off.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
> Cc: Mike Turquette <mturquette@linaro.org>
> 
>  drivers/clk/mvebu/kirkwood.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

I'm inclined to leave this as a hack in board-dt.c...  At some point we
are going to move kirkwood over to mach-mvebu/, and there will be eyes
on the code.  There's less chance of 're-discovering' this for cleanup
if it's in the clock driver, looking all proper and such.  Even with the
clear comment.

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Ezequiel Garcia]

On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote:
> On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
> > These clocks should never be gated, since the ethernet interfaces forget
> > the assigned MAC address assigned if their clock source is turned off.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> > Cc: Mike Turquette <mturquette@linaro.org>
> > 
> >  drivers/clk/mvebu/kirkwood.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> I'm inclined to leave this as a hack in board-dt.c...  At some point we
> are going to move kirkwood over to mach-mvebu/, and there will be eyes
> on the code.  There's less chance of 're-discovering' this for cleanup
> if it's in the clock driver, looking all proper and such.  Even with the
> clear comment.
> 

Hm.. Really? I honestly think this change is a far cleaner way of
dealing with the problem.

The motivation for these 4 patches, came from an attempt of booting a
multiplatform kernel Kirkwood, which _almost_ booted fine :P

So, I see this as preparation work for the move to mach-mvebu.

Does anybody have any insights on how a proper fix would be done
in the net driver?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Sebastian Hesselbarth]

On 10/01/2013 03:42 PM, Ezequiel Garcia wrote:
> On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote:
>> On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
>>> These clocks should never be gated, since the ethernet interfaces forget
>>> the assigned MAC address assigned if their clock source is turned off.
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>>> ---
>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>
>>>   drivers/clk/mvebu/kirkwood.c | 9 +++++++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> I'm inclined to leave this as a hack in board-dt.c...  At some point we
>> are going to move kirkwood over to mach-mvebu/, and there will be eyes
>> on the code.  There's less chance of 're-discovering' this for cleanup
>> if it's in the clock driver, looking all proper and such.  Even with the
>> clear comment.
>>
>
> Hm.. Really? I honestly think this change is a far cleaner way of
> dealing with the problem.

Actually I prefer to leave the workaround there, too.

> The motivation for these 4 patches, came from an attempt of booting a
> multiplatform kernel Kirkwood, which _almost_ booted fine :P

But the workaround doesn't affect "_almost_", does it?

> So, I see this as preparation work for the move to mach-mvebu.
>
> Does anybody have any insights on how a proper fix would be done
> in the net driver?

The proper fix would be anything that is related with providing
proper 'local-mac-address' property to the corresponding nodes *and*
properly setup some registers in eth ip, e.g. (R)GMII, basically
all that is usually done by u-boot.

Currently, there is no support for PHY configuration at all but
if your board uses reset defaults, IIRC it should work except for
the missing MAC address. There is one fix for kirkwood already
that enables proper clock.

Again, IIRC, I tried to push a fix similar to Jason's but used
of_set_property. Jason's looks way more sane and we should give
it a try.

Sebastian

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Ezequiel Garcia]

On Tue, Oct 01, 2013 at 03:49:43PM +0200, Sebastian Hesselbarth wrote:
> On 10/01/2013 03:42 PM, Ezequiel Garcia wrote:
> > On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote:
> >> On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
> >>> These clocks should never be gated, since the ethernet interfaces forget
> >>> the assigned MAC address assigned if their clock source is turned off.
> >>>
> >>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> >>> ---
> >>> Cc: Mike Turquette <mturquette@linaro.org>
> >>>
> >>>   drivers/clk/mvebu/kirkwood.c | 9 +++++++--
> >>>   1 file changed, 7 insertions(+), 2 deletions(-)
[..]
> > The motivation for these 4 patches, came from an attempt of booting a
> > multiplatform kernel Kirkwood, which _almost_ booted fine :P
> 
> But the workaround doesn't affect "_almost_", does it?
> 

No. I have no idea why the multiplatfrom didn't work: it booted
just fine and then just before entering userland, it freezed :P

Although I had to do some weird stuff to get it working, so it
doesn't really matter right now. It was a fun experiment though :)

> > So, I see this as preparation work for the move to mach-mvebu.
> >
> > Does anybody have any insights on how a proper fix would be done
> > in the net driver?
> 
> The proper fix would be anything that is related with providing
> proper 'local-mac-address' property to the corresponding nodes *and*
> properly setup some registers in eth ip, e.g. (R)GMII, basically
> all that is usually done by u-boot.
> 

The kirwood.dtsi file already contains 'local-mac-address' property,
but the driver is not doing anything with it.

I guess the only meaningful case we want to support is a nicely
defined MAC passed in that property, and not retaining any other
MAC set by some other way, uh?

But I haven't really looked at any of this until just now,
so I might be completely off.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Sebastian Hesselbarth]

On 10/01/2013 04:22 PM, Ezequiel Garcia wrote:
> On Tue, Oct 01, 2013 at 03:49:43PM +0200, Sebastian Hesselbarth wrote:
>> On 10/01/2013 03:42 PM, Ezequiel Garcia wrote:
>>> On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote:
>>>> On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
>>>>> These clocks should never be gated, since the ethernet interfaces forget
>>>>> the assigned MAC address assigned if their clock source is turned off.
>>>>>
>>>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>>>>> ---
>>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>>
>>>>>    drivers/clk/mvebu/kirkwood.c | 9 +++++++--
>>>>>    1 file changed, 7 insertions(+), 2 deletions(-)
> [..]
>>> The motivation for these 4 patches, came from an attempt of booting a
>>> multiplatform kernel Kirkwood, which _almost_ booted fine :P
>>
>> But the workaround doesn't affect "_almost_", does it?
>>
>
> No. I have no idea why the multiplatfrom didn't work: it booted
> just fine and then just before entering userland, it freezed :P
>
> Although I had to do some weird stuff to get it working, so it
> doesn't really matter right now. It was a fun experiment though :)
>
>>> So, I see this as preparation work for the move to mach-mvebu.
>>>
>>> Does anybody have any insights on how a proper fix would be done
>>> in the net driver?
>>
>> The proper fix would be anything that is related with providing
>> proper 'local-mac-address' property to the corresponding nodes *and*
>> properly setup some registers in eth ip, e.g. (R)GMII, basically
>> all that is usually done by u-boot.
>>
>
> The kirwood.dtsi file already contains 'local-mac-address' property,
> but the driver is not doing anything with it.

The local-mac-address property is a placeholder for the real mac
address. Also, mv643xx_eth calls of_get_mac_address which fails for
the invalid local-mac-address value of [00 00 00 00 00 00].

If you manage to setup local-mac-address in your bootloader, everything
should be fine. If we have a DT fixup that takes care of the above for
non-DT bootloaders, everything should be fine.

> I guess the only meaningful case we want to support is a nicely
> defined MAC passed in that property, and not retaining any other
> MAC set by some other way, uh?

For DT, you rely on valid addresses returned by of_get_mac_address() or
fall back to non-DT register readback, if none was passed. As most of
our kirkwood boards don't have DT support in u-boot at all, that is
what we do right now.

Drawback of the register readback is that contents get lost as soon as
you gate the clock. So the idea is to backup the contents early in the
DT to allow the clocks to be gated.

CLK_IGNORE_UNUSED is _not_ the right fix, because actually we *want*
those clocks to be gated. Either for those who not use ETH at all or
especially those boards providing only one ETH jack.

> But I haven't really looked at any of this until just now,
> so I might be completely off.

I still prefer Jason's approach and would give it a try on LAKML and
devtree ML: Backup register contents if of_get_mac_address does not
contain a valid address, and gate clocks.

Sebastian

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Gunthorpe]

On Tue, Oct 01, 2013 at 04:43:54PM +0200, Sebastian Hesselbarth wrote:

> The local-mac-address property is a placeholder for the real mac
> address. Also, mv643xx_eth calls of_get_mac_address which fails for
> the invalid local-mac-address value of [00 00 00 00 00 00].
> 
> If you manage to setup local-mac-address in your bootloader, everything
> should be fine. 

FWIW, my environment is like this, and it works fine (this is recent,
3.10 didn't work). The driver now properly sets the MAC from the DT.

The driver will also need an update to call of_get_phy_mode and set
the interface properly, and the board files will need update to
specify the phy-mode property. I'm guessing things are working for
most people because the POR value happens to match the board?

> If we have a DT fixup that takes care of the above for non-DT
> bootloaders, everything should be fine.

Exactly, the problem is a broken DT on input to the kernel, the proper
action, IMHO, is to correct the DT in the kernel, so the hacking is
contained. Similar to how pci-fixups.c centralizes fixing of broken
firmware for PCI, and how CONFIG_ARM_ATAG_DTB_COMPAT centralizes it
for ATAG conversion.

BTW, why doesn't CONFIG_ARM_ATAG_DTB_COMPAT fix this? Do the boot
loaders on these systems not pass the mac at all??

> I still prefer Jason's approach and would give it a try on LAKML and
> devtree ML: Backup register contents if of_get_mac_address does not
> contain a valid address, and gate clocks.

FWIW, I would not use the language of 'backup' - this is a fixup
correcting an unsupported input DT.

IMHO, when this code triggers it should also:

printk(KERN_ERROR FW_BUG "local-mac-address is not set");

Jason

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Tue, Oct 01, 2013 at 10:45:56AM -0600, Jason Gunthorpe wrote:
...
> BTW, why doesn't CONFIG_ARM_ATAG_DTB_COMPAT fix this? Do the boot
> loaders on these systems not pass the mac at all??

Because the ATAGs used were never approved.  The vendors just randomly
picked their own.

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Sebastian Hesselbarth]

On 10/01/2013 06:45 PM, Jason Gunthorpe wrote:
> On Tue, Oct 01, 2013 at 04:43:54PM +0200, Sebastian Hesselbarth wrote:
>
>> The local-mac-address property is a placeholder for the real mac
>> address. Also, mv643xx_eth calls of_get_mac_address which fails for
>> the invalid local-mac-address value of [00 00 00 00 00 00].
>>
>> If you manage to setup local-mac-address in your bootloader, everything
>> should be fine.
>
> FWIW, my environment is like this, and it works fine (this is recent,
> 3.10 didn't work). The driver now properly sets the MAC from the DT.
>
> The driver will also need an update to call of_get_phy_mode and set
> the interface properly, and the board files will need update to
> specify the phy-mode property. I'm guessing things are working for
> most people because the POR value happens to match the board?
>
>> If we have a DT fixup that takes care of the above for non-DT
>> bootloaders, everything should be fine.
>
> Exactly, the problem is a broken DT on input to the kernel, the proper
> action, IMHO, is to correct the DT in the kernel, so the hacking is
> contained. Similar to how pci-fixups.c centralizes fixing of broken
> firmware for PCI, and how CONFIG_ARM_ATAG_DTB_COMPAT centralizes it
> for ATAG conversion.
>
> BTW, why doesn't CONFIG_ARM_ATAG_DTB_COMPAT fix this? Do the boot
> loaders on these systems not pass the mac at all??
>
>> I still prefer Jason's approach and would give it a try on LAKML and
>> devtree ML: Backup register contents if of_get_mac_address does not
>> contain a valid address, and gate clocks.
>
> FWIW, I would not use the language of 'backup' - this is a fixup
> correcting an unsupported input DT.
>
> IMHO, when this code triggers it should also:
>
> printk(KERN_ERROR FW_BUG "local-mac-address is not set");

Ezequiel,

before you jump to prepare a patch for the above, I made one based on
my previous work and JasonG's suggestions above. Testing it right now,
looks promising.

@JasonC: To avoid rebase mess again, where do you want it based against?

Sebastian

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Sebastian Hesselbarth]

On 10/01/2013 09:10 PM, Sebastian Hesselbarth wrote:
> On 10/01/2013 06:45 PM, Jason Gunthorpe wrote:
>> On Tue, Oct 01, 2013 at 04:43:54PM +0200, Sebastian Hesselbarth wrote:
>>
>>> The local-mac-address property is a placeholder for the real mac
>>> address. Also, mv643xx_eth calls of_get_mac_address which fails for
>>> the invalid local-mac-address value of [00 00 00 00 00 00].
>>>
>>> If you manage to setup local-mac-address in your bootloader, everything
>>> should be fine.
>>
>> FWIW, my environment is like this, and it works fine (this is recent,
>> 3.10 didn't work). The driver now properly sets the MAC from the DT.
>>
>> The driver will also need an update to call of_get_phy_mode and set
>> the interface properly, and the board files will need update to
>> specify the phy-mode property. I'm guessing things are working for
>> most people because the POR value happens to match the board?
>>
>>> If we have a DT fixup that takes care of the above for non-DT
>>> bootloaders, everything should be fine.
>>
>> Exactly, the problem is a broken DT on input to the kernel, the proper
>> action, IMHO, is to correct the DT in the kernel, so the hacking is
>> contained. Similar to how pci-fixups.c centralizes fixing of broken
>> firmware for PCI, and how CONFIG_ARM_ATAG_DTB_COMPAT centralizes it
>> for ATAG conversion.
>>
>> BTW, why doesn't CONFIG_ARM_ATAG_DTB_COMPAT fix this? Do the boot
>> loaders on these systems not pass the mac at all??
>>
>>> I still prefer Jason's approach and would give it a try on LAKML and
>>> devtree ML: Backup register contents if of_get_mac_address does not
>>> contain a valid address, and gate clocks.
>>
>> FWIW, I would not use the language of 'backup' - this is a fixup
>> correcting an unsupported input DT.
>>
>> IMHO, when this code triggers it should also:
>>
>> printk(KERN_ERROR FW_BUG "local-mac-address is not set");
>
> Ezequiel,
>
> before you jump to prepare a patch for the above, I made one based on
> my previous work and JasonG's suggestions above. Testing it right now,
> looks promising.

And works:

Uncompressing Linux... done, booting the kernel.
[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Linux version 3.12.0-rc3-dirty (hesselba at edge) (gcc 
version 4.4.6 (Debian 4.4.6-14) ) #9 PREEMPT Tue Oct 1 21:01:34 CEST 2013
...
[    0.100128] [Firmware Bug]: 
/ocp at f1000000/ethernet-controller at 72000/ethernet0-port at 0: 
local-mac-address is not set
...
# cat /sys/kernel/debug/clk/clk_summary
    clock                        enable_cnt  prepare_cnt  rate
---------------------------------------------------------------------
...
  tclk                           6           6            200000000
...
     ge1                         0           0            200000000
...
     ge0                         0           0            200000000

# insmod mvmdio.ko
[  172.888802] libphy: orion_mdio_bus: probed
# insmod mv643xx_eth.ko
[  175.790366] mv643xx_eth: MV-643xx 10/100/1000 ethernet driver version 1.4
[  176.811888] mv643xx_eth_port mv643xx_eth_port.0 eth0: port 0 with MAC 
address 02:50:43:19:fd:e2

> @JasonC: To avoid rebase mess again, where do you want it based against?

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Tue, Oct 01, 2013 at 09:10:10PM +0200, Sebastian Hesselbarth wrote:
...
> before you jump to prepare a patch for the above, I made one based on
> my previous work and JasonG's suggestions above. Testing it right now,
> looks promising.
> 
> @JasonC: To avoid rebase mess again, where do you want it based against?

Just base against v3.12-rc1 and we'll see there's going to be any issues
with it.  Hopefully, the mv643xx_eth changes can be independent of the
other changes...

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Sebastian Hesselbarth]

On 10/02/2013 02:02 PM, Jason Cooper wrote:
> On Tue, Oct 01, 2013 at 09:10:10PM +0200, Sebastian Hesselbarth wrote:
> ...
>> before you jump to prepare a patch for the above, I made one based on
>> my previous work and JasonG's suggestions above. Testing it right now,
>> looks promising.
>>
>> @JasonC: To avoid rebase mess again, where do you want it based against?
>
> Just base against v3.12-rc1 and we'll see there's going to be any issues
> with it.  Hopefully, the mv643xx_eth changes can be independent of the
> other changes...

Ok, the mv643xx_eth fixes have been sent as fixes for v3.12 while the
corresponding kirkwood workaround should drop in v3.13. I hopefully find
some time tonight to prepare workaround patches.

Sebastian

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Tue, Oct 01, 2013 at 10:42:14AM -0300, Ezequiel Garcia wrote:
> On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote:
> > On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
> > > These clocks should never be gated, since the ethernet interfaces forget
> > > the assigned MAC address assigned if their clock source is turned off.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > > Cc: Mike Turquette <mturquette@linaro.org>
> > > 
> > >  drivers/clk/mvebu/kirkwood.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > I'm inclined to leave this as a hack in board-dt.c...  At some point we
> > are going to move kirkwood over to mach-mvebu/, and there will be eyes
> > on the code.  There's less chance of 're-discovering' this for cleanup
> > if it's in the clock driver, looking all proper and such.  Even with the
> > clear comment.
> > 
> 
> Hm.. Really? I honestly think this change is a far cleaner way of
> dealing with the problem.

That depends on your definition of 'problem' ;-)  It sounds like right
now you consider the problem to be kirkwood going multiplatform.  In
that case, this is a solution.

However, I think the problem is that the IP block loses it's mac address
when gated.  That hasn't been solved, all you're doing is moving the
hack/workaround from one place to another less visible place.

> The motivation for these 4 patches, came from an attempt of booting a
> multiplatform kernel Kirkwood, which _almost_ booted fine :P
> 
> So, I see this as preparation work for the move to mach-mvebu.

Fair enough.

> Does anybody have any insights on how a proper fix would be done
> in the net driver?

A reliable way to make sure the mac address is passed to the kernel from
existing bootloaders via DT.  I _really_ need to add fdt
parsing/modification to the impedance matcher.  Then we can safely say
"wrap the kernel and dtb with the impedance matcher".

/or/, we could save it in some fashion in mv643xx_eth's suspend/resume
code.  Unfortunately, it has to survive module unload/load, and there's
still the issue of loading the module after kernel boot.  Clocks would
be gated, so the mac address has to be retrieved from somewhere.

Since modifying the dt in the kernel is frowned upon, we're left with
inserting what the bootloader tells us and handing it off to the kernel
at boot time.

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Ezequiel Garcia]

On Tue, Oct 01, 2013 at 09:58:00AM -0400, Jason Cooper wrote:
> On Tue, Oct 01, 2013 at 10:42:14AM -0300, Ezequiel Garcia wrote:
> > On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote:
> > > On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
> > > > These clocks should never be gated, since the ethernet interfaces forget
> > > > the assigned MAC address assigned if their clock source is turned off.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > > ---
> > > > Cc: Mike Turquette <mturquette@linaro.org>
> > > > 
> > > >  drivers/clk/mvebu/kirkwood.c | 9 +++++++--
> > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > I'm inclined to leave this as a hack in board-dt.c...  At some point we
> > > are going to move kirkwood over to mach-mvebu/, and there will be eyes
> > > on the code.  There's less chance of 're-discovering' this for cleanup
> > > if it's in the clock driver, looking all proper and such.  Even with the
> > > clear comment.
> > > 
> > 
> > Hm.. Really? I honestly think this change is a far cleaner way of
> > dealing with the problem.
> 
> That depends on your definition of 'problem' ;-)  It sounds like right
> now you consider the problem to be kirkwood going multiplatform.  In
> that case, this is a solution.
> 

Our 'problem' definition is certainly the same.

> However, I think the problem is that the IP block loses it's mac address
> when gated.  That hasn't been solved, all you're doing is moving the
> hack/workaround from one place to another less visible place.
> 

Of course, my opinion doesn't count this time, but I still think the fix
is cleaner.

There's an already existing and clock-framework-specific way of preventing
a clock from being ever gated -which seems to match exactly this case-
so I have a hard time seeing why we wouldn't use it.

Also: I'm wondering why you think this change would 'hide' the clock gating.
It seems whenever a (future?) developer finds these clocks are not gated,
one of the first places he will look for is precisely on those flags.

Just my two cents, the hack doesn't really worth a fight.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Tue, Oct 01, 2013 at 11:17:18AM -0300, Ezequiel Garcia wrote:
> On Tue, Oct 01, 2013 at 09:58:00AM -0400, Jason Cooper wrote:
...
> > However, I think the problem is that the IP block loses it's mac address
> > when gated.  That hasn't been solved, all you're doing is moving the
> > hack/workaround from one place to another less visible place.
> > 
> 
> There's an already existing and clock-framework-specific way of preventing
> a clock from being ever gated -which seems to match exactly this case-
> so I have a hard time seeing why we wouldn't use it.
> 
> Also: I'm wondering why you think this change would 'hide' the clock gating.
> It seems whenever a (future?) developer finds these clocks are not gated,
> one of the first places he will look for is precisely on those flags.

True, I'm approaching it from "I unloaded mv643xx_eth.ko, and my power
draw didn't change.  Let's go look at the driver."  But you are correct.

I'd really like to see if we can extend your patch to having the
driver's probe function detect the mac address in the DT, and change the
IGNORE_UNUSED flag on the clock.  MikeT might have some thoughts on that
since I don't see anyone doing that atm.

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 1 Oct 2013 11:01:30 -0400, Jason Cooper wrote:
> On Tue, Oct 01, 2013 at 11:17:18AM -0300, Ezequiel Garcia wrote:
> > On Tue, Oct 01, 2013 at 09:58:00AM -0400, Jason Cooper wrote:
> ...
> > > However, I think the problem is that the IP block loses it's mac address
> > > when gated.  That hasn't been solved, all you're doing is moving the
> > > hack/workaround from one place to another less visible place.
> > > 
> > 
> > There's an already existing and clock-framework-specific way of preventing
> > a clock from being ever gated -which seems to match exactly this case-
> > so I have a hard time seeing why we wouldn't use it.
> > 
> > Also: I'm wondering why you think this change would 'hide' the clock gating.
> > It seems whenever a (future?) developer finds these clocks are not gated,
> > one of the first places he will look for is precisely on those flags.
> 
> True, I'm approaching it from "I unloaded mv643xx_eth.ko, and my power
> draw didn't change.  Let's go look at the driver."  But you are correct.
> 
> I'd really like to see if we can extend your patch to having the
> driver's probe function detect the mac address in the DT, and change the
> IGNORE_UNUSED flag on the clock.  MikeT might have some thoughts on that
> since I don't see anyone doing that atm.

Sorry for jumping into the discussion, but how does that solve the case
when the driver is loaded as a module, and therefore unused clocks are
disabled before the driver ->probe() function gets called?

Thomas
-- 
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Tue, Oct 01, 2013 at 05:09:32PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 1 Oct 2013 11:01:30 -0400, Jason Cooper wrote:
> > On Tue, Oct 01, 2013 at 11:17:18AM -0300, Ezequiel Garcia wrote:
> > > On Tue, Oct 01, 2013 at 09:58:00AM -0400, Jason Cooper wrote:
> > ...
> > > > However, I think the problem is that the IP block loses it's mac address
> > > > when gated.  That hasn't been solved, all you're doing is moving the
> > > > hack/workaround from one place to another less visible place.
> > > > 
> > > 
> > > There's an already existing and clock-framework-specific way of preventing
> > > a clock from being ever gated -which seems to match exactly this case-
> > > so I have a hard time seeing why we wouldn't use it.
> > > 
> > > Also: I'm wondering why you think this change would 'hide' the clock gating.
> > > It seems whenever a (future?) developer finds these clocks are not gated,
> > > one of the first places he will look for is precisely on those flags.
> > 
> > True, I'm approaching it from "I unloaded mv643xx_eth.ko, and my power
> > draw didn't change.  Let's go look at the driver."  But you are correct.
> > 
> > I'd really like to see if we can extend your patch to having the
> > driver's probe function detect the mac address in the DT, and change the
> > IGNORE_UNUSED flag on the clock.  MikeT might have some thoughts on that
> > since I don't see anyone doing that atm.
> 
> Sorry for jumping into the discussion, but how does that solve the case
> when the driver is loaded as a module, and therefore unused clocks are
> disabled before the driver ->probe() function gets called?

Sorry, I wrote the reply to JasonG just moments before and assumed a
little knowledge of that in this reply.

We would use Ezequiel's patch to ensure CLK_IGNORE_UNUSED is set for the
clock during boot, then, after boot, mv643xx_eth's probe would look for
the mac address in the DT.  If it's there, it would unset the flag
knowing that it can recover the mac address from the DT.  Then we could
get the power savings from suspend/unload.

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 1 Oct 2013 11:15:28 -0400, Jason Cooper wrote:

> > Sorry for jumping into the discussion, but how does that solve the case
> > when the driver is loaded as a module, and therefore unused clocks are
> > disabled before the driver ->probe() function gets called?
> 
> Sorry, I wrote the reply to JasonG just moments before and assumed a
> little knowledge of that in this reply.
> 
> We would use Ezequiel's patch to ensure CLK_IGNORE_UNUSED is set for the
> clock during boot, then, after boot, mv643xx_eth's probe would look for
> the mac address in the DT.  If it's there, it would unset the flag
> knowing that it can recover the mac address from the DT.  Then we could
> get the power savings from suspend/unload.

Ah, ok. What was wrong with the idea of getting the MAC address from
the hardware early at boot time, and adjust the in-memory Device Tree
with this information, so that the mv643xx_eth driver can find them
whenever it gets loaded, and regardless of whether clocks have been
disabled or not?

Thomas
-- 
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Tue, Oct 01, 2013 at 05:33:58PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 1 Oct 2013 11:15:28 -0400, Jason Cooper wrote:
> 
> > > Sorry for jumping into the discussion, but how does that solve the case
> > > when the driver is loaded as a module, and therefore unused clocks are
> > > disabled before the driver ->probe() function gets called?
> > 
> > Sorry, I wrote the reply to JasonG just moments before and assumed a
> > little knowledge of that in this reply.
> > 
> > We would use Ezequiel's patch to ensure CLK_IGNORE_UNUSED is set for the
> > clock during boot, then, after boot, mv643xx_eth's probe would look for
> > the mac address in the DT.  If it's there, it would unset the flag
> > knowing that it can recover the mac address from the DT.  Then we could
> > get the power savings from suspend/unload.
> 
> Ah, ok. What was wrong with the idea of getting the MAC address from
> the hardware early at boot time, and adjust the in-memory Device Tree
> with this information, so that the mv643xx_eth driver can find them
> whenever it gets loaded, and regardless of whether clocks have been
> disabled or not?

I'm personally not opposed to that at all.  iirc, there was some
opposition (from Grant?) to modifying the DT from within the kernel.
Sorry, I can't put my finger on the objection atm.

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 1 Oct 2013 11:36:31 -0400, Jason Cooper wrote:

> > Ah, ok. What was wrong with the idea of getting the MAC address from
> > the hardware early at boot time, and adjust the in-memory Device Tree
> > with this information, so that the mv643xx_eth driver can find them
> > whenever it gets loaded, and regardless of whether clocks have been
> > disabled or not?
> 
> I'm personally not opposed to that at all.  iirc, there was some
> opposition (from Grant?) to modifying the DT from within the kernel.
> Sorry, I can't put my finger on the objection atm.

Strange, because when we discussed the MBus DT binding, we were told
exactly the opposite: the in-memory DT should be updated to reflect the
MBus windows that are created dynamically (if I remember correctly).
So it seems like updating the DT from the kernel was not really
frowned upon (but it's true the of API does not really encourage that).

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Tue, Oct 01, 2013 at 05:42:51PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 1 Oct 2013 11:36:31 -0400, Jason Cooper wrote:
> 
> > > Ah, ok. What was wrong with the idea of getting the MAC address from
> > > the hardware early at boot time, and adjust the in-memory Device Tree
> > > with this information, so that the mv643xx_eth driver can find them
> > > whenever it gets loaded, and regardless of whether clocks have been
> > > disabled or not?
> > 
> > I'm personally not opposed to that at all.  iirc, there was some
> > opposition (from Grant?) to modifying the DT from within the kernel.
> > Sorry, I can't put my finger on the objection atm.
> 
> Strange, because when we discussed the MBus DT binding, we were told
> exactly the opposite: the in-memory DT should be updated to reflect the
> MBus windows that are created dynamically (if I remember correctly).
> So it seems like updating the DT from the kernel was not really
> frowned upon (but it's true the of API does not really encourage that).

Well, MBus is by it's nature a dynamic beast, the MAC address shouldn't
be. ;-)  So I think the change was viewed as using the DT as a form of
global data storage.

At any rate, we could keep guessing, or have Ezequiel float a patch
doing just that (store the mac address in the DT if one isn't set) and
see where things stand.

I suspect the proposed patch would need to be contained within
mv643xx_eth.c as opposed to mach-kirkwood/.  Which means doing the
CLK_IGNORE_UNUSED workaround until the driver loads.

thx,

Jason.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Sebastian Hesselbarth]

On 10/01/2013 05:53 PM, Jason Cooper wrote:
> On Tue, Oct 01, 2013 at 05:42:51PM +0200, Thomas Petazzoni wrote:
>> Dear Jason Cooper,
>>
>> On Tue, 1 Oct 2013 11:36:31 -0400, Jason Cooper wrote:
>>
>>>> Ah, ok. What was wrong with the idea of getting the MAC address from
>>>> the hardware early at boot time, and adjust the in-memory Device Tree
>>>> with this information, so that the mv643xx_eth driver can find them
>>>> whenever it gets loaded, and regardless of whether clocks have been
>>>> disabled or not?
>>>
>>> I'm personally not opposed to that at all.  iirc, there was some
>>> opposition (from Grant?) to modifying the DT from within the kernel.
>>> Sorry, I can't put my finger on the objection atm.
>>
>> Strange, because when we discussed the MBus DT binding, we were told
>> exactly the opposite: the in-memory DT should be updated to reflect the
>> MBus windows that are created dynamically (if I remember correctly).
>> So it seems like updating the DT from the kernel was not really
>> frowned upon (but it's true the of API does not really encourage that).
>
> Well, MBus is by it's nature a dynamic beast, the MAC address shouldn't
> be. ;-)  So I think the change was viewed as using the DT as a form of
> global data storage.
>
> At any rate, we could keep guessing, or have Ezequiel float a patch
> doing just that (store the mac address in the DT if one isn't set) and
> see where things stand.

Have a look at Jason G's proposal,
arch/xtensa/platforms/xtfpga/setup.c,
arch/arm/mach-mxs/mach-mxs.c

They all use of_update_property (JasonG uses of_add_property, which
is finally called by of_update_property if non-existant).

IMHO updating DT that is missing a _valid_ MAC address is the only
option we have here.

BTW, introducing of_update_property to mach-mxs.c got its Acked-by
from Benjamin Herrenschmidt, so I guess it is accepted practice to
update DT here.

> I suspect the proposed patch would need to be contained within
> mv643xx_eth.c as opposed to mach-kirkwood/.  Which means doing the
> CLK_IGNORE_UNUSED workaround until the driver loads.

[PATCH 3/4] clk: kirkwood: Add CLK_IGNORE_UNUSED to ethernet ge0 and ge1 clocks

[Jason Cooper]

On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote:
> These clocks should never be gated, since the ethernet interfaces forget
> the assigned MAC address assigned if their clock source is turned off.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
> Cc: Mike Turquette <mturquette@linaro.org>
> 
>  drivers/clk/mvebu/kirkwood.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

fyi - I've dropped this and #4 favor of Sebastian's series covering the
same issue.

thx,

Jason.

[PATCH 4/4] ARM: kirkwood: Remove redundant ethernet clock enable

[Ezequiel Garcia]

After adding the CLK_IGNORE_UNUSED flags for the ethernet clocks
ge0 and ge1, we can get rid of the hack to prevent such clocks
from being gated.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-kirkwood/board-dt.c | 34 ----------------------------------
 1 file changed, 34 deletions(-)

diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index 1bbced8..92be484 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -27,37 +27,6 @@
 #include <plat/common.h>
 #include "common.h"
 
-/*
- * 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.
-*/
-
-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;
-
-	/*
-	 * 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_dt_time_init(void)
 {
 	of_clk_init(NULL);
@@ -83,9 +52,6 @@ static void __init kirkwood_dt_init(void)
 
 	kirkwood_cpufreq_init();
 
-	/* Setup clocks for legacy devices */
-	kirkwood_legacy_clk_init();
-
 	kirkwood_cpuidle_init();
 
 #ifdef CONFIG_KEXEC
-- 
1.8.1.5

[PATCH 0/4] Kirkwood: minor cleanups

[Andrew Lunn]

> The third and fourth patch are related: the latter strictly depends on
> the former. The idea behind these two patches is basically to convert
> the "keep the ethernet clocks enabled by calling clk_enable()" hack
> into a simpler and cleaner usage of the CLK_IGNORE_UNUSED flags which
> prevent these clocks from being ever gated.

Hi Ezequiel

This should work, but what would be nicer is to look and see if there
is a node in DT for each device. If it does not have a node, don't set
CLK_IGNORE_UNUSED, so that the clock does get turned off, and we save
a bit of power. Quite a few of the kirkwood devices only have one
ethernet.

	Andrew

[PATCH 0/4] Kirkwood: minor cleanups

[Ezequiel Garcia]

On Mon, Sep 30, 2013 at 11:16:50PM +0200, Andrew Lunn wrote:
> > The third and fourth patch are related: the latter strictly depends on
> > the former. The idea behind these two patches is basically to convert
> > the "keep the ethernet clocks enabled by calling clk_enable()" hack
> > into a simpler and cleaner usage of the CLK_IGNORE_UNUSED flags which
> > prevent these clocks from being ever gated.
> 
> Hi Ezequiel
> 
> This should work, but what would be nicer is to look and see if there
> is a node in DT for each device. If it does not have a node, don't set
> CLK_IGNORE_UNUSED, so that the clock does get turned off, and we save
> a bit of power. Quite a few of the kirkwood devices only have one
> ethernet.
> 

Good idea. I'll take a look at it.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com