On 08/10/2020 11:15, Heinrich Schuchardt wrote:
Hi,
On 08.10.20 11:49, Stefan Roese wrote:
On 08.10.20 10:39, Heinrich Schuchardt wrote:
On 08.10.20 09:08, Stefan Roese wrote:
On 24.09.20 01:22, Andre Przywara wrote:
The cfi-flash driver uses an open-coded version of the generic algorithm to decode and translate multiple frames of a "reg" property.
This starts off the wrong foot by using the address-cells and size-cells properties of *this* very node, and not of the parent. This somewhat happened to work back when we were using a wrong default size of 2, but broke about a year ago with commit 0ba41ce1b781 ("libfdt: return correct value if #size-cells property is not present").
Instead of fixing the reinvented wheel, just use the generic function that does all of this properly.
This fixes U-Boot on QEMU (-arm64), which was crashing due to decoding a wrong flash base address: DRAM: 1 GiB Flash: "Synchronous Abort" handler, esr 0x96000044 elr: 00000000000211dc lr : 00000000000211b0 (reloc) elr: 000000007ff5e1dc lr : 000000007ff5e1b0 x0 : 00000000000000f0 x1 : 000000007ff5e1d8 x2 : 000000007edfbc48 x3 : 0000000000000000 x4 : 0000000000000000 x5 : 00000000000000f0 x6 : 000000007edfbc2c x7 : 0000000000000000 x8 : 000000007ffd8d70 x9 : 000000000000000c x10: 0400000000000003 x11: 0000000000000055 ^^^^^^^^^^^^^^^^
Signed-off-by: Andre Przywara andre.przywara@arm.com
Applied to u-boot-cfi-flash/master
Thanks, Stefan
Changes v1 .. v2:
- Use live tree compatible function
- Drop unneeded size variable
drivers/mtd/cfi_flash.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba5394..9e3a652f445 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2468,29 +2468,17 @@ unsigned long flash_init(void) #ifdef CONFIG_CFI_FLASH /* for driver model */ static int cfi_flash_probe(struct udevice *dev) { - const fdt32_t *cell; - int addrc, sizec; - int len, idx; + fdt_addr_t addr; + int idx; - addrc = dev_read_addr_cells(dev); - sizec = dev_read_size_cells(dev);
- /* decode regs; there may be multiple reg tuples. */ - cell = dev_read_prop(dev, "reg", &len); - if (!cell) - return -ENOENT; - idx = 0; - len /= sizeof(fdt32_t); - while (idx < len) { - phys_addr_t addr;
- addr = dev_translate_address(dev, cell + idx); + for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { + addr = dev_read_addr_index(dev, idx);
Why don't you additionally read the size here to populate flash_info[].size?
fdt_size_t size; addr = dev_read_addr_size_index(dev, idx, &size);
It was not done before this either. IIRC, then the size is auto detected by querying the flash chip.
Do you see any issue without this? Feel free to send a follow up patch is something needs to get fixed / tuned.
Yes, there is a function flash_get_size().
I am not worried about this special patch but about the logic of the driver as a whole.
Why does function flash_get_size() consider CONFIG_SYS_FLASH_BANKS_SIZES but not the size information from the DTB? Do we need CONFIG_SYS_FLASH_BANKS_SIZES and weak function cfi_flash_bank_size() at all?
That's a good point: given that this symbol is in config_whitelist.txt, it's probably some legacy from the dawn of time.
Maybe for some hacks, as those lines in cfi_flash.c suggest: ============ max_size = cfi_flash_bank_size(banknum); if (max_size && info->size > max_size) { debug("[truncated from %ldMiB]", info->size >> 20); info->size = max_size; } ============
Maybe someone mounted a bigger flash chip than there was space in the MMIO frame?
But I totally agree that this is very confusing and should either be removed entirely (preferred, but would need to be tested on those boards using it) or extended to cover DTBs as well.
Cheers, Andre