| Message ID | 4D8305D6.5060302@dawes.za.net |
|---|---|
| State | RFC |
| Headers | show |
I don't think that will work. Multiplying the offsets by two won't always work (i.e. offset 2AA * 2 != 555). I think this will require a more substantial fix. I started working on something but it may take me a few days, though I have both an 8-bit and a 16-bit board to play with (though no 8-bit chip on a 16-bit bus). -Aaron On 03/18/2011 12:12 AM, Rogan Dawes wrote: > On 2011/03/18 6:02 AM, Aaron Williams wrote: >> Hi, >> >> I am running into issues in that the CFI code is broken for flash with an 8- >> bit bus. The problem is that the CFI code uses the wrong addresses for 8-bits >> vs 16-bits. >> >> The CFI function flash_map incorrectly calculates the byte offset with >> Spansion flash. >> >> In our case, we have an 8-bit Spansion S29GL-N MirrorBit flash chip on an 8- >> bit bus. >> >> According to the data sheet, in 8-bit mode the first unlock address is 0xAAA >> and the second is 0x555. Basically all of the addresses are approximately >> double what they are in 16-bit mode. >> >> Most of the addresses in the CFI code are wrong for 8-bit mode. >> >> They should be as follows: >> unlock1: 0xAAA >> unlock2: 0x555 >> CFI Query: 0xAA >> >> Offset Device ID: 0x02, 0x1c, 0x1e >> Offset Silicon Sector Factory Protect: 0x6 >> Sector protect verify (sector address) + 4 >> >> The problem is that the dectection code detects (incorrectly) that we're an 8- >> bit device on a 16-bit bus when in fact it's an 8-bit bus. >> >> For 16-bits, it's: >> >> unlock1: 0x555 >> unlock2: 0x2aa >> CFI Query: 0x55 >> Offset Device ID: 0x1, 0x0e, 0x0f >> >> Now, in 16-bit mode these addresses should be effectively doubled so they >> should look to be the same to software as I understand it. >> >> This is all taken from the datasheet. >> >> Note that the CFI code works fine with the flash hooked up as 16-bit on a 16- >> bit bus. >> >> If it's an 8-bit flash on a 16-bit bus then the addresses should be doubled. >> >> -Aaron > Hi Aaron, > > Does this patch (originally by Stefan Roese) work for you? I also need > it for the DNS323 port. FWIW, mine is also a Spansion flash > (S29GL064M90TFIR4), although I am not 100% sure of the layout and > connectivity. > > Rogan > > --- > drivers/mtd/cfi_flash.c | 22 +++++++++++++++++++++- > 1 files changed, 21 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > index dd394a8..74e9ade 100644 > --- a/drivers/mtd/cfi_flash.c > +++ b/drivers/mtd/cfi_flash.c > @@ -291,6 +291,9 @@ static inline uchar flash_read_uchar (flash_info_t * > info, uint offset) > uchar *cp; > uchar retval; > > +#if 1 // test-only > + offset = offset * 2; > +#endif > cp = flash_map (info, 0, offset); > #if defined(__LITTLE_ENDIAN) || defined(CONFIG_SYS_WRITE_SWAPPED_DATA) > retval = flash_read8(cp); > @@ -308,6 +311,9 @@ static inline ushort flash_read_word (flash_info_t * > info, uint offset) > { > ushort *addr, retval; > > +#if 1 // test-only > + offset = offset * 2; > +#endif > addr = flash_map (info, 0, offset); > retval = flash_read16 (addr); > flash_unmap (info, 0, offset, addr); > @@ -328,6 +334,9 @@ static ulong flash_read_long (flash_info_t * info, > flash_sect_t sect, > #ifdef DEBUG > int x; > #endif > +#if 1 // test-only > + offset = offset * 2; > +#endif > addr = flash_map (info, sect, offset); > > #ifdef DEBUG > @@ -363,6 +372,9 @@ void flash_write_cmd (flash_info_t * info, > flash_sect_t sect, > void *addr; > cfiword_t cword; > > +#if 1 // test-only > + offset = offset * 2; > +#endif > addr = flash_map (info, sect, offset); > flash_make_cmd (info, cmd, &cword); > switch (info->portwidth) { > @@ -420,6 +432,9 @@ static int flash_isequal (flash_info_t * info, > flash_sect_t sect, > cfiword_t cword; > int retval; > > +#if 1 // test-only > + offset = offset * 2; > +#endif > addr = flash_map (info, sect, offset); > flash_make_cmd (info, cmd, &cword); > > @@ -1762,6 +1777,7 @@ static int __flash_detect_cfi (flash_info_t * > info, struct cfi_qry *qry) > info->addr_unlock1 = 0x555; > info->addr_unlock2 = 0x2aa; > > +#if 0 // test-only > /* > * modify the unlock address if we are > * in compatibility mode > @@ -1776,6 +1792,7 @@ static int __flash_detect_cfi (flash_info_t * > info, struct cfi_qry *qry) > info->addr_unlock1 = 0xaaa; > info->addr_unlock2 = 0x555; > } > +#endif > > info->name = "CFI conformant"; > return 1; > @@ -1942,11 +1959,13 @@ ulong flash_get_size (phys_addr_t base, int banknum) > debug ("cfi version is 0x%04x\n", info->cfi_version); > > size_ratio = info->portwidth / info->chipwidth; > +#if 0 // test-only > /* if the chip is x8/x16 reduce the ratio by half */ > if ((info->interface == FLASH_CFI_X8X16) > && (info->chipwidth == FLASH_CFI_BY8)) { > size_ratio >>= 1; > } > +#endif > debug ("size_ratio %d port %d bits chip %d bits\n", > size_ratio, info->portwidth << CFI_FLASH_SHIFT_WIDTH, > info->chipwidth << CFI_FLASH_SHIFT_WIDTH); > @@ -2034,12 +2053,13 @@ ulong flash_get_size (phys_addr_t base, int banknum) > /* round up when converting to ms */ > info->write_tout = (tmp + 999) / 1000; > info->flash_id = FLASH_MAN_CFI; > +#if 0 > if ((info->interface == FLASH_CFI_X8X16) && > (info->chipwidth == FLASH_CFI_BY8)) { > /* XXX - Need to test on x8/x16 in parallel. */ > info->portwidth >>= 1; > } > - > +#endif > flash_write_cmd (info, 0, 0, info->cmd_reset); > } >
I have it working on our 8-bit boards properly now, though I've only done some limited testing. I basically put in the correct addresses into cfi_flash.h and updated various addresses for 8-bit support. The problem was that the CFI code was written using the 16-bit addresses which don't work for an 8-bit bus. I added a mask which is used to calculate the correct address for 16 and 32- bit busses. I have only been able to test with 8-bit and 16-bit Spansion devices though. -Aaron On Friday, March 18, 2011 12:12:22 AM Rogan Dawes wrote: > On 2011/03/18 6:02 AM, Aaron Williams wrote: > > Hi, > > > > I am running into issues in that the CFI code is broken for flash with an > > 8- bit bus. The problem is that the CFI code uses the wrong addresses > > for 8-bits vs 16-bits. > > > > The CFI function flash_map incorrectly calculates the byte offset with > > Spansion flash. > > > > In our case, we have an 8-bit Spansion S29GL-N MirrorBit flash chip on an > > 8- bit bus. > > > > According to the data sheet, in 8-bit mode the first unlock address is > > 0xAAA and the second is 0x555. Basically all of the addresses are > > approximately double what they are in 16-bit mode. > > > > Most of the addresses in the CFI code are wrong for 8-bit mode. > > > > They should be as follows: > > unlock1: 0xAAA > > unlock2: 0x555 > > CFI Query: 0xAA > > > > Offset Device ID: 0x02, 0x1c, 0x1e > > Offset Silicon Sector Factory Protect: 0x6 > > Sector protect verify (sector address) + 4 > > > > The problem is that the dectection code detects (incorrectly) that we're > > an 8- bit device on a 16-bit bus when in fact it's an 8-bit bus. > > > > For 16-bits, it's: > > > > unlock1: 0x555 > > unlock2: 0x2aa > > CFI Query: 0x55 > > Offset Device ID: 0x1, 0x0e, 0x0f > > > > Now, in 16-bit mode these addresses should be effectively doubled so they > > should look to be the same to software as I understand it. > > > > This is all taken from the datasheet. > > > > Note that the CFI code works fine with the flash hooked up as 16-bit on a > > 16- bit bus. > > > > If it's an 8-bit flash on a 16-bit bus then the addresses should be > > doubled. > > > > > > > > -Aaron > > Hi Aaron, > > Does this patch (originally by Stefan Roese) work for you? I also need > it for the DNS323 port. FWIW, mine is also a Spansion flash > (S29GL064M90TFIR4), although I am not 100% sure of the layout and > connectivity. > > Rogan > > --- > drivers/mtd/cfi_flash.c | 22 +++++++++++++++++++++- > 1 files changed, 21 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > index dd394a8..74e9ade 100644 > --- a/drivers/mtd/cfi_flash.c > +++ b/drivers/mtd/cfi_flash.c > @@ -291,6 +291,9 @@ static inline uchar flash_read_uchar (flash_info_t * > info, uint offset) > uchar *cp; > uchar retval; > > +#if 1 // test-only > + offset = offset * 2; > +#endif > cp = flash_map (info, 0, offset); > #if defined(__LITTLE_ENDIAN) || defined(CONFIG_SYS_WRITE_SWAPPED_DATA) > retval = flash_read8(cp); > @@ -308,6 +311,9 @@ static inline ushort flash_read_word (flash_info_t * > info, uint offset) > { > ushort *addr, retval; > > +#if 1 // test-only > + offset = offset * 2; > +#endif > addr = flash_map (info, 0, offset); > retval = flash_read16 (addr); > flash_unmap (info, 0, offset, addr); > @@ -328,6 +334,9 @@ static ulong flash_read_long (flash_info_t * info, > flash_sect_t sect, > #ifdef DEBUG > int x; > #endif > +#if 1 // test-only > + offset = offset * 2; > +#endif > addr = flash_map (info, sect, offset); > > #ifdef DEBUG > @@ -363,6 +372,9 @@ void flash_write_cmd (flash_info_t * info, > flash_sect_t sect, > void *addr; > cfiword_t cword; > > +#if 1 // test-only > + offset = offset * 2; > +#endif > addr = flash_map (info, sect, offset); > flash_make_cmd (info, cmd, &cword); > switch (info->portwidth) { > @@ -420,6 +432,9 @@ static int flash_isequal (flash_info_t * info, > flash_sect_t sect, > cfiword_t cword; > int retval; > > +#if 1 // test-only > + offset = offset * 2; > +#endif > addr = flash_map (info, sect, offset); > flash_make_cmd (info, cmd, &cword); > > @@ -1762,6 +1777,7 @@ static int __flash_detect_cfi (flash_info_t * > info, struct cfi_qry *qry) > info->addr_unlock1 = 0x555; > info->addr_unlock2 = 0x2aa; > > +#if 0 // test-only > /* > * modify the unlock address if we are > * in compatibility mode > @@ -1776,6 +1792,7 @@ static int __flash_detect_cfi (flash_info_t * > info, struct cfi_qry *qry) > info->addr_unlock1 = 0xaaa; > info->addr_unlock2 = 0x555; > } > +#endif > > info->name = "CFI conformant"; > return 1; > @@ -1942,11 +1959,13 @@ ulong flash_get_size (phys_addr_t base, int > banknum) debug ("cfi version is 0x%04x\n", info->cfi_version); > > size_ratio = info->portwidth / info->chipwidth; > +#if 0 // test-only > /* if the chip is x8/x16 reduce the ratio by half */ > if ((info->interface == FLASH_CFI_X8X16) > && (info->chipwidth == FLASH_CFI_BY8)) { > size_ratio >>= 1; > } > +#endif > debug ("size_ratio %d port %d bits chip %d bits\n", > size_ratio, info->portwidth << CFI_FLASH_SHIFT_WIDTH, > info->chipwidth << CFI_FLASH_SHIFT_WIDTH); > @@ -2034,12 +2053,13 @@ ulong flash_get_size (phys_addr_t base, int > banknum) /* round up when converting to ms */ > info->write_tout = (tmp + 999) / 1000; > info->flash_id = FLASH_MAN_CFI; > +#if 0 > if ((info->interface == FLASH_CFI_X8X16) && > (info->chipwidth == FLASH_CFI_BY8)) { > /* XXX - Need to test on x8/x16 in parallel. */ > info->portwidth >>= 1; > } > - > +#endif > flash_write_cmd (info, 0, 0, info->cmd_reset); > }
Hi Aaron, On Thursday 24 March 2011 04:07:21 Aaron Williams wrote: > I have it working on our 8-bit boards properly now, though I've only done > some limited testing. Good. > I basically put in the correct addresses into cfi_flash.h and updated > various addresses for 8-bit support. The problem was that the CFI code was > written using the 16-bit addresses which don't work for an 8-bit bus. > > I added a mask which is used to calculate the correct address for 16 and > 32- bit busses. I have only been able to test with 8-bit and 16-bit > Spansion devices though. Please post the patch on the list. I'll try to test it on some of my systems as well. Thanks. Cheers, 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 2011/03/24 5:07 AM, Aaron Williams wrote: > I have it working on our 8-bit boards properly now, though I've only done some > limited testing. > > I basically put in the correct addresses into cfi_flash.h and updated various > addresses for 8-bit support. The problem was that the CFI code was written > using the 16-bit addresses which don't work for an 8-bit bus. > > I added a mask which is used to calculate the correct address for 16 and 32- > bit busses. I have only been able to test with 8-bit and 16-bit Spansion > devices though. > > -Aaron Hi Aaron, I'm happy to test it, when you post the patch. Regards, Rogan
I am still doing some testing and just added a fix today so that if a flash chip that supports both 8 and 16-bits is on an 8-bit bus the interface is set to 8-bits. I'll try and get patches out next week. So far it's working fairly well on all of our boards, most of which have an 8- bit bus but one has a 16-bit bus. I had to change all of the defined addresses for AMD parts and calculate the 16 and 32-bit addresses by applying a mask. The CFI code *should* work if there's an 8-bit part on a 16-bit bus, but it probably will not support two 8-bit parts in parallel. That should be doable with some work. -Aaron On Thursday, March 24, 2011 09:59:07 AM Rogan Dawes wrote: > On 2011/03/24 5:07 AM, Aaron Williams wrote: > > I have it working on our 8-bit boards properly now, though I've only done > > some limited testing. > > > > I basically put in the correct addresses into cfi_flash.h and updated > > various addresses for 8-bit support. The problem was that the CFI code > > was written using the 16-bit addresses which don't work for an 8-bit > > bus. > > > > I added a mask which is used to calculate the correct address for 16 and > > 32- bit busses. I have only been able to test with 8-bit and 16-bit > > Spansion devices though. > > > > -Aaron > > Hi Aaron, > > I'm happy to test it, when you post the patch. > > Regards, > > Rogan
On 2011/03/26 8:32 AM, Aaron Williams wrote: > I am still doing some testing and just added a fix today so that if a flash > chip that supports both 8 and 16-bits is on an 8-bit bus the interface is set > to 8-bits. I'll try and get patches out next week. > > So far it's working fairly well on all of our boards, most of which have an 8- > bit bus but one has a 16-bit bus. I had to change all of the defined addresses > for AMD parts and calculate the 16 and 32-bit addresses by applying a mask. > > The CFI code *should* work if there's an 8-bit part on a 16-bit bus, but it > probably will not support two 8-bit parts in parallel. That should be doable > with some work. > > -Aaron Hi Aaron, I'm looking forward to seeing your patches. With any luck, they'll also work on my board, and a nasty hack can go away. :-) Regards, Rogan
Le 30/03/2011 10:01, Rogan Dawes a écrit : > On 2011/03/26 8:32 AM, Aaron Williams wrote: >> I am still doing some testing and just added a fix today so that if a flash >> chip that supports both 8 and 16-bits is on an 8-bit bus the interface is set >> to 8-bits. I'll try and get patches out next week. >> >> So far it's working fairly well on all of our boards, most of which have an 8- >> bit bus but one has a 16-bit bus. I had to change all of the defined addresses >> for AMD parts and calculate the 16 and 32-bit addresses by applying a mask. >> >> The CFI code *should* work if there's an 8-bit part on a 16-bit bus, but it >> probably will not support two 8-bit parts in parallel. That should be doable >> with some work. >> >> -Aaron > > Hi Aaron, > > I'm looking forward to seeing your patches. With any luck, they'll also > work on my board, and a nasty hack can go away. :-) I also would like to see these patches, even in RFC form. Maybe it could fix the way the MX29LV400CB is detected on ED Mini V2 and will allows removing legacy code for this board. > Regards, > > Rogan Amicalement,
I'll post the patches as soon as I figure out how to get git to do it. The patches are fairly clean and works well for both an 8-bit bus and a 16-bit bus. My biggest obstical is that I'm not all that familiar with GIT and we use SVN internally. -Aaron On Wednesday, March 30, 2011 01:01:42 AM Rogan Dawes wrote: > On 2011/03/26 8:32 AM, Aaron Williams wrote: > > I am still doing some testing and just added a fix today so that if a > > flash chip that supports both 8 and 16-bits is on an 8-bit bus the > > interface is set to 8-bits. I'll try and get patches out next week. > > > > So far it's working fairly well on all of our boards, most of which have > > an 8- bit bus but one has a 16-bit bus. I had to change all of the > > defined addresses for AMD parts and calculate the 16 and 32-bit > > addresses by applying a mask. > > > > The CFI code *should* work if there's an 8-bit part on a 16-bit bus, but > > it probably will not support two 8-bit parts in parallel. That should be > > doable with some work. > > > > -Aaron > > Hi Aaron, > > I'm looking forward to seeing your patches. With any luck, they'll also > work on my board, and a nasty hack can go away. :-) > > Regards, > > Rogan
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index dd394a8..74e9ade 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -291,6 +291,9 @@ static inline uchar flash_read_uchar (flash_info_t * info, uint offset) uchar *cp; uchar retval; +#if 1 // test-only + offset = offset * 2; +#endif cp = flash_map (info, 0, offset); #if defined(__LITTLE_ENDIAN) || defined(CONFIG_SYS_WRITE_SWAPPED_DATA)