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

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

[Masahiro Yamada]

This series contains the same commits
as 01-05 of the RFC series.

Since somebody immediately marked it as RFC
(i.e. dropped from the TODO items in the patchwork),
nothing has happened to it.

I am resending 01-05 without RFC prefix
in order to leave this series in the TODO list
until appropriate action is taken.

As you see, 01 and 02 are apparent fixes
which can be applied soon.



Masahiro Yamada (5):
  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

 drivers/clk/Makefile     |  1 +
 drivers/clk/clk-fdt.c    | 37 ++++++++++++++++++++
 drivers/clk/clk-uclass.c | 10 ++++++
 include/clk.h            | 87 ++++++++++++++++++++++++++++++++++++++++++------
 4 files changed, 125 insertions(+), 10 deletions(-)
 create mode 100644 drivers/clk/clk-fdt.c

-- 
1.9.1

[U-Boot] [RESEND PATCH 1/5] 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] [RESEND PATCH 1/5] clk: fix comments in include/clk.h

[Simon Glass]

On 22 December 2015 at 03:04, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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(-)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [RESEND PATCH 2/5] 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] [RESEND PATCH 2/5] clk: add needed include and declaration to include/clk.h

[Simon Glass]

On 22 December 2015 at 03:04, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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(+)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [RESEND PATCH 3/5] 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] [RESEND PATCH 3/5] clk: add function to get peripheral ID

[Simon Glass]

Hi Masahiro,

On 22 December 2015 at 03:04, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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);

What are the arguments for? Who will use them? I wonder if the simple
case you have is the only useful case for now?

I think it would be better for the device tree parsing to happen
within the uclass rather than requiring the caller to understand how
it works. Here, we must obtain the arguments after the phandle and
pass them to this function.

>  };
>
>  #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
>

Regards,
Simon

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

[Masahiro Yamada]

Hi Simon,


2015-12-28 23:20 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> 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);
>
> What are the arguments for? Who will use them? I wonder if the simple
> case you have is the only useful case for now?
>
> I think it would be better for the device tree parsing to happen
> within the uclass rather than requiring the caller to understand how
> it works. Here, we must obtain the arguments after the phandle and
> pass them to this function.

My intention was to make the interface as generic as possible.

It is true that most of drivers have '#clock-cells = <1>',
but I found some have '#clock-cells = <2>'.

Under linux/Documentation:

devicetree/bindings/clock/qoriq-clock.txt:When the clockgen node is a
clock provider, #clock-cells = <2>.
devicetree/bindings/clock/qoriq-clock.txt:              #clock-cells = <2>;
devicetree/bindings/clock/renesas,cpg-mssr.txt:         #clock-cells = <2>;
devicetree/bindings/clock/st,stm32-rcc.txt:             #clock-cells = <2>
devicetree/bindings/clock/ux500.txt:            #clock-cells = <2>;
devicetree/bindings/clock/ux500.txt:            #clock-cells = <2>;


So, my idea was to cover the simple case with clk_get_id_simple(),
but still allow such rare-case drivers to have their own .get_id() callback.

However, U-Boot might never be hit by such a case, and your simple one
meets our demand well enough.



-- 
Best Regards
Masahiro Yamada

[U-Boot] [RESEND PATCH 4/5] 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] [RESEND PATCH 4/5] clk: add device tree support for clock framework

[Simon Glass]

Hi Masahiro,

On 22 December 2015 at 03:04, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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 +++++++++++++++++++++++++++++++++++++

I think clk_fdt.c is better since we mostly avoid hyphens except for the uclass.

>  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)

I think this should work using a device rather than a node offset.
I've pushed a working tree to u-boot-dm/rockchip-working to show what
I mean.

Also BTW I implemented your full pinctrl for rockchip in that tree -
seems to work well! The only problem is that init is quite slow. It
might be the phandle lookups, I'm not sure.

> +{
> +       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
>

Regards,
Simon

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

[Simon Glass]

Hi Masahiro,

On 27 December 2015 at 21:23, Simon Glass <sjg@chromium.org> wrote:
> Hi Masahiro,
>
> On 22 December 2015 at 03:04, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> 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 +++++++++++++++++++++++++++++++++++++
>
> I think clk_fdt.c is better since we mostly avoid hyphens except for the uclass.
>
>>  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)
>
> I think this should work using a device rather than a node offset.
> I've pushed a working tree to u-boot-dm/rockchip-working to show what
> I mean.
>
> Also BTW I implemented your full pinctrl for rockchip in that tree -
> seems to work well! The only problem is that init is quite slow. It
> might be the phandle lookups, I'm not sure.
>
>> +{
>> +       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);

With a bit more thought, I think the concerns I have are mostly
cosmetic. This function should be passed a struct udevice rather than
fdt and nodeoffset, since it can get them itself. Also I'm not keen on
the fdt_ prefix. Other than that it is similar to the
clk_get_by_index() that I was fiddling with for rockchip. I think it
is better to return the periph_id as a return value (as you have)
rather than a pointer arg (as I did).

>> +{
>> +       return -ENOSYS;
>> +}
>> +#endif
>> +
>>  #endif /* _CLK_H_ */
>> --
>> 1.9.1
>>
>
> Regards,
> Simon

Regards,
Simon

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

[Masahiro Yamada]

Hi Simon,


2015-12-28 23:20 GMT+09:00 Simon Glass <sjg@chromium.org>:

