do not use cmd_reset uninitialized in cfi_flash.c
PATCH 1/1 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.
---
drivers/mtd/cfi_flash.c +1 -1
@@ -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++) {
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 4 lines · Mike Frysinger
The
cmd_resetmember 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 usecmd_resetinside 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 at denx.de
=====================================================================
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 9 lines · Stefan Roese
Quoted 4 lines · Mike Frysinger
The
cmd_resetmember 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 usecmd_resetinside of the cfi detection functions.Right. But your version now uses the Intel reset command (
0xff) unconditionally. AMD/Spansion style flash chips needAMD_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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080129/8a3f936f/attachment.pgp
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 8 lines · Mike Frysinger
Quoted 7 lines · Stefan Roese
Quoted 5 lines · Mike Frysinger
The
cmd_resetmember 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 usecmd_resetinside of the cfi detection functions.
Quoted 6 lines
Quoted 3 lines
Right. But your version now uses the Intel reset command (
0xff) unconditionally. AMD/Spansion style flash chips needAMD_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.
Quoted 2 lines
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 at denx.de
=====================================================================
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 6 lines · Stefan Roese
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_RESETis 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.
Quoted 6 lines
Quoted 3 lines
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
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 14 lines · Michael Schwingen
Quoted 5 lines · Stefan Roese
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_RESETis 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.
Quoted 5 lines
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.
Quoted 12 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
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 at denx.de
=====================================================================
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 7 lines · Stefan Roese
Quoted 4 lines
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
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 11 lines · Michael Schwingen
Quoted 6 lines · Stefan Roese
Quoted 3 lines
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 at denx.de
=====================================================================
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 5 lines · Stefan Roese
Quoted 4 lines
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.
Quoted 7 lines
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
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 5 lines · Michael Schwingen
Quoted 1 line
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 at denx.de
=====================================================================
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
In reply to Stefan RoeseQuoted 7 lines
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
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 8 lines · Michael Schwingen
Quoted 6 lines
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. :)
Quoted 11 lines
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 at denx.de
=====================================================================
PATCH 1/1 do not use cmd_reset uninitialized in cfi_flash.c
Quoted 3 lines · On Wed, 20 Feb 2008 10:18:58 +0100 Stefan Roese <[email protected]> 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