Message ID | 1402929691-3972-1-git-send-email-vvv444@gmail.com |
---|---|
State | Deferred |
Delegated to: | Wolfgang Denk |
Headers | show |
On Mon, Jun 16, 2014 at 5:41 PM, Vasili Galka <vvv444@gmail.com> wrote: > TOP860 configuration assumes at most 128 flash sectors. Thus, the > AMLV256U flash can't be supported. The existing code could result in > memory corruption when writing to the flash_info->start[] array. > > Signed-off-by: Vasili Galka <vvv444@gmail.com> > Cc: Wolfgang Denk <wd@denx.de> > --- > board/emk/common/flash.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/board/emk/common/flash.c b/board/emk/common/flash.c > index ae5777c..4119b3b 100644 > --- a/board/emk/common/flash.c > +++ b/board/emk/common/flash.c > @@ -324,6 +324,7 @@ ulong flash_get_size (FPWV *addr, flash_info_t *info) > } > break; > } > +#ifndef CONFIG_TOP860 > if ((FPW)addr[FLASH_ID3] == (FPW)AMD_ID_LV256U_2 && > (FPW)addr[FLASH_ID4] == (FPW)AMD_ID_LV256U_3) > { > @@ -337,7 +338,8 @@ ulong flash_get_size (FPWV *addr, flash_info_t *info) > } > break; > } > - > +#endif > + > /* fall thru to here ! */ > default: > printf ("unknown AMD device=%x %x %x", > -- > 1.7.9 > > Any review? This was inspired by a a compiler warning. I'm still getting this warning on the latest master. Best, Vasili
Dear Vasili, In message <CA+gZxsOYLBU18LimMmfP9B-gZaykN=hZtM1FVgpd8p-eEW19bQ@mail.gmail.com> you wrote: > > > TOP860 configuration assumes at most 128 flash sectors. Thus, the > > AMLV256U flash can't be supported. The existing code could result in > > memory corruption when writing to the flash_info->start[] array. > > > > Signed-off-by: Vasili Galka <vvv444@gmail.com> > > Cc: Wolfgang Denk <wd@denx.de> > > --- > > board/emk/common/flash.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/board/emk/common/flash.c b/board/emk/common/flash.c > > index ae5777c..4119b3b 100644 > > --- a/board/emk/common/flash.c > > +++ b/board/emk/common/flash.c > > @@ -324,6 +324,7 @@ ulong flash_get_size (FPWV *addr, flash_info_t *info) > > } > > break; > > } > > +#ifndef CONFIG_TOP860 > > if ((FPW)addr[FLASH_ID3] == (FPW)AMD_ID_LV256U_2 && > > (FPW)addr[FLASH_ID4] == (FPW)AMD_ID_LV256U_3) > > { > > @@ -337,7 +338,8 @@ ulong flash_get_size (FPWV *addr, flash_info_t *info) > > } > > break; > > } > > - > > +#endif > > + > > /* fall thru to here ! */ > > default: > > printf ("unknown AMD device=%x %x %x", > > -- > > 1.7.9 > > > > > Any review? > This was inspired by a a compiler warning. I'm still getting this warning > on the latest master. Sorry, I missed that one. Would it not be more appropriate to adjust the CONFIG_SYS_MAX_FLASH_SECT setting in "include/configs/TOP860.h"? Or are you 100% sure that there were never be any AMLV256U flash chips fit on a TOP860 board? Best regards, Wolfgang Denk
Dear Reinhard, In message <CA+gZxsPKtwYAhHm88fBtGW17cxT7ncEz9OBxwpqiJ55WR-kfcg@mail.gmail.com> Vasili Galka wrote: > > You're right, that would probably be a better solution. Although I'm not a > user of TOP860 board so I'm not really the right person to ask... > I just found this bug theoretically from looking on compiler warnings and > suggested a possible solution. > > Best, > Vasili > > On Tue, Oct 28, 2014 at 11:33 AM, Wolfgang Denk <wd@denx.de> wrote: > > > Dear Vasili, > > > > In message <CA+gZxsOYLBU18LimMmfP9B-gZaykN= > > hZtM1FVgpd8p-eEW19bQ@mail.gmail.com> you wrote: > > > > > > > TOP860 configuration assumes at most 128 flash sectors. Thus, the > > > > AMLV256U flash can't be supported. The existing code could result in > > > > memory corruption when writing to the flash_info->start[] array. > > > > > > > > Signed-off-by: Vasili Galka <vvv444@gmail.com> > > > > Cc: Wolfgang Denk <wd@denx.de> > > > > --- > > > > board/emk/common/flash.c | 4 +++- > > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/board/emk/common/flash.c b/board/emk/common/flash.c > > > > index ae5777c..4119b3b 100644 > > > > --- a/board/emk/common/flash.c > > > > +++ b/board/emk/common/flash.c > > > > @@ -324,6 +324,7 @@ ulong flash_get_size (FPWV *addr, flash_info_t > > *info) > > > > } > > > > break; > > > > } > > > > +#ifndef CONFIG_TOP860 > > > > if ((FPW)addr[FLASH_ID3] == (FPW)AMD_ID_LV256U_2 && > > > > (FPW)addr[FLASH_ID4] == (FPW)AMD_ID_LV256U_3) > > > > { > > > > @@ -337,7 +338,8 @@ ulong flash_get_size (FPWV *addr, flash_info_t > > *info) > > > > } > > > > break; > > > > } > > > > - > > > > +#endif > > > > + > > > > /* fall thru to here ! */ > > > > default: > > > > printf ("unknown AMD device=%x %x %x", > > > > -- > > > > 1.7.9 > > > > > > > > > > > Any review? > > > This was inspired by a a compiler warning. I'm still getting this warning > > > on the latest master. > > > > Sorry, I missed that one. > > > > Would it not be more appropriate to adjust the CONFIG_SYS_MAX_FLASH_SECT > > setting in "include/configs/TOP860.h"? Or are you 100% sure that > > there were never be any AMLV256U flash chips fit on a TOP860 board? Maybe you can comment? Or is the TOP860 board so obsolete that we can remove it alltogether? What about the other boards in board/emk ? I don't see any real changes there during the last 5 years or so? Are these still actively maintained? Best regards, Wolfgang Denk
Am 28.10.2014 10:48, schrieb Wolfgang Denk: > Dear Reinhard, > > In message <CA+gZxsPKtwYAhHm88fBtGW17cxT7ncEz9OBxwpqiJ55WR-kfcg@mail.gmail.com> Vasili Galka wrote: >> You're right, that would probably be a better solution. Although I'm not a >> user of TOP860 board so I'm not really the right person to ask... >> I just found this bug theoretically from looking on compiler warnings and >> suggested a possible solution. >> >> Best, >> Vasili >> >> On Tue, Oct 28, 2014 at 11:33 AM, Wolfgang Denk <wd@denx.de> wrote: >> >>> Dear Vasili, >>> >>> In message <CA+gZxsOYLBU18LimMmfP9B-gZaykN= >>> hZtM1FVgpd8p-eEW19bQ@mail.gmail.com> you wrote: >>>>> TOP860 configuration assumes at most 128 flash sectors. Thus, the >>>>> AMLV256U flash can't be supported. The existing code could result in >>>>> memory corruption when writing to the flash_info->start[] array. >>>>> >>>>> Signed-off-by: Vasili Galka <vvv444@gmail.com> >>>>> Cc: Wolfgang Denk <wd@denx.de> >>>>> --- >>>>> board/emk/common/flash.c | 4 +++- >>>>> 1 files changed, 3 insertions(+), 1 deletions(-) >>>>> >>>>> diff --git a/board/emk/common/flash.c b/board/emk/common/flash.c >>>>> index ae5777c..4119b3b 100644 >>>>> --- a/board/emk/common/flash.c >>>>> +++ b/board/emk/common/flash.c >>>>> @@ -324,6 +324,7 @@ ulong flash_get_size (FPWV *addr, flash_info_t >>> *info) >>>>> } >>>>> break; >>>>> } >>>>> +#ifndef CONFIG_TOP860 >>>>> if ((FPW)addr[FLASH_ID3] == (FPW)AMD_ID_LV256U_2 && >>>>> (FPW)addr[FLASH_ID4] == (FPW)AMD_ID_LV256U_3) >>>>> { >>>>> @@ -337,7 +338,8 @@ ulong flash_get_size (FPWV *addr, flash_info_t >>> *info) >>>>> } >>>>> break; >>>>> } >>>>> - >>>>> +#endif >>>>> + >>>>> /* fall thru to here ! */ >>>>> default: >>>>> printf ("unknown AMD device=%x %x %x", >>>>> -- >>>>> 1.7.9 >>>>> >>>>> >>>> Any review? >>>> This was inspired by a a compiler warning. I'm still getting this warning >>>> on the latest master. >>> Sorry, I missed that one. >>> >>> Would it not be more appropriate to adjust the CONFIG_SYS_MAX_FLASH_SECT >>> setting in "include/configs/TOP860.h"? Or are you 100% sure that >>> there were never be any AMLV256U flash chips fit on a TOP860 board? > > Maybe you can comment? > > Or is the TOP860 board so obsolete that we can remove it alltogether? > > What about the other boards in board/emk ? I don't see any real > changes there during the last 5 years or so? Are these still > actively maintained? > > Best regards, > > Wolfgang Denk > Dear Wolfgang, top860 can be removed (We already had that discussion a while ago.) top5200 is still active in several older projects, but there was no need to make changes to u-boot or to integrate new features of u-boot. Therefore I am not testing whether any changes to u-boot break the function of top5200. top9000 is dead. Thanks atmel :( However it might be left in u-boot as an example. Best regards, Reinhard
Dear Reinhard, In message <544F6AD8.2070605@emk-elektronik.de> you wrote: > > top860 can be removed (We already had that discussion a while ago.) > > top5200 is still active in several older projects, but there was no need > to make changes to u-boot or to integrate new features of u-boot. > Therefore I am not testing whether any changes to u-boot break the > function of top5200. > > top9000 is dead. Thanks atmel :( > However it might be left in u-boot as an example. Hm... if two of the boards are dead, and you are not even testing the third, can we not just remove all of them? That would also allow to get rid of board/emk/common: - am79c874.c is completely bogus anyway - flash.c should have been replaced by the CFI driver years ago - vpd.c is causing efforts when I2C / EEPROM code gets changed What do you think? Best regards, Wolfgang Denk
Dear Wolfgang, > Dear Reinhard, > > In message <544F6AD8.2070605@emk-elektronik.de> you wrote: >> top860 can be removed (We already had that discussion a while ago.) >> >> top5200 is still active in several older projects, but there was no need >> to make changes to u-boot or to integrate new features of u-boot. >> Therefore I am not testing whether any changes to u-boot break the >> function of top5200. >> >> top9000 is dead. Thanks atmel :( >> However it might be left in u-boot as an example. > Hm... if two of the boards are dead, and you are not even testing the > third, can we not just remove all of them? That would also allow to > get rid of board/emk/common: Agreed. > > - am79c874.c is completely bogus anyway How so? > - flash.c should have been replaced by the CFI driver years ago I think there was/is an issue with the fact that top5200 uses GPIO for the uppermost (A16+) address lines AND that FLASH is connected 8 Bit wide. Maybe CFI code does support such by now, at that time it did not. > - vpd.c is causing efforts when I2C / EEPROM code gets changed ideally, the API should not change that often ;) > > What do you think? Since its quite unlikely I will make any changes to those boards in the future, they can be removed. Best Regards, Reinhard
Dear Reinhard, In message <544F7157.9040701@emk-elektronik.de> you wrote: > > > - am79c874.c is completely bogus anyway > How so? Because it contains just a single code line: #include <common.h> That does not generate any executable code... > > - flash.c should have been replaced by the CFI driver years ago > I think there was/is an issue with the fact that top5200 uses GPIO for > the uppermost (A16+) address lines AND that FLASH is connected 8 Bit wide. > Maybe CFI code does support such by now, at that time it did not. > > - vpd.c is causing efforts when I2C / EEPROM code gets changed > ideally, the API should not change that often ;) > > > > What do you think? > Since its quite unlikely I will make any changes to those boards in the > future, they can be removed. Excellent. I'll prepare a patch, then. Thanks. Best regards, Wolfgang Denk
diff --git a/board/emk/common/flash.c b/board/emk/common/flash.c index ae5777c..4119b3b 100644 --- a/board/emk/common/flash.c +++ b/board/emk/common/flash.c @@ -324,6 +324,7 @@ ulong flash_get_size (FPWV *addr, flash_info_t *info) } break; } +#ifndef CONFIG_TOP860 if ((FPW)addr[FLASH_ID3] == (FPW)AMD_ID_LV256U_2 && (FPW)addr[FLASH_ID4] == (FPW)AMD_ID_LV256U_3) { @@ -337,7 +338,8 @@ ulong flash_get_size (FPWV *addr, flash_info_t *info) } break; } - +#endif + /* fall thru to here ! */ default: printf ("unknown AMD device=%x %x %x",
TOP860 configuration assumes at most 128 flash sectors. Thus, the AMLV256U flash can't be supported. The existing code could result in memory corruption when writing to the flash_info->start[] array. Signed-off-by: Vasili Galka <vvv444@gmail.com> Cc: Wolfgang Denk <wd@denx.de> --- board/emk/common/flash.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)