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

[Masahiro Yamada <yamada.masahiro@socionext.com>]

Hi Simon,


2015-12-28 23:20 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 18 December 2015 at 04:15, Masahiro Yamada
> <yamada.masahiro@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.


>>
>> 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@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...



>> +#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.



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