Re: [PATCH 1/4] mtd: spi-nor: Rework set_4byte()

On 08.08.2024 09:00, tkuw584924@gmail.com wrote:

From: Takahiro Kuwano Takahiro.Kuwano@infineon.com

Synchronize set_4byte() with Linux v6.10 as much as possible.

Let's aim for spi-nor/for-v6.12.

Introduce {nor, params}->set_4byte_addr_mode(). The params->set_4byte_addr_mode is initialized with one of the manufacturer specific methods, copied to nor->set_4byte_addr_mode, then it is called from spi_nor_set_4byte_addr_mode() renamed from set_4byte().

this happens indeed. We need to explain why: we aim to split the manufacturer specific code out of the core, thus introduce set_4byte_addr_mode hook so that the manufacturers/SFDP be empowered to pin point a specific 4byte addr mode method.

There are still manufacturer checks here and there.

unfortunately.

And The set_4byte_addr_mode() method in both nor-> and params-> looks redundant. Those should be cleaned up separately in another patch set.

then don't introduce it in the first place :)

The following commits in Linux are related to this patch. 64c160f32235 ("mtd: spi-nor: Create a ->set_4byte() method") 81924dae5194 ("mtd: spi-nor: Emphasise which is the genericset_4byte_addr_mode() method") 076aa4eac8b3 ("mtd: spi-nor: core: Move generic method to core - micron_st_nor_set_4byte_addr_mode") 288df4378319 ("mtd: spi-nor: core: Update name and description of micron_st_nor_set_4byte_addr_mode") f1f1976224f3 ("mtd: spi-nor: core: Update name and description of spansion_set_4byte_addr_mode") b6094ac83dd4 ("mtd: spi-nor: core: Introduce spi_nor_set_4byte_addr_mode()")

yeah, these are good for reference.

Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com

drivers/mtd/spi/spi-nor-core.c | 179 +++++++++++++++++++++++++-------- include/linux/mtd/spi-nor.h | 3 + 2 files changed, 138 insertions(+), 44 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 8d2afaa0e2..d523c045f4 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -680,54 +680,123 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor, } #endif /* !CONFIG_SPI_FLASH_BAR */

-/* Enable/disable 4-byte addressing mode. */ -static int set_4byte(struct spi_nor *nor, const struct flash_info *info,

•      int enable)
  

+/**

• • spi_nor_set_4byte_addr_mode_en4b_ex4b() - Enter/Exit 4-byte address mode

• • 	using SPINOR_OP_EN4B/SPINOR_OP_EX4B. Typically used by
    

• • 	Winbond and Macronix. Cypress also uses SPINOR_OP_EN4B
    

• • 	to enter, but not SPINOR_OP_EX4B to exit.
    

• • @nor: pointer to 'struct spi_nor'.
• • @enable: true to enter the 4-byte address mode, false to exit the 4-byte

• • address mode.
    

• • Return: 0 on success, -errno otherwise.
• */

+static int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor,

• 				 bool enable)
  

{

• int status;
• bool need_wren = false;
• u8 cmd;

• u8 opcode;
• int ret;

• switch (JEDEC_MFR(info)) {
• case SNOR_MFR_ST:
• case SNOR_MFR_MICRON:

• /* Some Micron need WREN command; all will accept it */
  

• need_wren = true;
  

• fallthrough;
  

• case SNOR_MFR_ISSI:
• case SNOR_MFR_MACRONIX:
• case SNOR_MFR_WINBOND:

• if (need_wren)
  

• 	write_enable(nor);
  

• if (enable)

• opcode = SPINOR_OP_EN4B;
  

• else if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS)

• opcode = SPINOR_OP_EX4B_CYPRESS;
  

• else

• opcode = SPINOR_OP_EX4B;
  
  

• cmd = enable ? SPINOR_OP_EN4B : SPINOR_OP_EX4B;
  

• status = nor->write_reg(nor, cmd, NULL, 0);
  

• if (need_wren)
  

• 	write_disable(nor);
  

• ret = nor->write_reg(nor, opcode, NULL, 0);
• if (ret)

• return ret;
  
  

• if (!status && !enable &&
  

•     JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
  

• 	/*
  

• 	 * On Winbond W25Q256FV, leaving 4byte mode causes
  

• 	 * the Extended Address Register to be set to 1, so all
  

• 	 * 3-byte-address reads come from the second 16M.
  

• 	 * We must clear the register to enable normal behavior.
  

• 	 */
  

• 	write_enable(nor);
  

• 	nor->cmd_buf[0] = 0;
  

• 	nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1);
  

• 	write_disable(nor);
  

• }
  

• /*

• * On Winbond W25Q256FV, leaving 4byte mode causes the Extended
  

please follow linux, and introduce winbond_nor_set_4byte_addr_mode() for the chunk below.

• * Address Register to be set to 1, so all 3-byte-address reads
  

• * come from the second 16M. We must clear the register to
  

• * enable normal behavior.
  

• */
  

