[U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver

[Simon Glass]

Hi Masahiro,

On 28 December 2015 at 10:08, Masahiro Yamada
<yamada.masahiro at socionext.com> wrote:
> Hi Simon,
>
>
> 2015-12-28 23:20 GMT+09:00 Simon Glass <sjg at chromium.org>:
>> Hi Masahiro,
>>
>> On 18 December 2015 at 04:15, Masahiro Yamada
>> <yamada.masahiro at socionext.com> wrote:
>>> This commit intends to implement "fixed-clock" as in Linux.
>>> (drivers/clk/clk-fixed-rate.c in Linux)
>>>
>>> If you need a very simple clock to just provide fixed clock rate
>>> like a crystal oscillator, you do not have to write a new driver.
>>> This driver can support it.
>>>
>>> Note:
>>> As you see in dts/ directories, fixed clocks are often collected in
>>> one container node like this:
>>>
>>>   clocks {
>>>           refclk_a: refclk_a {
>>>                   #clock-cells = <0>;
>>>                   compatible = "fixed-clock";
>>>                   clock-frequency = <10000000>;
>>>           };
>>>
>>>           refclk_b: refclk_b {
>>>                   #clock-cells = <0>;
>>>                   compatible = "fixed-clock";
>>>                   clock-frequency = <20000000>;
>>>           };
>>>   };
>>>
>>> This does not work in the current DM of U-Boot, unfortunately.
>>> The "clocks" node must have 'compatible = "simple-bus";' or something
>>> to bind children.
>>
>> I suppose we could explicitly probe the children of the 'clocks' node
>> somewhere. What do you suggest?
>
> Sounds reasonable.
>
> Or, postpone this until somebody urges to implement it.

I haven't picked this patch up but would like to. Are you able to
respin it? Sorry if you were waiting on me.

Sounds good.

>
>
>>>
>>> Most of developers would be unhappy about adding such a compatible
>>> string only in U-Boot because we generally want to use the same set
>>> of device trees beyond projects.
>>
>> I'm not sure we need to change it, but if we did, we could try to
>> upstream the change.
>
> As you suggest, no change is needed in the core part.
>
>
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>>> ---
>>>
>>> I do not understand why we need both .get_rate and .get_periph_rate.
>>>
>>> I set both in this driver, but I am not sure if I am doing right.
>>
>> This is to avoid needing a new clock device for every single clock
>> divider in the SoC. For example, it is common to have a PLL be used by
>> 20-30 devices. In U-Boot we can encode the device number as a
>> peripheral ID, Then we can adjust those dividers by settings the
>> clock's rate on a per-peripheral basis. Thus we need only one clock
>> device instead of 20-30.
>
> I understand this.  The peripheral ID is useful.
> (I think this is similar to the  of_clk_provider in Linux)
> If so, we can only use .get_periph_rate(), and drop .get_rate().
>
>> In the case of your clock I think you could return -ENOSYS for
>> get_periph_rate().
>
>
> This is "#clock-cells = <0>" case.
>
> Maybe, can we use .get_periph_rate() with index == 0
> for .get_rate() ?
>
> I am not sure if I am understanding correctly...

The idea is that the peripheral clock is a child of the main clock
(e.g. with a clock enable and a divider). So

get_rate(SOME_CLOCK)
get_periph_rate(SOME_CLOCK, SOME_PERIPH)

They are different clocks, but we avoid needing a device for every
peripheral that uses the source clock.

>
>
>
>>> +#include <common.h>
>>> +#include <clk.h>
>>> +#include <dm/device.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +struct clk_fixed_rate {
>>
>> Perhaps have a _priv suffix so it is clear that this is device-private data?
>>
>>> +       unsigned long fixed_rate;
>>> +};
>>> +
>>> +#define to_clk_fixed_rate(dev) ((struct clk_fixed_rate *)dev_get_priv(dev))
>>
>> Should this be to_priv()?
>
> OK, they are local in the file, so name space conflict would never happen.
>
> I took these names from drivers/clk/clk-fixed-rate.c in Linux, though.

OK, it's just different from how it is normally done in U-Boot. I
think consistency is best. But it's a minor issue, I'll leave it up to
you.

>
>
>
>>> +
>>> +static ulong clk_fixed_get_rate(struct udevice *dev)
>>> +{
>>> +       return to_clk_fixed_rate(dev)->fixed_rate;
>>> +}
>>> +
>>> +static ulong clk_fixed_get_periph_rate(struct udevice *dev, int ignored)
>>
>> Don't need this.
>>
>>> +{
>>> +       return to_clk_fixed_rate(dev)->fixed_rate;
>>> +}
>>> +
>>> +const struct clk_ops clk_fixed_rate_ops = {
>>> +       .get_rate = clk_fixed_get_rate,
>>> +       .get_periph_rate = clk_fixed_get_periph_rate,
>>> +       .get_id = clk_get_id_simple,
>>> +};
>>> +
>>> +static int clk_fixed_rate_probe(struct udevice *dev)
>>
>> Could be in the ofdata_to_platdata() method if you like.
>
>
> Right.
>
>>> +{
>>> +       to_clk_fixed_rate(dev)->fixed_rate =
>>> +                               fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>>> +                                              "clock-frequency", 0);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static const struct udevice_id clk_fixed_rate_match[] = {
>>> +       {
>>> +               .compatible = "fixed-clock",
>>> +       },
>>> +       { /* sentinel */ }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(clk_fixed_rate) = {
>>> +       .name = "Fixed Rate Clock",
>>
>> Current we avoid spaces in the names - how about "fixed_rate_clock"?
>
> OK.
>
>
> --
> Best Regards
> Masahiro Yamada


Regards.
Simon