Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1186046 > unrolled thread

Re: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System Control Processor)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System  Control Processor) Sudeep Holla <sudeep.holla@arm.com> - 2015-07-16 18:20 +0200
    Re: [PATCH v4 3/8] clk: add support for clocks provided by  SCP(System Control Processor) Stephen Boyd <sboyd@codeaurora.org> - 2015-07-16 21:40 +0200
      Re: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System  Control Processor) Sudeep Holla <sudeep.holla@arm.com> - 2015-07-17 13:20 +0200
        Re: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System  Control Processor) Sudeep Holla <sudeep.holla@arm.com> - 2015-07-22 16:20 +0200

#1186046 — Re: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System Control Processor)

Hi Stephen,

On 08/07/15 02:46, Stephen Boyd wrote:
> On 07/07, Sudeep Holla wrote:
>>
>>
>> On 06/07/15 20:52, Stephen Boyd wrote:
>>>>
>>>
>>> If I have time I may try to start doing the clk_register() conversion,
>>> but it will take a while so I doubt it will be in v4.3. I'm asking if
>>> you can add a clk_hw based API that does something like
>>> clk_set_rate_range() without requiring a struct clk pointer. i.e.
>>> clk_hw_set_rate_range(struct clk_hw *hw, min, max) that constraints the
>>> min/max rate of the clock. This way, the driver is only using clk
>>> provider APIs and not clk consumer APIs.
>>>
>>
>> I understand the intention of separating clk provider helpers/APIs
>> and clk consumer APIs. Since {min,max}_rate are part of struct clk
>> itself, I was thinking that you would have moved it to struct clk_core
>> as part of the rework you mentioned and hence asked about the patches.
>>
>> IIUC, if {min,max}_rate remain part of struct clk, then how are we
>> restricting that operation to just the clk providers ? clk consumer
>> can still directly modify or use clk_set_rate_range.
>>
>> Do we continue to provide that feature for both provider and consumer ?
>> If so I assume {min,max}_rate range requested by consumer should be
>> within the limits set by provider and do we maintain both the limits ?
>>
>> Sorry if I am missing something fundamental since I don't have much
>> knowledge of clk layer internals.
>>
>
> Yes struct clk would have min/max, and struct clk_core would have
> min/max. Then some sort of provider API (or possibly even
> clk_init_data) would take the min/max fields and copy them over
> to struct clk_core. Then during set_rate operations we would
> aggregate the constraints from struct clk like we already do and
> add in the constrains in struct clk_core.
>
> One downside to adding new fields to clk_init_data is that there
> are drivers out there that aren't initializing that structure to
> 0, and they're putting it on the stack, so stack junk can come
> through. Furthermore, min/max would mean that every driver needs
> to specify some large number for max or we have to special case
> min == max == 0 and ignore it. Somehow it needs to be opt-in. If
> we want to go down the clk_init_data route then perhaps we need
> some sort of rate_constraint struct pointer in there that drivers
> can optionally setup.
>
> 	struct clk_rate_constraint {
> 		unsigned long min;
> 		unsigned long max;
> 	};
>
> 	struct clk_init_data {
> 		...
> 		struct clk_rate_constraint *rate_constraint;
> 	};
>
> I haven't thought it through completely, but I can probably write
> up some patch tomorrow after I sleep on it.
>

I am hoping to get this series for v4.3. In order to avoid using
consumer API, I can revert back to the min,max check I had in the
round_rate earlier if that's fine with you ? Let me know so that I can
post the next version based on that. All the other comments are already
addressed.

Also since this series depends on SCPI, I was thinking to get it merged
via ARM-SoC, but that might conflict with the round_rate prototype
change. Do do plan to share a stable base with arm-soc guys or you
expect all the changes to be contained in clk tree ?

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1186214 — Re: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System Control Processor)

