[PATCH] cfi_flash: Fix devicetree address determination

["André Przywara" <andre.przywara@arm.com>]

On 23/09/2020 06:26, Stefan Roese wrote:
> Hi Simon,
> 
> On 22.09.20 15:51, Simon Glass wrote:
>> Hi Stefan,
>>
>> On Mon, 21 Sep 2020 at 07:28, Stefan Roese <sr@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@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);

Ouch, sorry for that!

One thing I noticed: Technically this fix is no longer needed, since
Heinrich's patch ae6b33dcc342 ("dm: fix ofnode_read_addr/size_cells()")
recently fixed that particular issue (I missed that one when doing the
bisect earlier).

However I still consider this patch here useful, since it removes code
duplication (and the original bug gives a good rationale for that!).
So I will repost this one here, but leave it up to you whether to merge
it or not.

Also: this function was the only user of dev_read_{addr,size}_cells().
Shall we consequently remove them? They have this somewhat surprising
feature of querying the parent now, which prevents them from being used
when someone want to determine the current #a-c and #s-c applicable for
subnodes, for instance.

> 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.
That sounds useful, but can this be done easily?

Cheers,
Andre