[PATCH] cfi_flash: Fix devicetree address determination

[Simon Glass]

Hi Stefan,

On Mon, 21 Sep 2020 at 07:28, Stefan Roese <sr at denx.de> wrote:
>
> Hi Andre,
>
> (added Simon)
>
> On 18.09.20 19:45, 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 (outside of EL1), which was crashing due to
> > decoding a wrong start 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 at arm.com>
> > ---
> >   drivers/mtd/cfi_flash.c | 25 +++++++------------------
> >   1 file changed, 7 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> > index b7289ba5394..656ff326e17 100644
> > --- a/drivers/mtd/cfi_flash.c
> > +++ b/drivers/mtd/cfi_flash.c
> > @@ -2468,29 +2468,18 @@ 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;
> > +     fdt_size_t size;
> > +     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 = devfdt_get_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;
> >               cfi_flash_num_flash_banks++;
> > -
> > -             idx += addrc + sizec;
> >       }
> >       gd->bd->bi_flashstart = flash_info[0].base;
> >
> >
>
> This fails on my Octeon MIPS64 platform "octeon_ebb7304". I did some
> debugging and found that here "of_offset" is a 64 bit value (type long)
> which gets truncated in dev_of_offset() to 32 bit (type int).
>
> This problem only arises when of_live_active() is set. Here, "of_offset"
> holds a pointer AFACT and truncating it to 32 bits breaks things.
>
> I'm wondering why this did not hit me earlier on this 64bit platform.
> Simon, do you have a quick idea how to solve this?

Well I don't think ofnode should use long for of_offset, since int
should be enough.

ofnode_to_offset() converts an ofnode to a DT offset but only if it is
not using livetree. With livetree there are no offsets so this is not
going to work. If you define OF_CHECKS you will see that.

Note that an ofnode can either hold a pointer or an offset. There are
detailed comments on ofnode_union to explain how it is supposed to
work.

This patch looks correct to me, but perhaps there is something else going on?

Regards,
Simon