[U-Boot] [RESEND PATCH 0/5] clk: some fixes, device tree support, new features
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
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);
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
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 {
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
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_ */
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
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.
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_ */
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
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
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.
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
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)
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
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.
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
participants (2)
-
Masahiro Yamada -
Simon Glass