From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gerlando Falauto Date: Thu, 27 Sep 2012 09:21:19 +0200 Subject: [U-Boot] [PATCH v1 2/4] mpc83xx: add support for mpc8309 In-Reply-To: <20120926202224.dc0052d97374ad467fbeb948@freescale.com> References: <1348648090-861-1-git-send-email-gerlando.falauto@keymile.com><1348648090-861-3-git-send-email-gerlando.falauto@keymile.com> <20120926202224.dc0052d97374ad467fbeb948@freescale.com> Message-ID: <5063FE6F.6070002@keymile.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 09/27/2012 03:22 AM, Kim Phillips wrote: > On Wed, 26 Sep 2012 10:28:08 +0200 > Gerlando Falauto 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