Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <417fb0fa-e774-c744-8641-7618347a8d1a@linaro.org>
Date:   Tue, 19 Sep 2023 12:25:35 +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 10/41] mtd: spi-nor: make sector_size optional
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-10-e60548861b10@kernel.org>
From:   Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <20230807-mtd-flash-info-db-rework-v3-10-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:
> Most of the (old, non-SFDP) flashes use a sector size of 64k. Make that
> a default value so it can be optional in the flash_info database.
> 
> As a preparation for conversion to the new database format, set the
> sector size to zero if the default value is used. This way, the actual
> change is happening with this patch ant not with a later conversion
> patch.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
>  drivers/mtd/spi-nor/core.c | 6 ++++--
>  drivers/mtd/spi-nor/core.h | 8 +++++---
>  drivers/mtd/spi-nor/swp.c  | 6 +++++-
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index c84be791341e..368851ff9f40 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2756,7 +2756,8 @@ static void spi_nor_no_sfdp_init_params(struct spi_nor *nor)
>  {
>  	struct spi_nor_flash_parameter *params = nor->params;
>  	struct spi_nor_erase_map *map = &params->erase_map;
> -	const u8 no_sfdp_flags = nor->info->no_sfdp_flags;
> +	const struct flash_info *info = nor->info;
> +	const u8 no_sfdp_flags = info->no_sfdp_flags;
>  	u8 i, erase_mask;
>  
>  	if (no_sfdp_flags & SPI_NOR_DUAL_READ) {
> @@ -2810,7 +2811,8 @@ static void spi_nor_no_sfdp_init_params(struct spi_nor *nor)
>  		i++;
>  	}
>  	erase_mask |= BIT(i);
> -	spi_nor_set_erase_type(&map->erase_type[i], nor->info->sector_size,
> +	spi_nor_set_erase_type(&map->erase_type[i],
> +			       info->sector_size ?: SPI_NOR_DEFAULT_SECTOR_SIZE,
>  			       SPINOR_OP_SE);

all these info->sector_size checks can be removed if we use
params->sector_size. I'll do it after applying the series.

>  	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
>  }
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 8627d0b95be6..fba3ea8536a5 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -16,6 +16,7 @@
>   */
>  #define SPI_NOR_DEFAULT_PAGE_SIZE 256
>  #define SPI_NOR_DEFAULT_N_BANKS 1
> +#define SPI_NOR_DEFAULT_SECTOR_SIZE SZ_64K
>  
>  /* Standard SPI NOR flash operations. */
>  #define SPI_NOR_READID_OP(naddr, ndummy, buf, len)			\
> @@ -452,8 +453,9 @@ struct spi_nor_fixups {
>   *                  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.
> + * @sector_size:    (optional) the size listed here is what works with
> + *                  SPINOR_OP_SE, which isn't necessarily called a "sector" by
> + *                  the vendor. Defaults to 64k.
>   * @n_banks:        (optional) the number of banks. Defaults to 1.
>   * @page_size:      (optional) the flash's page size. Defaults to 256.
>   * @addr_nbytes:    number of address bytes to send.
> @@ -565,7 +567,7 @@ struct flash_info {
>  
>  #define SPI_NOR_GEOMETRY(_sector_size, _n_sectors, _n_banks)		\
>  	.size = (_sector_size) * (_n_sectors),				\
> -	.sector_size = (_sector_size),					\
> +	.sector_size = (_sector_size == SZ_64K) ? 0 : (_sector_size),	\
>  	.n_banks = (_n_banks)
>  
>  /* Used when the "_ext_id" is two bytes at most */
> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index 40bf52867095..585813310ee1 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -34,7 +34,11 @@ 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;
> +	/*
> +	 * sector_size will eventually be replaced with the max erase size of
> +	 * the flash. For now, we need to have that ugly default.
> +	 */
> +	unsigned int sector_size = nor->info->sector_size ?: SPI_NOR_DEFAULT_SECTOR_SIZE;
>  	u64 n_sectors = div_u64(nor->params->size, sector_size);
>  	u8 mask = spi_nor_get_sr_bp_mask(nor);
>  
>