| Message ID | 1291288812-12653-1-git-send-email-premi@ti.com |
|---|---|
| State | Rejected |
| Delegated to: | Sandeep Paulraj |
| Headers | show |
Dear Sanjeev Premi, In message <1291288812-12653-1-git-send-email-premi@ti.com> you wrote: > This patch does following changes: > * Change the type (u8 -> int) for omap3_evm_version. > * Introduce an 'undefined' board revision for init > value. > * Use of #define instead of magic numbers > > Signed-off-by: Sanjeev Premi <premi@ti.com> > --- > board/ti/evm/evm.c | 39 +++++++++++++++++++++++---------------- > board/ti/evm/evm.h | 17 +++++++---------- > 2 files changed, 30 insertions(+), 26 deletions(-) > > diff --git a/board/ti/evm/evm.c b/board/ti/evm/evm.c > index 09d14f7..8d9ce73 100644 > --- a/board/ti/evm/evm.c > +++ b/board/ti/evm/evm.c > @@ -37,36 +37,43 @@ > #include <asm/mach-types.h> > #include "evm.h" > > -static u8 omap3_evm_version; > +static int omap3_evm_version = OMAP3EVM_BOARD_UNDEF; ... > +#define OMAP3EVM_BOARD_UNDEF -1 /* EVM revision not detected */ Sorry, but I will not accept this patch. The only purpose of this initialization with a non-zero value is to paper over a problem. As a result, the problem will be left unsolved, so it bites us again elsewhere, and we increase the memory footprint of the U-Boot image without need. NAK. Best regards, Wolfgang Denk
Le 02/12/2010 12:37, Wolfgang Denk a écrit : > Dear Sanjeev Premi, > > In message<1291288812-12653-1-git-send-email-premi@ti.com> you wrote: >> This patch does following changes: >> * Change the type (u8 -> int) for omap3_evm_version. >> * Introduce an 'undefined' board revision for init >> value. >> * Use of #define instead of magic numbers >> >> Signed-off-by: Sanjeev Premi<premi@ti.com> >> --- >> board/ti/evm/evm.c | 39 +++++++++++++++++++++++---------------- >> board/ti/evm/evm.h | 17 +++++++---------- >> 2 files changed, 30 insertions(+), 26 deletions(-) >> >> diff --git a/board/ti/evm/evm.c b/board/ti/evm/evm.c >> index 09d14f7..8d9ce73 100644 >> --- a/board/ti/evm/evm.c >> +++ b/board/ti/evm/evm.c >> @@ -37,36 +37,43 @@ >> #include<asm/mach-types.h> >> #include "evm.h" >> >> -static u8 omap3_evm_version; >> +static int omap3_evm_version = OMAP3EVM_BOARD_UNDEF; > ... >> +#define OMAP3EVM_BOARD_UNDEF -1 /* EVM revision not detected */ > > Sorry, but I will not accept this patch. > > The only purpose of this initialization with a non-zero value is to > paper over a problem. As a result, the problem will be left unsolved, > so it bites us again elsewhere, and we increase the memory footprint > of the U-Boot image without need. > > NAK. Note that initialization should be unnecessary if the static variable is int rather than u8. > Best regards, > > Wolfgang Denk Amicalement,
Dear Albert ARIBAUD, In message <4CF7896B.5090007@free.fr> you wrote: > > Note that initialization should be unnecessary if the static variable is > int rather than u8. It should ALWAYS be not necessary. Otherwise we have a bug, and that bug needs to be fixed rather than papered over. Best regards, Wolfgang Denk
Le 02/12/2010 13:01, Wolfgang Denk a écrit : > Dear Albert ARIBAUD, > > In message<4CF7896B.5090007@free.fr> you wrote: >> >> Note that initialization should be unnecessary if the static variable is >> int rather than u8. > > It should ALWAYS be not necessary. I understand your point re: the linker warning, i.e. initializing should never be done to just get rid of a warning. > Otherwise we have a bug, and that bug needs to be fixed rather than > papered over. Yes, there is a bug whereby an u8 BSS variable causes a linker warning, and I believe this bug to be with the linker -- I'm working on getting a minimal example of it so that I could completely verify that the warning does not affect the semantics of the code generated. Now, on an unrelated note, omap3_emv's code arbitrarily uses an u8 where an int (or enum) would be more appropriate, and this should be changed not because it removes a linker warning, but because the u8 choice is arbitrary and at best as effective as using an int, at worst suboptimal to that. > Best regards, > > Wolfgang Denk Amicalement,
> -----Original Message----- > From: Wolfgang Denk [mailto:wd@denx.de] > Sent: Thursday, December 02, 2010 5:07 PM > To: Premi, Sanjeev > Cc: u-boot@lists.denx.de > Subject: Re: [U-Boot] [PATCH] omap3evm: Clean-up EVM detection code. > > Dear Sanjeev Premi, > > In message <1291288812-12653-1-git-send-email-premi@ti.com> you wrote: > > This patch does following changes: > > * Change the type (u8 -> int) for omap3_evm_version. > > * Introduce an 'undefined' board revision for init > > value. > > * Use of #define instead of magic numbers > > > > Signed-off-by: Sanjeev Premi <premi@ti.com> > > --- > > board/ti/evm/evm.c | 39 +++++++++++++++++++++++---------------- > > board/ti/evm/evm.h | 17 +++++++---------- > > 2 files changed, 30 insertions(+), 26 deletions(-) > > > > diff --git a/board/ti/evm/evm.c b/board/ti/evm/evm.c > > index 09d14f7..8d9ce73 100644 > > --- a/board/ti/evm/evm.c > > +++ b/board/ti/evm/evm.c > > @@ -37,36 +37,43 @@ > > #include <asm/mach-types.h> > > #include "evm.h" > > > > -static u8 omap3_evm_version; > > +static int omap3_evm_version = OMAP3EVM_BOARD_UNDEF; > ... > > +#define OMAP3EVM_BOARD_UNDEF -1 /* EVM revision > not detected */ > > Sorry, but I will not accept this patch. > > The only purpose of this initialization with a non-zero value is to > paper over a problem. As a result, the problem will be left unsolved, > so it bites us again elsewhere, and we increase the memory footprint > of the U-Boot image without need. At least I haven't deserted the problem; but the patch will help some one to test beyond the basic boot and see if there is any other impact on the functionality. The board has been broken for many weeks. As we see problem with sort($LIBS); there could be more issues that are yet to be discovered. ~sanjeev > > NAK. > > 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 > You get a wonderful view from the point of no return. > - Terry Pratchett, _Making_Money_ >
Dear Albert ARIBAUD, In message <4CF7922B.3020504@free.fr> you wrote: > > Now, on an unrelated note, omap3_emv's code arbitrarily uses an u8 where > an int (or enum) would be more appropriate, and this should be changed > not because it removes a linker warning, but because the u8 choice is > arbitrary and at best as effective as using an int, at worst suboptimal > to that. Well, an u8 is as good a data type as any other. The available range of 0...255 seems more than sufficient to store the needed information, so why should I waste 4 bytes of storage when a single byte is sufficient as well? Best regards, Wolfgang Denk
Le 02/12/2010 14:58, Wolfgang Denk a écrit : > Dear Albert ARIBAUD, > > In message<4CF7922B.3020504@free.fr> you wrote: >> >> Now, on an unrelated note, omap3_emv's code arbitrarily uses an u8 where >> an int (or enum) would be more appropriate, and this should be changed >> not because it removes a linker warning, but because the u8 choice is >> arbitrary and at best as effective as using an int, at worst suboptimal >> to that. > > Well, an u8 is as good a data type as any other. The available range > of 0...255 seems more than sufficient to store the needed > information, so why should I waste 4 bytes of storage when a single > byte is sufficient as well? You don't necessarily use only one byte when declaring an u8 instead of an int, because the next declaration may have alignment requirements that will cause the compiler to skip bytes after the u8. Besides, u8 is not "as good a data type" as any other, it is a specific data type whereas 'int' is the native data type of the platform, supposed to be the most natural to deal with for the cpu -- 32-bit for an ARM. u8 are perfect and normal, for instance, as fields of a structure which represents byte registers, or to perform 8-bit arithmetic. Here, however, there is indeed no reason to use any specific type, so we should use the cpu's native type. > Best regards, > > Wolfgang Denk Amicalement,
Dear Albert ARIBAUD, In message <4CF7C7F4.6030803@free.fr> you wrote: > > > Well, an u8 is as good a data type as any other. The available range > > of 0...255 seems more than sufficient to store the needed > > information, so why should I waste 4 bytes of storage when a single > > byte is sufficient as well? > > You don't necessarily use only one byte when declaring an u8 instead of > an int, because the next declaration may have alignment requirements > that will cause the compiler to skip bytes after the u8. Besides, u8 is The compiler / linker may (or may not) optimize this and collect variables of similar alignment. An "int foo;" is likely to end in .bss segment, while an "char foo;" will probably show up in .sbss - I don;t know how good or bad the current situation for ARM is, but I'm sure it is improving (look for example at all the microoptimizations done by Linaro). > not "as good a data type" as any other, it is a specific data type > whereas 'int' is the native data type of the platform, supposed to be > the most natural to deal with for the cpu -- 32-bit for an ARM. Can an ARM CPU not read1s and write single bytes, too? > u8 are perfect and normal, for instance, as fields of a structure which > represents byte registers, or to perform 8-bit arithmetic. Here, > however, there is indeed no reason to use any specific type, so we > should use the cpu's native type. I do not share your opinion. But this is a pretty academic topic, and I'm neither in the mood nor do I have the time for lengthy discussions. Let's stop this here. Best regards, Wolfgang Denk
Le 02/12/2010 19:51, Wolfgang Denk a écrit : > Dear Albert ARIBAUD, > > In message<4CF7C7F4.6030803@free.fr> you wrote: >> >>> Well, an u8 is as good a data type as any other. The available range >>> of 0...255 seems more than sufficient to store the needed >>> information, so why should I waste 4 bytes of storage when a single >>> byte is sufficient as well? >> >> You don't necessarily use only one byte when declaring an u8 instead of >> an int, because the next declaration may have alignment requirements >> that will cause the compiler to skip bytes after the u8. Besides, u8 is > > The compiler / linker may (or may not) optimize this and collect > variables of similar alignment. An "int foo;" is likely to end in > .bss segment, while an "char foo;" will probably show up in .sbss - I > don;t know how good or bad the current situation for ARM is, but I'm > sure it is improving (look for example at all the microoptimizations > done by Linaro). There is only a single .bss for ARM. >> not "as good a data type" as any other, it is a specific data type >> whereas 'int' is the native data type of the platform, supposed to be >> the most natural to deal with for the cpu -- 32-bit for an ARM. > > Can an ARM CPU not read1s and write single bytes, too? It can, but for many of its operations, it can only work with 32-bit data. > Let's stop this here. Understood. > Best regards, > > Wolfgang Denk Amicalement,
diff --git a/board/ti/evm/evm.c b/board/ti/evm/evm.c index 09d14f7..8d9ce73 100644 --- a/board/ti/evm/evm.c +++ b/board/ti/evm/evm.c @@ -37,36 +37,43 @@ #include <asm/mach-types.h> #include "evm.h" -static u8 omap3_evm_version; +static int omap3_evm_version = OMAP3EVM_BOARD_UNDEF; -u8 get_omap3_evm_rev(void) +int get_omap3_evm_rev(void) { return omap3_evm_version; } +/** + * The board revision can be ascertained only by identifying the + * Ethernet chipset used on the board. + * + * The revision can be read from ID_REV register on the PHY. + */ static void omap3_evm_get_revision(void) { -#if defined(CONFIG_CMD_NET) - /* - * Board revision can be ascertained only by identifying - * the Ethernet chipset. - */ - unsigned int smsc_id; +#define CONFIG_SMC911X_ID_REV (CONFIG_SMC911X_BASE + 0x50) - /* Ethernet PHY ID is stored at ID_REV register */ - smsc_id = readl(CONFIG_SMC911X_BASE + 0x50) & 0xFFFF0000; - printf("Read back SMSC id 0x%x\n", smsc_id); +#define SMSC_ID_9115 0x01150000 /* SMSC9115 chipset */ +#define SMSC_ID_9220 0x92200000 /* SMSC9220 chipset */ + +#if defined(CONFIG_CMD_NET) + unsigned int smsc_id = readl(CONFIG_SMC911X_ID_REV) & 0xFFFF0000; switch (smsc_id) { - /* SMSC9115 chipset */ - case 0x01150000: + case SMSC_ID_9115: omap3_evm_version = OMAP3EVM_BOARD_GEN_1; break; - /* SMSC 9220 chipset */ - case 0x92200000: + case SMSC_ID_9220: + omap3_evm_version = OMAP3EVM_BOARD_GEN_2; + break; default: + printf ("Unknown board revision."); + /* + * Assume the latest revision + */ omap3_evm_version = OMAP3EVM_BOARD_GEN_2; - } + } #else #if defined(CONFIG_STATIC_BOARD_REV) /* diff --git a/board/ti/evm/evm.h b/board/ti/evm/evm.h index a76deb8..de96504 100644 --- a/board/ti/evm/evm.h +++ b/board/ti/evm/evm.h @@ -34,18 +34,15 @@ const omap3_sysinfo sysinfo = { }; /* - * OMAP35x EVM revision - * Run time detection of EVM revision is done by reading Ethernet - * PHY ID - - * GEN_1 = 0x01150000 - * GEN_2 = 0x92200000 + * OMAP35x EVM Revision. + * The revision can be detected only based on Ethernet PHY ID. */ -enum { - OMAP3EVM_BOARD_GEN_1 = 0, /* EVM Rev between A - D */ - OMAP3EVM_BOARD_GEN_2, /* EVM Rev >= Rev E */ -}; +#define OMAP3EVM_BOARD_UNDEF -1 /* EVM revision not detected */ + +#define OMAP3EVM_BOARD_GEN_1 1 /* EVM Rev between A - D */ +#define OMAP3EVM_BOARD_GEN_2 2 /* EVM Rev >= Rev E */ -u8 get_omap3_evm_rev(void); +int get_omap3_evm_rev(void); #if defined(CONFIG_CMD_NET) static void setup_net_chip(void);
This patch does following changes: * Change the type (u8 -> int) for omap3_evm_version. * Introduce an 'undefined' board revision for init value. * Use of #define instead of magic numbers Signed-off-by: Sanjeev Premi <premi@ti.com> --- board/ti/evm/evm.c | 39 +++++++++++++++++++++++---------------- board/ti/evm/evm.h | 17 +++++++---------- 2 files changed, 30 insertions(+), 26 deletions(-)