[PATCH v2 4/8] mtd: spi-nor: spansion: Rework cypress_nor_set_page_size() for multi-chip device support

[Tudor Ambarus]

On 08.02.2023 07:53, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> 
> For multi-chip devices, we can use 512B page only when the all dice are
> configured as 512B page size. Simple for-loop with params->num_of_dice can
> check the CFR3V in each die. The volatile register address is calculated
> inside the loop by using die number and volatile register offset.
> 
> The location of cypress_nor_set_page_size() call is moved from
> post_bfpt_fixup() to post_sfdp_fixup(), because the number of dice and
> volatile register offset are parsed after BFPT. The post_sfdp_fixup()

No, this looks wrong.

BFPT is the only mandatory table and should be sufficient to make any
flash work (in the multi die case, just for the first die). All the
other tables are considered optional. If we get an error when parsing
BFPT we return an error, if we get an error in any optional table, we
just print a warning and continue. So you should make sure that the
flash works fine even if sccr_map_mc fails.

> does not return error code so cypress_nor_set_page_size() prints error
> message instead of returning error code.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> ---
>   drivers/mtd/spi-nor/spansion.c | 47 ++++++++++++++++++----------------
>   1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 12a256c0ef4c..7c533737d7c3 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -18,7 +18,7 @@
>   #define SPINOR_REG_CYPRESS_CFR1_QUAD_EN		BIT(1)	/* Quad Enable */
>   #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
>   #define SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24	0xb
> -#define SPINOR_REG_CYPRESS_CFR3V		0x00800004
> +#define SPINOR_REG_CYPRESS_CFR3			0x4
>   #define SPINOR_REG_CYPRESS_CFR3_PGSZ		BIT(4) /* Page size. */
>   #define SPINOR_REG_CYPRESS_CFR5V		0x00800006
>   #define SPINOR_REG_CYPRESS_CFR5_BIT6		BIT(6)
> @@ -195,27 +195,34 @@ static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>    * The BFPT table advertises a 512B or 256B page size depending on part but the
>    * page size is actually configurable (with the default being 256B). Read from
>    * CFR3V[4] and set the correct size.
> - *
> - * Return: 0 on success, -errno otherwise.
>    */
> -static int cypress_nor_set_page_size(struct spi_nor *nor)
> +static void cypress_nor_set_page_size(struct spi_nor *nor)
>   {
> +	struct spi_nor_flash_parameter *params = nor->params;
>   	struct spi_mem_op op =
> -		CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_mode_nbytes,
> -					  SPINOR_REG_CYPRESS_CFR3V,
> +		CYPRESS_NOR_RD_ANY_REG_OP(params->addr_mode_nbytes, 0,
>   					  nor->bouncebuf);
>   	int ret;
> +	u8 i;
>   
> -	ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
> -	if (ret)
> -		return ret;
> +	/* Set to default page size */
> +	params->page_size = 256;
>   
> -	if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3_PGSZ)
> -		nor->params->page_size = 512;
> -	else
> -		nor->params->page_size = 256;
> +	for (i = 0; i < params->num_of_dice; i++) {
> +		op.addr.val = params->vreg_offset[i] + SPINOR_REG_CYPRESS_CFR3;

in case sccr_map fails vreg_offset is zero here, you'll read garbage
>   
> -	return 0;
> +		ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
> +		if (ret) {
> +			dev_err(nor->dev, "Failed to read page size configuration. Use default 256B.\n");
> +			return;

why did we consider a no-go error and exited in the past and now we just
use a default config?

> +		}
> +
> +		if (!(nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3_PGSZ))
> +			return;
> +	}
> +
> +	/* We reach here only when all dice are configured to 512B */
> +	params->page_size = 512;
>   }
>   
>   static int
> @@ -226,7 +233,7 @@ s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
>   	/* Replace Quad Enable with volatile version */
>   	nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>   
> -	return cypress_nor_set_page_size(nor);
> +	return 0;
>   }
>   
>   static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
> @@ -251,6 +258,8 @@ static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
>   			break;
>   		}
>   	}
> +
> +	cypress_nor_set_page_size(nor);
>   }
>   
>   static void s25hx_t_late_init(struct spi_nor *nor)
> @@ -312,13 +321,8 @@ static void s28hx_t_post_sfdp_fixup(struct spi_nor *nor)
>   	 * actual value for that is 4.
>   	 */
>   	nor->params->rdsr_addr_nbytes = 4;
> -}
>   
> -static int s28hx_t_post_bfpt_fixup(struct spi_nor *nor,
> -				   const struct sfdp_parameter_header *bfpt_header,
> -				   const struct sfdp_bfpt *bfpt)
> -{
> -	return cypress_nor_set_page_size(nor);
> +	cypress_nor_set_page_size(nor);
>   }
>   
>   static void s28hx_t_late_init(struct spi_nor *nor)
> @@ -329,7 +333,6 @@ static void s28hx_t_late_init(struct spi_nor *nor)
>   
>   static const struct spi_nor_fixups s28hx_t_fixups = {
>   	.post_sfdp = s28hx_t_post_sfdp_fixup,
> -	.post_bfpt = s28hx_t_post_bfpt_fixup,
>   	.late_init = s28hx_t_late_init,
>   };
>