[PATCH 0/3] clk: keystone: some minor fixes

[PATCH 0/3] clk: keystone: some minor fixes

[Tero Kristo]

Hi Santosh,

This series contains a few fixes for the TI SCI clock driver.
- Patch #1 is a clear bug fix, where we missed to parse assigned-clock
  data properly to detect which clocks are in use on the SoC.
- Patch #2 is a performance improvement patch which avoids some
  unnecessary round trips to firmware side while setting clock
  frequency.
- Patch #3 fixes some issues with set_rate passed to firmware, where the
  parameters are too strict; namely, firmware fails to handle some cases
  properly if min,tgt,max values for a clock rate are exactly the same
  value. Yeah, the firmware is quite weird here but nothing much else we
  can do from kernel side other than this....

-Tero


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/3] clk: keystone: sci-clk: fix parsing assigned-clock data during probe

[Tero Kristo]

The DT clock probe loop incorrectly terminates after processing "clocks"
only, fix this by re-starting the loop when all entries for current
DT property have been parsed.

Fixes: 8e48b33f9def ("clk: keystone: sci-clk: probe clocks from DT instead of firmware")
Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/keystone/sci-clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 2ad26cb927fd..f126b6045afa 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -522,7 +522,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
 		np = of_find_node_with_property(np, *clk_name);
 		if (!np) {
 			clk_name++;
-			break;
+			continue;
 		}
 
 		if (!of_device_is_available(np))
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/3] clk: keystone: sci-clk: cache results of last query rate operation

[Tero Kristo]

