[U-Boot] [RFC] cfi_flash and complex address mapping
Stefan Roese
sr at denx.de
Thu Mar 26 11:03:28 CET 2009
On Wednesday 25 March 2009, thomas.langer at infineon.com wrote:
> I'm in the process of preparing some code for a new board and want to use
> the generic cfi flash driver. The problem is EBU (External Bus Unit) of the
> chip, which is internally 32bit.
Which SoC is this btw?
> The access to 8/16bit values are always mapped by the EBU, but in a little
> endian way, which is bad on a big endian system: On an interface configured
> for 16bit, the following conversion from internal address to bus-address is
> done: CPU > external bus
> 0x00000000 > 0x00000002
> 0x00000002 > 0x00000000
> 0x00000004 > 0x00000006
> 0x00000006 > 0x00000004
Arrgh!
> If I implement my own flash_read%/flash_write% to do the address mapping,
> the data is also swapped. In this functions I cannot decide if the access
> is for the flash control or the data to be programmed.
Yes, too bad that this doesn't work for you.
> My current solution is the following patch and these definitions in the
> board config file (for a 16bit flash):
>
> #define FLASH_FIXUP_ADDR_8(addr) ((void*)((ulong)(addr)^2))
> #define FLASH_FIXUP_ADDR_16(addr) ((void*)((ulong)(addr)^2))
>
> My question now is: Would such a change be accepted for the mainline
> u-boot? Or does someone has a better idea?
Yes, I think this could be accepted. The overall impact on the driver is not
too big. Let's see if others have some complaints about it. If not, I'll
accept you patch after you changed some minor details.
Please find some more comments below.
> Thanks for your comments,
> Thomas
>
> PS: The patch is against u-boot-2009.01
You should add your Signed-off-by line to this commit text. Best you generate
and send your patches using the git tools.
>
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -176,6 +176,23 @@ flash_info_t flash_info[CFI_MAX_FLASH_BA
> #define CONFIG_SYS_FLASH_CFI_WIDTH FLASH_CFI_8BIT
> #endif
>
> +/*
> + * Check if address fixup macros are defined, define defaults otherwise
> + */
> +#ifndef FLASH_FIXUP_ADDR_8
> +#define FLASH_FIXUP_ADDR_8(addr) (addr)
> +#endif
> +#ifndef FLASH_FIXUP_ADDR_16
> +#define FLASH_FIXUP_ADDR_16(addr) (addr)
> +#endif
> +#ifndef FLASH_FIXUP_ADDR_32
> +#define FLASH_FIXUP_ADDR_32(addr) (addr)
> +#endif
> +#ifndef FLASH_FIXUP_ADDR_64
> +#define FLASH_FIXUP_ADDR_64(addr) (addr)
> +#endif
> +
> +
> /* CFI standard query structure */
> struct cfi_qry {
> u8 qry[3];
> @@ -392,9 +409,9 @@ static inline uchar flash_read_uchar (fl
>
> cp = flash_map (info, 0, offset);
> #if defined(__LITTLE_ENDIAN) || defined(CONFIG_SYS_WRITE_SWAPPED_DATA)
> - retval = flash_read8(cp);
> + retval = flash_read8(FLASH_FIXUP_ADDR_8(cp));
> #else
> - retval = flash_read8(cp + info->portwidth - 1);
> + retval = flash_read8(FLASH_FIXUP_ADDR_8(cp) + info->portwidth - 1);
> #endif
> flash_unmap (info, 0, offset, cp);
> return retval;
> @@ -408,7 +425,7 @@ static inline ushort flash_read_word (fl
> ushort *addr, retval;
>
> addr = flash_map (info, 0, offset);
> - retval = flash_read16 (addr);
> + retval = flash_read16 (FLASH_FIXUP_ADDR_16(addr));
> flash_unmap (info, 0, offset, addr);
> return retval;
> }
> @@ -433,19 +450,28 @@ static ulong flash_read_long (flash_info
> debug ("long addr is at %p info->portwidth = %d\n", addr,
> info->portwidth);
> for (x = 0; x < 4 * info->portwidth; x++) {
> - debug ("addr[%x] = 0x%x\n", x, flash_read8(addr + x));
> + debug ("addr[%x] = 0x%x\n", x,
> + flash_read8(FLASH_FIXUP_ADDR_32(addr) + x));
> }
> #endif
> #if defined(__LITTLE_ENDIAN) || defined(CONFIG_SYS_WRITE_SWAPPED_DATA)
> - retval = ((flash_read8(addr) << 16) |
> - (flash_read8(addr + info->portwidth) << 24) |
> - (flash_read8(addr + 2 * info->portwidth)) |
> - (flash_read8(addr + 3 * info->portwidth) << 8));
> + retval = ((flash_read8(FLASH_FIXUP_ADDR_8
> + (addr) << 16) |
I don't like this line split above and below, where the parameter for
FLASH_FIXUP_ADDR_8 is moved into the 2nd line. But I understand that you did
this because of the line length restrictions, correct? Perhaps we should
introduce a local variable for info->portwidth (e.g. pw) and the statement
will still fit into one line? Or at least the parameter for FLASH_FIXUP...
> + (flash_read8(FLASH_FIXUP_ADDR_8
> + (addr + info->portwidth)) << 24) |
> + (flash_read8(FLASH_FIXUP_ADDR_8
> + (addr + 2 * info->portwidth))) |
> + (flash_read8(FLASH_FIXUP_ADDR_8
> + (addr + 3 * info->portwidth)) << 8));
> #else
> - retval = ((flash_read8(addr + 2 * info->portwidth - 1) << 24) |
> - (flash_read8(addr + info->portwidth - 1) << 16) |
> - (flash_read8(addr + 4 * info->portwidth - 1) << 8) |
> - (flash_read8(addr + 3 * info->portwidth - 1)));
> + retval = ((flash_read8(FLASH_FIXUP_ADDR_8
> + (addr + 2 * info->portwidth - 1)) << 24) |
> + (flash_read8(FLASH_FIXUP_ADDR_8
> + (addr + info->portwidth - 1)) << 16) |
> + (flash_read8(FLASH_FIXUP_ADDR_8
> + (addr + 4 * info->portwidth - 1)) << 8) |
> + (flash_read8(FLASH_FIXUP_ADDR_8
> + (addr + 3 * info->portwidth - 1))));
> #endif
> flash_unmap(info, sect, offset, addr);
>
> @@ -466,21 +492,22 @@ static void flash_write_cmd (flash_info_
> flash_make_cmd (info, cmd, &cword);
> switch (info->portwidth) {
> case FLASH_CFI_8BIT:
> - debug ("fwc addr %p cmd %x %x 8bit x %d bit\n", addr, cmd,
> - cword.c, info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
> - flash_write8(cword.c, addr);
> + debug ("fwc addr %p cmd %x %x 8bit x %d bit\n",
> + FLASH_FIXUP_ADDR_8(addr), cmd, cword.c,
> + info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
> + flash_write8(cword.c, FLASH_FIXUP_ADDR_8(addr));
> break;
> case FLASH_CFI_16BIT:
> - debug ("fwc addr %p cmd %x %4.4x 16bit x %d bit\n", addr,
> - cmd, cword.w,
> + debug ("fwc addr %p cmd %x %4.4x 16bit x %d bit\n",
> + FLASH_FIXUP_ADDR_16(addr), cmd, cword.w,
> info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
> - flash_write16(cword.w, addr);
> + flash_write16(cword.w, FLASH_FIXUP_ADDR_16(addr));
> break;
> case FLASH_CFI_32BIT:
> - debug ("fwc addr %p cmd %x %8.8lx 32bit x %d bit\n", addr,
> - cmd, cword.l,
> + debug ("fwc addr %p cmd %x %8.8lx 32bit x %d bit\n",
> + FLASH_FIXUP_ADDR_32(addr), cmd, cword.l,
> info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
> - flash_write32(cword.l, addr);
> + flash_write32(cword.l, FLASH_FIXUP_ADDR_32(addr));
> break;
> case FLASH_CFI_64BIT:
> #ifdef DEBUG
> @@ -490,11 +517,11 @@ static void flash_write_cmd (flash_info_
> print_longlong (str, cword.ll);
>
> debug ("fwrite addr %p cmd %x %s 64 bit x %d bit\n",
> - addr, cmd, str,
> + FLASH_FIXUP_ADDR_64(addr), cmd, str,
> info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
> }
> #endif
> - flash_write64(cword.ll, addr);
> + flash_write64(cword.ll, FLASH_FIXUP_ADDR_64(addr));
> break;
> }
>
> @@ -525,16 +552,19 @@ static int flash_isequal (flash_info_t *
> debug ("is= cmd %x(%c) addr %p ", cmd, cmd, addr);
> switch (info->portwidth) {
> case FLASH_CFI_8BIT:
> - debug ("is= %x %x\n", flash_read8(addr), cword.c);
> - retval = (flash_read8(addr) == cword.c);
> + debug ("is= %x %x\n",
> + flash_read8(FLASH_FIXUP_ADDR_8(addr)), cword.c);
> + retval = (flash_read8(FLASH_FIXUP_ADDR_8(addr)) == cword.c);
> break;
> case FLASH_CFI_16BIT:
> - debug ("is= %4.4x %4.4x\n", flash_read16(addr), cword.w);
> - retval = (flash_read16(addr) == cword.w);
> + debug ("is= %4.4x %4.4x\n",
> + flash_read16(FLASH_FIXUP_ADDR_16(addr)), cword.w);
> + retval = (flash_read16(FLASH_FIXUP_ADDR_16(addr)) == cword.w);
> break;
> case FLASH_CFI_32BIT:
> - debug ("is= %8.8x %8.8lx\n", flash_read32(addr), cword.l);
> - retval = (flash_read32(addr) == cword.l);
> + debug ("is= %8.8x %8.8lx\n",
> + flash_read32(FLASH_FIXUP_ADDR_32(addr)), cword.l);
> + retval = (flash_read32(FLASH_FIXUP_ADDR_32(addr)) == cword.l);
> break;
> case FLASH_CFI_64BIT:
> #ifdef DEBUG
> @@ -542,12 +572,13 @@ static int flash_isequal (flash_info_t *
> char str1[20];
> char str2[20];
>
> - print_longlong (str1, flash_read64(addr));
> + print_longlong (str1, flash_read64(FLASH_FIXUP_ADDR_64
> + (addr)));
> print_longlong (str2, cword.ll);
> debug ("is= %s %s\n", str1, str2);
> }
> #endif
> - retval = (flash_read64(addr) == cword.ll);
> + retval = (flash_read64(FLASH_FIXUP_ADDR_64(addr)) == cword.ll);
> break;
> default:
> retval = 0;
> @@ -571,16 +602,20 @@ static int flash_isset (flash_info_t * i
> flash_make_cmd (info, cmd, &cword);
> switch (info->portwidth) {
> case FLASH_CFI_8BIT:
> - retval = ((flash_read8(addr) & cword.c) == cword.c);
> + retval = ((flash_read8(FLASH_FIXUP_ADDR_8(addr))
> + & cword.c) == cword.c);
> break;
> case FLASH_CFI_16BIT:
> - retval = ((flash_read16(addr) & cword.w) == cword.w);
> + retval = ((flash_read16(FLASH_FIXUP_ADDR_16(addr))
> + & cword.w) == cword.w);
> break;
> case FLASH_CFI_32BIT:
> - retval = ((flash_read32(addr) & cword.l) == cword.l);
> + retval = ((flash_read32(FLASH_FIXUP_ADDR_32(addr))
> + & cword.l) == cword.l);
> break;
> case FLASH_CFI_64BIT:
> - retval = ((flash_read64(addr) & cword.ll) == cword.ll);
> + retval = ((flash_read64(FLASH_FIXUP_ADDR_64(addr))
> + & cword.ll) == cword.ll);
> break;
> default:
> retval = 0;
> @@ -604,17 +639,22 @@ static int flash_toggle (flash_info_t *
> flash_make_cmd (info, cmd, &cword);
> switch (info->portwidth) {
> case FLASH_CFI_8BIT:
> - retval = flash_read8(addr) != flash_read8(addr);
> + retval = flash_read8(FLASH_FIXUP_ADDR_8(addr)) !=
> + flash_read8(FLASH_FIXUP_ADDR_8(addr));
> break;
> case FLASH_CFI_16BIT:
> - retval = flash_read16(addr) != flash_read16(addr);
> + retval = flash_read16(FLASH_FIXUP_ADDR_16(addr)) !=
> + flash_read16(FLASH_FIXUP_ADDR_16(addr));
> break;
> case FLASH_CFI_32BIT:
> - retval = flash_read32(addr) != flash_read32(addr);
> + retval = flash_read32(FLASH_FIXUP_ADDR_32(addr)) !=
> + flash_read32(FLASH_FIXUP_ADDR_32(addr));
> break;
> case FLASH_CFI_64BIT:
> - retval = ( (flash_read32( addr ) != flash_read32( addr )) ||
> - (flash_read32(addr+4) != flash_read32(addr+4)) );
> + retval = ( (flash_read32(FLASH_FIXUP_ADDR_64( addr )) !=
> + flash_read32(FLASH_FIXUP_ADDR_64( addr ))) ||
> + (flash_read32(FLASH_FIXUP_ADDR_64(addr+4)) !=
> + flash_read32(FLASH_FIXUP_ADDR_64(addr+4))) );
Please remove the spaces before the ")" and after the "(". They don't make
sense now anymore.
And please CC me on CFI related patches. This makes it easier for me not to
miss anything.
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
=====================================================================
More information about the U-Boot
mailing list