* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
@ 2013-11-18 14:10 Michael Trimarchi
2013-11-19 8:24 ` Igor Grinberg
0 siblings, 1 reply; 9+ messages in thread
From: Michael Trimarchi @ 2013-11-18 14:10 UTC (permalink / raw)
To: u-boot
Enable clocks for the peripherals only if they are used
Signed-off-by: Michael Trimarchi <michael@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);
sdelay(1000);
}
diff --git a/arch/arm/include/asm/arch-omap3/clock.h b/arch/arm/include/asm/arch-omap3/clock.h
index be669c1..1912cc9 100644
--- a/arch/arm/include/asm/arch-omap3/clock.h
+++ b/arch/arm/include/asm/arch-omap3/clock.h
@@ -27,8 +27,6 @@
#define ICK_DSS_ON 0x00000001
#define FCK_CAM_ON 0x00000001
#define ICK_CAM_ON 0x00000001
-#define FCK_PER_ON 0x0003ffff
-#define ICK_PER_ON 0x0003ffff
/* Used to index into DPLL parameter tables */
typedef struct {
--
1.8.1.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
2013-11-18 14:10 [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used Michael Trimarchi
@ 2013-11-19 8:24 ` Igor Grinberg
2013-11-19 8:49 ` Michael Trimarchi
0 siblings, 1 reply; 9+ messages in thread
From: Igor Grinberg @ 2013-11-19 8:24 UTC (permalink / raw)
To: u-boot
Hi Michael,
On 11/18/13 16:10, Michael Trimarchi wrote:
> Enable clocks for the peripherals only if they are used
>
> Signed-off-by: Michael Trimarchi <michael@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?
>
> sdelay(1000);
> }
> diff --git a/arch/arm/include/asm/arch-omap3/clock.h b/arch/arm/include/asm/arch-omap3/clock.h
> index be669c1..1912cc9 100644
> --- a/arch/arm/include/asm/arch-omap3/clock.h
> +++ b/arch/arm/include/asm/arch-omap3/clock.h
> @@ -27,8 +27,6 @@
> #define ICK_DSS_ON 0x00000001
> #define FCK_CAM_ON 0x00000001
> #define ICK_CAM_ON 0x00000001
> -#define FCK_PER_ON 0x0003ffff
> -#define ICK_PER_ON 0x0003ffff
>
> /* Used to index into DPLL parameter tables */
> typedef struct {
>
--
Regards,
Igor.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
2013-11-19 8:24 ` Igor Grinberg
@ 2013-11-19 8:49 ` Michael Trimarchi
2013-11-19 8:59 ` Michael Trimarchi
0 siblings, 1 reply; 9+ messages in thread
From: Michael Trimarchi @ 2013-11-19 8:49 UTC (permalink / raw)
To: u-boot
Hi Igor
On Tue, Nov 19, 2013 at 9:24 AM, Igor Grinberg <grinberg@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
>>
>> Signed-off-by: Michael Trimarchi <michael@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.
Michael
>>
>> sdelay(1000);
>> }
>> diff --git a/arch/arm/include/asm/arch-omap3/clock.h b/arch/arm/include/asm/arch-omap3/clock.h
>> index be669c1..1912cc9 100644
>> --- a/arch/arm/include/asm/arch-omap3/clock.h
>> +++ b/arch/arm/include/asm/arch-omap3/clock.h
>> @@ -27,8 +27,6 @@
>> #define ICK_DSS_ON 0x00000001
>> #define FCK_CAM_ON 0x00000001
>> #define ICK_CAM_ON 0x00000001
>> -#define FCK_PER_ON 0x0003ffff
>> -#define ICK_PER_ON 0x0003ffff
>>
>> /* Used to index into DPLL parameter tables */
>> typedef struct {
>>
>
> --
> Regards,
> Igor.
--
| Michael Nazzareno Trimarchi Amarula Solutions BV |
| COO - Founder
Cruquiuskade 47 |
| +31(0)851119172 Amsterdam 1018 AM NL |
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
2013-11-19 8:49 ` Michael Trimarchi
@ 2013-11-19 8:59 ` Michael Trimarchi
2013-11-19 11:10 ` Igor Grinberg
0 siblings, 1 reply; 9+ messages in thread
From: Michael Trimarchi @ 2013-11-19 8:59 UTC (permalink / raw)
To: u-boot
Hi Igor
On Tue, Nov 19, 2013 at 9:49 AM, Michael Trimarchi
<michael@amarulasolutions.com> wrote:
> Hi Igor
>
> On Tue, Nov 19, 2013 at 9:24 AM, Igor Grinberg <grinberg@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
>>>
>>> Signed-off-by: Michael Trimarchi <michael@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.
>
To be more clear, this change just enable clocks that are supposed
to be used. Now that code was supposed to be removed long
time ago.
Michael
> Michael
>
>>>
>>> sdelay(1000);
>>> }
>>> diff --git a/arch/arm/include/asm/arch-omap3/clock.h b/arch/arm/include/asm/arch-omap3/clock.h
>>> index be669c1..1912cc9 100644
>>> --- a/arch/arm/include/asm/arch-omap3/clock.h
>>> +++ b/arch/arm/include/asm/arch-omap3/clock.h
>>> @@ -27,8 +27,6 @@
>>> #define ICK_DSS_ON 0x00000001
>>> #define FCK_CAM_ON 0x00000001
>>> #define ICK_CAM_ON 0x00000001
>>> -#define FCK_PER_ON 0x0003ffff
>>> -#define ICK_PER_ON 0x0003ffff
>>>
>>> /* Used to index into DPLL parameter tables */
>>> typedef struct {
>>>
>>
>> --
>> Regards,
>> Igor.
>
>
>
> --
> | Michael Nazzareno Trimarchi Amarula Solutions BV |
> | COO - Founder
> Cruquiuskade 47 |
> | +31(0)851119172 Amsterdam 1018 AM NL |
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
2013-11-19 8:59 ` Michael Trimarchi
@ 2013-11-19 11:10 ` Igor Grinberg
2013-11-19 11:56 ` Michael Trimarchi
0 siblings, 1 reply; 9+ messages in thread
From: Igor Grinberg @ 2013-11-19 11:10 UTC (permalink / raw)
To: u-boot
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@amarulasolutions.com> wrote:
>> Hi Igor
>>
>> On Tue, Nov 19, 2013 at 9:24 AM, Igor Grinberg <grinberg@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.
May be some of the consequences can be avoided?
>>>>
>>>> Signed-off-by: Michael Trimarchi <michael@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.
As a consequence of this patch, there might be boards that
are not correctly configured and thus broken.
In other words, this patch breaks boards that rely on those clocks
being enabled.
>>
>
> To be more clear, this change just enable clocks that are supposed
> to be used.
Well, if I read the patch correctly, it does not enable anything.
It removes the unconditional configuration of some clocks...
> Now that code was supposed to be removed long
> time ago.
Were there any discussions on this meter?
Can you please point me to a discussion on this?
Otherwise, the 'supposed' word here does not have a basis.
>
> Michael
>
>> Michael
>>
>>>>
>>>> sdelay(1000);
>>>> }
>>>> diff --git a/arch/arm/include/asm/arch-omap3/clock.h b/arch/arm/include/asm/arch-omap3/clock.h
>>>> index be669c1..1912cc9 100644
>>>> --- a/arch/arm/include/asm/arch-omap3/clock.h
>>>> +++ b/arch/arm/include/asm/arch-omap3/clock.h
>>>> @@ -27,8 +27,6 @@
>>>> #define ICK_DSS_ON 0x00000001
>>>> #define FCK_CAM_ON 0x00000001
>>>> #define ICK_CAM_ON 0x00000001
>>>> -#define FCK_PER_ON 0x0003ffff
>>>> -#define ICK_PER_ON 0x0003ffff
>>>>
>>>> /* Used to index into DPLL parameter tables */
>>>> typedef struct {
>>>>
>>>
>>> --
>>> Regards,
>>> Igor.
>>
>>
>>
>> --
>> | Michael Nazzareno Trimarchi Amarula Solutions BV |
>> | COO - Founder
>> Cruquiuskade 47 |
>> | +31(0)851119172 Amsterdam 1018 AM NL |
>
--
Regards,
Igor.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
2013-11-19 11:10 ` Igor Grinberg
@ 2013-11-19 11:56 ` Michael Trimarchi
2013-11-20 16:19 ` Tom Rini
0 siblings, 1 reply; 9+ messages in thread
From: Michael Trimarchi @ 2013-11-19 11:56 UTC (permalink / raw)
To: u-boot
Hi
On Tue, Nov 19, 2013 at 12:10 PM, Igor Grinberg <grinberg@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@amarulasolutions.com> wrote:
>>> Hi Igor
>>>
>>> On Tue, Nov 19, 2013 at 9:24 AM, Igor Grinberg <grinberg@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@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
> As a consequence of this patch, there might be boards that
> are not correctly configured and thus broken.
> In other words, this patch breaks boards that rely on those clocks
> being enabled.
Michael
>
>>>
>>
>> To be more clear, this change just enable clocks that are supposed
>> to be used.
>
> Well, if I read the patch correctly, it does not enable anything.
> It removes the unconditional configuration of some clocks...
>
>> Now that code was supposed to be removed long
>> time ago.
>
> Were there any discussions on this meter?
> Can you please point me to a discussion on this?
> Otherwise, the 'supposed' word here does not have a basis.
>
>>
>> Michael
>>
>>> Michael
>>>
>>>>>
>>>>> sdelay(1000);
>>>>> }
>>>>> diff --git a/arch/arm/include/asm/arch-omap3/clock.h b/arch/arm/include/asm/arch-omap3/clock.h
>>>>> index be669c1..1912cc9 100644
>>>>> --- a/arch/arm/include/asm/arch-omap3/clock.h
>>>>> +++ b/arch/arm/include/asm/arch-omap3/clock.h
>>>>> @@ -27,8 +27,6 @@
>>>>> #define ICK_DSS_ON 0x00000001
>>>>> #define FCK_CAM_ON 0x00000001
>>>>> #define ICK_CAM_ON 0x00000001
>>>>> -#define FCK_PER_ON 0x0003ffff
>>>>> -#define ICK_PER_ON 0x0003ffff
>>>>>
>>>>> /* Used to index into DPLL parameter tables */
>>>>> typedef struct {
>>>>>
>>>>
>>>> --
>>>> Regards,
>>>> Igor.
>>>
>>>
>>>
>>> --
>>> | Michael Nazzareno Trimarchi Amarula Solutions BV |
>>> | COO - Founder
>>> Cruquiuskade 47 |
>>> | +31(0)851119172 Amsterdam 1018 AM NL |
>>
>
> --
> Regards,
> Igor.
--
| Michael Nazzareno Trimarchi Amarula Solutions BV |
| COO - Founder
Cruquiuskade 47 |
| +31(0)851119172 Amsterdam 1018 AM NL |
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
2013-11-19 11:56 ` Michael Trimarchi
@ 2013-11-20 16:19 ` Tom Rini
2013-11-20 20:07 ` Michael Trimarchi
0 siblings, 1 reply; 9+ messages in thread
From: Tom Rini @ 2013-11-20 16:19 UTC (permalink / raw)
To: u-boot
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@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@amarulasolutions.com> wrote:
> >>> Hi Igor
> >>>
> >>> On Tue, Nov 19, 2013 at 9:24 AM, Igor Grinberg <grinberg@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@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,
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131120/b399b431/attachment.pgp>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
2013-11-20 16:19 ` Tom Rini
@ 2013-11-20 20:07 ` Michael Trimarchi
2013-11-21 9:26 ` Igor Grinberg
0 siblings, 1 reply; 9+ messages in thread
From: Michael Trimarchi @ 2013-11-20 20:07 UTC (permalink / raw)
To: u-boot
Hi
On Wed, Nov 20, 2013 at 5:19 PM, Tom Rini <trini@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@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@amarulasolutions.com> wrote:
>> >>> Hi Igor
>> >>>
>> >>> On Tue, Nov 19, 2013 at 9:24 AM, Igor Grinberg <grinberg@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@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used
2013-11-20 20:07 ` Michael Trimarchi
@ 2013-11-21 9:26 ` Igor Grinberg
0 siblings, 0 replies; 9+ messages in thread
From: Igor Grinberg @ 2013-11-21 9:26 UTC (permalink / raw)
To: u-boot
On 11/20/13 22:07, Michael Trimarchi wrote:
> Hi
>
> On Wed, Nov 20, 2013 at 5:19 PM, Tom Rini <trini@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@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@amarulasolutions.com> wrote:
>>>>>> Hi Igor
>>>>>>
>>>>>> On Tue, Nov 19, 2013 at 9:24 AM, Igor Grinberg <grinberg@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@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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-11-21 9:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-18 14:10 [U-Boot] [PATCH] arm: omap3: Enable clocks for peripherals only if they are used Michael Trimarchi
2013-11-19 8:24 ` Igor Grinberg
2013-11-19 8:49 ` Michael Trimarchi
2013-11-19 8:59 ` Michael Trimarchi
2013-11-19 11:10 ` Igor Grinberg
2013-11-19 11:56 ` Michael Trimarchi
2013-11-20 16:19 ` Tom Rini
2013-11-20 20:07 ` Michael Trimarchi
2013-11-21 9:26 ` Igor Grinberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox