[PATCH v2 1/2] mtd: cfi: respect reg address length
flash_get_size() will get the flash size from the device itself and go through all erase regions to read protection status. However, the device mappable region (eg: devicetree reg property) might be lower than the device full size which means that the above cycle will result in a data bus exception. This change fixes it by reading the 'addr_size' during probe() and also use that as one possible upper limit.
Signed-off-by: Nuno Sá nuno.sa@analog.com --- v2: * Fix compilation when CONFIG_CFI_FLASH is not set. Done by redefining cfi_flash_bank_size() when CONFIG_CFI_FLASH is set (by returning dts size).
drivers/mtd/cfi_flash.c | 11 +++++++++-- include/flash.h | 1 + 2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index f378f6fb6139..47a48d927201 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -116,12 +116,16 @@ phys_addr_t cfi_flash_bank_addr(int i) { return flash_info[i].base; } + +unsigned long cfi_flash_bank_size(int i) +{ + return flash_info[i].addr_size; +} #else __weak phys_addr_t cfi_flash_bank_addr(int i) { return ((phys_addr_t [])CFG_SYS_FLASH_BANKS_LIST)[i]; } -#endif
__weak unsigned long cfi_flash_bank_size(int i) { @@ -131,6 +135,7 @@ __weak unsigned long cfi_flash_bank_size(int i) return 0; #endif } +#endif
__maybe_weak void flash_write8(u8 value, void *addr) { @@ -2492,15 +2497,17 @@ unsigned long flash_init(void) static int cfi_flash_probe(struct udevice *dev) { fdt_addr_t addr; + fdt_size_t size; int idx;
for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) { - addr = dev_read_addr_index(dev, idx); + addr = dev_read_addr_size_index(dev, idx, &size); 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; + flash_info[cfi_flash_num_flash_banks].addr_size = size; cfi_flash_num_flash_banks++; } gd->bd->bi_flashstart = flash_info[0].base; diff --git a/include/flash.h b/include/flash.h index 95992fa689bb..3710a2731b76 100644 --- a/include/flash.h +++ b/include/flash.h @@ -46,6 +46,7 @@ typedef struct { #ifdef CONFIG_CFI_FLASH /* DM-specific parts */ struct udevice *dev; phys_addr_t base; + phys_size_t addr_size; #endif } flash_info_t;
Move return type to phys_size_t instead of plain unsigned long.
Signed-off-by: Nuno Sá nuno.sa@analog.com --- v2: * new patch.
drivers/mtd/cfi_flash.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 47a48d927201..9c030de3afef 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -117,7 +117,7 @@ phys_addr_t cfi_flash_bank_addr(int i) return flash_info[i].base; }
-unsigned long cfi_flash_bank_size(int i) +phys_size_t cfi_flash_bank_size(int i) { return flash_info[i].addr_size; } @@ -127,10 +127,10 @@ __weak phys_addr_t cfi_flash_bank_addr(int i) return ((phys_addr_t [])CFG_SYS_FLASH_BANKS_LIST)[i]; }
-__weak unsigned long cfi_flash_bank_size(int i) +__weak phys_size_t cfi_flash_bank_size(int i) { #ifdef CFG_SYS_FLASH_BANKS_SIZES - return ((unsigned long [])CFG_SYS_FLASH_BANKS_SIZES)[i]; + return ((phys_size_t [])CFG_SYS_FLASH_BANKS_SIZES)[i]; #else return 0; #endif @@ -2112,7 +2112,7 @@ ulong flash_get_size(phys_addr_t base, int banknum) int erase_region_size; int erase_region_count; struct cfi_qry qry; - unsigned long max_size; + phys_size_t max_size;
memset(&qry, 0, sizeof(qry));
Hi Nuno Sá
On 4/18/23 15:07, Nuno Sá wrote:
flash_get_size() will get the flash size from the device itself and go through all erase regions to read protection status. However, the device mappable region (eg: devicetree reg property) might be lower than the device full size which means that the above cycle will result in a data bus exception. This change fixes it by reading the 'addr_size' during probe() and also use that as one possible upper limit.
Signed-off-by: Nuno Sá nuno.sa@analog.com
v2:
- Fix compilation when CONFIG_CFI_FLASH is not set. Done by redefining
cfi_flash_bank_size() when CONFIG_CFI_FLASH is set (by returning dts size).
Unfortunately this does not work. Build fails on multiple targets. Here an example:
$ make qemu_arm64_defconfig $ make -sj drivers/mtd/cfi_flash.c:120:13: error: conflicting types for 'cfi_flash_bank_size'; have 'phys_size_t(int)' {aka 'long long unsigned int(int)'} 120 | phys_size_t cfi_flash_bank_size(int i) | ^~~~~~~~~~~~~~~~~~~ In file included from drivers/mtd/cfi_flash.c:36: include/mtd/cfi_flash.h:178:15: note: previous declaration of 'cfi_flash_bank_size' with type 'long unsigned int(int)' 178 | unsigned long cfi_flash_bank_size(int i); | ^~~~~~~~~~~~~~~~~~~
Please run a world build next before sending out the next patchset version.
Thanks, Stefan
drivers/mtd/cfi_flash.c | 11 +++++++++-- include/flash.h | 1 + 2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index f378f6fb6139..47a48d927201 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -116,12 +116,16 @@ phys_addr_t cfi_flash_bank_addr(int i) { return flash_info[i].base; }
+unsigned long cfi_flash_bank_size(int i) +{
- return flash_info[i].addr_size;
+} #else __weak phys_addr_t cfi_flash_bank_addr(int i) { return ((phys_addr_t [])CFG_SYS_FLASH_BANKS_LIST)[i]; } -#endif
__weak unsigned long cfi_flash_bank_size(int i) { @@ -131,6 +135,7 @@ __weak unsigned long cfi_flash_bank_size(int i) return 0; #endif } +#endif
__maybe_weak void flash_write8(u8 value, void *addr) { @@ -2492,15 +2497,17 @@ unsigned long flash_init(void) static int cfi_flash_probe(struct udevice *dev) { fdt_addr_t addr;
fdt_size_t size; int idx;
for (idx = 0; idx < CFI_MAX_FLASH_BANKS; idx++) {
addr = dev_read_addr_index(dev, idx);
addr = dev_read_addr_size_index(dev, idx, &size);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;
flash_info[cfi_flash_num_flash_banks].addr_size = size;cfi_flash_num_flash_banks++; } gd->bd->bi_flashstart = flash_info[0].base;
diff --git a/include/flash.h b/include/flash.h index 95992fa689bb..3710a2731b76 100644 --- a/include/flash.h +++ b/include/flash.h @@ -46,6 +46,7 @@ typedef struct { #ifdef CONFIG_CFI_FLASH /* DM-specific parts */ struct udevice *dev; phys_addr_t base;
- phys_size_t addr_size; #endif } flash_info_t;
Viele Grüße, Stefan Roese
On Mon, 2023-04-24 at 08:54 +0200, Stefan Roese wrote:
Hi Stefan,
Hi Nuno Sá
On 4/18/23 15:07, Nuno Sá wrote:
flash_get_size() will get the flash size from the device itself and go through all erase regions to read protection status. However, the device mappable region (eg: devicetree reg property) might be lower than the device full size which means that the above cycle will result in a data bus exception. This change fixes it by reading the 'addr_size' during probe() and also use that as one possible upper limit.
Signed-off-by: Nuno Sá nuno.sa@analog.com
v2: * Fix compilation when CONFIG_CFI_FLASH is not set. Done by redefining cfi_flash_bank_size() when CONFIG_CFI_FLASH is set (by returning dts size).
Unfortunately this does not work. Build fails on multiple targets. Here an example:
$ make qemu_arm64_defconfig $ make -sj drivers/mtd/cfi_flash.c:120:13: error: conflicting types for 'cfi_flash_bank_size'; have 'phys_size_t(int)' {aka 'long long unsigned int(int)'} 120 | phys_size_t cfi_flash_bank_size(int i) | ^~~~~~~~~~~~~~~~~~~ In file included from drivers/mtd/cfi_flash.c:36: include/mtd/cfi_flash.h:178:15: note: previous declaration of 'cfi_flash_bank_size' with type 'long unsigned int(int)' 178 | unsigned long cfi_flash_bank_size(int i); | ^~~~~~~~~~~~~~~~~~~
Please run a world build next before sending out the next patchset version.
Arghh, sorry for this!
I did run it for micoblaze_defconfig and integratorcp_cm926ejs_defconfig. Should I build it with V1 or something? Or is there somewhere where I can push my patchset to trigger some CI and look for these kind of things myself before sending v3?
- Nuno Sá
On 4/24/23 09:13, Nuno Sá wrote:
On Mon, 2023-04-24 at 08:54 +0200, Stefan Roese wrote:
Hi Stefan,
Hi Nuno Sá
On 4/18/23 15:07, Nuno Sá wrote:
flash_get_size() will get the flash size from the device itself and go through all erase regions to read protection status. However, the device mappable region (eg: devicetree reg property) might be lower than the device full size which means that the above cycle will result in a data bus exception. This change fixes it by reading the 'addr_size' during probe() and also use that as one possible upper limit.
Signed-off-by: Nuno Sá nuno.sa@analog.com
v2: * Fix compilation when CONFIG_CFI_FLASH is not set. Done by redefining cfi_flash_bank_size() when CONFIG_CFI_FLASH is set (by returning dts size).
Unfortunately this does not work. Build fails on multiple targets. Here an example:
$ make qemu_arm64_defconfig $ make -sj drivers/mtd/cfi_flash.c:120:13: error: conflicting types for 'cfi_flash_bank_size'; have 'phys_size_t(int)' {aka 'long long unsigned int(int)'} 120 | phys_size_t cfi_flash_bank_size(int i) | ^~~~~~~~~~~~~~~~~~~ In file included from drivers/mtd/cfi_flash.c:36: include/mtd/cfi_flash.h:178:15: note: previous declaration of 'cfi_flash_bank_size' with type 'long unsigned int(int)' 178 | unsigned long cfi_flash_bank_size(int i); | ^~~~~~~~~~~~~~~~~~~
Please run a world build next before sending out the next patchset version.
Arghh, sorry for this!
I did run it for micoblaze_defconfig and integratorcp_cm926ejs_defconfig. Should I build it with V1 or something? Or is there somewhere where I can push my patchset to trigger some CI and look for these kind of things myself before sending v3?
I'm testing with a world build on Azure, Gitlab provides a similar infrastructure. Some docs should be available for this.
Thanks, Stefan
On Mon, 2023-04-24 at 09:36 +0200, Stefan Roese wrote:
On 4/24/23 09:13, Nuno Sá wrote:
On Mon, 2023-04-24 at 08:54 +0200, Stefan Roese wrote:
Hi Stefan,
Hi Nuno Sá
On 4/18/23 15:07, Nuno Sá wrote:
flash_get_size() will get the flash size from the device itself and go through all erase regions to read protection status. However, the device mappable region (eg: devicetree reg property) might be lower than the device full size which means that the above cycle will result in a data bus exception. This change fixes it by reading the 'addr_size' during probe() and also use that as one possible upper limit.
Signed-off-by: Nuno Sá nuno.sa@analog.com
v2: * Fix compilation when CONFIG_CFI_FLASH is not set. Done by redefining cfi_flash_bank_size() when CONFIG_CFI_FLASH is set (by returning dts size).
Unfortunately this does not work. Build fails on multiple targets. Here an example:
$ make qemu_arm64_defconfig $ make -sj drivers/mtd/cfi_flash.c:120:13: error: conflicting types for 'cfi_flash_bank_size'; have 'phys_size_t(int)' {aka 'long long unsigned int(int)'} 120 | phys_size_t cfi_flash_bank_size(int i) | ^~~~~~~~~~~~~~~~~~~ In file included from drivers/mtd/cfi_flash.c:36: include/mtd/cfi_flash.h:178:15: note: previous declaration of 'cfi_flash_bank_size' with type 'long unsigned int(int)' 178 | unsigned long cfi_flash_bank_size(int i); | ^~~~~~~~~~~~~~~~~~~
Please run a world build next before sending out the next patchset version.
Arghh, sorry for this!
I did run it for micoblaze_defconfig and integratorcp_cm926ejs_defconfig. Should I build it with V1 or something? Or is there somewhere where I can push my patchset to trigger some CI and look for these kind of things myself before sending v3?
I'm testing with a world build on Azure, Gitlab provides a similar infrastructure. Some docs should be available for this.
I see, I just cloned the tree from https://source.denx.de/u-boot/u-boot.git. I will fork from github and trigger those builds.
- Nuno Sá
participants (3)
-
Nuno Sá -
Nuno Sá -
Stefan Roese