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

Commit Message

Sudeep Holla June 8, 2015, 10:39 a.m. UTC

On some ARM based systems, a separate Cortex-M based System Control
Processor(SCP) provides the overall power, clock, reset and system
control. System Control and Power Interface(SCPI) Message Protocol
is defined for the communication between the Application Cores(AP)
and the SCP.

This patch adds support for the clocks provided by SCP using SCPI
protocol.

Signed-off-by: Sudeep Holla <[e-mail]>
Cc: Mike Turquette <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Liviu Dudau <[e-mail]>
Cc: Lorenzo Pieralisi <[e-mail]>
Cc: Jon Medhurst (Tixy) <[e-mail]>
Cc: [e-mail]
---
 drivers/clk/Kconfig    |  10 ++
 drivers/clk/Makefile   |   1 +
 drivers/clk/clk-scpi.c | 286 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 297 insertions(+)
 create mode 100644 drivers/clk/clk-scpi.c

Comments

Stephen Boyd July 2, 2015, 5:23 p.m. UTC | #1

On 06/08, Sudeep Holla wrote:
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9897f353bf1a..0fe8daefc105 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
>  	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>  	  by control register.
>  
> +config COMMON_CLK_SCPI
> +        tristate "Clock driver controlled via SCPI interface"
> +        depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
> +        ---help---
> +          This driver provides support for clocks that are controlled
> +          by firmware that implements the SCPI interface.
> +
> +	  This driver uses SCPI Message Protocol to interact with the
> +	  firmware providing all the clock controls.

The tabbing is weird here. Both paragraphs should have the same
alignment.

> diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
> new file mode 100644
> index 000000000000..707b3430c55f
> --- /dev/null
> +++ b/drivers/clk/clk-scpi.c
> +
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/scpi_protocol.h>

Please include <linux/platform_device.h> as well.

