Re: [PATCH 2/5] mtd: spi-nor: spansion: Rework octal_dtr_enable()

[Tudor Ambarus <tudor.ambarus@linaro.org>]

On 6/12/23 11:04, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> S28HS02GT is multi-chip package (MCP) device that requires Octal DTR
> configuraion for each die. As preparation for MCP support, this patch
> replaces cypress_nor_octal_dtr_en/dis() with cypress_nor_setup_memlat()
> and cypress_nor_setup_opiddr(). And the ID check part is moved to
> cypress_nor_octal_dtr_enable().
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi-nor/spansion.c | 118 +++++++++++++++++----------------
>  1 file changed, 62 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 7804be3a9f2a..0daa3a357ae8 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -156,7 +156,7 @@ static int cypress_nor_sr_ready_and_clear(struct spi_nor *nor)
>  	return 1;
>  }
>  
> -static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
> +static int cypress_nor_setup_memlat(struct spi_nor *nor)

cypress_nor_set_memlat?

Be kind and introduce a description for the method so that we don't
cross check the datasheet each time. I see that for memlat we use a
hardcoded value, whereas it should have been dynamically determined
based on the flash freq. Something to improve in the future if you care.


>  {
>  	struct spi_mem_op op;
>  	u8 *buf = nor->bouncebuf;
> @@ -178,67 +178,37 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
>  		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
>  					  SPINOR_REG_CYPRESS_CFR2V, 1, buf);
>  
> -	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> -	if (ret)
> -		return ret;
> -
> -	nor->read_dummy = 24;
> -
> -	/* Set the octal and DTR enable bits. */
> -	buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN;
> -	op = (struct spi_mem_op)
> -		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
> -					  SPINOR_REG_CYPRESS_CFR5V, 1, buf);
> -
> -	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> -	if (ret)
> -		return ret;
> -
> -	/* Read flash ID to make sure the switch was successful. */
> -	ret = spi_nor_read_id(nor, nor->addr_nbytes, 3, buf,
> -			      SNOR_PROTO_8_8_8_DTR);
> -	if (ret) {
> -		dev_dbg(nor->dev, "error %d reading JEDEC ID after enabling 8D-8D-8D mode\n", ret);
> -		return ret;
> -	}
> -
> -	if (memcmp(buf, nor->info->id, nor->info->id_len))
> -		return -EINVAL;
> -
> -	return 0;
> +	return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
>  }
>  
> -static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
> +static int cypress_nor_setup_opiddr(struct spi_nor *nor, bool enable)

what does opiddr stand for? Let's rename it to something humans can
understand.

>  {
>  	struct spi_mem_op op;
>  	u8 *buf = nor->bouncebuf;
> -	int ret;
> -
> -	/*
> -	 * The register is 1-byte wide, but 1-byte transactions are not allowed
> -	 * in 8D-8D-8D mode. Since there is no register at the next location,
> -	 * just initialize the value to 0 and let the transaction go on.
> -	 */
> -	buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS;
> -	buf[1] = 0;
> -	op = (struct spi_mem_op)
> -		CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes,
> -					  SPINOR_REG_CYPRESS_CFR5V, 2, buf);
> -	ret = spi_nor_write_any_volatile_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
> -	if (ret)
> -		return ret;
>  
> -	/* Read flash ID to make sure the switch was successful. */
> -	ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
> -	if (ret) {
> -		dev_dbg(nor->dev, "error %d reading JEDEC ID after disabling 8D-8D-8D mode\n", ret);
> -		return ret;
> +	if (enable) {
> +		/* Set the octal and DTR enable bits. */
> +		buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN;
> +		op = (struct spi_mem_op)
> +			CYPRESS_NOR_WR_ANY_REG_OP(nor->params->addr_mode_nbytes,
> +						  SPINOR_REG_CYPRESS_CFR5V, 1,
> +						  buf);
> +	} else {
> +		/*
> +		 * The register is 1-byte wide, but 1-byte transactions are not
> +		 * allowed in 8D-8D-8D mode. Since there is no register at the
> +		 * next location, just initialize the value to 0 and let the
> +		 * transaction go on.
> +		 */
> +		buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS;
> +		buf[1] = 0;
> +		op = (struct spi_mem_op)
> +			CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes,
> +						  SPINOR_REG_CYPRESS_CFR5V, 2,
> +						  buf);
>  	}
>  
> -	if (memcmp(buf, nor->info->id, nor->info->id_len))
> -		return -EINVAL;
> -
> -	return 0;
> +	return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
>  }
>  
>  static int cypress_nor_quad_enable_volatile_reg(struct spi_nor *nor, u64 addr)
> @@ -642,8 +612,44 @@ static struct spi_nor_fixups s25hx_t_fixups = {
>   */
>  static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)

what a terrible name

>  {
> -	return enable ? cypress_nor_octal_dtr_en(nor) :
> -			cypress_nor_octal_dtr_dis(nor);
> +	int ret;
> +	u8 naddr, ndummy;
> +	enum spi_nor_protocol proto;
> +
> +	if (enable) {

so we have cypress_nor_octal_dtr_enable and now we check for enable,
yuck. I know it comes from the SPI NOR core, we shall update the core, I
wouldn't continue like this.

> +		ret = cypress_nor_setup_memlat(nor);
> +		if (ret)
> +			return ret;
> +
> +		nor->read_dummy = 24;

shouldn't this be set in cypress_nor_set_memlat?
> +	}
> +
> +	ret = cypress_nor_setup_opiddr(nor, enable);
> +	if (ret)
> +		return ret;
> +
> +	/* Read flash ID to make sure the switch was successful. */
> +	if (enable) {
> +		naddr = nor->addr_nbytes;
> +		ndummy = 3;
> +		proto = SNOR_PROTO_8_8_8_DTR;
> +	} else {
> +		naddr = 0;
> +		ndummy = 0;
> +		proto = SNOR_PROTO_1_1_1;
> +	}

I don't like all the if conditions in the octal_dtr_enable methods, I
find the method hard to read and I feel we are butchering the code just
to make it work.
> +
> +	ret = spi_nor_read_id(nor, naddr, ndummy, nor->bouncebuf, proto);
> +	if (ret) {
> +		dev_dbg(nor->dev, "error %d reading JEDEC ID after %s 8D-8D-8D mode\n",
> +			ret, enable ? "enabling" : "disabling");
> +		return ret;
> +	}
> +
> +	if (memcmp(nor->bouncebuf, nor->info->id, nor->info->id_len))
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/