Re: [RFC,PATCH 1/3] Add a common struct clk

Hi Jeremy,

Sorry, if the formatting is weird. Using webmail client

On Mon, February 14, 2011 11:26 pm, Jeremy Kerr wrote:

quoted

quoted

We
may even want to disallow set_rate (and set_parent) when prepare_count

is

quoted

non- zero.

This is definitely not right.

Why is that? Consider two devices using one clock; one does some
initialisation based on the return value of clk_get_rate(), the other
calls
clk_set_rate() some time later. Now the first device is incorrectly
initialised.

The case you describe is certainly something I consider as incorrect and
agree with you in that we should try to prevent it. But

(prepare_count != 0) != (two devices using one clock).

For one, prepare_count = 1 would be a normal case when a clock is enabled
and the MSM drivers certainly want to be able to set the rate when the
clock is enabled.

But (prepare_count > 1 || enable_count > 1) doesn't mean more than one
device/device driver using the clock either. A simple example would be a
driver wrapping all it's register accesses with a clk_enable/clk_disable
and not having to worry about if a clock is enabled during register access
when it has multiple execution paths (threads, interrupt handler, timers,
etc) that access registers. The driver would just do the enable/disable
around register accesses and let the clock code's ref counting dealing
with making sure a clock is never turned off when it's needed. In these
case both the prepare_count (less likely, but likely) and enable_count can
greater than 1.

Long story short, I support your desire to prevent one driver from
changing the rate from underneath another driver, but the condition you
chose to figure that out is not accurate.

Regardless, this is definitely something to flag for a later discussion.
I'm
happy to return to that, but we should focus on one issue at a time here.

Sure, this discussion of set rate with count is non-zero can be reserved
for later. But I think the discussion of grabbing the lock during
set_parent should be discussed in the context of this patch.

Waiting to see how others feel about this.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.