Message-ID: <5251E334.2070008@gmail.com>
Date:	Mon, 07 Oct 2013 00:24:52 +0200
From:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To:	Mike Turquette <mturquette@...aro.org>,
	Andrew Lunn <andrew@...n.ch>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Russell King <linux@....linux.org.uk>,
	Jason Cooper <jason@...edaemon.net>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	linux-kernel@...r.kernel.org,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Grant Likely <grant.likely@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] clk: provide public clk_is_enabled function

On 10/06/2013 10:02 PM, Mike Turquette wrote:
> Quoting Sebastian Hesselbarth (2013-10-06 12:42:01)
>> On 10/06/2013 06:30 PM, Andrew Lunn wrote:
>>> On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
>>>> What's wrong with an explicit enable/disable around the data
>>>> acquisition?
>>>
>>> It avoids the CPU locking hard, but will not get us a valid MAC
>>> address, which is the point of the exercise.
>>
>> While I agree to all Andew explained above, I somehow have the strong
>> feeling that an clk_is_enabled will just be abused where possible. We
>> already have two ppl complaining about it - even though a quick look at
>> clk/core.c should have cleared out most of it.
>>
>> Maybe we should just enable the clock, get the (possibly bogus) MAC
>> and disable it again. We loose one possible FW_BUG report and overwrite
>> an invalid local-mac-address property with another invalid one.
>
> Firstly, I'm OK with adding a new clk_is_enabled API for The Right
> Reasons, if we can figure out what those reasons are. A major concern is
> the lack of locks/barriers around that call that create a critical
> section where the enabled state is guaranteed not to change. Those locks
> are not exported to drivers nor do I want them to be.
>
> Secondly, this specific Ethernet/MAC address issue seems like another
> case of "the driver should be calling clk_get and clk_prepare_enable but
> for some reason it doesn't". This seems to be a common pattern and I'm
> not sure why. Does calling clk_enable on your clock which has been

Ok, the driver is doing clk_prepare_enable/disable_unprepare just fine.
It has nothing to do with the driver but the workaround for broken HW
explained below.

> already enabled by the bootloader cause issues for you? If not then it
> is better to just call clk_enable and have the clock framework book
> keeping in-sync with the hardware state.
>
> Is it possible in your case to only detect the invalid MAC address
> without sniffing the state of the clock hardware? Isn't it valid to
> report a bootloader bug after only looking at the MAC address and not
> the clock enabled state?

The ethernet IP used on Kirkwood (mv643xx_eth) is loosing the MAC
address register contents on gated clocks. Everything was fine without
DT and CCF, because clocks had to be enabled by bootloader. With DT and
CCF, we gained clock gating but realized that IP has this flaw. For
built-in ethernet driver usually everything is just fine, because driver
picks up clock early and it never gets gated.

As people were complaining about modular ethernet driver, we thought
about how to (a) work around non-DT aware boot loaders, which we have
a lot of and its not likely to change soon nor quick and (b) gate those
clocks if possible.

The workaround until now was to always enable the clocks in board init
code, leaving clocks running. Now we want to switch to update the DT
with the MAC address possibly found in those registers. It is also done
on some other machs and archs, so I guess it is not uncommon do
workaround broken/unaware bootloaders this way.

The idea is to check nodes status and skip already disabled nodes. For
enabled nodes, check the MAC address properties and skip those with
valid MAC. Now, my initial workaround was to read registers for those
left and copy the MAC address to local-mac-address property.

Andrew has mentioned, that some bootloaders might disable clocks but
leave the nodes enabled. Reading those registers would lock up
the HW, of course. So we thought about to check clk gate status first,
which this patch is about.

Of course, we can do clk_enable, read, clk_disable as said before - and
given the amount of questions and misinterpretation, I think it is the
saner way.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists