[U-Boot] [PATCH 03/21] fsl_ifc: add support for different IFC bank count

[Fleming Andy-AFLEMING]

On May 14, 2013, at 3:59, "Hu Mingkai-B21284" <B21284 at freescale.com<mailto:B21284 at freescale.com>> wrote:

Hi Andy,

Please find my replies in line.

Thanks,
Mingkai


There's no IFC controller instead of eLBC controller on some platforms,
such as MPC8536, P2041, P3041, P4080 etc, so there's no macro definition
for the number of IFC chip select(CONFIG_SYS_FSL_IFC_BANK_COUNT) which
is used in the IFC controller header file fsl_ifc.h on these platforms.


This paragraph is pretty confusing. Is this just explaining that IFC_BANK_COUNT isn't being defined for devices that don't use IFC? Or is it explaining why you have to guard fsl_ifc.h with a #ifdef?
[Mingkai] Both. Some devices doesn’t use IFC, thus, there’s no IFC_BANK_COUNT for these devices. While some common files include fsl_ifc.h, which needs IFC_BANK_COUNT definition, that’s the reason why I have to guard fsl_ifc.h. I will change it to the following:

3. Guard fsl_ifc.h with CONFIG_FSL_IFC to eliminate the compile error on some devices that doesn’t have IFC controller.


That's good, thanks


diff --git a/arch/powerpc/include/asm/fsl_ifc.h b/arch/powerpc/include/asm/fsl_ifc.h
index ba41b73..debcb6b 100644
--- a/arch/powerpc/include/asm/fsl_ifc.h
+++ b/arch/powerpc/include/asm/fsl_ifc.h
@@ -21,6 +21,7 @@
 #ifndef __ASM_PPC_FSL_IFC_H
 #define __ASM_PPC_FSL_IFC_H

+#ifdef CONFIG_FSL_IFC
 #include <config.h>
 #include <common.h>

[...]

@@ -907,6 +910,22 @@ struct fsl_ifc_gpcm {
        u32 res4[0x1F3];
 };

+#ifdef CONFIG_SYS_FSL_IFC_BANK_COUNT
+#if (CONFIG_SYS_FSL_IFC_BANK_COUNT <= 8)
+#define CONFIG_SYS_FSL_IFC_CSPR_RES \
+       (0x25 - CONFIG_SYS_FSL_IFC_BANK_COUNT * 3)
+#define CONFIG_SYS_FSL_IFC_AMASK_RES \
+       (0x24 - CONFIG_SYS_FSL_IFC_BANK_COUNT * 3)
+#define CONFIG_SYS_FSL_IFC_CSOR_RES \
+       (0x24 - CONFIG_SYS_FSL_IFC_BANK_COUNT * 3)
+#define CONFIG_SYS_FSL_IFC_FTIM_RES \
+       (0x90 - CONFIG_SYS_FSL_IFC_BANK_COUNT * 0xc)


These aren't really config values. They are values derived from a CONFIG value, and they only have local scope (so there's no need for the very global scoping of the names).

Also... Are these correct? 0x25 is a strange amount of gap in register space.
[Mingkai]They’re correct. I’ve test it on T4, C293QDS and P1010RDB platform when I submitted this patch.
The value 0x25 is caused by the fact that the 0x25 is represents the length of cspr (148 bytes) in unit 4 bytes.
Here is the magic math: 148 / 4 = 37 = 0x25

All that aside, I think we should just define the register offsets to cover all existing (or even predicted) registers, and make the gap hard-coded as before. There's no real advantage to specifying only as many as are implemented, and this method seems ripe for potential bugs in the future.

[Mingkai] I modified the code as follows,

First define the different register space length in bytes as follows:

#define IFC_CSPR_REG_LEN        148
#define IFC_AMASK_REG_LEN       140
#define IFC_CSOR_REG_LEN        140
#define IFC_FTIM_REG_LEN        576

Then in the struct, use the register space length minus the actual used space and get the reserved space as the red line indicates.

struct fsl_ifc {
        u32 ifc_rev;
        u32 res1[0x2];
        struct {
                u32 cspr_ext;
                u32 cspr;
                u32 res2;
        } cspr_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT];
        u32 res3[IFC_CSPR_REG_LEN - sizeof(cspr_cs)];


That's a bit clearer. However, it needs to be u8 if you are going to specify bytes.


        struct {
                u32 amask;
                u32 res4[0x2];
        } amask_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT];
        u32 res5[IFC_AMASK_REG_LEN - sizeof(amask_cs)];
        struct {
                u32 csor;
                u32 csor_ext;
                u32 res6;
        } csor_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT];
        u32 res7[IFC_CSOR_REG_LEN - sizeof(csor_cs)];
        struct {
                u32 ftim[4];
                u32 res8[0x8];
        } ftim_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT];
        u32 res9[IFC_FTIM_REG_LEN - sizeof(ftim_cs)];
}

BTW, the patch can’t be applied to your mpc85xx public tree cleanly, so if you think this modification is ok, I will submit a new patch based on the latest git tree base.

Ok, thanks!

Andy