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

[Masahiro Yamada]

2016-01-20 13:35 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 18 January 2016 at 22:15, Masahiro Yamada
> <yamada.masahiro at socionext.com> wrote:
>> 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?
>>>
>>>>
>>>> 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.
>>>
>>>>
>>>> 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.
>>>
>>> In the case of your clock I think you could return -ENOSYS for
>>> get_periph_rate().
>>
>> I've just posted v2.
>>
>> I am still keeping both .get_rate() and .get_periph_rate().
>>
>> If I follow your suggestion, each clock consumer must know the
>> detail of its clock providers to choose the correct one,
>> either .get_rate() or .get_periph_rate().
>>
>> Or do you want drivers to do like this?
>>
>>
>>
>>     clock_cells = clk_get_nr_cells(...);
>>
>>     if (clock_cells == 0)
>>           rate = clk_get_rate(...);
>>     else
>>           rate = clk_get_periph_rate(...);
>>
>>
>>
>> For the proper use of these two, the details of clocks must be
>> hard-coded in drivers.
>> They, of course should be describe in device tree and clock providers.
>
> In general drivers don't use PLLs directly. So I doubt this case will
> arise. Do you have an example?
>

No, I don't.

You commented "In the case of your clock I think you could return -ENOSYS for
get_periph_rate().", so I just imagined there is a case where drivers
call clk_get_rate(), not clk_get_periph_rate().



-- 
Best Regards
Masahiro Yamada