On Fri, Mar 22, 2013 at 12:23 PM, York Sun <york...@freescale.com> wrote:
> From: Mingkai Hu <mingkai...@freescale.com> > > Calculate reserved fields according to IFC bank count > > 1. Move csor_ext register behind csor register and fix res offset > 2. move ifc bank count to config_mpc85xx.h to support 8 bank count > > 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? > > Signed-off-by: Mingkai Hu <mingkai...@freescale.com> > --- > arch/powerpc/cpu/mpc85xx/cpu.c | 2 +- > arch/powerpc/cpu/mpc8xxx/fsl_ifc.c | 58 > ++++++++++++++++++++++++++++- > arch/powerpc/include/asm/config_mpc85xx.h | 7 ++++ > arch/powerpc/include/asm/fsl_ifc.h | 42 +++++++++++++++------ > 4 files changed, 96 insertions(+), 13 deletions(-) > > diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c > b/arch/powerpc/cpu/mpc85xx/cpu.c > index df2ab6d..379a7df 100644 > --- a/arch/powerpc/cpu/mpc85xx/cpu.c > +++ b/arch/powerpc/cpu/mpc85xx/cpu.c > @@ -34,8 +34,8 @@ > #include <asm/io.h> > #include <asm/mmu.h> > #include <asm/fsl_ifc.h> > -#include <asm/fsl_law.h> > #include <asm/fsl_lbc.h> > +#include <asm/fsl_law.h> > #include <post.h> > #include <asm/processor.h> > #include <asm/fsl_ddr_sdram.h> > diff --git a/arch/powerpc/cpu/mpc8xxx/fsl_ifc.c > b/arch/powerpc/cpu/mpc8xxx/fsl_ifc.c > index 56b319f..f0da355 100644 > --- a/arch/powerpc/cpu/mpc8xxx/fsl_ifc.c > +++ b/arch/powerpc/cpu/mpc8xxx/fsl_ifc.c > @@ -26,7 +26,7 @@ void print_ifc_regs(void) > int i, j; > > printf("IFC Controller Registers\n"); > - for (i = 0; i < FSL_IFC_BANK_COUNT; i++) { > + for (i = 0; i < CONFIG_SYS_FSL_IFC_BANK_COUNT; i++) { > printf("CSPR%d:0x%08X\tAMASK%d:0x%08X\tCSOR%d:0x%08X\n", > i, get_ifc_cspr(i), i, get_ifc_amask(i), > i, get_ifc_csor(i)); > @@ -94,4 +94,60 @@ void init_early_memctl_regs(void) > set_ifc_amask(IFC_CS3, CONFIG_SYS_AMASK3); > set_ifc_csor(IFC_CS3, CONFIG_SYS_CSOR3); > #endif > + > +#ifdef CONFIG_SYS_CSPR4_EXT > + set_ifc_cspr_ext(IFC_CS4, CONFIG_SYS_CSPR4_EXT); > +#endif > +#if defined(CONFIG_SYS_CSPR4) && defined(CONFIG_SYS_CSOR4) > There seem to be a large number of CONFIG options associated with each and every new bank. It's following the pattern of the existing code, so I'll accept it, but I can't help but think that some of these config options are entirely redundant (would we ever have CSPR4, and *not* CSOR4?). This is, admittedly, based only on a high-level view of the code -- I'm not familiar with the specifics of the IFC design. [...] 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. 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. +#else +#error IFC BANK count not vaild +#endif +#else +#error IFC BANK count not defined +#endif /* * IFC Controller Registers @@ -918,24 +937,24 @@ struct fsl_ifc { u32 cspr_ext; u32 cspr; u32 res2; - } cspr_cs[FSL_IFC_BANK_COUNT]; - u32 res3[0x19]; + } cspr_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT]; + u32 res3[CONFIG_SYS_FSL_IFC_CSPR_RES]; struct { u32 amask; u32 res4[0x2]; - } amask_cs[FSL_IFC_BANK_COUNT]; - u32 res5[0x17]; + } amask_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT]; + u32 res5[CONFIG_SYS_FSL_IFC_AMASK_RES]; struct { - u32 csor_ext; u32 csor; + u32 csor_ext; u32 res6; - } csor_cs[FSL_IFC_BANK_COUNT]; - u32 res7[0x19]; + } csor_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT]; + u32 res7[CONFIG_SYS_FSL_IFC_CSOR_RES]; struct { u32 ftim[4]; u32 res8[0x8]; - } ftim_cs[FSL_IFC_BANK_COUNT]; - u32 res9[0x60]; + } ftim_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT]; + u32 res9[CONFIG_SYS_FSL_IFC_FTIM_RES]; u32 rb_stat; u32 res10[0x2]; u32 ifc_gcr; @@ -961,6 +980,7 @@ struct fsl_ifc { #undef CSPR_MSEL_NOR #define CSPR_MSEL_NOR CSPR_MSEL_GPCM #endif +#endif /* CONFIG_FSL_IFC */ #endif /* __ASSEMBLY__ */ #endif /* __ASM_PPC_FSL_IFC_H */ -- 1.7.9.5 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot