Re: [PATCH 2/7] clk: Add basic infrastructure for Pistachio clocks

[Andrew Bresticker]

Hi Mike,

On Mon, Mar 30, 2015 at 6:36 PM, Michael Turquette
<mturquette@xxxxxxxxxx> wrote:
> Quoting Andrew Bresticker (2015-03-30 17:15:43)
>> On Mon, Mar 30, 2015 at 4:59 PM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
>> > On 02/24/15 19:56, Andrew Bresticker wrote:
>> >> +
>> >> +void pistachio_clk_force_enable(struct pistachio_clk_provider *p,
>> >> +                             unsigned int *clk_ids, unsigned int num)
>> >> +{
>> >> +     unsigned int i;
>> >> +     int err;
>> >> +
>> >> +     for (i = 0; i < num; i++) {
>> >> +             struct clk *clk = p->clk_data.clks[clk_ids[i]];
>> >> +
>> >> +             if (IS_ERR(clk))
>> >> +                     continue;
>> >> +
>> >> +             err = clk_prepare_enable(clk);
>> >> +             if (err)
>> >> +                     pr_err("Failed to enable clock %s: %d\n",
>> >> +                            __clk_get_name(clk), err);
>> >> +     }
>> >> +}
>> >>
>> >
>> > Is this to workaround some problems in the framework where clocks are
>> > turned off? Or is it that these clocks are already on before we boot
>> > Linux and we need to make sure the framework knows that?
>>
>> It's the former.  These clocks are enabled at POR and may only be
>> gated as the final step to entering suspend, so they must remain on at
>> runtime.  The issue we were running into was that consumers of these
>> critical clocks or their descendants would enable/disable their clocks
>> during boot or runtime PM and cause these clocks to get disabled.
>> Bumping up the prepare/enable count of these critical clocks seemed
>> like the best way to handle this - is there a more preferred way?
>> FWIW, this is also how the Tegra and Rockchip drivers handled this
>> problem.
>
> Hi Andrew,
>
> Why are your drivers allowed to disable clocks which must not be
> disabled? (you mentioned boot and runtime pm)

The issue is that they do not directly consume a critical clock, but
rather a descendant of the critical clock and thus could cause the
critical clock to be disabled.  For example, the periph_sys clock is
one of these critical clocks and it is the parent of various
peripheral clocks, like the watchdog, I2C, PWM, etc.  If the I2C
driver enables and disables it's clock during probe(), and nothing
else has caused the periph_sys clock to be enabled, the disable() call
will cause periph_sys to become disabled since its enable count drops
to 0.

Now this could be solved if there was a driver to directly consume
these clocks and keep them enabled, but, like Stephen mentioned, there
really isn't a proper driver for that.  So it looks like I want
something like the always-on feature Lee is trying to introduce.

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