[PATCH v2 1/2] firmware: arm_scmi: Keep the discrete clock rates sorted

[PATCH v2 1/2] firmware: arm_scmi: Keep the discrete clock rates sorted

[Sudeep Holla]

Instead of relying on the firmware to keep the clock rates sorted, let
us sort the list. This is not essential for clock layer but it helps
to find the min and max rates easily from the list.

Link: https://lore.kernel.org/r/20200708110725.18017-1-sudeep.holla@arm.com
Fixes: 5f6c6430e904 ("firmware: arm_scmi: add initial support for clock protocol")
Reported-by: Dien Pham <dien.pham.ry@renesas.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)


Hi Dien-san,

If you could review/test these patches, I can queue them ASAP.
I am planning to send the PR for ARM SoC later this week, so I need
your tested-by.

v1[1]->v2:
	- Fixed the warning, sent the wrong version earlier

Regards,
Sudeep

[1] https://lore.kernel.org/r/20200708110725.18017-1-sudeep.holla@arm.com

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 4c2227662b26..c90f23a812f5 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -5,6 +5,8 @@
  * Copyright (C) 2018 ARM Ltd.
  */

+#include <linux/sort.h>
+
 #include "common.h"

 enum scmi_clock_protocol_cmd {
@@ -121,6 +123,13 @@ static int scmi_clock_attributes_get(const struct scmi_handle *handle,
 	return ret;
 }

+static int rate_cmp_func(const void *_r1, const void *_r2)
+{
+	const u64 *r1 = _r1, *r2 = _r2;
+
+	return r1 - r2;
+}
+
 static int
 scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
 			      struct scmi_clock_info *clk)
@@ -184,8 +193,10 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
 		 */
 	} while (num_returned && num_remaining);

-	if (rate_discrete)
+	if (rate_discrete) {
 		clk->list.num_rates = tot_rate_cnt;
+		sort(rate, tot_rate_cnt, sizeof(*rate), rate_cmp_func, NULL);
+	}

 	clk->rate_discrete = rate_discrete;

--
2.17.1


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

[PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates

[Sudeep Holla]

Currently we are not initializing the scmi clock with discrete rates
correctly. We fetch the min_rate and max_rate value only for clocks with
ranges and ignore the ones with discrete rates. This will lead to wrong
initialization of rate range when clock supports discrete rate.

Fix this by using the first and the last rate in the sorted list of the
discrete clock rates while registering the clock.

Link: https://lore.kernel.org/r/20200708110725.18017-2-sudeep.holla@arm.com
Fixes: 6d6a1d82eaef7 ("clk: add support for clocks provided by SCMI")
Reported-by: Dien Pham <dien.pham.ry@renesas.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/clk/clk-scmi.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Hi Stephen,

If you are fine, I can take this via ARM SoC along with the change in
firmware driver. However it is also fine if you want to merge this
independently as there is no strict dependency. Let me know either way.

v1[1]->v2:
	- Fixed the missing ; which was sent by mistake.

Regards,
Sudeep

[1] https://lore.kernel.org/r/20200708110725.18017-2-sudeep.holla@arm.com

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index c491f5de0f3f..c754dfbb73fd 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -103,6 +103,8 @@ static const struct clk_ops scmi_clk_ops = {
 static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
 {
 	int ret;
+	unsigned long min_rate, max_rate;
+
 	struct clk_init_data init = {
 		.flags = CLK_GET_RATE_NOCACHE,
 		.num_parents = 0,
@@ -112,9 +114,23 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
 
 	sclk->hw.init = &init;
 	ret = devm_clk_hw_register(dev, &sclk->hw);
-	if (!ret)
-		clk_hw_set_rate_range(&sclk->hw, sclk->info->range.min_rate,
-				      sclk->info->range.max_rate);
+	if (ret)
+		return ret;
+
+	if (sclk->info->rate_discrete) {
+		int num_rates = sclk->info->list.num_rates;
+
+		if (num_rates <= 0)
+			return -EINVAL;
+
+		min_rate = sclk->info->list.rates[0];
+		max_rate = sclk->info->list.rates[num_rates - 1];
+	} else {
+		min_rate = sclk->info->range.min_rate;
+		max_rate = sclk->info->range.max_rate;
+	}
+
+	clk_hw_set_rate_range(&sclk->hw, min_rate, max_rate);
 	return ret;
 }
 
-- 
2.17.1


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

RE: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates

[Dien Pham]

Hi Sudeep,

Thanks for your patch.

>-----Original Message-----
>From: Sudeep Holla <sudeep.holla@arm.com> 
>Sent: Thursday, July 9, 2020 3:17 PM
>To: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; Stephen Boyd <sboyd@kernel.org>
>Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-kernel@vger.kernel.org; Michael Turquette <mturquette@baylibre.com>; Dien Pham <dien.pham.ry@renesas.com>
>Subject: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates
>
>Currently we are not initializing the scmi clock with discrete rates correctly. We fetch the min_rate and max_rate value only for clocks with ranges and ignore the ones with discrete rates. This will lead to wrong initialization of rate range when clock supports discrete rate.
>
>Fix this by using the first and the last rate in the sorted list of the discrete clock rates while registering the clock.
>
>Link: https://lore.kernel.org/r/20200708110725.18017-2-sudeep.holla@arm.com
>Fixes: 6d6a1d82eaef7 ("clk: add support for clocks provided by SCMI")
>Reported-by: Dien Pham <dien.pham.ry@renesas.com>
>Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>---
> drivers/clk/clk-scmi.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
>Hi Stephen,
>
>If you are fine, I can take this via ARM SoC along with the change in firmware driver. However it is also fine if you want to merge this independently as there is no strict dependency. Let me know either way.
>
>v1[1]->v2:
>	- Fixed the missing ; which was sent by mistake.

I tested the patch,
I is ok and can fix my issue.

>Regards,
>Sudeep
>
>[1] https://lore.kernel.org/r/20200708110725.18017-2-sudeep.holla@arm.com
>
>diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index c491f5de0f3f..c754dfbb73fd 100644
>--- a/drivers/clk/clk-scmi.c
>+++ b/drivers/clk/clk-scmi.c
>@@ -103,6 +103,8 @@ static const struct clk_ops scmi_clk_ops = {  static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)  {
> 	int ret;
>+	unsigned long min_rate, max_rate;
>+
> 	struct clk_init_data init = {
> 		.flags = CLK_GET_RATE_NOCACHE,
> 		.num_parents = 0,
>@@ -112,9 +114,23 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
> 
> 	sclk->hw.init = &init;
> 	ret = devm_clk_hw_register(dev, &sclk->hw);
>-	if (!ret)
>-		clk_hw_set_rate_range(&sclk->hw, sclk->info->range.min_rate,
>-				      sclk->info->range.max_rate);
>+	if (ret)
>+		return ret;
>+
>+	if (sclk->info->rate_discrete) {
>+		int num_rates = sclk->info->list.num_rates;
>+
>+		if (num_rates <= 0)
>+			return -EINVAL;
>+
>+		min_rate = sclk->info->list.rates[0];
>+		max_rate = sclk->info->list.rates[num_rates - 1];
>+	} else {
>+		min_rate = sclk->info->range.min_rate;
>+		max_rate = sclk->info->range.max_rate;
>+	}
>+
>+	clk_hw_set_rate_range(&sclk->hw, min_rate, max_rate);
> 	return ret;
> }
> 
>--
>2.17.1

Best regard,
DIEN Pham

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

Re: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates

[Sudeep Holla]

On Thu, Jul 09, 2020 at 08:26:54AM +0000, Dien Pham wrote:
> Hi Sudeep,
>
> Thanks for your patch.
>
> >-----Original Message-----
> >From: Sudeep Holla <sudeep.holla@arm.com>
> >Sent: Thursday, July 9, 2020 3:17 PM
> >To: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; Stephen Boyd <sboyd@kernel.org>
> >Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-kernel@vger.kernel.org; Michael Turquette <mturquette@baylibre.com>; Dien Pham <dien.pham.ry@renesas.com>
> >Subject: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates
> >
> >Currently we are not initializing the scmi clock with discrete rates correctly. We fetch the min_rate and max_rate value only for clocks with ranges and ignore the ones with discrete rates. This will lead to wrong initialization of rate range when clock supports discrete rate.
> >
> >Fix this by using the first and the last rate in the sorted list of the discrete clock rates while registering the clock.
> >
> >Link: https://lore.kernel.org/r/20200708110725.18017-2-sudeep.holla@arm.com
> >Fixes: 6d6a1d82eaef7 ("clk: add support for clocks provided by SCMI")
> >Reported-by: Dien Pham <dien.pham.ry@renesas.com>
> >Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >---
> > drivers/clk/clk-scmi.c | 22 +++++++++++++++++++---
> > 1 file changed, 19 insertions(+), 3 deletions(-)
> >
> >Hi Stephen,
> >
> >If you are fine, I can take this via ARM SoC along with the change in firmware driver. However it is also fine if you want to merge this independently as there is no strict dependency. Let me know either way.
> >
> >v1[1]->v2:
> >	- Fixed the missing ; which was sent by mistake.
>
> I tested the patch,
> I is ok and can fix my issue.
>

Thanks for testing. Can I add ?

Tested-by: Dien Pham <dien.pham.ry@renesas.com>

--
Regards,
Sudeep

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

RE: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates

[Dien Pham]

Dear Sudeep-san,

>-----Original Message-----
>From: Sudeep Holla <sudeep.holla@arm.com> 
>Sent: Thursday, July 9, 2020 3:39 PM
>To: Dien Pham <dien.pham.ry@renesas.com>
>Cc: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; Stephen Boyd <sboyd@kernel.org>; linux-kernel@vger.kernel.org; Michael Turquette <mturquette@baylibre.com>; Sudeep Holla <sudeep.holla@arm.com>
>Subject: Re: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates
>
>On Thu, Jul 09, 2020 at 08:26:54AM +0000, Dien Pham wrote:
>> Hi Sudeep,
>>
>> Thanks for your patch.
>>
>> >-----Original Message-----
>> >From: Sudeep Holla <sudeep.holla@arm.com>
>> >Sent: Thursday, July 9, 2020 3:17 PM
>> >To: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; Stephen Boyd <sboyd@kernel.org>
>> >Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-kernel@vger.kernel.org; Michael Turquette <mturquette@baylibre.com>; Dien Pham <dien.pham.ry@renesas.com>
>> >Subject: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates
>> >
>> >Currently we are not initializing the scmi clock with discrete rates correctly. We fetch the min_rate and max_rate value only for clocks with ranges and ignore the ones with discrete rates. This will lead to wrong initialization of rate range when clock supports discrete rate.
>> >
>> >Fix this by using the first and the last rate in the sorted list of the discrete clock rates while registering the clock.
>> >
>> >Link: https://lore.kernel.org/r/20200708110725.18017-2-sudeep.holla@arm.com
>> >Fixes: 6d6a1d82eaef7 ("clk: add support for clocks provided by SCMI")
>> >Reported-by: Dien Pham <dien.pham.ry@renesas.com>
>> >Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> >---
>> > drivers/clk/clk-scmi.c | 22 +++++++++++++++++++---
>> > 1 file changed, 19 insertions(+), 3 deletions(-)
>> >
>> >Hi Stephen,
>> >
>> >If you are fine, I can take this via ARM SoC along with the change in firmware driver. However it is also fine if you want to merge this independently as there is no strict dependency. Let me know either way.
>> >
>> >v1[1]->v2:
>> >	- Fixed the missing ; which was sent by mistake.
>>
>> I tested the patch,
>> I is ok and can fix my issue.
>>
>
>Thanks for testing. Can I add ?
>
>Tested-by: Dien Pham <dien.pham.ry@renesas.com>

It is ok.

Thanks,
Best regard,
DIEN Pham

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

Re: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates

[Stephen Boyd]

Quoting Sudeep Holla (2020-07-09 01:17:05)
> Currently we are not initializing the scmi clock with discrete rates
> correctly. We fetch the min_rate and max_rate value only for clocks with
> ranges and ignore the ones with discrete rates. This will lead to wrong
> initialization of rate range when clock supports discrete rate.
> 
> Fix this by using the first and the last rate in the sorted list of the
> discrete clock rates while registering the clock.
> 
> Link: https://lore.kernel.org/r/20200708110725.18017-2-sudeep.holla@arm.com
> Fixes: 6d6a1d82eaef7 ("clk: add support for clocks provided by SCMI")
> Reported-by: Dien Pham <dien.pham.ry@renesas.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/clk/clk-scmi.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> Hi Stephen,
> 
> If you are fine, I can take this via ARM SoC along with the change in
> firmware driver. However it is also fine if you want to merge this
> independently as there is no strict dependency. Let me know either way.

I don't mind either way. If you want to send it in along with the
firmware change then that's fine.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

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

Re: [PATCH v2 2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates

[Sudeep Holla]

On Fri, Jul 10, 2020 at 04:50:40PM -0700, Stephen Boyd wrote:
> Quoting Sudeep Holla (2020-07-09 01:17:05)
> > Currently we are not initializing the scmi clock with discrete rates
> > correctly. We fetch the min_rate and max_rate value only for clocks with
> > ranges and ignore the ones with discrete rates. This will lead to wrong
> > initialization of rate range when clock supports discrete rate.
> > 
> > Fix this by using the first and the last rate in the sorted list of the
> > discrete clock rates while registering the clock.
> > 
> > Link: https://lore.kernel.org/r/20200708110725.18017-2-sudeep.holla@arm.com
> > Fixes: 6d6a1d82eaef7 ("clk: add support for clocks provided by SCMI")
> > Reported-by: Dien Pham <dien.pham.ry@renesas.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/clk/clk-scmi.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > Hi Stephen,
> > 
> > If you are fine, I can take this via ARM SoC along with the change in
> > firmware driver. However it is also fine if you want to merge this
> > independently as there is no strict dependency. Let me know either way.
> 
> I don't mind either way. If you want to send it in along with the
> firmware change then that's fine.
>

OK I have now queued and will send it to arm-soc.

> Reviewed-by: Stephen Boyd <sboyd@kernel.org>

Thanks for the review.

-- 
Regards,
Sudeep

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

Re: [PATCH v2 1/2] firmware: arm_scmi: Keep the discrete clock rates sorted

[Sudeep Holla]

On Thu, 9 Jul 2020 09:17:04 +0100, Sudeep Holla wrote:
> Instead of relying on the firmware to keep the clock rates sorted, let
> us sort the list. This is not essential for clock layer but it helps
> to find the min and max rates easily from the list.

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/2] firmware: arm_scmi: Keep the discrete clock rates sorted
      https://git.kernel.org/sudeep.holla/c/dccec73de9
[2/2] clk: scmi: Fix min and max rate when registering clocks with discrete rates
      https://git.kernel.org/sudeep.holla/c/fcd2e0deae

--
Regards,
Sudeep


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