[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c
The cmd_reset member of the flash info struct is not initialized until the specific cmdset function is called. This normally happens by: flash_get_size -> flash_detect_cfi -> cmdset_*_init That means we cannot use cmd_reset inside of the cfi detection functions.
Signed-off-by: Sonic Zhang sonic.zhang@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org --- diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index eb509f5..eb3f517 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -1538,7 +1538,7 @@ { int cfi_offset;
- flash_write_cmd (info, 0, 0, info->cmd_reset); + flash_write_cmd (info, 0, 0, FLASH_CMD_RESET); for (cfi_offset=0; cfi_offset < sizeof(flash_offset_cfi) / sizeof(uint); cfi_offset++) {
On Tuesday 29 January 2008, Mike Frysinger wrote:
The cmd_reset member of the flash info struct is not initialized until the specific cmdset function is called. This normally happens by: flash_get_size -> flash_detect_cfi -> cmdset_*_init That means we cannot use cmd_reset inside of the cfi detection functions.
Right. But your version now uses the Intel reset command (0xff) unconditionally. AMD/Spansion style flash chips need AMD_CMD_RESET (0xf0) instead. Any idea how we should handle this?
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
On Tuesday 29 January 2008, Stefan Roese wrote:
On Tuesday 29 January 2008, Mike Frysinger wrote:
The cmd_reset member of the flash info struct is not initialized until the specific cmdset function is called. This normally happens by: flash_get_size -> flash_detect_cfi -> cmdset_*_init That means we cannot use cmd_reset inside of the cfi detection functions.
Right. But your version now uses the Intel reset command (0xff) unconditionally. AMD/Spansion style flash chips need AMD_CMD_RESET (0xf0) instead. Any idea how we should handle this?
i saw that, but there's a define already to account for this i think: CFG_FLASH_CFI_AMD_RESET ... or did i miss the purpose of this ?
the other option is to just not issue a reset ... when i looked at the kernel, it didnt seem to issue a reset in the cfi detection case -mike
On Tuesday 29 January 2008, Mike Frysinger wrote:
On Tuesday 29 January 2008, Stefan Roese wrote:
On Tuesday 29 January 2008, Mike Frysinger wrote:
The cmd_reset member of the flash info struct is not initialized until the specific cmdset function is called. This normally happens by: flash_get_size -> flash_detect_cfi -> cmdset_*_init That means we cannot use cmd_reset inside of the cfi detection functions.
Right. But your version now uses the Intel reset command (0xff) unconditionally. AMD/Spansion style flash chips need AMD_CMD_RESET (0xf0) instead. Any idea how we should handle this?
i saw that, but there's a define already to account for this i think: CFG_FLASH_CFI_AMD_RESET ... or did i miss the purpose of this ?
But you need to know what FLASH chips you are using in this case (Intel or AMD/Spansion). But you are right, they are not pin compatible, so it should be fixed for one board and your solution should be ok. The board config file just has to select CFG_FLASH_CFI_AMD_RESET is AMD/Spasion style flash chips are used.
the other option is to just not issue a reset ... when i looked at the kernel, it didnt seem to issue a reset in the cfi detection case
Yes, this is probably an even better solution, since we have here a chicken-egg-problem.
Any thoughts on this? How should we proceed?
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Stefan Roese wrote:
But you need to know what FLASH chips you are using in this case (Intel or AMD/Spansion). But you are right, they are not pin compatible, so it should be fixed for one board and your solution should be ok. The board config file just has to select CFG_FLASH_CFI_AMD_RESET is AMD/Spasion style flash chips are used.
Um - in the general case: no.
I do have boards where there are alternative (overlapping) footprints for either AMD or Intel flash roms, which are connected to the same chip select line, and both versions are in production - I think either Intel or AMD has an application note on how to do this in the PCB layout.
Also, having both alternatives on one board is not out of the question - AcTux-4 has a small 8-bit AMD bootflash, and a bigger 16-bit Intel flash on different chip selects. The AMD flash is non-CFI, but a CFI flash might be used in such a situation, which means a simple #define in the board config will not be sufficient.
the other option is to just not issue a reset ... when i looked at the kernel, it didnt seem to issue a reset in the cfi detection case
Yes, this is probably an even better solution, since we have here a chicken-egg-problem.
This should work if the flash is correctly wired to a hardware reset, but there are flash roms which have no reset input. In general, on power-up this is no problem, but if some other part of the system (kernel or other application) leaves the flash in the middle of some command mode before jumping back to u-boot, we are in trouble.
The safest method would be to run the CFI query twice, once for each type of reset command.
However, I wonder if it would be possible to simply issue *both* reset commands - if the flash safely ignores the second (unknown) command, this should be fine, but it is relying on undocumented behaviour.
cu Michael
On Sunday 03 February 2008, Michael Schwingen wrote:
Stefan Roese wrote:
But you need to know what FLASH chips you are using in this case (Intel or AMD/Spansion). But you are right, they are not pin compatible, so it should be fixed for one board and your solution should be ok. The board config file just has to select CFG_FLASH_CFI_AMD_RESET is AMD/Spasion style flash chips are used.
Um - in the general case: no.
I do have boards where there are alternative (overlapping) footprints for either AMD or Intel flash roms, which are connected to the same chip select line, and both versions are in production - I think either Intel or AMD has an application note on how to do this in the PCB layout.
Also, having both alternatives on one board is not out of the question - AcTux-4 has a small 8-bit AMD bootflash, and a bigger 16-bit Intel flash on different chip selects. The AMD flash is non-CFI, but a CFI flash might be used in such a situation, which means a simple #define in the board config will not be sufficient.
OK, understood.
the other option is to just not issue a reset ... when i looked at the kernel, it didnt seem to issue a reset in the cfi detection case
Yes, this is probably an even better solution, since we have here a chicken-egg-problem.
This should work if the flash is correctly wired to a hardware reset, but there are flash roms which have no reset input. In general, on power-up this is no problem, but if some other part of the system (kernel or other application) leaves the flash in the middle of some command mode before jumping back to u-boot, we are in trouble.
The safest method would be to run the CFI query twice, once for each type of reset command.
However, I wonder if it would be possible to simply issue *both* reset commands - if the flash safely ignores the second (unknown) command, this should be fine, but it is relying on undocumented behaviour.
Good idea. Do you (or somebody else) have HW available to test such a change?
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Stefan Roese wrote:
However, I wonder if it would be possible to simply issue *both* reset commands - if the flash safely ignores the second (unknown) command, this should be fine, but it is relying on undocumented behaviour.
Good idea. Do you (or somebody else) have HW available to test such a change?
I think I can run tests on a small set (~5-10 different AMD-commandset and 2 intel-commandset, all 16 bit) flashs, but that still leaves the (small) possibility that there are flash roms that behave different.
cu Michael
On Monday 04 February 2008, Michael Schwingen wrote:
Stefan Roese wrote:
However, I wonder if it would be possible to simply issue *both* reset commands - if the flash safely ignores the second (unknown) command, this should be fine, but it is relying on undocumented behaviour.
Good idea. Do you (or somebody else) have HW available to test such a change?
I think I can run tests on a small set (~5-10 different AMD-commandset and 2 intel-commandset, all 16 bit) flashs, but that still leaves the (small) possibility that there are flash roms that behave different.
Right. But it will be an improvement to the current implementation, where a random command is written as RESET command. And also an improvement to the fixed AMD/Intel RESET command. So I vote for trying this solution. I'll test on a few of mine platforms too.
Michael, could please you provide a patch?
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Stefan Roese wrote:
I think I can run tests on a small set (~5-10 different AMD-commandset and 2 intel-commandset, all 16 bit) flashs, but that still leaves the (small) possibility that there are flash roms that behave different.
Right. But it will be an improvement to the current implementation, where a random command is written as RESET command. And also an improvement to the fixed AMD/Intel RESET command. So I vote for trying this solution. I'll test on a few of mine platforms too.
Michael, could please you provide a patch?
Yes, but it may take some days - I probably need to run the tests on a board that has socketed flash, using a special test case that really puts the flash in the middle of a command cycle and tests reset behaviour.
cu Michael
On Tuesday 05 February 2008, Michael Schwingen wrote:
Michael, could please you provide a patch?
Yes, but it may take some days - I probably need to run the tests on a board that has socketed flash, using a special test case that really puts the flash in the middle of a command cycle and tests reset behaviour.
Thanks would be great. Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Right. But it will be an improvement to the current implementation, where a random command is written as RESET command. And also an improvement to the fixed AMD/Intel RESET command. So I vote for trying this solution. I'll test on a few of mine platforms too.
Michael, could please you provide a patch?
Okay - sorry, I did not have time for as much testing as I planned.
From a short test, it looks like AMD-style flash roms treat *any* unknown
command write as a reset, at least when in CFI Query mode, so issuing the Intel reset command to AMD-style flashs seems safe (from the small sample I have), plus the 3-cycle magic sequence should kick the state machine into the right state even without a reset command. Since the AMD-style flashs require the unlock sequence for real operation, I chose to try the AMD reset command first, so that Intel flashs do no see an invalid command prior to the CFI query.
I have tested the patch on AM29LV320-style flashs from Fujitsu and Macronix, plus Intel StrataFlash.
cu Michael
From c18599658f8470898dc12ac99528d03b3a8d570c Mon Sep 17 00:00:00 2001
From: Michael Schwingen michael@schwingen.org Date: Mon, 18 Feb 2008 23:04:17 +0100 Subject: [PATCH] do not use uninitialized cmd_reset; issue both AMD and Intel reset commands instead
Signed-off-by: Michael Schwingen michael@schwingen.org --- drivers/mtd/cfi_flash.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index eb509f5..439c950 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -1538,7 +1538,12 @@ static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry) { int cfi_offset;
- flash_write_cmd (info, 0, 0, info->cmd_reset); + /* We do not yet know what kind of commandset to use, so we issue + the reset command in both Intel and AMD variants, in the hope + that AMD flash roms ignore the Intel command. */ + flash_write_cmd (info, 0, 0, AMD_CMD_RESET); + flash_write_cmd (info, 0, 0, FLASH_CMD_RESET); + for (cfi_offset=0; cfi_offset < sizeof(flash_offset_cfi) / sizeof(uint); cfi_offset++) {
On Monday 18 February 2008, Michael Schwingen wrote:
Right. But it will be an improvement to the current implementation, where a random command is written as RESET command. And also an improvement to the fixed AMD/Intel RESET command. So I vote for trying this solution. I'll test on a few of mine platforms too.
Michael, could please you provide a patch?
Okay - sorry, I did not have time for as much testing as I planned.
This sounds familiar. :)
From a short test, it looks like AMD-style flash roms treat *any* unknown command write as a reset, at least when in CFI Query mode, so issuing the Intel reset command to AMD-style flashs seems safe (from the small sample I have), plus the 3-cycle magic sequence should kick the state machine into the right state even without a reset command. Since the AMD-style flashs require the unlock sequence for real operation, I chose to try the AMD reset command first, so that Intel flashs do no see an invalid command prior to the CFI query.
I have tested the patch on AM29LV320-style flashs from Fujitsu and Macronix, plus Intel StrataFlash.
I'll test it on some platforms here too (Intel & Spansion) and if nobody objects I'll commit your patch and ask Wolfgang to pull into the 1.3.2 release, since it's a bug fix.
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
On Wed, 20 Feb 2008 10:18:58 +0100 Stefan Roese sr@denx.de wrote:
I'll test it on some platforms here too (Intel & Spansion) and if nobody objects I'll commit your patch and ask Wolfgang to pull into the 1.3.2 release, since it's a bug fix.
Tested on ATNGW100 (AT49BV642D flash). Works fine (as expected).
Haavard
participants (5)
-
Haavard Skinnemoen -
Michael Schwingen -
Michael Schwingen -
Mike Frysinger -
Stefan Roese