[PATCH v3 1/2] mtd: cfi: respect reg address length

[PATCH v3 1/2] mtd: cfi: respect reg address length

[Nuno Sá]

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).

v3:
 * Fix another compilation warning by explicitly casting to unsigned
long in cfi_flash_bank_size()

Stefan, I did ran a world build [1] by opening a PR in github (to force CI
to run). However I had to bypass the pytest stage (it was giving me some
unrelated problems) and there are a couple of jobs failing but the
errors apparently are not related to this patchset. Hopefully things now
pass in your tests.

[1]: https://github.com/u-boot/u-boot/pull/281

 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..87a3daebdabe 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 (unsigned long)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;
 
-- 
2.40.0

[PATCH v3 2/2] mtd: cfi: change cfi_flash_bank_size() return type

[Nuno Sá]

Move return type to phys_size_t instead of plain unsigned long.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
v2:
 * new patch.

v3:
 * also make sure cfi_flash_bank_size() declaration (in cfi_flash.h)
gets updated to phys_size_t.

 drivers/mtd/cfi_flash.c | 10 +++++-----
 include/mtd/cfi_flash.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 87a3daebdabe..9c030de3afef 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -117,9 +117,9 @@ 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 (unsigned long)flash_info[i].addr_size;
+	return flash_info[i].addr_size;
 }
 #else
 __weak phys_addr_t cfi_flash_bank_addr(int i)
@@ -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));
 
diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
index 52cd1c4dbc4e..1900a4a2f6e4 100644
--- a/include/mtd/cfi_flash.h
+++ b/include/mtd/cfi_flash.h
@@ -175,7 +175,7 @@ extern int cfi_flash_num_flash_banks;
 #endif
 
 phys_addr_t cfi_flash_bank_addr(int i);
-unsigned long cfi_flash_bank_size(int i);
+phys_size_t cfi_flash_bank_size(int i);
 
 #ifdef CONFIG_CFI_FLASH_USE_WEAK_ACCESSORS
 void flash_write8(u8 value, void *addr);
-- 
2.40.0

Re: [PATCH v3 1/2] mtd: cfi: respect reg address length

[Stefan Roese]

Hi Nuno Sá,

On 4/26/23 16:16, 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).
> 
> v3:
>   * Fix another compilation warning by explicitly casting to unsigned
> long in cfi_flash_bank_size()
> 
> Stefan, I did ran a world build [1] by opening a PR in github (to force CI
> to run). However I had to bypass the pytest stage (it was giving me some
> unrelated problems) and there are a couple of jobs failing but the
> errors apparently are not related to this patchset. Hopefully things now
> pass in your tests.
> 
> [1]: https://github.com/u-boot/u-boot/pull/281

Unfortunately this breaks my usual Azure CI build in the test_py phase.
And this works without any issues without those 2 patches applied. So
its related to these changes.

Sorry, I can't apply these patches. Please take another look at fixing
these issues.

Thanks,
Stefan

[1] 
https://dev.azure.com/sr0718/u-boot/_build/results?buildId=298&view=results

>   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..87a3daebdabe 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 (unsigned long)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

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH v3 1/2] mtd: cfi: respect reg address length

[Nuno Sá]

Hi Stefan,

On Fri, 2023-04-28 at 11:59 +0200, Stefan Roese wrote:
> Hi Nuno Sá,
> 
> On 4/26/23 16:16, 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).
> > 
> > v3:
> >   * Fix another compilation warning by explicitly casting to unsigned
> > long in cfi_flash_bank_size()
> > 
> > Stefan, I did ran a world build [1] by opening a PR in github (to force CI
> > to run). However I had to bypass the pytest stage (it was giving me some
> > unrelated problems) and there are a couple of jobs failing but the
> > errors apparently are not related to this patchset. Hopefully things now
> > pass in your tests.
> > 
> > [1]: https://github.com/u-boot/u-boot/pull/281
> 
> Unfortunately this breaks my usual Azure CI build in the test_py phase.
> And this works without any issues without those 2 patches applied. So
> its related to these changes.
> 

Sorry for this... It seemed unrelated and the logs don't say much about why is
it failing. I'll try to see what's the problem.

- Nuno Sá