| Message ID | 1334735852-23415-8-git-send-email-timo@exertus.fi |
|---|---|
| State | Rejected, archived |
| Headers | show |
On 18/04/2012 09:57, Timo Ketola wrote: > One might want to define CONFIG_SYS_FSL_ESDHC_ADDR with the macro already > define in imx-regs.h, e.g. with IMX_MMC_SDHC1_BASE. Then the header must be > included here. > > Signed-off-by: Timo Ketola <timo@exertus.fi> > --- > drivers/mmc/fsl_esdhc.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c > index a2f35e3..5ada747 100644 > --- a/drivers/mmc/fsl_esdhc.c > +++ b/drivers/mmc/fsl_esdhc.c > @@ -36,6 +36,7 @@ > #include <fsl_esdhc.h> > #include <fdt_support.h> > #include <asm/io.h> > +#include <asm/arch/imx-regs.h> > NAK. There is a good reason to avoid it. The fsl_esdhc driver is common to both i.MX and PowerPc architecture, and of course PowerPC have not imx-regs.h. And CONFIG_SYS_FSL_ESDHC_ADDR cannot be set by a macro in imx-regs.h, because it is different on PowerPC. By the way, why do you need it if you do not use that macro ? Best regards, Stefano Babic
On 18.04.2012 11:43, Stefano Babic wrote: > On 18/04/2012 09:57, Timo Ketola wrote: >> One might want to define CONFIG_SYS_FSL_ESDHC_ADDR with the macro already >> define in imx-regs.h, e.g. with IMX_MMC_SDHC1_BASE. Then the header must be >> included here. >> ... >> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c >> ... >> +#include<asm/arch/imx-regs.h> > > NAK. There is a good reason to avoid it. The fsl_esdhc driver is common > to both i.MX and PowerPc architecture, and of course PowerPC have not > imx-regs.h. And CONFIG_SYS_FSL_ESDHC_ADDR cannot be set by a macro in > imx-regs.h, because it is different on PowerPC. Ok, I was afraid about something like that and tried first to include it in board configuration but that broke something else (at least arm926ejs didn't compile any more). > By the way, why do you need it if you do not use that macro ? I use it in my board (support of which I'm preparing to send) configuration file and I think it is annoying to write a literal constant there which is already defined in imx-regs.h. PPC seems to use a predefined macro from asm/immap_8xxx.h files. Where is that file included? -- Timo
On 18/04/2012 11:11, Timo Ketola wrote: > > Ok, I was afraid about something like that and tried first to include it > in board configuration but that broke something else (at least arm926ejs > didn't compile any more). > >> By the way, why do you need it if you do not use that macro ? > > I use it in my board (support of which I'm preparing to send) > configuration file and I think it is annoying to write a literal > constant there which is already defined in imx-regs.h. fsl_esdhc.c includes config.h. If your board configuration file includes imx-regs.h, as most i.MX boards do, the file is automatically included, I suppose. > > PPC seems to use a predefined macro from asm/immap_8xxx.h files. Where > is that file included? It is a different way. The board configuration file includes the register description file, so for example immap_86xx.h, immap_85xx.h, or imx-regs.h, and defines CONFIG_SYS_FSL_ESDHC_ADDR using its own specific macro, if any, for example: #define CONFIG_SYS_FSL_ESDHC_ADDR CONFIG_SYS_MPC85xx_ESDHC_ADDR Why is it not enough for you to set in your board configuration file: #define CONFIG_SYS_FSL_ESDHC_ADDR IMX_MMC_SDHC1_BASE Best regards, Stefano Babic
On 18.04.2012 13:30, Stefano Babic wrote: > On 18/04/2012 11:11, Timo Ketola wrote: > >> >> Ok, I was afraid about something like that and tried first to include it >> in board configuration but that broke something else (at least arm926ejs >> didn't compile any more). >> >>> By the way, why do you need it if you do not use that macro ? >> >> I use it in my board (support of which I'm preparing to send) >> configuration file and I think it is annoying to write a literal >> constant there which is already defined in imx-regs.h. > > fsl_esdhc.c includes config.h. If your board configuration file includes > imx-regs.h, as most i.MX boards do, the file is automatically included, > I suppose. I tried that but then: .../u-boot-imx/build-exe4026/include/asm/arch/imx-regs.h:43:2: error: expected specifier-qualifier-list before ‘u32’ when compiling arch/arm/cpu/arm926ejs/cpu.o > >> >> PPC seems to use a predefined macro from asm/immap_8xxx.h files. Where >> is that file included? > > It is a different way. The board configuration file includes the > register description file, so for example immap_86xx.h, immap_85xx.h, Where? I don't see an example. But I see them included in common.h. Should there be also imx-regs? Seems to work if I do so. > or > imx-regs.h, and defines CONFIG_SYS_FSL_ESDHC_ADDR using its own specific > macro, if any, for example: > > #define CONFIG_SYS_FSL_ESDHC_ADDR CONFIG_SYS_MPC85xx_ESDHC_ADDR > > Why is it not enough for you to set in your board configuration file: > > #define CONFIG_SYS_FSL_ESDHC_ADDR IMX_MMC_SDHC1_BASE I tried also exactly that, but then: fsl_esdhc.c:544:20: error: ‘IMX_MMC_SDHC1_BASE’ undeclared (first use in this function) fsl_esdhc.c seems not to see imx-regs.h file. Then I tried to include imx-regs.h in fsl_esdhc.c and 'MAKEALL -a arm' was happy. Maybe the right fix is to include imx-regs in common.h? What would be the right expression for #ifdef? -- Timo
On 18/04/2012 13:05, Timo Ketola wrote: >> >> fsl_esdhc.c includes config.h. If your board configuration file includes >> imx-regs.h, as most i.MX boards do, the file is automatically included, >> I suppose. > > I tried that but then: > > .../u-boot-imx/build-exe4026/include/asm/arch/imx-regs.h:43:2: error: > expected specifier-qualifier-list before ‘u32’ > > when compiling > > arch/arm/cpu/arm926ejs/cpu.o Well, I have not said that there cannot be other issues. At first glance you must include asm/types.h, in cpu.c or in imx-regs.h. >>> PPC seems to use a predefined macro from asm/immap_8xxx.h files. Where >>> is that file included? >> >> It is a different way. The board configuration file includes the >> register description file, so for example immap_86xx.h, immap_85xx.h, > > Where? I don't see an example. For PPC86xx I can see at least: arch/powerpc/cpu/mpc86xx/mpc8641_serdes.c:#include <asm/immap_86xx.h> arch/powerpc/cpu/mpc86xx/mpc8610_serdes.c:#include <asm/immap_86xx.h> board/freescale/mpc8610hpcd/mpc8610hpcd.c:#include <asm/immap_86xx.h> board/freescale/mpc8641hpcn/mpc8641hpcn.c:#include <asm/immap_86xx.h> > But I see them included in common.h. > Should there be also imx-regs? Seems to work if I do so. No, this is wrong. > >> or >> imx-regs.h, and defines CONFIG_SYS_FSL_ESDHC_ADDR using its own specific >> macro, if any, for example: >> >> #define CONFIG_SYS_FSL_ESDHC_ADDR CONFIG_SYS_MPC85xx_ESDHC_ADDR >> >> Why is it not enough for you to set in your board configuration file: >> >> #define CONFIG_SYS_FSL_ESDHC_ADDR IMX_MMC_SDHC1_BASE > > I tried also exactly that, but then: > > fsl_esdhc.c:544:20: error: ‘IMX_MMC_SDHC1_BASE’ undeclared (first use in > this function) ...then imx-regs.h was not included... > > fsl_esdhc.c seems not to see imx-regs.h file. > > Then I tried to include imx-regs.h in fsl_esdhc.c and 'MAKEALL -a arm' > was happy. > > Maybe the right fix is to include imx-regs in common.h? No. common.h, as the name suggests, is for all architectures, not only for i.MX. We cannot fix i:MX and break other boards. Best regards, Stefano Babic
On 18.04.2012 18:05, Stefano Babic wrote: > On 18/04/2012 13:05, Timo Ketola wrote: >> Stefano Babic wrote: >>> Timo Ketola wrote: >>>> PPC seems to use a predefined macro from asm/immap_8xxx.h files. Where >>>> is that file included? >>> >>> It is a different way. The board configuration file includes the >>> register description file, so for example immap_86xx.h, immap_85xx.h, >> >> Where? I don't see an example. > > For PPC86xx I can see at least: > > arch/powerpc/cpu/mpc86xx/mpc8641_serdes.c:#include<asm/immap_86xx.h> > arch/powerpc/cpu/mpc86xx/mpc8610_serdes.c:#include<asm/immap_86xx.h> > board/freescale/mpc8610hpcd/mpc8610hpcd.c:#include<asm/immap_86xx.h> > board/freescale/mpc8641hpcn/mpc8641hpcn.c:#include<asm/immap_86xx.h> Yes, I saw those but when you said that board configuration file includes those, I thought that you meant the header files in include/configs. >> But I see them included in common.h. >> Should there be also imx-regs? Seems to work if I do so. > > No, this is wrong. ... >> Then I tried to include imx-regs.h in fsl_esdhc.c and 'MAKEALL -a arm' >> was happy. >> >> Maybe the right fix is to include imx-regs in common.h? > > No. common.h, as the name suggests, is for all architectures, not only > for i.MX. We cannot fix i:MX and break other boards. But why PPC register description files are included there then? For example line 87: #ifdef CONFIG_MPC86xx #include <mpc86xx.h> #include <asm/immap_86xx.h> #endif Is that deprecated? And how would adding imx file with the same logic break other boards? I mean, putting there: #if defined(CONFIG_MX25) || defined(CONFIG_MX31) || ... #include <asm/arch/imx-regs.h> #endif But if the board configuration file in include/configs is the correct place to include it, I shall then find the obstacle on that approach... -- Timo
On 18.04.2012 19:27, Timo Ketola wrote: > But if the board configuration file in include/configs is the correct place to > include it, I shall then find the obstacle on that approach... Ok, including asm/arch/imx-regs.h in board configuration file *and* asm/types.h in asm/arch/imx-regs.h file seems to make build happy. This would be the right fix then? -- Timo
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index a2f35e3..5ada747 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -36,6 +36,7 @@ #include <fsl_esdhc.h> #include <fdt_support.h> #include <asm/io.h> +#include <asm/arch/imx-regs.h> DECLARE_GLOBAL_DATA_PTR;
One might want to define CONFIG_SYS_FSL_ESDHC_ADDR with the macro already define in imx-regs.h, e.g. with IMX_MMC_SDHC1_BASE. Then the header must be included here. Signed-off-by: Timo Ketola <timo@exertus.fi> --- drivers/mmc/fsl_esdhc.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)