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

login
register
mail settings
Submitter Andy Fleming
Date May 10, 2013, 9:49 p.m.
Message ID <CAKWjMd4t0Ey8-vAN-snYzV-J4F12LTKDJse_paoOqUgKu25ZNw@mail.gmail.com>
Download mbox | patch
Permalink /patch/243072/
State Changes Requested
Delegated to: Andy Fleming
Headers show

Comments

Andy Fleming - May 10, 2013, 9:49 p.m.
On Fri, Mar 22, 2013 at 12:23 PM, York Sun <yorksun@freescale.com> wrote:

> From: Mingkai Hu <Mingkai.hu@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@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.

[...]



[...]

@@ -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

Patch

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>