[U-Boot] [PATCH v1 2/4] mpc83xx: add support for mpc8309

[Gerlando Falauto <gerlando.falauto@keymile.com>]

On 09/27/2012 03:22 AM, Kim Phillips wrote:
> On Wed, 26 Sep 2012 10:28:08 +0200
> Gerlando Falauto<gerlando.falauto@keymile.com>  wrote:
>
>> This processor, though very similar to other members of the
>> PowerQUICC II Pro family (namely 8308, 8360 and 832x), provides
>> yet another feature set than any supported sibling.
>>
>> So we add a bunch of new #ifdefs (or complicate the existing ones)
>> to arch/powerpc/cpu/mpc83xx/speed.c.
>>
>> Perhaps it would be worth to refactor the whole file so to first
>> identify the featureset of the given CPU, and enclose each block within
>>   #ifdef CONFIG_MPC83XX_FEAT_XXXX
>> for instance:
>>   - CONFIG_MPC83XX_FEAT_USBDR
>
> this is CONFIG_HAS_FSL_DR_USB
>
>>   - CONFIG_MPC83XX_FEAT_QE
>
> this could be CONFIG_QE but should probably be CONFIG_HAS_FSL_QE
> which doesn't exist.

Assuming we keep CONFIG_QE, do you think that could replace the whole:

#if defined(CONFIG_MPC8309) || defined(CONFIG_MPC8360) || 
defined(CONFIG_MPC832x)

which I am not very happy with?

>>   - CONFIG_MPC83XX_FEAT_DDRSEC
>> ...etc...
>
> it still wouldn't help that much with the cases like one SoC getting
> its tsec clock from somewhere completely different than the others.

It doesn't have to cover all cases, but some simplification could still 
be an improvement, I think.

> Plus, the mpc8309 should be the last of the Mohicans...

I'll take your word for it. :-)
Well in that case it's not so crucial then.

>> @@ -120,14 +122,17 @@ int get_clocks(void)
>>   #if defined(CONFIG_FSL_ESDHC)
>>   	u32 sdhc_clk;
>>   #endif
>> +#if !defined(CONFIG_MPC8309)
>>   	u32 enc_clk;
>> +#endif
>
> the 8309 is supposed to be similar to the 8308, which also doesn't
> have enc_clk (even though it doesn't do this).  I'm thinking
> CONFIG_MPC8308 should be renamed _MPC830x before adding support for
> the 8309.

Wouldn't that be confusing? The way I understand it we'd also need some 
way to distinguish between the two, so we'd have:

#define CONFIG_MPC83xx          1
#define CONFIG_MPC830x		1
#define CONFIG_MPC8309          1

Plus (assuming my patch is functionally correct), there's only a couple 
of occurences of:

#if defined(CONFIG_MPC8308) || defined(CONFIG_MPC8309)

>> @@ -457,6 +470,8 @@ int get_clocks(void)
>>   	gd->tsec1_clk = tsec1_clk;
>>   	gd->tsec2_clk = tsec2_clk;
>>   	gd->usbdr_clk = usbdr_clk;
>> +#elif defined(CONFIG_MPC8309)
>> +	gd->usbdr_clk = usbdr_clk;
>>   #endif
>
> this change generates this new compiler warning:
>
> Configuring for MPC8308RDB board...
>     text	   data	    bss	    dec	    hex	filename
>   261821	   6860	 235952	 504633	  7b339	./u-boot
> speed.c: In function 'get_clocks':
> speed.c:472:16: warning: 'usbdr_clk' may be used uninitialized in this function [-Wuninitialized]

Actually it's this one:

@@ -185,7 +190,10 @@ int get_clocks(void)
                 /* unkown SCCR_TSEC1CM value */
                 return -2;
         }
+#endif

