[U-Boot] [PATCH v2 01/50] dm: clk: Add support for decoding clocks from the device tree
Add a method which can locate a clock for a device, given its index. This uses the normal device tree bindings to return the clock device and the first argument which is normally used as a peripheral ID in U-Boot.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Make the peripheral ID a return value - Add an assert for clk_devp - Make the function dependent on OF_CONTROL
drivers/clk/clk-uclass.c | 28 ++++++++++++++++++++++++++++ include/clk.h | 14 ++++++++++++++ 2 files changed, 42 insertions(+)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 8078b0f..f80beaf 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -12,6 +12,8 @@ #include <dm/lists.h> #include <dm/root.h>
+DECLARE_GLOBAL_DATA_PTR; + ulong clk_get_rate(struct udevice *dev) { struct clk_ops *ops = clk_get_ops(dev); @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) return ops->get_id(dev, args_count, args); }
+#ifdef CONFIG_OF_CONTROL +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp) +{ + struct fdtdec_phandle_args args; + int ret; + + assert(*clk_devp); + ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset, + "clocks", "#clock-cells", 0, index, + &args); + if (ret) { + debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n", + __func__, ret); + return ret; + } + + ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp); + if (ret) { + debug("%s: uclass_get_device_by_of_offset failed: err=%d\n", + __func__, ret); + return ret; + } + return args.args_count > 0 ? args.args[0] : 0; +} +#endif + UCLASS_DRIVER(clk) = { .id = UCLASS_CLK, .name = "clk", diff --git a/include/clk.h b/include/clk.h index ce2db41..0af2824 100644 --- a/include/clk.h +++ b/include/clk.h @@ -150,4 +150,18 @@ static inline int fdt_clk_get(const void *fdt, int nodeoffset, int index, } #endif
+/** + * clk_get_by_index() - look up a clock referenced by a device + * + * Parse a device's 'clocks' list, returning information on the indexed clock, + * ensuring that it is activated. + * + * @dev: Device containing the clock reference + * @index: Clock index to return (0 = first) + * @clk_devp: Returns clock device + * @return: Peripheral ID for the device to control. This is the first + * argument after the clock node phandle. If there is no arguemnt, + * returns 0. Return -ve error code on any error + */ +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp); #endif /* _CLK_H_ */
Hi Simon,
2016-01-19 11:27 GMT+09:00 Simon Glass sjg@chromium.org:
Add a method which can locate a clock for a device, given its index. This uses the normal device tree bindings to return the clock device and the first argument which is normally used as a peripheral ID in U-Boot.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Make the peripheral ID a return value
- Add an assert for clk_devp
- Make the function dependent on OF_CONTROL
drivers/clk/clk-uclass.c | 28 ++++++++++++++++++++++++++++ include/clk.h | 14 ++++++++++++++ 2 files changed, 42 insertions(+)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 8078b0f..f80beaf 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -12,6 +12,8 @@ #include <dm/lists.h> #include <dm/root.h>
+DECLARE_GLOBAL_DATA_PTR;
ulong clk_get_rate(struct udevice *dev) { struct clk_ops *ops = clk_get_ops(dev); @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) return ops->get_id(dev, args_count, args); }
+#ifdef CONFIG_OF_CONTROL
Isn't this #if CONFIG_IS_ENABLED(OF_CONTROL)?
SPL is controlled by CONFIG_SPL_OF_CONTROL.
+int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp) +{
struct fdtdec_phandle_args args;int ret;assert(*clk_devp);ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset,"clocks", "#clock-cells", 0, index,&args);if (ret) {debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n",__func__, ret);return ret;}ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp);if (ret) {debug("%s: uclass_get_device_by_of_offset failed: err=%d\n",__func__, ret);return ret;}return args.args_count > 0 ? args.args[0] : 0;+} +#endif
UCLASS_DRIVER(clk) = { .id = UCLASS_CLK, .name = "clk", diff --git a/include/clk.h b/include/clk.h index ce2db41..0af2824 100644 --- a/include/clk.h +++ b/include/clk.h @@ -150,4 +150,18 @@ static inline int fdt_clk_get(const void *fdt, int nodeoffset, int index, } #endif
+/**
- clk_get_by_index() - look up a clock referenced by a device
- Parse a device's 'clocks' list, returning information on the indexed clock,
- ensuring that it is activated.
- @dev: Device containing the clock reference
- @index: Clock index to return (0 = first)
- @clk_devp: Returns clock device
- @return: Peripheral ID for the device to control. This is the first
argument after the clock node phandle. If there is no arguemnt,
returns 0. Return -ve error code on any error- */
+int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp); #endif /* _CLK_H_ */
I want #ifdef in the header too, like mine http://patchwork.ozlabs.org/patch/566812/
Hi Masahiro,
On 18 January 2016 at 21:29, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
2016-01-19 11:27 GMT+09:00 Simon Glass sjg@chromium.org:
Add a method which can locate a clock for a device, given its index. This uses the normal device tree bindings to return the clock device and the first argument which is normally used as a peripheral ID in U-Boot.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Make the peripheral ID a return value
- Add an assert for clk_devp
- Make the function dependent on OF_CONTROL
drivers/clk/clk-uclass.c | 28 ++++++++++++++++++++++++++++ include/clk.h | 14 ++++++++++++++ 2 files changed, 42 insertions(+)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 8078b0f..f80beaf 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -12,6 +12,8 @@ #include <dm/lists.h> #include <dm/root.h>
+DECLARE_GLOBAL_DATA_PTR;
ulong clk_get_rate(struct udevice *dev) { struct clk_ops *ops = clk_get_ops(dev); @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args) return ops->get_id(dev, args_count, args); }
+#ifdef CONFIG_OF_CONTROL
Isn't this #if CONFIG_IS_ENABLED(OF_CONTROL)?
SPL is controlled by CONFIG_SPL_OF_CONTROL.
OK
+int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp) +{
struct fdtdec_phandle_args args;int ret;assert(*clk_devp);ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset,"clocks", "#clock-cells", 0, index,&args);if (ret) {debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n",__func__, ret);return ret;}ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp);if (ret) {debug("%s: uclass_get_device_by_of_offset failed: err=%d\n",__func__, ret);return ret;}return args.args_count > 0 ? args.args[0] : 0;+} +#endif
UCLASS_DRIVER(clk) = { .id = UCLASS_CLK, .name = "clk", diff --git a/include/clk.h b/include/clk.h index ce2db41..0af2824 100644 --- a/include/clk.h +++ b/include/clk.h @@ -150,4 +150,18 @@ static inline int fdt_clk_get(const void *fdt, int nodeoffset, int index, } #endif
+/**
- clk_get_by_index() - look up a clock referenced by a device
- Parse a device's 'clocks' list, returning information on the indexed clock,
- ensuring that it is activated.
- @dev: Device containing the clock reference
- @index: Clock index to return (0 = first)
- @clk_devp: Returns clock device
- @return: Peripheral ID for the device to control. This is the first
argument after the clock node phandle. If there is no arguemnt,
returns 0. Return -ve error code on any error- */
+int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp); #endif /* _CLK_H_ */
I want #ifdef in the header too, like mine http://patchwork.ozlabs.org/patch/566812/
I am not keen on that idea since it clutters up header files and we'll get a link error anyway if something is missing. Anyway, I've added it.
Regards, Simon
Hi Simon,
+/**
- clk_get_by_index() - look up a clock referenced by a device
- Parse a device's 'clocks' list, returning information on the indexed clock,
- ensuring that it is activated.
- @dev: Device containing the clock reference
- @index: Clock index to return (0 = first)
- @clk_devp: Returns clock device
- @return: Peripheral ID for the device to control. This is the first
argument after the clock node phandle. If there is no arguemnt,
returns 0. Return -ve error code on any error- */
+int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp); #endif /* _CLK_H_ */
I want #ifdef in the header too, like mine http://patchwork.ozlabs.org/patch/566812/
I am not keen on that idea since it clutters up header files and we'll get a link error anyway if something is missing. Anyway, I've added it.
I am afraid there is misunderstanding here.
Please see my patch carefully.
What I mean is like this:
#if ... declaration of function prototype #else static inline empty function #endif
This is a common technique to avoid a link error.
Hi Masahiro,
On 19 January 2016 at 22:38, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
+/**
- clk_get_by_index() - look up a clock referenced by a device
- Parse a device's 'clocks' list, returning information on the indexed clock,
- ensuring that it is activated.
- @dev: Device containing the clock reference
- @index: Clock index to return (0 = first)
- @clk_devp: Returns clock device
- @return: Peripheral ID for the device to control. This is the first
argument after the clock node phandle. If there is no arguemnt,
returns 0. Return -ve error code on any error- */
+int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp); #endif /* _CLK_H_ */
I want #ifdef in the header too, like mine http://patchwork.ozlabs.org/patch/566812/
I am not keen on that idea since it clutters up header files and we'll get a link error anyway if something is missing. Anyway, I've added it.
I am afraid there is misunderstanding here.
Please see my patch carefully.
What I mean is like this:
#if ... declaration of function prototype #else static inline empty function #endif
This is a common technique to avoid a link error.
Do you think this function will be called when device tree is not enabled? I cannot see how it would have any meaning in that case. What is the purpose?
Regards, Simon
Hi Simon,
2016-01-21 0:24 GMT+09:00 Simon Glass sjg@chromium.org:
Hi Masahiro,
On 19 January 2016 at 22:38, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
+/**
- clk_get_by_index() - look up a clock referenced by a device
- Parse a device's 'clocks' list, returning information on the indexed clock,
- ensuring that it is activated.
- @dev: Device containing the clock reference
- @index: Clock index to return (0 = first)
- @clk_devp: Returns clock device
- @return: Peripheral ID for the device to control. This is the first
argument after the clock node phandle. If there is no arguemnt,
returns 0. Return -ve error code on any error- */
+int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp); #endif /* _CLK_H_ */
I want #ifdef in the header too, like mine http://patchwork.ozlabs.org/patch/566812/
I am not keen on that idea since it clutters up header files and we'll get a link error anyway if something is missing. Anyway, I've added it.
I am afraid there is misunderstanding here.
Please see my patch carefully.
What I mean is like this:
#if ... declaration of function prototype #else static inline empty function #endif
This is a common technique to avoid a link error.
Do you think this function will be called when device tree is not enabled? I cannot see how it would have any meaning in that case. What is the purpose?
In order to avoid build errors.
I think this coding style is often used.
For example, you can see include/linux/clk.h in Linux.
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) struct clk *of_clk_get(struct device_node *np, int index); struct clk *of_clk_get_by_name(struct device_node *np, const char *name); struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec); #else static inline struct clk *of_clk_get(struct device_node *np, int index) { return ERR_PTR(-ENOENT); } static inline struct clk *of_clk_get_by_name(struct device_node *np, const char *name) { return ERR_PTR(-ENOENT); } #endif
If every driver entry in Kconfig specifies "depends on OF_CONTROL" correctly, no link error would happen for this.
So, I leave this decision to you. If you do not like #ifdef in header files, just rip it off.
Hi Masahiro,
On 20 January 2016 at 19:26, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
2016-01-21 0:24 GMT+09:00 Simon Glass sjg@chromium.org:
Hi Masahiro,
On 19 January 2016 at 22:38, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
+/**
- clk_get_by_index() - look up a clock referenced by a device
- Parse a device's 'clocks' list, returning information on the indexed clock,
- ensuring that it is activated.
- @dev: Device containing the clock reference
- @index: Clock index to return (0 = first)
- @clk_devp: Returns clock device
- @return: Peripheral ID for the device to control. This is the first
argument after the clock node phandle. If there is no arguemnt,
returns 0. Return -ve error code on any error- */
+int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp); #endif /* _CLK_H_ */
I want #ifdef in the header too, like mine http://patchwork.ozlabs.org/patch/566812/
I am not keen on that idea since it clutters up header files and we'll get a link error anyway if something is missing. Anyway, I've added it.
I am afraid there is misunderstanding here.
Please see my patch carefully.
What I mean is like this:
#if ... declaration of function prototype #else static inline empty function #endif
This is a common technique to avoid a link error.
Do you think this function will be called when device tree is not enabled? I cannot see how it would have any meaning in that case. What is the purpose?
In order to avoid build errors.
I think this coding style is often used.
For example, you can see include/linux/clk.h in Linux.
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) struct clk *of_clk_get(struct device_node *np, int index); struct clk *of_clk_get_by_name(struct device_node *np, const char *name); struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec); #else static inline struct clk *of_clk_get(struct device_node *np, int index) { return ERR_PTR(-ENOENT); } static inline struct clk *of_clk_get_by_name(struct device_node *np, const char *name) { return ERR_PTR(-ENOENT); } #endif
If every driver entry in Kconfig specifies "depends on OF_CONTROL" correctly, no link error would happen for this.
So, I leave this decision to you. If you do not like #ifdef in header files, just rip it off.
I'm not a huge fan of this - I feel that a link error might be preferable to a run-time error. But I don't really have a strong opinion on it. I've sent a new patch.
Regards, Simon
Hi Simon,
2016-01-21 11:46 GMT+09:00 Simon Glass sjg@chromium.org:
Hi Masahiro,
On 20 January 2016 at 19:26, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
2016-01-21 0:24 GMT+09:00 Simon Glass sjg@chromium.org:
Hi Masahiro,
On 19 January 2016 at 22:38, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
> > +/** > + * clk_get_by_index() - look up a clock referenced by a device > + * > + * Parse a device's 'clocks' list, returning information on the indexed clock, > + * ensuring that it is activated. > + * > + * @dev: Device containing the clock reference > + * @index: Clock index to return (0 = first) > + * @clk_devp: Returns clock device > + * @return: Peripheral ID for the device to control. This is the first > + * argument after the clock node phandle. If there is no arguemnt, > + * returns 0. Return -ve error code on any error > + */ > +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp); > #endif /* _CLK_H_ */
I want #ifdef in the header too, like mine http://patchwork.ozlabs.org/patch/566812/
I am not keen on that idea since it clutters up header files and we'll get a link error anyway if something is missing. Anyway, I've added it.
I am afraid there is misunderstanding here.
Please see my patch carefully.
What I mean is like this:
#if ... declaration of function prototype #else static inline empty function #endif
This is a common technique to avoid a link error.
Do you think this function will be called when device tree is not enabled? I cannot see how it would have any meaning in that case. What is the purpose?
In order to avoid build errors.
I think this coding style is often used.
For example, you can see include/linux/clk.h in Linux.
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) struct clk *of_clk_get(struct device_node *np, int index); struct clk *of_clk_get_by_name(struct device_node *np, const char *name); struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec); #else static inline struct clk *of_clk_get(struct device_node *np, int index) { return ERR_PTR(-ENOENT); } static inline struct clk *of_clk_get_by_name(struct device_node *np, const char *name) { return ERR_PTR(-ENOENT); } #endif
If every driver entry in Kconfig specifies "depends on OF_CONTROL" correctly, no link error would happen for this.
So, I leave this decision to you. If you do not like #ifdef in header files, just rip it off.
I'm not a huge fan of this - I feel that a link error might be preferable to a run-time error. But I don't really have a strong opinion on it. I've sent a new patch.
The only useful case I've come up with is when OF_CONTROL is optional.
ret = clk_get_by_index() if (ret == -ENOSYS) { If OF_CONTROL is disabled, try best effort to do something ... }
participants (2)
-
Masahiro Yamada -
Simon Glass