[PATCH] mtd: cfi: respect reg address length
Stefan Roese
sr at denx.de
Fri Apr 14 10:10:14 CEST 2023
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 at 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
More information about the U-Boot
mailing list