Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <80641a1c-d922-22cc-edd4-76d475e1aac8@linaro.org>
Date:   Tue, 19 Sep 2023 12:24:07 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.0
Subject: Re: [PATCH v3 05/41] mtd: spi-nor: convert .n_sectors to .size
Content-Language: en-US
To:     Michael Walle <mwalle@kernel.org>,
        Pratyush Yadav <pratyush@kernel.org>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>
Cc:     linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org
References: <20230807-mtd-flash-info-db-rework-v3-0-e60548861b10@kernel.org>
 <20230807-mtd-flash-info-db-rework-v3-5-e60548861b10@kernel.org>
From:   Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <20230807-mtd-flash-info-db-rework-v3-5-e60548861b10@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 08.09.2023 13:16, 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;
>  	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;

this should use nor->params instead, but we can do it later on. New
flash additions must specify the sector size, otherwise we get a
division by zero below. I see you introduce a default value in patch 10,
which will guard us. I assume  you checked that there's no flash that
specifies BP and sector size of zero up to patch 9 inclusively,
otherwise we'll break bisect-ability. Let me know if yes, and I'll
update the commit message when applying.

> +	u64 n_sectors = div_u64(nor->params->size, sector_size);
>  	u8 mask = spi_nor_get_sr_bp_mask(nor);
>  
>  	/* Reserved one for "protect none" and one for "protect all". */
>  	bp_slots = (1 << hweight8(mask)) - 2;
> -	bp_slots_needed = ilog2(nor->info->n_sectors);
> +	bp_slots_needed = ilog2(n_sectors);
>  
>  	if (bp_slots_needed > bp_slots)
> -		return nor->info->sector_size <<
> -			(bp_slots_needed - bp_slots);
> +		return sector_size << (bp_slots_needed - bp_slots);
>  	else
> -		return nor->info->sector_size;
> +		return sector_size;
>  }
>  
>  static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index 34267591282c..284e2e4970ab 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -23,8 +23,8 @@
>  
>  #define S3AN_INFO(_jedec_id, _n_sectors, _page_size)			\
>  		SPI_NOR_ID(_jedec_id, 0),				\
> +		.size = 8 * (_page_size) * (_n_sectors),		\
>  		.sector_size = (8 * (_page_size)),			\
> -		.n_sectors = (_n_sectors),				\
>  		.page_size = (_page_size),				\
>  		.n_banks = 1,						\
>  		.flags = SPI_NOR_NO_FR
> @@ -138,7 +138,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
>  		page_size = (nor->params->page_size == 264) ? 256 : 512;
>  		nor->params->page_size = page_size;
>  		nor->mtd.writebufsize = page_size;
> -		nor->params->size = 8 * page_size * nor->info->n_sectors;
> +		nor->params->size = nor->info->size;
>  		nor->mtd.erasesize = 8 * page_size;
>  	} else {
>  		/* Flash in Default addressing mode */
>