On Mon, Mar 30, 2015 at 6:21 PM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> On 03/30/15 17:15, Andrew Bresticker wrote:
>> 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.
>
> Ideally clock providers just provide clocks and don't actually call
> clock consumer APIs. I don't see where these clocks are disabled in this
> series. Is it just because suspend isn't done right now so there isn't a
> place to hook the disable part? I hope that it's a 1:1 relation between
> the clocks that are turned on here and the clocks that are turned off
> during suspend.
Suspend hasn't been hooked up yet and it's still a long ways off.
> I have a slightly similar problem on my hardware. Consider the case
> where the bootloader has left on the display and audio clocks and they
> share a common parent PLL. When the kernel boots up, all it knows is
> that the display clock and audio clock share a common PLL and the rate
> they're running at. If the audio driver probes first, calls clk_enable()
> on the audio clock (almost a no-op except for the fact that we call the
> .enable op when it's already on) and then calls clk_disable() on the
> audio clock when it's done we'll also turn off the shared PLL.
> Unfortunately it's also being used by the display clock for the display
> driver that hasn't probed yet and so the display stops working and it
> may show an artifact or black screen.
>
> Other cases are where certain clocks should never be turned off because
> they're used by some non-linux entity (dram controller for example) and
> we don't have a place to put the clk_prepare_enable() besides in the
> clock driver itself. In these cases, it may be better to tell the
> framework that a clock should always be on. I think this case is what
> Lee Jones is working on here[1].
>
> Do you fall into one of these two cases? It isn't clear to me how
> suspend is special and needs to be dealt with differently. Why wouldn't
> these clocks be always on?
These clocks fall into the latter case in that there's really no good
place to put the clk_prepare_enable() calls, so it looks like I want
to use what Lee is proposing.
