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 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.
Sure, I never lobbied for using partnumbers in code. This is a seriously broken concept.
The question is why should a new ARM ATAG be introduced if an existing one is good enough for the purpose?
What do you believe is "good enough"? Something which is not used currently? I still fail to see how anybody studying code should understand that ATAG_REVISION has a connection to the maximum allowed frquency.
Personally I strive to write maintainable and extensible software always keeping in mind that someone someday will have to touch the code again, so I'll better help as best as I can.
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.
Ok, then currently I would vote for "cpumaxspeed_mhz".
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.
Cheers Detlev