[U-Boot] [PATCH] dm: clk: Remove simple version of clk_get_by_index()

[U-Boot] [PATCH] dm: clk: Remove simple version of clk_get_by_index()

[Michal Simek]

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

[U-Boot] [PATCH] dm: clk: Remove simple version of clk_get_by_index()

[Stephen Warren]

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.

[U-Boot] [PATCH] dm: clk: Remove simple version of clk_get_by_index()

[Michal Simek]

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

[U-Boot] [PATCH] dm: clk: Remove simple version of clk_get_by_index()

[Simon Glass]

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