* [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