diff mbox series

clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present

Message ID 20240308000432.32369-1-semen.protsenko@linaro.org
State New
Headers show
Series clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present | expand

Commit Message

Sam Protsenko March 8, 2024, 12:04 a.m. UTC 
Sometimes clocks provided to a consumer might not have .set_rate
operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag
set. In that case it's usually possible to find a parent up the tree
which is capable of setting the rate (div, pll, etc). Implement a simple
lookup procedure for such cases, to traverse the clock tree until
.set_rate capable parent is found, and use that parent to actually
change the rate. The search will stop once the first .set_rate capable
clock is found, which is usually enough to handle most cases.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/clk/clk-uclass.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Sam Protsenko March 19, 2024, 6:42 p.m. UTC | #1 
On Thu, Mar 7, 2024 at 6:04 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Sometimes clocks provided to a consumer might not have .set_rate
> operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag
> set. In that case it's usually possible to find a parent up the tree
> which is capable of setting the rate (div, pll, etc). Implement a simple
> lookup procedure for such cases, to traverse the clock tree until
> .set_rate capable parent is found, and use that parent to actually
> change the rate. The search will stop once the first .set_rate capable
> clock is found, which is usually enough to handle most cases.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---

Hi Lukasz, Sean, Tom,

If this patch looks good to you and there are no outstanding comments,
can you please apply it? It's needed as a part of eMMC enablement for
E850-96 board, as eMMC gate (leaf) clock is specified as ciu clock in
dts, which requires further clock rate change propagation up to
divider clock.

Thanks!

>  drivers/clk/clk-uclass.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index ed6e60bc4841..755f05f34669 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -571,8 +571,20 @@ ulong clk_set_rate(struct clk *clk, ulong rate)
>                 return 0;
>         ops = clk_dev_ops(clk->dev);
>
> -       if (!ops->set_rate)
> -               return -ENOSYS;
> +       /* Try to find parents which can set rate */
> +       while (!ops->set_rate) {
> +               struct clk *parent;
> +
> +               if (!(clk->flags & CLK_SET_RATE_PARENT))
> +                       return -ENOSYS;
> +
> +               parent = clk_get_parent(clk);
> +               if (IS_ERR_OR_NULL(parent) || !clk_valid(parent))
> +                       return -ENODEV;
> +
> +               clk = parent;
> +               ops = clk_dev_ops(clk->dev);
> +       }
>
>         /* get private clock struct used for cache */
>         clk_get_priv(clk, &clkp);
> --
> 2.39.2
>
Yang Xiwen March 20, 2024, 2:02 p.m. UTC | #2 
On 3/20/2024 2:42 AM, Sam Protsenko wrote:
> On Thu, Mar 7, 2024 at 6:04 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>> Sometimes clocks provided to a consumer might not have .set_rate
>> operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag
>> set. In that case it's usually possible to find a parent up the tree
>> which is capable of setting the rate (div, pll, etc). Implement a simple
>> lookup procedure for such cases, to traverse the clock tree until
>> .set_rate capable parent is found, and use that parent to actually
>> change the rate. The search will stop once the first .set_rate capable
>> clock is found, which is usually enough to handle most cases.
>>
>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>> ---
> Hi Lukasz, Sean, Tom,
>
> If this patch looks good to you and there are no outstanding comments,
> can you please apply it? It's needed as a part of eMMC enablement for
> E850-96 board, as eMMC gate (leaf) clock is specified as ciu clock in
> dts, which requires further clock rate change propagation up to
> divider clock.
>
> Thanks!


Please be patient. The release cycle is quite long. My patch set for 
HiSilicon clk framework has been ignored for ~2months already.


>
>>   drivers/clk/clk-uclass.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>> index ed6e60bc4841..755f05f34669 100644
>> --- a/drivers/clk/clk-uclass.c
>> +++ b/drivers/clk/clk-uclass.c
>> @@ -571,8 +571,20 @@ ulong clk_set_rate(struct clk *clk, ulong rate)
>>                  return 0;
>>          ops = clk_dev_ops(clk->dev);
>>
>> -       if (!ops->set_rate)
>> -               return -ENOSYS;
>> +       /* Try to find parents which can set rate */
>> +       while (!ops->set_rate) {
>> +               struct clk *parent;
>> +
>> +               if (!(clk->flags & CLK_SET_RATE_PARENT))
>> +                       return -ENOSYS;
>> +
>> +               parent = clk_get_parent(clk);
>> +               if (IS_ERR_OR_NULL(parent) || !clk_valid(parent))
>> +                       return -ENODEV;
>> +
>> +               clk = parent;
>> +               ops = clk_dev_ops(clk->dev);
>> +       }
>>
>>          /* get private clock struct used for cache */
>>          clk_get_priv(clk, &clkp);
>> --
>> 2.39.2
>>
Sean Anderson March 20, 2024, 2:41 p.m. UTC | #3 
On 3/20/24 10:02, Yang Xiwen wrote:
> On 3/20/2024 2:42 AM, Sam Protsenko wrote:
>> On Thu, Mar 7, 2024 at 6:04 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>>> Sometimes clocks provided to a consumer might not have .set_rate
>>> operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag
>>> set. In that case it's usually possible to find a parent up the tree
>>> which is capable of setting the rate (div, pll, etc). Implement a simple
>>> lookup procedure for such cases, to traverse the clock tree until
>>> .set_rate capable parent is found, and use that parent to actually
>>> change the rate. The search will stop once the first .set_rate capable
>>> clock is found, which is usually enough to handle most cases.
>>>
>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>> ---
>> Hi Lukasz, Sean, Tom,
>>
>> If this patch looks good to you and there are no outstanding comments,
>> can you please apply it? It's needed as a part of eMMC enablement for
>> E850-96 board, as eMMC gate (leaf) clock is specified as ciu clock in
>> dts, which requires further clock rate change propagation up to
>> divider clock.
>>
>> Thanks!
> 
> 
> Please be patient. The release cycle is quite long. My patch set for HiSilicon clk framework has been ignored for ~2months already.

