Re: [PATCH v15 5/8] mtd: spi-nor: core: Couple the number of address bytes with the address mode

[Michael Walle <michael@walle.cc>]

Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Some of Infineon chips support volatile version of configuration 
> registers
> and it is recommended to update volatile registers in the field 
> application
> due to a risk of the non-volatile registers corruption by power 
> interrupt.
> Such a volatile configuration register is used to enable the Quad mode.
> The register write sequence requires the number of bytes of address in
> order to be programmed. As it was before, the nor->addr_nbytes was set 
> to 4
> before calling the volatile Quad enable method. This was incorrect 
> because
> the Write Any Register command does not have a 4B opcode equivalent and 
> the
> address mode was still at default (3-byte mode) and not changed to 4 by
> entering in the 4 Byte Address Mode, so the operation failed.
> 
> Move the setting of the number of bytes of address after the Quad 
> Enable
> method to allow reads or writes to registers that require the number of
> address bytes to work with the default address mode. The number of 
> address
> bytes and the address mode are tightly coupled, this is a natural 
> change.
> 
> Other (standard) Quad Enable methods are not affected, as they don't
> require the number of address bytes, so no functionality changes 
> expected.
> 
> Reported-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi-nor/core.c | 134 +++++++++++++++++++------------------
>  1 file changed, 70 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index dd71deba9f11..1c14a95a23fd 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2270,49 +2270,6 @@ static int spi_nor_default_setup(struct spi_nor 
> *nor,
>  	return 0;
>  }
> 
> -static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
> -{
> -	if (nor->params->addr_nbytes) {
> -		nor->addr_nbytes = nor->params->addr_nbytes;
> -	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> -		/*
> -		 * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
> -		 * in this protocol an odd address width cannot be used because
> -		 * then the address phase would only span a cycle and a half.
> -		 * Half a cycle would be left over. We would then have to start
> -		 * the dummy phase in the middle of a cycle and so too the data
> -		 * phase, and we will end the transaction with half a cycle left
> -		 * over.
> -		 *
> -		 * Force all 8D-8D-8D flashes to use an address width of 4 to
> -		 * avoid this situation.
> -		 */
> -		nor->addr_nbytes = 4;
> -	} else if (nor->info->addr_nbytes) {
> -		nor->addr_nbytes = nor->info->addr_nbytes;
> -	} else {
> -		nor->addr_nbytes = 3;
> -	}
> -
> -	if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
> -		/* enable 4-byte addressing if the device exceeds 16MiB */
> -		nor->addr_nbytes = 4;
> -	}
> -
> -	if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
> -		dev_dbg(nor->dev, "address width is too large: %u\n",
> -			nor->addr_nbytes);
> -		return -EINVAL;
> -	}
> -
> -	/* Set 4byte opcodes when possible. */
> -	if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
> -	    !(nor->flags & SNOR_F_HAS_4BAIT))
> -		spi_nor_set_4byte_opcodes(nor);
> -
> -	return 0;
> -}
> -
>  static int spi_nor_setup(struct spi_nor *nor,
>  			 const struct spi_nor_hwcaps *hwcaps)
>  {
> @@ -2322,10 +2279,7 @@ static int spi_nor_setup(struct spi_nor *nor,
>  		ret = nor->params->setup(nor, hwcaps);
>  	else
>  		ret = spi_nor_default_setup(nor, hwcaps);
> -	if (ret)
> -		return ret;
> -
> -	return spi_nor_set_addr_nbytes(nor);
> +	return ret;
>  }
> 
>  /**
> @@ -2707,6 +2661,74 @@ static int spi_nor_quad_enable(struct spi_nor 
> *nor)
>  	return nor->params->quad_enable(nor);
>  }
> 
> +static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
> +{
> +	if (nor->params->addr_nbytes) {
> +		nor->addr_nbytes = nor->params->addr_nbytes;
> +	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> +		/*
> +		 * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
> +		 * in this protocol an odd address width cannot be used because
> +		 * then the address phase would only span a cycle and a half.
> +		 * Half a cycle would be left over. We would then have to start
> +		 * the dummy phase in the middle of a cycle and so too the data
> +		 * phase, and we will end the transaction with half a cycle left
> +		 * over.
> +		 *
> +		 * Force all 8D-8D-8D flashes to use an address width of 4 to
> +		 * avoid this situation.
> +		 */
> +		nor->addr_nbytes = 4;
> +	} else if (nor->info->addr_nbytes) {
> +		nor->addr_nbytes = nor->info->addr_nbytes;
> +	} else {
> +		nor->addr_nbytes = 3;
> +	}
> +
> +	if (nor->addr_nbytes == 3 && nor->params->size > 0x1000000) {
> +		/* enable 4-byte addressing if the device exceeds 16MiB */
> +		nor->addr_nbytes = 4;
> +	}
> +
> +	if (nor->addr_nbytes > SPI_NOR_MAX_ADDR_NBYTES) {
> +		dev_dbg(nor->dev, "address width is too large: %u\n",
> +			nor->addr_nbytes);
> +		return -EINVAL;
> +	}
> +
> +	/* Set 4byte opcodes when possible. */
> +	if (nor->addr_nbytes == 4 && nor->flags & SNOR_F_4B_OPCODES &&
> +	    !(nor->flags & SNOR_F_HAS_4BAIT))
> +		spi_nor_set_4byte_opcodes(nor);
> +
> +	return 0;
> +}
> +
> +static int spi_nor_set_addr_mode(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	ret = spi_nor_set_addr_nbytes(nor);
> +	if (ret)
> +		return ret;
> +
> +	if (nor->addr_nbytes == 4 && nor->read_proto != SNOR_PROTO_8_8_8_DTR 
> &&
> +	    !(nor->flags & SNOR_F_4B_OPCODES)) {
> +		/*
> +		 * If the RESET# pin isn't hooked up properly, or the system
> +		 * otherwise doesn't perform a reset command in the boot
> +		 * sequence, it's impossible to 100% protect against unexpected
> +		 * reboots (e.g., crashes). Warn the user (or hopefully, system
> +		 * designer) that this is bad.
> +		 */
> +		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
> +			  "enabling reset hack; may not recover from unexpected 
> reboots\n");
> +		nor->params->set_4byte_addr_mode(nor, true);

Why don't we check the return code here? I know it was this
way before, but that looks wrong.

-michael

> +	}
> +
> +	return 0;
> +}
> +
>  static int spi_nor_init(struct spi_nor *nor)
>  {
>  	int err;
> @@ -2738,22 +2760,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  	     nor->flags & SNOR_F_SWP_IS_VOLATILE))
>  		spi_nor_try_unlock_all(nor);
> 
> -	if (nor->addr_nbytes == 4 &&
> -	    nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
> -	    !(nor->flags & SNOR_F_4B_OPCODES)) {
> -		/*
> -		 * If the RESET# pin isn't hooked up properly, or the system
> -		 * otherwise doesn't perform a reset command in the boot
> -		 * sequence, it's impossible to 100% protect against unexpected
> -		 * reboots (e.g., crashes). Warn the user (or hopefully, system
> -		 * designer) that this is bad.
> -		 */
> -		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
> -			  "enabling reset hack; may not recover from unexpected 
> reboots\n");
> -		nor->params->set_4byte_addr_mode(nor, true);
> -	}
> -
> -	return 0;
> +	return spi_nor_set_addr_mode(nor);
>  }
> 
>  /**
> @@ -3014,7 +3021,6 @@ int spi_nor_scan(struct spi_nor *nor, const char 
> *name,
>  	 * - select op codes for (Fast) Read, Page Program and Sector Erase.
>  	 * - set the number of dummy cycles (mode cycles + wait states).
>  	 * - set the SPI protocols for register and memory accesses.
> -	 * - set the address width.
>  	 */
>  	ret = spi_nor_setup(nor, hwcaps);
>  	if (ret)

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