* [U-Boot] [RFC PATCH 1/6] clk: fix comments in include/clk.h
2015-12-18 11:15 [U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features Masahiro Yamada
@ 2015-12-18 11:15 ` Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 2/6] clk: add needed include and declaration to include/clk.h Masahiro Yamada
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Masahiro Yamada @ 2015-12-18 11:15 UTC (permalink / raw)
To: u-boot
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);
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 2/6] clk: add needed include and declaration to include/clk.h
2015-12-18 11:15 [U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 1/6] clk: fix comments in include/clk.h Masahiro Yamada
@ 2015-12-18 11:15 ` Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 3/6] clk: add function to get peripheral ID Masahiro Yamada
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Masahiro Yamada @ 2015-12-18 11:15 UTC (permalink / raw)
To: u-boot
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 {
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 3/6] clk: add function to get peripheral ID
2015-12-18 11:15 [U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 1/6] clk: fix comments in include/clk.h Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 2/6] clk: add needed include and declaration to include/clk.h Masahiro Yamada
@ 2015-12-18 11:15 ` Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 4/6] clk: add device tree support for clock framework Masahiro Yamada
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Masahiro Yamada @ 2015-12-18 11:15 UTC (permalink / raw)
To: u-boot
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_ */
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 4/6] clk: add device tree support for clock framework
2015-12-18 11:15 [U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features Masahiro Yamada
` (2 preceding siblings ...)
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 3/6] clk: add function to get peripheral ID Masahiro Yamada
@ 2015-12-18 11:15 ` Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 5/6] clk: add enable() callback Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver Masahiro Yamada
5 siblings, 0 replies; 13+ messages in thread
From: Masahiro Yamada @ 2015-12-18 11:15 UTC (permalink / raw)
To: u-boot
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_ */
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 5/6] clk: add enable() callback
2015-12-18 11:15 [U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features Masahiro Yamada
` (3 preceding siblings ...)
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 4/6] clk: add device tree support for clock framework Masahiro Yamada
@ 2015-12-18 11:15 ` Masahiro Yamada
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver Masahiro Yamada
5 siblings, 0 replies; 13+ messages in thread
From: Masahiro Yamada @ 2015-12-18 11:15 UTC (permalink / raw)
To: u-boot
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)
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
2015-12-18 11:15 [U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features Masahiro Yamada
` (4 preceding siblings ...)
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 5/6] clk: add enable() callback Masahiro Yamada
@ 2015-12-18 11:15 ` Masahiro Yamada
2015-12-28 14:20 ` Simon Glass
5 siblings, 1 reply; 13+ messages in thread
From: Masahiro Yamada @ 2015-12-18 11:15 UTC (permalink / raw)
To: u-boot
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,
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
2015-12-18 11:15 ` [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver Masahiro Yamada
@ 2015-12-28 14:20 ` Simon Glass
2015-12-28 17:08 ` Masahiro Yamada
2016-01-19 5:15 ` Masahiro Yamada
0 siblings, 2 replies; 13+ messages in thread
From: Simon Glass @ 2015-12-28 14:20 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
2015-12-28 14:20 ` Simon Glass
@ 2015-12-28 17:08 ` Masahiro Yamada
2016-01-15 13:22 ` Simon Glass
2016-01-19 5:15 ` Masahiro Yamada
1 sibling, 1 reply; 13+ messages in thread
From: Masahiro Yamada @ 2015-12-28 17:08 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
2015-12-28 17:08 ` Masahiro Yamada
@ 2016-01-15 13:22 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2016-01-15 13:22 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
2015-12-28 14:20 ` Simon Glass
2015-12-28 17:08 ` Masahiro Yamada
@ 2016-01-19 5:15 ` Masahiro Yamada
2016-01-20 4:35 ` Simon Glass
1 sibling, 1 reply; 13+ messages in thread
From: Masahiro Yamada @ 2016-01-19 5:15 UTC (permalink / raw)
To: u-boot
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.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
2016-01-19 5:15 ` Masahiro Yamada
@ 2016-01-20 4:35 ` Simon Glass
2016-01-21 16:48 ` Masahiro Yamada
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2016-01-20 4:35 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver
2016-01-20 4:35 ` Simon Glass
@ 2016-01-21 16:48 ` Masahiro Yamada
0 siblings, 0 replies; 13+ messages in thread
From: Masahiro Yamada @ 2016-01-21 16:48 UTC (permalink / raw)
To: u-boot
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().
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 13+ messages in thread