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

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.

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.

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.


 drivers/clk/Makefile         |  2 +-
 drivers/clk/clk-fixed-rate.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/clk-fixed-rate.c

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?

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

>
>
>  drivers/clk/Makefile         |  2 +-
>  drivers/clk/clk-fixed-rate.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/clk-fixed-rate.c
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 5fcdf39..75054db 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -5,7 +5,7 @@
>  # SPDX-License-Identifier:      GPL-2.0+
>  #
>
> -obj-$(CONFIG_CLK) += clk-uclass.o
> +obj-$(CONFIG_CLK) += clk-uclass.o clk-fixed-rate.o
>  obj-$(CONFIG_$(SPL_)OF_CONTROL) += clk-fdt.o
>  obj-$(CONFIG_ROCKCHIP_RK3036) += clk_rk3036.o
>  obj-$(CONFIG_ROCKCHIP_RK3288) += clk_rk3288.o
> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> new file mode 100644
> index 0000000..048d450
> --- /dev/null
> +++ b/drivers/clk/clk-fixed-rate.c
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#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()?

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

> +{
> +       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"?

> +       .id = UCLASS_CLK,
> +       .of_match = clk_fixed_rate_match,
> +       .probe = clk_fixed_rate_probe,
> +       .priv_auto_alloc_size = sizeof(struct clk_fixed_rate),
> +       .ops = &clk_fixed_rate_ops,
> +};
> --
> 1.9.1
>

Regards,
Simon

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.

Hi Masahiro,

On 28 December 2015 at 10:08, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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.

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

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

Hi Masahiro,

On 18 January 2016 at 22:15, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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?
>>
>>>
>>> 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@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?

Regards,
Simon

2016-01-20 13:35 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 18 January 2016 at 22:15, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> 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?
>>>
>>>>
>>>> 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@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().