[U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
Simon Glass
sjg at chromium.org
Mon Dec 28 15:20:58 CET 2015
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().
>
>
> 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 at 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
More information about the U-Boot
mailing list