[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[Vasili Galka]

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

[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[Vasili Galka]

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

[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[Wolfgang Denk]

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

-- 
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 at denx.de
"I've finally learned what `upward compatible' means. It means we get
to keep all our old mistakes." - Dennie van Tassel

[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[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 at 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

-- 
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 at denx.de
What's the sound a name makes when it's dropped?

[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[Reinhard Meyer]

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 at 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

[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[Wolfgang Denk]

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

-- 
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 at denx.de
Certainly there are things in life that money  can't  buy,  but  it's
very funny - Did you ever try buying them without money? - Ogden Nash

[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[Reinhard Meyer]

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

[U-Boot] [PATCH] Disable FLASH_AMLV256U support for TOP860 target

[Wolfgang Denk]

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

-- 
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 at denx.de
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is a nanocentury.
                                                - Tom Duff, Bell Labs