Hi Stefan, On Fri, 2023-04-14 at 08:57 +0200, Stefan Roese wrote: > Hi Nuno, > > On 3/27/23 15:22, Nuno Sá wrote: > > flash_get_size() will get the flash size from the device itself and > > go > > through all erase regions to read protection status. However, the > > device > > mappable region (eg: devicetree reg property) might be lower than > > the > > device full size which means that the above cycle will result in a > > data > > bus exception. This change fixes it by reading the 'addr_size' > > during > > probe() and also use that as one possible upper limit. > > > > Signed-off-by: Nuno Sá <nuno...@analog.com> > > --- > > > > To give a bit more of background for this patch, I caught this > > issue > > when running u-boot on microblaze which uses xilinx > > axi_linear_flash IP. > > On ADI designs using that IP, the flash size was set to 32MiB > > resulting > > in the mentioned data bus exception as we obviously access past the > > IP size. > > > > That said, there was not real reason for the flash truncation so > > we'll > > be fixing our designs so the IP will contemplate the complete flash > > size. > > > > For the above reason, I was not really sure to mark the patch as > > RFC or > > not... Anyways, it still feels like a valid "protection" to have > > rather > > then just aborting (even if it does not really make sense to ever > > truncate the flash in devicetree). > > > > drivers/mtd/cfi_flash.c | 10 ++++++++-- > > include/flash.h | 1 + > > 2 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > > index f378f6fb6139..432d0d5c9ecb 100644 > > --- a/drivers/mtd/cfi_flash.c > > +++ b/drivers/mtd/cfi_flash.c > > @@ -2196,7 +2196,11 @@ ulong flash_get_size(phys_addr_t base, int > > banknum) > > /* multiply the size by the number of chips */ > > info->size *= size_ratio; > > max_size = cfi_flash_bank_size(banknum); > > - if (max_size && info->size > max_size) { > > + if (max_size) > > + max_size = min(info->addr_size, max_size); > > + else > > + max_size = info->addr_size; > > + if (info->size > max_size) { > > debug("[truncated from %ldMiB]", info->size > > >> 20); > > info->size = max_size; > > } > > @@ -2492,15 +2496,17 @@ unsigned long flash_init(void) > > static int cfi_flash_probe(struct udevice *dev) > > { > > fdt_addr_t addr; > > + fdt_size_t size; > > int idx; > > > > for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { > > - addr = dev_read_addr_index(dev, idx); > > + addr = dev_read_addr_size_index(dev, idx, &size); > > if (addr == FDT_ADDR_T_NONE) > > break; > > > > flash_info[cfi_flash_num_flash_banks].dev = dev; > > flash_info[cfi_flash_num_flash_banks].base = addr; > > + flash_info[cfi_flash_num_flash_banks].addr_size = > > size; > > cfi_flash_num_flash_banks++; > > } > > gd->bd->bi_flashstart = flash_info[0].base; > > diff --git a/include/flash.h b/include/flash.h > > index 95992fa689bb..3710a2731b76 100644 > > --- a/include/flash.h > > +++ b/include/flash.h > > @@ -46,6 +46,7 @@ typedef struct { > > #ifdef CONFIG_CFI_FLASH /* DM-specific > > parts */ > > struct udevice *dev; > > phys_addr_t base; > > + phys_size_t addr_size; > > #endif > > } flash_info_t; > > > > Running a CI world build leads to many errors. Here an example: > > $ make integratorcp_cm926ejs_defconfig > $ make -sj > In file included from include/linux/bitops.h:22, > from include/log.h:15, > from include/linux/printk.h:4, > from include/common.h:20, > from drivers/mtd/cfi_flash.c:19: > drivers/mtd/cfi_flash.c: In function 'flash_get_size': > drivers/mtd/cfi_flash.c:2200:44: error: 'flash_info_t' has no member > named 'addr_size' > 2200 | max_size = min(info->addr_size, > max_size); > | ^~ > include/linux/kernel.h:182:16: note: in definition of macro 'min' > 182 | typeof(x) _min1 = (x); \ > | ^ > drivers/mtd/cfi_flash.c:2200:44: error: 'flash_info_t' has no member > named 'addr_size' > 2200 | max_size = min(info->addr_size, > max_size); > | ^~ > include/linux/kernel.h:182:28: note: in definition of macro 'min' > 182 | typeof(x) _min1 = (x); \ > | ^ > include/linux/kernel.h:184:24: warning: comparison of distinct > pointer > types lacks a cast > 184 | (void) (&_min1 == &_min2); \ > | ^~ > drivers/mtd/cfi_flash.c:2200:36: note: in expansion of macro 'min' > 2200 | max_size = min(info->addr_size, > max_size); > | ^~~ > drivers/mtd/cfi_flash.c:2202:40: error: 'flash_info_t' has no member > named 'addr_size' > 2202 | max_size = info->addr_size; > | ^~ > make[2]: *** [scripts/Makefile.build:257: drivers/mtd/cfi_flash.o] > Error 1 > make[1]: *** [scripts/Makefile.build:397: drivers/mtd] Error 2 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1850: drivers] Error 2 > make: *** Waiting for unfinished jobs.... > > Could you please take a look and make sure that v2 successfully runs > a world build?
Ahh crap, I did compiled it but for a microblaze config which defines CONFIG_CFI_FLASH. I failed to spot that... One question though... I see that cfi_flash_bank_addr() is mutually exclusive with CFG_SYS_FLASH_BANKS_LIST in the sense we get one or another. In my patch, my approach was that we could get the size from devicetree and also from cfi_flash_bank_size() and thus, I was taking the minimum. But, is that correct? Or can I have the same approach as for the addr? Something like: #ifdef CONFIG_CFI_FLASH /* for driver model */ unsigned long cfi_flash_bank_size(int i) { return flash_info[i].addr_size; } #else __weak unsigned long cfi_flash_bank_size(int i) { #ifdef CFG_SYS_FLASH_BANKS_SIZES return ((unsigned long [])CFG_SYS_FLASH_BANKS_SIZES)[i]; #else return 0; #endif } #endif > Maybe also changing unsigned long to phys_size_t? Does the above makes sense? It would simplify things. Thanks! - Nuno Sá