Message-ID: <CAPtuhTiaTX1VX0OnkJN6AeqtT+FW+uK7022iJ2wOCirQmMXH4Q@mail.gmail.com>
Date:	Sun, 6 Oct 2013 14:04:08 -0700
From:	Mike Turquette <mturquette@...aro.org>
To:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Cc:	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" <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" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] clk: provide public clk_is_enabled function

On Sun, Oct 6, 2013 at 3:24 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@...il.com> wrote:
> 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.

Sorry for any misinterpretation on my end. I agree reading the
register(s) within a clk_enable/clk_disable-protected section is the
most sane option.

Regards,
Mike

>
> 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