On 07/16, Sudeep Holla wrote:
> On 08/07/15 02:46, Stephen Boyd wrote:
> >
> >Yes struct clk would have min/max, and struct clk_core would have
> >min/max. Then some sort of provider API (or possibly even
> >clk_init_data) would take the min/max fields and copy them over
> >to struct clk_core. Then during set_rate operations we would
> >aggregate the constraints from struct clk like we already do and
> >add in the constrains in struct clk_core.
> >
> >One downside to adding new fields to clk_init_data is that there
> >are drivers out there that aren't initializing that structure to
> >0, and they're putting it on the stack, so stack junk can come
> >through. Furthermore, min/max would mean that every driver needs
> >to specify some large number for max or we have to special case
> >min == max == 0 and ignore it. Somehow it needs to be opt-in. If
> >we want to go down the clk_init_data route then perhaps we need
> >some sort of rate_constraint struct pointer in there that drivers
> >can optionally setup.
> >
> >	struct clk_rate_constraint {
> >		unsigned long min;
> >		unsigned long max;
> >	};
> >
> >	struct clk_init_data {
> >		...
> >		struct clk_rate_constraint *rate_constraint;
> >	};
> >
> >I haven't thought it through completely, but I can probably write
> >up some patch tomorrow after I sleep on it.
> >
> 
> I am hoping to get this series for v4.3. In order to avoid using
> consumer API, I can revert back to the min,max check I had in the
> round_rate earlier if that's fine with you ? Let me know so that I can
> post the next version based on that. All the other comments are already
> addressed.

Ok. I'm fine with the consumer API being used, but it would be
nice if we didn't have to do so. Try out the patch below,
hopefully it's good enough for your purposes. It may need to be
more robust, and we may still want to use the init_data structure
to avoid races with providers and consumers, but we can leave
that for later after sweeping all the structure users.

> 
> Also since this series depends on SCPI, I was thinking to get it merged
> via ARM-SoC, but that might conflict with the round_rate prototype
> change. Do do plan to share a stable base with arm-soc guys or you
> expect all the changes to be contained in clk tree ?
> 

We can share a stable branch for the determine_rate change with
arm-soc. We already have it on a separate branch but haven't
published it so far because nobody has asked.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

