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

[PATCH v2 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).

 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;
 
-- 
2.40.0

[PATCH v2 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.

 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));
 
-- 
2.40.0

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

[Stefan Roese]

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

-- 
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 v2 1/2] mtd: cfi: respect reg address length

[Nuno Sá]

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á

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

[Stefan Roese]

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

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

[Nuno Sá]

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á