Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Wolfgang, > Dear Detlev Zundel, > > In message you wrote: >> >> > Even though its pretty descriptive - for the sake of consistency I >> > recommend to omit the "_mhz" part. >> >> I know that I will mutter a not so nice remark every time I use the >> variable and am forced to look up the documentation for a single setenv >> command instead of typing 4 more characters. > > I understand what you mean, but then - have a guess on how many "*clk" > and "*clock" symbols we use in U-Boot, and how many in Linux - and > how many of these use a unit suffix? Sure - if one uses the canonical unit, then no suffix is needed. I never said anything else. > Actually I also think that using MHz as unit is broken by design - > keep in mind that we use Hz basicly everywhere else, and for a reason. > >> I still remember the days of "clocks_in_mhz" and the waste of time it >> produced - maybe this is why I continue to raise this concern. > > Note that the Linux kernel changed this interface from MHz to Hz by > then! Sure - still this left an impression on me ;) > Where would be the problem? In this case, we are talking about an > environment variable; here we are using a string representation of > (more or less) arbitrary size. > > If we really shoould see CPU clocks way beyond 4 GHz one day, the code > needs to deal with htat in an appropriate way - but even if you decide > to operate in a MHz unit internally, you can still use Hz at the > interface. Yep, I know. I only stated that using Hz for such numbers for user changeable values probably will create a few "misentries" which will be somewhat hard to diagnose. If this is not a concern - I'll gladly go with the canonical unit. > As long as all other clocks are specified in Hz, I strongly recommend > to do the same here, too. Well sure - as always, it's a compromise. One should consider all the pros and cons and then _decide and document_. We are in complete accordance. >> > Can we agree on "cpumaxclk" ? >> >> If nobody else shares my concerncs this sounds good. > > Thinking again about this, I actually tend to prefer > > "maxcpuclk" > > :-) Whatever - my concerns were not coupled to permutations ;) Cheers Detlev -- Microsoft gives you windows, Linux gives you the whole house. -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de ___ 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
Dear Detlev Zundel, In message you wrote: > > > Even though its pretty descriptive - for the sake of consistency I > > recommend to omit the "_mhz" part. > > I know that I will mutter a not so nice remark every time I use the > variable and am forced to look up the documentation for a single setenv > command instead of typing 4 more characters. I understand what you mean, but then - have a guess on how many "*clk" and "*clock" symbols we use in U-Boot, and how many in Linux - and how many of these use a unit suffix? Actually I also think that using MHz as unit is broken by design - keep in mind that we use Hz basicly everywhere else, and for a reason. > I still remember the days of "clocks_in_mhz" and the waste of time it > produced - maybe this is why I continue to raise this concern. Note that the Linux kernel changed this interface from MHz to Hz by then! Where would be the problem? In this case, we are talking about an environment variable; here we are using a string representation of (more or less) arbitrary size. If we really shoould see CPU clocks way beyond 4 GHz one day, the code needs to deal with htat in an appropriate way - but even if you decide to operate in a MHz unit internally, you can still use Hz at the interface. As long as all other clocks are specified in Hz, I strongly recommend to do the same here, too. > > Can we agree on "cpumaxclk" ? > > If nobody else shares my concerncs this sounds good. Thinking again about this, I actually tend to prefer "maxcpuclk" :-) Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Es ist offensichtlich, dass das menschliche Gehirn wie ein Computer funktioniert. Da es keine dummen Computer gibt, gibt es also auch keine dummen Menschen. Nur ein paar Leute, die unter DOS laufen. -- ___ 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 v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Wolfgang, > Dear Detlev Zundel, > > In message you wrote: >> >> > 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. >> >> Ok, then currently I would vote for "cpumaxspeed_mhz". > > Even though its pretty descriptive - for the sake of consistency I > recommend to omit the "_mhz" part. I know that I will mutter a not so nice remark every time I use the variable and am forced to look up the documentation for a single setenv command instead of typing 4 more characters. I still remember the days of "clocks_in_mhz" and the waste of time it produced - maybe this is why I continue to raise this concern. > Also, I feel that "clock" would be more appropriate than "speed". And > we should consider that we already use "cpuclk" for very similar > purposes in U-Boot. > > Can we agree on "cpumaxclk" ? If nobody else shares my concerncs this sounds good. Cheers Detlev -- #!/usr/bin/perl $c="print\"\#\!\/usr\/bin\/perl\ \\\$c\=\\\"\"\.quotemeta\(\$c\)\.\"\\\"\;\\n\$c;\""; print"#!/usr/bin/perl\n\$c=\"".quotemeta($c)."\";\n$c;"; -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de ___ 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
Dear Detlev Zundel, In message you wrote: > > > 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. > > Ok, then currently I would vote for "cpumaxspeed_mhz". Even though its pretty descriptive - for the sake of consistency I recommend to omit the "_mhz" part. Also, I feel that "clock" would be more appropriate than "speed". And we should consider that we already use "cpuclk" for very similar purposes in U-Boot. Can we agree on "cpumaxclk" ? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Far back in the mists of ancient time, in the great and glorious days of the former Galactic Empire, life was wild, rich and largely tax free. - Douglas Adams, _The Hitchhiker's Guide to the Galaxy_ ___ 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 Sekhar,
> 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.
He does not influence the rating, but maybe he wants to limit the top
frequency the CPU runs with? I've seen customers do this in order to
save power.
> 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.
I'm getting lost in your arguments. First you tell me that only the
user can find out what speed a single individual CPU can run at and now
you tell me that the user is not involved at all.
If the original statement is correct then it is of course a good thing
to have an environment variable as this the preferred way of user
interaction.
> 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.
I consider having different binaries for different maximum operating
frequencies a completely broken conecpt sorry. In Linux we are striving
to run a single binary on different CPU types and you want to rebuild
U-Boot for a single parameter _not even used by U-Boot_?
> So, the user is just "configuring" the software according to the
> hardware. There no user "influence" on the silicon speed grade.
Exactly, the user is configuring this parameter _of his system_. He may
use the label on the CPU as this parameter or he may even choose to
ignore the labeling - ever heard of overclocking?
>> So why not call it "maximum operating frequency"?
>
> Yes, that's a correct description. Where do you want to call it such?
In the name of the ATAG.
>> 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.
About a characteristic of the maximum usable clock, yes. Nobody except
you is talking about a "current clock speed".
> The DA850 kernel reads the system registers directly to find out the
> clock speed. Even in the avr32 platform this ATAG is unused.
I'm getting lost. Now you are talking about a current clock speed, not
a maximum clock speed, right? Actually I was only pointing out the
ATAG_CLOCK to show that there _are_ ATAGS which are more in line with
what I perceive to be the problem that you try to solve.
>>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 "board revision" are you now talking of?
> What if instead of "maxspeed", I named the variable as "soctype" and had
> values like "am18x-300", "am18x-375", "am18x-450" passed to it?
Well yes, you could do that but do you believe that everybody would
infer that the setting of this variable influences a maximum clock
frequency used by the Linux kernel? I fail to see why you say yourself
that you are configuring a "maximum clock frequency" and then go on to
define a variable "soctype".
> It means the same thing, but will prob
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
Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Nori, > 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. Very likely because of what I wrote above - if this is a qualification _after_ the manufacturing. >> > 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=dsp§ionId=2&tabId=2641&familyId=1877¶mCriteria=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. So you call labeling the parts with the same revision a "documentation trick"? Is that easier than to accept the fact that the maximum speed rating of a CPU is not a revision? >> > 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? 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. So why not call it "maximum operating frequency"? 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")? >> >> >> > +/* >> >> >> > + * 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 menti
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=dsp§ionId=2&tabId=2641&familyId=1877¶mCriteria=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 - 45
Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
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. > 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=dsp§ionId=2&tabId=2641&familyId=1877¶mCriteria=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: AM1806 ARM Microprocessor (Rev. B) (PDF 1431 KB) 03 May 2010 AM1806 Silicon Errata Silicon Revision 2.0 (PDF 66 KB) 12 Mar 2010 So I cannot follow your "differnet datasheets" here. Also the usage of "revision" here is not coupled to any operating frequency. > 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. When writing code which should also be maintainable by other people it is a good idea to consider common expectations also of other people. >> >> > + >> >> > +/* >> >> > + * 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. >> >> 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 w
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=dsp§ionId=2&tabId=2641&familyId=1877¶mCriteria=no
and the 300 MHz OMAP-L1x parts:
http://focus.ti.com/paramsearch/docs/parametricsearch.tsp?family=dsp§ionId=2&tabId=2231&familyId=1621¶mCriteria=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 v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
Hi Nori,
>> > 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?
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. The maximum speed of a CPU is orthogonal for
me, i.e. there can still be a fast and a slow rev 1 CPU but still we
cannot enable cache. Do you see my point? If you want to express a
maximum speed, then use an ATAG which supports such a usage. Reading
the name ATAG_REVISION in code I would _never_ think that this
transports the maximum clock frequency.
>> 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.
I hope I explained my point better this time.
>> > 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.
>> > Signed-off-by: Sekhar Nori
>> > ---
>> > 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?
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.
>> 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 still remember old times when the Linux kernel for PowerPC
switched from its interpretation of clock frequencies from hz to mhz and
the many support questions it generated
Cheers
Detlev
--
Of course my password is the same as my pet's name
My macaw's name was Q47pY!3 and I change it every 90 days
-- Trevor Lin
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
> > ---
> > 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 v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
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? Is this really a good idea? I can easily imagine different
CPU revisions with different maximum clock frequencies. How will you
handle that?
Is the counterpart in the Linux kernel already implemented?
> 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?
> Signed-off-by: Sekhar Nori
> ---
> 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.
> + */
> +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.
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"?
> + 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?
Cheers
Detlev
--
Warning: this comic occasionally contains strong language (which may be unsuit-
able for children), unusual humor (which may be unsuitable for adults), and ad-
vanced mathematics (which may be unsuitable for liberal-arts majors). /xkcd.org
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel
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
---
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
+ */
+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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
