[PATCH] mtd: spi-nor: spansion: Use nor->addr_nbytes in octal DTR mode in RD_ANY_REG_OP

[Pratyush Yadav]

Hi,

On Wed, Oct 16 2024, Miquel Raynal wrote:

> Hi Pratyush,
>
> pratyush at kernel.org wrote on Wed, 16 Oct 2024 13:59:24 +0200:
>
>> Side note: honestly, this whole thing with params->addr_nbytes,
>> params->addr_mode_nbytes, and nor->addr_nbytes is quite confusing. I
>> hope to find some time to clean it up some day.
>
> I am currently looking into dtr for spi-nands, would you mind quickly
> going through the approaches you think don't work well/would be better?

I don't think the addr_mode_nbytes and addr_nbytes has a lot to do with
Octal DTR. We just forgot to record the fact that internal address mode
changes when enabling Octal DTR.

It has been some time since I worked on it so I have forgotten some
details. I remember working with Apurva Nandan on doing Octal DTR on SPI
NANDs. He sent out 3 revisions of a series [0], but IIRC we didn't quite
nail down the design and see it through. Below is a random collection of
ideas and things I can remember.

For one, I think I didn't do the Octal DTR support in SPI NOR as well as
I could have. It is kind of shoehorned into the core and not very neat.
For example, we create an op in functions like
spi_nor_spimem_read_data() or spi_nor_spimem_write_data() that is
partially filled in with the opcode, address width, etc. and then
spi_nor_spimem_setup_op() fills in the rest of the data, adjusting
things on the fly.

	if (spi_nor_protocol_is_dtr(proto)) {
		/*
		 * SPIMEM supports mixed DTR modes, but right now we can only
		 * have all phases either DTR or STR. IOW, SPIMEM can have
		 * something like 4S-4D-4D, but SPI NOR can't. So, set all 4
		 * phases to either DTR or STR.
		 */
		op->cmd.dtr = true;
		op->addr.dtr = true;
		op->dummy.dtr = true;
		op->data.dtr = true;

		/* 2 bytes per clock cycle in DTR mode. */
		op->dummy.nbytes *= 2;

		ext = spi_nor_get_cmd_ext(nor, op);
		op->cmd.opcode = (op->cmd.opcode << 8) | ext;
		op->cmd.nbytes = 2;
	}

This is hacky. It allowed me to implement this without a major refactor
of the core, but it makes things harder to understand and debug, and
harder to extend in the future to support more modes.

I think SPI NAND already does the right thing by having
spinand_op_variants. I think you should use those for Octal DTR ops as
well, and that is what Apurva's patch series already does.

Another problem is how we enable the modes. We select each of read, page
program, and erase operations individually in spi_nor_setup(). But if
you select an 8D-8D-8D read, you _have_ select page program and erases
in the same mode. While it does happen in practice, there is nothing in
the code that makes sure of it. Tying the op selection to an overall
mode would help scale to supporting other modes like 8S-8S-8D or
4S-4S-4D. I see SPI NAND doing something similar via
spinand_match_and_init() and spinand_init_quad_enable(). Similarly, once
you have an overall mode you want to enable, instead of doing
spinand_init_quad_enable() and spinand_init_octal_enable(), etc. in
series and seeing which one accepts the command, you can just call the
enable function for that mode. Apurva's patches don't do that so you'd
have to refactor them.

One problem we didn't really solve in SPI NOR yet is mode detection.
When a flash comes up, how do you detect if it is 1S-1S-1S or 8D-8D-8D
or something else? One idea was to use the Read SFDP command in all
modes and see which one comes up with correct signature. For now, SPI
NOR assumes the flash is in 1S-1S-1S mode. IIUC there is nothing similar
to SFDP for NANDs, so you might have to have that information in device
tree, or detect some other way.

Overall, I think Apurva's patches are a good starting point that you
should use and fix/improve parts you don't like. We already spent some
time figuring out the design with Boris, so they should be in decent
shape already.

Hope the brain dump helps, and gives you what you were looking for. Let
me know if you would like to know something else.

[0] https://lore.kernel.org/linux-mtd/20220101074250.14443-1-a-nandan@ti.com/

-- 
Regards,
Pratyush Yadav