Disable FLASH_AMLV256U support for TOP860 target

PATCH New Vasili Galka 3 participants 8 messages Jun 16, 2014 – Oct 28, 2014 1 message not in the archive

Share & tools

Vasili Galka

author

2014-06-16 14:41 UTC #0 lore raw

PATCH 1/1 Disable FLASH_AMLV256U support for TOP860 target

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 <[email protected]>

Cc Wolfgang Denk <[email protected]>

---
 board/emk/common/flash.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

board/emk/common/flash.c +3 -1

index ae5777c..4119b3b 100644
@@ -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",

Signature · 1 line

1.7.9

Vasili Galka

author

2014-08-12 15:17 UTC #1 lore raw

PATCH 1/1 Disable FLASH_AMLV256U support for TOP860 target

Quoted 33 lines · 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 <[email protected]>

Cc Wolfgang Denk <[email protected]>

---
 board/emk/common/flash.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

board/emk/common/flash.c +3 -2

index ae5777c..4119b3b 100644
@@ -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",
--

Quoted 3 lines

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

Wolfgang Denk

2014-10-28 09:33 UTC #2 lore raw

PATCH 1/1 Disable FLASH_AMLV256U support for TOP860 target

Dear Vasili,

Quoted 32 lines · In message <CA+gZxsOYLBU18LimMmfP9B-gZaykN=hZtM1FVgpd8p-eEW19bQ@mail.gmail.com>…

Quoted 32 lines

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 <[email protected]>

Cc Wolfgang Denk <[email protected]>

---
 board/emk/common/flash.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

board/emk/common/flash.c +3 -1

index ae5777c..4119b3b 100644
@@ -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:

Quoted 8 lines

Quoted 5 lines
board/emk/common/flash.c +0 -1
                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

Signature · 5 lines

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

A message that is not in the archive (sent off-list or not yet received): CA+gZxsPKtwYAhHm88fBtGW17cxT7ncEz9OBxwpqiJ55WR-kfcg@mail.gmail.com

Wolfgang Denk

2014-10-28 09:48 UTC #4 lore raw

PATCH 1/1 Disable FLASH_AMLV256U support for TOP860 target

Dear Reinhard,

Quoted 6 lines · In message <CA+gZxsPKtwYAhHm88fBtGW17cxT7ncEz9OBxwpqiJ55WR-kfcg@mail.gmail.com>…

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.

Quoted 57 lines

Best,
Vasili
Quoted 52 lines · Wolfgang Denk
Dear Vasili,
In message <CA+gZxsOYLBU18LimMmfP9B-gZaykN=
Quoted 11 lines · hZtM1FVgpd8p-eEW19bQ at mail.gmail.com> you wrote:
Quoted 11 lines
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 <[email protected]>
Cc Wolfgang Denk <[email protected]>
---
 board/emk/common/flash.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Quoted 5 lines
board/emk/common/flash.c ulong flash_get_size (FPWV *addr, flash_info_t
Quoted 5 lines
board/emk/common/flash.c +0 -0
index ae5777c..4119b3b 100644
@@ -324,6 +324,7 @@ ulong flash_get_size (FPWV *addr, flash_info_t
*info)
Quoted 8 lines
board/emk/common/flash.c ulong flash_get_size (FPWV *addr, flash_info_t
Quoted 8 lines
board/emk/common/flash.c +1 -0
                        }
                        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)
Quoted 8 lines
Quoted 8 lines
board/emk/common/flash.c +2 -1
                        }
                        break;
                }
-
+#endif
+
                /* fall thru to here ! */
        default:
Quoted 8 lines
Quoted 5 lines
board/emk/common/flash.c +0 -1
                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

Signature · 4 lines

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?

Reinhard Meyer

2014-10-28 10:07 UTC #5 lore raw

PATCH 1/1 Disable FLASH_AMLV256U support for TOP860 target

Am 28.10.2014 10:48, schrieb Wolfgang Denk:

Quoted 68 lines

Dear Reinhard,
Quoted 5 lines · In message <CA+gZxsPKtwYAhHm88fBtGW17cxT7ncEz9OBxwpqiJ55WR-kfcg@mail.gmail.com>…
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.
Quoted 55 lines
Best,
Vasili
Quoted 50 lines · Wolfgang Denk
Dear Vasili,
In message <CA+gZxsOYLBU18LimMmfP9B-gZaykN=
Quoted 10 lines · hZtM1FVgpd8p-eEW19bQ at mail.gmail.com> you wrote:
Quoted 10 lines
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 <[email protected]>
Cc Wolfgang Denk <[email protected]>
---
  board/emk/common/flash.c |    4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)
Quoted 5 lines
board/emk/common/flash.c ulong flash_get_size (FPWV *addr, flash_info_t
Quoted 5 lines
board/emk/common/flash.c +0 -0
index ae5777c..4119b3b 100644
@@ -324,6 +324,7 @@ ulong flash_get_size (FPWV *addr, flash_info_t
*info)
Quoted 8 lines
board/emk/common/flash.c ulong flash_get_size (FPWV *addr, flash_info_t
Quoted 8 lines
board/emk/common/flash.c +1 -0
                         }
                         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)
Quoted 8 lines
Quoted 8 lines
board/emk/common/flash.c +2 -1
                         }
                         break;
                 }
-
+#endif
+
                 /* fall thru to here ! */
         default:
Quoted 8 lines
Quoted 5 lines
board/emk/common/flash.c +0 -1
                 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?

Quoted 8 lines

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

Wolfgang Denk

2014-10-28 10:23 UTC #6 lore raw

PATCH 1/1 Disable FLASH_AMLV256U support for TOP860 target

Dear Reinhard,

Quoted 10 lines · In message <[email protected]> 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

Signature · 5 lines

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

Reinhard Meyer

2014-10-28 10:35 UTC #7 lore raw

PATCH 1/1 Disable FLASH_AMLV256U support for TOP860 target

Dear Wolfgang,

Quoted 10 lines

Dear Reinhard,
Quoted 7 lines · In message <[email protected]> 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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 2 lines

- am79c874.c is completely bogus anyway

How so?

Quoted 1 line

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

Quoted 1 line

- vpd.c is causing efforts when I2C / EEPROM code gets changed

ideally, the API should not change that often ;)

Quoted 2 lines

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

Wolfgang Denk

2014-10-28 10:40 UTC #8 lore raw

PATCH 1/1 Disable FLASH_AMLV256U support for TOP860 target

Dear Reinhard,

Quoted 3 lines · In message <[email protected]> you wrote:

Quoted 2 lines
- 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...

Quoted 10 lines

Quoted 1 line
- 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.
Quoted 1 line
- vpd.c is causing efforts when I2C / EEPROM code gets changed
ideally, the API should not change that often ;)
Quoted 2 lines
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

Signature · 7 lines

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