>>>  drivers/clk/clk-fdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>
>> I think clk_fdt.c is better since we mostly avoid hyphens except for the uclass.

OK.


>>>  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)
>>
>> I think this should work using a device rather than a node offset.
>> I've pushed a working tree to u-boot-dm/rockchip-working to show what
>> I mean.

Looks good to me.



>> Also BTW I implemented your full pinctrl for rockchip in that tree -
>> seems to work well!

That's good to hear!

>> The only problem is that init is quite slow. It
>> might be the phandle lookups, I'm not sure.

I did not realize the slowness, at least on my board.
I enable D-cache on both SPL and U-Boot proper.



>>> +{
>>> +       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);
>
> With a bit more thought, I think the concerns I have are mostly
> cosmetic. This function should be passed a struct udevice rather than
> fdt and nodeoffset, since it can get them itself. Also I'm not keen on
> the fdt_ prefix. Other than that it is similar to the
> clk_get_by_index() that I was fiddling with for rockchip. I think it
> is better to return the periph_id as a return value (as you have)
> rather than a pointer arg (as I did).


My main motivation is to use device trees to get clocks, so I do not mind
cosmetic stuff here.

Yours is simpler and works for me too.
So,  shall we replace 03 and 04 with the one in your rockchip branch?
(with modification of the return value)

If you like, please submit the patch.  I am glad to test it.


-- 
Best Regards
Masahiro Yamada

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

[Simon Glass]

Hi Masahiro,

On 28 December 2015 at 10:06, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Hi Simon,
>
>
> 2015-12-28 23:20 GMT+09:00 Simon Glass <sjg@chromium.org>:
>
>>>>  drivers/clk/clk-fdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>>
>>> I think clk_fdt.c is better since we mostly avoid hyphens except for the uclass.
>
> OK.
>
>
>>>>  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)
>>>
>>> I think this should work using a device rather than a node offset.
>>> I've pushed a working tree to u-boot-dm/rockchip-working to show what
>>> I mean.
>
> Looks good to me.
>
>
>
>>> Also BTW I implemented your full pinctrl for rockchip in that tree -
>>> seems to work well!
>
> That's good to hear!
>
>>> The only problem is that init is quite slow. It
>>> might be the phandle lookups, I'm not sure.
>
> I did not realize the slowness, at least on my board.
> I enable D-cache on both SPL and U-Boot proper.

Yes, and Rockchip should do the same, really. But I think it is worth
digging into. I end up with quite a lot of pinconfig devices (maybe
50). I'll see if I can figure it out in the next few weeks.

>
>
>
>>>> +{
>>>> +       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);
>>
>> With a bit more thought, I think the concerns I have are mostly
>> cosmetic. This function should be passed a struct udevice rather than
>> fdt and nodeoffset, since it can get them itself. Also I'm not keen on
>> the fdt_ prefix. Other than that it is similar to the
>> clk_get_by_index() that I was fiddling with for rockchip. I think it
>> is better to return the periph_id as a return value (as you have)
>> rather than a pointer arg (as I did).
>
>
> My main motivation is to use device trees to get clocks, so I do not mind
> cosmetic stuff here.
>
> Yours is simpler and works for me too.
> So,  shall we replace 03 and 04 with the one in your rockchip branch?
> (with modification of the return value)
>
> If you like, please submit the patch.  I am glad to test it.

OK I did that, but we need to sort out the return value. See my email
on the patch.

Regards,
Simon

[U-Boot] [RESEND PATCH 5/5] 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] [RESEND PATCH 5/5] clk: add enable() callback

[Simon Glass]

Hi Masahiro,

On 22 December 2015 at 03:04, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 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(+)

Acked-by: Simon Glass <sjg@chromium.org>

Thinking ahead, should we have disable() also, or maybe replacing both
with set_enable(bool enable) would be better?

>
> 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
>

Regards,
Simon

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

[Masahiro Yamada]

Hi Simon,

2015-12-28 23:20 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 22 December 2015 at 03:04, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> 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(+)
>
> Acked-by: Simon Glass <sjg@chromium.org>
>
> Thinking ahead, should we have disable() also, or maybe replacing both
> with set_enable(bool enable) would be better?

Yes, we'd better to implement disable() just in case.

We mostly just enable clocks, and  never disable them in a boot loader, though.


-- 
Best Regards
Masahiro Yamada

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

[Simon Glass]

Hi Masahiro,

On 22 December 2015 at 03:04, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> This series contains the same commits
> as 01-05 of the RFC series.
>
> Since somebody immediately marked it as RFC
> (i.e. dropped from the TODO items in the patchwork),
> nothing has happened to it.
>
> I am resending 01-05 without RFC prefix
> in order to leave this series in the TODO list
> until appropriate action is taken.
>
> As you see, 01 and 02 are apparent fixes
> which can be applied soon.

I noticed this a few days ago and applied it locally. I did manage to
find it in patchwork in the end. I have a few review comments so will
tidy those up and send them through...

>
>
>
> Masahiro Yamada (5):
>   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
>
>  drivers/clk/Makefile     |  1 +
>  drivers/clk/clk-fdt.c    | 37 ++++++++++++++++++++
>  drivers/clk/clk-uclass.c | 10 ++++++
>  include/clk.h            | 87 ++++++++++++++++++++++++++++++++++++++++++------
>  4 files changed, 125 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/clk/clk-fdt.c
>
> --
> 1.9.1
>

Regards,
Simon