On 09/27/2012 03:22 AM, Kim Phillips wrote:
On Wed, 26 Sep 2012 10:28:08 +0200
Gerlando Falauto<gerlando.fala...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
