[U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features

[U-Boot] [RFC PATCH 0/6] clk: some fixes, device tree support, new features

[Masahiro Yamada]

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

-- 
1.9.1

[U-Boot] [RFC PATCH 1/6] clk: fix comments in include/clk.h

[Masahiro Yamada]

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

[U-Boot] [RFC PATCH 2/6] clk: add needed include and declaration to include/clk.h

[Masahiro Yamada]

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

[U-Boot] [RFC PATCH 3/6] clk: add function to get peripheral ID

[Masahiro Yamada]

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

[U-Boot] [RFC PATCH 4/6] clk: add device tree support for clock framework

[Masahiro Yamada]

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

[U-Boot] [RFC PATCH 5/6] clk: add enable() callback

[Masahiro Yamada]

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

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

[Masahiro Yamada]

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

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

[Simon Glass]

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

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

[Masahiro Yamada]

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

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

[Simon Glass]

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

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

[Masahiro Yamada]

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

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

[Simon Glass]

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

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

[Masahiro Yamada]

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