[U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used

[Igor Grinberg]

On 11/20/13 22:07, Michael Trimarchi wrote:
> Hi
> 
> On Wed, Nov 20, 2013 at 5:19 PM, Tom Rini <trini at ti.com> wrote:
>> On Tue, Nov 19, 2013 at 12:56:13PM +0100, Michael Trimarchi wrote:
>>> Hi
>>>
>>> On Tue, Nov 19, 2013 at 12:10 PM, Igor Grinberg <grinberg at compulab.co.il> wrote:
>>>> Hi Michael,
>>>>
>>>> On 11/19/13 10:59, Michael Trimarchi wrote:
>>>>> Hi Igor
>>>>>
>>>>> On Tue, Nov 19, 2013 at 9:49 AM, Michael Trimarchi
>>>>> <michael at amarulasolutions.com> wrote:
>>>>>> Hi Igor
>>>>>>
>>>>>> On Tue, Nov 19, 2013 at 9:24 AM, Igor Grinberg <grinberg at compulab.co.il> wrote:
>>>>>>> Hi Michael,
>>>>>>>
>>>>>>> On 11/18/13 16:10, Michael Trimarchi wrote:
>>>>>>>> Enable clocks for the peripherals only if they are used
>>>>
>>>> This is not exactly what the patch does, right?
>>>> I would expect a better description of what you do in the patch
>>>> and what are the consequences of this patch.
>>>
>>> It does the right. If you open the files you can find that clock
>>> are enabled if the peripheral is used.
>>>
>>>> May be some of the consequences can be avoided?
>>>>
>>>
>>> Yes, this is correct. Most of the consequences come if
>>> you use gpio bank and you don't active them
>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
>>>>>>>> ---
>>>>>>>>  arch/arm/cpu/armv7/omap3/clock.c        | 2 --
>>>>>>>>  arch/arm/include/asm/arch-omap3/clock.h | 2 --
>>>>>>>>  2 files changed, 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/cpu/armv7/omap3/clock.c b/arch/arm/cpu/armv7/omap3/clock.c
>>>>>>>> index 14fc7e8..1bc27bd 100644
>>>>>>>> --- a/arch/arm/cpu/armv7/omap3/clock.c
>>>>>>>> +++ b/arch/arm/cpu/armv7/omap3/clock.c
>>>>>>>> @@ -730,8 +730,6 @@ void per_clocks_enable(void)
>>>>>>>>               sr32(&prcm_base->fclken_cam, 0, 32, FCK_CAM_ON);
>>>>>>>>               sr32(&prcm_base->iclken_cam, 0, 32, ICK_CAM_ON);
>>>>>>>>       }
>>>>>>>> -     sr32(&prcm_base->fclken_per, 0, 32, FCK_PER_ON);
>>>>>>>> -     sr32(&prcm_base->iclken_per, 0, 32, ICK_PER_ON);
>>>>>>>
>>>>>>> Hmmm...
>>>>>>> Am I missing something or is this change breaks boards that
>>>>>>> currently rely on the peripheral clocks being enabled here?
>>>>>>>
>>>>>>
>>>>>> This change break boards that are not correctly configured.
>>>>
>>>> I don't agree on this one.
>>>> This change breaks boards that are _correctly_ configured
>>>> prior to the patch.
>>>
>>> If you think that if the boards are correct they should boot
>>
>> I think this is an extension of the CONFIG_SYS_ENABLE_PADS_ALL /
>> CONFIG_SYS_CLOCKS_ENABLE_ALL problem.  The long stated design of U-Boot
>> is we enable what we need.  The long stated design of the Linux Kernel,
> 
> Do you want me to re-submit with a better description?

I definitely do!!!
One of my biggest concerns is the lack of the description
for such patches.
It must be clear to anyone who looks at the log, what this patch does.
So, as for me, the patch itself is _fine_ (I also was always concerned
about enabling clocks, remember the OMAP3 USB submissions?), but
we must have a good description of what exactly may break after the
patch. So if its lack of CONFIG_* defines, it must be stated in the
commit message. If it is GPT clocks, WDOG clock, etc... - it must be
stated in the commit message and we also need to consider adding
CONFIG_* options for those that don't have such an option (e.g. GPT, WDOG).

> 
> Michael
> 
>> in general, is that it doesn't rely on the bootloader to have enabled
>> all of this.  And then various vendors (mine included) broke both rules
>> for a while when people weren't paying enough attention.  So yes, it's
>> likely that some boards will break because they assume the clock was
>> enabled for them.
>>
>> The question is, are there boards where it's reasonable to assume people
>> would upgrade U-Boot but not upgrade the kernel and see this problem?
>> That I don't know the answer to.

Well, yes there are customers who will upgrade U-Boot and
not touch the kernel.

>>
>> The second question is, are there problems with leaving the clock
>> enablement on?  I suspect there is, which is how this was run into in
>> the first place.

Unless, we can leave OMAP3 like it was, but be more strict regarding
newer SoCs.

>>
>> At some level the platforms people use today and their needs outweigh
>> the problems of the platforms people used to use, but still maybe could,
>> if they wanted to dust it off and plug it in, and it didn't just up and
>> die on them.
>>
>> --
>> Tom
> 

-- 
Regards,
Igor.