* [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
@ 2010-08-10 11:39 Sekhar Nori
2010-08-11 4:03 ` Nishanth Menon
0 siblings, 1 reply; 6+ messages in thread
From: Sekhar Nori @ 2010-08-10 11:39 UTC (permalink / raw)
To: u-boot
The TI DA850/OMAP-L138/AM18x EVM can be populated with devices
of different speed grades.
The maximum speed the chip can support can only be determined from
the label on the package (not software readable).
Introduce a method to pass the speed grade information to kernel
using ATAG_REVISION. The kernel uses this information to determine
the maximum speed reachable using cpufreq.
Note that U-Boot itself does not set the CPU rate. The CPU
speed is setup by a primary bootloader ("UBL"). The rate setup
by UBL could be different from the maximum speed grade of the
device.
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
Applies to latest of U-Boot mainline master
board/davinci/da8xxevm/da850evm.c | 38 +++++++++++++++++++++++++++++++++++++
include/configs/da850evm.h | 1 +
2 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c
index 959b2c6..6a6d4fb 100644
--- a/board/davinci/da8xxevm/da850evm.c
+++ b/board/davinci/da8xxevm/da850evm.c
@@ -70,6 +70,44 @@ static const struct lpsc_resource lpsc[] = {
{ DAVINCI_LPSC_GPIO },
};
+#ifndef CONFIG_DA850_EVM_MAX_SPEED
+#define CONFIG_DA850_EVM_MAX_SPEED 300000
+#endif
+
+/*
+ * get_board_rev() - setup to pass kernel board revision information
+ * Returns:
+ * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part
+ * 0 - 300 MHz
+ * 1 - 372 MHz
+ * 2 - 408 MHz
+ * 3 - 456 MHz
+ */
+u32 get_board_rev(void)
+{
+ char *s;
+ u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED;
+ u32 rev = 0;
+
+ s = getenv("maxspeed");
+ if (s)
+ maxspeed = simple_strtoul(s, NULL, 10);
+
+ switch (maxspeed) {
+ case 456000:
+ rev |= 3;
+ break;
+ case 408000:
+ rev |= 2;
+ break;
+ case 372000:
+ rev |= 1;
+ break;
+ }
+
+ return rev;
+}
+
int board_init(void)
{
#ifndef CONFIG_USE_IRQ
diff --git a/include/configs/da850evm.h b/include/configs/da850evm.h
index 357715d..3ea9032 100644
--- a/include/configs/da850evm.h
+++ b/include/configs/da850evm.h
@@ -102,6 +102,7 @@
*/
#define LINUX_BOOT_PARAM_ADDR (CONFIG_SYS_MEMTEST_START + 0x100)
#define CONFIG_CMDLINE_TAG
+#define CONFIG_REVISION_TAG
#define CONFIG_SETUP_MEMORY_TAGS
#define CONFIG_BOOTARGS \
"mem=32M console=ttyS2,115200n8 root=/dev/mtdblock2 rw noinitrd ip=dhcp"
--
1.6.2.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel 2010-08-10 11:39 [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel Sekhar Nori @ 2010-08-11 4:03 ` Nishanth Menon 2010-08-11 15:37 ` Nori, Sekhar 0 siblings, 1 reply; 6+ messages in thread From: Nishanth Menon @ 2010-08-11 4:03 UTC (permalink / raw) To: u-boot On 08/10/2010 06:39 AM, Sekhar Nori wrote: > The TI DA850/OMAP-L138/AM18x EVM can be populated with devices > of different speed grades. > > The maximum speed the chip can support can only be determined from > the label on the package (not software readable). > > Introduce a method to pass the speed grade information to kernel > using ATAG_REVISION. The kernel uses this information to determine > the maximum speed reachable using cpufreq. > > Note that U-Boot itself does not set the CPU rate. The CPU > speed is setup by a primary bootloader ("UBL"). The rate setup > by UBL could be different from the maximum speed grade of the > device. > > Signed-off-by: Sekhar Nori<nsekhar@ti.com> > --- > Applies to latest of U-Boot mainline master > > board/davinci/da8xxevm/da850evm.c | 38 +++++++++++++++++++++++++++++++++++++ > include/configs/da850evm.h | 1 + > 2 files changed, 39 insertions(+), 0 deletions(-) > > diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c > index 959b2c6..6a6d4fb 100644 > --- a/board/davinci/da8xxevm/da850evm.c > +++ b/board/davinci/da8xxevm/da850evm.c > @@ -70,6 +70,44 @@ static const struct lpsc_resource lpsc[] = { > { DAVINCI_LPSC_GPIO }, > }; > > +#ifndef CONFIG_DA850_EVM_MAX_SPEED > +#define CONFIG_DA850_EVM_MAX_SPEED 300000 > +#endif > + > +/* > + * get_board_rev() - setup to pass kernel board revision information > + * Returns: > + * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part > + * 0 - 300 MHz > + * 1 - 372 MHz > + * 2 - 408 MHz > + * 3 - 456 MHz > + */ > +u32 get_board_rev(void) > +{ > + char *s; > + u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED; > + u32 rev = 0; > + > + s = getenv("maxspeed"); > + if (s) > + maxspeed = simple_strtoul(s, NULL, 10); > + > + switch (maxspeed) { > + case 456000: > + rev |= 3; > + break; > + case 408000: > + rev |= 2; > + break; > + case 372000: > + rev |= 1; wondering if the |= makes any sense... > + break; > + } > + > + return rev; IMHO, the logic could be simplified? option 1: u8 rev=0; s = simple_strtoul(s, NULL, 10); if (s) { switch (simple_strtoul(s, NULL, 10)) { case 456000: rev = 3; break; case 408000: rev = 2; break; case 372000: rev = 1; break; } } option 2: if you think that the speeds could get added in the future, that switch is going to look pretty ugly.. use a lookup instead.. u32 speeds[]={372000,408000,456000}; int i; u32 mspeed; s=simple_strtoul(s, NULL, 10); maxspeed = simple_strtoul(s, NULL, 10); for (i=0;i<ARRAY_SIZE(speeds);i++) if (maxspeed == speeds[i]) return i + 1; return 0; > +} > + > int board_init(void) > { > #ifndef CONFIG_USE_IRQ > diff --git a/include/configs/da850evm.h b/include/configs/da850evm.h > index 357715d..3ea9032 100644 > --- a/include/configs/da850evm.h > +++ b/include/configs/da850evm.h > @@ -102,6 +102,7 @@ > */ > #define LINUX_BOOT_PARAM_ADDR (CONFIG_SYS_MEMTEST_START + 0x100) > #define CONFIG_CMDLINE_TAG > +#define CONFIG_REVISION_TAG > #define CONFIG_SETUP_MEMORY_TAGS > #define CONFIG_BOOTARGS \ > "mem=32M console=ttyS2,115200n8 root=/dev/mtdblock2 rw noinitrd ip=dhcp" ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel 2010-08-11 4:03 ` Nishanth Menon @ 2010-08-11 15:37 ` Nori, Sekhar 2010-08-12 5:43 ` Nishanth Menon 0 siblings, 1 reply; 6+ messages in thread From: Nori, Sekhar @ 2010-08-11 15:37 UTC (permalink / raw) To: u-boot Hi Nishanth, On Wed, Aug 11, 2010 at 09:33:29, Nishanth Menon wrote: > On 08/10/2010 06:39 AM, Sekhar Nori wrote: > > diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c > > index 959b2c6..6a6d4fb 100644 > > --- a/board/davinci/da8xxevm/da850evm.c > > +++ b/board/davinci/da8xxevm/da850evm.c > > @@ -70,6 +70,44 @@ static const struct lpsc_resource lpsc[] = { > > { DAVINCI_LPSC_GPIO }, > > }; > > > > +#ifndef CONFIG_DA850_EVM_MAX_SPEED > > +#define CONFIG_DA850_EVM_MAX_SPEED 300000 > > +#endif > > + > > +/* > > + * get_board_rev() - setup to pass kernel board revision information > > + * Returns: > > + * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part > > + * 0 - 300 MHz > > + * 1 - 372 MHz > > + * 2 - 408 MHz > > + * 3 - 456 MHz > > + */ > > +u32 get_board_rev(void) > > +{ > > + char *s; > > + u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED; > > + u32 rev = 0; > > + > > + s = getenv("maxspeed"); > > + if (s) > > + maxspeed = simple_strtoul(s, NULL, 10); > > + > > + switch (maxspeed) { > > + case 456000: > > + rev |= 3; > > + break; > > + case 408000: > > + rev |= 2; > > + break; > > + case 372000: > > + rev |= 1; > wondering if the |= makes any sense... Yeah, I put it in there in case additional fields pop-up in board revision. It doesn't make a lot of sense now so I will remove it. > > + break; > > + } > > + > > + return rev; > > IMHO, the logic could be simplified? > > option 1: > u8 rev=0; > s = simple_strtoul(s, NULL, 10); > if (s) { > switch (simple_strtoul(s, NULL, 10)) { > case 456000: > rev = 3; > break; > case 408000: > rev = 2; > break; > case 372000: > rev = 1; > break; > } > } Not sure how this simplifies the logic. I'd argue multiple strtoul calls are actually better avoided. How does it handle the case where max speed is to be set using board configuration? > > option 2: > if you think that the speeds could get added in the future, that switch > is going to look pretty ugly.. use a lookup instead.. Right now there is no known plan to add more speeds so I will stick with the switch. You are right, option of using a lookup deserves a look-in if there were a lot more speed steps. Thanks, Sekhar ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel 2010-08-11 15:37 ` Nori, Sekhar @ 2010-08-12 5:43 ` Nishanth Menon 2010-08-12 6:14 ` Nori, Sekhar 0 siblings, 1 reply; 6+ messages in thread From: Nishanth Menon @ 2010-08-12 5:43 UTC (permalink / raw) To: u-boot On 08/11/2010 10:37 AM, Nori, Sekhar wrote: > Hi Nishanth, > > On Wed, Aug 11, 2010 at 09:33:29, Nishanth Menon wrote: >> On 08/10/2010 06:39 AM, Sekhar Nori wrote: > >>> diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c >>> index 959b2c6..6a6d4fb 100644 >>> --- a/board/davinci/da8xxevm/da850evm.c >>> +++ b/board/davinci/da8xxevm/da850evm.c >>> @@ -70,6 +70,44 @@ static const struct lpsc_resource lpsc[] = { >>> { DAVINCI_LPSC_GPIO }, >>> }; >>> >>> +#ifndef CONFIG_DA850_EVM_MAX_SPEED >>> +#define CONFIG_DA850_EVM_MAX_SPEED 300000 >>> +#endif >>> + >>> +/* >>> + * get_board_rev() - setup to pass kernel board revision information >>> + * Returns: >>> + * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part >>> + * 0 - 300 MHz >>> + * 1 - 372 MHz >>> + * 2 - 408 MHz >>> + * 3 - 456 MHz >>> + */ >>> +u32 get_board_rev(void) >>> +{ >>> + char *s; >>> + u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED; >>> + u32 rev = 0; >>> + >>> + s = getenv("maxspeed"); >>> + if (s) >>> + maxspeed = simple_strtoul(s, NULL, 10); >>> + >>> + switch (maxspeed) { >>> + case 456000: >>> + rev |= 3; >>> + break; >>> + case 408000: >>> + rev |= 2; >>> + break; >>> + case 372000: >>> + rev |= 1; >> wondering if the |= makes any sense... > > Yeah, I put it in there in case additional fields pop-up > in board revision. It doesn't make a lot of sense now so > I will remove it. thx. > >>> + break; >>> + } >>> + >>> + return rev; >> >> IMHO, the logic could be simplified? >> >> option 1: >> u8 rev=0; >> s = simple_strtoul(s, NULL, 10); aarrg.. emailing with eyes half shut mistake should have been: s = getenv("maxspeed"); >> if (s) { >> switch (simple_strtoul(s, NULL, 10)) { >> case 456000: >> rev = 3; >> break; >> case 408000: >> rev = 2; >> break; >> case 372000: >> rev = 1; >> break; >> } >> } > > Not sure how this simplifies the logic. I'd argue multiple strtoul > calls are actually better avoided. How does it handle the case where > max speed is to be set using board configuration? > my bad, the above should explain.. >> >> option 2: >> if you think that the speeds could get added in the future, that switch >> is going to look pretty ugly.. use a lookup instead.. > > Right now there is no known plan to add more speeds so I will > stick with the switch. You are right, option of using a lookup > deserves a look-in if there were a lot more speed steps. ok. Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel 2010-08-12 5:43 ` Nishanth Menon @ 2010-08-12 6:14 ` Nori, Sekhar 2010-08-12 11:38 ` Nishanth Menon 0 siblings, 1 reply; 6+ messages in thread From: Nori, Sekhar @ 2010-08-12 6:14 UTC (permalink / raw) To: u-boot Hi Nishanth, On Thu, Aug 12, 2010 at 11:13:10, Nishanth Menon wrote: > On 08/11/2010 10:37 AM, Nori, Sekhar wrote: > > Hi Nishanth, > > > > On Wed, Aug 11, 2010 at 09:33:29, Nishanth Menon wrote: > >> On 08/10/2010 06:39 AM, Sekhar Nori wrote: > > > >>> diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c > >>> index 959b2c6..6a6d4fb 100644 > >>> --- a/board/davinci/da8xxevm/da850evm.c > >>> +++ b/board/davinci/da8xxevm/da850evm.c > >>> @@ -70,6 +70,44 @@ static const struct lpsc_resource lpsc[] = { > >>> { DAVINCI_LPSC_GPIO }, > >>> }; > >>> > >>> +#ifndef CONFIG_DA850_EVM_MAX_SPEED > >>> +#define CONFIG_DA850_EVM_MAX_SPEED 300000 > >>> +#endif > >>> + > >>> +/* > >>> + * get_board_rev() - setup to pass kernel board revision information > >>> + * Returns: > >>> + * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part > >>> + * 0 - 300 MHz > >>> + * 1 - 372 MHz > >>> + * 2 - 408 MHz > >>> + * 3 - 456 MHz > >>> + */ > >>> +u32 get_board_rev(void) > >>> +{ > >>> + char *s; > >>> + u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED; > >>> + u32 rev = 0; > >>> + > >>> + s = getenv("maxspeed"); > >>> + if (s) > >>> + maxspeed = simple_strtoul(s, NULL, 10); > >>> + > >>> + switch (maxspeed) { > >>> + case 456000: > >>> + rev |= 3; > >>> + break; > >>> + case 408000: > >>> + rev |= 2; > >>> + break; > >>> + case 372000: > >>> + rev |= 1; [...] > > > >>> + break; > >>> + } > >>> + > >>> + return rev; > >> > >> IMHO, the logic could be simplified? > >> > >> option 1: > >> u8 rev=0; > >> s = simple_strtoul(s, NULL, 10); > aarrg.. emailing with eyes half shut mistake > should have been: > s = getenv("maxspeed"); > >> if (s) { > >> switch (simple_strtoul(s, NULL, 10)) { > >> case 456000: > >> rev = 3; > >> break; > >> case 408000: > >> rev = 2; > >> break; > >> case 372000: > >> rev = 1; > >> break; > >> } > >> } > > > > Not sure how this simplifies the logic. I'd argue multiple strtoul > > calls are actually better avoided. How does it handle the case where > > max speed is to be set using board configuration? > > > my bad, the above should explain.. I still don't see how this handles the case when maxspeed env variable is not set. The above code will just default to rev = 0 in that case. We haven't gotten rid of any constructs either so not sure what the simplification here is. Thanks, Sekhar ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel 2010-08-12 6:14 ` Nori, Sekhar @ 2010-08-12 11:38 ` Nishanth Menon 0 siblings, 0 replies; 6+ messages in thread From: Nishanth Menon @ 2010-08-12 11:38 UTC (permalink / raw) To: u-boot On 08/12/2010 01:14 AM, Nori, Sekhar wrote: > > Hi Nishanth, > > On Thu, Aug 12, 2010 at 11:13:10, Nishanth Menon wrote: >> On 08/11/2010 10:37 AM, Nori, Sekhar wrote: >>> Hi Nishanth, >>> >>> On Wed, Aug 11, 2010 at 09:33:29, Nishanth Menon wrote: >>>> On 08/10/2010 06:39 AM, Sekhar Nori wrote: >>> >>>>> diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c >>>>> index 959b2c6..6a6d4fb 100644 >>>>> --- a/board/davinci/da8xxevm/da850evm.c >>>>> +++ b/board/davinci/da8xxevm/da850evm.c >>>>> @@ -70,6 +70,44 @@ static const struct lpsc_resource lpsc[] = { >>>>> { DAVINCI_LPSC_GPIO }, >>>>> }; >>>>> >>>>> +#ifndef CONFIG_DA850_EVM_MAX_SPEED >>>>> +#define CONFIG_DA850_EVM_MAX_SPEED 300000 >>>>> +#endif >>>>> + >>>>> +/* >>>>> + * get_board_rev() - setup to pass kernel board revision information >>>>> + * Returns: >>>>> + * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part >>>>> + * 0 - 300 MHz >>>>> + * 1 - 372 MHz >>>>> + * 2 - 408 MHz >>>>> + * 3 - 456 MHz >>>>> + */ >>>>> +u32 get_board_rev(void) >>>>> +{ >>>>> + char *s; >>>>> + u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED; >>>>> + u32 rev = 0; >>>>> + >>>>> + s = getenv("maxspeed"); >>>>> + if (s) >>>>> + maxspeed = simple_strtoul(s, NULL, 10); >>>>> + >>>>> + switch (maxspeed) { >>>>> + case 456000: >>>>> + rev |= 3; >>>>> + break; >>>>> + case 408000: >>>>> + rev |= 2; >>>>> + break; >>>>> + case 372000: >>>>> + rev |= 1; > > [...] > >>> >>>>> + break; >>>>> + } >>>>> + >>>>> + return rev; >>>> >>>> IMHO, the logic could be simplified? >>>> >>>> option 1: >>>> u8 rev=0; >>>> s = simple_strtoul(s, NULL, 10); >> aarrg.. emailing with eyes half shut mistake >> should have been: >> s = getenv("maxspeed"); >>>> if (s) { >>>> switch (simple_strtoul(s, NULL, 10)) { >>>> case 456000: >>>> rev = 3; >>>> break; >>>> case 408000: >>>> rev = 2; >>>> break; >>>> case 372000: >>>> rev = 1; >>>> break; >>>> } >>>> } >>> >>> Not sure how this simplifies the logic. I'd argue multiple strtoul >>> calls are actually better avoided. How does it handle the case where >>> max speed is to be set using board configuration? >>> >> my bad, the above should explain.. > > I still don't see how this handles the case when maxspeed env variable > is not set. The above code will just default to rev = 0 in that case. it does the same here as well (default rev = 0). and the fact that compared to v1, |= was avoided (this is part of v2 of the patch). > > We haven't gotten rid of any constructs either so not sure what the > simplification here is. got rid of a var maxspeed - but I doubt it will have much benefit, please consider my objection withdrawn.. just to point to the note that the var has not much of a lifetime or a function beyond providing data to the switch.. Regards, Nishanth Mnon ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-08-12 11:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-10 11:39 [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel Sekhar Nori 2010-08-11 4:03 ` Nishanth Menon 2010-08-11 15:37 ` Nori, Sekhar 2010-08-12 5:43 ` Nishanth Menon 2010-08-12 6:14 ` Nori, Sekhar 2010-08-12 11:38 ` Nishanth Menon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox