Hi,
On Thu, Nov 13, 2014 at 6:53 AM, Heiko St?bner <heiko at sntech.de> wrote:
> Am Donnerstag, 13. November 2014, 21:20:25 schrieb Kever Yang:
>> Usually we assigned a clock to a default rate in dts,
>> there is a situation that the clock already initialized to the rate
>> we intend to set before kernel(hardware default or init in uboot etc).
>> For the PLLs we can get a rate from different PLL parameter configure,
>> we can't change the PLL parameter if the rate is not changed by now.
>>
>> This patch adds a option property 'assigned-clock-force-rates'
>> to make sure we update all the setting even if we don't need to
>> update the clock rate.
>>
>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>> ---
>>
>> drivers/clk/clk-conf.c | 33 ++++++++++++++++++++++++++++++++-
>> 1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
>> index aad4796..0c9df48 100644
>> --- a/drivers/clk/clk-conf.c
>> +++ b/drivers/clk/clk-conf.c
>> @@ -84,7 +84,7 @@ static int __set_clk_rates(struct device_node *node, bool
>> clk_supplier) struct clk *clk;
>> u32 rate;
>>
>> - of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
>> + of_property_for_each_u32(node, "assigned-force-rates", prop, cur, rate) {
>> if (rate) {
>> rc = of_parse_phandle_with_args(node, "assigned-clocks",
>> "#clock-cells", index, &clkspec);
>> @@ -104,7 +104,38 @@ static int __set_clk_rates(struct device_node *node,
>> bool clk_supplier) index, node->full_name);
>> return PTR_ERR(clk);
>> }
>> + /* change the old rate to 0 to make sure we can get into
>> + * clk_change_rate */
>> + clk->rate = 0;
>> + rc = clk_set_rate(clk, rate);
>> + if (rc < 0)
>> + pr_err("clk: couldn't set %s clock rate: %d\n",
>> + __clk_get_name(clk), rc);
>> + clk_put(clk);
>
> Forcing clocks to 0 at first will probably create issues on some platfoms.
> I think what Doug meant was something like [0], which would then enable
> the clk_conf part to force the rate change. I haven't tested this yet, but it
> seems the check in clk_set_rate is the only one checking for identical new
> and old rates.
Hrm, I was actually not thinking of adding a new device tree property.
I was thinking that we'd _always_ call "force" for
"assigned-clock-rates". Really the check in clk_set_rate() is an
optimization (right?), not for correctness. Thus it should be OK to
bypass it at bootup.
Actually, maybe even better: for all clocks you should always skip the
"clk_get_rate()" check the first time through. Then you'd ensure that
you aren't using some default or firmware-assigned clock settings.
AKA, something like this untested patch:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 59d853d..56db138 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1618,9 +1618,10 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
- /* bail early if nothing to do */
- if (rate == clk_get_rate(clk))
+ /* bail early if nothing to do; linux should always set the rate once */
+ if (rate == clk_get_rate(clk) && clk->did_set_rate)
goto out;
+ clk->did_set_rate = true;
if ((clk->flags & CLK_SET_RATE_GATE) && clk->prepare_count) {
ret = -EBUSY;
I'm ducking now in case Mike decides to throw a tomato at me.
-Doug
