[U-Boot] [PATCH 03/21] fsl_ifc: add support for different IFC bank count
Andy Fleming
afleming at gmail.com
Fri May 10 23:49:01 CEST 2013
On Fri, Mar 22, 2013 at 12:23 PM, York Sun <yorksun at freescale.com> wrote:
> From: Mingkai Hu <Mingkai.hu at 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.hu at 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 at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list