diff mbox series

[v2] cfi_flash: Fix devicetree address determination

Message ID 20200923232204.24466-1-andre.przywara@arm.com
State Awaiting Upstream
Delegated to: Stefan Roese
Headers show
Series [v2] cfi_flash: Fix devicetree address determination | expand

Commit Message

André Przywara Sept. 23, 2020, 11:22 p.m. UTC
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@arm.com>
---
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(-)

Comments

Stefan Roese Sept. 24, 2020, 6:05 a.m. UTC | #1
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@arm.com>
> ---
> Changes v1 .. v2:
> - Use live tree compatible function
> - Drop unneeded size variable

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

>   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);
> +		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;
>   
> 


Viele Grüße,
Stefan
Stefan Roese Oct. 8, 2020, 7:08 a.m. UTC | #2
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@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);
> +		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;
>   
> 


Viele Grüße,
Stefan
Heinrich Schuchardt Oct. 8, 2020, 8:39 a.m. UTC | #3
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@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);

Best regards

Heinrich

>> +        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;
>>  
>
>
> Viele Grüße,
> Stefan
>
Stefan Roese Oct. 8, 2020, 9:49 a.m. UTC | #4
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@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.

Thanks,
Stefan
Heinrich Schuchardt Oct. 8, 2020, 10:15 a.m. UTC | #5
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@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
André Przywara Oct. 8, 2020, 10:27 a.m. UTC | #6
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@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
Stefan Roese Oct. 8, 2020, 10:42 a.m. UTC | #7
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@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
diff mbox series

Patch

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);
+		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;