lkml.org 
[lkml]   [2023]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 05/41] mtd: spi-nor: convert .n_sectors to .size
Am 2023-08-24 10:25, schrieb Tudor Ambarus:
> On 8/22/23 08:09, Michael Walle wrote:
>> .n_sectors is rarely used. In fact it is only used in swp.c and to
>> calculate the flash size in the core. The use in swp.c might be
>> converted to use the (largest) flash erase size. For now, we just
>> locally calculate the sector size.
>>
>> Simplify the flash_info database and set the size of the flash
>> directly.
>> This also let us use the SZ_x macros.
>>
>> Signed-off-by: Michael Walle <mwalle@kernel.org>
>> ---
>> drivers/mtd/spi-nor/core.c | 2 +-
>> drivers/mtd/spi-nor/core.h | 8 ++++----
>> drivers/mtd/spi-nor/swp.c | 9 +++++----
>> drivers/mtd/spi-nor/xilinx.c | 4 ++--
>> 4 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 286155002cdc..f4cc2eafcc5e 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2999,7 +2999,7 @@ static void spi_nor_init_default_params(struct
>> spi_nor *nor)
>>
>> /* Set SPI NOR sizes. */
>> params->writesize = 1;
>> - params->size = (u64)info->sector_size * info->n_sectors;
>> + params->size = info->size;
>
> would be good to check the sanity of info->size to not be null and to
> be
> divisible by sector_size.

Have a look at the later patches, info->size can be zero, indicating
that we need to parse SFDP for this flash.

I could validate that the size is a multiple of sector_size, but
that one might also be zero. Of course you could solve this by
additional logic. But I treated that one as a misconfiguration
of the flash_info entry. It's nothing which can happen during
runtime. Anyway, I have no strong opinion on the "is multiple
of sector_size" check (for now, maybe there's something I didn't
thought of now).

>
>> params->bank_size = params->size;
>> params->page_size = info->page_size;
>>
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index dfc20a3296fb..12c35409493b 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -443,9 +443,9 @@ struct spi_nor_fixups {
>> * @id: the flash's ID bytes. The first three bytes are
>> the
>> * JEDIC ID. JEDEC ID zero means "no ID" (mostly
>> older chips).
>> * @id_len: the number of bytes of ID.
>> + * @size: the size of the flash in bytes.
>> * @sector_size: the size listed here is what works with
>> SPINOR_OP_SE, which
>> * isn't necessarily called a "sector" by the
>> vendor.
>> - * @n_sectors: the number of sectors.
>> * @n_banks: the number of banks.
>> * @page_size: the flash's page size.
>> * @addr_nbytes: number of address bytes to send.
>> @@ -505,8 +505,8 @@ struct flash_info {
>> char *name;
>> u8 id[SPI_NOR_MAX_ID_LEN];
>> u8 id_len;
>> + size_t size;
>> unsigned sector_size;
>> - u16 n_sectors;
>> u16 page_size;
>> u8 n_banks;
>> u8 addr_nbytes;
>> @@ -556,8 +556,8 @@ struct flash_info {
>> .id_len = 6
>>
>> #define SPI_NOR_GEOMETRY(_sector_size, _n_sectors, _n_banks) \
>> + .size = (_sector_size) * (_n_sectors), \
>> .sector_size = (_sector_size), \
>> - .n_sectors = (_n_sectors), \
>> .page_size = 256, \
>> .n_banks = (_n_banks)
>>
>> @@ -575,8 +575,8 @@ struct flash_info {
>> SPI_NOR_GEOMETRY((_sector_size), (_n_sectors), 1),
>>
>> #define CAT25_INFO(_sector_size, _n_sectors, _page_size,
>> _addr_nbytes) \
>> + .size = (_sector_size) * (_n_sectors), \
>> .sector_size = (_sector_size), \
>> - .n_sectors = (_n_sectors), \
>> .page_size = (_page_size), \
>> .n_banks = 1, \
>> .addr_nbytes = (_addr_nbytes), \
>> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
>> index 5ab9d5324860..40bf52867095 100644
>> --- a/drivers/mtd/spi-nor/swp.c
>> +++ b/drivers/mtd/spi-nor/swp.c
>> @@ -34,17 +34,18 @@ static u8 spi_nor_get_sr_tb_mask(struct spi_nor
>> *nor)
>> static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
>> {
>> unsigned int bp_slots, bp_slots_needed;
>> + unsigned int sector_size = nor->info->sector_size;
>> + u64 n_sectors = div_u64(nor->params->size, sector_size);
>
> if params(info)->size is zero here, we get into trouble.

Please note that this is not the info->size, params->size cannot
be zero. If info->size was zero, we have to parse SFDP which hopefully
will give us a sane size.

And regarding the sector_size, which could be zero here. I want to
replace that logic, so we don't rely on the flash_info sector size.
From a later patch:

/*
* sector_size will eventually be replaced with the max erase
size of
* the flash. For now, we need to have that ugly default.
*/

-michael

\
 
 \ /
  Last update: 2023-09-01 13:01    [W:0.101 / U:0.808 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site