| Message ID | 20230327132229.1061821-1-nuno.sa@analog.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | mtd: cfi: respect reg address length | expand |
On 3/27/23 15:22, 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> > --- > > To give a bit more of background for this patch, I caught this issue > when running u-boot on microblaze which uses xilinx axi_linear_flash IP. > On ADI designs using that IP, the flash size was set to 32MiB resulting > in the mentioned data bus exception as we obviously access past the > IP size. > > That said, there was not real reason for the flash truncation so we'll > be fixing our designs so the IP will contemplate the complete flash > size. > > For the above reason, I was not really sure to mark the patch as RFC or > not... Anyways, it still feels like a valid "protection" to have rather > then just aborting (even if it does not really make sense to ever > truncate the flash in devicetree). > > drivers/mtd/cfi_flash.c | 10 ++++++++-- > include/flash.h | 1 + > 2 files changed, 9 insertions(+), 2 deletions(-) Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > index f378f6fb6139..432d0d5c9ecb 100644 > --- a/drivers/mtd/cfi_flash.c > +++ b/drivers/mtd/cfi_flash.c > @@ -2196,7 +2196,11 @@ ulong flash_get_size(phys_addr_t base, int banknum) > /* multiply the size by the number of chips */ > info->size *= size_ratio; > max_size = cfi_flash_bank_size(banknum); > - if (max_size && info->size > max_size) { > + if (max_size) > + max_size = min(info->addr_size, max_size); > + else > + max_size = info->addr_size; > + if (info->size > max_size) { > debug("[truncated from %ldMiB]", info->size >> 20); > info->size = max_size; > } > @@ -2492,15 +2496,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
Hi Nuno, On 3/27/23 15:22, 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> > --- > > To give a bit more of background for this patch, I caught this issue > when running u-boot on microblaze which uses xilinx axi_linear_flash IP. > On ADI designs using that IP, the flash size was set to 32MiB resulting > in the mentioned data bus exception as we obviously access past the > IP size. > > That said, there was not real reason for the flash truncation so we'll > be fixing our designs so the IP will contemplate the complete flash > size. > > For the above reason, I was not really sure to mark the patch as RFC or > not... Anyways, it still feels like a valid "protection" to have rather > then just aborting (even if it does not really make sense to ever > truncate the flash in devicetree). > > drivers/mtd/cfi_flash.c | 10 ++++++++-- > include/flash.h | 1 + > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > index f378f6fb6139..432d0d5c9ecb 100644 > --- a/drivers/mtd/cfi_flash.c > +++ b/drivers/mtd/cfi_flash.c > @@ -2196,7 +2196,11 @@ ulong flash_get_size(phys_addr_t base, int banknum) > /* multiply the size by the number of chips */ > info->size *= size_ratio; > max_size = cfi_flash_bank_size(banknum); > - if (max_size && info->size > max_size) { > + if (max_size) > + max_size = min(info->addr_size, max_size); > + else > + max_size = info->addr_size; > + if (info->size > max_size) { > debug("[truncated from %ldMiB]", info->size >> 20); > info->size = max_size; > } > @@ -2492,15 +2496,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; > Running a CI world build leads to many errors. Here an example: $ make integratorcp_cm926ejs_defconfig $ make -sj In file included from include/linux/bitops.h:22, from include/log.h:15, from include/linux/printk.h:4, from include/common.h:20, from drivers/mtd/cfi_flash.c:19: drivers/mtd/cfi_flash.c: In function 'flash_get_size': drivers/mtd/cfi_flash.c:2200:44: error: 'flash_info_t' has no member named 'addr_size' 2200 | max_size = min(info->addr_size, max_size); | ^~ include/linux/kernel.h:182:16: note: in definition of macro 'min' 182 | typeof(x) _min1 = (x); \ | ^ drivers/mtd/cfi_flash.c:2200:44: error: 'flash_info_t' has no member named 'addr_size' 2200 | max_size = min(info->addr_size, max_size); | ^~ include/linux/kernel.h:182:28: note: in definition of macro 'min' 182 | typeof(x) _min1 = (x); \ | ^ include/linux/kernel.h:184:24: warning: comparison of distinct pointer types lacks a cast 184 | (void) (&_min1 == &_min2); \ | ^~ drivers/mtd/cfi_flash.c:2200:36: note: in expansion of macro 'min' 2200 | max_size = min(info->addr_size, max_size); | ^~~ drivers/mtd/cfi_flash.c:2202:40: error: 'flash_info_t' has no member named 'addr_size' 2202 | max_size = info->addr_size; | ^~ make[2]: *** [scripts/Makefile.build:257: drivers/mtd/cfi_flash.o] Error 1 make[1]: *** [scripts/Makefile.build:397: drivers/mtd] Error 2 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1850: drivers] Error 2 make: *** Waiting for unfinished jobs.... Could you please take a look and make sure that v2 successfully runs a world build? Thanks, Stefan
Hi Stefan, On Fri, 2023-04-14 at 08:57 +0200, Stefan Roese wrote: > Hi Nuno, > > On 3/27/23 15:22, 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> > > --- > > > > To give a bit more of background for this patch, I caught this > > issue > > when running u-boot on microblaze which uses xilinx > > axi_linear_flash IP. > > On ADI designs using that IP, the flash size was set to 32MiB > > resulting > > in the mentioned data bus exception as we obviously access past the > > IP size. > > > > That said, there was not real reason for the flash truncation so > > we'll > > be fixing our designs so the IP will contemplate the complete flash > > size. > > > > For the above reason, I was not really sure to mark the patch as > > RFC or > > not... Anyways, it still feels like a valid "protection" to have > > rather > > then just aborting (even if it does not really make sense to ever > > truncate the flash in devicetree). > > > > drivers/mtd/cfi_flash.c | 10 ++++++++-- > > include/flash.h | 1 + > > 2 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > > index f378f6fb6139..432d0d5c9ecb 100644 > > --- a/drivers/mtd/cfi_flash.c > > +++ b/drivers/mtd/cfi_flash.c > > @@ -2196,7 +2196,11 @@ ulong flash_get_size(phys_addr_t base, int > > banknum) > > /* multiply the size by the number of chips */ > > info->size *= size_ratio; > > max_size = cfi_flash_bank_size(banknum); > > - if (max_size && info->size > max_size) { > > + if (max_size) > > + max_size = min(info->addr_size, max_size); > > + else > > + max_size = info->addr_size; > > + if (info->size > max_size) { > > debug("[truncated from %ldMiB]", info->size > > >> 20); > > info->size = max_size; > > } > > @@ -2492,15 +2496,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; > > > > Running a CI world build leads to many errors. Here an example: > > $ make integratorcp_cm926ejs_defconfig > $ make -sj > In file included from include/linux/bitops.h:22, > from include/log.h:15, > from include/linux/printk.h:4, > from include/common.h:20, > from drivers/mtd/cfi_flash.c:19: > drivers/mtd/cfi_flash.c: In function 'flash_get_size': > drivers/mtd/cfi_flash.c:2200:44: error: 'flash_info_t' has no member > named 'addr_size' > 2200 | max_size = min(info->addr_size, > max_size); > | ^~ > include/linux/kernel.h:182:16: note: in definition of macro 'min' > 182 | typeof(x) _min1 = (x); \ > | ^ > drivers/mtd/cfi_flash.c:2200:44: error: 'flash_info_t' has no member > named 'addr_size' > 2200 | max_size = min(info->addr_size, > max_size); > | ^~ > include/linux/kernel.h:182:28: note: in definition of macro 'min' > 182 | typeof(x) _min1 = (x); \ > | ^ > include/linux/kernel.h:184:24: warning: comparison of distinct > pointer > types lacks a cast > 184 | (void) (&_min1 == &_min2); \ > | ^~ > drivers/mtd/cfi_flash.c:2200:36: note: in expansion of macro 'min' > 2200 | max_size = min(info->addr_size, > max_size); > | ^~~ > drivers/mtd/cfi_flash.c:2202:40: error: 'flash_info_t' has no member > named 'addr_size' > 2202 | max_size = info->addr_size; > | ^~ > make[2]: *** [scripts/Makefile.build:257: drivers/mtd/cfi_flash.o] > Error 1 > make[1]: *** [scripts/Makefile.build:397: drivers/mtd] Error 2 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1850: drivers] Error 2 > make: *** Waiting for unfinished jobs.... > > Could you please take a look and make sure that v2 successfully runs > a world build? Ahh crap, I did compiled it but for a microblaze config which defines CONFIG_CFI_FLASH. I failed to spot that... One question though... I see that cfi_flash_bank_addr() is mutually exclusive with CFG_SYS_FLASH_BANKS_LIST in the sense we get one or another. In my patch, my approach was that we could get the size from devicetree and also from cfi_flash_bank_size() and thus, I was taking the minimum. But, is that correct? Or can I have the same approach as for the addr? Something like: #ifdef CONFIG_CFI_FLASH /* for driver model */ unsigned long cfi_flash_bank_size(int i) { return flash_info[i].addr_size; } #else __weak unsigned long cfi_flash_bank_size(int i) { #ifdef CFG_SYS_FLASH_BANKS_SIZES return ((unsigned long [])CFG_SYS_FLASH_BANKS_SIZES)[i]; #else return 0; #endif } #endif > Maybe also changing unsigned long to phys_size_t? Does the above makes sense? It would simplify things. Thanks! - Nuno Sá
Hi Nuno Sá, On 4/14/23 09:37, Nuno Sá wrote: > Hi Stefan, > > On Fri, 2023-04-14 at 08:57 +0200, Stefan Roese wrote: >> Hi Nuno, >> >> On 3/27/23 15:22, 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> >>> --- >>> >>> To give a bit more of background for this patch, I caught this >>> issue >>> when running u-boot on microblaze which uses xilinx >>> axi_linear_flash IP. >>> On ADI designs using that IP, the flash size was set to 32MiB >>> resulting >>> in the mentioned data bus exception as we obviously access past the >>> IP size. >>> >>> That said, there was not real reason for the flash truncation so >>> we'll >>> be fixing our designs so the IP will contemplate the complete flash >>> size. >>> >>> For the above reason, I was not really sure to mark the patch as >>> RFC or >>> not... Anyways, it still feels like a valid "protection" to have >>> rather >>> then just aborting (even if it does not really make sense to ever >>> truncate the flash in devicetree). >>> >>> drivers/mtd/cfi_flash.c | 10 ++++++++-- >>> include/flash.h | 1 + >>> 2 files changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c >>> index f378f6fb6139..432d0d5c9ecb 100644 >>> --- a/drivers/mtd/cfi_flash.c >>> +++ b/drivers/mtd/cfi_flash.c >>> @@ -2196,7 +2196,11 @@ ulong flash_get_size(phys_addr_t base, int >>> banknum) >>> /* multiply the size by the number of chips */ >>> info->size *= size_ratio; >>> max_size = cfi_flash_bank_size(banknum); >>> - if (max_size && info->size > max_size) { >>> + if (max_size) >>> + max_size = min(info->addr_size, max_size); >>> + else >>> + max_size = info->addr_size; >>> + if (info->size > max_size) { >>> debug("[truncated from %ldMiB]", info->size >>>>> 20); >>> info->size = max_size; >>> } >>> @@ -2492,15 +2496,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; >>> >> >> Running a CI world build leads to many errors. Here an example: >> >> $ make integratorcp_cm926ejs_defconfig >> $ make -sj >> In file included from include/linux/bitops.h:22, >> from include/log.h:15, >> from include/linux/printk.h:4, >> from include/common.h:20, >> from drivers/mtd/cfi_flash.c:19: >> drivers/mtd/cfi_flash.c: In function 'flash_get_size': >> drivers/mtd/cfi_flash.c:2200:44: error: 'flash_info_t' has no member >> named 'addr_size' >> 2200 | max_size = min(info->addr_size, >> max_size); >> | ^~ >> include/linux/kernel.h:182:16: note: in definition of macro 'min' >> 182 | typeof(x) _min1 = (x); \ >> | ^ >> drivers/mtd/cfi_flash.c:2200:44: error: 'flash_info_t' has no member >> named 'addr_size' >> 2200 | max_size = min(info->addr_size, >> max_size); >> | ^~ >> include/linux/kernel.h:182:28: note: in definition of macro 'min' >> 182 | typeof(x) _min1 = (x); \ >> | ^ >> include/linux/kernel.h:184:24: warning: comparison of distinct >> pointer >> types lacks a cast >> 184 | (void) (&_min1 == &_min2); \ >> | ^~ >> drivers/mtd/cfi_flash.c:2200:36: note: in expansion of macro 'min' >> 2200 | max_size = min(info->addr_size, >> max_size); >> | ^~~ >> drivers/mtd/cfi_flash.c:2202:40: error: 'flash_info_t' has no member >> named 'addr_size' >> 2202 | max_size = info->addr_size; >> | ^~ >> make[2]: *** [scripts/Makefile.build:257: drivers/mtd/cfi_flash.o] >> Error 1 >> make[1]: *** [scripts/Makefile.build:397: drivers/mtd] Error 2 >> make[1]: *** Waiting for unfinished jobs.... >> make: *** [Makefile:1850: drivers] Error 2 >> make: *** Waiting for unfinished jobs.... >> >> Could you please take a look and make sure that v2 successfully runs >> a world build? > > Ahh crap, I did compiled it but for a microblaze config which defines > CONFIG_CFI_FLASH. I failed to spot that... > > One question though... I see that cfi_flash_bank_addr() is mutually > exclusive with CFG_SYS_FLASH_BANKS_LIST in the sense we get one or > another. In my patch, my approach was that we could get the size from > devicetree and also from cfi_flash_bank_size() and thus, I was taking > the minimum. But, is that correct? Or can I have the same approach as > for the addr? Frankly, I've not looked into the CFI flash code for quite some time so my memory is a bit foggy here. ;) Additionally I don't even have a board using parallel CFI flash here any more. > Something like: > > > #ifdef CONFIG_CFI_FLASH /* for driver model */ > unsigned long cfi_flash_bank_size(int i) > { > return flash_info[i].addr_size; > } > #else > __weak unsigned long cfi_flash_bank_size(int i) > { > #ifdef CFG_SYS_FLASH_BANKS_SIZES > return ((unsigned long [])CFG_SYS_FLASH_BANKS_SIZES)[i]; > #else > return 0; > #endif > } > #endif >> Looks reasonable to me. > Maybe also changing unsigned long to phys_size_t? Yes, also a good idea. Please in a separate patch though. > Does the above makes sense? It would simplify things. Ack. Thanks, Stefan
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index f378f6fb6139..432d0d5c9ecb 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2196,7 +2196,11 @@ ulong flash_get_size(phys_addr_t base, int banknum) /* multiply the size by the number of chips */ info->size *= size_ratio; max_size = cfi_flash_bank_size(banknum); - if (max_size && info->size > max_size) { + if (max_size) + max_size = min(info->addr_size, max_size); + else + max_size = info->addr_size; + if (info->size > max_size) { debug("[truncated from %ldMiB]", info->size >> 20); info->size = max_size; } @@ -2492,15 +2496,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;
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> --- To give a bit more of background for this patch, I caught this issue when running u-boot on microblaze which uses xilinx axi_linear_flash IP. On ADI designs using that IP, the flash size was set to 32MiB resulting in the mentioned data bus exception as we obviously access past the IP size. That said, there was not real reason for the flash truncation so we'll be fixing our designs so the IP will contemplate the complete flash size. For the above reason, I was not really sure to mark the patch as RFC or not... Anyways, it still feels like a valid "protection" to have rather then just aborting (even if it does not really make sense to ever truncate the flash in devicetree). drivers/mtd/cfi_flash.c | 10 ++++++++-- include/flash.h | 1 + 2 files changed, 9 insertions(+), 2 deletions(-)