[U-Boot] [PATCH] dm: clk: Remove simple version of clk_get_by_index()
Simple version of clk_get_by_index() added by: "dm: clk: Add a simple version of clk_get_by_index()" (sha1: a4b10c088c4f6ef2e2bba33e8cfea369bcbbce44) is not sufficient if you use multiple clocks in the system because clk->id is phandle id which for example fixed-clock is not able to handle. Use the same implementation as is used in full version.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
Diff for ZynqMP is 773 B based on buildman. --- drivers/clk/clk-uclass.c | 55 ++++++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 37 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 6e4d67220a16..c6c5ba2f2511 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -21,30 +21,29 @@ static inline struct clk_ops *clk_dev_ops(struct udevice *dev)
#if CONFIG_IS_ENABLED(OF_CONTROL) #ifdef CONFIG_SPL_BUILD -int clk_get_by_index(struct udevice *dev, int index, struct clk *clk) -{ - int ret; - u32 cell[2]; - - if (index != 0) - return -ENOSYS; - assert(clk); - ret = uclass_get_device(UCLASS_CLK, 0, &clk->dev); - if (ret) - return ret; - ret = fdtdec_get_int_array(gd->fdt_blob, dev->of_offset, "clocks", - cell, 2); - if (ret) - return ret; - clk->id = cell[1]; - return 0; -} - int clk_get_by_name(struct udevice *dev, const char *name, struct clk *clk) { return -ENOSYS; } #else +int clk_get_by_name(struct udevice *dev, const char *name, struct clk *clk) +{ + int index; + + debug("%s(dev=%p, name=%s, clk=%p)\n", __func__, dev, name, clk); + + index = fdt_find_string(gd->fdt_blob, dev->of_offset, "clock-names", + name); + if (index < 0) { + debug("fdt_find_string() failed: %d\n", index); + return index; + } + + return clk_get_by_index(dev, index, clk); +} +#endif +#endif + static int clk_of_xlate_default(struct clk *clk, struct fdtdec_phandle_args *args) { @@ -102,24 +101,6 @@ int clk_get_by_index(struct udevice *dev, int index, struct clk *clk) return clk_request(dev_clk, clk); }
-int clk_get_by_name(struct udevice *dev, const char *name, struct clk *clk) -{ - int index; - - debug("%s(dev=%p, name=%s, clk=%p)\n", __func__, dev, name, clk); - - index = fdt_find_string(gd->fdt_blob, dev->of_offset, "clock-names", - name); - if (index < 0) { - debug("fdt_find_string() failed: %d\n", index); - return index; - } - - return clk_get_by_index(dev, index, clk); -} -#endif -#endif - int clk_request(struct udevice *dev, struct clk *clk) { struct clk_ops *ops = clk_dev_ops(dev);
On 07/14/2016 05:24 AM, Michal Simek wrote:
Simple version of clk_get_by_index() added by: "dm: clk: Add a simple version of clk_get_by_index()" (sha1: a4b10c088c4f6ef2e2bba33e8cfea369bcbbce44) is not sufficient if you use multiple clocks in the system because clk->id is phandle id which for example fixed-clock is not able to handle. Use the same implementation as is used in full version.
It took me a while to work out what failure case you were describing. It might be worth more explicitly pointing out that the existing simple implementation fails in any case where #clock-cells=<0>, or for larger #clock-cells, where the clock ID isn't in the first cell.
To be honest, I'd be inclined to always include the real clk_get_by_name() in SPL builds too. If it's never called, the function will be dropped by the linker. If it is called, the dummy implementation probably actively causes failures that we should avoid by using the real implementation. I'm not sure why the original SPL-specific code existed, unless the 773 byte code increase you mention is actually problematic for some specific build?
Signed-off-by: Michal Simek michal.simek@xilinx.com
Diff for ZynqMP is 773 B based on buildman.
On 14.7.2016 20:17, Stephen Warren wrote:
On 07/14/2016 05:24 AM, Michal Simek wrote:
Simple version of clk_get_by_index() added by: "dm: clk: Add a simple version of clk_get_by_index()" (sha1: a4b10c088c4f6ef2e2bba33e8cfea369bcbbce44) is not sufficient if you use multiple clocks in the system because clk->id is phandle id which for example fixed-clock is not able to handle. Use the same implementation as is used in full version.
It took me a while to work out what failure case you were describing. It might be worth more explicitly pointing out that the existing simple implementation fails in any case where #clock-cells=<0>, or for larger #clock-cells, where the clock ID isn't in the first cell.
TBH I didn't try to understand if this is related to clock-cells size because I thought that fixed-clock are tested in mainline and my testing platform is ZynqMP where we don't have own clock driver. That's why this is not described in commit message.
To be honest, I'd be inclined to always include the real clk_get_by_name() in SPL builds too. If it's never called, the function will be dropped by the linker. If it is called, the dummy implementation probably actively causes failures that we should avoid by using the real implementation. I'm not sure why the original SPL-specific code existed, unless the 773 byte code increase you mention is actually problematic for some specific build?
Agree.
The next part I found is that when I was playing with mmc and serial driver that I do call the same clk sequence in both drivers and will be good to introduce own function to ensure that the same code is not copied in several places which increase just size.
For my testing the sequence is like this (clock should be probably return value).
+ int ret; + struct clk clk; + unsigned long clock; + + ret = clk_get_by_index(dev, 0, &clk); + if (ret < 0) { + dev_err(dev, "failed to get clock\n"); + return ret; + } + + clock = clk_get_rate(&clk); + if (IS_ERR_VALUE(clock)) { + dev_err(dev, "failed to get rate\n"); + return clock; + } + debug("%s: CLK %ld\n", __func__, clock); + + ret = clk_enable(&clk); + if (ret && ret != -ENOSYS) { + dev_err(dev, "failed to enable clock\n"); + return ret; + }
Thanks, Michal
Hi Michal,
On 15 July 2016 at 00:01, Michal Simek michal.simek@xilinx.com wrote:
On 14.7.2016 20:17, Stephen Warren wrote:
On 07/14/2016 05:24 AM, Michal Simek wrote:
Simple version of clk_get_by_index() added by: "dm: clk: Add a simple version of clk_get_by_index()" (sha1: a4b10c088c4f6ef2e2bba33e8cfea369bcbbce44) is not sufficient if you use multiple clocks in the system because clk->id is phandle id which for example fixed-clock is not able to handle. Use the same implementation as is used in full version.
It took me a while to work out what failure case you were describing. It might be worth more explicitly pointing out that the existing simple implementation fails in any case where #clock-cells=<0>, or for larger #clock-cells, where the clock ID isn't in the first cell.
TBH I didn't try to understand if this is related to clock-cells size because I thought that fixed-clock are tested in mainline and my testing platform is ZynqMP where we don't have own clock driver. That's why this is not described in commit message.
To be honest, I'd be inclined to always include the real clk_get_by_name() in SPL builds too. If it's never called, the function will be dropped by the linker. If it is called, the dummy implementation probably actively causes failures that we should avoid by using the real implementation. I'm not sure why the original SPL-specific code existed, unless the 773 byte code increase you mention is actually problematic for some specific build?
Agree.
The next part I found is that when I was playing with mmc and serial driver that I do call the same clk sequence in both drivers and will be good to introduce own function to ensure that the same code is not copied in several places which increase just size.
For my testing the sequence is like this (clock should be probably return value).
int ret;struct clk clk;unsigned long clock;ret = clk_get_by_index(dev, 0, &clk);if (ret < 0) {dev_err(dev, "failed to get clock\n");return ret;}clock = clk_get_rate(&clk);if (IS_ERR_VALUE(clock)) {dev_err(dev, "failed to get rate\n");return clock;}debug("%s: CLK %ld\n", __func__, clock);ret = clk_enable(&clk);if (ret && ret != -ENOSYS) {dev_err(dev, "failed to enable clock\n");return ret;}
Yes I think a helper function in the uclass would be good.
Regards, Simon
participants (3)
-
Michal Simek -
Simon Glass -
Stephen Warren