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

[Michael Trimarchi]

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?

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.
>
> 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.
>
> 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