[PATCH RESEND 0/5] clk: A few bugfixes/enhancements for CCF
They are found during my development for HiSilicon clock driver. Details are in commit logs.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- Yang Xiwen (5): clk: export clk_register_mux_table() clk: call log_debug() instead to avoid console log printing clk: also handle ENOENT in *_optional functions clk: promote clk_dev_ops to linux/clk-provider.h clk: ccf: call clock provided ops directly for endisable()
drivers/clk/clk-uclass.c | 5 ----- drivers/clk/clk.c | 16 +++++++++++++--- include/clk.h | 8 +++++--- include/linux/clk-provider.h | 11 +++++++++++ 4 files changed, 29 insertions(+), 11 deletions(-) --- base-commit: 580eb31199be8a822e62f20965854a242f895d03 change-id: 20230807-clk-fix-17e895f79817
Best regards,
From: Yang Xiwen forbidden405@outlook.com
It's already implemented in clk-mux.c, export it in the header file.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- include/linux/clk-provider.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index b8acacd49e..801404480b 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -247,6 +247,12 @@ struct clk *clk_register_mux(struct device *dev, const char *name, void __iomem *reg, u8 shift, u8 width, u8 clk_mux_flags);
+struct clk *clk_register_mux_table(struct device *dev, const char *name, + const char * const *parent_names, u8 num_parents, + unsigned long flags, + void __iomem *reg, u8 shift, u32 mask, + u8 clk_mux_flags, u32 *table); + struct clk *clk_register_fixed_rate(struct device *dev, const char *name, ulong rate);
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
It's already implemented in clk-mux.c, export it in the header file.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
include/linux/clk-provider.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index b8acacd49e..801404480b 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -247,6 +247,12 @@ struct clk *clk_register_mux(struct device *dev, const char *name, void __iomem *reg, u8 shift, u8 width, u8 clk_mux_flags);
+struct clk *clk_register_mux_table(struct device *dev, const char *name,
const char * const *parent_names, u8 num_parents,unsigned long flags,void __iomem *reg, u8 shift, u32 mask,u8 clk_mux_flags, u32 *table);- struct clk *clk_register_fixed_rate(struct device *dev, const char *name, ulong rate);
Why do you want to export this? None of your other patches use it.
--Sean
On 11/2/2023 1:50 AM, Sean Anderson wrote:
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
It's already implemented in clk-mux.c, export it in the header file.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
include/linux/clk-provider.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index b8acacd49e..801404480b 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -247,6 +247,12 @@ struct clk *clk_register_mux(struct device *dev, const char *name, void __iomem *reg, u8 shift, u8 width, u8 clk_mux_flags); +struct clk *clk_register_mux_table(struct device *dev, const char *name, + const char * const *parent_names, u8 num_parents, + unsigned long flags, + void __iomem *reg, u8 shift, u32 mask, + u8 clk_mux_flags, u32 *table);
struct clk *clk_register_fixed_rate(struct device *dev, const char *name, ulong rate);
Why do you want to export this? None of your other patches use it.
It will be used in HiSilicon clk framework driver which i will send after this series is applied. And this function is exported in Linux kernel CCF. So i think it's fine to export it in U-Boot as well.
--Sean
On 11/1/23 14:37, Yang Xiwen wrote:
On 11/2/2023 1:50 AM, Sean Anderson wrote:
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
It's already implemented in clk-mux.c, export it in the header file.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
include/linux/clk-provider.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index b8acacd49e..801404480b 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -247,6 +247,12 @@ struct clk *clk_register_mux(struct device *dev, const char *name, void __iomem *reg, u8 shift, u8 width, u8 clk_mux_flags); +struct clk *clk_register_mux_table(struct device *dev, const char *name, + const char * const *parent_names, u8 num_parents, + unsigned long flags, + void __iomem *reg, u8 shift, u32 mask, + u8 clk_mux_flags, u32 *table);
struct clk *clk_register_fixed_rate(struct device *dev, const char *name, ulong rate);
Why do you want to export this? None of your other patches use it.
It will be used in HiSilicon clk framework driver which i will send after this series is applied. And this function is exported in Linux kernel CCF. So i think it's fine to export it in U-Boot as well.
Please just send it along with that series then.
--Sean
From: Yang Xiwen forbidden405@outlook.com
it's a very common case to register a clock without a parent, such as clk_register_fixed_rate(). Replace log_error() with log_debug() to avoid useless console log if not debugging.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- drivers/clk/clk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a5a3461b66..a38daaac0c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -24,8 +24,8 @@ int clk_register(struct clk *clk, const char *drv_name,
ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); if (ret) { - log_err("%s: failed to get %s device (parent of %s)\n", - __func__, parent_name, name); + log_debug("%s: failed to get %s device (parent of %s)\n", + __func__, parent_name, name); } else { log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, parent->name, parent);
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
it's a very common case to register a clock without a parent, such as clk_register_fixed_rate().
Actually, that seems like the only place this is done.
Replace log_error() with log_debug() to avoid useless console log if not debugging.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
drivers/clk/clk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a5a3461b66..a38daaac0c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -24,8 +24,8 @@ int clk_register(struct clk *clk, const char *drv_name,
ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); if (ret) {
log_err("%s: failed to get %s device (parent of %s)\n",__func__, parent_name, name);
log_debug("%s: failed to get %s device (parent of %s)\n",__func__, parent_name, name);
I think a correct fix would be
diff --git i/drivers/clk/clk.c w/drivers/clk/clk.c index a5a3461b66c..cb333c83f66 100644 --- i/drivers/clk/clk.c +++ w/drivers/clk/clk.c @@ -18,17 +18,19 @@ int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) { - struct udevice *parent; + struct udevice *parent = NULL; struct driver *drv; int ret;
- ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); - if (ret) { - log_err("%s: failed to get %s device (parent of %s)\n", - __func__, parent_name, name); - } else { - log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, - parent->name, parent); + if (parent_name) { + ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, + &parent); + if (ret) + log_err("%s: failed to get %s device (parent of %s)\n", + __func__, parent_name, name); + else + log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, + name, parent->name, parent); }
drv = lists_driver_lookup_name(drv_name);
--Sean
On 11/1/23 13:55, Sean Anderson wrote:
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
it's a very common case to register a clock without a parent, such as clk_register_fixed_rate().
Actually, that seems like the only place this is done.
Replace log_error() with log_debug() to avoid useless console log if not debugging.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
drivers/clk/clk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a5a3461b66..a38daaac0c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -24,8 +24,8 @@ int clk_register(struct clk *clk, const char *drv_name, ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); if (ret) { - log_err("%s: failed to get %s device (parent of %s)\n", - __func__, parent_name, name); + log_debug("%s: failed to get %s device (parent of %s)\n", + __func__, parent_name, name); } else { log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, parent->name, parent);
I think a correct fix would be
diff --git i/drivers/clk/clk.c w/drivers/clk/clk.c index a5a3461b66c..cb333c83f66 100644 --- i/drivers/clk/clk.c +++ w/drivers/clk/clk.c @@ -18,17 +18,19 @@ int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) { - struct udevice *parent; + struct udevice *parent = NULL; struct driver *drv; int ret;
- ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); - if (ret) { - log_err("%s: failed to get %s device (parent of %s)\n", - __func__, parent_name, name); - } else { - log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, - parent->name, parent); + if (parent_name) { + ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, + &parent); + if (ret) + log_err("%s: failed to get %s device (parent of %s)\n", + __func__, parent_name, name); + else + log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, + name, parent->name, parent); }
drv = lists_driver_lookup_name(drv_name);
--Sean
or you could modify the condition to be `if (ret && parent_name)` with appropriate modification of the second debug message.
--Sean
On 11/2/2023 2:01 AM, Sean Anderson wrote:
On 11/1/23 13:55, Sean Anderson wrote:
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
it's a very common case to register a clock without a parent, such as clk_register_fixed_rate().
Actually, that seems like the only place this is done.
Replace log_error() with log_debug() to avoid useless console log if not debugging.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
drivers/clk/clk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a5a3461b66..a38daaac0c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -24,8 +24,8 @@ int clk_register(struct clk *clk, const char *drv_name, ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); if (ret) { - log_err("%s: failed to get %s device (parent of %s)\n", - __func__, parent_name, name); + log_debug("%s: failed to get %s device (parent of %s)\n", + __func__, parent_name, name); } else { log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, parent->name, parent);
I think a correct fix would be
diff --git i/drivers/clk/clk.c w/drivers/clk/clk.c index a5a3461b66c..cb333c83f66 100644 --- i/drivers/clk/clk.c +++ w/drivers/clk/clk.c @@ -18,17 +18,19 @@ int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) { - struct udevice *parent; + struct udevice *parent = NULL; struct driver *drv; int ret;
- ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, &parent); - if (ret) { - log_err("%s: failed to get %s device (parent of %s)\n", - __func__, parent_name, name); - } else { - log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, name, - parent->name, parent); + if (parent_name) { + ret = uclass_get_device_by_name(UCLASS_CLK, parent_name, + &parent); + if (ret) + log_err("%s: failed to get %s device (parent of %s)\n", + __func__, parent_name, name); + else + log_debug("%s: name: %s parent: %s [0x%p]\n", __func__, + name, parent->name, parent); }
drv = lists_driver_lookup_name(drv_name);
--Sean
or you could modify the condition to be `if (ret && parent_name)` with appropriate modification of the second debug message.
Thanks for your review. I'll use your patch with your SoB and drop mine in next release.
--Sean
From: Yang Xiwen forbidden405@outlook.com
If the device does not specify any clocks in device tree, these functions will return PTR_ERR(-ENOENT). This is not the intended behavior and does not comply with linux kernel CCF. Fix that by returning NULL under such circumstances instead.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- include/clk.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/clk.h b/include/clk.h index d91285235f..f95da0838d 100644 --- a/include/clk.h +++ b/include/clk.h @@ -223,9 +223,11 @@ struct clk *devm_clk_get(struct udevice *dev, const char *id); static inline struct clk *devm_clk_get_optional(struct udevice *dev, const char *id) { + int ret; struct clk *clk = devm_clk_get(dev, id);
- if (PTR_ERR(clk) == -ENODATA) + ret = PTR_ERR(clk); + if (ret == -ENODATA || ret == -ENOENT) return NULL;
return clk; @@ -335,7 +337,7 @@ static inline int clk_get_by_name_optional(struct udevice *dev, int ret;
ret = clk_get_by_name(dev, name, clk); - if (ret == -ENODATA) + if (ret == -ENODATA || ret == -ENOENT) return 0;
return ret; @@ -359,7 +361,7 @@ static inline int clk_get_by_name_nodev_optional(ofnode node, const char *name, int ret;
ret = clk_get_by_name_nodev(node, name, clk); - if (ret == -ENODATA) + if (ret == -ENODATA || ret == -ENOENT) return 0;
return ret;
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
If the device does not specify any clocks in device tree, these functions will return PTR_ERR(-ENOENT). This is not the intended behavior and does not comply with linux kernel CCF. Fix that by returning NULL under such circumstances instead.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
include/clk.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/clk.h b/include/clk.h index d91285235f..f95da0838d 100644 --- a/include/clk.h +++ b/include/clk.h @@ -223,9 +223,11 @@ struct clk *devm_clk_get(struct udevice *dev, const char *id); static inline struct clk *devm_clk_get_optional(struct udevice *dev, const char *id) {
- int ret; struct clk *clk = devm_clk_get(dev, id);
- if (PTR_ERR(clk) == -ENODATA)
ret = PTR_ERR(clk);
if (ret == -ENODATA || ret == -ENOENT) return NULL;
return clk;
@@ -335,7 +337,7 @@ static inline int clk_get_by_name_optional(struct udevice *dev, int ret;
ret = clk_get_by_name(dev, name, clk);
- if (ret == -ENODATA)
if (ret == -ENODATA || ret == -ENOENT) return 0;
return ret;
@@ -359,7 +361,7 @@ static inline int clk_get_by_name_nodev_optional(ofnode node, const char *name, int ret;
ret = clk_get_by_name_nodev(node, name, clk);
- if (ret == -ENODATA)
if (ret == -ENODATA || ret == -ENOENT) return 0;
return ret;
Reviewed-by: Sean Anderson seanga2@gmail.com
From: Yang Xiwen forbidden405@outlook.com
So that it can be used by others.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- drivers/clk/clk-uclass.c | 5 ----- include/linux/clk-provider.h | 5 +++++ 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index dc3e9d6a26..5cc80e5e39 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -25,11 +25,6 @@ #include <linux/clk-provider.h> #include <linux/err.h>
-static inline const struct clk_ops *clk_dev_ops(struct udevice *dev) -{ - return (const struct clk_ops *)dev->driver->ops; -} - struct clk *dev_get_clk_ptr(struct udevice *dev) { return (struct clk *)dev_get_uclass_priv(dev); diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 801404480b..dfafb4cc9d 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -21,6 +21,11 @@ static inline void clk_dm(ulong id, struct clk *clk) clk->id = id; }
+static inline const struct clk_ops *clk_dev_ops(struct udevice *dev) +{ + return (const struct clk_ops *)dev->driver->ops; +} + /* * flags used across common struct clk. these flags should only affect the * top-level framework. custom flags for dealing with hardware specifics
From: Yang Xiwen forbidden405@outlook.com
Calling into CCF framework will cause a clock being enabled twice instead of once (clk->enable_count becomes 2 rather than 1), thus making it hard to disable (needs to call clk_disable() twice). Fix that by calling clock provided ops directly.
Signed-off-by: Yang Xiwen forbidden405@outlook.com --- drivers/clk/clk.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a38daaac0c..00d082c46f 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -14,6 +14,7 @@ #include <dm/uclass.h> #include <dm/lists.h> #include <dm/device-internal.h> +#include <linux/clk-provider.h>
int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) @@ -115,11 +116,20 @@ int ccf_clk_set_parent(struct clk *clk, struct clk *parent) static int ccf_clk_endisable(struct clk *clk, bool enable) { struct clk *c; + const struct clk_ops *ops; int err = clk_get_by_id(clk->id, &c);
if (err) return err; - return enable ? clk_enable(c) : clk_disable(c); + else + ops = clk_dev_ops(c->dev); + + if (enable && ops->enable) + return ops->enable(c); + else if (!enable && ops->disable) + return ops->disable(c); + + return -ENOSYS; }
int ccf_clk_enable(struct clk *clk)
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
Calling into CCF framework will cause a clock being enabled twice instead of once (clk->enable_count becomes 2 rather than 1), thus making it hard to disable (needs to call clk_disable() twice). Fix that by calling clock provided ops directly.
Can you describe this scenario more? From what I can tell, clk_enable doesn't increment enable_count for CCF clocks.
--Sean
Signed-off-by: Yang Xiwen forbidden405@outlook.com
drivers/clk/clk.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a38daaac0c..00d082c46f 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -14,6 +14,7 @@ #include <dm/uclass.h> #include <dm/lists.h> #include <dm/device-internal.h> +#include <linux/clk-provider.h>
int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) @@ -115,11 +116,20 @@ int ccf_clk_set_parent(struct clk *clk, struct clk *parent) static int ccf_clk_endisable(struct clk *clk, bool enable) { struct clk *c;
const struct clk_ops *ops; int err = clk_get_by_id(clk->id, &c);
if (err) return err;
- return enable ? clk_enable(c) : clk_disable(c);
else
ops = clk_dev_ops(c->dev);if (enable && ops->enable)
return ops->enable(c);else if (!enable && ops->disable)
return ops->disable(c);return -ENOSYS; }
int ccf_clk_enable(struct clk *clk)
On 11/2/2023 2:19 AM, Sean Anderson wrote:
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen forbidden405@outlook.com
Calling into CCF framework will cause a clock being enabled twice instead of once (clk->enable_count becomes 2 rather than 1), thus making it hard to disable (needs to call clk_disable() twice). Fix that by calling clock provided ops directly.
Can you describe this scenario more? From what I can tell, clk_enable doesn't increment enable_count for CCF clocks.
Well, it's hard to describe clearly. But I can only tell this patch fixed the issue when i was trying to write an Ethernet driver[1] which calls clk_disable() and expects the clock to be disabled after that. Also I found that CCF driver does not have a corresponding test file. I will try to write a test for that in next release.
--Sean
Signed-off-by: Yang Xiwen forbidden405@outlook.com
drivers/clk/clk.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a38daaac0c..00d082c46f 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -14,6 +14,7 @@ #include <dm/uclass.h> #include <dm/lists.h> #include <dm/device-internal.h> +#include <linux/clk-provider.h> int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) @@ -115,11 +116,20 @@ int ccf_clk_set_parent(struct clk *clk, struct clk *parent) static int ccf_clk_endisable(struct clk *clk, bool enable) { struct clk *c; + const struct clk_ops *ops; int err = clk_get_by_id(clk->id, &c); if (err) return err; - return enable ? clk_enable(c) : clk_disable(c); + else + ops = clk_dev_ops(c->dev);
+ if (enable && ops->enable) + return ops->enable(c); + else if (!enable && ops->disable) + return ops->disable(c);
+ return -ENOSYS; } int ccf_clk_enable(struct clk *clk)
[1] https://lore.kernel.org/all/20230814-wip-hisi_femac-trunk-v2-0-1e29f400585d@...
Why is this patchset completely ignored for more than half a month already? I have some other patches pending because of this one. Please tell me what's wrong with this patchset so that i can fix them.
On Fri, 18 Aug 2023 01:03:59 +0800, Yang Xiwen wrote:
They are found during my development for HiSilicon clock driver. Details are in commit logs.
Applied, thanks!
[3/5] clk: also handle ENOENT in *_optional functions commit: c4b52fda6924e92c6745351f32c4cafc36034170
Best regards,
participants (3)
-
Sean Anderson -
Yang Xiwen -
Yang Xiwen via B4 Relay