[PATCH 05/14] at91: use structure to store the current soc

[ryan@bluewatersys.com (Ryan Mallon)]

On 04/27/2011 01:27 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>
>>>> This way the cpu detection code is much clearer and easier to read and
>>>> only one set of __cpu_is macros are needed. The values for
>>>> ARCH_ID_AT91RM9200, etc can be moved into soc.c since they are not
>>>> needed elsewhere.
>>> This will never work you do not listen
>>>
>>> the DBUG is at DIFFERENT base address on the AT91
>>> you need to check them one by one
>>>
>>> I keep the structure to keep more imformation inside and do not want to be
>>> limited by a 32bit and complex encoding to maintain
>>>
>>> Best Regards,
>>> J.
>> You can only be running one machine at a time. Are you implying that
>> this change breaks at91_sys_read in such a way that it reads the wrong
>> registers? If so, then its definitely a NAK.
> Certernly not ALL of the current work is too allow in a UNIQUE kernel to have
> all the atmel inside and other vendors and boards in the same kernel

Agreed. At the moment we still have a situation where we can only
compile one at91 SoC variant into the kernel. So, at the moment, we do
not need to deal with the fact that the DBGU exists at different
locations, we can just read it for the board that we are booting on.

What I am objecting to is adding two entire sets of cpu_is macros which
is just needless extra code with no benefit. We should determine the
cpu/SoC type _once_ inside a single function (at91_initialize) and the
the cpu_is macros should just read the shadow value. There is no reason
to have two sets of macros.

> and for at91_sys_read it's not a big deal as I'm going to drop it at the end
> the timers does not use it anymore and the other drivers will update also one
> by one

Okay. But it shouldn't get broken now.

>> Your patch does this:
>>
>> static inline unsigned long at91_cpu_identify(void)
>> {
>> 	return (at91_sys_read(AT91_DBGU_CIDR) & ~AT91_CIDR_VERSION);
>> }
>>
> Yeah as I said already I need to fix a lots of place and I do it step by step
> so for now on I do not update the at91_cpu_identify & co but I will in time

Right. But the point is, if __cpu_is_at91rm9200 calls at91_cpu_identify,
then why can't we drop the __cpu_is_at91rm9200 call entirely and just
work out the cpu/SoC type in a big switch statement?

In short, what is the benefit of the __cpu_is macros if they are never
used outside of at91_initialize?

>> ...
>>
>> #ifdef CONFIG_ARCH_AT91RM9200
>> #define __cpu_is_at91rm9200()	(at91_cpu_identify() == ARCH_ID_AT91RM9200)
>> #else
>> #define __cpu_is_at91rm9200()	(0)
>> #endif
>>
>> ...
>>
>> void __init at91_initialize(unsigned long main_clock)
>> {
>> 	...
>> 	if (__cpu_is_at91rm9200()) {
>> 		cpu_id.is_at91rm9200_soc;
>> 		current_soc = at91rm9200_soc;
>> 	}
>> 	...
>> }
>>
>>
>> So at91_initialize calls the __cpu_is macros, which in turn read the
>> AT91_DBGU registers via at91_sys_read. What I am proposing above is to
>> replace the massive collection of if statements with a simpler switch
>> statement and replacing the __cpu_is macros with direct register reads
>> via at91_sys_read, which results in far less code and more readable
>> code. If your version works, then how can mine not?
> Because It will never allow you to have a __cpu_is_ soc specific
> which is the goal here

You already have the existing cpu_is versions (which would now use the
shadowed value rather than register reads). After at91_initialize has
determined the cpu/SoC type the __cpu_is macros are no longer used right?

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934