Cache results of the latest query rate operation. This optimizes the
firmware interface a bit, avoiding unnecessary calls to firmware if we
know the result already; the firmware interface is pretty expensive
to use for query rate functionality.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/keystone/sci-clk.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index f126b6045afa..b6477b08af04 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -54,6 +54,8 @@ struct sci_clk_provider {
  * @provider:	 Master clock provider
  * @flags:	 Flags for the clock
  * @node:	 Link for handling clocks probed via DT
+ * @cached_req:	 Cached requested freq for determine rate calls
+ * @cached_res:	 Cached result freq for determine rate calls
  */
 struct sci_clk {
 	struct clk_hw hw;
@@ -63,6 +65,8 @@ struct sci_clk {
 	struct sci_clk_provider *provider;
 	u8 flags;
 	struct list_head node;
+	unsigned long cached_req;
+	unsigned long cached_res;
 };
 
 #define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
@@ -175,6 +179,11 @@ static int sci_clk_determine_rate(struct clk_hw *hw,
 	int ret;
 	u64 new_rate;
 
+	if (clk->cached_req && clk->cached_req == req->rate) {
+		req->rate = clk->cached_res;
+		return 0;
+	}
+
 	ret = clk->provider->ops->get_best_match_freq(clk->provider->sci,
 						      clk->dev_id,
 						      clk->clk_id,
@@ -189,6 +198,9 @@ static int sci_clk_determine_rate(struct clk_hw *hw,
 		return ret;
 	}
 
+	clk->cached_req = req->rate;
+	clk->cached_res = new_rate;
+
 	req->rate = new_rate;
 
 	return 0;
@@ -249,6 +261,8 @@ static int sci_clk_set_parent(struct clk_hw *hw, u8 index)
 {
 	struct sci_clk *clk = to_sci_clk(hw);
 
+	clk->cached_req = 0;
+
 	return clk->provider->ops->set_parent(clk->provider->sci, clk->dev_id,
 					      clk->clk_id,
 					      index + 1 + clk->clk_id);
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 3/3] clk: keystone: sci-clk: add 10% slack to set_rate

[Tero Kristo]

Currently, we request exact clock rates from the firmware to be set with
set_rate. Due to some rounding errors and internal functionality of the
firmware itself, this can fail. Thus, add some slack to the set_rate
functionality so that we are always guaranteed to pass. The firmware
always attempts to use frequency as close to the target freq as
possible despite the slack given here.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/keystone/sci-clk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index b6477b08af04..aaf31abe1c8f 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -221,7 +221,8 @@ static int sci_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	struct sci_clk *clk = to_sci_clk(hw);
 
 	return clk->provider->ops->set_freq(clk->provider->sci, clk->dev_id,
-					    clk->clk_id, rate, rate, rate);
+					    clk->clk_id, rate / 10 * 9, rate,
+					    rate / 10 * 11);
 }
 
 /**
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 0/3] clk: keystone: some minor fixes

[santosh.shilimkar]

On 9/7/20 1:57 AM, Tero Kristo wrote:
> Hi Santosh,
> 
> This series contains a few fixes for the TI SCI clock driver.
> - Patch #1 is a clear bug fix, where we missed to parse assigned-clock
>    data properly to detect which clocks are in use on the SoC.
> - Patch #2 is a performance improvement patch which avoids some
>    unnecessary round trips to firmware side while setting clock
>    frequency.
> - Patch #3 fixes some issues with set_rate passed to firmware, where the
>    parameters are too strict; namely, firmware fails to handle some cases
>    properly if min,tgt,max values for a clock rate are exactly the same
>    value. Yeah, the firmware is quite weird here but nothing much else we
>    can do from kernel side other than this....
> 
Looks fine to me Tero.

Acked-by: Santosh Shilimkar <ssantosh@kernel.org>


Hi Stephen, Mike,
Can you please pick these fixes via clk tree ?

Re: [PATCH 0/3] clk: keystone: some minor fixes

[Stephen Boyd]

Quoting santosh.shilimkar@oracle.com (2020-09-08 10:19:32)
> 
> 
> On 9/7/20 1:57 AM, Tero Kristo wrote:
> > Hi Santosh,
> > 
> > This series contains a few fixes for the TI SCI clock driver.
> > - Patch #1 is a clear bug fix, where we missed to parse assigned-clock
> >    data properly to detect which clocks are in use on the SoC.
> > - Patch #2 is a performance improvement patch which avoids some
> >    unnecessary round trips to firmware side while setting clock
> >    frequency.
> > - Patch #3 fixes some issues with set_rate passed to firmware, where the
> >    parameters are too strict; namely, firmware fails to handle some cases
> >    properly if min,tgt,max values for a clock rate are exactly the same
> >    value. Yeah, the firmware is quite weird here but nothing much else we
> >    can do from kernel side other than this....
> > 
> Looks fine to me Tero.
> 
> Acked-by: Santosh Shilimkar <ssantosh@kernel.org>
> 
> 
> Hi Stephen, Mike,
> Can you please pick these fixes via clk tree ?

Sure. I assume this is -next material and not critical fixes.

Re: [PATCH 0/3] clk: keystone: some minor fixes

[santosh.shilimkar]

On 9/10/20 2:29 PM, Stephen Boyd wrote:
> Quoting santosh.shilimkar@oracle.com (2020-09-08 10:19:32)
>>
>>
>> On 9/7/20 1:57 AM, Tero Kristo wrote:
>>> Hi Santosh,
>>>
>>> This series contains a few fixes for the TI SCI clock driver.
>>> - Patch #1 is a clear bug fix, where we missed to parse assigned-clock
>>>     data properly to detect which clocks are in use on the SoC.
>>> - Patch #2 is a performance improvement patch which avoids some
>>>     unnecessary round trips to firmware side while setting clock
>>>     frequency.
>>> - Patch #3 fixes some issues with set_rate passed to firmware, where the
>>>     parameters are too strict; namely, firmware fails to handle some cases
>>>     properly if min,tgt,max values for a clock rate are exactly the same
>>>     value. Yeah, the firmware is quite weird here but nothing much else we
>>>     can do from kernel side other than this....
>>>
>> Looks fine to me Tero.
>>
>> Acked-by: Santosh Shilimkar <ssantosh@kernel.org>
>>
>>
>> Hi Stephen, Mike,
>> Can you please pick these fixes via clk tree ?
> 
> Sure. I assume this is -next material and not critical fixes.
> 
Yep.

Re: [PATCH 1/3] clk: keystone: sci-clk: fix parsing assigned-clock data during probe

[Stephen Boyd]

Quoting Tero Kristo (2020-09-07 01:57:38)
> The DT clock probe loop incorrectly terminates after processing "clocks"
> only, fix this by re-starting the loop when all entries for current
> DT property have been parsed.
> 
> Fixes: 8e48b33f9def ("clk: keystone: sci-clk: probe clocks from DT instead of firmware")
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---

Applied to clk-next

Re: [PATCH 2/3] clk: keystone: sci-clk: cache results of last query rate operation

[Stephen Boyd]

Quoting Tero Kristo (2020-09-07 01:57:39)
> Cache results of the latest query rate operation. This optimizes the
> firmware interface a bit, avoiding unnecessary calls to firmware if we
> know the result already; the firmware interface is pretty expensive
> to use for query rate functionality.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---

Applied to clk-next

Re: [PATCH 3/3] clk: keystone: sci-clk: add 10% slack to set_rate

[Stephen Boyd]

Quoting Tero Kristo (2020-09-07 01:57:40)
> Currently, we request exact clock rates from the firmware to be set with
> set_rate. Due to some rounding errors and internal functionality of the
> firmware itself, this can fail. Thus, add some slack to the set_rate
> functionality so that we are always guaranteed to pass. The firmware
> always attempts to use frequency as close to the target freq as
> possible despite the slack given here.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---

Applied to clk-next