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

Michael Walle
2022-05-12T22:07:48Z

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)