[PATCH v4] GPIO PL061: Adding Clk framework support

[viresh.kumar@st.com (Viresh KUMAR)]

On 7/13/2010 11:56 PM, Russell King - ARM Linux wrote:
> On Tue, Jul 13, 2010 at 01:00:51PM +0200, Linus Walleij wrote:
>> Since that works, it means that we can likely insert
>> amba_bus_clk_[disable|enable] at the same sites that we have
>> this in the current code for the external clocks at the same time,
>> atleast for pl011, pl022 and pl180.
> 
> Almost - but there's a few corner cases.  Basically, what I think you'll
> need for pl011 is:
> 1. add an amba_bus_clk_disable() at the end of the successful probe
>    function
> 2. add amba_bus_clk_enable() at the beginning of the remove function.
> 3. add amba_bus_clk_enable()/amba_bus_clk_disable() around each of the
>    suspend(), resume(), pl011_console_write(), and
>    pl011_console_get_options() functions.
> 4. amba_bus_clk_enable() at the start of the startup method.
> 5. amba_bus_clk_disable() at the end of the shutdown method.
> 
> Most of these do tie up with existing clk_enable()s, but there are some
> additional places you'd need to enable the pclk because of register
> accesses.
> 

Russell,

These cases particularly in our architecture where functional and
interface clock are tied together (through same gating option) would
produce difficult scenarios (as already mentioned by you) and which
would be difficult and not so clean to handle.

For example just looking at driver src which disables bus clock (without
enabling it) in different scenarios would not be very readable.
Further that would vary from architecture to architecture (even for
standard drivers).

>> > For drivers that need to conserve power, also the block
>> > clock has to be gated when the device is not in use,
>> > sometimes since that affects some hardware that keep
>> > track of the usecount of the clock tree for a larger region of
>> > silicon, so the power savings can be drastic.
> You can't have the core code doing that.  If you unconditionally turn
> the bus clock off after probe, what happens when a driver receives an
> interrupt and tries to access its registers?
> 
> Hint: the core code can't know that the driver has registered an IRQ
> handler.

We haven't seen this kind of issues in our devices, SPEAr as well as
U300 (as we have both clocks controlled by same bit). Normally, when device
is not in use then interrupts are disabled. When device is used then interrupts
and clock are enabled and clocks are not disabled till the time work is
finished. So, this condition might not occur that you have landed in interrupt
handler with clocks off.

IMO, Clocks enabling is required at following places:
1. During device add where we need to read id registers.
- We can do this by simple enabling/disabling "apb_bus" clk before and
  after reading registers.
2. In driver, when device is used.
- This can be handled best by the driver itself. We don't require to
  enable interface clock from amba_probe for ever.


One way to do this cleanly is:
- Add clock structures for devices with connection id "apb_bus".
  (Platforms can have 2 different structures if they have different
controlling bits otherwise single structure will solve the purpose.)
- amba_device_register can simple get this clk and use it.
- Drivers need to enable/disable both interface and functional clock to
save maximum power.



regards,
viresh kumar.