lkml.org 
[lkml]   [2015]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System Control Processor)
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


\
 
 \ /
  Last update: 2015-07-03 17:21    [W:0.153 / U:0.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site