[PATCH v2] cfi_flash: Fix devicetree address determination

[André Przywara]

On 08/10/2020 11:15, Heinrich Schuchardt wrote:

Hi,

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

That's a good point: given that this symbol is in config_whitelist.txt,
it's probably some legacy from the dawn of time.

Maybe for some hacks, as those lines in cfi_flash.c suggest:
============
	max_size = cfi_flash_bank_size(banknum);
	if (max_size && info->size > max_size) {
		debug("[truncated from %ldMiB]", info->size >> 20);
		info->size = max_size;
	}
============

Maybe someone mounted a bigger flash chip than there was space in the
MMIO frame?

But I totally agree that this is very confusing and should either be
removed entirely (preferred, but would need to be tested on those boards
using it) or extended to cover DTBs as well.

Cheers,
Andre