+#if defined(CONFIG_MPC8309) || defined(CONFIG_MPC831x) || \
+       defined(CONFIG_MPC834x) || defined(CONFIG_MPC837x)
         switch ((sccr & SCCR_USBDRCM) >> SCCR_USBDRCM_SHIFT) {
         case 0:
                 usbdr_clk = 0;

where the code gets dropped in the case of 8308.
So, do you think CONFIG_HAS_FSL_DR_USB would do the trick in that case?

>
>> +++ b/arch/powerpc/include/asm/immap_83xx.h
>> @@ -73,12 +73,19 @@ typedef struct sysconf83xx {
>>   	u32 obir;		/* Output Buffer Impedance Register */
>>   	u8 res8[0xC];
>>   	u32 pecr1;		/* PCI Express control register 1 */
>> -#ifdef CONFIG_MPC8308
>> -	u32 sdhccr;		/* eSDHC Control Registers for MPC8308 */
>> +#if defined(CONFIG_MPC8308) || defined(CONFIG_MPC8309)
>
> MPC830x

See above.

>
>> @@ -389,6 +390,86 @@
>>   #define SICRH_TSOBI1_V2P5		(1<<  1)
>>   #define SICRH_TSOBI2_V3P3		(0<<  0)
>>   #define SICRH_TSOBI2_V2P5		(1<<  0)
>> +
>> +#elif defined(CONFIG_MPC8309)
>> +/* SICR_1 */
>> +#define SICR_1_UART1_UART1S		(0<<  (30-2))
>> +#define SICR_1_UART1_UART1RTS		(1<<  (30-2))
>> +#define SICR_1_I2C_I2C			(0<<  (30-4))
>> +#define SICR_1_I2C_CKSTOP		(1<<  (30-4))
>> +#define SICR_1_IRQ_A_IRQ		(0<<  (30-6))
>> +#define SICR_1_IRQ_A_MCP		(1<<  (30-6))
>> +#define SICR_1_IRQ_B_IRQ		(0<<  (30-8))
>> +#define SICR_1_IRQ_B_CKSTOP		(1<<  (30-8))
>> +#define SICR_1_GPIO_A_GPIO		(0<<  (30-10))
>> +#define SICR_1_GPIO_A_SD		(2<<  (30-10))
>> +#define SICR_1_GPIO_A_DDR		(3<<  (30-10))
>> +#define SICR_1_GPIO_B_GPIO		(0<<  (30-12))
>> +#define SICR_1_GPIO_B_SD		(2<<  (30-12))
>> +#define SICR_1_GPIO_B_QE		(3<<  (30-12))
>> +#define SICR_1_GPIO_C_GPIO		(0<<  (30-14))
>> +#define SICR_1_GPIO_C_CAN		(1<<  (30-14))
>> +#define SICR_1_GPIO_C_DDR		(2<<  (30-14))
>> +#define SICR_1_GPIO_C_LCS		(3<<  (30-14))
>> +#define SICR_1_GPIO_D_GPIO		(0<<  (30-16))
>> +#define SICR_1_GPIO_D_CAN		(1<<  (30-16))
>> +#define SICR_1_GPIO_D_DDR		(2<<  (30-16))
>> +#define SICR_1_GPIO_D_LCS		(3<<  (30-16))
>> +#define SICR_1_GPIO_E_GPIO		(0<<  (30-18))
>> +#define SICR_1_GPIO_E_CAN		(1<<  (30-18))
>> +#define SICR_1_GPIO_E_DDR		(2<<  (30-18))
>> +#define SICR_1_GPIO_E_LCS		(3<<  (30-18))
>> +#define SICR_1_GPIO_F_GPIO		(0<<  (30-20))
>> +#define SICR_1_GPIO_F_CAN		(1<<  (30-20))
>> +#define SICR_1_GPIO_F_CK		(2<<  (30-20))
>> +#define SICR_1_USB_A_USBDR		(0<<  (30-22))
>> +#define SICR_1_USB_A_UART2S		(1<<  (30-22))
>> +#define SICR_1_USB_B_USBDR		(0<<  (30-24))
>> +#define SICR_1_USB_B_UART2S		(1<<  (30-24))
>> +#define SICR_1_USB_B_UART2RTS		(2<<  (30-24))
>> +#define SICR_1_USB_C_USBDR		(0<<  (30-26))
>> +#define SICR_1_USB_C_QE_EXT		(3<<  (30-26))
>> +#define SICR_1_FEC1_FEC1		(0<<  (30-28))
>> +#define SICR_1_FEC1_GTM			(1<<  (30-28))
>> +#define SICR_1_FEC1_GPIO		(2<<  (30-28))
>> +#define SICR_1_FEC2_FEC2		(0<<  (30-30))
>> +#define SICR_1_FEC2_GTM			(1<<  (30-30))
>> +#define SICR_1_FEC2_GPIO		(2<<  (30-30))
>> +/* SICR_2 */
>> +#define SICR_2_FEC3_FEC3		(0<<  (30-0))
>> +#define SICR_2_FEC3_TMR			(1<<  (30-0))
>> +#define SICR_2_FEC3_GPIO		(2<<  (30-0))
>> +#define SICR_2_HDLC1_A_HDLC1		(0<<  (30-2))
>> +#define SICR_2_HDLC1_A_GPIO		(1<<  (30-2))
>> +#define SICR_2_HDLC1_A_TDM1		(2<<  (30-2))
>> +#define SICR_2_ELBC_A_LA		(0<<  (30-4))
>> +#define SICR_2_ELBC_B_LCLK		(0<<  (30-6))
>> +#define SICR_2_HDLC2_A_HDLC2		(0<<  (30-8))
>> +#define SICR_2_HDLC2_A_GPIO		(0<<  (30-8))
>> +#define SICR_2_HDLC2_A_TDM2		(0<<  (30-8))
>> +/* bits 10-11 unused */
>> +#define SICR_2_USB_D_USBDR		(0<<  (30-12))
>> +#define SICR_2_USB_D_GPIO		(2<<  (30-12))
>> +#define SICR_2_USB_D_QE_BRG		(3<<  (30-12))
>> +#define SICR_2_PCI_PCI			(0<<  (30-14))
>> +#define SICR_2_PCI_CPCI_HS		(2<<  (30-14))
>> +#define SICR_2_HDLC1_B_HDLC1		(0<<  (30-16))
>> +#define SICR_2_HDLC1_B_GPIO		(1<<  (30-16))
>> +#define SICR_2_HDLC1_B_QE_BRG		(2<<  (30-16))
>> +#define SICR_2_HDLC1_B_TDM1		(3<<  (30-16))
>> +#define SICR_2_HDLC1_C_HDLC1		(0<<  (30-18))
>> +#define SICR_2_HDLC1_C_GPIO		(1<<  (30-18))
>> +#define SICR_2_HDLC1_C_TDM1		(2<<  (30-18))
>> +#define SICR_2_HDLC2_B_HDLC2		(0<<  (30-20))
>> +#define SICR_2_HDLC2_B_GPIO		(1<<  (30-20))
>> +#define SICR_2_HDLC2_B_QE_BRG		(2<<  (30-20))
>> +#define SICR_2_HDLC2_B_TDM2		(3<<  (30-20))
>> +#define SICR_2_HDLC2_C_HDLC2		(0<<  (30-22))
>> +#define SICR_2_HDLC2_C_GPIO		(1<<  (30-22))
>> +#define SICR_2_HDLC2_C_TDM2		(2<<  (30-22))
>> +#define SICR_2_HDLC2_C_QE_BRG		(3<<  (30-22))
>> +#define SICR_2_QUIESCE_B		(0<<  (30-24))
>> +
>>   #endif
>
> was there an inadequacy in the other SoCs' SICRL/H_ naming
> convention and/or value definition in this area?  If not, then why
> should the 8309 get its own reinvented SICR_1/2_ etc.?

As for the naming, I used SICR_1/2 as opposed to SICRL/H because that's 
how the registers are called in the datasheet.
As for the value definition, I added my own (third, at least!) 
convention so to match the bit numbering in the datasheet.
This should makes double checking a trivial task.

 > Just looking for some consistency here...

Thanks a lot for your review!
Gerlando