[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Mike Frysinger]

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++) {

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Mike Frysinger]

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

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Michael Schwingen]

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

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Michael Schwingen]

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

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Michael Schwingen]

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

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Michael Schwingen]

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

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [patch] do not use cmd_reset uninitialized in cfi_flash.c

[Haavard Skinnemoen]

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