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

[Saravana Kannan]

Hi Jeremy,

Sorry, if the formatting is weird. Using webmail client

On Mon, February 14, 2011 11:26 pm, Jeremy Kerr wrote:
>> > We
>> > may even want to disallow set_rate (and set_parent) when prepare_count
>> is
>> > 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.


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