[PATCH v2] cfi_flash: Fix devicetree address determination

[Heinrich Schuchardt]

On 08.10.20 11:49, Stefan Roese wrote:
> On 08.10.20 10:39, Heinrich Schuchardt wrote:
>> On 08.10.20 09:08, Stefan Roese wrote:
>>> On 24.09.20 01:22, 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 (-arm64), which was crashing due to decoding
>>>> a wrong flash base 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>
>>>
>>> Applied to u-boot-cfi-flash/master
>>>
>>> Thanks,
>>> Stefan
>>>
>>>> ---
>>>> Changes v1 .. v2:
>>>> - Use live tree compatible function
>>>> - Drop unneeded size variable
>>>>
>>>>    drivers/mtd/cfi_flash.c | 24 ++++++------------------
>>>>    1 file changed, 6 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
>>>> index b7289ba5394..9e3a652f445 100644
>>>> --- a/drivers/mtd/cfi_flash.c
>>>> +++ b/drivers/mtd/cfi_flash.c
>>>> @@ -2468,29 +2468,17 @@ 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;
>>>> +    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 = dev_read_addr_index(dev, idx);
>>
>> Why don't you additionally read the size here to populate
>> flash_info[].size?
>>
>> fdt_size_t size;
>> addr = dev_read_addr_size_index(dev, idx, &size);
>
> It was not done before this either. IIRC, then the size is auto detected
> by querying the flash chip.
>
> Do you see any issue without this? Feel free to send a follow up patch
> is something needs to get fixed / tuned.

Yes, there is a function flash_get_size().

I am not worried about this special patch but about the logic of the
driver as a whole.

Why does function flash_get_size() consider CONFIG_SYS_FLASH_BANKS_SIZES
but not the size information from the DTB? Do we need
CONFIG_SYS_FLASH_BANKS_SIZES and weak function cfi_flash_bank_size() at all?

Best regards

Heinrich

>
> Thanks,
> Stefan