Hi Andrew, On Tue, 14 May 2013 12:27:52 -0500, Andrew Gabbasov <andrew_gabba...@mentor.com> wrote:
> Packed structure cfi_qry contains unaligned 16- and 32-bits members, > accessing which causes problems when cfi_flash driver is compiled with > -munaligned-access option: flash initialization hangs, probably > due to data error. > > Since the structure is supposed to replicate the actual data layout > in CFI Flash chips, the alignment issue can't be fixed in the structure. > So, unaligned fields need using of explicit unaligned access macros. > > Signed-off-by: Andrew Gabbasov <andrew_gabba...@mentor.com> > --- > drivers/mtd/cfi_flash.c | 15 +++++++++------ > include/mtd/cfi_flash.h | 18 +++++++++++------- > 2 files changed, 20 insertions(+), 13 deletions(-) > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > index 22d8440..f6759a8 100644 > --- a/drivers/mtd/cfi_flash.c > +++ b/drivers/mtd/cfi_flash.c > @@ -38,6 +38,7 @@ > #include <asm/processor.h> > #include <asm/io.h> > #include <asm/byteorder.h> > +#include <asm/unaligned.h> > #include <environment.h> > #include <mtd/cfi_flash.h> > #include <watchdog.h> > @@ -1640,9 +1641,10 @@ static void cfi_reverse_geometry(struct cfi_qry *qry) > u32 tmp; > > for (i = 0, j = qry->num_erase_regions - 1; i < j; i++, j--) { > - tmp = qry->erase_region_info[i]; > - qry->erase_region_info[i] = qry->erase_region_info[j]; > - qry->erase_region_info[j] = tmp; > + tmp = get_unaligned(&(qry->erase_region_info[i])); > + put_unaligned(get_unaligned(&(qry->erase_region_info[j])), > + &(qry->erase_region_info[i])); > + put_unaligned(tmp, &(qry->erase_region_info[j])); > } > } > > @@ -2073,8 +2075,8 @@ ulong flash_get_size (phys_addr_t base, int banknum) > info->start[0] = (ulong)map_physmem(base, info->portwidth, MAP_NOCACHE); > > if (flash_detect_cfi (info, &qry)) { > - info->vendor = le16_to_cpu(qry.p_id); > - info->ext_addr = le16_to_cpu(qry.p_adr); > + info->vendor = le16_to_cpu(get_unaligned(&(qry.p_id))); > + info->ext_addr = le16_to_cpu(get_unaligned(&(qry.p_adr))); > num_erase_regions = qry.num_erase_regions; > > if (info->ext_addr) { > @@ -2163,7 +2165,8 @@ ulong flash_get_size (phys_addr_t base, int banknum) > break; > } > > - tmp = le32_to_cpu(qry.erase_region_info[i]); > + tmp = le32_to_cpu(get_unaligned( > + &(qry.erase_region_info[i]))); > debug("erase region %u: 0x%08lx\n", i, tmp); > > erase_region_count = (tmp & 0xffff) + 1; > diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h > index 966b5e0..b644b91 100644 > --- a/include/mtd/cfi_flash.h > +++ b/include/mtd/cfi_flash.h > @@ -129,12 +129,16 @@ typedef union { > } cfiword_t; > > /* CFI standard query structure */ > +/* The offsets and sizes of this packed structure members correspond > + * to the actual layout in CFI Flash chips. Some 16- and 32-bit members > + * are unaligned and must be accessed with explicit unaligned access macros. > + */ > struct cfi_qry { > u8 qry[3]; > - u16 p_id; > - u16 p_adr; > - u16 a_id; > - u16 a_adr; > + u16 p_id; /* unaligned */ > + u16 p_adr; /* unaligned */ > + u16 a_id; /* unaligned */ > + u16 a_adr; /* unaligned */ > u8 vcc_min; > u8 vcc_max; > u8 vpp_min; > @@ -148,10 +152,10 @@ struct cfi_qry { > u8 block_erase_timeout_max; > u8 chip_erase_timeout_max; > u8 dev_size; > - u16 interface_desc; > - u16 max_buf_write_size; > + u16 interface_desc; /* aligned */ > + u16 max_buf_write_size; /* aligned */ > u8 num_erase_regions; > - u32 erase_region_info[NUM_ERASE_REGIONS]; > + u32 erase_region_info[NUM_ERASE_REGIONS]; /* unaligned */ > } __attribute__((packed)); > > struct cfi_pri_hdr { Reviewed-By: Albert ARIBAUD <albert.u.b...@aribaud.net> Seems ok to me. Now, seeing as this is global to all architectures, yet motivated by ARM alignment considerations, which repo should this go to? Amicalement, -- Albert. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot