[PATCH v2] cfi_flash: Fix devicetree address determination

[Stefan Roese]

On 08.10.20 12:15, Heinrich Schuchardt wrote:
> 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?

All this CFI code is pretty ancient. Many years before devicetree was
introduced to U-Boot. IIRC, there were many boards that could be
equipped with different CFI flash chips (different sizes). That's were
this flash_get_size() etc comes from.

> Do we need
> CONFIG_SYS_FLASH_BANKS_SIZES and weak function cfi_flash_bank_size() at all?

I agree that this code could benefit from some "cleanup", removing
some of the old legacy stuff. Perhaps I will find some time to tackle
this in the near future. But if someone beats me here, I would not
be too disappointed. ;)

Thanks,
Stefan