[PATCH] mtd: cfi: respect reg address length
Nuno Sá
noname.nuno at gmail.com
Fri Apr 14 09:37:36 CEST 2023
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.sa at 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á
More information about the U-Boot
mailing list