[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[Heinrich Schuchardt]

dev_read_size_cells() and dev_read_addr_cells() do not walk up the device
tree to find the number of cells. On error they return 1 and 2
respectively. On qemu_arm64_defconfig this leads to the incorrect detection
of address of the second flash bank as 0x400000000000000 instead of
0x4000000.

When running

    qemu-system-aarch64 -machine virt -bios u-boot.bin \
    -cpu cortex-a53 -nographic \
    -drive if=pflash,format=raw,index=1,file=envstore.img

the command 'saveenv' fails with

    Saving Environment to Flash... Error: start and/or end address not on
    sector boundary
    Error: start and/or end address not on sector boundary
    Failed (1)

due to this incorrect address.

Use function fdtdec_get_addr_size_auto_noparent() to read the array of
flash banks from the device tree.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/mtd/cfi_flash.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index b7289ba539..dfa104bcf0 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -2469,28 +2469,24 @@ unsigned long flash_init(void)
 static int cfi_flash_probe(struct udevice *dev)
 {
 	const fdt32_t *cell;
-	int addrc, sizec;
-	int len, idx;
-
-	addrc = dev_read_addr_cells(dev);
-	sizec = dev_read_size_cells(dev);
+	int len;

 	/* 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) {
+
+	for (cfi_flash_num_flash_banks = 0; ; ++cfi_flash_num_flash_banks) {
 		phys_addr_t addr;

-		addr = dev_translate_address(dev, cell + idx);
+		addr = fdtdec_get_addr_size_auto_noparent(
+				gd->fdt_blob, dev_of_offset(dev), "reg",
+				cfi_flash_num_flash_banks, NULL, false);
+		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;

--
2.27.0

[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[Stefan Roese]

On 21.07.20 04:51, Heinrich Schuchardt wrote:
> dev_read_size_cells() and dev_read_addr_cells() do not walk up the device
> tree to find the number of cells. On error they return 1 and 2
> respectively. On qemu_arm64_defconfig this leads to the incorrect detection
> of address of the second flash bank as 0x400000000000000 instead of
> 0x4000000.
> 
> When running
> 
>      qemu-system-aarch64 -machine virt -bios u-boot.bin \
>      -cpu cortex-a53 -nographic \
>      -drive if=pflash,format=raw,index=1,file=envstore.img
> 
> the command 'saveenv' fails with
> 
>      Saving Environment to Flash... Error: start and/or end address not on
>      sector boundary
>      Error: start and/or end address not on sector boundary
>      Failed (1)
> 
> due to this incorrect address.
> 
> Use function fdtdec_get_addr_size_auto_noparent() to read the array of
> flash banks from the device tree.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   drivers/mtd/cfi_flash.c | 20 ++++++++------------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index b7289ba539..dfa104bcf0 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -2469,28 +2469,24 @@ unsigned long flash_init(void)
>   static int cfi_flash_probe(struct udevice *dev)
>   {
>   	const fdt32_t *cell;
> -	int addrc, sizec;
> -	int len, idx;
> -
> -	addrc = dev_read_addr_cells(dev);
> -	sizec = dev_read_size_cells(dev);
> +	int len;
> 
>   	/* 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) {
> +
> +	for (cfi_flash_num_flash_banks = 0; ; ++cfi_flash_num_flash_banks) {
>   		phys_addr_t addr;

Please use fdt_addr_t here when switching to the different API.

> 
> -		addr = dev_translate_address(dev, cell + idx);
> +		addr = fdtdec_get_addr_size_auto_noparent(
> +				gd->fdt_blob, dev_of_offset(dev), "reg",
> +				cfi_flash_num_flash_banks, NULL, false);
> +		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 does not work on my MIPS 64bit Octeon platform, I'm afraid. Using
this patch, fdtdec_get_addr_size_auto_noparent() returns FDT_ADDR_T_NONE
upon the first and only CFI flash bank. Here my platform:

https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/mips/dts/mrvl,octeon-ebb7304.dts#L72

I didn't look closely yet - perhaps you have a quick idea whats going
wrong.

Thanks,
Stefan

[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[Heinrich Schuchardt]

On 22.07.20 08:21, Stefan Roese wrote:
> On 21.07.20 04:51, Heinrich Schuchardt wrote:
>> dev_read_size_cells() and dev_read_addr_cells() do not walk up the device
>> tree to find the number of cells. On error they return 1 and 2
>> respectively. On qemu_arm64_defconfig this leads to the incorrect
>> detection
>> of address of the second flash bank as 0x400000000000000 instead of
>> 0x4000000.
>>
>> When running
>>
>> ???? qemu-system-aarch64 -machine virt -bios u-boot.bin \
>> ???? -cpu cortex-a53 -nographic \
>> ???? -drive if=pflash,format=raw,index=1,file=envstore.img
>>
>> the command 'saveenv' fails with
>>
>> ???? Saving Environment to Flash... Error: start and/or end address
>> not on
>> ???? sector boundary
>> ???? Error: start and/or end address not on sector boundary
>> ???? Failed (1)
>>
>> due to this incorrect address.
>>
>> Use function fdtdec_get_addr_size_auto_noparent() to read the array of
>> flash banks from the device tree.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> ? drivers/mtd/cfi_flash.c | 20 ++++++++------------
>> ? 1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
>> index b7289ba539..dfa104bcf0 100644
>> --- a/drivers/mtd/cfi_flash.c
>> +++ b/drivers/mtd/cfi_flash.c
>> @@ -2469,28 +2469,24 @@ unsigned long flash_init(void)
>> ? static int cfi_flash_probe(struct udevice *dev)
>> ? {
>> ????? const fdt32_t *cell;
>> -??? int addrc, sizec;
>> -??? int len, idx;
>> -
>> -??? addrc = dev_read_addr_cells(dev);
>> -??? sizec = dev_read_size_cells(dev);
>> +??? int len;
>>
>> ????? /* 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) {
>> +
>> +??? for (cfi_flash_num_flash_banks = 0; ; ++cfi_flash_num_flash_banks) {
>> ????????? phys_addr_t addr;
>
> Please use fdt_addr_t here when switching to the different API.

Why do you want to change it?

We are writing the information to
flash_info[cfi_flash_num_flash_banks].base. flash_info_t.base is of type
phys_addr_t.

Furthermore the types are anyway the same:
include/fdtdec.h:24:typedef phys_addr_t fdt_addr_t;

>
>>
>> -??????? addr = dev_translate_address(dev, cell + idx);
>> +??????? addr = fdtdec_get_addr_size_auto_noparent(
>> +??????????????? gd->fdt_blob, dev_of_offset(dev), "reg",
>> +??????????????? cfi_flash_num_flash_banks, NULL, false);
>> +??????? 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 does not work on my MIPS 64bit Octeon platform, I'm afraid. Using
> this patch, fdtdec_get_addr_size_auto_noparent() returns FDT_ADDR_T_NONE
> upon the first and only CFI flash bank. Here my platform:
>
> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/mips/dts/mrvl,octeon-ebb7304.dts#L72

Thanks for testing. I will set up a test scenario where I can debug what
is happening here.

Best regards

Heinrich

>
>
> I didn't look closely yet - perhaps you have a quick idea whats going
> wrong.
>
> Thanks,
> Stefan

[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[Rick Chen]

Hi Heinrich

> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Heinrich Schuchardt
> Sent: Tuesday, July 21, 2020 10:51 AM
> To: Stefan Roese
> Cc: Simon Glass; u-boot at lists.denx.de; Heinrich Schuchardt
> Subject: [PATCH 1/1] mtd: cfi_flash: read device tree correctly
>
> dev_read_size_cells() and dev_read_addr_cells() do not walk up the device tree to find the number of cells. On error they return 1 and 2 respectively. On qemu_arm64_defconfig this leads to the incorrect detection of address of the second flash bank as 0x400000000000000 instead of 0x4000000.
>
> When running
>
>     qemu-system-aarch64 -machine virt -bios u-boot.bin \
>     -cpu cortex-a53 -nographic \
>     -drive if=pflash,format=raw,index=1,file=envstore.img
>
> the command 'saveenv' fails with
>
>     Saving Environment to Flash... Error: start and/or end address not on
>     sector boundary
>     Error: start and/or end address not on sector boundary
>     Failed (1)
>
> due to this incorrect address.
>
> Use function fdtdec_get_addr_size_auto_noparent() to read the array of flash banks from the device tree.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/mtd/cfi_flash.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba539..dfa104bcf0 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -2469,28 +2469,24 @@ unsigned long flash_init(void)  static int cfi_flash_probe(struct udevice *dev)  {
>         const fdt32_t *cell;
> -       int addrc, sizec;
> -       int len, idx;
> -
> -       addrc = dev_read_addr_cells(dev);
> -       sizec = dev_read_size_cells(dev);
> +       int len;
>
>         /* 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) {
> +
> +       for (cfi_flash_num_flash_banks = 0; ; ++cfi_flash_num_flash_banks) {
>                 phys_addr_t addr;
>
> -               addr = dev_translate_address(dev, cell + idx);
> +               addr = fdtdec_get_addr_size_auto_noparent(
> +                               gd->fdt_blob, dev_of_offset(dev), "reg",
> +                               cfi_flash_num_flash_banks, NULL, false);
> +               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;
>
> --
> 2.27.0
>

This patch remind me that I have encounter flash bank detection
problem on AE350 platform a period time ago.
And have commit a patch to work around this problem as below:

commit cca8b1e5b20cdab7299a5ee7139e70783f73ccdf

    riscv: dts: Add #address-cells and #size-cells in nor node

    Those are required for cfi-flash driver to get correct address information.
    Also modify size description correctly.

With this patch, there is unnecessary to re-declaration address-cells
and size-cells in nor node indeed.

Tested-by: Rick Chen <rick@andestech.com>

Thanks,
Rick

[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[Heinrich Schuchardt]

On 24.07.20 11:14, Rick Chen wrote:
> Hi Heinrich
>
>> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Heinrich Schuchardt
>> Sent: Tuesday, July 21, 2020 10:51 AM
>> To: Stefan Roese
>> Cc: Simon Glass; u-boot at lists.denx.de; Heinrich Schuchardt
>> Subject: [PATCH 1/1] mtd: cfi_flash: read device tree correctly
>>
>> dev_read_size_cells() and dev_read_addr_cells() do not walk up the device tree to find the number of cells. On error they return 1 and 2 respectively. On qemu_arm64_defconfig this leads to the incorrect detection of address of the second flash bank as 0x400000000000000 instead of 0x4000000.
>>
>> When running
>>
>>     qemu-system-aarch64 -machine virt -bios u-boot.bin \
>>     -cpu cortex-a53 -nographic \
>>     -drive if=pflash,format=raw,index=1,file=envstore.img
>>
>> the command 'saveenv' fails with
>>
>>     Saving Environment to Flash... Error: start and/or end address not on
>>     sector boundary
>>     Error: start and/or end address not on sector boundary
>>     Failed (1)
>>
>> due to this incorrect address.
>>
>> Use function fdtdec_get_addr_size_auto_noparent() to read the array of flash banks from the device tree.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  drivers/mtd/cfi_flash.c | 20 ++++++++------------
>>  1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba539..dfa104bcf0 100644
>> --- a/drivers/mtd/cfi_flash.c
>> +++ b/drivers/mtd/cfi_flash.c
>> @@ -2469,28 +2469,24 @@ unsigned long flash_init(void)  static int cfi_flash_probe(struct udevice *dev)  {
>>         const fdt32_t *cell;
>> -       int addrc, sizec;
>> -       int len, idx;
>> -
>> -       addrc = dev_read_addr_cells(dev);
>> -       sizec = dev_read_size_cells(dev);
>> +       int len;
>>
>>         /* 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) {
>> +
>> +       for (cfi_flash_num_flash_banks = 0; ; ++cfi_flash_num_flash_banks) {
>>                 phys_addr_t addr;
>>
>> -               addr = dev_translate_address(dev, cell + idx);
>> +               addr = fdtdec_get_addr_size_auto_noparent(
>> +                               gd->fdt_blob, dev_of_offset(dev), "reg",
>> +                               cfi_flash_num_flash_banks, NULL, false);
>> +               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;
>>
>> --
>> 2.27.0
>>
>
> This patch remind me that I have encounter flash bank detection
> problem on AE350 platform a period time ago.
> And have commit a patch to work around this problem as below:
>

> commit cca8b1e5b20cdab7299a5ee7139e70783f73ccdf
>
>     riscv: dts: Add #address-cells and #size-cells in nor node
>
>     Those are required for cfi-flash driver to get correct address information.
>     Also modify size description correctly.
>
> With this patch, there is unnecessary to re-declaration address-cells
> and size-cells in nor node indeed.
>
> Tested-by: Rick Chen <rick@andestech.com>
>
> Thanks,
> Rick
>

Dear Stefan, dear Rick,

thanks for testing on different systems.

The reason for the different test results is the usage of CONFIG_OF_LIVE:

CONFIG_OF_LIVE=y
* octeon_ebb7304_defconfig

CONFIG_OF_LIVE=n
* ae350_rv32_defconfig
* qemu_arm64_defconfig

dev_translate_address() behaves differently depending on the usage of a
live tree.

I will send a revised patch that only changes the behavior only for the
CONFIG_OF_LIVE=n case after testing qemu_arm64_defconfig both with and
without live tree.

Best regards

Heinrich

[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[Stefan Roese]

Hi Heinrich,

(added Simon to Cc)

On 24.07.20 18:34, Heinrich Schuchardt wrote:
> On 24.07.20 11:14, Rick Chen wrote:
>> Hi Heinrich
>>
>>> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Heinrich Schuchardt
>>> Sent: Tuesday, July 21, 2020 10:51 AM
>>> To: Stefan Roese
>>> Cc: Simon Glass; u-boot at lists.denx.de; Heinrich Schuchardt
>>> Subject: [PATCH 1/1] mtd: cfi_flash: read device tree correctly
>>>
>>> dev_read_size_cells() and dev_read_addr_cells() do not walk up the device tree to find the number of cells. On error they return 1 and 2 respectively. On qemu_arm64_defconfig this leads to the incorrect detection of address of the second flash bank as 0x400000000000000 instead of 0x4000000.
>>>
>>> When running
>>>
>>>      qemu-system-aarch64 -machine virt -bios u-boot.bin \
>>>      -cpu cortex-a53 -nographic \
>>>      -drive if=pflash,format=raw,index=1,file=envstore.img
>>>
>>> the command 'saveenv' fails with
>>>
>>>      Saving Environment to Flash... Error: start and/or end address not on
>>>      sector boundary
>>>      Error: start and/or end address not on sector boundary
>>>      Failed (1)
>>>
>>> due to this incorrect address.
>>>
>>> Use function fdtdec_get_addr_size_auto_noparent() to read the array of flash banks from the device tree.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>   drivers/mtd/cfi_flash.c | 20 ++++++++------------
>>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba539..dfa104bcf0 100644
>>> --- a/drivers/mtd/cfi_flash.c
>>> +++ b/drivers/mtd/cfi_flash.c
>>> @@ -2469,28 +2469,24 @@ unsigned long flash_init(void)  static int cfi_flash_probe(struct udevice *dev)  {
>>>          const fdt32_t *cell;
>>> -       int addrc, sizec;
>>> -       int len, idx;
>>> -
>>> -       addrc = dev_read_addr_cells(dev);
>>> -       sizec = dev_read_size_cells(dev);
>>> +       int len;
>>>
>>>          /* 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) {
>>> +
>>> +       for (cfi_flash_num_flash_banks = 0; ; ++cfi_flash_num_flash_banks) {
>>>                  phys_addr_t addr;
>>>
>>> -               addr = dev_translate_address(dev, cell + idx);
>>> +               addr = fdtdec_get_addr_size_auto_noparent(
>>> +                               gd->fdt_blob, dev_of_offset(dev), "reg",
>>> +                               cfi_flash_num_flash_banks, NULL, false);
>>> +               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;
>>>
>>> --
>>> 2.27.0
>>>
>>
>> This patch remind me that I have encounter flash bank detection
>> problem on AE350 platform a period time ago.
>> And have commit a patch to work around this problem as below:
>>
> 
>> commit cca8b1e5b20cdab7299a5ee7139e70783f73ccdf
>>
>>      riscv: dts: Add #address-cells and #size-cells in nor node
>>
>>      Those are required for cfi-flash driver to get correct address information.
>>      Also modify size description correctly.
>>
>> With this patch, there is unnecessary to re-declaration address-cells
>> and size-cells in nor node indeed.
>>
>> Tested-by: Rick Chen <rick@andestech.com>
>>
>> Thanks,
>> Rick
>>
> 
> Dear Stefan, dear Rick,
> 
> thanks for testing on different systems.
> 
> The reason for the different test results is the usage of CONFIG_OF_LIVE:
> 
> CONFIG_OF_LIVE=y
> * octeon_ebb7304_defconfig
> 
> CONFIG_OF_LIVE=n
> * ae350_rv32_defconfig
> * qemu_arm64_defconfig
> 
> dev_translate_address() behaves differently depending on the usage of a
> live tree.
> 
> I will send a revised patch that only changes the behavior only for the
> CONFIG_OF_LIVE=n case after testing qemu_arm64_defconfig both with and
> without live tree.

Thanks for looking into this. But I wonder, if it makes more sense to
change the OF_LIVE implementation so that it matches the "non-OF_LIVE"
version? We should strive to have both implementations achieving the
same results I think.

Or am I missing something?

Thanks,
Stefan

[PATCH 1/1] mtd: cfi_flash: read device tree correctly

[Simon Glass]

Hi Stefan,

On Sat, 25 Jul 2020 at 05:47, Stefan Roese <sr@denx.de> wrote:
>
> Hi Heinrich,
>
> (added Simon to Cc)
>
> On 24.07.20 18:34, Heinrich Schuchardt wrote:
> > On 24.07.20 11:14, Rick Chen wrote:
> >> Hi Heinrich
> >>
> >>> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Heinrich Schuchardt
> >>> Sent: Tuesday, July 21, 2020 10:51 AM
> >>> To: Stefan Roese
> >>> Cc: Simon Glass; u-boot at lists.denx.de; Heinrich Schuchardt
> >>> Subject: [PATCH 1/1] mtd: cfi_flash: read device tree correctly
> >>>
> >>> dev_read_size_cells() and dev_read_addr_cells() do not walk up the device tree to find the number of cells. On error they return 1 and 2 respectively. On qemu_arm64_defconfig this leads to the incorrect detection of address of the second flash bank as 0x400000000000000 instead of 0x4000000.
> >>>
> >>> When running
> >>>
> >>>      qemu-system-aarch64 -machine virt -bios u-boot.bin \
> >>>      -cpu cortex-a53 -nographic \
> >>>      -drive if=pflash,format=raw,index=1,file=envstore.img
> >>>
> >>> the command 'saveenv' fails with
> >>>
> >>>      Saving Environment to Flash... Error: start and/or end address not on
> >>>      sector boundary
> >>>      Error: start and/or end address not on sector boundary
> >>>      Failed (1)
> >>>
> >>> due to this incorrect address.
> >>>
> >>> Use function fdtdec_get_addr_size_auto_noparent() to read the array of flash banks from the device tree.
> >>>
> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> ---
> >>>   drivers/mtd/cfi_flash.c | 20 ++++++++------------
> >>>   1 file changed, 8 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index b7289ba539..dfa104bcf0 100644
> >>> --- a/drivers/mtd/cfi_flash.c
> >>> +++ b/drivers/mtd/cfi_flash.c
> >>> @@ -2469,28 +2469,24 @@ unsigned long flash_init(void)  static int cfi_flash_probe(struct udevice *dev)  {
> >>>          const fdt32_t *cell;
> >>> -       int addrc, sizec;
> >>> -       int len, idx;
> >>> -
> >>> -       addrc = dev_read_addr_cells(dev);
> >>> -       sizec = dev_read_size_cells(dev);
> >>> +       int len;
> >>>
> >>>          /* 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) {
> >>> +
> >>> +       for (cfi_flash_num_flash_banks = 0; ; ++cfi_flash_num_flash_banks) {
> >>>                  phys_addr_t addr;
> >>>
> >>> -               addr = dev_translate_address(dev, cell + idx);
> >>> +               addr = fdtdec_get_addr_size_auto_noparent(
> >>> +                               gd->fdt_blob, dev_of_offset(dev), "reg",
> >>> +                               cfi_flash_num_flash_banks, NULL, false);
> >>> +               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;
> >>>
> >>> --
> >>> 2.27.0
> >>>
> >>
> >> This patch remind me that I have encounter flash bank detection
> >> problem on AE350 platform a period time ago.
> >> And have commit a patch to work around this problem as below:
> >>
> >
> >> commit cca8b1e5b20cdab7299a5ee7139e70783f73ccdf
> >>
> >>      riscv: dts: Add #address-cells and #size-cells in nor node
> >>
> >>      Those are required for cfi-flash driver to get correct address information.
> >>      Also modify size description correctly.
> >>
> >> With this patch, there is unnecessary to re-declaration address-cells
> >> and size-cells in nor node indeed.
> >>
> >> Tested-by: Rick Chen <rick@andestech.com>
> >>
> >> Thanks,
> >> Rick
> >>
> >
> > Dear Stefan, dear Rick,
> >
> > thanks for testing on different systems.
> >
> > The reason for the different test results is the usage of CONFIG_OF_LIVE:
> >
> > CONFIG_OF_LIVE=y
> > * octeon_ebb7304_defconfig
> >
> > CONFIG_OF_LIVE=n
> > * ae350_rv32_defconfig
> > * qemu_arm64_defconfig
> >
> > dev_translate_address() behaves differently depending on the usage of a
> > live tree.
> >
> > I will send a revised patch that only changes the behavior only for the
> > CONFIG_OF_LIVE=n case after testing qemu_arm64_defconfig both with and
> > without live tree.
>
> Thanks for looking into this. But I wonder, if it makes more sense to
> change the OF_LIVE implementation so that it matches the "non-OF_LIVE"
> version? We should strive to have both implementations achieving the
> same results I think.
>
> Or am I missing something?

Yes we should, and also add tests to catch the difference.

Regards,
Simon