Re: [PATCH] clk: provide public clk_is_enabled function
From: Sebastian Hesselbarth
Date: Sun Oct 06 2013 - 16:25:35 EST
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/