> +
> +struct scpi_clk {
> +	u32 id;
> +	const char *name;

Do you need this? Or can you just use __clk_get_name() in places
where the name is used?

> +	struct clk_hw hw;
> +	struct scpi_dvfs_info *info;
> +	struct scpi_ops *scpi_ops;
> +};
> +
> +#define to_scpi_clk(clk) container_of(clk, struct scpi_clk, hw)
> +
> +static unsigned long scpi_clk_recalc_rate(struct clk_hw *hw,
> +					  unsigned long parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +
> +	return clk->scpi_ops->clk_get_val(clk->id);
> +}
> +
> +static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long *parent_rate)
> +{

Maybe a comment here like:

/*
 * We can't figure out what rate it will be, so just return the rate
 * back to the caller. scpi_clk_recalc_rate() will be called
 * after the rate is set and we'll know what rate the clock is
 * running at then.
 */

> +	return rate;
> +}
> +
> +static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +			     unsigned long parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +
> +	return clk->scpi_ops->clk_set_val(clk->id, rate);
> +}
> +
> +static void scpi_clk_disable(struct clk_hw *hw)
> +{
> +	scpi_clk_set_rate(hw, 0, 0);

Does this mean you have to set a rate to enable the clock? Are
you relying on drivers to call clk_set_rate() to implicitly
enable the clock? If so, it would be better to cache the rate of
the clock in set_rate if the clock isn't enabled in software and
then send the cached rate during enable.

> +}
> +
[..]
> +/* find closest match to given frequency in OPP table */
> +static int __scpi_dvfs_round_rate(struct scpi_clk *clk, unsigned long rate)
> +{
> +	int idx;
> +	u32 fmin = 0, fmax = ~0, ftmp;
> +	struct scpi_opp *opp = clk->info->opps;
> +

const?

> +	for (idx = 0; idx < clk->info->count; idx++, opp++) {
> +		ftmp = opp->freq;
> +		if (ftmp >= (u32)rate) {
> +			if (ftmp <= fmax)
> +				fmax = ftmp;
> +			break;
> +		} else if (ftmp >= fmin) {
> +			fmin = ftmp;
> +		}
> +	}
> +	return fmax != ~0 ? fmax : fmin;
> +}
> +
> +static unsigned long scpi_dvfs_recalc_rate(struct clk_hw *hw,
> +					   unsigned long parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +	int idx = clk->scpi_ops->dvfs_get_idx(clk->id);
> +	struct scpi_opp *opp;

const?

> +
> +	if (idx < 0)
> +		return 0;
> +
> +	opp = clk->info->opps + idx;
> +	return opp->freq;
> +}
> +
> +static long scpi_dvfs_round_rate(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long *parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +
> +	return __scpi_dvfs_round_rate(clk, rate);
> +}
> +
> +static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned long rate)
> +{
> +	int idx, max_opp = clk->info->count;
> +	struct scpi_opp *opp = clk->info->opps;

const?

> +
> +	for (idx = 0; idx < max_opp; idx++, opp++)
> +		if (opp->freq == rate)
> +			return idx;
> +	return -EINVAL;
> +}
> +
[..]
> +static struct clk *
> +scpi_clk_ops_init(struct device *dev, const struct of_device_id *match,
> +		  struct scpi_clk *sclk)
> +{
> +	struct clk_init_data init;
> +	struct clk *clk;
> +	unsigned long min = 0, max = 0;
> +
> +	init.name = sclk->name;
> +	init.flags = CLK_IS_ROOT;
> +	init.num_parents = 0;
> +	init.ops = match->data;
> +	sclk->hw.init = &init;
> +	sclk->scpi_ops = get_scpi_ops();
> +
> +	if (init.ops == &scpi_dvfs_ops) {
> +		sclk->info = sclk->scpi_ops->dvfs_get_info(sclk->id);
> +		if (IS_ERR(sclk->info))
> +			return NULL;
> +	} else if (init.ops == &scpi_clk_ops) {
> +		if (sclk->scpi_ops->clk_get_range(sclk->id, &min, &max) || !max)
> +			return NULL;
> +	} else {
> +		return NULL;
> +	}
> +
> +	clk = devm_clk_register(dev, &sclk->hw);
> +	if (!IS_ERR(clk) && max)
> +		clk_set_rate_range(clk, min, max);

Hm.. we're planning to make clk_register() return a struct
clk_hw, so this will block that. We need some sort of clk_hw API
that allows us to setup min/max limits on the clock from the
provider side. Care to add that?

Sudeep Holla July 3, 2015, 2:52 p.m. UTC | #2

Hi Stephen,


Thanks for the review.

On 02/07/15 18:23, Stephen Boyd wrote:
> On 06/08, Sudeep Holla wrote:
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index 9897f353bf1a..0fe8daefc105 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
>>   	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>>   	  by control register.
>>
>> +config COMMON_CLK_SCPI
>> +        tristate "Clock driver controlled via SCPI interface"
>> +        depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
>> +        ---help---
>> +          This driver provides support for clocks that are controlled
>> +          by firmware that implements the SCPI interface.
>> +
>> +	  This driver uses SCPI Message Protocol to interact with the
>> +	  firmware providing all the clock controls.
>
> The tabbing is weird here. Both paragraphs should have the same
> alignment.
>

Indeed, sorry for that, fixed now.

>> diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
>> new file mode 100644
>> index 000000000000..707b3430c55f
>> --- /dev/null
>> +++ b/drivers/clk/clk-scpi.c
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/scpi_protocol.h>
>
> Please include <linux/platform_device.h> as well.
>

Added now, didn't bother as of_platform.h includes it.

>> +
>> +struct scpi_clk {
>> +	u32 id;
>> +	const char *name;
>
> Do you need this? Or can you just use __clk_get_name() in places
> where the name is used?
>

Not used, so removed it.


[...]

>> +static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
>> +				unsigned long *parent_rate)
>> +{
>
> Maybe a comment here like:
>
> /*
>   * We can't figure out what rate it will be, so just return the rate
>   * back to the caller. scpi_clk_recalc_rate() will be called
>   * after the rate is set and we'll know what rate the clock is
>   * running at then.
>   */
>

Done

>> +	return rate;
>> +}
>> +
>> +static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>> +			     unsigned long parent_rate)
>> +{
>> +	struct scpi_clk *clk = to_scpi_clk(hw);
>> +
>> +	return clk->scpi_ops->clk_set_val(clk->id, rate);
>> +}
>> +
>> +static void scpi_clk_disable(struct clk_hw *hw)
>> +{
>> +	scpi_clk_set_rate(hw, 0, 0);
>
> Does this mean you have to set a rate to enable the clock? Are
> you relying on drivers to call clk_set_rate() to implicitly
> enable the clock? If so, it would be better to cache the rate of
> the clock in set_rate if the clock isn't enabled in software and
> then send the cached rate during enable.
>

Agreed, I have asked the firmware/SCPI specification guys about
more details on what to expect from firmware. Once they get back,
will update the code considering your feedback.

[...]

>> +static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned long rate)
>> +{
>> +	int idx, max_opp = clk->info->count;
>> +	struct scpi_opp *opp = clk->info->opps;
>
> const?
>

All the 3 above fixed now.

[...]

>> +
>> +	clk = devm_clk_register(dev, &sclk->hw);
>> +	if (!IS_ERR(clk) && max)
>> +		clk_set_rate_range(clk, min, max);
>
> Hm.. we're planning to make clk_register() return a struct
> clk_hw, so this will block that. We need some sort of clk_hw API
> that allows us to setup min/max limits on the clock from the
> provider side. Care to add that?
>

Can you provide pointer to the patches or the tree containing those
changes ? Are they targeted for v4.3 ?

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sudeep Holla July 3, 2015, 4:12 p.m. UTC | #3

On 03/07/15 15:52, Sudeep Holla wrote:

[...]

>>> +static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>>> +			     unsigned long parent_rate)
>>> +{
>>> +	struct scpi_clk *clk = to_scpi_clk(hw);
>>> +
>>> +	return clk->scpi_ops->clk_set_val(clk->id, rate);
>>> +}
>>> +
>>> +static void scpi_clk_disable(struct clk_hw *hw)
>>> +{
>>> +	scpi_clk_set_rate(hw, 0, 0);
>>
>> Does this mean you have to set a rate to enable the clock? Are
>> you relying on drivers to call clk_set_rate() to implicitly
>> enable the clock? If so, it would be better to cache the rate of
>> the clock in set_rate if the clock isn't enabled in software and
>> then send the cached rate during enable.
>>
>
> Agreed, I have asked the firmware/SCPI specification guys about
> more details on what to expect from firmware. Once they get back,
> will update the code considering your feedback.
>

OK, I got feedback and looks like it was never planned to implement that
and even specification needs to be fixed. So I will drop the disable
callback support. For now, we don't support {en,dis}able
features. We need to amend the specification to fix that.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd July 6, 2015, 7:52 p.m. UTC | #4

On 07/03/2015 07:52 AM, Sudeep Holla wrote:
> Hi Stephen,
>
>
> Thanks for the review.
>
> On 02/07/15 18:23, Stephen Boyd wrote:
>> On 06/08, Sudeep Holla wrote:
>>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>>> index 9897f353bf1a..0fe8daefc105 100644
>>> --- a/drivers/clk/Kconfig
>>> +++ b/drivers/clk/Kconfig
>>> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
>>>         clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>>>         by control register.
>>>
>>> +config COMMON_CLK_SCPI
>>> +        tristate "Clock driver controlled via SCPI interface"
>>> +        depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
>>> +        ---help---
>>> +          This driver provides support for clocks that are controlled
>>> +          by firmware that implements the SCPI interface.
>>> +
>>> +      This driver uses SCPI Message Protocol to interact with the
>>> +      firmware providing all the clock controls.
>>
>> The tabbing is weird here. Both paragraphs should have the same
>> alignment.
>>
>
> Indeed, sorry for that, fixed now.
>
>>> diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
>>> new file mode 100644
>>> index 000000000000..707b3430c55f
>>> --- /dev/null
>>> +++ b/drivers/clk/clk-scpi.c
>>> +
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/device.h>
>>> +#include <linux/err.h>
>>> +#include <linux/of.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/scpi_protocol.h>
>>
>> Please include <linux/platform_device.h> as well.
>>
>
> Added now, didn't bother as of_platform.h includes it.
>
>>> +
>>> +struct scpi_clk {
>>> +    u32 id;
>>> +    const char *name;
>>
>> Do you need this? Or can you just use __clk_get_name() in places
>> where the name is used?
>>
>
> Not used, so removed it.
>
>
> [...]
>
>>> +static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
>>> +                unsigned long *parent_rate)
>>> +{
>>
>> Maybe a comment here like:
>>
>> /*
>>   * We can't figure out what rate it will be, so just return the rate
>>   * back to the caller. scpi_clk_recalc_rate() will be called
>>   * after the rate is set and we'll know what rate the clock is
>>   * running at then.
>>   */
>>
>
> Done
>
>>> +    return rate;
>>> +}
>>> +
>>> +static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>>> +                 unsigned long parent_rate)
>>> +{
>>> +    struct scpi_clk *clk = to_scpi_clk(hw);
>>> +
>>> +    return clk->scpi_ops->clk_set_val(clk->id, rate);
>>> +}
>>> +
>>> +static void scpi_clk_disable(struct clk_hw *hw)
>>> +{
>>> +    scpi_clk_set_rate(hw, 0, 0);
>>
>> Does this mean you have to set a rate to enable the clock? Are
>> you relying on drivers to call clk_set_rate() to implicitly
>> enable the clock? If so, it would be better to cache the rate of
>> the clock in set_rate if the clock isn't enabled in software and
>> then send the cached rate during enable.
>>
>
> Agreed, I have asked the firmware/SCPI specification guys about
> more details on what to expect from firmware. Once they get back,
> will update the code considering your feedback.
>
> [...]
>
>>> +static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned
>>> long rate)
>>> +{
>>> +    int idx, max_opp = clk->info->count;
>>> +    struct scpi_opp *opp = clk->info->opps;
>>
>> const?
>>
>
> All the 3 above fixed now.
>
> [...]
>
>>> +
>>> +    clk = devm_clk_register(dev, &sclk->hw);
>>> +    if (!IS_ERR(clk) && max)
>>> +        clk_set_rate_range(clk, min, max);
>>
>> Hm.. we're planning to make clk_register() return a struct
>> clk_hw, so this will block that. We need some sort of clk_hw API
>> that allows us to setup min/max limits on the clock from the
>> provider side. Care to add that?
>>
>
> Can you provide pointer to the patches or the tree containing those
> changes ? Are they targeted for v4.3 ?
>

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.

Sudeep Holla July 7, 2015, 4:03 p.m. UTC | #5

On 06/07/15 20:52, Stephen Boyd wrote:
> On 07/03/2015 07:52 AM, Sudeep Holla wrote:
>> Hi Stephen,
>>
>>
>> Thanks for the review.
>>
>> On 02/07/15 18:23, Stephen Boyd wrote:
>>> On 06/08, Sudeep Holla wrote:

[...]

>>>> +
>>>> +    clk = devm_clk_register(dev, &sclk->hw);
>>>> +    if (!IS_ERR(clk) && max)
>>>> +        clk_set_rate_range(clk, min, max);
>>>
>>> Hm.. we're planning to make clk_register() return a struct
>>> clk_hw, so this will block that. We need some sort of clk_hw API
>>> that allows us to setup min/max limits on the clock from the
>>> provider side. Care to add that?
>>>
>>
>> Can you provide pointer to the patches or the tree containing those
>> changes ? Are they targeted for v4.3 ?
>>
>
> 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.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd July 8, 2015, 1:46 a.m. UTC | #6

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.

Sudeep Holla July 16, 2015, 4:11 p.m. UTC | #7

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-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9897f353bf1a..0fe8daefc105 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -59,6 +59,16 @@  config COMMON_CLK_RK808
 	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
 	  by control register.
 
+config COMMON_CLK_SCPI
+        tristate "Clock driver controlled via SCPI interface"
+        depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
+        ---help---
+          This driver provides support for clocks that are controlled
+          by firmware that implements the SCPI interface.
+
+	  This driver uses SCPI Message Protocol to interact with the
+	  firmware providing all the clock controls.
+
 config COMMON_CLK_SI5351
 	tristate "Clock driver for SiLabs 5351A/B/C"
 	depends on I2C
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3d00c25382c5..442ab6ebd5b1 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -36,6 +36,7 @@  obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
 obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
 obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
+obj-$(CONFIG_COMMON_CLK_SCPI)           += clk-scpi.o
 obj-$(CONFIG_COMMON_CLK_SI5351)		+= clk-si5351.o
 obj-$(CONFIG_COMMON_CLK_SI570)		+= clk-si570.o
 obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
new file mode 100644
index 000000000000..707b3430c55f
--- /dev/null
+++ b/drivers/clk/clk-scpi.c
@@ -0,0 +1,286 @@ 
+/*
+ * System Control and Power Interface (SCPI) Protocol based clock driver
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/scpi_protocol.h>
+
+struct scpi_clk {
+	u32 id;
+	const char *name;
+	struct clk_hw hw;
+	struct scpi_dvfs_info *info;
+	struct scpi_ops *scpi_ops;
+};
+
+#define to_scpi_clk(clk) container_of(clk, struct scpi_clk, hw)
+
+static unsigned long scpi_clk_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+
+	return clk->scpi_ops->clk_get_val(clk->id);
+}
+
+static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long *parent_rate)
+{
+	return rate;
+}
+
+static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+			     unsigned long parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+
+	return clk->scpi_ops->clk_set_val(clk->id, rate);
+}
+
+static void scpi_clk_disable(struct clk_hw *hw)
+{
+	scpi_clk_set_rate(hw, 0, 0);
+}
+
+static const struct clk_ops scpi_clk_ops = {
+	.recalc_rate = scpi_clk_recalc_rate,
+	.round_rate = scpi_clk_round_rate,
+	.set_rate = scpi_clk_set_rate,
+	.disable = scpi_clk_disable,
+};
+
+/* find closest match to given frequency in OPP table */
+static int __scpi_dvfs_round_rate(struct scpi_clk *clk, unsigned long rate)
+{
+	int idx;
+	u32 fmin = 0, fmax = ~0, ftmp;
+	struct scpi_opp *opp = clk->info->opps;
+
+	for (idx = 0; idx < clk->info->count; idx++, opp++) {
+		ftmp = opp->freq;
+		if (ftmp >= (u32)rate) {
+			if (ftmp <= fmax)
+				fmax = ftmp;
+			break;
+		} else if (ftmp >= fmin) {
+			fmin = ftmp;
+		}
+	}
+	return fmax != ~0 ? fmax : fmin;
+}
+
+static unsigned long scpi_dvfs_recalc_rate(struct clk_hw *hw,
+					   unsigned long parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+	int idx = clk->scpi_ops->dvfs_get_idx(clk->id);
+	struct scpi_opp *opp;
+
+	if (idx < 0)
+		return 0;
+
+	opp = clk->info->opps + idx;
+	return opp->freq;
+}
+
+static long scpi_dvfs_round_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long *parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+
+	return __scpi_dvfs_round_rate(clk, rate);
+}
+
+static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned long rate)
+{
+	int idx, max_opp = clk->info->count;
+	struct scpi_opp *opp = clk->info->opps;
+
+	for (idx = 0; idx < max_opp; idx++, opp++)
+		if (opp->freq == rate)
+			return idx;
+	return -EINVAL;
+}
+
+static int scpi_dvfs_set_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+	int ret = __scpi_find_dvfs_index(clk, rate);
+
+	if (ret < 0)
+		return ret;
+	return clk->scpi_ops->dvfs_set_idx(clk->id, (u8)ret);
+}
+
+static const struct clk_ops scpi_dvfs_ops = {
+	.recalc_rate = scpi_dvfs_recalc_rate,
+	.round_rate = scpi_dvfs_round_rate,
+	.set_rate = scpi_dvfs_set_rate,
+};
+
+static const struct of_device_id scpi_clk_match[] = {
+	{ .compatible = "arm,scpi-dvfs-clocks", .data = &scpi_dvfs_ops, },
+	{ .compatible = "arm,scpi-variable-clocks", .data = &scpi_clk_ops, },
+	{}
+};
+
+static struct clk *
+scpi_clk_ops_init(struct device *dev, const struct of_device_id *match,
+		  struct scpi_clk *sclk)
+{
+	struct clk_init_data init;
+	struct clk *clk;
+	unsigned long min = 0, max = 0;
+
+	init.name = sclk->name;
+	init.flags = CLK_IS_ROOT;
+	init.num_parents = 0;
+	init.ops = match->data;
+	sclk->hw.init = &init;
+	sclk->scpi_ops = get_scpi_ops();
+
+	if (init.ops == &scpi_dvfs_ops) {
+		sclk->info = sclk->scpi_ops->dvfs_get_info(sclk->id);
+		if (IS_ERR(sclk->info))
+			return NULL;
+	} else if (init.ops == &scpi_clk_ops) {
+		if (sclk->scpi_ops->clk_get_range(sclk->id, &min, &max) || !max)
+			return NULL;
+	} else {
+		return NULL;
+	}
+
+	clk = devm_clk_register(dev, &sclk->hw);
+	if (!IS_ERR(clk) && max)
+		clk_set_rate_range(clk, min, max);
+	return clk;
+}
+
+static int scpi_clk_add(struct device *dev, struct device_node *np,
+			const struct of_device_id *match)
+{
+	struct clk **clks;
+	int idx, count;
+	struct clk_onecell_data *clk_data;
+
+	count = of_property_count_strings(np, "clock-output-names");
+	if (count < 0) {
+		dev_err(dev, "%s: invalid clock output count\n", np->name);
+		return -EINVAL;
+	}
+
+	clk_data = devm_kmalloc(dev, sizeof(*clk_data), GFP_KERNEL);
+	if (!clk_data)
+		return -ENOMEM;
+
+	clks = devm_kcalloc(dev, count, sizeof(*clks), GFP_KERNEL);
+	if (!clks)
+		return -ENOMEM;
+
+	for (idx = 0; idx < count; idx++) {
+		struct scpi_clk *sclk;
+		u32 val;
+
+		sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
+		if (!sclk)
+			return -ENOMEM;
+
+		if (of_property_read_string_index(np, "clock-output-names",
+						  idx, &sclk->name)) {
+			dev_err(dev, "invalid clock name @ %s\n", np->name);
+			return -EINVAL;
+		}
+
+		if (of_property_read_u32_index(np, "clock-indices",
+					       idx, &val)) {
+			dev_err(dev, "invalid clock index @ %s\n", np->name);
+			return -EINVAL;
+		}
+
+		sclk->id = val;
+
+		clks[idx] = scpi_clk_ops_init(dev, match, sclk);
+		if (IS_ERR_OR_NULL(clks[idx]))
+			dev_err(dev, "failed to register clock '%s'\n",
+				sclk->name);
+		else
+			dev_dbg(dev, "Registered clock '%s'\n", sclk->name);
+	}
+
+	clk_data->clks = clks;
+	clk_data->clk_num = idx;
+
+	return of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
+}
+
+static int scpi_clocks_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *child, *np = dev->of_node;
+
+	for_each_available_child_of_node(np, child)
+		of_clk_del_provider(np);
+	return 0;
+}
+
+static int scpi_clocks_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct device_node *child, *np = dev->of_node;
+	const struct of_device_id *match;
+
+	if (!get_scpi_ops())
+		return -ENXIO;
+
+	for_each_available_child_of_node(np, child) {
+		match = of_match_node(scpi_clk_match, child);
+		if (!match)
+			continue;
+		ret = scpi_clk_add(dev, child, match);
+		if (ret) {
+			scpi_clocks_remove(pdev);
+			return ret;
+		}
+	}
+	return 0;
+}
+
+static const struct of_device_id scpi_clocks_ids[] = {
+	{ .compatible = "arm,scpi-clocks", },
+	{}
+};
+
+static struct platform_driver scpi_clocks_driver = {
+	.driver	= {
+		.name = "scpi_clocks",
+		.of_match_table = scpi_clocks_ids,
+	},
+	.probe = scpi_clocks_probe,
+	.remove = scpi_clocks_remove,
+};
+module_platform_driver(scpi_clocks_driver);
+
+MODULE_AUTHOR("Sudeep Holla <[e-mail]>");
+MODULE_DESCRIPTION("ARM SCPI clock driver");
+MODULE_LICENSE("GPL v2");