* [PATCH 1/1] mtd: cfi_flash: read device tree correctly
@ 2020-07-21 2:51 Heinrich Schuchardt
2020-07-22 6:21 ` Stefan Roese
[not found] ` <752D002CFF5D0F4FA35C0100F1D73F3FA472AD72@ATCPCS16.andestech.com>
0 siblings, 2 replies; 7+ messages in thread
From: Heinrich Schuchardt @ 2020-07-21 2:51 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 1/1] mtd: cfi_flash: read device tree correctly
2020-07-21 2:51 [PATCH 1/1] mtd: cfi_flash: read device tree correctly Heinrich Schuchardt
@ 2020-07-22 6:21 ` Stefan Roese
2020-07-22 7:34 ` Heinrich Schuchardt
[not found] ` <752D002CFF5D0F4FA35C0100F1D73F3FA472AD72@ATCPCS16.andestech.com>
1 sibling, 1 reply; 7+ messages in thread
From: Stefan Roese @ 2020-07-22 6:21 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] mtd: cfi_flash: read device tree correctly
2020-07-22 6:21 ` Stefan Roese
@ 2020-07-22 7:34 ` Heinrich Schuchardt
0 siblings, 0 replies; 7+ messages in thread
From: Heinrich Schuchardt @ 2020-07-22 7:34 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] mtd: cfi_flash: read device tree correctly
[not found] ` <752D002CFF5D0F4FA35C0100F1D73F3FA472AD72@ATCPCS16.andestech.com>
@ 2020-07-24 9:14 ` Rick Chen
2020-07-24 16:34 ` Heinrich Schuchardt
0 siblings, 1 reply; 7+ messages in thread
From: Rick Chen @ 2020-07-24 9:14 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] mtd: cfi_flash: read device tree correctly
2020-07-24 9:14 ` Rick Chen
@ 2020-07-24 16:34 ` Heinrich Schuchardt
2020-07-25 11:46 ` Stefan Roese
0 siblings, 1 reply; 7+ messages in thread
From: Heinrich Schuchardt @ 2020-07-24 16:34 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] mtd: cfi_flash: read device tree correctly
2020-07-24 16:34 ` Heinrich Schuchardt
@ 2020-07-25 11:46 ` Stefan Roese
2020-07-26 14:54 ` Simon Glass
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Roese @ 2020-07-25 11:46 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] mtd: cfi_flash: read device tree correctly
2020-07-25 11:46 ` Stefan Roese
@ 2020-07-26 14:54 ` Simon Glass
0 siblings, 0 replies; 7+ messages in thread
From: Simon Glass @ 2020-07-26 14:54 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-26 14:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 2:51 [PATCH 1/1] mtd: cfi_flash: read device tree correctly Heinrich Schuchardt
2020-07-22 6:21 ` Stefan Roese
2020-07-22 7:34 ` Heinrich Schuchardt
[not found] ` <752D002CFF5D0F4FA35C0100F1D73F3FA472AD72@ATCPCS16.andestech.com>
2020-07-24 9:14 ` Rick Chen
2020-07-24 16:34 ` Heinrich Schuchardt
2020-07-25 11:46 ` Stefan Roese
2020-07-26 14:54 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.