ST M29W320D incorrectly configured

[Eric W. Biederman]

David Woodhouse <dwmw2 at infradead.org> writes:

> On Wed, 2008-10-29 at 19:53 -0400, Corinna Schultz wrote:
>> I'm having a problem getting the unlock addresses correctly configured  
>> for the ST M29W320D chip. CFI query data is shown below (from  
>> debugging statements I enabled and/or added to the driver). The chip  
>> is wired so that the #BYTE pin is low, putting the chip into x8 mode.  
>> The data bus is physically 8 bits.
>> 
>> I don't understand everything that's going on in the code, but it  
>> seems to me that the following code (taken from cfi_cmdset_0002.c,  
>> which sets the unlock addresses needed for writing and erasing) has a  
>> logic error. Maybe someone can explain it to me?
>> 
>>   if (    /* x16 in x8 mode */
>>      ((cfi->device_type == CFI_DEVICETYPE_X8) &&
>>       (cfi->cfiq->InterfaceDesc == 2))
>> 
>> 
>> The reason I think this is in error is that both the device type and  
>> the interfaceDesc are defined by the hardware, and not indicative of  
>> the mode. Besides, isn't this conditional actually testing if the chip
>> is an X8 chip and supports x8 and x16 async modes?
>
> That condition is doubly wrong, I think. Not only does it never trigger,
> but it'd do the wrong thing if it _did_ trigger. I think the answer is
> to revert this:

Yep.  The logic came from a misunderstanding of how the devices would show up.
> http://lists.infradead.org/pipermail/linux-mtd-cvs/2004-September/004096.html

> It would be nice if we could do it that way, but these ST chips don't
> seem to work. When they're in 16-bit mode, they really do need a byte
> address of 0x555 for the second unlock address, not 0x554 (0x2aa*2) as
> every other chip seems to accept. Although it takes _extra_ logic to
> check the input on the 'A-1' pin, they seem to have it.
>
> So I think the answer is to go back to cfi->addr_unlock[12] being _byte_
> addresses within the chip...

And in case you prefer to go do everything in byte address here are some
comments in line.

> +		cfi->addr_unlock1 = 0x555 << cfi->device_type;
> +		cfi->addr_unlock2 = 0x2aa << cfi->device_type;
> +
> +		/* Handle the case of x16 chips in x8 mode which are _really_
> +		   picky about the unlock addresses, and require that A-1 is
> +		   set too. This is only some ST chips so far...  */
> + if (cfi->device_type == 2 && map_bankwidth(map) == cfi_interleave(cfi))
> + cfi->addr_unlock2 |= 1; /* i.e. 0x555 instead of 0x554 */

Note the correct test here is:
		if (cfi->device_type/2 == (map_bankwidth(map)/cfi_interleave(cfi)))
			cfi_addr_unlock2 |= 1;

You need the division or else crazy cases like interleaved x16 devices in x8 mode 
won't work.  Testing device_type/2 == map_bankwidth(map)/cfi_interleave(cfi) automatically
enables x32 devices in x16 mode as well.
                        
>   retry:
>  	if (!cfi->numchips) {
> + unsigned unlock_mask = 0xFFFF << (map_bankwidth(map) / cfi_interleave(cfi) -
> 1);
> +

Looking at the specified unlock address it looks like the loss of the low
bit here is actually a bug in jedec_probe.  So we should not need unlock_mask.

It appears you have passed over the probe sequence in cfi_probe_setup to reset
the chip.  It doesn't seem to make a difference in this case but still it
won't work on x16 devices in x8 mode as current written.

Eric