Hi Stefan, On Sat, 25 Jul 2020 at 05:47, Stefan Roese <s...@denx.de> wrote: > > Hi Heinrich, > > (added Simon to Cc) > > On 24.07.20 18:34, Heinrich Schuchardt wrote: > > On 24.07.20 11:14, Rick Chen wrote: > >> Hi Heinrich > >> > >>> From: U-Boot [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Heinrich > >>> Schuchardt > >>> Sent: Tuesday, July 21, 2020 10:51 AM > >>> To: Stefan Roese > >>> Cc: Simon Glass; u-boot@lists.denx.de; Heinrich Schuchardt > >>> Subject: [PATCH 1/1] mtd: cfi_flash: read device tree correctly > >>> > >>> dev_read_size_cells() and dev_read_addr_cells() do not walk up the device > >>> tree to find the number of cells. On error they return 1 and 2 > >>> respectively. On qemu_arm64_defconfig this leads to the incorrect > >>> detection of address of the second flash bank as 0x400000000000000 > >>> instead of 0x4000000. > >>> > >>> When running > >>> > >>> qemu-system-aarch64 -machine virt -bios u-boot.bin \ > >>> -cpu cortex-a53 -nographic \ > >>> -drive if=pflash,format=raw,index=1,file=envstore.img > >>> > >>> the command 'saveenv' fails with > >>> > >>> Saving Environment to Flash... Error: start and/or end address not on > >>> sector boundary > >>> Error: start and/or end address not on sector boundary > >>> Failed (1) > >>> > >>> due to this incorrect address. > >>> > >>> Use function fdtdec_get_addr_size_auto_noparent() to read the array of > >>> flash banks from the device tree. > >>> > >>> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de> > >>> --- > >>> drivers/mtd/cfi_flash.c | 20 ++++++++------------ > >>> 1 file changed, 8 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index > >>> b7289ba539..dfa104bcf0 100644 > >>> --- a/drivers/mtd/cfi_flash.c > >>> +++ b/drivers/mtd/cfi_flash.c > >>> @@ -2469,28 +2469,24 @@ unsigned long flash_init(void) static int > >>> cfi_flash_probe(struct udevice *dev) { > >>> const fdt32_t *cell; > >>> - int addrc, sizec; > >>> - int len, idx; > >>> - > >>> - addrc = dev_read_addr_cells(dev); > >>> - sizec = dev_read_size_cells(dev); > >>> + int len; > >>> > >>> /* 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) { > >>> + > >>> + for (cfi_flash_num_flash_banks = 0; ; > >>> ++cfi_flash_num_flash_banks) { > >>> phys_addr_t addr; > >>> > >>> - addr = dev_translate_address(dev, cell + idx); > >>> + addr = fdtdec_get_addr_size_auto_noparent( > >>> + gd->fdt_blob, dev_of_offset(dev), "reg", > >>> + cfi_flash_num_flash_banks, NULL, false); > >>> + 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; > >>> - cfi_flash_num_flash_banks++; > >>> - > >>> - idx += addrc + sizec; > >>> } > >>> gd->bd->bi_flashstart = flash_info[0].base; > >>> > >>> -- > >>> 2.27.0 > >>> > >> > >> This patch remind me that I have encounter flash bank detection > >> problem on AE350 platform a period time ago. > >> And have commit a patch to work around this problem as below: > >> > > > >> commit cca8b1e5b20cdab7299a5ee7139e70783f73ccdf > >> > >> riscv: dts: Add #address-cells and #size-cells in nor node > >> > >> Those are required for cfi-flash driver to get correct address > >> information. > >> Also modify size description correctly. > >> > >> With this patch, there is unnecessary to re-declaration address-cells > >> and size-cells in nor node indeed. > >> > >> Tested-by: Rick Chen <r...@andestech.com> > >> > >> Thanks, > >> Rick > >> > > > > Dear Stefan, dear Rick, > > > > thanks for testing on different systems. > > > > The reason for the different test results is the usage of CONFIG_OF_LIVE: > > > > CONFIG_OF_LIVE=y > > * octeon_ebb7304_defconfig > > > > CONFIG_OF_LIVE=n > > * ae350_rv32_defconfig > > * qemu_arm64_defconfig > > > > dev_translate_address() behaves differently depending on the usage of a > > live tree. > > > > I will send a revised patch that only changes the behavior only for the > > CONFIG_OF_LIVE=n case after testing qemu_arm64_defconfig both with and > > without live tree. > > Thanks for looking into this. But I wonder, if it makes more sense to > change the OF_LIVE implementation so that it matches the "non-OF_LIVE" > version? We should strive to have both implementations achieving the > same results I think. > > Or am I missing something?
Yes we should, and also add tests to catch the difference. Regards, Simon
