Re: [PATCH RESEND v3] mtd: spi-nor: micron-st: Add support for is25wx01g

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: spi-nor: micron-st: missing SPI_NOR_IO_MODE_EN_VOLATILE for is25wx01g
--

commit 4a6df57d42f0427ebe6bde7c75e5852f84d2c7c8
Author: Nuno Sá <nuno.sa@analog.com>

mtd: spi-nor: micron-st: Add support for is25wx01g

This commit adds support for the is25wx01g octal flash in the micron-st
driver, including its flash info entry and a post_bfpt fixup to clear
read_sr2 and write_sr1_and_sr2.

> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index f97f2dead8c2f..eba07efdca753 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c

[ ... ]

> @@ -655,6 +680,7 @@ static const struct spi_nor_fixups micron_st_nor_fixups = {
>  
>  static const struct spi_nor_fixup micron_fixups[] = {
>  	{ .fixups = &micron_st_nor_fixups },
> +	{ .id = SNOR_ID(0x9d, 0x5b, 0x1b), .fixups = &is25wx01g_fixups },

[Severity: High]
Should the micron_fixups[] entry for is25wx01g in
drivers/mtd/spi-nor/micron-st.c also set:

	.fixup_flags = SPI_NOR_IO_MODE_EN_VOLATILE

like the adjacent mt35xu entries?

During spi_nor_scan() -> spi_nor_init_params() -> spi_nor_parse_sfdp(),
spi_nor_parse_profile1() populates SNOR_HWCAPS_READ_8_8_8_DTR and
SNOR_HWCAPS_PP_8_8_8_DTR in nor->params->hwcaps.mask:

drivers/mtd/spi-nor/sfdp.c:spi_nor_parse_profile1() {
	...
	nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
	spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_8_8_8_DTR],
				  0, dummy, opcode,
				  SNOR_PROTO_8_8_8_DTR);
	...
	nor->params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;
	...
}

Because the sfdp tables on this part do not include the sccr map table
(0xff87), spi_nor_parse_sccr() is never called and SNOR_F_IO_MODE_EN_VOLATILE
is not set during sfdp parsing.

Later, spi_nor_late_init_params() calls micron_st_nor_late_init(), which
assigns params->set_octal_dtr = micron_st_nor_set_octal_dtr, and
spi_nor_init_fixup_flags() applies fixup_flags from micron_fixups[]. Without
SPI_NOR_IO_MODE_EN_VOLATILE in micron_fixups[], SNOR_F_IO_MODE_EN_VOLATILE
remains unset in nor->params->flags.

When attached to an 8d-8d-8d capable controller, spi_nor_setup() selects
SNOR_PROTO_8_8_8_DTR for nor->read_proto and nor->write_proto, and
spi_nor_init() then calls spi_nor_set_octal_dtr(nor, true):

drivers/mtd/spi-nor/core.c:spi_nor_set_octal_dtr() {
	...
	if (!nor->params->set_octal_dtr)
		return 0;

	if (!(nor->params->flags & SNOR_F_IO_MODE_EN_VOLATILE))
		return 0;

	ret = nor->params->set_octal_dtr(nor, enable);
	if (ret)
		return ret;

	if (enable)
		nor->reg_proto = SNOR_PROTO_8_8_8_DTR;
	else
		nor->reg_proto = SNOR_PROTO_1_1_1;

	return 0;
}

Wouldn't spi_nor_set_octal_dtr() return 0 early without calling
micron_st_nor_set_octal_dtr() or updating nor->reg_proto, leaving the flash
in 1s-1s-1s mode while the controller uses SNOR_PROTO_8_8_8_DTR for reads
and writes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-mtd-spi-nor-new-issi-chip-v3-1-f5e38e200fc8@analog.com?part=1

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