-----8<------
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a1d34a2ed9c6..8760b743bb70 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -58,6 +58,8 @@ struct clk_core {
 	unsigned long		flags;
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
+	unsigned long		min_rate;
+	unsigned long		max_rate;
 	unsigned long		accuracy;
 	int			phase;
 	struct hlist_head	children;
@@ -512,8 +514,8 @@ static void clk_core_get_boundaries(struct clk_core *core,
 {
 	struct clk *clk_user;
 
-	*min_rate = 0;
-	*max_rate = ULONG_MAX;
+	*min_rate = core->min_rate;
+	*max_rate = core->max_rate;
 
 	hlist_for_each_entry(clk_user, &core->clks, clks_node)
 		*min_rate = max(*min_rate, clk_user->min_rate);
@@ -522,6 +524,13 @@ static void clk_core_get_boundaries(struct clk_core *core,
 		*max_rate = min(*max_rate, clk_user->max_rate);
 }
 
+void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
+			   unsigned long max_rate)
+{
+	hw->core->min_rate = min_rate;
+	hw->core->max_rate = max_rate;
+}
+
 /*
  * Helper for finding best parent to provide a given frequency. This can be used
  * directly as a determine_rate callback (e.g. for a mux), or from a more
@@ -2496,6 +2505,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	core->hw = hw;
 	core->flags = hw->init->flags;
 	core->num_parents = hw->init->num_parents;
+	core->min_rate = 0;
+	core->max_rate = ULONG_MAX;
 	hw->core = core;
 
 	/* allocate local copy in case parent_names is __initdata */
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 2116e2b8a5f2..d62e7eab1dbe 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -619,6 +619,8 @@ int __clk_determine_rate(struct clk_hw *core, struct clk_rate_request *req);
 int __clk_mux_determine_rate_closest(struct clk_hw *hw,
 				     struct clk_rate_request *req);
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
+void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
+			   unsigned long max_rate);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186717

On 16/07/15 20:31, Stephen Boyd wrote:
> On 07/16, Sudeep Holla wrote:
>> On 08/07/15 02:46, Stephen Boyd wrote:
>>>
>>> Yes struct clk would have min/max, and struct clk_core would have
>>> min/max. Then some sort of provider API (or possibly even
>>> clk_init_data) would take the min/max fields and copy them over
>>> to struct clk_core. Then during set_rate operations we would
>>> aggregate the constraints from struct clk like we already do and
>>> add in the constrains in struct clk_core.
>>>
>>> One downside to adding new fields to clk_init_data is that there
>>> are drivers out there that aren't initializing that structure to
>>> 0, and they're putting it on the stack, so stack junk can come
>>> through. Furthermore, min/max would mean that every driver needs
>>> to specify some large number for max or we have to special case
>>> min == max == 0 and ignore it. Somehow it needs to be opt-in. If
>>> we want to go down the clk_init_data route then perhaps we need
>>> some sort of rate_constraint struct pointer in there that drivers
>>> can optionally setup.
>>>
>>> 	struct clk_rate_constraint {
>>> 		unsigned long min;
>>> 		unsigned long max;
>>> 	};
>>>
>>> 	struct clk_init_data {
>>> 		...
>>> 		struct clk_rate_constraint *rate_constraint;
>>> 	};
>>>
>>> I haven't thought it through completely, but I can probably write
>>> up some patch tomorrow after I sleep on it.
>>>
>>
>> I am hoping to get this series for v4.3. In order to avoid using
>> consumer API, I can revert back to the min,max check I had in the
>> round_rate earlier if that's fine with you ? Let me know so that I can
>> post the next version based on that. All the other comments are already
>> addressed.
>
> Ok. I'm fine with the consumer API being used, but it would be
> nice if we didn't have to do so. Try out the patch below,
> hopefully it's good enough for your purposes. It may need to be
> more robust, and we may still want to use the init_data structure
> to avoid races with providers and consumers, but we can leave
> that for later after sweeping all the structure users.
>

Agreed, I would avoid using clk consumer API or use it with TODO so that
I remember to remove it soon. Anyways, thanks for the patch, I tested it
and works fine to me. You can add Tested-by if you decide to push it.

>>
>> Also since this series depends on SCPI, I was thinking to get it merged
>> via ARM-SoC, but that might conflict with the round_rate prototype
>> change. Do do plan to share a stable base with arm-soc guys or you
>> expect all the changes to be contained in clk tree ?
>>
>
> We can share a stable branch for the determine_rate change with
> arm-soc. We already have it on a separate branch but haven't
> published it so far because nobody has asked.
>

determine_rate change shouldn't affect SCPI clock driver but I remember
seeing round_rate change too on the list which returns value using the
argument from Boris. Is that planned for v4.3 ? I would need the stable
branch from this clk_hw_set_rate_range if you decide to push. Let me
know your preferences. I will post the updated version of the patch
accordingly.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1189996

On 21/07/15 19:05, Stephen Boyd wrote:
> On 07/20/2015 01:54 AM, Sudeep Holla wrote:
>>
>>
>> On 17/07/15 19:13, Stephen Boyd wrote:
>>> On 07/17/2015 04:17 AM, Sudeep Holla wrote:
>>
>> [...]
>>
>>>>
>>>> determine_rate change shouldn't affect SCPI clock driver but I remember
>>>> seeing round_rate change too on the list which returns value using the
>>>> argument from Boris. Is that planned for v4.3 ? I would need the stable
>>>> branch from this clk_hw_set_rate_range if you decide to push. Let me
>>>> know your preferences. I will post the updated version of the patch
>>>> accordingly.
>>>>
>>>
>>> We're not going to change round_rate() so it sounds like you don't need
>>> a stable branch. But you would need this new consumer API. So you still
>>> need a branch right?
>>>
>>
>> I am fine either way. If no one else need the stable branch to be shared
>> with arm-soc, I prefer to use clock consumer API for now to avoid all
>> the troubles to you guys and switch to provider API later. I will post
>> it once the both this driver and that provider API is merged, if that's
>> fine with you ?
>
> Ok. Sounds fine as long as we don't forget.
>

It's ready indeed :), will post it when everything is in place.

Regards,
Sudeep

--->8

 From 3ddf7fe5b1286efa712d4f2ba8108ce1855f3e74 Mon Sep 17 00:00:00 2001
From: Sudeep Holla <sudeep.holla@arm.com>
Date: Fri, 17 Jul 2015 12:19:54 +0100
Subject: [PATCH] clk: scpi: use provider clk_hw_set_rate_range API 
instead of
  consumer API

SCPI clock provider uses the consumer API clk_set_rate_range to set
min/max rate constraints on the clocks it provides.

Commit c87c640f915a ("clk: Allow providers to configure min/max rates")
added a separate provider API clk_hw_set_rate_range to set the min/max
rate of a clock.

This patch switches away from clk_set_rate_range consumer API to
clk_hw_set_rate_range provider API.

Cc: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
  drivers/clk/clk-scpi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
index 06d0558e8f46..f7c8b7a03cfe 100644
--- a/drivers/clk/clk-scpi.c
+++ b/drivers/clk/clk-scpi.c
@@ -174,7 +174,7 @@ scpi_clk_ops_init(struct device *dev, const struct 
of_device_id *match,

  	clk = devm_clk_register(dev, &sclk->hw);
  	if (!IS_ERR(clk) && max)
-		clk_set_rate_range(clk, min, max);
+		clk_hw_set_rate_range(&sclk->hw, min, max);
  	return clk;
  }

-- 
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web