[PATCH clk-fixes v1 0/2] Fix clk-composite to support .determine_rate

[PATCH clk-fixes v1 0/2] Fix clk-composite to support .determine_rate

[Martin Blumenstingl]

Alex reports [0] that commit 69a00fb3d69706 ("clk: divider: Implement
and wire up .determine_rate by default") breaks Rockchip platforms
because the parent is not considered anymore. This is because
clk-composite skips the "best parent" selection when
rate_ops.determine_rate is set. Above commit does this by adding
clk_divider_ops.determine_rate by default (then the Rockchip platform
drivers are using clk_divider_ops as rate_ops in clk-composite).

With these two patches a revert of above commit is not needed anymore
(which would result in a revert of five follow-up commits as well).
Instead the first patch changes the order so clk_divider_ops which
has both, .determine_rate and .round_rate are supported by clk-divider
(again).
The second patch makes clk-composite use (and even prefer)
rate_ops.determine_rate when available.

Special thanks to Alex for his patience and helping test these patches
off-list (since I don't have any board with Rockchip SoC).

At least the first patch should go into -fixes.


[0] https://lore.kernel.org/linux-clk/4eb964ac-4fff-b59d-2660-2f84d8af5901@gmail.com/


Martin Blumenstingl (2):
  clk: composite: Also consider .determine_rate for rate + mux
    composites
  clk: composite: Use rate_ops.determine_rate when also a mux is
    available

 drivers/clk/clk-composite.c | 76 +++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 24 deletions(-)

-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH clk-fixes v1 1/2] clk: composite: Also consider .determine_rate for rate + mux composites

[Martin Blumenstingl]

Commit 69a00fb3d69706 ("clk: divider: Implement and wire up
.determine_rate by default") switches clk_divider_ops to implement
.determine_rate by default. This breaks composite clocks with multiple
parents because clk-composite.c does not use the special handling for
mux + divider combinations anymore (that was restricted to rate clocks
which only implement .round_rate, but not .determine_rate).

Alex reports:
  This breaks lot of clocks for Rockchip which intensively uses
  composites,  i.e. those clocks will always stay at the initial parent,
  which in some cases  is the XTAL clock and I strongly guess it is the
  same for other platforms,  which use composite clocks having more than
  one parent (e.g. mediatek, ti ...)

  Example (RK3399)
  clk_sdio is set (initialized) with XTAL (24 MHz) as parent in u-boot.
  It will always stay at this parent, even if the mmc driver sets a rate
  of  200 MHz (fails, as the nature of things), which should switch it
  to   any of its possible parent PLLs defined in
  mux_pll_src_cpll_gpll_npll_ppll_upll_24m_p (see clk-rk3399.c)  - which
  never happens.

Restore the original behavior by changing the priority of the conditions
inside clk-composite.c. Now the special rate + mux case (with rate_ops
having a .round_rate - which is still the case for the default
clk_divider_ops) is preferred over rate_ops which have .determine_rate
defined (and not further considering the mux).

Fixes: 69a00fb3d69706 ("clk: divider: Implement and wire up .determine_rate by default")
Reported-by: Alex Bee <knaerzche@gmail.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/clk-composite.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index c7b97fb0051b..ba8d4d8cf8dd 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -58,11 +58,8 @@ static int clk_composite_determine_rate(struct clk_hw *hw,
 	long rate;
 	int i;
 
-	if (rate_hw && rate_ops && rate_ops->determine_rate) {
-		__clk_hw_set_clk(rate_hw, hw);
-		return rate_ops->determine_rate(rate_hw, req);
-	} else if (rate_hw && rate_ops && rate_ops->round_rate &&
-		   mux_hw && mux_ops && mux_ops->set_parent) {
+	if (rate_hw && rate_ops && rate_ops->round_rate &&
+	    mux_hw && mux_ops && mux_ops->set_parent) {
 		req->best_parent_hw = NULL;
 
 		if (clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT) {
@@ -107,6 +104,9 @@ static int clk_composite_determine_rate(struct clk_hw *hw,
 
 		req->rate = best_rate;
 		return 0;
+	} else if (rate_hw && rate_ops && rate_ops->determine_rate) {
+		__clk_hw_set_clk(rate_hw, hw);
+		return rate_ops->determine_rate(rate_hw, req);
 	} else if (mux_hw && mux_ops && mux_ops->determine_rate) {
 		__clk_hw_set_clk(mux_hw, hw);
 		return mux_ops->determine_rate(mux_hw, req);
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH clk-fixes v1 2/2] clk: composite: Use rate_ops.determine_rate when also a mux is available

[Martin Blumenstingl]

Update clk_composite_determine_rate() to use rate_ops.determine_rate
when available in combination with a mux. So far clk_divider_ops provide
both, .round_rate and .determine_rate. Removing the former would make
clk-composite fail silently for example on Rockchip platforms (which
heavily use composite clocks).
Add support for using rate_ops.determine_rate when either
rate_ops.round_rate is not available or both (.round_rate and
.determine_rate) are provided.

Suggested-by: Alex Bee <knaerzche@gmail.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/clk-composite.c | 68 ++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index ba8d4d8cf8dd..c04ae0e7e4b4 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -42,6 +42,29 @@ static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
 	return rate_ops->recalc_rate(rate_hw, parent_rate);
 }
 
+static int clk_composite_determine_rate_for_parent(struct clk_hw *rate_hw,
+						   struct clk_rate_request *req,
+						   struct clk_hw *parent_hw,
+						   const struct clk_ops *rate_ops)
+{
+	long rate;
+
+	req->best_parent_hw = parent_hw;
+	req->best_parent_rate = clk_hw_get_rate(parent_hw);
+
+	if (rate_ops->determine_rate)
+		return rate_ops->determine_rate(rate_hw, req);
+
+	rate = rate_ops->round_rate(rate_hw, req->rate,
+				    &req->best_parent_rate);
+	if (rate < 0)
+		return rate;
+
+	req->rate = rate;
+
+	return 0;
+}
+
 static int clk_composite_determine_rate(struct clk_hw *hw,
 					struct clk_rate_request *req)
 {
@@ -51,51 +74,56 @@ static int clk_composite_determine_rate(struct clk_hw *hw,
 	struct clk_hw *rate_hw = composite->rate_hw;
 	struct clk_hw *mux_hw = composite->mux_hw;
 	struct clk_hw *parent;
-	unsigned long parent_rate;
-	long tmp_rate, best_rate = 0;
 	unsigned long rate_diff;
 	unsigned long best_rate_diff = ULONG_MAX;
-	long rate;
-	int i;
+	unsigned long best_rate = 0;
+	int i, ret;
 
-	if (rate_hw && rate_ops && rate_ops->round_rate &&
+	if (rate_hw && rate_ops &&
+	    (rate_ops->determine_rate || rate_ops->round_rate) &&
 	    mux_hw && mux_ops && mux_ops->set_parent) {
 		req->best_parent_hw = NULL;
 
 		if (clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT) {
+			struct clk_rate_request tmp_req = *req;
+
 			parent = clk_hw_get_parent(mux_hw);
-			req->best_parent_hw = parent;
-			req->best_parent_rate = clk_hw_get_rate(parent);
 
-			rate = rate_ops->round_rate(rate_hw, req->rate,
-						    &req->best_parent_rate);
-			if (rate < 0)
-				return rate;
+			ret = clk_composite_determine_rate_for_parent(rate_hw,
+								      &tmp_req,
+								      parent,
+								      rate_ops);
+			if (ret)
+				return ret;
+
+			req->rate = tmp_req.rate;
+			req->best_parent_rate = tmp_req.best_parent_rate;
 
-			req->rate = rate;
 			return 0;
 		}
 
 		for (i = 0; i < clk_hw_get_num_parents(mux_hw); i++) {
+			struct clk_rate_request tmp_req = *req;
+
 			parent = clk_hw_get_parent_by_index(mux_hw, i);
 			if (!parent)
 				continue;
 
-			parent_rate = clk_hw_get_rate(parent);
-
-			tmp_rate = rate_ops->round_rate(rate_hw, req->rate,
-							&parent_rate);
-			if (tmp_rate < 0)
+			ret = clk_composite_determine_rate_for_parent(rate_hw,
+								      &tmp_req,
+								      parent,
+								      rate_ops);
+			if (ret)
 				continue;
 
-			rate_diff = abs(req->rate - tmp_rate);
+			rate_diff = abs(req->rate - tmp_req.rate);
 
 			if (!rate_diff || !req->best_parent_hw
 				       || best_rate_diff > rate_diff) {
 				req->best_parent_hw = parent;
-				req->best_parent_rate = parent_rate;
+				req->best_parent_rate = tmp_req.best_parent_rate;
 				best_rate_diff = rate_diff;
-				best_rate = tmp_rate;
+				best_rate = tmp_req.rate;
 			}
 
 			if (!rate_diff)
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH clk-fixes v1 0/2] Fix clk-composite to support .determine_rate

[Alex Bee]

Hi Martin, Hi Stephen,

Am 16.10.21 um 12:50 schrieb Martin Blumenstingl:
> Alex reports [0] that commit 69a00fb3d69706 ("clk: divider: Implement
> and wire up .determine_rate by default") breaks Rockchip platforms
> because the parent is not considered anymore. This is because
> clk-composite skips the "best parent" selection when
> rate_ops.determine_rate is set. Above commit does this by adding
> clk_divider_ops.determine_rate by default (then the Rockchip platform
> drivers are using clk_divider_ops as rate_ops in clk-composite).
>
> With these two patches a revert of above commit is not needed anymore
> (which would result in a revert of five follow-up commits as well).
> Instead the first patch changes the order so clk_divider_ops which
> has both, .determine_rate and .round_rate are supported by clk-divider
> (again).
> The second patch makes clk-composite use (and even prefer)
> rate_ops.determine_rate when available.
>
> Special thanks to Alex for his patience and helping test these patches
> off-list (since I don't have any board with Rockchip SoC).
>
> At least the first patch should go into -fixes.
>
>
> [0] https://lore.kernel.org/linux-clk/4eb964ac-4fff-b59d-2660-2f84d8af5901@gmail.com/

thanks for looking into this and fixing .determine_rate + mux as well 
for making this future-proof.

Both [PATCH 1/2] as a standalone fix, as well as  [PATCH 1/2] and [PATCH 
2/2] in combination are

     Tested-by: Alex Bee <knaerzche@gmail.com>

Regards,
Alex


>
> Martin Blumenstingl (2):
>    clk: composite: Also consider .determine_rate for rate + mux
>      composites
>    clk: composite: Use rate_ops.determine_rate when also a mux is
>      available
>
>   drivers/clk/clk-composite.c | 76 +++++++++++++++++++++++++------------
>   1 file changed, 52 insertions(+), 24 deletions(-)
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH clk-fixes v1 0/2] Fix clk-composite to support .determine_rate

[Chen-Yu Tsai]

On Sat, Oct 16, 2021 at 7:51 PM Alex Bee <knaerzche@gmail.com> wrote:
>
> Hi Martin, Hi Stephen,
>
> Am 16.10.21 um 12:50 schrieb Martin Blumenstingl:
> > Alex reports [0] that commit 69a00fb3d69706 ("clk: divider: Implement
> > and wire up .determine_rate by default") breaks Rockchip platforms
> > because the parent is not considered anymore. This is because
> > clk-composite skips the "best parent" selection when
> > rate_ops.determine_rate is set. Above commit does this by adding
> > clk_divider_ops.determine_rate by default (then the Rockchip platform
> > drivers are using clk_divider_ops as rate_ops in clk-composite).
> >
> > With these two patches a revert of above commit is not needed anymore
> > (which would result in a revert of five follow-up commits as well).
> > Instead the first patch changes the order so clk_divider_ops which
> > has both, .determine_rate and .round_rate are supported by clk-divider
> > (again).
> > The second patch makes clk-composite use (and even prefer)
> > rate_ops.determine_rate when available.
> >
> > Special thanks to Alex for his patience and helping test these patches
> > off-list (since I don't have any board with Rockchip SoC).
> >
> > At least the first patch should go into -fixes.
> >
> >
> > [0] https://lore.kernel.org/linux-clk/4eb964ac-4fff-b59d-2660-2f84d8af5901@gmail.com/
>
> thanks for looking into this and fixing .determine_rate + mux as well
> for making this future-proof.
>
> Both [PATCH 1/2] as a standalone fix, as well as  [PATCH 1/2] and [PATCH
> 2/2] in combination are
>
>      Tested-by: Alex Bee <knaerzche@gmail.com>
>

Tested-by: Chen-Yu Tsai <wens@csie.org> # on NanoPi M4B and M4V2

showing SDIO back at around 150 MHz.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH clk-fixes v1 1/2] clk: composite: Also consider .determine_rate for rate + mux composites

[Stephen Boyd]

Quoting Martin Blumenstingl (2021-10-16 03:50:21)
> Commit 69a00fb3d69706 ("clk: divider: Implement and wire up
> .determine_rate by default") switches clk_divider_ops to implement
> .determine_rate by default. This breaks composite clocks with multiple
> parents because clk-composite.c does not use the special handling for
> mux + divider combinations anymore (that was restricted to rate clocks
> which only implement .round_rate, but not .determine_rate).
> 
> Alex reports:
>   This breaks lot of clocks for Rockchip which intensively uses
>   composites,  i.e. those clocks will always stay at the initial parent,
>   which in some cases  is the XTAL clock and I strongly guess it is the
>   same for other platforms,  which use composite clocks having more than
>   one parent (e.g. mediatek, ti ...)
> 
>   Example (RK3399)
>   clk_sdio is set (initialized) with XTAL (24 MHz) as parent in u-boot.
>   It will always stay at this parent, even if the mmc driver sets a rate
>   of  200 MHz (fails, as the nature of things), which should switch it
>   to   any of its possible parent PLLs defined in
>   mux_pll_src_cpll_gpll_npll_ppll_upll_24m_p (see clk-rk3399.c)  - which
>   never happens.
> 
> Restore the original behavior by changing the priority of the conditions
> inside clk-composite.c. Now the special rate + mux case (with rate_ops
> having a .round_rate - which is still the case for the default
> clk_divider_ops) is preferred over rate_ops which have .determine_rate
> defined (and not further considering the mux).
> 
> Fixes: 69a00fb3d69706 ("clk: divider: Implement and wire up .determine_rate by default")
> Reported-by: Alex Bee <knaerzche@gmail.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Applied to clk-fixes

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH clk-fixes v1 2/2] clk: composite: Use rate_ops.determine_rate when also a mux is available

[Stephen Boyd]

Quoting Martin Blumenstingl (2021-10-16 03:50:22)
> Update clk_composite_determine_rate() to use rate_ops.determine_rate
> when available in combination with a mux. So far clk_divider_ops provide
> both, .round_rate and .determine_rate. Removing the former would make
> clk-composite fail silently for example on Rockchip platforms (which
> heavily use composite clocks).
> Add support for using rate_ops.determine_rate when either
> rate_ops.round_rate is not available or both (.round_rate and
> .determine_rate) are provided.
> 
> Suggested-by: Alex Bee <knaerzche@gmail.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Applied to clk-next

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH clk-fixes v1 1/2] clk: composite: Also consider .determine_rate for rate + mux composites

[Alex Bee]

Hi Stephen,

Am 18.10.21 um 22:01 schrieb Stephen Boyd:
> Quoting Martin Blumenstingl (2021-10-16 03:50:21)
>> Commit 69a00fb3d69706 ("clk: divider: Implement and wire up
>> .determine_rate by default") switches clk_divider_ops to implement
>> .determine_rate by default. This breaks composite clocks with multiple
>> parents because clk-composite.c does not use the special handling for
>> mux + divider combinations anymore (that was restricted to rate clocks
>> which only implement .round_rate, but not .determine_rate).
>>
>> Alex reports:
>>   This breaks lot of clocks for Rockchip which intensively uses
>>   composites,  i.e. those clocks will always stay at the initial parent,
>>   which in some cases  is the XTAL clock and I strongly guess it is the
>>   same for other platforms,  which use composite clocks having more than
>>   one parent (e.g. mediatek, ti ...)
>>
>>   Example (RK3399)
>>   clk_sdio is set (initialized) with XTAL (24 MHz) as parent in u-boot.
>>   It will always stay at this parent, even if the mmc driver sets a rate
>>   of  200 MHz (fails, as the nature of things), which should switch it
>>   to   any of its possible parent PLLs defined in
>>   mux_pll_src_cpll_gpll_npll_ppll_upll_24m_p (see clk-rk3399.c)  - which
>>   never happens.
>>
>> Restore the original behavior by changing the priority of the conditions
>> inside clk-composite.c. Now the special rate + mux case (with rate_ops
>> having a .round_rate - which is still the case for the default
>> clk_divider_ops) is preferred over rate_ops which have .determine_rate
>> defined (and not further considering the mux).
>>
>> Fixes: 69a00fb3d69706 ("clk: divider: Implement and wire up .determine_rate by default")
>> Reported-by: Alex Bee <knaerzche@gmail.com>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
> 
> Applied to clk-fixes

any chance this can make it in 5.15?

Regards,
Alex
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH clk-fixes v1 1/2] clk: composite: Also consider .determine_rate for rate + mux composites

[Stephen Boyd]

Quoting Alex Bee (2021-10-27 15:47:03)
> > 
> > Applied to clk-fixes
> 
> any chance this can make it in 5.15?
> 

That's the plan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel