[U-Boot] [PATCH 1/2] mpc8308: support for Freescale MPC8308 cpu

[Ilya Yanok]

Dear Wolfgang,

thanks for your review.

On 21.06.2010 11:44, Wolfgang Denk wrote:
>> This patch adds basic support for Freescale MPC8308 CPU. Serial ports,
>> NOR flash and integrated Ethernet controllers are supported.
>> PCI Express is also supported. eSDHC, NAND and USB may work but aren't
>> tested (using ULPI PHY requires additional patch).
>>
>> Signed-off-by: Ilya Yanok<yanok at emcraft.com>
>>      
> ...
>    
>> -#if defined(CONFIG_MPC834x) || defined(CONFIG_MPC837x) || defined(CONFIG_MPC8315)
>> +#if defined(CONFIG_MPC834x) || defined(CONFIG_MPC837x) || \
>> +	defined(CONFIG_MPC8315) || defined(CONFIG_MPC8308)
>>      
> Please sort this list.
>    

Fixed.

>> +#if defined(CONFIG_MPC8308)
>> +#define SCCR_SDHCCM			0x0c000000
>> +#define SCCR_SDHCCM_SHIFT		26
>> +#define SCCR_SDHCCM_0			0x00000000
>> +#define SCCR_SDHCCM_1			0x04000000
>> +#define SCCR_SDHCCM_2			0x08000000
>> +#define SCCR_SDHCCM_3			0x0c000000
>> +#endif
>>      
> Would it make sense to write this as:
>
> And: why do we need the #ifdef? Unused defines should not hurt?
>
> 	#define SCCR_SDHCCM_MASK	0x0c000000	/* is it a mask? */
> 	#define SCCR_SDHCCM_SHIFT	26
> 	#define SCCR_SDHCCM(arg)	((arg)<<SCCR_SDHCCM_SHIFT)
>
>    

As you already mentioned I'm just following the style used in this file.

>> +#if defined(CONFIG_MPC8315)
>>   #define SCCR_SATA1CM			0x00003000
>>   #define SCCR_SATA1CM_SHIFT		12
>>   #define SCCR_SATACM			0x00003c00
>> @@ -765,6 +776,7 @@
>>   #define SCCR_SATACM_1			0x00001400
>>   #define SCCR_SATACM_2			0x00002800
>>   #define SCCR_SATACM_3			0x00003c00
>> +#endif
>>      
> Do we need that #ifdef? Ok, the #defines don't apply to the 8308, but
> do they hurt if they are just there, unused?
>    

Well, it seems to be safer not to have unused defines so that you can't 
erroneously use some define not applicable for current CPU, but if you 
wish I'll remove these ifdefs.

Regards, Ilya.