Hi Nuno Sá,
On 4/14/23 09:37, Nuno Sá wrote:
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?
Frankly, I've not looked into the CFI flash code for quite some time
so my memory is a bit foggy here. ;) Additionally I don't even have a
board using parallel CFI flash here any more.
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
Looks reasonable to me.
Maybe also changing unsigned long to phys_size_t?
Yes, also a good idea. Please in a separate patch though.
Does the above makes sense? It would simplify things.
Ack.
Thanks,
Stefan
