[U-Boot] [PATCH 03/21] fsl_ifc: add support for different IFC bank count
Hu Mingkai-B21284
B21284 at freescale.com
Tue May 14 10:59:52 CEST 2013
Hi Andy,
Please find my replies in line.
Thanks,
Mingkai
From: Andy Fleming [mailto:afleming at gmail.com]
Sent: Saturday, May 11, 2013 5:49 AM
To: sun york-R58495; Hu Mingkai-B21284
Cc: Fleming Andy-AFLEMING; U-Boot list
Subject: Re: [U-Boot] [PATCH 03/21] fsl_ifc: add support for different IFC bank count
On Fri, Mar 22, 2013 at 12:23 PM, York Sun <yorksun at freescale.com<mailto:yorksun at freescale.com>> wrote:
From: Mingkai Hu <Mingkai.hu at freescale.com<mailto: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?
[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.
Signed-off-by: Mingkai Hu <Mingkai.hu at freescale.com<mailto: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.
[...]
[Mingkai] These codes are redundant, we can factor this out in another separate patch.
Prabhakar, what would you say here?
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)];
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.
Thanks,
Mingkai
+#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<mailto:U-Boot at lists.denx.de>
http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list