* [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
[parent not found: <752D002CFF5D0F4FA35C0100F1D73F3FA472AD72@ATCPCS16.andestech.com>]
* [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.