Sorry, I've been busy IRL recently.

I will try to review patches in the backlog, but it might be a few more weeks.

--Sean
Sean Anderson April 11, 2024, 2:53 a.m. UTC | #4 
On 3/7/24 19:04, Sam Protsenko wrote:
> Sometimes clocks provided to a consumer might not have .set_rate
> operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag
> set. In that case it's usually possible to find a parent up the tree
> which is capable of setting the rate (div, pll, etc). Implement a simple
> lookup procedure for such cases, to traverse the clock tree until
> .set_rate capable parent is found, and use that parent to actually
> change the rate. The search will stop once the first .set_rate capable
> clock is found, which is usually enough to handle most cases.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   drivers/clk/clk-uclass.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index ed6e60bc4841..755f05f34669 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -571,8 +571,20 @@ ulong clk_set_rate(struct clk *clk, ulong rate)
>   		return 0;
>   	ops = clk_dev_ops(clk->dev);
>   
> -	if (!ops->set_rate)
> -		return -ENOSYS;
> +	/* Try to find parents which can set rate */
> +	while (!ops->set_rate) {
> +		struct clk *parent;
> +
> +		if (!(clk->flags & CLK_SET_RATE_PARENT))
> +			return -ENOSYS;
> +
> +		parent = clk_get_parent(clk);
> +		if (IS_ERR_OR_NULL(parent) || !clk_valid(parent))
> +			return -ENODEV;
> +
> +		clk = parent;
> +		ops = clk_dev_ops(clk->dev);
> +	}
>   
>   	/* get private clock struct used for cache */
>   	clk_get_priv(clk, &clkp);

Can you give an example of where this is needed?

--Sean
Sam Protsenko May 1, 2024, 9:12 p.m. UTC | #5 
On Wed, Apr 10, 2024 at 9:53 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 3/7/24 19:04, Sam Protsenko wrote:
> > Sometimes clocks provided to a consumer might not have .set_rate
> > operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag
> > set. In that case it's usually possible to find a parent up the tree
> > which is capable of setting the rate (div, pll, etc). Implement a simple
> > lookup procedure for such cases, to traverse the clock tree until
> > .set_rate capable parent is found, and use that parent to actually
> > change the rate. The search will stop once the first .set_rate capable
> > clock is found, which is usually enough to handle most cases.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---

[snip]

>
> Can you give an example of where this is needed?
>

Sure. In my case it's needed for eMMC enablement on E850-96 board.
eMMC node in the device tree [1] is a gate clock
(CLK_GOUT_MMC_EMBD_SDCLKIN), so the MMC driver won't be able to change
its rate. But that clock actually has CLK_SET_RATE_PARENT flag set in
the clock driver [2]. So the right thing to do in this case (and
that's basically how it's done in Linux kernel too) is to traverse the
clock tree upwards, and try to find the parent capable to do .set_rate
(which is usually a divider clock), and use it to change the clock
rate. I'm working on exynos_dw_mmc driver [3] right now, making it use
CCF clocks, but I can't do that before this patch is applied.

Grepping the U-Boot tree I can see the CLK_SET_RATE_PARENT flag is
also used in various IMX clock drivers and STM32MP13 clock driver. I
guess without this change those flags will be basically ignored.

Thanks!

[1] https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/exynos850.dtsi#L408
[2] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/clk/exynos/clk-exynos850.c#L353
[3] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/exynos_dw_mmc.c#L117

> --Sean
diff mbox series

Patch

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index ed6e60bc4841..755f05f34669 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -571,8 +571,20 @@  ulong clk_set_rate(struct clk *clk, ulong rate)
 		return 0;
 	ops = clk_dev_ops(clk->dev);
 
-	if (!ops->set_rate)
-		return -ENOSYS;
+	/* Try to find parents which can set rate */
+	while (!ops->set_rate) {
+		struct clk *parent;
+
+		if (!(clk->flags & CLK_SET_RATE_PARENT))
+			return -ENOSYS;
+
+		parent = clk_get_parent(clk);
+		if (IS_ERR_OR_NULL(parent) || !clk_valid(parent))
+			return -ENODEV;
+
+		clk = parent;
+		ops = clk_dev_ops(clk->dev);
+	}
 
 	/* get private clock struct used for cache */
 	clk_get_priv(clk, &clkp);