[U-Boot] [PATCH] OMAP3EVM: net_chip uses CS5 not CS6
Signed-off-by: Matthias Ludwig mludwig@ultratronik.de --- board/omap3/evm/evm.c | 16 ++++++++-------- include/asm-arm/arch-omap3/cpu.h | 5 +++-- 2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/board/omap3/evm/evm.c b/board/omap3/evm/evm.c index c008c2e..5fd5efa 100644 --- a/board/omap3/evm/evm.c +++ b/board/omap3/evm/evm.c @@ -92,17 +92,17 @@ void set_muxconf_regs(void) static void setup_net_chip(void) { gpio_t *gpio3_base = (gpio_t *)OMAP34XX_GPIO3_BASE; - gpmc_csx_t *gpmc_cs6_base = (gpmc_csx_t *)GPMC_CONFIG_CS6_BASE; + gpmc_csx_t *gpmc_cs5_base = (gpmc_csx_t *)GPMC_CONFIG_CS5_BASE; ctrl_t *ctrl_base = (ctrl_t *)OMAP34XX_CTRL_BASE;
/* Configure GPMC registers */ - writel(NET_GPMC_CONFIG1, &gpmc_cs6_base->config1); - writel(NET_GPMC_CONFIG2, &gpmc_cs6_base->config2); - writel(NET_GPMC_CONFIG3, &gpmc_cs6_base->config3); - writel(NET_GPMC_CONFIG4, &gpmc_cs6_base->config4); - writel(NET_GPMC_CONFIG5, &gpmc_cs6_base->config5); - writel(NET_GPMC_CONFIG6, &gpmc_cs6_base->config6); - writel(NET_GPMC_CONFIG7, &gpmc_cs6_base->config7); + writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1); + writel(NET_GPMC_CONFIG2, &gpmc_cs5_base->config2); + writel(NET_GPMC_CONFIG3, &gpmc_cs5_base->config3); + writel(NET_GPMC_CONFIG4, &gpmc_cs5_base->config4); + writel(NET_GPMC_CONFIG5, &gpmc_cs5_base->config5); + writel(NET_GPMC_CONFIG6, &gpmc_cs5_base->config6); + writel(NET_GPMC_CONFIG7, &gpmc_cs5_base->config7);
/* Enable off mode for NWE in PADCONF_GPMC_NWE register */ writew(readw(&ctrl_base ->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe); diff --git a/include/asm-arm/arch-omap3/cpu.h b/include/asm-arm/arch-omap3/cpu.h index c544e0c..a4ce45a 100644 --- a/include/asm-arm/arch-omap3/cpu.h +++ b/include/asm-arm/arch-omap3/cpu.h @@ -84,9 +84,10 @@ typedef struct ctrl_id { /* GPMC CS3/cs4/cs6 not avaliable */ #define GPMC_BASE (OMAP34XX_GPMC_BASE) #define GPMC_CONFIG_CS0 0x60 -#define GPMC_CONFIG_CS6 0x150 +#define GPMC_CONFIG_CS5 0x150 + #define GPMC_CONFIG_CS0_BASE (GPMC_BASE + GPMC_CONFIG_CS0) -#define GPMC_CONFIG_CS6_BASE (GPMC_BASE + GPMC_CONFIG_CS6) +#define GPMC_CONFIG_CS5_BASE (GPMC_BASE + GPMC_CONFIG_CS5) #define GPMC_CONFIG_WP 0x10
#define GPMC_CONFIG_WIDTH 0x30
Matthias Ludwig wrote:
Signed-off-by: Matthias Ludwig mludwig@ultratronik.de
Matthias: Thanks for fixing this!
Mani: Can we get your ack as EVM maintainer?
Many thanks and best regards
Dirk
board/omap3/evm/evm.c | 16 ++++++++-------- include/asm-arm/arch-omap3/cpu.h | 5 +++-- 2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/board/omap3/evm/evm.c b/board/omap3/evm/evm.c index c008c2e..5fd5efa 100644 --- a/board/omap3/evm/evm.c +++ b/board/omap3/evm/evm.c @@ -92,17 +92,17 @@ void set_muxconf_regs(void) static void setup_net_chip(void) { gpio_t *gpio3_base = (gpio_t *)OMAP34XX_GPIO3_BASE;
- gpmc_csx_t *gpmc_cs6_base = (gpmc_csx_t *)GPMC_CONFIG_CS6_BASE;
gpmc_csx_t *gpmc_cs5_base = (gpmc_csx_t *)GPMC_CONFIG_CS5_BASE; ctrl_t *ctrl_base = (ctrl_t *)OMAP34XX_CTRL_BASE;
/* Configure GPMC registers */
- writel(NET_GPMC_CONFIG1, &gpmc_cs6_base->config1);
- writel(NET_GPMC_CONFIG2, &gpmc_cs6_base->config2);
- writel(NET_GPMC_CONFIG3, &gpmc_cs6_base->config3);
- writel(NET_GPMC_CONFIG4, &gpmc_cs6_base->config4);
- writel(NET_GPMC_CONFIG5, &gpmc_cs6_base->config5);
- writel(NET_GPMC_CONFIG6, &gpmc_cs6_base->config6);
- writel(NET_GPMC_CONFIG7, &gpmc_cs6_base->config7);
writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1);
writel(NET_GPMC_CONFIG2, &gpmc_cs5_base->config2);
writel(NET_GPMC_CONFIG3, &gpmc_cs5_base->config3);
writel(NET_GPMC_CONFIG4, &gpmc_cs5_base->config4);
writel(NET_GPMC_CONFIG5, &gpmc_cs5_base->config5);
writel(NET_GPMC_CONFIG6, &gpmc_cs5_base->config6);
writel(NET_GPMC_CONFIG7, &gpmc_cs5_base->config7);
/* Enable off mode for NWE in PADCONF_GPMC_NWE register */ writew(readw(&ctrl_base ->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe);
diff --git a/include/asm-arm/arch-omap3/cpu.h b/include/asm-arm/arch-omap3/cpu.h index c544e0c..a4ce45a 100644 --- a/include/asm-arm/arch-omap3/cpu.h +++ b/include/asm-arm/arch-omap3/cpu.h @@ -84,9 +84,10 @@ typedef struct ctrl_id { /* GPMC CS3/cs4/cs6 not avaliable */ #define GPMC_BASE (OMAP34XX_GPMC_BASE) #define GPMC_CONFIG_CS0 0x60 -#define GPMC_CONFIG_CS6 0x150 +#define GPMC_CONFIG_CS5 0x150
#define GPMC_CONFIG_CS0_BASE (GPMC_BASE + GPMC_CONFIG_CS0) -#define GPMC_CONFIG_CS6_BASE (GPMC_BASE + GPMC_CONFIG_CS6) +#define GPMC_CONFIG_CS5_BASE (GPMC_BASE + GPMC_CONFIG_CS5) #define GPMC_CONFIG_WP 0x10
#define GPMC_CONFIG_WIDTH 0x30
-----Original Message----- From: Dirk Behme [mailto:dirk.behme@googlemail.com] Sent: Wednesday, May 06, 2009 8:25 PM To: Matthias Ludwig; Pillai, Manikandan Cc: u-boot@lists.denx.de Subject: Re: [PATCH] OMAP3EVM: net_chip uses CS5 not CS6
Matthias Ludwig wrote:
Signed-off-by: Matthias Ludwig mludwig@ultratronik.de
Matthias: Thanks for fixing this!
[Pillai, Manikandan] not sure why this change in the Chip select
Mani: Can we get your ack as EVM maintainer?
Many thanks and best regards
Dirk
board/omap3/evm/evm.c | 16 ++++++++-------- include/asm-arm/arch-omap3/cpu.h | 5 +++-- 2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/board/omap3/evm/evm.c b/board/omap3/evm/evm.c index c008c2e..5fd5efa 100644 --- a/board/omap3/evm/evm.c +++ b/board/omap3/evm/evm.c @@ -92,17 +92,17 @@ void set_muxconf_regs(void) static void setup_net_chip(void) { gpio_t *gpio3_base = (gpio_t *)OMAP34XX_GPIO3_BASE;
- gpmc_csx_t *gpmc_cs6_base = (gpmc_csx_t *)GPMC_CONFIG_CS6_BASE;
gpmc_csx_t *gpmc_cs5_base = (gpmc_csx_t *)GPMC_CONFIG_CS5_BASE; ctrl_t *ctrl_base = (ctrl_t *)OMAP34XX_CTRL_BASE;
/* Configure GPMC registers */
- writel(NET_GPMC_CONFIG1, &gpmc_cs6_base->config1);
- writel(NET_GPMC_CONFIG2, &gpmc_cs6_base->config2);
- writel(NET_GPMC_CONFIG3, &gpmc_cs6_base->config3);
- writel(NET_GPMC_CONFIG4, &gpmc_cs6_base->config4);
- writel(NET_GPMC_CONFIG5, &gpmc_cs6_base->config5);
- writel(NET_GPMC_CONFIG6, &gpmc_cs6_base->config6);
- writel(NET_GPMC_CONFIG7, &gpmc_cs6_base->config7);
writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1);
writel(NET_GPMC_CONFIG2, &gpmc_cs5_base->config2);
writel(NET_GPMC_CONFIG3, &gpmc_cs5_base->config3);
writel(NET_GPMC_CONFIG4, &gpmc_cs5_base->config4);
writel(NET_GPMC_CONFIG5, &gpmc_cs5_base->config5);
writel(NET_GPMC_CONFIG6, &gpmc_cs5_base->config6);
writel(NET_GPMC_CONFIG7, &gpmc_cs5_base->config7);
/* Enable off mode for NWE in PADCONF_GPMC_NWE register */ writew(readw(&ctrl_base ->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe);
diff --git a/include/asm-arm/arch-omap3/cpu.h b/include/asm-arm/arch-
omap3/cpu.h
index c544e0c..a4ce45a 100644 --- a/include/asm-arm/arch-omap3/cpu.h +++ b/include/asm-arm/arch-omap3/cpu.h @@ -84,9 +84,10 @@ typedef struct ctrl_id { /* GPMC CS3/cs4/cs6 not avaliable */ #define GPMC_BASE (OMAP34XX_GPMC_BASE) #define GPMC_CONFIG_CS0 0x60 -#define GPMC_CONFIG_CS6 0x150 +#define GPMC_CONFIG_CS5 0x150
#define GPMC_CONFIG_CS0_BASE (GPMC_BASE + GPMC_CONFIG_CS0) -#define GPMC_CONFIG_CS6_BASE (GPMC_BASE + GPMC_CONFIG_CS6) +#define GPMC_CONFIG_CS5_BASE (GPMC_BASE + GPMC_CONFIG_CS5) #define GPMC_CONFIG_WP 0x10
#define GPMC_CONFIG_WIDTH 0x30
This is not really a change. The cs configuration was correct, but not the naming of it.
OMAP34XX_GPMC_BASE (0x6e000000) + 0x150 = base address of configuration registers for GPMC-CS5 not GPMC-CS6.
best regards, Matthias
On Thu, May 07, 2009 at 12:34:01PM +0530, Pillai, Manikandan wrote:
-----Original Message----- From: Dirk Behme [mailto:dirk.behme@googlemail.com] Sent: Wednesday, May 06, 2009 8:25 PM To: Matthias Ludwig; Pillai, Manikandan Cc: u-boot@lists.denx.de Subject: Re: [PATCH] OMAP3EVM: net_chip uses CS5 not CS6
Matthias Ludwig wrote:
Signed-off-by: Matthias Ludwig mludwig@ultratronik.de
Matthias: Thanks for fixing this!
[Pillai, Manikandan] not sure why this change in the Chip select
Mani: Can we get your ack as EVM maintainer?
Many thanks and best regards
Dirk
board/omap3/evm/evm.c | 16 ++++++++-------- include/asm-arm/arch-omap3/cpu.h | 5 +++-- 2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/board/omap3/evm/evm.c b/board/omap3/evm/evm.c index c008c2e..5fd5efa 100644 --- a/board/omap3/evm/evm.c +++ b/board/omap3/evm/evm.c @@ -92,17 +92,17 @@ void set_muxconf_regs(void) static void setup_net_chip(void) { gpio_t *gpio3_base = (gpio_t *)OMAP34XX_GPIO3_BASE;
- gpmc_csx_t *gpmc_cs6_base = (gpmc_csx_t *)GPMC_CONFIG_CS6_BASE;
gpmc_csx_t *gpmc_cs5_base = (gpmc_csx_t *)GPMC_CONFIG_CS5_BASE; ctrl_t *ctrl_base = (ctrl_t *)OMAP34XX_CTRL_BASE;
/* Configure GPMC registers */
- writel(NET_GPMC_CONFIG1, &gpmc_cs6_base->config1);
- writel(NET_GPMC_CONFIG2, &gpmc_cs6_base->config2);
- writel(NET_GPMC_CONFIG3, &gpmc_cs6_base->config3);
- writel(NET_GPMC_CONFIG4, &gpmc_cs6_base->config4);
- writel(NET_GPMC_CONFIG5, &gpmc_cs6_base->config5);
- writel(NET_GPMC_CONFIG6, &gpmc_cs6_base->config6);
- writel(NET_GPMC_CONFIG7, &gpmc_cs6_base->config7);
writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1);
writel(NET_GPMC_CONFIG2, &gpmc_cs5_base->config2);
writel(NET_GPMC_CONFIG3, &gpmc_cs5_base->config3);
writel(NET_GPMC_CONFIG4, &gpmc_cs5_base->config4);
writel(NET_GPMC_CONFIG5, &gpmc_cs5_base->config5);
writel(NET_GPMC_CONFIG6, &gpmc_cs5_base->config6);
writel(NET_GPMC_CONFIG7, &gpmc_cs5_base->config7);
/* Enable off mode for NWE in PADCONF_GPMC_NWE register */ writew(readw(&ctrl_base ->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe);
diff --git a/include/asm-arm/arch-omap3/cpu.h b/include/asm-arm/arch-
omap3/cpu.h
index c544e0c..a4ce45a 100644 --- a/include/asm-arm/arch-omap3/cpu.h +++ b/include/asm-arm/arch-omap3/cpu.h @@ -84,9 +84,10 @@ typedef struct ctrl_id { /* GPMC CS3/cs4/cs6 not avaliable */ #define GPMC_BASE (OMAP34XX_GPMC_BASE) #define GPMC_CONFIG_CS0 0x60 -#define GPMC_CONFIG_CS6 0x150 +#define GPMC_CONFIG_CS5 0x150
#define GPMC_CONFIG_CS0_BASE (GPMC_BASE + GPMC_CONFIG_CS0) -#define GPMC_CONFIG_CS6_BASE (GPMC_BASE + GPMC_CONFIG_CS6) +#define GPMC_CONFIG_CS5_BASE (GPMC_BASE + GPMC_CONFIG_CS5) #define GPMC_CONFIG_WP 0x10
#define GPMC_CONFIG_WIDTH 0x30
ACKed
-----Original Message----- From: Matthias Ludwig [mailto:mludwig@ultratronik.de] Sent: Thursday, May 07, 2009 12:42 PM To: Pillai, Manikandan Cc: Dirk Behme; u-boot@lists.denx.de Subject: Re: [PATCH] OMAP3EVM: net_chip uses CS5 not CS6
This is not really a change. The cs configuration was correct, but not the naming of it.
OMAP34XX_GPMC_BASE (0x6e000000) + 0x150 = base address of configuration registers for GPMC-CS5 not GPMC-CS6.
best regards, Matthias
On Thu, May 07, 2009 at 12:34:01PM +0530, Pillai, Manikandan wrote:
-----Original Message----- From: Dirk Behme [mailto:dirk.behme@googlemail.com] Sent: Wednesday, May 06, 2009 8:25 PM To: Matthias Ludwig; Pillai, Manikandan Cc: u-boot@lists.denx.de Subject: Re: [PATCH] OMAP3EVM: net_chip uses CS5 not CS6
Matthias Ludwig wrote:
Signed-off-by: Matthias Ludwig mludwig@ultratronik.de
Matthias: Thanks for fixing this!
[Pillai, Manikandan] not sure why this change in the Chip select
Mani: Can we get your ack as EVM maintainer?
Many thanks and best regards
Dirk
board/omap3/evm/evm.c | 16 ++++++++-------- include/asm-arm/arch-omap3/cpu.h | 5 +++-- 2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/board/omap3/evm/evm.c b/board/omap3/evm/evm.c index c008c2e..5fd5efa 100644 --- a/board/omap3/evm/evm.c +++ b/board/omap3/evm/evm.c @@ -92,17 +92,17 @@ void set_muxconf_regs(void) static void setup_net_chip(void) { gpio_t *gpio3_base = (gpio_t *)OMAP34XX_GPIO3_BASE;
- gpmc_csx_t *gpmc_cs6_base = (gpmc_csx_t *)GPMC_CONFIG_CS6_BASE;
gpmc_csx_t *gpmc_cs5_base = (gpmc_csx_t *)GPMC_CONFIG_CS5_BASE; ctrl_t *ctrl_base = (ctrl_t *)OMAP34XX_CTRL_BASE;
/* Configure GPMC registers */
- writel(NET_GPMC_CONFIG1, &gpmc_cs6_base->config1);
- writel(NET_GPMC_CONFIG2, &gpmc_cs6_base->config2);
- writel(NET_GPMC_CONFIG3, &gpmc_cs6_base->config3);
- writel(NET_GPMC_CONFIG4, &gpmc_cs6_base->config4);
- writel(NET_GPMC_CONFIG5, &gpmc_cs6_base->config5);
- writel(NET_GPMC_CONFIG6, &gpmc_cs6_base->config6);
- writel(NET_GPMC_CONFIG7, &gpmc_cs6_base->config7);
writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1);
writel(NET_GPMC_CONFIG2, &gpmc_cs5_base->config2);
writel(NET_GPMC_CONFIG3, &gpmc_cs5_base->config3);
writel(NET_GPMC_CONFIG4, &gpmc_cs5_base->config4);
writel(NET_GPMC_CONFIG5, &gpmc_cs5_base->config5);
writel(NET_GPMC_CONFIG6, &gpmc_cs5_base->config6);
writel(NET_GPMC_CONFIG7, &gpmc_cs5_base->config7);
/* Enable off mode for NWE in PADCONF_GPMC_NWE register */ writew(readw(&ctrl_base ->gpmc_nwe) | 0x0E00, &ctrl_base-
gpmc_nwe);
diff --git a/include/asm-arm/arch-omap3/cpu.h b/include/asm-arm/arch-
omap3/cpu.h
index c544e0c..a4ce45a 100644 --- a/include/asm-arm/arch-omap3/cpu.h +++ b/include/asm-arm/arch-omap3/cpu.h @@ -84,9 +84,10 @@ typedef struct ctrl_id { /* GPMC CS3/cs4/cs6 not avaliable */ #define GPMC_BASE (OMAP34XX_GPMC_BASE) #define GPMC_CONFIG_CS0 0x60 -#define GPMC_CONFIG_CS6 0x150 +#define GPMC_CONFIG_CS5 0x150
#define GPMC_CONFIG_CS0_BASE (GPMC_BASE + GPMC_CONFIG_CS0) -#define GPMC_CONFIG_CS6_BASE (GPMC_BASE + GPMC_CONFIG_CS6) +#define GPMC_CONFIG_CS5_BASE (GPMC_BASE + GPMC_CONFIG_CS5) #define GPMC_CONFIG_WP 0x10
#define GPMC_CONFIG_WIDTH 0x30
-- Matthias Ludwig, Software Development Ultratronik Entwicklungs GmbH, Gewerbestrasse 52, 82211 Herrsching, Germany http://www.ultratronik.de Tel: +49 8152 3709-356 Fax: +49 8152 5183 Registergericht Muenchen, HRB 55584
Dear Matthias Ludwig,
In message 20090507071155.GA8961@ultratronik.de you wrote:
This is not really a change. The cs configuration was correct, but not the naming of it.
OMAP34XX_GPMC_BASE (0x6e000000) + 0x150 = base address of configuration registers for GPMC-CS5 not GPMC-CS6.
Can we please get rid of all this crap with register offsets and device accesses through pointers using base address plus offset?
Please provide proper C structs!
Best regards,
Wolfgang Denk
Dear Wolfgang,
Wolfgang Denk wrote:
Dear Matthias Ludwig,
In message 20090507071155.GA8961@ultratronik.de you wrote:
This is not really a change. The cs configuration was correct, but not the naming of it.
OMAP34XX_GPMC_BASE (0x6e000000) + 0x150 = base address of configuration registers for GPMC-CS5 not GPMC-CS6.
Can we please get rid of all this crap with register offsets and device accesses through pointers using base address plus offset?
Please provide proper C structs!
Would you like to have a look to the code snippet visible in Matthias' patch
http://lists.denx.de/pipermail/u-boot/2009-May/052157.html
?
It's my understanding that what's in code
writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1);
is what you want? I.e. as I understand it, the code is correct (we use C structs), and the style Matthias used above you complain about was just for patch explanation (to make it easier understandable).
Sorry if I missed something, just correct then ;)
Best regards
Dirk
Dear Dirk,
In message 4A02FB34.2090907@googlemail.com you wrote:
Please provide proper C structs!
Would you like to have a look to the code snippet visible in Matthias' patch
http://lists.denx.de/pipermail/u-boot/2009-May/052157.html
?
Done.
It's my understanding that what's in code
writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1);
is what you want? I.e. as I understand it, the code is correct (we use
It is only part of what I want to see. There are still deficiencies.
Yes, we have a struct gpmc_csx, that's good.
Note though that using such typedef's not in line with the Coding Style, which says: "It's a _mistake_ to use typedef for structures and pointers." Also, checkpatch.pl complains about this.
The next problem is that the entries in these structs are declared as "unsigned int", which just happens to work by chance. "u32" would be more reliable.
Finally, and this is what I really compalin about, is that there is no big structure which includes all the blocks that make up the CPU into one big structure (as it's done for example for PowerPC systems in the include/asm-ppc/*immap* files) - you still use code like
gpmc_csx_t *gpmc_cs5_base = (gpmc_csx_t *)GPMC_CONFIG_CS5_BASE;
to locate each of the individual C structs in the memory map; instead, onle one single pointer to the internal memory should be needed.
Sorry if I missed something, just correct then ;)
Done. Thanks.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Finally, and this is what I really compalin about, is that there is no big structure which includes all the blocks that make up the CPU into one big structure (as it's done for example for PowerPC systems in the include/asm-ppc/*immap* files) - you still use code like
Those immap structs are a huge pain to maintain (or to verify the correctness of), loaded with ifdeffery and magic numbers describing reserved spans. We should not be emulating them.
We used to have them in Linux, and got rid of them.
-Scott
Dear Scott,
In message 4A0333FC.6090900@freescale.com you wrote:
Wolfgang Denk wrote:
Finally, and this is what I really compalin about, is that there is no big structure which includes all the blocks that make up the CPU into one big structure (as it's done for example for PowerPC systems in the include/asm-ppc/*immap* files) - you still use code like
Those immap structs are a huge pain to maintain (or to verify the correctness of), loaded with ifdeffery and magic numbers describing reserved spans. We should not be emulating them.
Well, #define'ing long lists of register offsets is not easier to maintain or verify, and you don't have any typechecking by the compiler.
We used to have them in Linux, and got rid of them.
Hm... Seems I have missed this change... What's things like
struct qe_immap __iomem *qe_immr or cpm2_map_t __iomem *cpm2_immr or immap_t __iomem *mpc8xx_immr
then? Or what replaced the "immr" structs?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Scott,
In message 4A0333FC.6090900@freescale.com you wrote:
Wolfgang Denk wrote:
Finally, and this is what I really compalin about, is that there is no big structure which includes all the blocks that make up the CPU into one big structure (as it's done for example for PowerPC systems in the include/asm-ppc/*immap* files) - you still use code like
Those immap structs are a huge pain to maintain (or to verify the correctness of), loaded with ifdeffery and magic numbers describing reserved spans. We should not be emulating them.
Well, #define'ing long lists of register offsets is not easier to maintain or verify,
IMHO it is; you can just compare to the manual rather than have offsets be screwed up if something is missing, out of order, or the wrong size. That's countered by the typechecking and ease of use of structs, though.
and you don't have any typechecking by the compiler.
It doesn't have to be all one or the other. Use structs to describe individual blocks (provided they're not too sparsely populated), but define block addresses rather than huge structs consisting of sub-blocks and empty space. And let the details be flexible at the author/maintainer's discretion, rather than a rigid rule.
We used to have them in Linux, and got rid of them.
Hm... Seems I have missed this change... What's things like
struct qe_immap __iomem *qe_immr or cpm2_map_t __iomem *cpm2_immr or immap_t __iomem *mpc8xx_immr
then?
Legacy stuff that hasn't been fully cleaned up. There used to be immap structs for 83xx and 85xx back in arch/ppc IIRC.
Or what replaced the "immr" structs?
The device tree, mainly. But #defines can work for u-boot.
-Scott
Dear Scott Wood,
In message 4A034B09.7030105@freescale.com you wrote:
Or what replaced the "immr" structs?
The device tree, mainly...
Right, of course.
... But #defines can work for u-boot.
Of course they _can_ work. But they can easily fail (as we just see in this patch), and we don't have typechecking. So until DT's are omnipresent, let's use structs in U-Boot, please.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Scott Wood,
In message 4A034B09.7030105@freescale.com you wrote:
Or what replaced the "immr" structs?
The device tree, mainly...
Right, of course.
... But #defines can work for u-boot.Of course they _can_ work. But they can easily fail (as we just see in this patch), and we don't have typechecking. So until DT's are omnipresent, let's use structs in U-Boot, please.
You *do* have typechecking as long as the individual blocks are described with structs.
We could take immap to extremes by defining one big 4GiB struct that shows where memory, immr, flash, desired PCI bars, FPGAs, etc. are -- but that would be silly. IMHO, so is doing it at the immr level. :-)
How would you deal with blocks being at different locations in different chips? It's a lot easier to ifdef (or have the config file specify) a couple addresses than to ifdef the locations of fields in a struct, especially when you have more than a couple variations.
-Scott
Hi,
Wolfgang Denk wrote:
Dear Scott Wood,
In message 4A034B09.7030105@freescale.com you wrote:
Or what replaced the "immr" structs?
The device tree, mainly...
Right, of course.
... But #defines can work for u-boot.Of course they _can_ work. But they can easily fail (as we just see in this patch), and we don't have typechecking. So until DT's are omnipresent, let's use structs in U-Boot, please.
You *do* have typechecking as long as the individual blocks are described with structs.
We could take immap to extremes by defining one big 4GiB struct that shows where memory, immr, flash, desired PCI bars, FPGAs, etc. are -- but that would be silly. IMHO, so is doing it at the immr level. :-)
How would you deal with blocks being at different locations in different chips? It's a lot easier to ifdef (or have the config file specify) a couple addresses than to ifdef the locations of fields in a struct, especially when you have more than a couple variations.
For what its worth, I'm with Scott here. Structures for register blocks is very nice and should be mandated and it seems they are maintainable. Locations of individual blocks (or number of incarnations thereof) seem to change frequently and thus tend to be less friendly to "whole internal address space" structures. So the latter may better be mapped by single defines. The correctness of them is easily validated and an incorrect value will immediatley be discovered.
Cheers Detlev
Hi,
Detlev Zundel wrote:
Hi,
Wolfgang Denk wrote:
Dear Scott Wood,
In message 4A034B09.7030105@freescale.com you wrote:
Or what replaced the "immr" structs?
The device tree, mainly...
Right, of course.
... But #defines can work for u-boot.Of course they _can_ work. But they can easily fail (as we just see in this patch), and we don't have typechecking. So until DT's are omnipresent, let's use structs in U-Boot, please.
You *do* have typechecking as long as the individual blocks are described with structs.
We could take immap to extremes by defining one big 4GiB struct that shows where memory, immr, flash, desired PCI bars, FPGAs, etc. are -- but that would be silly. IMHO, so is doing it at the immr level. :-)
How would you deal with blocks being at different locations in different chips? It's a lot easier to ifdef (or have the config file specify) a couple addresses than to ifdef the locations of fields in a struct, especially when you have more than a couple variations.
For what its worth, I'm with Scott here. Structures for register blocks is very nice and should be mandated and it seems they are maintainable. Locations of individual blocks (or number of incarnations thereof) seem to change frequently and thus tend to be less friendly to "whole internal address space" structures. So the latter may better be mapped by single defines. The correctness of them is easily validated and an incorrect value will immediatley be discovered.
I tend to agree with Scott and Detlev, too. At least from practical point of view
http://www.ti.com/litv/pdf/spruf98b
(attention: ~40MB) ;)
Best regards
Dirk
Wolfgang, Dirk,
cause for this patch was originally the plan to bring my company's new omap3-based platform into U-Boot. As we do use other CS for a network chip this patch was intended to be the starting point (otherwise our patchset will not apply).
I can prepare a big #define -> c_struct patch for omap next week. The only problem i see, my following patches will be rebased onto that and will only apply after the transition #define -> c_struct is done.
If this is ok for you i will submit this mid/end of next week.
best regards, Matthias
On Thu, May 07, 2009 at 08:58:17PM +0200, Wolfgang Denk wrote:
Dear Dirk,
In message 4A02FB34.2090907@googlemail.com you wrote:
Please provide proper C structs!
Would you like to have a look to the code snippet visible in Matthias' patch
http://lists.denx.de/pipermail/u-boot/2009-May/052157.html
?
Done.
It's my understanding that what's in code
writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1);
is what you want? I.e. as I understand it, the code is correct (we use
It is only part of what I want to see. There are still deficiencies.
Yes, we have a struct gpmc_csx, that's good.
Note though that using such typedef's not in line with the Coding Style, which says: "It's a _mistake_ to use typedef for structures and pointers." Also, checkpatch.pl complains about this.
The next problem is that the entries in these structs are declared as "unsigned int", which just happens to work by chance. "u32" would be more reliable.
Finally, and this is what I really compalin about, is that there is no big structure which includes all the blocks that make up the CPU into one big structure (as it's done for example for PowerPC systems in the include/asm-ppc/*immap* files) - you still use code like
gpmc_csx_t *gpmc_cs5_base = (gpmc_csx_t *)GPMC_CONFIG_CS5_BASE;
to locate each of the individual C structs in the memory map; instead, onle one single pointer to the internal memory should be needed.
Sorry if I missed something, just correct then ;)
Done. Thanks.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Winners never talk about glorious victories. That's because they're the ones who see what the battlefield looks like afterwards. It's only the losers who have glorious victories. - Terry Pratchett, _Small Gods_
Dear Matthias,
In message 20090508084234.GA6403@ultratronik.de you wrote:
cause for this patch was originally the plan to bring my company's new omap3-based platform into U-Boot. As we do use other CS for a network chip this patch was intended to be the starting point (otherwise our patchset will not apply).
I can prepare a big #define -> c_struct patch for omap next week. The only problem i see, my following patches will be rebased onto that and will only apply after the transition #define -> c_struct is done.
If this is ok for you i will submit this mid/end of next week.
It's fine with me.
Best regards,
Wolfgang Denk
Dear Matthias,
Matthias Ludwig wrote:
Wolfgang, Dirk,
cause for this patch was originally the plan to bring my company's new omap3-based platform into U-Boot. As we do use other CS for a network chip this patch was intended to be the starting point (otherwise our patchset will not apply).
I can prepare a big #define -> c_struct patch for omap next week. The only problem i see, my following patches will be rebased onto that and will only apply after the transition #define -> c_struct is done.
If this is ok for you i will submit this mid/end of next week.
If you do it for all OMAP3 boards currently in mainline and don't break anything, I'm fine with this, too.
Many thanks
Dirk
On Thu, May 07, 2009 at 08:58:17PM +0200, Wolfgang Denk wrote:
Dear Dirk,
In message 4A02FB34.2090907@googlemail.com you wrote:
Please provide proper C structs!
Would you like to have a look to the code snippet visible in Matthias' patch
http://lists.denx.de/pipermail/u-boot/2009-May/052157.html
?
Done.
It's my understanding that what's in code
writel(NET_GPMC_CONFIG1, &gpmc_cs5_base->config1);
is what you want? I.e. as I understand it, the code is correct (we use
It is only part of what I want to see. There are still deficiencies.
Yes, we have a struct gpmc_csx, that's good.
Note though that using such typedef's not in line with the Coding Style, which says: "It's a _mistake_ to use typedef for structures and pointers." Also, checkpatch.pl complains about this.
The next problem is that the entries in these structs are declared as "unsigned int", which just happens to work by chance. "u32" would be more reliable.
Finally, and this is what I really compalin about, is that there is no big structure which includes all the blocks that make up the CPU into one big structure (as it's done for example for PowerPC systems in the include/asm-ppc/*immap* files) - you still use code like
gpmc_csx_t *gpmc_cs5_base = (gpmc_csx_t *)GPMC_CONFIG_CS5_BASE;
to locate each of the individual C structs in the memory map; instead, onle one single pointer to the internal memory should be needed.
Sorry if I missed something, just correct then ;)
Done. Thanks.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Winners never talk about glorious victories. That's because they're the ones who see what the battlefield looks like afterwards. It's only the losers who have glorious victories. - Terry Pratchett, _Small Gods_
On 09:11 Thu 07 May , Matthias Ludwig wrote:
This is not really a change. The cs configuration was correct, but not the naming of it.
OMAP34XX_GPMC_BASE (0x6e000000) + 0x150 = base address of configuration registers for GPMC-CS5 not GPMC-CS6.
so please fix the comment
Best Regards, J.
participants (7)
-
Detlev Zundel -
Dirk Behme -
Jean-Christophe PLAGNIOL-VILLARD -
Matthias Ludwig -
Pillai, Manikandan -
Scott Wood -
Wolfgang Denk