Re: [PATCH v1 2/4] mtd: spi-nor: Support mixed 1S-xD-xD DTR protocols in spi_nor_spimem_setup_op()

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4189 bytes --]

On Thu Oct 22, 2026 at 12:45 PM CEST, Miquel Raynal wrote:
> On 22/10/2026 at 03:26:12 -07, Changhuang Liang <changhuang.liang@starfivetech.com> wrote:
>
>> Currently spi_nor_spimem_setup_op() sets all four phases (cmd, addr,
>> dummy, data) to DTR whenever spi_nor_protocol_is_dtr(proto) is true.
>> This is too coarse: the 1S-xD-xD DTR protocols (1_1_1_DTR, 1_2_2_DTR,
>> 1_4_4_DTR, 1_8_8_DTR) send the command phase in STR on a single line,
>> and only the address, dummy and data phases in DTR. SPIMEM supports
>> such mixed DTR modes.
>>
>> Split the handling:
>>
>>   - For SNOR_PROTO_8_8_8_DTR, keep the existing behavior: set all
>>     phases to DTR and use a 2-byte opcode (opcode + extension).
>>
>>   - For the other DTR protocols, set the command phase to STR and only
>>     the address, dummy and data phases to DTR. Double op->dummy.nbytes
>>     since DTR transfers 2 bytes per clock cycle, matching how the dummy
>>     cycle count is specified for these protocols.
>>
>> No in-tree flash currently declares SNOR_HWCAPS_READ_1_x_x_DTR, so
>> this is not a bug fix but preparation for enabling these protocols.
>> A follow-up patch will enable 1S-4D-4D for the MX66L1G45G. The
>> 8D-8D-8D path is unchanged.
>>
>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>> ---
>>  drivers/mtd/spi-nor/core.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 90524c58602e..43e506e728ab 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -97,13 +97,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
>>  	if (op->data.nbytes)
>>  		op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
>>  
>> -	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.
>> -		 */
>> +	if (proto == SNOR_PROTO_8_8_8_DTR) {
>>  		op->cmd.dtr = true;
>>  		op->addr.dtr = true;
>>  		op->dummy.dtr = true;
>> @@ -115,6 +109,19 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
>>  		ext = spi_nor_get_cmd_ext(nor, op);
>>  		op->cmd.opcode = (op->cmd.opcode << 8) | ext;
>>  		op->cmd.nbytes = 2;
>> +	} else if (spi_nor_protocol_is_dtr(proto)) {
>> +		/*
>> +		 * 1S-xD-xD protocols: the opcode is sent in STR on a single
>> +		 * line, only the address, dummy and data phases are DTR. The
>> +		 * opcode stays 1 byte (no command extension).
>> +		 */
>> +		op->cmd.dtr = false;
>> +		op->addr.dtr = true;
>> +		op->dummy.dtr = true;
>> +		op->data.dtr = true;
>
> Still to coarse IMHO.
>
> What about xS-xS-xD?

Is this a thing? Are there any NOR flashes which support the address
being SDR? That doesn't make much sense, does it? The command being
SDR makes sense because you don't have to switch modes and thus
you're stateless.

Also spi-nor.h only defines 1S-nD-nD (for n=2,4,8) for DTR. IIRC the
original DTR code was just for 8D-8D-8D because that is actually
backed by the xSPI standard.

Anyway, can we just have something like:


if (spi_nor_protocol_is_dtr(proto)) {
		/* we only support 1SnDnD or 8D8D8D */
		op->addr.dtr = true;
		op->dummy.dtr = true;
		op->data.dtr = true;

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

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

Also you missed the support for SNOR_F_SWAP16.

-michael

> As mentioned in the spi controller patch you must make sure these are
> refused if they are not supported by your controller because SPI NAND at
> least allow them (and I believe SPI NOR might as well)?.
>
>> +
>> +		/* 2 bytes per clock cycle in DTR mode. */
>> +		op->dummy.nbytes *= 2;
>>  	}
>>  
>>  	if (proto == SNOR_PROTO_8_8_8_DTR && params->flags & SNOR_F_SWAP16)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]