Re: [PATCH v6 3/3] clk: basic clock hardware types

[Rob Herring]

On 03/12/2012 03:58 PM, Turquette, Mike wrote:
> On Mon, Mar 12, 2012 at 1:18 PM, Rob Herring <robherring2@xxxxxxxxx> wrote:
>> On 03/10/2012 01:54 AM, Mike Turquette wrote:
>>> Many platforms support simple gateable clocks, fixed-rate clocks,
>>> adjustable divider clocks and multi-parent multiplexer clocks.
>>>
>>> This patch introduces basic clock types for the above-mentioned hardware
>>> which share some common characteristics.
>>>
>>> Based on original work by Jeremy Kerr and contribution by Jamie Iles.
>>> Dividers and multiplexor clocks originally contributed by Richard Zhao &
>>> Sascha Hauer.
>>>

snip

>>> +static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
>>> +             unsigned long parent_rate)
>>> +{
>>> +     struct clk_divider *divider = to_clk_divider(hw);
>>> +     unsigned int div;
>>> +     unsigned long flags = 0;
>>> +
>>> +     if (divider->lock)
>>> +             spin_lock_irqsave(divider->lock, flags);
>>> +
>>> +     div = readl(divider->reg) >> divider->shift;
>>> +     div &= (1 << divider->width) - 1;
>>> +
>>> +     if (divider->lock)
>>> +             spin_unlock_irqrestore(divider->lock, flags);
>>
>> What are you locking against? You are only reading the register.
> 
> Hi Rob,
> 
> These register-level locks originally came in from the divider &
> multiplexer patches from Richard and Sascha and I'm sure they can give
> you more details than I.
> 
> Basically on their platform they have some 32-bits regs that have a
> lot of overlapping clock functions in them, such as enable/disable and
> adjusting a divider all in one reg.  Those operations are protected by
> different locks (enable spinlock and prepare mutex, respectively) so
> some synchronization mechanism is necessary.  On OMAP we don't use
> this since we have a billion registers that typically only map to one
> clock each.  I also wonder if having device type memory for the
> affected regions makes a this irrelevant on ARM... but that wouldn't
> matter for non-ARM architectures.  Just a thought.
> 

In fact, I pointed out that locking for a register access was needed in
an early version of this series as well. However, locking is only needed
if you are doing a read-mod-write on a field in a shared register or
reading from multiple registers. It makes no sense if you are *only*
reading a single shared register as is the case here. That's already an
atomic operation.

Rob

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