ST M29W320D incorrectly configured

David Woodhouse <dwmw2@infradead.org> writes:

> On Fri, 2008-10-31 at 23:33 -0700, Eric W. Biederman wrote:
>> Forget my last I see now how we can try a 16bit device on an 8 bit
>> bus. I had missed the type <<=1 in gen_probe_new_chip.  Ooops.
>> 
>> With that said I think we need to fix cfi_send_gen_cmd as this problem
>> applies to all uses of it.
>> 
>> Unless I've missed something the patch below completely fixes the
>> problem.
>
> Yeah, that looks better. I had looked at the possibility of continuing
> to use 'word' addresses and fixing up cfi_build_cmd_addr(), but for some
> reason I hadn't noticed that it was used _only_ for the unlock addresses
> (or zero). I thought I was going to need to put a special case in for
> when it was being used with unlock addresses, and it all got a bit
> complex. So I switched to using byte addresses in the variables instead.
>
> I prefer your approach, although I think the patch isn't quite correct.

> You have to make sure we properly handle the case of a 16-bit device in
> 16-bit mode. We mustn't set the byte address to 0x555 there; it has to
> remain 0x554. We need to do the 'addr |= ....' bit _only_ if the device
> is in compatibility mode (i.e. interleave * type > map_bankwidth(map)).

Good point.

From: Eric W. Biederman <ebiederm@xmission.com>
Subject: [PATCH] mtd: Fix cfi_send_gen_cmd the handling of x16 devices in x8 mode

cfi_send_gen_cmd is only passed in the addresses:
0, 0x55, 0x2aa, 0x555 and the addresses addr_unlock1 and addr_unlock2
from jeded_probe.  The address 0, 0x55, and 0x555 will not be
effected by this patch.

For 16bit devices in 8bit compatibility mode we need to use the
byte address:  0xaaa and 0x555.  Which effectively match
the word address 0x555 and 0x2aa.  Except the last has it's low byte
set.  We need to set the low bit to maintain the alternating bit
sequence.

Likewise the addresses in jedec_probe whose word address ends
in the bits 10 also need their low bit set.

So generically modify cfi_send_gen_cmd to extend alternating bit
sequences in addresses.  And every current cfi_send_gen_cmd that
assumes cfi_cmd_set_0002 (i.e. uses addresses 0x2aa and 0x555)
needs to be updated.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   13 -------------
 include/linux/mtd/cfi.h             |   12 +++++++++++-
 2 files changed, 11 insertions(+), 14 deletions(-)

On Sat, 2008-11-01 at 01:36 -0700, Eric W. Biederman wrote:
> Likewise the addresses in jedec_probe whose word address ends
> in the bits 10 also need their low bit set.

Some of these places in jedec_probe.c use cfi_build_cmd_addr() directly,
and this version of the patch puts the low-bit-mangling into
cfi_gen_send_cmd() so those bits miss out...

David Woodhouse <dwmw2@infradead.org> writes:

> On Sat, 2008-11-01 at 01:36 -0700, Eric W. Biederman wrote:
>> Likewise the addresses in jedec_probe whose word address ends
>> in the bits 10 also need their low bit set.
>
> Some of these places in jedec_probe.c use cfi_build_cmd_addr() directly,
> and this version of the patch puts the low-bit-mangling into
> cfi_gen_send_cmd() so those bits miss out...

Sorry.  It's late and was tired.   One more try.
This time as a patch series.

The first patch is the bug fix.  The second patch kills
the unnecessary arguments to cfi_send_gen_cmd, making the
code that uses it more readable.

Eric

On Sat, 2008-11-01 at 03:29 -0700, Eric W. Biederman wrote:
> @@ -1808,9 +1808,7 @@ static inline u32 jedec_read_mfr(struct map_info *map, uint32_t base,
>          * several first banks can contain 0x7f instead of actual ID
>          */
>         do {
> -               uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8),
> -                                                 cfi_interleave(cfi),
> -                                                 cfi->device_type);
> +               uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi);
>                 mask = (1 << (cfi->device_type * 8)) - 1;
>                 result = map_read(map, base + ofs);
>                 bank++;

I think this one is still broken by your patch -- it's the exception to
your observation that we only ever use addresses ending in 00, 55 or aa.

> 
> +       /* Modify the unlock address if we are in compatiblity mode.
> +        * For 16bit devices on 8 bit busses
> +        * and 32bit devices on 16 bit busses
> +        * set the low bit of the alternating bit sequence of the address.
> +        */
> +       if (((type * interleave) > bankwidth) && (cmd_addr & 2))
> +               addr |= (type >> 1)*interleave;

Perhaps '&& ((cmd_addr & 0xff) == 0xaa)' is the answer?

On Sat, 2008-11-01 at 10:43 +0000, David Woodhouse wrote:
> I think this one is still broken by your patch -- it's the exception to
> your observation that we only ever use addresses ending in 00, 55 or aa.

No, I misunderstood how that works -- your patch is fine. Although I
think I'll split patch 1/2 into cleanup vs. the actual fix, too.

David Woodhouse <dwmw2@infradead.org> writes:

> On Sat, 2008-11-01 at 10:43 +0000, David Woodhouse wrote:
>> I think this one is still broken by your patch -- it's the exception to
>> your observation that we only ever use addresses ending in 00, 55 or aa.
>
> No, I misunderstood how that works -- your patch is fine. Although I
> think I'll split patch 1/2 into cleanup vs. the actual fix, too.

Sounds good.   The (cmd_ofs & 0xff) == 0xaa might be less prone to false
positives.  Or perhaps it just doesn't make sense for the id reads in jedec_probe.
Sounds like you have a handle on it though.

Eric