| Message ID | 1380881310-24345-1-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote: > To determine if a clk has been previously enabled, provide a public > clk_is_enabled function. This is especially helpful to check the state > of clk-gate without actually changing the state of the gate. > > Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> Tested-by: Andrew Lunn <andrew@lunn.ch> Andrew > --- > Cc: Mike Turquette <mturquette@linaro.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/clk/clk.c | 18 ++++++++++++++++++ > include/linux/clk.h | 13 +++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index a004769..26698d3 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -929,6 +929,24 @@ int clk_enable(struct clk *clk) > EXPORT_SYMBOL_GPL(clk_enable); > > /** > + * clk_is_enabled - return the enabled state of a clk > + * @clk: the clk whose enabled state gets returned > + * > + * Returns true if the clock is enabled, false otherwise. > + */ > +bool clk_is_enabled(struct clk *clk) > +{ > + bool is_en; > + > + clk_prepare_lock(); > + is_en = __clk_is_enabled(clk); > + clk_prepare_unlock(); > + > + return is_en; > +} > +EXPORT_SYMBOL_GPL(clk_is_enabled); > + > +/** > * __clk_round_rate - round the given rate for a clk > * @clk: round the rate of this clock > * @rate: the rate which is to be rounded > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 9a6d045..8cbf2f7 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -187,6 +187,14 @@ int clk_enable(struct clk *clk); > void clk_disable(struct clk *clk); > > /** > + * clk_is_enabled - return the enabled state of a clk > + * @clk: the clk whose enabled state gets returned > + * > + * Returns true if the clock is enabled, false otherwise. > + */ > +bool clk_is_enabled(struct clk *clk); > + > +/** > * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. > * This is only valid once the clock source has been enabled. > * @clk: clock source > @@ -299,6 +307,11 @@ static inline int clk_enable(struct clk *clk) > > static inline void clk_disable(struct clk *clk) {} > > +static inline bool clk_is_enabled(struct clk *clk) > +{ > + return false; > +} > + > static inline unsigned long clk_get_rate(struct clk *clk) > { > return 0; > -- > 1.7.10.4 >
On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote: > To determine if a clk has been previously enabled, provide a public > clk_is_enabled function. This is especially helpful to check the state > of clk-gate without actually changing the state of the gate. I wonder what you want to do with the return value. When doing if (clk_is_enabled(someclk)) do_something(); you cannot in general know if the clock is still on when you start to do_something. Best regards Uwe
On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote: > On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote: > > To determine if a clk has been previously enabled, provide a public > > clk_is_enabled function. This is especially helpful to check the state > > of clk-gate without actually changing the state of the gate. > I wonder what you want to do with the return value. > > When doing > > if (clk_is_enabled(someclk)) > do_something(); > > you cannot in general know if the clock is still on when you start to > do_something. Hi Uwe At least in the use case Sebastian needs it for, we don't need an "in general" solution. It is used early boot time to see if the boot loader left the clock running. The other user of the clock is the ethernet driver, which we know cannot change it yet, because driver probing has not started yet. Andrew
On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote: > > On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote: > > On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote: > > > To determine if a clk has been previously enabled, provide a public > > > clk_is_enabled function. This is especially helpful to check the state > > > of clk-gate without actually changing the state of the gate. > > I wonder what you want to do with the return value. > > > > When doing > > > > if (clk_is_enabled(someclk)) > > do_something(); > > > > you cannot in general know if the clock is still on when you start to > > do_something. > > Hi Uwe > > At least in the use case Sebastian needs it for, we don't need an "in > general" solution. It is used early boot time to see if the boot > loader left the clock running. Wait, unless I'm missing something, the clk_is_enabled() call _won't_ determine whether the clock is enabled in hardware (whether the boot loader created or left this condition), instead it only determines whether clk_enable() was called previously and thus the clock _shall_ be enabled. AFAIK the kernel's CCF support is "self contained" and does not consider any data or state that was "inherited" from boot staged before the kernel. That's why the "disable unused" step disables everything that wasn't acquired _in the kernel_ regardless of what the boot loader may have done or what is enabled at reset. > The other user of the clock is the > ethernet driver, which we know cannot change it yet, because driver > probing has not started yet. I understand that the situation here is, that the ethernet driver hasn't probed yet, but the clock driver did. You are in early setup code and want to (check and) fetch data from the hardware which the ethernet driver later needs. What's wrong with an explicit enable/disable around the data acquisition? This allows to reliably fetch and store the information (into the device tree data?) for later use, and is neutral to the enable counter. If the clock was enabled before, it remains enabled. If the clock was disabled before, it will get disabled again. In any case clock only may be turned off after you have grabbed the data, the data is only taken when it's available and valid. This approach of explicit enable/disable around data access only breaks if the ethernet driver won't acquire its related clock appropriately, which would be a bug in the ethernet driver that should be easy to fix. (Strictly speaking it's not the data gathering that breaks, but the already broken network driver which accesses the hardware without acquiring the clock, and not finds the clock disabled.) virtually yours Gerhard Sittig
On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote: > On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote: > > > > On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote: > > > On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote: > > > > To determine if a clk has been previously enabled, provide a public > > > > clk_is_enabled function. This is especially helpful to check the state > > > > of clk-gate without actually changing the state of the gate. > > > I wonder what you want to do with the return value. > > > > > > When doing > > > > > > if (clk_is_enabled(someclk)) > > > do_something(); > > > > > > you cannot in general know if the clock is still on when you start to > > > do_something. > > > > Hi Uwe > > > > At least in the use case Sebastian needs it for, we don't need an "in > > general" solution. It is used early boot time to see if the boot > > loader left the clock running. > > Wait, unless I'm missing something, the clk_is_enabled() call > _won't_ determine whether the clock is enabled in hardware > (whether the boot loader created or left this condition), instead > it only determines whether clk_enable() was called previously and > thus the clock _shall_ be enabled. Nope, you are wrong. static int clk_gate_is_enabled(struct clk_hw *hw) { u32 reg; struct clk_gate *gate = to_clk_gate(hw); reg = clk_readl(gate->reg); /* if a set bit disables this clk, flip it before masking */ if (gate->flags & CLK_GATE_SET_TO_DISABLE) reg ^= BIT(gate->bit_idx); reg &= BIT(gate->bit_idx); return reg ? 1 : 0; } It reads the hardware state. > AFAIK the kernel's CCF support is "self contained" and does not > consider any data or state that was "inherited" from boot staged > before the kernel. That's why the "disable unused" step disables > everything that wasn't acquired _in the kernel_ regardless of > what the boot loader may have done or what is enabled at reset. Not quite true. It uses the is_enabled(), which gets the real hardware state, to turn off clocks which are unused but on. It will not turn off clock which are already off. So it is inheriting some state from the boot loader, in that it knows if the bootloader turned it on. However this is not propagated into prepare/enable status. > > The other user of the clock is the > > ethernet driver, which we know cannot change it yet, because driver > > probing has not started yet. > > I understand that the situation here is, that the ethernet driver > hasn't probed yet, but the clock driver did. You are in early setup > code and want to (check and) fetch data from the hardware which the > ethernet driver later needs. Nearly, but not quite. If there is an enabled DT node for the device, and that node does not have a valid local-mac-address property in the node, the bootloader should of programmed the MAC address into the device. If it has done that, the clock should be running, because as soon as you turn the clock off, it forgets the MAC address. Thus, if we find an enabled device in DT, without a valid local-mac-address, and the clock is off, we have a bootloader bug, which we want to report. > 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. Andrew
On 10/06/2013 06:30 PM, Andrew Lunn wrote: > On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote: >> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote: >>> >>> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote: >>>> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote: >>>>> To determine if a clk has been previously enabled, provide a public >>>>> clk_is_enabled function. This is especially helpful to check the state >>>>> of clk-gate without actually changing the state of the gate. >>>> I wonder what you want to do with the return value. >>>> >>>> When doing >>>> >>>> if (clk_is_enabled(someclk)) >>>> do_something(); >>>> >>>> you cannot in general know if the clock is still on when you start to >>>> do_something. >>> >>> Hi Uwe >>> >>> At least in the use case Sebastian needs it for, we don't need an "in >>> general" solution. It is used early boot time to see if the boot >>> loader left the clock running. >> >> Wait, unless I'm missing something, the clk_is_enabled() call >> _won't_ determine whether the clock is enabled in hardware >> (whether the boot loader created or left this condition), instead >> it only determines whether clk_enable() was called previously and >> thus the clock _shall_ be enabled. > > Nope, you are wrong. > > static int clk_gate_is_enabled(struct clk_hw *hw) > { > u32 reg; > struct clk_gate *gate = to_clk_gate(hw); > > reg = clk_readl(gate->reg); > > /* if a set bit disables this clk, flip it before masking */ > if (gate->flags & CLK_GATE_SET_TO_DISABLE) > reg ^= BIT(gate->bit_idx); > > reg &= BIT(gate->bit_idx); > > return reg ? 1 : 0; > } > > It reads the hardware state. > >> AFAIK the kernel's CCF support is "self contained" and does not >> consider any data or state that was "inherited" from boot staged >> before the kernel. That's why the "disable unused" step disables >> everything that wasn't acquired _in the kernel_ regardless of >> what the boot loader may have done or what is enabled at reset. > > Not quite true. It uses the is_enabled(), which gets the real hardware > state, to turn off clocks which are unused but on. It will not turn > off clock which are already off. So it is inheriting some state from > the boot loader, in that it knows if the bootloader turned it > on. However this is not propagated into prepare/enable status. > >>> The other user of the clock is the >>> ethernet driver, which we know cannot change it yet, because driver >>> probing has not started yet. >> >> I understand that the situation here is, that the ethernet driver >> hasn't probed yet, but the clock driver did. You are in early setup >> code and want to (check and) fetch data from the hardware which the >> ethernet driver later needs. > > Nearly, but not quite. If there is an enabled DT node for the device, > and that node does not have a valid local-mac-address property in the > node, the bootloader should of programmed the MAC address into the > device. If it has done that, the clock should be running, because as > soon as you turn the clock off, it forgets the MAC address. Thus, if > we find an enabled device in DT, without a valid local-mac-address, > and the clock is off, we have a bootloader bug, which we want to > report. > >> 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. Sebastian
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: > >> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote: > >>> > >>> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote: > >>>> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote: > >>>>> To determine if a clk has been previously enabled, provide a public > >>>>> clk_is_enabled function. This is especially helpful to check the state > >>>>> of clk-gate without actually changing the state of the gate. > >>>> I wonder what you want to do with the return value. > >>>> > >>>> When doing > >>>> > >>>> if (clk_is_enabled(someclk)) > >>>> do_something(); > >>>> > >>>> you cannot in general know if the clock is still on when you start to > >>>> do_something. > >>> > >>> Hi Uwe > >>> > >>> At least in the use case Sebastian needs it for, we don't need an "in > >>> general" solution. It is used early boot time to see if the boot > >>> loader left the clock running. > >> > >> Wait, unless I'm missing something, the clk_is_enabled() call > >> _won't_ determine whether the clock is enabled in hardware > >> (whether the boot loader created or left this condition), instead > >> it only determines whether clk_enable() was called previously and > >> thus the clock _shall_ be enabled. > > > > Nope, you are wrong. > > > > static int clk_gate_is_enabled(struct clk_hw *hw) > > { > > u32 reg; > > struct clk_gate *gate = to_clk_gate(hw); > > > > reg = clk_readl(gate->reg); > > > > /* if a set bit disables this clk, flip it before masking */ > > if (gate->flags & CLK_GATE_SET_TO_DISABLE) > > reg ^= BIT(gate->bit_idx); > > > > reg &= BIT(gate->bit_idx); > > > > return reg ? 1 : 0; > > } > > > > It reads the hardware state. > > > >> AFAIK the kernel's CCF support is "self contained" and does not > >> consider any data or state that was "inherited" from boot staged > >> before the kernel. That's why the "disable unused" step disables > >> everything that wasn't acquired _in the kernel_ regardless of > >> what the boot loader may have done or what is enabled at reset. > > > > Not quite true. It uses the is_enabled(), which gets the real hardware > > state, to turn off clocks which are unused but on. It will not turn > > off clock which are already off. So it is inheriting some state from > > the boot loader, in that it knows if the bootloader turned it > > on. However this is not propagated into prepare/enable status. > > > >>> The other user of the clock is the > >>> ethernet driver, which we know cannot change it yet, because driver > >>> probing has not started yet. > >> > >> I understand that the situation here is, that the ethernet driver > >> hasn't probed yet, but the clock driver did. You are in early setup > >> code and want to (check and) fetch data from the hardware which the > >> ethernet driver later needs. > > > > Nearly, but not quite. If there is an enabled DT node for the device, > > and that node does not have a valid local-mac-address property in the > > node, the bootloader should of programmed the MAC address into the > > device. If it has done that, the clock should be running, because as > > soon as you turn the clock off, it forgets the MAC address. Thus, if > > we find an enabled device in DT, without a valid local-mac-address, > > and the clock is off, we have a bootloader bug, which we want to > > report. > > > >> 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 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? Regards, Mike > > Sebastian
On Sun, Oct 6, 2013 at 3:24 PM, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.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
> 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. Hi Sebastian I agree. As you say, too many people have asked questions or misinterpretation what is happening, so lets go for the simpler method people can understand. I would also suggest in the ethernet driver, maybe set_params(), check if we have a valid MAC address, and if not, issue a warning and call to eth_hw_addr_random(dev) to get a random one. Andrew
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
On 10/06/2013 11:04 PM, Mike Turquette wrote: > On Sun, Oct 6, 2013 at 3:24 PM, Sebastian Hesselbarth > <sebastian.hesselbarth@gmail.com> wrote: >> 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. Well, as you are not the only one misinterpreting the purpose, I guess it is more about the clk_is_enabled() function itself. Uwe was very right, that it will lead to patches using it in a wrong way. Using the common enable/disable functions does no harm to our workaround and we will use it. Thanks for taking the time to raise those questions and surface those critical interpretations early! Sebastian
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a004769..26698d3 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -929,6 +929,24 @@ int clk_enable(struct clk *clk) EXPORT_SYMBOL_GPL(clk_enable); /** + * clk_is_enabled - return the enabled state of a clk + * @clk: the clk whose enabled state gets returned + * + * Returns true if the clock is enabled, false otherwise. + */ +bool clk_is_enabled(struct clk *clk) +{ + bool is_en; + + clk_prepare_lock(); + is_en = __clk_is_enabled(clk); + clk_prepare_unlock(); + + return is_en; +} +EXPORT_SYMBOL_GPL(clk_is_enabled); + +/** * __clk_round_rate - round the given rate for a clk * @clk: round the rate of this clock * @rate: the rate which is to be rounded diff --git a/include/linux/clk.h b/include/linux/clk.h index 9a6d045..8cbf2f7 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -187,6 +187,14 @@ int clk_enable(struct clk *clk); void clk_disable(struct clk *clk); /** + * clk_is_enabled - return the enabled state of a clk + * @clk: the clk whose enabled state gets returned + * + * Returns true if the clock is enabled, false otherwise. + */ +bool clk_is_enabled(struct clk *clk); + +/** * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. * This is only valid once the clock source has been enabled. * @clk: clock source @@ -299,6 +307,11 @@ static inline int clk_enable(struct clk *clk) static inline void clk_disable(struct clk *clk) {} +static inline bool clk_is_enabled(struct clk *clk) +{ + return false; +} + static inline unsigned long clk_get_rate(struct clk *clk) { return 0;
To determine if a clk has been previously enabled, provide a public clk_is_enabled function. This is especially helpful to check the state of clk-gate without actually changing the state of the gate. Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> --- Cc: Mike Turquette <mturquette@linaro.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Grant Likely <grant.likely@linaro.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/clk/clk.c | 18 ++++++++++++++++++ include/linux/clk.h | 13 +++++++++++++ 2 files changed, 31 insertions(+)