[PATCH] cfi_flash: Fix devicetree address determination

[Stefan Roese]

Hi Simon,

On 22.09.20 15:51, Simon Glass wrote:
> 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.

This does not work right now. I'll send a patch fixing compiling with
OF_CHECK enabled shortly.

> 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.

Right. Thanks for all the detailed infos in the header. The main issue
seems to be, that this CFI patch uses a function from fdtaddr.c
(devfdt_get_addr_size_index), which unconditionally uses dev_of_offset()
without checking if livetree is enabled or not. This breaks on my
64 bit platform (see below).

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

Making this change below, works for me:

-		addr = devfdt_get_addr_size_index(dev, idx, &size);
+		addr = dev_read_addr_index(dev, idx);

Maybe we should make sure, that all functions from fdtaddr.c are not
used with livetree active? To prevent similar issues using devfdt_foo()
functions with livetree active.

Thanks,
Stefan