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 * 0000b - 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 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. Thanks, Sekhar _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
