[PATCH 5/9] at91: use structure to store the current soc

[ryan@bluewatersys.com (Ryan Mallon)]

On 05/11/2011 11:40 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>  
>>> +
>>> +static void soc_detect(u32 dbgu_base)
>>> +{
>>
>> This looks far better than your previous patch, and is how I suggested
>> it should have been done :-).
>>
>> I think possibly that this function should return an boolean indicating
>> whether or not it was able to detect the cpu, rather than the calling
>> function checking the internals of the at91_soc_data structure.
> no need as at91_soc_data.type is init to SOC_AT91_NONE first
> this ways we use less code

Read the second part of my sentence. I think it is clearer/cleaner to
have a return value specifying that the soc detection failed rather than
checking the internals of the structure.

>>
>>> +	u32 cidr, socid;
>>> +
>>> +	cidr = __raw_readl(AT91_IO_P2V(dbgu_base) + AT91_DBGU_CIDR);
>>> +	socid = cidr & ~AT91_CIDR_VERSION;
>>>
>>> +	switch (socid) {
>>> +	case ARCH_ID_AT572D940HF:
>>> +		at91_soc_data.type = SOC_572D940HF;
>>> +		set_at91_boot_soc(at572d940hf_soc);
>>> +		break;
>>> +
>>> +	case ARCH_ID_AT91CAP9: {
>>> +#ifdef CONFIG_AT91_PMC_UNIT
>>> +		u32 pmc_ver = at91_sys_read(AT91_PMC_VER);
>>> +
>>> +		if (pmc_ver == ARCH_REVISION_CAP9_B)
>>> +			at91_soc_data.subtype = SOC_CAP9_REV_B;
>>> +		else if (pmc_ver == ARCH_REVISION_CAP9_C)
>>> +			at91_soc_data.subtype = SOC_CAP9_REV_C;
>>> +#endif
>>> +		at91_soc_data.type = SOC_CAP9;
>>> +		set_at91_boot_soc(at91cap9_soc);
>>> +		break;
>>> +	}
>>> +
>>> +	case ARCH_ID_AT91RM9200:
>>> +		at91_soc_data.type = SOC_RM9200;
>>> +		set_at91_boot_soc(at91rm9200_soc);
>>> +		break;
>>> +
>>> +	case ARCH_ID_AT91SAM9260:
>>> +		at91_soc_data.type = SOC_SAM9260;
>>> +		set_at91_boot_soc(at91sam9260_soc);
>>> +		break;
>>> +
>>> +	case ARCH_ID_AT91SAM9261:
>>> +		at91_soc_data.type = SOC_SAM9261;
>>> +		set_at91_boot_soc(at91sam9261_soc);
>  > +
>>> +	if (at91_soc_data.type == SOC_AT91_NONE)
>>> +		return;
>>> +
>>> +	at91_soc_data.cidr = cidr;
>>
>> Why not directly assign this to at91_soc_data.cidr above and remove the
>> cidr local variable?
> because I prefer to udpate the global struct only if we detect the soc

Okay.

>>
>>> +
>>> +	/* sub version of soc */
>>> +	at91_soc_data.exid = __raw_readl(AT91_IO_P2V(dbgu_base) + AT91_DBGU_EXID);
>>> +
> 
>>> +		case ARCH_EXID_AT91SAM9M11:
>>> +			at91_soc_data.subtype = SOC_SAM9M11;
>>> +			break;
>>> +		}
>>
>> If we reused the ARCH_EXID_ defines, then the above could become:
>>
>> 	at91_soc_data.subtype = at91_soc_data.exid;
> as sais in the commint comment to have a uniq subtype accross soc as example
> g35 and 9m11 have the same exid

Why is this necessary? The cpu_is_macros, which are the _only_ way that
the cpu/soc type should ever be checked, can do this. For example:

#define cpu_is_at91sam9m11() \
	(at91_soc_data.type == ARCH_ID_AT91SAM9G45 &&
	 at91_soc_data.subtype == ARCH_EXID_AT91SAM9M11)

This way you don't need any extra #defines, and the switch statements
for the exid in soc_detect can be removed.

This would also result in a smaller impact change since the cpu_is
macros would still work exactly the same, just the method used to read
the id and exid change.

>>> +
>>> +const char *get_at91_soc_type(struct at91_socinfo *c)
>>> +{
>>
>> Should probably be called at91_get_soc_type_name. Get get_type implies
>> it will return the enum type value. I think the at91 prefix belongs at
>> the start also.
>>
>> Also, since at91_boot_soc is __initdata, the soc_name array should also
>> be __initdata and this function should be __init.
> no this data will be use for sysfs export later
> and at91_soc_data is not __initdata

Okay. I still think the function names should be changed.

>>
>>> +
>>> +	if (at91_soc_data.type == SOC_AT91_NONE)
>>> +		panic("AT91: Impossible to detect the SOC type");
>>> +
>>> +	pr_info("AT91: Detected soc type: %s\n", get_at91_soc_type(&at91_soc_data));
>>> +	pr_info("AT91: Detected soc subtype: %s\n", get_at91_soc_subtype(&at91_soc_data));
>>> +
>>> +	if (!at91_boot_soc.init)
>>> +		panic("AT91: Soc not enabled");
>>
>> I would still like to see if it is possible to reorder these patches so
>> that this intermediate rewrite is not needed.
> there is none intermedaite write

You snipped it out in the reply :-). Patch 2/9 adds soc detection in
at91_map_io via a series of 'if (cpu_is_xxx)' statements. This patch
then removes that method and replaces it with the switch statement in
soc_detect. It would be nice to try and avoid the intermediate step in
at91_map_io since it only exists for three commits.

>>>  
>>>  	if (at91_boot_soc.map_io)
>>>  		at91_boot_soc.map_io();
>>> diff --git a/arch/arm/mach-at91/soc.h b/arch/arm/mach-at91/soc.h
>>> index d847565..1125671 100644
>>> --- a/arch/arm/mach-at91/soc.h
>>> +++ b/arch/arm/mach-at91/soc.h
>>> @@ -20,3 +20,41 @@ extern struct at91_soc at91sam9263_soc;
>>>  extern struct at91_soc at91sam9g45_soc;
>>>  extern struct at91_soc at91sam9rl_soc;
>>>  extern struct at91_soc at91sam9x5_soc;
>>> +
>>> +#define set_at91_boot_soc(c) do { at91_boot_soc = c; } while(0)
>>
>> Should be a static inline function.
> a macro is fine here

Static inlines are the preferred method wherever possible.

>>
>>> +
>>> +#if !defined(CONFIG_ARCH_AT572D940HF)
>>> +#define at572d940hf_soc	at91_boot_soc
>>> +#endif
>>
>> Possibly this needs to be reworked a little so that the user gets an
>> informative if the soc type is detected correctly, but support for the
>> soc is not included in the kernel?
> it's already the case

Okay.

Thanks,
~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