[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[Simon Glass]

Hi Stefan,

On Sat, 25 Jul 2020 at 05:47, Stefan Roese <sr at 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-bounces at lists.denx.de] On Behalf Of Heinrich Schuchardt
> >>> Sent: Tuesday, July 21, 2020 10:51 AM
> >>> To: Stefan Roese
> >>> Cc: Simon Glass; u-boot at 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.glpk at 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 <rick at 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