[U-Boot] [PATCH v1 2/4] mpc83xx: add support for mpc8309
Gerlando Falauto
gerlando.falauto at keymile.com
Thu Sep 27 09:21:19 CEST 2012
On 09/27/2012 03:22 AM, Kim Phillips wrote:
> On Wed, 26 Sep 2012 10:28:08 +0200
> Gerlando Falauto<gerlando.falauto at 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
More information about the U-Boot
mailing list