| Message ID | 20230519190522.194729-2-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Fix 64 bit issues in common clock framework | expand |
Hi Sebastian, On Fri, 2023-05-19 at 21:05 +0200, Sebastian Reichel wrote: > ULONG_MAX is used by a few drivers to figure out the highest available > clock rate via clk_round_rate(clk, ULONG_MAX). Since abs() takes a > signed value as input, the current logic effectively calculates with > ULONG_MAX = -1, which results in the worst parent clock being chosen > instead of the best one. > > For example on Rockchip RK3588 the eMMC driver tries to figure out > the highest available clock rate. There are three parent clocks > available resulting in the following rate diffs with the existing > logic: > > GPLL: abs(18446744073709551615 - 1188000000) = 1188000001 > CPLL: abs(18446744073709551615 - 1500000000) = 1500000001 > XIN24M: abs(18446744073709551615 - 24000000) = 24000001 > > As a result the clock framework will promote a maximum supported > clock rate of 24 MHz, even though 1.5GHz are possible. With the > updated logic any casting between signed and unsigned is avoided > and the numbers look like this instead: > > GPLL: 18446744073709551615 - 1188000000 = 18446744072521551615 > CPLL: 18446744073709551615 - 1500000000 = 18446744072209551615 > XIN24M: 18446744073709551615 - 24000000 = 18446744073685551615 > > As a result the parent with the highest acceptable rate is chosen > instead of the parent clock with the lowest one. > > Cc: stable@vger.kernel.org > Fixes: 49502408007b ("mmc: sdhci-of-dwcmshc: properly determine max clock on Rockchip") > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> This patch series fixes the error on Rockchip RK3588 on ROCK 5 Model B. Tested-by: Christopher Obbard <chris.obbard@collabora.com> > --- > drivers/clk/clk-composite.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c > index edfa94641bbf..66759fe28fad 100644 > --- a/drivers/clk/clk-composite.c > +++ b/drivers/clk/clk-composite.c > @@ -119,7 +119,10 @@ static int clk_composite_determine_rate(struct clk_hw *hw, > if (ret) > continue; > > - rate_diff = abs(req->rate - tmp_req.rate); > + if (req->rate >= tmp_req.rate) > + rate_diff = req->rate - tmp_req.rate; > + else > + rate_diff = tmp_req.rate - req->rate; > > if (!rate_diff || !req->best_parent_hw > || best_rate_diff > rate_diff) { > -- > 2.39.2 > >
On Sat, May 20, 2023 at 08:57:16AM +0100, Christopher Obbard wrote: > On Fri, 2023-05-19 at 21:05 +0200, Sebastian Reichel wrote: ... > > - rate_diff = abs(req->rate - tmp_req.rate); > > + if (req->rate >= tmp_req.rate) > > + rate_diff = req->rate - tmp_req.rate; > > + else > > + rate_diff = tmp_req.rate - req->rate; Btw, we have abs_diff() helper for this kind of cases.
diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c index edfa94641bbf..66759fe28fad 100644 --- a/drivers/clk/clk-composite.c +++ b/drivers/clk/clk-composite.c @@ -119,7 +119,10 @@ static int clk_composite_determine_rate(struct clk_hw *hw, if (ret) continue; - rate_diff = abs(req->rate - tmp_req.rate); + if (req->rate >= tmp_req.rate) + rate_diff = req->rate - tmp_req.rate; + else + rate_diff = tmp_req.rate - req->rate; if (!rate_diff || !req->best_parent_hw || best_rate_diff > rate_diff) {
ULONG_MAX is used by a few drivers to figure out the highest available clock rate via clk_round_rate(clk, ULONG_MAX). Since abs() takes a signed value as input, the current logic effectively calculates with ULONG_MAX = -1, which results in the worst parent clock being chosen instead of the best one. For example on Rockchip RK3588 the eMMC driver tries to figure out the highest available clock rate. There are three parent clocks available resulting in the following rate diffs with the existing logic: GPLL: abs(18446744073709551615 - 1188000000) = 1188000001 CPLL: abs(18446744073709551615 - 1500000000) = 1500000001 XIN24M: abs(18446744073709551615 - 24000000) = 24000001 As a result the clock framework will promote a maximum supported clock rate of 24 MHz, even though 1.5GHz are possible. With the updated logic any casting between signed and unsigned is avoided and the numbers look like this instead: GPLL: 18446744073709551615 - 1188000000 = 18446744072521551615 CPLL: 18446744073709551615 - 1500000000 = 18446744072209551615 XIN24M: 18446744073709551615 - 24000000 = 18446744073685551615 As a result the parent with the highest acceptable rate is chosen instead of the parent clock with the lowest one. Cc: stable@vger.kernel.org Fixes: 49502408007b ("mmc: sdhci-of-dwcmshc: properly determine max clock on Rockchip") Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- drivers/clk/clk-composite.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)