[U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features
Masahiro Yamada (6): clk: fix comments in include/clk.h clk: add needed include and declaration to include/clk.h clk: add function to get peripheral ID clk: add device tree support for clock framework clk: add enable() callback clk: add fixed rate clock driver
drivers/clk/Makefile | 3 +- drivers/clk/clk-fdt.c | 37 +++++++++++++++++++ drivers/clk/clk-fixed-rate.c | 58 +++++++++++++++++++++++++++++ drivers/clk/clk-uclass.c | 10 +++++ include/clk.h | 87 +++++++++++++++++++++++++++++++++++++++----- 5 files changed, 184 insertions(+), 11 deletions(-) create mode 100644 drivers/clk/clk-fdt.c create mode 100644 drivers/clk/clk-fixed-rate.c
The comment about get_periph_rate() is the same as that of set_periph_rate().
I am fixing typos here and there while I am in this file.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
include/clk.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/include/clk.h b/include/clk.h index 254ad2b..f244301 100644 --- a/include/clk.h +++ b/include/clk.h @@ -29,19 +29,19 @@ struct clk_ops { ulong (*set_rate)(struct udevice *dev, ulong rate);
/** - * clk_set_periph_rate() - Set clock rate for a peripheral - * - * @dev: Device to adjust (UCLASS_CLK) - * @rate: New clock rate in Hz - * @return new clock rate in Hz, or -ve error code - */ + * get_periph_rate() - Get clock rate for a peripheral + * + * @dev: Device to check (UCLASS_CLK) + * @periph: Peripheral ID to check + * @return clock rate in Hz, or -ve error code + */ ulong (*get_periph_rate)(struct udevice *dev, int periph);
/** - * clk_set_periph_rate() - Set current clock rate for a peripheral + * set_periph_rate() - Set current clock rate for a peripheral * * @dev: Device to update (UCLASS_CLK) - * @periph: Peripheral ID to cupdate + * @periph: Peripheral ID to update * @return new clock rate in Hz, or -ve error code */ ulong (*set_periph_rate)(struct udevice *dev, int periph, ulong rate); @@ -58,7 +58,7 @@ struct clk_ops { ulong clk_get_rate(struct udevice *dev);
/** - * set_rate() - Set current clock rate + * clk_set_rate() - Set current clock rate * * @dev: Device to adjust * @rate: New clock rate in Hz @@ -78,7 +78,7 @@ ulong clk_get_periph_rate(struct udevice *dev, int periph); * clk_set_periph_rate() - Set current clock rate for a peripheral * * @dev: Device to update (UCLASS_CLK) - * @periph: Peripheral ID to cupdate + * @periph: Peripheral ID to update * @return new clock rate in Hz, or -ve error code */ ulong clk_set_periph_rate(struct udevice *dev, int periph, ulong rate);
This header uses ulong, so it needs to include <linux/types.h>. Likewise, "struct udevice" must be declared before it is used.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
include/clk.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/clk.h b/include/clk.h index f244301..371784a 100644 --- a/include/clk.h +++ b/include/clk.h @@ -8,6 +8,10 @@ #ifndef _CLK_H_ #define _CLK_H_
+#include <linux/types.h> + +struct udevice; + int soc_clk_dump(void);
struct clk_ops {
Currently, this framework does not provide the systematic way to get the peripheral ID (clock index).
I assume that the functions added by this commit are mainly used to get the ID from "clocks" properties in device trees although they are not limited to that use.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/clk/clk-uclass.c | 10 ++++++++++ include/clk.h | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 73dfd7d..8078b0f 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -52,6 +52,16 @@ ulong clk_set_periph_rate(struct udevice *dev, int periph, ulong rate) return ops->set_periph_rate(dev, periph, rate); }
+int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) +{ + struct clk_ops *ops = clk_get_ops(dev); + + if (!ops->get_id) + return -ENOSYS; + + return ops->get_id(dev, args_count, args); +} + UCLASS_DRIVER(clk) = { .id = UCLASS_CLK, .name = "clk", diff --git a/include/clk.h b/include/clk.h index 371784a..1efbaf2 100644 --- a/include/clk.h +++ b/include/clk.h @@ -49,6 +49,16 @@ struct clk_ops { * @return new clock rate in Hz, or -ve error code */ ulong (*set_periph_rate)(struct udevice *dev, int periph, ulong rate); + + /** + * get_id() - Get peripheral ID + * + * @dev: clock provider + * @args_count: number of arguments + * @args: arguments. The meaning is driver specific. + * @return peripheral ID, or -ve error code + */ + int (*get_id)(struct udevice *dev, int args_count, uint32_t *args); };
#define clk_get_ops(dev) ((struct clk_ops *)(dev)->driver->ops) @@ -87,4 +97,28 @@ ulong clk_get_periph_rate(struct udevice *dev, int periph); */ ulong clk_set_periph_rate(struct udevice *dev, int periph, ulong rate);
+/** + * clk_get_id() - Get peripheral ID + * + * @dev: clock provider + * @args_count: number of arguments + * @args: arguments. The meaning is driver specific. + * @return peripheral ID, or -ve error code + */ +int clk_get_id(struct udevice *dev, int args_count, uint32_t *args); + +/** + * clk_get_id_simple() - Simple implementation of get_id() callback + * + * @dev: clock provider + * @args_count: number of arguments + * @args: arguments. + * @return peripheral ID, or -ve error code + */ +static inline int clk_get_id_simple(struct udevice *dev, int args_count, + uint32_t *args) +{ + return args_count > 0 ? args[0] : 0; +} + #endif /* _CLK_H_ */
Add device tree binding support for the clock uclass. This allows clock consumers to get the peripheral ID based on the "clocks" property in the device tree.
Usage: Assume the following device tree:
clk: myclock { compatible = "myclocktype"; #clock-cells = <1>; };
uart { compatible = "myuart"; clocks = <&clk 3>; };
i2c { compatible = "myi2c"; clocks = <&clk 5>; };
The UART, I2C driver can get the peripheral ID 3, 5, respectively by calling fdt_clk_get(). The clock provider should set its get_id callback to clk_get_id_simple. This should be enough for most cases although more complicated DT-PeripheralID translation would be possible by a specific get_id callback.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
drivers/clk/Makefile | 1 + drivers/clk/clk-fdt.c | 37 +++++++++++++++++++++++++++++++++++++ include/clk.h | 20 ++++++++++++++++++++ 3 files changed, 58 insertions(+) create mode 100644 drivers/clk/clk-fdt.c
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 4a6a4a8..5fcdf39 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -6,6 +6,7 @@ #
obj-$(CONFIG_CLK) += clk-uclass.o +obj-$(CONFIG_$(SPL_)OF_CONTROL) += clk-fdt.o obj-$(CONFIG_ROCKCHIP_RK3036) += clk_rk3036.o obj-$(CONFIG_ROCKCHIP_RK3288) += clk_rk3288.o obj-$(CONFIG_SANDBOX) += clk_sandbox.o diff --git a/drivers/clk/clk-fdt.c b/drivers/clk/clk-fdt.c new file mode 100644 index 0000000..fc53157 --- /dev/null +++ b/drivers/clk/clk-fdt.c @@ -0,0 +1,37 @@ +/* + * Copyright (C) 2015 Masahiro Yamada yamada.masahiro@socionext.com + * + * Device Tree support for clk uclass + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <clk.h> +#include <dm/uclass.h> +#include <fdtdec.h> + +int fdt_clk_get(const void *fdt, int nodeoffset, int index, + struct udevice **dev) +{ + struct fdtdec_phandle_args clkspec; + struct udevice *clkdev; + int rc; + + rc = fdtdec_parse_phandle_with_args(fdt, nodeoffset, "clocks", + "#clock-cells", 0, index, &clkspec); + if (rc) + return rc; + + rc = uclass_get_device_by_of_offset(UCLASS_CLK, clkspec.node, &clkdev); + if (rc) + return rc; + + rc = clk_get_id(clkdev, clkspec.args_count, clkspec.args); + if (rc < 0) + return rc; + + if (dev) + *dev = clkdev; + + return rc; +} diff --git a/include/clk.h b/include/clk.h index 1efbaf2..518cb47 100644 --- a/include/clk.h +++ b/include/clk.h @@ -121,4 +121,24 @@ static inline int clk_get_id_simple(struct udevice *dev, int args_count, return args_count > 0 ? args[0] : 0; }
+#if CONFIG_IS_ENABLED(OF_CONTROL) +/** + * fdt_clk_get() - Get peripheral ID from device tree + * + * @fdt: FDT blob + * @periph: Offset of clock consumer node + * @index: index of a phandle to parse out in "clocks" property + * @dev: if not NULL, filled with pointer of clock provider + * @return peripheral ID, or -ve error code + */ +int fdt_clk_get(const void *fdt, int nodeoffset, int index, + struct udevice **dev); +#else +static inline int fdt_clk_get(const void *fdt, int nodeoffset, int index, + struct udevice **dev); +{ + return -ENOSYS; +} +#endif + #endif /* _CLK_H_ */
The most basic thing for clock is to enable it, but it is missing in this uclass.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
include/clk.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/include/clk.h b/include/clk.h index 518cb47..ce2db41 100644 --- a/include/clk.h +++ b/include/clk.h @@ -33,6 +33,15 @@ struct clk_ops { ulong (*set_rate)(struct udevice *dev, ulong rate);
/** + * enable() - Enable the clock for a peripheral + * + * @dev: clock provider + * @periph: Peripheral ID to enable + * @return zero on success, or -ve error code + */ + int (*enable)(struct udevice *dev, int periph); + + /** * get_periph_rate() - Get clock rate for a peripheral * * @dev: Device to check (UCLASS_CLK)
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
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 { + unsigned long fixed_rate; +}; + +#define to_clk_fixed_rate(dev) ((struct clk_fixed_rate *)dev_get_priv(dev)) + +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) +{ + 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) +{ + 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", + .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, +};
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().
participants (2)
-
Masahiro Yamada -
Simon Glass