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.przyw...@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? Best regards Heinrich > > Thanks, > Stefan