Re: [PATCH V4 1/2] clk: bcm2835: mark enabled clocks with CLK_IS_CRITICAL

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

Martin Sperl <kernel@martin.sperl.org> writes:

>> On 02.05.2016, at 17:36, Eric Anholt <eric@anholt.net> wrote:
>> 
>> kernel@martin.sperl.org writes:
>> 
>>> From: Martin Sperl <kernel@martin.sperl.org>
>>> 
>>> The bcm2835 firmware enables several clocks and plls before
>>> booting the linux kernel.
>>> 
>>> These plls should never get disabled as it may result in a
>>> stopped system clock and more.
>>> 
>>> So during probing we check if the clock is enabled
>>> and if it is then mark that clock with CLK_IS_CRITICAL.
>>> 
>>> As a consequence this will also enable the corresponding
>>> parent plls and pll-divs.
>>> 
>>> This is intended as a stop-gap until CLK_ENABLE_HAND_OFF
>>> becomes available, at which point it should be used instead.
>>> 
>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>> 
>> I still think that we don't want this patch.  We should be able to
>> disable clocks that the firmware turned on, unless they really need to
>> stay on.  If you have troubles on the upstream DT, let's talk about
>> individual clocks.
>
> May I ask you what is your main concern about using
> CLK_IS_CRITICAL as a stop-gap/in general?

Burning power when you shouldn't, which is a bug.

> Also the current situation of the machine crashing when releasing the
> PCM clock when the parent is PLLC or PLLD is worse than having some
> clocks/pll running unnecessarily.

Are you saying this happens on the upstream kernel?  This sounds like a
bug you'd see in the downstream kernel because they haven't hooked up
the clocks in their DT.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]