Message ID | 1325764937-7342-1-git-send-email-grinberg@compulab.co.il |
---|---|
State | Changes Requested |
Headers | show |
Dear Igor Grinberg, In message <1325764937-7342-1-git-send-email-grinberg@compulab.co.il> you wrote: > From: Nikita Kiryanov <nikita@compulab.co.il> > > Add board specific EEPROM handling module, > read the serial number from the EEPROM and pass it to Linux. ... > * Fix strange linker warning: ".bss section overlaps previous sections" > by changing the type of the eeprom_layout static global variable to int > (probably this is a compiler bug). Probably it is now. Did you inspect the linke rmap? > +static int cm_t3x_eeprom_read(uint offset, uchar *buf, int len) > +{ > + return i2c_read(CONFIG_SYS_I2C_EEPROM_ADDR, offset, > + CONFIG_SYS_I2C_EEPROM_ADDR_LEN, buf, len); > +} Is there any specific reason why you cannot use the standard eeprom_read() function here? > --- /dev/null > +++ b/board/cm_t35/eeprom.h ... > +#ifndef CONFIG_DRIVER_OMAP34XX_I2C > +void get_board_serial(struct tag_serialnr *serialnr) > +{ > + /* > + * This corresponds to what happens when we can communicate with the > + * eeprom but don't get a valid board serial value. > + */ > + serialnr->low = 0; > + serialnr->high = 0; > +}; > +#endif /* CONFIG_DRIVER_OMAP34XX_I2C */ Please do NOT place code into header files. Best regards, Wolfgang Denk
On 01/05/2012 04:56 PM, Wolfgang Denk wrote: > Dear Igor Grinberg, > > In message<1325764937-7342-1-git-send-email-grinberg@compulab.co.il> you wrote: >> From: Nikita Kiryanov<nikita@compulab.co.il> >> >> Add board specific EEPROM handling module, >> read the serial number from the EEPROM and pass it to Linux. > ... > >> * Fix strange linker warning: ".bss section overlaps previous sections" >> by changing the type of the eeprom_layout static global variable to int >> (probably this is a compiler bug). > Probably it is now. Did you inspect the linke rmap? u-boot.map shows the bss section aligning perfectly with the start of rel.dyn. The difference between the original "working" version and the version with the warning was an additional byte added by uchar eeprom_layout to the size of libcm_t35.o. This shouldn't be a problem because the bss section is followed by an ALIGN(4), but we decided to try changing eeprom_layout to an int and the problem went away. When we tried to define 4 uchars the problem reappeared. This suggests that this might be a compiler bug. There's been some discussion about this in the following threads: http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/114646 http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/90723 and we're not aware of any fix to the issue. >> +static int cm_t3x_eeprom_read(uint offset, uchar *buf, int len) >> +{ >> + return i2c_read(CONFIG_SYS_I2C_EEPROM_ADDR, offset, >> + CONFIG_SYS_I2C_EEPROM_ADDR_LEN, buf, len); >> +} > Is there any specific reason why you cannot use the standard > eeprom_read() function here? The inclusion of the entire eeprom library for this one function will just increase the image size needlessly. Moreover we don't want to add eeprom commands, because the eeprom on this module is for internal use only. >> --- /dev/null >> +++ b/board/cm_t35/eeprom.h > ... >> +#ifndef CONFIG_DRIVER_OMAP34XX_I2C >> +void get_board_serial(struct tag_serialnr *serialnr) >> +{ >> + /* >> + * This corresponds to what happens when we can communicate with the >> + * eeprom but don't get a valid board serial value. >> + */ >> + serialnr->low = 0; >> + serialnr->high = 0; >> +}; >> +#endif /* CONFIG_DRIVER_OMAP34XX_I2C */ > Please do NOT place code into header files. Will be fixed in the next version. > Best regards, > > Wolfgang Denk >
Le 09/01/2012 09:30, Nikita Kiryanov a écrit : > On 01/05/2012 04:56 PM, Wolfgang Denk wrote: >> Dear Igor Grinberg, >> >> In message<1325764937-7342-1-git-send-email-grinberg@compulab.co.il> >> you wrote: >>> From: Nikita Kiryanov<nikita@compulab.co.il> >>> >>> Add board specific EEPROM handling module, >>> read the serial number from the EEPROM and pass it to Linux. >> ... >> >>> * Fix strange linker warning: ".bss section overlaps previous sections" >>> by changing the type of the eeprom_layout static global variable to int >>> (probably this is a compiler bug). >> Probably it is now. Did you inspect the linke rmap? > > u-boot.map shows the bss section aligning perfectly with the start of > rel.dyn. > > The difference between the original "working" version and the version > with the warning > was an additional byte added by uchar eeprom_layout to the size of > libcm_t35.o. > This shouldn't be a problem because the bss section is followed by an > ALIGN(4), but > we decided to try changing eeprom_layout to an int and the problem went > away. > When we tried to define 4 uchars the problem reappeared. > > This suggests that this might be a compiler bug. > > There's been some discussion about this in the following threads: > http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/114646 > http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/90723 > and we're not aware of any fix to the issue. Which prompted me to test --no-check-sections with CS 2009q1. Adding it to LDFLAGS_u-boot does reduce the annoyance from errors to a warning, but there is no way to completely make it disappear. Amicalement,
Hi Albert, On 02/07/12 00:56, Albert ARIBAUD wrote: > Le 09/01/2012 09:30, Nikita Kiryanov a écrit : >> On 01/05/2012 04:56 PM, Wolfgang Denk wrote: >>> Dear Igor Grinberg, >>> >>> In message<1325764937-7342-1-git-send-email-grinberg@compulab.co.il> >>> you wrote: >>>> From: Nikita Kiryanov<nikita@compulab.co.il> >>>> >>>> Add board specific EEPROM handling module, >>>> read the serial number from the EEPROM and pass it to Linux. >>> ... >>> >>>> * Fix strange linker warning: ".bss section overlaps previous sections" >>>> by changing the type of the eeprom_layout static global variable to int >>>> (probably this is a compiler bug). >>> Probably it is now. Did you inspect the linke rmap? >> >> u-boot.map shows the bss section aligning perfectly with the start of >> rel.dyn. >> >> The difference between the original "working" version and the version >> with the warning >> was an additional byte added by uchar eeprom_layout to the size of >> libcm_t35.o. >> This shouldn't be a problem because the bss section is followed by an >> ALIGN(4), but >> we decided to try changing eeprom_layout to an int and the problem went >> away. >> When we tried to define 4 uchars the problem reappeared. >> >> This suggests that this might be a compiler bug. >> >> There's been some discussion about this in the following threads: >> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/114646 >> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/90723 >> and we're not aware of any fix to the issue. > > Which prompted me to test --no-check-sections with CS 2009q1. > Adding it to LDFLAGS_u-boot does reduce the annoyance from errors to a warning, > but there is no way to completely make it disappear. So the conclusion is still a tool chain bug, right? Probably it doesn't hurt besides the annoying warning.
Hi Igor, Le 07/02/2012 09:01, Igor Grinberg a écrit : > Hi Albert, > > On 02/07/12 00:56, Albert ARIBAUD wrote: >> Le 09/01/2012 09:30, Nikita Kiryanov a écrit : >>> On 01/05/2012 04:56 PM, Wolfgang Denk wrote: >>>> Dear Igor Grinberg, >>>> >>>> In message<1325764937-7342-1-git-send-email-grinberg@compulab.co.il> >>>> you wrote: >>>>> From: Nikita Kiryanov<nikita@compulab.co.il> >>>>> >>>>> Add board specific EEPROM handling module, >>>>> read the serial number from the EEPROM and pass it to Linux. >>>> ... >>>> >>>>> * Fix strange linker warning: ".bss section overlaps previous sections" >>>>> by changing the type of the eeprom_layout static global variable to int >>>>> (probably this is a compiler bug). >>>> Probably it is now. Did you inspect the linke rmap? >>> >>> u-boot.map shows the bss section aligning perfectly with the start of >>> rel.dyn. >>> >>> The difference between the original "working" version and the version >>> with the warning >>> was an additional byte added by uchar eeprom_layout to the size of >>> libcm_t35.o. >>> This shouldn't be a problem because the bss section is followed by an >>> ALIGN(4), but >>> we decided to try changing eeprom_layout to an int and the problem went >>> away. >>> When we tried to define 4 uchars the problem reappeared. >>> >>> This suggests that this might be a compiler bug. >>> >>> There's been some discussion about this in the following threads: >>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/114646 >>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/90723 >>> and we're not aware of any fix to the issue. >> >> Which prompted me to test --no-check-sections with CS 2009q1. >> Adding it to LDFLAGS_u-boot does reduce the annoyance from errors to a warning, >> but there is no way to completely make it disappear. > > So the conclusion is still a tool chain bug, right? > Probably it doesn't hurt besides the annoying warning. Sorry for being slow. Yes, it seems to be a toolchain bug and limited to a spurious warning. However, spurious warnings are always a pain when doing a MAKEALL arm, because the board is listed among the unclean and failed builds and then one must go through these individually and remember that this specific warning is both unimportant and unavoidable. Amicalement,
On 02/17/2012 02:50 AM, Albert ARIBAUD wrote: > Hi Igor, > > Le 07/02/2012 09:01, Igor Grinberg a écrit : >> Hi Albert, >> >> On 02/07/12 00:56, Albert ARIBAUD wrote: >>> Le 09/01/2012 09:30, Nikita Kiryanov a écrit : >>>> On 01/05/2012 04:56 PM, Wolfgang Denk wrote: >>>>> Dear Igor Grinberg, >>>>> >>>>> In message<1325764937-7342-1-git-send-email-grinberg@compulab.co.il> >>>>> you wrote: >>>>>> From: Nikita Kiryanov<nikita@compulab.co.il> >>>>>> >>>>>> Add board specific EEPROM handling module, >>>>>> read the serial number from the EEPROM and pass it to Linux. >>>>> ... >>>>> >>>>>> * Fix strange linker warning: ".bss section overlaps previous >>>>>> sections" >>>>>> by changing the type of the eeprom_layout static global variable >>>>>> to int >>>>>> (probably this is a compiler bug). >>>>> Probably it is now. Did you inspect the linke rmap? >>>> >>>> u-boot.map shows the bss section aligning perfectly with the start of >>>> rel.dyn. >>>> >>>> The difference between the original "working" version and the version >>>> with the warning >>>> was an additional byte added by uchar eeprom_layout to the size of >>>> libcm_t35.o. >>>> This shouldn't be a problem because the bss section is followed by an >>>> ALIGN(4), but >>>> we decided to try changing eeprom_layout to an int and the problem went >>>> away. >>>> When we tried to define 4 uchars the problem reappeared. >>>> >>>> This suggests that this might be a compiler bug. >>>> >>>> There's been some discussion about this in the following threads: >>>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/114646 >>>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/90723 >>>> and we're not aware of any fix to the issue. >>> >>> Which prompted me to test --no-check-sections with CS 2009q1. >>> Adding it to LDFLAGS_u-boot does reduce the annoyance from errors to >>> a warning, >>> but there is no way to completely make it disappear. >> >> So the conclusion is still a tool chain bug, right? >> Probably it doesn't hurt besides the annoying warning. > > Sorry for being slow. > > Yes, it seems to be a toolchain bug and limited to a spurious warning. > However, spurious warnings are always a pain when doing a MAKEALL arm, > because the board is listed among the unclean and failed builds and then > one must go through these individually and remember that this specific > warning is both unimportant and unavoidable. Have we gone and bugged any toolchain folks about what caused / fixed this problem so that perhaps we can option around it or otherwise squash this?
Le 17/02/2012 18:10, Tom Rini a écrit : > On 02/17/2012 02:50 AM, Albert ARIBAUD wrote: >> Hi Igor, >> >> Le 07/02/2012 09:01, Igor Grinberg a écrit : >>> Hi Albert, >>> >>> On 02/07/12 00:56, Albert ARIBAUD wrote: >>>> Le 09/01/2012 09:30, Nikita Kiryanov a écrit : >>>>> On 01/05/2012 04:56 PM, Wolfgang Denk wrote: >>>>>> Dear Igor Grinberg, >>>>>> >>>>>> In message<1325764937-7342-1-git-send-email-grinberg@compulab.co.il> >>>>>> you wrote: >>>>>>> From: Nikita Kiryanov<nikita@compulab.co.il> >>>>>>> >>>>>>> Add board specific EEPROM handling module, >>>>>>> read the serial number from the EEPROM and pass it to Linux. >>>>>> ... >>>>>> >>>>>>> * Fix strange linker warning: ".bss section overlaps previous >>>>>>> sections" >>>>>>> by changing the type of the eeprom_layout static global variable >>>>>>> to int >>>>>>> (probably this is a compiler bug). >>>>>> Probably it is now. Did you inspect the linke rmap? >>>>> >>>>> u-boot.map shows the bss section aligning perfectly with the start of >>>>> rel.dyn. >>>>> >>>>> The difference between the original "working" version and the version >>>>> with the warning >>>>> was an additional byte added by uchar eeprom_layout to the size of >>>>> libcm_t35.o. >>>>> This shouldn't be a problem because the bss section is followed by an >>>>> ALIGN(4), but >>>>> we decided to try changing eeprom_layout to an int and the problem >>>>> went >>>>> away. >>>>> When we tried to define 4 uchars the problem reappeared. >>>>> >>>>> This suggests that this might be a compiler bug. >>>>> >>>>> There's been some discussion about this in the following threads: >>>>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/114646 >>>>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/90723 >>>>> and we're not aware of any fix to the issue. >>>> >>>> Which prompted me to test --no-check-sections with CS 2009q1. >>>> Adding it to LDFLAGS_u-boot does reduce the annoyance from errors to >>>> a warning, >>>> but there is no way to completely make it disappear. >>> >>> So the conclusion is still a tool chain bug, right? >>> Probably it doesn't hurt besides the annoying warning. >> >> Sorry for being slow. >> >> Yes, it seems to be a toolchain bug and limited to a spurious warning. >> However, spurious warnings are always a pain when doing a MAKEALL arm, >> because the board is listed among the unclean and failed builds and then >> one must go through these individually and remember that this specific >> warning is both unimportant and unavoidable. > > Have we gone and bugged any toolchain folks about what caused / fixed > this problem so that perhaps we can option around it or otherwise squash > this? I haven't, as I work mostly with ELDK42 and occasionally recent (2010 and later) CS and current Ubuntu-provided Linaro toolchains, and don't see the warning -- and I don't think toolchain providers will bother about warnings from old releases that do not still exist in current ones, just like we don't bother with issues present only in older version of U-Boot. But if somebody on the list is using the current version of some toolchain that does emit this warning, then yes, they should indeed bring the issue to the toolchain provider, or at least raise their hand here. Amicalement,
On 02/17/12 11:50, Albert ARIBAUD wrote: > Hi Igor, > > Le 07/02/2012 09:01, Igor Grinberg a écrit : >> Hi Albert, >> >> On 02/07/12 00:56, Albert ARIBAUD wrote: >>> Le 09/01/2012 09:30, Nikita Kiryanov a écrit : >>>> On 01/05/2012 04:56 PM, Wolfgang Denk wrote: >>>>> Dear Igor Grinberg, >>>>> >>>>> In message<1325764937-7342-1-git-send-email-grinberg@compulab.co.il> >>>>> you wrote: >>>>>> From: Nikita Kiryanov<nikita@compulab.co.il> >>>>>> >>>>>> Add board specific EEPROM handling module, >>>>>> read the serial number from the EEPROM and pass it to Linux. >>>>> ... >>>>> >>>>>> * Fix strange linker warning: ".bss section overlaps previous sections" >>>>>> by changing the type of the eeprom_layout static global variable to int >>>>>> (probably this is a compiler bug). >>>>> Probably it is now. Did you inspect the linke rmap? >>>> >>>> u-boot.map shows the bss section aligning perfectly with the start of >>>> rel.dyn. >>>> >>>> The difference between the original "working" version and the version >>>> with the warning >>>> was an additional byte added by uchar eeprom_layout to the size of >>>> libcm_t35.o. >>>> This shouldn't be a problem because the bss section is followed by an >>>> ALIGN(4), but >>>> we decided to try changing eeprom_layout to an int and the problem went >>>> away. >>>> When we tried to define 4 uchars the problem reappeared. >>>> >>>> This suggests that this might be a compiler bug. >>>> >>>> There's been some discussion about this in the following threads: >>>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/114646 >>>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/90723 >>>> and we're not aware of any fix to the issue. >>> >>> Which prompted me to test --no-check-sections with CS 2009q1. >>> Adding it to LDFLAGS_u-boot does reduce the annoyance from errors to a warning, >>> but there is no way to completely make it disappear. >> >> So the conclusion is still a tool chain bug, right? >> Probably it doesn't hurt besides the annoying warning. > > Sorry for being slow. This is fine, I was also out for a week or so... > > Yes, it seems to be a toolchain bug and limited to a spurious warning. However, spurious warnings are always a pain when doing a MAKEALL arm, because the board is listed among the unclean and failed builds and then one must go through these individually and remember that this specific warning is both unimportant and unavoidable. That is exactly the reason why we tried (and succeeded) to workaround the issue...
diff --git a/board/cm_t35/Makefile b/board/cm_t35/Makefile index 27693f0..894fa09 100644 --- a/board/cm_t35/Makefile +++ b/board/cm_t35/Makefile @@ -25,7 +25,9 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(BOARD).o -COBJS := cm_t35.o leds.o +COBJS-$(CONFIG_DRIVER_OMAP34XX_I2C) += eeprom.o + +COBJS := cm_t35.o leds.o $(COBJS-y) SRCS := $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(COBJS)) diff --git a/board/cm_t35/eeprom.c b/board/cm_t35/eeprom.c new file mode 100644 index 0000000..93ed6cb --- /dev/null +++ b/board/cm_t35/eeprom.c @@ -0,0 +1,78 @@ +/* + * (C) Copyright 2011 CompuLab, Ltd. <www.compulab.co.il> + * + * Authors: Nikita Kiryanov <nikita@compulab.co.il> + * Igor Grinberg <grinberg@compulab.co.il> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc. + */ + +#include <common.h> +#include <i2c.h> + +#define EEPROM_LAYOUT_VER_OFFSET 44 +#define BOARD_SERIAL_OFFSET 20 +#define BOARD_SERIAL_OFFSET_LEGACY 8 + +#define LAYOUT_INVALID 0 +#define LAYOUT_LEGACY 0xff + +static int eeprom_layout; /* Implicitly LAYOUT_INVALID */ + +static int cm_t3x_eeprom_read(uint offset, uchar *buf, int len) +{ + return i2c_read(CONFIG_SYS_I2C_EEPROM_ADDR, offset, + CONFIG_SYS_I2C_EEPROM_ADDR_LEN, buf, len); +} + +static int eeprom_setup_layout(void) +{ + int res; + + if (eeprom_layout != LAYOUT_INVALID) + return 0; + + res = cm_t3x_eeprom_read(EEPROM_LAYOUT_VER_OFFSET, + (uchar *)&eeprom_layout, 1); + if (res) { + eeprom_layout = LAYOUT_INVALID; + return res; + } + + if (eeprom_layout == 0 || eeprom_layout >= 0x20) + eeprom_layout = LAYOUT_LEGACY; + + return 0; +} + +void get_board_serial(struct tag_serialnr *serialnr) +{ + u32 serial[2]; + uint offset; + + memset(serialnr, 0, sizeof(*serialnr)); + if (eeprom_setup_layout()) + return; + + offset = (eeprom_layout != LAYOUT_LEGACY) ? + BOARD_SERIAL_OFFSET : BOARD_SERIAL_OFFSET_LEGACY; + if (cm_t3x_eeprom_read(offset, (uchar *)serial, 8)) + return; + + if (serial[0] != 0xffffffff && serial[1] != 0xffffffff) { + serialnr->low = serial[0]; + serialnr->high = serial[1]; + } +} diff --git a/board/cm_t35/eeprom.h b/board/cm_t35/eeprom.h new file mode 100644 index 0000000..97a002a --- /dev/null +++ b/board/cm_t35/eeprom.h @@ -0,0 +1,38 @@ +/* + * (C) Copyright 2011 CompuLab, Ltd. <www.compulab.co.il> + * + * Authors: Nikita Kiryanov <nikita@compulab.co.il> + * Igor Grinberg <grinberg@compulab.co.il> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc. + */ +#ifndef _EEPROM_ +#define _EEPROM_ + +#include <common.h> + +#ifndef CONFIG_DRIVER_OMAP34XX_I2C +void get_board_serial(struct tag_serialnr *serialnr) +{ + /* + * This corresponds to what happens when we can communicate with the + * eeprom but don't get a valid board serial value. + */ + serialnr->low = 0; + serialnr->high = 0; +}; +#endif /* CONFIG_DRIVER_OMAP34XX_I2C */ + +#endif diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h index 61226d5..fe91c10 100644 --- a/include/configs/cm_t35.h +++ b/include/configs/cm_t35.h @@ -72,6 +72,7 @@ #define CONFIG_SETUP_MEMORY_TAGS #define CONFIG_INITRD_TAG #define CONFIG_REVISION_TAG +#define CONFIG_SERIAL_TAG /* * Size of malloc() pool @@ -153,6 +154,8 @@ #define CONFIG_SYS_I2C_BUS 0 #define CONFIG_SYS_I2C_BUS_SELECT 1 #define CONFIG_DRIVER_OMAP34XX_I2C +#define CONFIG_SYS_I2C_EEPROM_ADDR 0x50 +#define CONFIG_SYS_I2C_EEPROM_ADDR_LEN 1 /* * TWL4030