Re: [PATCH 05/11] ST SPEAr: Added clock framework for SPEAr platform and machines

[Viresh KUMAR]

On 3/11/2010 12:30 PM, Linus Walleij wrote:
> 2010/3/3 Viresh KUMAR <viresh.kumar@xxxxxx>:
> 
>> +{
>> +       unsigned int val;
>> +
>> +       if (!clk->en_reg)
>> +               return -EFAULT;
>> +
>> +       val = readl(clk->en_reg);
>> +       if (unlikely(clk->flags & RESET_TO_ENABLE))
>> +               val &= ~(1 << clk->en_reg_bit);
>> +       else
>> +               val |= 1 << clk->en_reg_bit;
>> +       writel(val, clk->en_reg);
> 
> I don't understand one bit of this. What happens if the RESET_TO_ENABLE
> flag is set for the clock? The exact same bit is &-masked and then
> immediately |:ed to 1 again. Then it is written to the register. Practical
> effect: absolutely none.
> 
> Is there a writel(val, clk->en_reg); missing from the unlikely() execution
> path?

Already explained by shiraz.

> 
>> +
>> +       return 0;
>> +}
>> +
>> +static void generic_clk_disable(struct clk *clk)
>> +{
>> +       unsigned int val;
>> +
>> +       if (!clk->en_reg)
>> +               return;
>> +
>> +       val = readl(clk->en_reg);
>> +       if (unlikely(clk->flags & RESET_TO_ENABLE))
>> +               val |= 1 << clk->en_reg_bit;
>> +       else
>> +               val &= ~(1 << clk->en_reg_bit);
> 
> Same issue here...
> 
>> +
>> +       writel(val, clk->en_reg);
>> +}
>> +
>> +/* generic clk ops */
>> +static struct clkops generic_clkops = {
>> +       .enable = generic_clk_enable,
>> +       .disable = generic_clk_disable,
>> +};
>> +
>> +/*
>> + * clk_enable - inform the system when the clock source should be running.
>> + * @clk: clock source
>> + *
>> + * If the clock can not be enabled/disabled, this should return success.
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_enable(struct clk *clk)
>> +{
>> +       unsigned long flags;
>> +       int ret = 0;
>> +
>> +       if (!clk || IS_ERR(clk))
>> +               return -EFAULT;
>> +
>> +       spin_lock_irqsave(&clocks_lock, flags);
>> +       if (clk->usage_count++ == 0) {
> 
> Isnt if (++clk.>usage_count == 1) easier to understand, or is it just me?
> BTW doing this:
> clk->usage_count++;
> if (clk->usage_count == 1)
> will not use more memory, the compiler optimize this, so choose the
> version you think is most readable. If you think this is very readable, by
> all means keep it.
> 

I will simplify it, to make it more readable.

>> +               if (clk->ops && clk->ops->enable)
>> +                       ret = clk->ops->enable(clk);
>> +       }
>> +       spin_unlock_irqrestore(&clocks_lock, flags);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(clk_enable);
>> +
>> +/*
>> + * clk_disable - inform the system when the clock source is no longer required.
>> + * @clk: clock source
>> + *
>> + * Inform the system that a clock source is no longer required by
>> + * a driver and may be shut down.
>> + *
>> + * Implementation detail: if the clock source is shared between
>> + * multiple drivers, clk_enable() calls must be balanced by the
>> + * same number of clk_disable() calls for the clock source to be
>> + * disabled.
>> + */
>> +void clk_disable(struct clk *clk)
>> +{
>> +       unsigned long flags;
>> +
>> +       if (!clk || IS_ERR(clk))
>> +               return;
>> +
>> +       WARN_ON(clk->usage_count == 0);
>> +
>> +       spin_lock_irqsave(&clocks_lock, flags);
>> +       if (--clk->usage_count == 0) {
> 
> Same readability issue here.

I will simplify it, to make it more readable.

> 
>> +       if (!clk || IS_ERR(clk) || !parent || IS_ERR(parent))
>> +               return -EFAULT;
>> +       if (clk->usage_count == 0)
>> +               return -EBUSY;
> 
> Why will the clk_set_parent() call fail if there are *no* users of the clock?
> Should it not be the other way around? Or what am I misunderstanding here?
> 

My mistake. should be !=.

>> +       if (!clk->pclk_sel)
>> +               return -EPERM;
>> +       if (clk->pclk == parent)
>> +               return 0;
>> +
>> +       for (i = 0; i < clk->pclk_sel->pclk_count; i++) {
>> +               if (clk->pclk_sel->pclk_info[i].pclk == parent) {
>> +                       found = 1;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (!found)
>> +               return -EPERM;
> 
> What about -ENODEV or so? (I don't know what people typically
> use for clocks that don't exist.)

Will correct it to return correct error.

> 
>> + */
>> +void recalc_root_clocks(void)
>> +{
>> +       propagate_rate(&root_clks);
>> +}
> 
> I understand (I think) how speed change can propagate through the clocks.
> However I think it will be hard to notify users that the clock rate has changed,
> because there is nothing in the clk framework that supports that. If you have
> drivers with dynamic clocks, do you have any plans on how you will
> notify drivers?
> 
> OMAP uses CPUfreq but that is really about the CPU. As it happens, all
> their clk:s always change frequency at the same operating points as the
> CPU. So they can have pre/post calls from CPUfreq in their code, but
> this will not work with things like PrimeCells where other users of the cell
> may not have operating points correlated with CPU operating points.
> 
> (I'm not requesting you to solve this problem, more to be aware of it.)
> 

already answered by shiraz.

>> diff --git a/arch/arm/plat-spear/include/plat/clkdev.h b/arch/arm/plat-spear/include/plat/clkdev.h
>> +/* clk values */
>> +#define KHZ                    (1000)
>> +#define MHZ                    (1000000)
> 
> This looks far to generic to be hidden in some weird special architecture.
> And I *think* the preferred way to encode frequencies in the kernel is raw
> Hertz measure with all the extra zeroes.
> 
> Doing
> .foo = MHZ *48;
> 
> Is a bit awkward, don't you think it's better to do:
> #define MHZ(f)  f * 1000000
> .foo = MHZ(48);
> 
> If you absolutely want to do this, I would suggest to add the KHZ and MHZ
> macros to some global kernel file but I honestly cannot say which one.
> Perhaps inlcude/linux/clk.h?
> 

I will better remove them.


regards,
viresh kumar.
--
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/