• if (!enable && JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) {

• ret = write_enable(nor);
  

• if (ret)
  

• 	return ret;
  
  

• return status;
  

• case SNOR_MFR_CYPRESS:

• cmd = enable ? SPINOR_OP_EN4B : SPINOR_OP_EX4B_CYPRESS;
  

• return nor->write_reg(nor, cmd, NULL, 0);
  

• default:

• /* Spansion style */
  

• nor->cmd_buf[0] = enable << 7;
  

• return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1);
  

• nor->cmd_buf[0] = 0;
  

• ret = nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf,
  

• 			1);
  

• if (ret)
  

• 	return ret;
  

• ret = write_disable(nor);
  

• if (ret)
  

• 	return ret;
  

• }
• return 0;

+}

+/**

• • spi_nor_set_4byte_addr_mode_wren_en4b_ex4b() - Set 4-byte address mode using

• • 	SPINOR_OP_WREN followed by SPINOR_OP_EN4B or
    

• • 	SPINOR_OP_EX4B. Typically used by ST and Micron flashes.
    

• • @nor: pointer to 'struct spi_nor'.
• • @enable: true to enter the 4-byte address mode, false to exit the 4-byte

• • address mode.
    

• • Return: 0 on success, -errno otherwise.
• */

+static int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor,

• 				      bool enable)
  

+{

• int ret;
• ret = write_enable(nor);
• if (ret)

• return ret;
  

• ret = spi_nor_set_4byte_addr_mode_en4b_ex4b(nor, enable);
• if (ret)

• return ret;
  

• return write_disable(nor);

+}

+/**

• • spi_nor_set_4byte_addr_mode_brwr() - Set 4-byte address mode using

• • 	SPINOR_OP_BRWR. Typically used by Spansion flashes.
    

• • @nor: pointer to 'struct spi_nor'.
• • @enable: true to enter the 4-byte address mode, false to exit the 4-byte

• • address mode.
    

• • 8-bit volatile bank register used to define A[30:A24] bits. MSB (bit[7]) is
• • used to enable/disable 4-byte address mode. When MSB is set to ‘1’, 4-byte
• • address mode is active and A[30:24] bits are don’t care. Write instruction is
• • SPINOR_OP_BRWR(17h) with 1 byte of data.
• • Return: 0 on success, -errno otherwise.
• */

+static int spi_nor_set_4byte_addr_mode_brwr(struct spi_nor *nor, bool enable) +{

• nor->cmd_buf[0] = enable ? BIT(7) : 0;

what's wrong with enable << 7? Keep the code as it was, don't update it on the fly.

• return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1);

+}

+/* Enable/disable 4-byte addressing mode. */ +static int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable) +{

• int ret;

follow linux and do the if (nor->flags & SNOR_F_BROKEN_RESET) check here. You'll have a good debug message for your flashes too ...

• ret = nor->set_4byte_addr_mode(nor, enable);
• if (ret)

• return ret;
  

• if (enable) {

• nor->addr_width = 4;
  

• nor->addr_mode_nbytes = 4;
  

• } else {

• nor->addr_width = 3;
  

• nor->addr_mode_nbytes = 3;
  

  }

I don't see where were these done previously. Thus don't introduce new code. If needed, make a dedicated patch and explain why you need these.

• return 0;

}

#ifdef CONFIG_SPI_FLASH_SPANSION @@ -2884,6 +2953,25 @@ static int spi_nor_init_params(struct spi_nor *nor, } }

• /* Select the procedure to enter/exit 4-byte address mode. */
• switch (JEDEC_MFR(info)) {
• case SNOR_MFR_ST:
• case SNOR_MFR_MICRON:

• params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
  

• break;
  

• case SNOR_MFR_ISSI:
• case SNOR_MFR_MACRONIX:
• case SNOR_MFR_WINBOND:
• case SNOR_MFR_CYPRESS:

• params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_en4b_ex4b;
  

• break;
  

• default:

• params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
  

• break;
  

• }

follow linux, you need to set these after parsing SFDP

/* Override the parameters with data read from SFDP tables. */ nor->addr_width = 0; nor->mtd.erasesize = 0; @@ -3272,6 +3360,9 @@ static int spi_nor_default_setup(struct spi_nor *nor, else nor->quad_enable = NULL;

• /* Enter/Exit 4-byte address mode */
• nor->set_4byte_addr_mode = params->set_4byte_addr_mode;

you won't need nor->set_4byte_addr_mode, don't introduce it.

As a general comment, follow linux, just move code, don't introduce new code. No changes in functionality shall be expected with this rework.

Looking good, thanks for working on this. Cheers, ta