Re: [U-Boot] [STATUS] v2010.12-rc2 released
Hi Ben, On Wed, Dec 01, 2010 at 01:20:36, Ben Gardiner wrote: On Tue, Nov 30, 2010 at 10:00 AM, Wolfgang Denk w...@denx.de wrote: Hello everybody. I apologise for being a bit late with this announcement: * U-Boot v2010.12-rc2 was released on Sunday, November 28. * Release v2010.12 is (still) scheduled in 13 days: on December 13, 2010. Please help testing, and check if all your relevant patches have been included. da850evm was broken by commit 4f6fc15b42776b12244af8aa28da42c8e6497742 Author: Sekhar Nori nsek...@ti.com Date: Fri Nov 19 11:39:48 2010 -0500 DA850 EVM: passing maximum clock rate information to kernel The TI DA850/OMAP-L138/AM18x EVM can be populated with devices having different maximum allowed CPU clock rating. The maximum clock the chip can support can only be determined from the label on the package (not software readable). Introduce a method to pass the maximum allowed clock rate information to kernel using ATAG_REVISION. The kernel uses this information to determine the maximum cpu clock rate reachable using cpufreq. Note that U-Boot itself does not set the CPU clock rate. The CPU clock is setup by a primary bootloader (UBL). The rate setup by UBL could be different from the maximum clock rate supported by the device. Signed-off-by: Sekhar Nori nsek...@ti.com Signed-off-by: Sandeep Paulraj s-paul...@ti.com With the introduction of this patch the board freezes after UBL jumps to 0xC108; however, I don't think it was the implementation in this patch that caused the breakage. Hmm, I had tested DA850 EVM yesterday with U-Boot 2010.12-rc2, and was able to successfully boot (at least most of the times). ---logs Booting with TI UBL Device OPP (300MHz, 1.2V) U-Boot 2010.12-rc2 (Nov 30 2010 - 11:55:35) I2C: ready DRAM: 128 MiB Using default environment In:serial Out: serial Err: serial Net: Ethernet PHY: GENERIC @ 0x00 DA850-evm Some of the times, the boot hung after printing DRAM: 128 MiB, but never did it hang without printing anything. Removing the added #define CONFIG_REVISION_TAG does not fix the freeze. Furthermore, reverting the patch on top of 2010.12-rc2 does not fix the freeze either. Finally to add to the (/my) confusion, Ah, that is a sure indication that the issue is elsewhere. adding OPTFLAGS=-O0 -g to 4f6fc15b42776b12244af8aa28da42c8e6497742^ (the commit parent) results in a boot freeze, where there was none before the addition of the compiler flags. I suppose we should be suspecting the timers? I think I recall hearing that the time code can mix poorly with relocation. I am not sure about that (haven't been following all the relocation related changes lately). Since it is booting for me, may be it is related to the build environment? I am using CodeSourcery 2009q1-203 for building the U-Boot. Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RESEND PATCH v4 2/2] TI: DaVinci DA850 EVM: support passing maximum allowed cpu clock rate information to kernel
On Wed, Nov 17, 2010 at 17:02:50, Nori, Sekhar wrote: The TI DA850/OMAP-L138/AM18x EVM can be populated with devices having different maximum allowed CPU clock rating. The maximum clock the chip can support can only be determined from the label on the package (not software readable). Introduce a method to pass the maximum allowed clock rate information to kernel using ATAG_REVISION. The kernel uses this information to determine the maximum cpu clock rate reachable using cpufreq. Note that U-Boot itself does not set the CPU clock rate. The CPU clock is setup by a primary bootloader (UBL). The rate setup by UBL could be different from the maximum clock rate supported by the device. Signed-off-by: Sekhar Nori nsek...@ti.com Ben had sent a Tested-by: earlier which I failed to include here. Sorry about that. Here it is: Tested-by: Ben Gardiner bengardi...@nanometrics.ca Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] TI: DaVinci DA850 EVM: support passing maximum allowed cpu clock rate information to kernel
Hi Ben, On Tue, Sep 28, 2010 at 19:31:02, Ben Gardiner wrote: On Fri, Sep 24, 2010 at 1:10 AM, Nori, Sekhar nsek...@ti.com wrote: Hello All, On Fri, Aug 27, 2010 at 10:44:58, Nori, Sekhar wrote: The TI DA850/OMAP-L138/AM18x EVM can be populated with devices having different maximum allowed CPU clock rating. The maximum clock the chip can support can only be determined from the label on the package (not software readable). Introduce a method to pass the maximum allowed clock rate information to kernel using ATAG_REVISION. The kernel uses this information to determine the maximum cpu clock rate reachable using cpufreq. Note that U-Boot itself does not set the CPU clock rate. The CPU clock is setup by a primary bootloader (UBL). The rate setup by UBL could be different from the maximum clock rate supported by the device. Any more feedback on this patch? There are couple of kernel patches which depend on this, that's why I ask. FWIW: it's fine with me. Applies cleanly to 3df61957938586c512c17e72d83551d190400981 of u-boot/next. Tested on da850evm -- bootm of linux uImage works with the patch 'da850evm: fix linux bootparam address' aplied. The kernel uImage used did not have support for parsing the ATAG_REVISION information since that patch has not been posted to the davinci-linux list. Tested-by: Ben Gardiner bengardi...@nanometrics.ca Thanks for the testing! Regards, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] TI: DaVinci DA850 EVM: support passing maximum allowed cpu clock rate information to kernel
Hello All, On Fri, Aug 27, 2010 at 10:44:58, Nori, Sekhar wrote: The TI DA850/OMAP-L138/AM18x EVM can be populated with devices having different maximum allowed CPU clock rating. The maximum clock the chip can support can only be determined from the label on the package (not software readable). Introduce a method to pass the maximum allowed clock rate information to kernel using ATAG_REVISION. The kernel uses this information to determine the maximum cpu clock rate reachable using cpufreq. Note that U-Boot itself does not set the CPU clock rate. The CPU clock is setup by a primary bootloader (UBL). The rate setup by UBL could be different from the maximum clock rate supported by the device. Any more feedback on this patch? There are couple of kernel patches which depend on this, that's why I ask. Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/2] TI: DaVinci DA850 EVM: support passing maximum allowed cpu clock rate information to kernel
Hi Stefano, On Thu, Aug 19, 2010 at 13:51:46, Stefano Babic wrote: Sekhar Nori wrote: The TI DA850/OMAP-L138/AM18x EVM can be populated with devices having different maximum allowed CPU clock rating. The maximum clock the chip can support can only be determined from the label on the package (not software readable). Introduce a method to pass the maximum allowed clock rate information to kernel using ATAG_REVISION. The kernel uses this information to determine the maximum cpu clock rate reachable using cpufreq. Note that U-Boot itself does not set the CPU clock rate. The CPU clock is setup by a primary bootloader (UBL). The rate setup by UBL could be different from the maximum clock rate supported by the device. Signed-off-by: Sekhar Nori nsek...@ti.com --- Changes in v3: a) renamed maxspeed to maxcpuclk b) add information regarding the new environment variable in README.davinci c) use if-else instead of switch to check for value range rather than specific values of maxcpuclk d) change comment to document values returned in bit[0-3] by get_board_rev() in binary board/davinci/da8xxevm/da850evm.c | 33 + doc/README.davinci| 13 + include/configs/da850evm.h|1 + 3 files changed, 47 insertions(+), 0 deletions(-) Hi, sorry if I come only late in the discussion, I have a general question. As I see, the UBL does not take care at all of the possibility to have different frequencies, and sets the PLL with a fix value (300 Mhz, as I read in sources). From my point of view, it makes sense to use the maximum allowed cpu clock if we then set the CPU to increase the performances. Yes, cpufreq in kernel can do this based on cpu load. If you set the value in a u-boot environment, the value could be easy overwritten and a wrong value could be displayed. So I have doubt on using an environment to get a so hardware-related value.. Overwritten by whom and displayed where? Do you plan to use the maximum cpu clock to set the PLL ? Or is it only for info purposes ? The kernel will use the value to understand the maximum frequency it can reach using cpufreq. So yes, kernel uses this to set the PLL. Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Detlev, On Tue, Aug 17, 2010 at 20:46:16, Detlev Zundel wrote: And by the way, if you still fail to see any point in my reasoning, then remember that I never NAKed your patches - I was only trying to help. The repsective custodians have the final word over acceptance. Thanks for the review. I will go ahead and submit a v3 based upon the items agreed upon for change. Regards, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/2] TI: DaVinci DA850 EVM: support passing maximum allowed cpu clock rate information to kernel
Hi Detlev, On Wed, Aug 18, 2010 at 21:23:46, Detlev Zundel wrote: Hi Sekhar, The TI DA850/OMAP-L138/AM18x EVM can be populated with devices having different maximum allowed CPU clock rating. The maximum clock the chip can support can only be determined from the label on the package (not software readable). Introduce a method to pass the maximum allowed clock rate information to kernel using ATAG_REVISION. The kernel uses this information to determine the maximum cpu clock rate reachable using cpufreq. Ok, so I see you ignore my comments on not using the ATAG_REVISION tag for passing clock frequncies which is fair enough if nobody else No, not clock frequncies, but maximum device operating point information (which depends on the silicon populated on the EVM). I am sure you mean exactly that, but using terminology loosely can create a very different impression. But then please _at least_ document this in the README and not only in the git diff files. This is still the most obscure aspects of the change for me and should be prominent in the documentation. Sure, will send out an update. Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Detlev,
On Tue, Aug 17, 2010 at 17:42:32, Detlev Zundel wrote:
Yes, but I am still unconvinced ATAG_REVISION is not suitable for this
purpose.
When writing code which should also be maintainable by other people it
is a good idea to consider common expectations also of other people.
Agreed. And I am open to concrete suggestions on how this could be
better done.
Let's take a step back. What information do you want to pass to the
Linux kernel? What does the Linux kernel do with it?
From the patch description: The kernel uses this information to determine
the maximum speed reachable using cpufreq
As far as I understand you, it is a maximum frequency, correct? The
absolute limit is given by the labeling of individual parts - but for
whatever reason - maybe the user also wants to influence that.
The user does not influence this rating. Do you call it influenced by
user because there is an environment variable to set this? The way
the patch is written it can also be specified in the board configuration
header file. The environment variable is just a convenience option. The
expectation is that the user will rebuild U-Boot based on which silicon
he is using so he doesn't have to set the environment variable at all.
So, the user is just configuring the software according to the
hardware. There no user influence on the silicon speed grade.
So why
not call it maximum operating frequency?
Yes, that's a correct description. Where do you want to call it such?
If you really do have to pass the informatino to the kernel (why does no
other SoC in ARM use such a aconcept yet? How do they handle multiple
frequencies?) and because I'm not too familiar with the ATAGs (flat
device trees are far superior), let's see what the current Linux kernel
declares. [Studying the code] Ah, in arch/avr32/include/asm/setup.h
someone came up with the idea to create a generic ATAG_CLOCK tag. That
does look promising - it scales, i.e. one can specify multiple clocks
(i.e. core, bus, whatever) and it uses long long so it will not overflow
in the near future.
Why not reuse such an existing concept which matches your usage much
better (arch/arm/include/asm/setup.h comments ATAG_REVISION as board
revision)?
Note again that we are not trying to pass information regarding the
current clock speed here. We are passing information regarding a silicon
characteristic. The DA850 kernel reads the system registers directly to
find out the clock speed. Even in the avr32 platform this ATAG is unused.
From kernel file: arch/avr32/kernel/setup.c:
static int __init parse_tag_clock(struct tag *tag)
{
/*
* We'll figure out the clocks by peeking at the system
* manager regs directly.
*/
return 0;
}
__tagtable(ATAG_CLOCK, parse_tag_clock);
Anyway, I can see the talk of speed and board revision has created
some confusion.
What if instead of maxspeed, I named the variable as soctype and had
values like am18x-300, am18x-375, am18x-450 passed to it?
It means the same thing, but will probably create a different perception?
I wanted to avoid taking this route since the same code supports
different SoC part numbers and passing part number specific values
can cause some confusion for users of other parts. That is all.
The question is why should a new ARM ATAG be introduced if an existing
one is good enough for the purpose?
Using HZ would probably settle the which unit is used question, but
the code would overflow at ~4.2 GHz and the numbers will be large and
entry errors have to be expected. As Hz is too fine for CPU frequencies
this would lead me to use either kilo or megaherz but I would express it
in the variable name.
I don't have a very strong inclination on this so I will go with
your suggestion.
Did you check if we can learn from other code already present in U-Boot?
Let's see - in arch/mips/cpu/incaip_clock.c there is an environment
variable cpuclk which is in MHz. Aha, the 8xx parts also use a
cpuclk environment variable which is even documented in
doc/README.MPC866.
Yes, U-Boot online documentation also has a reference to it:
http://www.denx.de/wiki/view/DULG/UBootEnvVariables.
Ah, now I'm in a tight spot - contrary to my previous writings when I
belived we did not have a comparably construct - I would now vote to use
exactly the same name and thus unfortunately not use a _mhz suffix but
still specify the clock in mhz.
You mean replace maxspeed by cpuclk? As I have noted a number of times
before, we are not passing the cpu clock speed here. That information kernel
directly reads from system registers. No need to pass it from U-Boot. The
example you are giving is not the right comparison.
The cpuclk variable in MPC866 is used to set the current cpu speed and pass that
information to kernel. I had a feeling this confusion is going to arise and
that is why I noted in my patch description:
Note that U-Boot itself does not set
Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Detlev, On Fri, Aug 13, 2010 at 16:09:41, Detlev Zundel wrote: Hi Nori, A revision for me is attached to certain bugs/problems which we may need to work around in software. Think about something like we can enable caching only on rev2 CPUs. For all I know, the ATAG_REVISION tag seems to capture this aspect. We will most likely end up using this aspect of ATAG_REVISION as further revisions of the EVM appear. The maximum speed of a CPU is orthogonal for me, i.e. there can still be a fast and a slow rev 1 CPU Note that we are not taking about CPU being fast or slow at a given point of time. We are talking about whether the cpu on the board can support a given rate. It means that the silicon has been characterized (and probably priced) differently. So you can actually treat it as a different CPU revision. Well yes, you _can_ treat that as a revision, but certainly I would not understand what you mean. A CPU revision for me is connected to the physical chip mask on the CPU (i.e. what goes into the manufacturing process) and the maximum allowed operating frequency is a property of an individual chip possibly only detected by actual measurements (what comes out of manufacturing). I never heard people talk about _functionally equivalent_ CPUs with different graded operating frequencies as different revisions, but YMMV. It would have been nice if hardware provided a way to detect this difference between chips, but unfortunately it does not. In fact, all of these speed graded parts are sold separately with different datasheets. Please see the 375 and 456 AM1x parts: http://focus.ti.com/paramsearch/docs/parametricsearch.tsp?family=dspsectionId=2tabId=2641familyId=1877paramCriteria=no They surely have differnet part numbers and this is perfectly fine. But let's take a look at your first link. I see two parts here with different allowed frequencies: AM1806-375 and AM1806-456. _Both_ links lead to the same page with these datasheets: This is just a documentation trick to make sure common aspects don't have to be maintained separately. Yes, but I am still unconvinced ATAG_REVISION is not suitable for this purpose. When writing code which should also be maintainable by other people it is a good idea to consider common expectations also of other people. Agreed. And I am open to concrete suggestions on how this could be better done. + +/* + * 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 The description says it returns bit[0-3] which may seem that those canstants are encoded by a single bit each, whereas the code uses integer values. Change either the comment or the code. [0-3] usually indicates the range the range 0 to 3. Do you have suggestions on how else this might be documented? As I said, bit[0-3] denotes the 4 bits 0-3, i.e. a range from 0-15. (In this context the numbers below would actually translate into individual bits.) Just drop this bit[0-3] and it is clear. Why would dropping bit[0-3] make anything clear? As I mentioned above other bits can be used in future to determine other aspects of board revision. May be I can add that information. Is the following more clear? /* * 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 * bit[4-31]Reserved (unused for now) */ Let me try to reformulate the ambiguity that I see here - when a documentation tells that bit[0-3] is used, then I would really expect something like 0001 - xx 0010 - yy ; 0100 - zz. If I don't see this and read 0 - xx; 1 - yy; 2 - zz; 3 - aa on first read I would interpret this as bit 0 - xx; bit 1 - yy; bit 2 - zz; bit 3 - aa which is obviously _not_ what you do. Okay. I can document the values in binary instead of decimal if that helps readability. Does the following look good? /* * get_board_rev() - setup to pass kernel board revision information * Returns: * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part * b - 300 MHz * 0001b - 372 MHz * 0010b - 408 MHz * 0011b - 456 MHz * bit[4-31]Reserved (unused for now) */ Using HZ would probably settle the which unit is used question, but the code would overflow at ~4.2 GHz and the numbers will be large and entry errors have to be expected. As
Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Detlev, On Fri, Aug 13, 2010 at 14:00:21, Detlev Zundel wrote: A revision for me is attached to certain bugs/problems which we may need to work around in software. Think about something like we can enable caching only on rev2 CPUs. For all I know, the ATAG_REVISION tag seems to capture this aspect. We will most likely end up using this aspect of ATAG_REVISION as further revisions of the EVM appear. The maximum speed of a CPU is orthogonal for me, i.e. there can still be a fast and a slow rev 1 CPU Note that we are not taking about CPU being fast or slow at a given point of time. We are talking about whether the cpu on the board can support a given rate. It means that the silicon has been characterized (and probably priced) differently. So you can actually treat it as a different CPU revision. In fact, all of these speed graded parts are sold separately with different datasheets. Please see the 375 and 456 AM1x parts: http://focus.ti.com/paramsearch/docs/parametricsearch.tsp?family=dspsectionId=2tabId=2641familyId=1877paramCriteria=no and the 300 MHz OMAP-L1x parts: http://focus.ti.com/paramsearch/docs/parametricsearch.tsp?family=dspsectionId=2tabId=2231familyId=1621paramCriteria=no Moreover, CPU revision only occupies bits 0-3 of the ATAG_REVISION. As you mention the rest of the bits can be used to mark other changes in the EVMs (bug fixes etc). but still we cannot enable cache. Do you see my point? No, let me know if the above explanation clarifies the topic for you. I hope I explained my point better this time. Yes, but I am still unconvinced ATAG_REVISION is not suitable for this purpose. 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. I do not understand how the UBL gets to set the _U-Boot_ environment variable maxspeed. Can you please explain how this is done? UBL does not (cannot) set the maxspeed variable. The user of U-Boot is expected to set it based on what he sees on the packaging. Alternately he can also change the U-Boot configuration file and re-build U-Boot. UBL will setup the board to work at the common frequency of 300 MHz. I see, so please write in the documentation that the user is supposed to set this variable correctly for his chip. I did not read this from the text. Sure, I can include this in the commit text and in the README I promised. + +/* + * 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 The description says it returns bit[0-3] which may seem that those canstants are encoded by a single bit each, whereas the code uses integer values. Change either the comment or the code. [0-3] usually indicates the range the range 0 to 3. Do you have suggestions on how else this might be documented? As I said, bit[0-3] denotes the 4 bits 0-3, i.e. a range from 0-15. (In this context the numbers below would actually translate into individual bits.) Just drop this bit[0-3] and it is clear. Why would dropping bit[0-3] make anything clear? As I mentioned above other bits can be used in future to determine other aspects of board revision. May be I can add that information. Is the following more clear? /* * 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 * bit[4-31]Reserved (unused for now) */ Moreover I do not like that you call the variable maxpseed but interpret the value in kHz. Maybe the variable should be called maxspeed_khz? I am hoping the documentation promised in the response above will help clarify its usage. I wanted to keep the variable name short. Shortness is a worthwhile goal but clearness even more so. Those 4 characters would prevent anyone from ever looking into the documentation on deciding what unit the value is in. Personally I believe this is worth it. I see. Let me toss between this and specifying the speed in HZ and leaving the variable as is. I guess you would be OK both ways? Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
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 30
+#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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Detlev,
On Thu, Aug 12, 2010 at 18:23:42, Detlev Zundel wrote:
Hi Sekhar,
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.
Do I understand you correctly that you _misuses_ an atag defined to
carry the revision of a CPU to carry the maximum allowed clock
frequency?
The EVM can be populated with devices of different speed grades and this
patch treats those as different revisions of the EVM. Why would this be
treated as a misuse of ATAG_REVISION?
Is this really a good idea? I can easily imagine different
CPU revisions with different maximum clock frequencies. How will you
handle that?
I don't think I understood you. This patch _is_ meant to handle
different revisions of DA850 EVMs populated with DA850 devices
of differing speed grades.
Is the counterpart in the Linux kernel already implemented?
It is implemented, but its submission upstream depends on this patch.
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.
I do not understand how the UBL gets to set the _U-Boot_ environment
variable maxspeed. Can you please explain how this is done?
UBL does not (cannot) set the maxspeed variable. The user of U-Boot is
expected to set it based on what he sees on the packaging. Alternately
he can also change the U-Boot configuration file and re-build U-Boot.
UBL will setup the board to work at the common frequency of 300 MHz.
Signed-off-by: Sekhar Nori nsek...@ti.com
---
v2: removed unnecessary logical OR while constructing revision value
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 eeb456c..0eb3608 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 30
+#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
The description says it returns bit[0-3] which may seem that those
canstants are encoded by a single bit each, whereas the code uses
integer values. Change either the comment or the code.
[0-3] usually indicates the range the range 0 to 3. Do you have
suggestions on how else this might be documented?
+ */
+u32 get_board_rev(void)
+{
+ char *s;
+ u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED;
+ u32 rev = 0;
+
+ s = getenv(maxspeed);
You introduce a new magic environment variable, so it should be
documented at least in a board specific readme file.
Sure. Will do.
Moreover I do not like that you call the variable maxpseed but
interpret the value in kHz. Maybe the variable should be called
maxspeed_khz?
I am hoping the documentation promised in the response above
will help clarify its usage. I wanted to keep the variable name
short.
+ 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;
+ }
Although the speeds are maximum values you check for _exact_ matches.
Does this make sense? Why not use increasing less than compares?
Can do and will change.
Thanks for the feedback!
Thanks,
Sekhar
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
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 30
+#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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Improve DaVinci SPI speed
Hi Delio, On Sun, May 23, 2010 at 17:11:56, Delio Brignoli wrote: Hello Wolfgang, On 21/05/2010, at 15:13, Wolfgang Denk wrote: + *rxp = buf_reg_val 0xFF; + rxp++; + } Please change into: if (rxp) *rxp++ = buf_reg_val 0xFF; Are you sure? Is folding that 3 line block into a one liner really worth the loss of readability? I know what that means, so do you and many others on this ML, but that's bound to raise a few eyebrows. A quick google search reveals quite a few articles devoted to clearing up the confusion about the semantics of that pointer dereference and post-increment. Just a few years ago I would have preferred your suggestion, but now I find that readability trumps cleverness, at least in a case like this. In the end it's your decision: if you like to be kinky, it's your prerogative ;-) Looking for the pointer de-reference and increment expression in Linux kernel, I had ~4500 hits and ~600 hits in U-Boot. So, may be this is not such an uncommon expression after all (at least in the kernel/U-Boot world). Considering this, can you please accept the change Wolfgang is asking for for so this useful patch can move forward? After all, code readability is also about pattern matching in the mind and if developers are already used to seeing this expression not many would find it odd. Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Improve DaVinci SPI speed
Hi Delio,
On Thu, May 13, 2010 at 18:27:51, Delio Brignoli wrote:
Reduce the number of reads per byte transferred on the BUF register from 2 to
1 and
take advantage of the TX buffer in the SPI module.
The patch looks good to me.
Can you please publish some sort of numbers in the
patch description indicating the performance improvement
achieved?
A minor nit below:
Signed-off-by: Delio Brignoli dbrign...@audioscience.com
---
drivers/spi/davinci_spi.c | 67 +++-
1 files changed, 35 insertions(+), 32 deletions(-)
diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
index 60ba007..b4a74de 100644
--- a/drivers/spi/davinci_spi.c
+++ b/drivers/spi/davinci_spi.c
@@ -131,6 +131,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
{
[...]
+ /* write to DAT1 is required to keep the serial
transfer going */
+ /* we just terminate when we reach the end */
+ if((o_cnt == (len -1)) (flags SPI_XFER_END)) {
Please include a space before the '-'
Thanks,
Sekhar
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Improve DaVinci SPI speed
Hi Sandeep, On Thu, May 13, 2010 at 23:01:31, Paulraj, Sandeep wrote: Hello Paulraj, On 13/05/2010, at 17:10, Paulraj, Sandeep wrote: Reduce the number of reads per byte transferred on the BUF register from 2 to 1 and take advantage of the TX buffer in the SPI module. May I ask which chip device this was tested on. Sure, it was tested on a LogicPD Zoom(tm) OMAP-L138 eXperimenter Kit (da850_omapl138_evm_config). -- If nobody has objections then I will apply it this weekend to u-boot-ti I am yet to review this (came in only yesterday evening). Can you please wait till early next week before applying it. Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] da850/L138 SPI flash transfer speed
Hello Delio, On Sat, Apr 24, 2010 at 05:00:49, Delio Brignoli wrote: Hello Wolfgang, On 24/04/2010, at 10:29 AM, Wolfgang Denk wrote: please mind the NetiQuette and restrict your line length to some 70 charatcers or so. Thanks. Will do, thanks. Everything is slow as caches are not enabled. OK, so reducing the number of reads from registers and writes to RAM should improve performance. To you knowledge, would enabling the cache for davinci da850 break anything in U-Boot? It would break EMAC driver for sure. The driver does not flush/invalidate the buffers under the assumption that the data cache is kept disabled. Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] da850/L138 SPI flash transfer speed
On Sat, Apr 24, 2010 at 03:59:22, Wolfgang Denk wrote: Dear Delio Brignoli, please mind the NetiQuette and restrict your line length to some 70 charatcers or so. Thanks. In message 4d573595-069a-4490-af2d-38ed3aad7...@audioscience.com you wrote: I am working on reducing boot time on an L138 EVM and SPI flash transfer speed is currently the worst offender. U-Boot transfers from the SPI flash at 0.6Mbytes/s, this a lot slower than I would expect for a 50MHz SPI clock. Using a scope we found that the chip select is active throughout the transfer (as expected), we see ~160ns bursts of activity on the clock line for each byte transferred (8 bits @ 50MHz) with 1us idle periods in between. Where does the 1us delay between byte transfers come from? I s reading data bytes from the SPI registers very slow or is writing to RAM one byte at a time slowing the transfer? Everything is slow as caches are not enabled. The only delays being configured in the driver are the chip-select hold time delays which should not matter here as you see delays inserted between bytes which are part of a single transfer. I am starting to doubt peripheral mis-configuration as a possible cause here. One way to mitigate the slow access to RAM would be to take advantage of external RAM burst by using EDMA. That will be some work because there is no other example of EDMA usage in U-Boot. Thanks, Sekhar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
