From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Mon, 28 Dec 2015 07:20:58 -0700
Subject: [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
In-Reply-To: <1450437315-26333-7-git-send-email-yamada.masahiro@socionext.com>
References: <1450437315-26333-1-git-send-email-yamada.masahiro@socionext.com>
 <1450437315-26333-7-git-send-email-yamada.masahiro@socionext.com>
Message-ID: <CAPnjgZ1BDdgw1zQUa6iD9SFQpGcjm5yCwzBdBwqqvK8+bix9wg@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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