Re: [U-Boot] [PATCH v1 4/5] drivers: clk: Fix using assigned-clocks in the node of the clock it sets up

2019-10-29 Thread Simon Glass
Hi Jean-Jacques,

On Mon, 30 Sep 2019 at 06:29, Jean-Jacques Hiblot  wrote:
>
> This fixes the case where assigned-clocks is used to define a clock
> defaults inside this same clock's node. This is used sometimes to setup a
> default parents and/or rate for a clock.
>
> example:
> muxed_clock: muxed_clock {
> clocks = <_provider 0>, <_provider 1>;
> #clock-cells = <0>;
> assigned-clocks = <_clock>;
> assigned-clock-parents = <_provider 1>;
> };
>
> It doesn't work in u-boot because the assigned-clocks are setup *before*

U-Boot

> the clock is probed. (clk_set_parent() will likely crash or fail if called
> before the device probe function)
> Making it work by handling "assigned-clocks" in 2 steps: first before the
> clk device is probed, and then after the clk device is probed.
>
> Signed-off-by: Jean-Jacques Hiblot 
> ---
>
>  drivers/clk/clk-uclass.c | 48 +++-
>  drivers/core/device.c|  2 +-
>  include/clk.h|  7 --
>  3 files changed, 48 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass 

+Stephen Warren who is the expert here

>
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index e7ec6347de..75d618a47b 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -178,7 +178,7 @@ bulk_get_err:
> return ret;
>  }
>
> -static int clk_set_default_parents(struct udevice *dev)
> +static int clk_set_default_parents(struct udevice *dev, int stage)
>  {
> struct clk clk, parent_clk;
> int index;
> @@ -214,8 +214,18 @@ static int clk_set_default_parents(struct udevice *dev)
> return ret;
> }
>
> -   ret = clk_set_parent(, _clk);
> +   /* This is clk provider device trying to reparent itself

/*
 * This ...

> +* It cannot be done right now but need to wait after the
> +* device is probed
> +*/
> +   if (stage == 0 && clk.dev == dev)
> +   continue;
> +
> +   if (stage > 0 && clk.dev != dev)
> +   /* do not setup twice the parent clocks */
> +   continue;
>
> +   ret = clk_set_parent(, _clk);
> /*
>  * Not all drivers may support clock-reparenting (as of now).
>  * Ignore errors due to this.
> @@ -233,7 +243,7 @@ static int clk_set_default_parents(struct udevice *dev)
> return 0;
>  }
>
> -static int clk_set_default_rates(struct udevice *dev)
> +static int clk_set_default_rates(struct udevice *dev, int stage)
>  {
> struct clk clk;
> int index;
> @@ -268,7 +278,19 @@ static int clk_set_default_rates(struct udevice *dev)
> continue;
> }
>
> +   /* This is clk provider device trying to program itself
> +* It cannot be done right now but need to wait after the
> +* device is probed
> +*/

wait until after?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


[U-Boot] [PATCH v1 4/5] drivers: clk: Fix using assigned-clocks in the node of the clock it sets up

2019-09-30 Thread Jean-Jacques Hiblot
This fixes the case where assigned-clocks is used to define a clock
defaults inside this same clock's node. This is used sometimes to setup a
default parents and/or rate for a clock.

example:
muxed_clock: muxed_clock {
clocks = <_provider 0>, <_provider 1>;
#clock-cells = <0>;
assigned-clocks = <_clock>;
assigned-clock-parents = <_provider 1>;
};

It doesn't work in u-boot because the assigned-clocks are setup *before*
the clock is probed. (clk_set_parent() will likely crash or fail if called
before the device probe function)
Making it work by handling "assigned-clocks" in 2 steps: first before the
clk device is probed, and then after the clk device is probed.

Signed-off-by: Jean-Jacques Hiblot 
---

 drivers/clk/clk-uclass.c | 48 +++-
 drivers/core/device.c|  2 +-
 include/clk.h|  7 --
 3 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index e7ec6347de..75d618a47b 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -178,7 +178,7 @@ bulk_get_err:
return ret;
 }
 
-static int clk_set_default_parents(struct udevice *dev)
+static int clk_set_default_parents(struct udevice *dev, int stage)
 {
struct clk clk, parent_clk;
int index;
@@ -214,8 +214,18 @@ static int clk_set_default_parents(struct udevice *dev)
return ret;
}
 
-   ret = clk_set_parent(, _clk);
+   /* This is clk provider device trying to reparent itself
+* It cannot be done right now but need to wait after the
+* device is probed
+*/
+   if (stage == 0 && clk.dev == dev)
+   continue;
+
+   if (stage > 0 && clk.dev != dev)
+   /* do not setup twice the parent clocks */
+   continue;
 
+   ret = clk_set_parent(, _clk);
/*
 * Not all drivers may support clock-reparenting (as of now).
 * Ignore errors due to this.
@@ -233,7 +243,7 @@ static int clk_set_default_parents(struct udevice *dev)
return 0;
 }
 
-static int clk_set_default_rates(struct udevice *dev)
+static int clk_set_default_rates(struct udevice *dev, int stage)
 {
struct clk clk;
int index;
@@ -268,7 +278,19 @@ static int clk_set_default_rates(struct udevice *dev)
continue;
}
 
+   /* This is clk provider device trying to program itself
+* It cannot be done right now but need to wait after the
+* device is probed
+*/
+   if (stage == 0 && clk.dev == dev)
+   continue;
+
+   if (stage > 0 && clk.dev != dev)
+   /* do not setup twice the parent clocks */
+   continue;
+
ret = clk_set_rate(, rates[index]);
+
if (ret < 0) {
debug("%s: failed to set rate on clock index %d (%ld) 
for %s\n",
  __func__, index, clk.id, dev_read_name(dev));
@@ -281,7 +303,7 @@ fail:
return ret;
 }
 
-int clk_set_defaults(struct udevice *dev)
+int clk_set_defaults(struct udevice *dev, int stage)
 {
int ret;
 
@@ -294,11 +316,11 @@ int clk_set_defaults(struct udevice *dev)
 
debug("%s(%s)\n", __func__, dev_read_name(dev));
 
-   ret = clk_set_default_parents(dev);
+   ret = clk_set_default_parents(dev, stage);
if (ret)
return ret;
 
-   ret = clk_set_default_rates(dev);
+   ret = clk_set_default_rates(dev, stage);
if (ret < 0)
return ret;
 
@@ -673,7 +695,21 @@ void devm_clk_put(struct udevice *dev, struct clk *clk)
WARN_ON(rc);
 }
 
+int clk_uclass_post_probe(struct udevice *dev)
+{
+   /*
+* when a clock provider is probed. Call clk_set_defaults()
+* also after the device is probed. This takes care of cases
+* where the DT is used to setup default parents and rates
+* using assigned-clocks
+*/
+   clk_set_defaults(dev, 1);
+
+   return 0;
+}
+
 UCLASS_DRIVER(clk) = {
.id = UCLASS_CLK,
.name   = "clk",
+   .post_probe = clk_uclass_post_probe,
 };
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 05dadf98f9..cbcbb1b15d 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -416,7 +416,7 @@ int device_probe(struct udevice *dev)
 * Process 'assigned-{clocks/clock-parents/clock-rates}'
 * properties
 */
-   ret = clk_set_defaults(dev);
+   ret = clk_set_defaults(dev, 0);
if (ret)
goto fail;
}
diff --git a/include/clk.h b/include/clk.h
index