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

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

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

In octal DTR mode, RD_ANY_REG_OP needs to use 4-byte address regardless
of flash's internal address mode. Use nor->addr_nbytes which is set to 4
during setup.

Fixes: eff9604390d6 ("mtd: spi-nor: spansion: add octal DTR support in RD_ANY_REG_OP")
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi-nor/spansion.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index d6c92595f6bc..5a88a6096ca8 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -106,6 +106,7 @@ static int cypress_nor_sr_ready_and_clear_reg(struct spi_nor *nor, u64 addr)
 	int ret;
 
 	if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
+		op.addr.nbytes = nor->addr_nbytes;
 		op.dummy.nbytes = params->rdsr_dummy;
 		op.data.nbytes = 2;
 	}
-- 
2.34.1


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

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

[Pratyush Yadav]

On Wed, Oct 16 2024, tkuw584924@gmail.com wrote:

> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>
> In octal DTR mode, RD_ANY_REG_OP needs to use 4-byte address regardless
> of flash's internal address mode. Use nor->addr_nbytes which is set to 4
> during setup.

If the flash is in Octal DTR mode, shouldn't addr_mode_nbytes also be 4?
IIUC addr_mode_nbytes is supposed to track the flash's internal address
mode. If the flash goes into Octal DTR mode then its internal address
mode switches to 4, and we should update params->addr_mode_nbytes as
well. We do that in spi_nor_set_4byte_addr_mode() for example.

I suppose the best place to do it for Octal DTR would be
spi_nor_set_octal_dtr().

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.

>
> Fixes: eff9604390d6 ("mtd: spi-nor: spansion: add octal DTR support in RD_ANY_REG_OP")
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi-nor/spansion.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index d6c92595f6bc..5a88a6096ca8 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -106,6 +106,7 @@ static int cypress_nor_sr_ready_and_clear_reg(struct spi_nor *nor, u64 addr)
>  	int ret;
>  
>  	if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
> +		op.addr.nbytes = nor->addr_nbytes;
>  		op.dummy.nbytes = params->rdsr_dummy;
>  		op.data.nbytes = 2;
>  	}

-- 
Regards,
Pratyush Yadav

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

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

[Miquel Raynal]

Hi Pratyush,

pratyush@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?

Thanks,
Miquèl

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

Re: [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@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

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

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

[Takahiro Kuwano]

Hi Pratyush,

On 10/16/2024 8:59 PM, Pratyush Yadav wrote:
> On Wed, Oct 16 2024, tkuw584924@gmail.com wrote:
> 
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> In octal DTR mode, RD_ANY_REG_OP needs to use 4-byte address regardless
>> of flash's internal address mode. Use nor->addr_nbytes which is set to 4
>> during setup.
> 
> If the flash is in Octal DTR mode, shouldn't addr_mode_nbytes also be 4?
> IIUC addr_mode_nbytes is supposed to track the flash's internal address
> mode. If the flash goes into Octal DTR mode then its internal address
> mode switches to 4, and we should update params->addr_mode_nbytes as
> well. We do that in spi_nor_set_4byte_addr_mode() for example.
> 
The flash's internal address mode is represented by CFR2[7] (ADRBYT) and
addr_mode_nbytes tracks the state of this bit. If the flash goes into Octal
DTR mode, this configuration bit is unchanged and referred after exit from
Octal DTR mode. We need to remember addr_mode_nbytes for MTD suspend/resume
event that exits/enter Octal DTR mode.

> I suppose the best place to do it for Octal DTR would be
> spi_nor_set_octal_dtr().
> 
> 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.
> 
Agree. This is because flash's behavior itself is confusing:(

>>
>> Fixes: eff9604390d6 ("mtd: spi-nor: spansion: add octal DTR support in RD_ANY_REG_OP")
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi-nor/spansion.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index d6c92595f6bc..5a88a6096ca8 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -106,6 +106,7 @@ static int cypress_nor_sr_ready_and_clear_reg(struct spi_nor *nor, u64 addr)
>>  	int ret;
>>  
>>  	if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
>> +		op.addr.nbytes = nor->addr_nbytes;
>>  		op.dummy.nbytes = params->rdsr_dummy;
>>  		op.data.nbytes = 2;
>>  	}
> 

Thanks,
Takahiro


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

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

[Tudor Ambarus]

On 10/16/24 1:08 AM, tkuw584924@gmail.com wrote:
> In octal DTR mode, RD_ANY_REG_OP needs to use 4-byte address regardless
> of flash's internal address mode. Use nor->addr_nbytes which is set to 4
> during setup.

Applied, thanks.

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