[PATCH] Revert "mtd: spi-nor: core: replace dummy buswidth from addr to data"

[PATCH] Revert "mtd: spi-nor: core: replace dummy buswidth from addr to data"

[Pratyush Yadav]

This reverts commit 98d1fb94ce75f39febd456d6d3cbbe58b6678795. The commit
uses data nbits instead of addr nbits for dummy phase. This causes a
regression for all boards where spi-tx-bus-width is smaller than
spi-rx-bus-width. It is a common pattern for boards to have
spi-tx-bus-width == 1 and spi-rx-bus-width > 1. The regression causes
all reads with a dummy phase to become unavailable for such boards,
leading to a usually slower 0-dummy-cycle read being selected.

Most controllers' supports_op hooks call spi_mem_default_supports_op().
In spi_mem_default_supports_op(), spi_mem_check_buswidth() is called to
check if the buswidths for the op can actually be supported by the
board's wiring. This wiring information comes from (among other things)
the spi-{tx,rx}-bus-width DT properties. Based on these properties,
SPI_TX_* or SPI_RX_* flags are set by of_spi_parse_dt().
spi_mem_check_buswidth() then uses these flags to make the decision
whether an op can be supported by the board's wiring (in a way,
indirectly checking against spi-{rx,tx}-bus-width).

Now the tricky bit here is that spi_mem_check_buswidth() does:

	if (op->dummy.nbytes &&
	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
		return false;

The true argument to spi_check_buswidth_req() means the op is treated as
a TX op. For a board that has say 1-bit TX and 4-bit RX, a 4-bit dummy
TX is considered as unsupported, and the op gets rejected.

The commit being reverted uses the data buswidth for dummy buswidth. So
for reads, the RX buswidth gets used for the dummy phase, uncovering
this issue. In reality, a dummy phase is neither RX nor TX. As the name
suggests, these are just dummy cycles that send or receive no data, and
thus don't really need to have any buswidth at all.

Ideally, dummy phases should not be checked against the board's wiring
capabilities at all, and should only be sanity-checked for having a sane
buswidth value. Since we are now at rc7 and such a change might
introduce many unexpected bugs, revert the commit for now. It can be
sent out later along with the spi_mem_check_buswidth() fix.

Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Closes: https://lore.kernel.org/linux-mtd/3342163.44csPzL39Z@steina-w/
Signed-off-by: Pratyush Yadav <pratyush@kernel.org>
---

I have only compile-tested this patch. Alexander, if you can send your Tested-by
it would be great!

Miquel, since the bug was introduced in v6.13-rc1, I want to have this patch go
in v6.13. So you would have to send Linus a pull request before he likely
releases it this Sunday. How do you want to take the patch? Do you want to apply
it directly to mtd/fixes or should I send out a pull request from a
spi-nor/fixes branch?

 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 66949d9f0cc5a..b6f374ded390a 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -89,7 +89,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
 		op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
 
 	if (op->dummy.nbytes)
-		op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
+		op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
 
 	if (op->data.nbytes)
 		op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
-- 
2.40.1

Re: [PATCH] Revert "mtd: spi-nor: core: replace dummy buswidth from addr to data"

[Tudor Ambarus]

On 1/15/25 10:16 AM, Pratyush Yadav wrote:
> This reverts commit 98d1fb94ce75f39febd456d6d3cbbe58b6678795. The commit

"The commit" on a new paragraph please

> uses data nbits instead of addr nbits for dummy phase. This causes a
> regression for all boards where spi-tx-bus-width is smaller than
> spi-rx-bus-width. It is a common pattern for boards to have
> spi-tx-bus-width == 1 and spi-rx-bus-width > 1. The regression causes
> all reads with a dummy phase to become unavailable for such boards,
> leading to a usually slower 0-dummy-cycle read being selected.
> 
> Most controllers' supports_op hooks call spi_mem_default_supports_op().
> In spi_mem_default_supports_op(), spi_mem_check_buswidth() is called to
> check if the buswidths for the op can actually be supported by the
> board's wiring. This wiring information comes from (among other things)
> the spi-{tx,rx}-bus-width DT properties. Based on these properties,
> SPI_TX_* or SPI_RX_* flags are set by of_spi_parse_dt().
> spi_mem_check_buswidth() then uses these flags to make the decision
> whether an op can be supported by the board's wiring (in a way,
> indirectly checking against spi-{rx,tx}-bus-width).
> 
> Now the tricky bit here is that spi_mem_check_buswidth() does:
> 
> 	if (op->dummy.nbytes &&
> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
> 		return false;
> 
> The true argument to spi_check_buswidth_req() means the op is treated as
> a TX op. For a board that has say 1-bit TX and 4-bit RX, a 4-bit dummy
> TX is considered as unsupported, and the op gets rejected.
> 
> The commit being reverted uses the data buswidth for dummy buswidth. So
> for reads, the RX buswidth gets used for the dummy phase, uncovering
> this issue. In reality, a dummy phase is neither RX nor TX. As the name
> suggests, these are just dummy cycles that send or receive no data, and
> thus don't really need to have any buswidth at all.
> 
> Ideally, dummy phases should not be checked against the board's wiring
> capabilities at all, and should only be sanity-checked for having a sane
> buswidth value. Since we are now at rc7 and such a change might
> introduce many unexpected bugs, revert the commit for now. It can be
> sent out later along with the spi_mem_check_buswidth() fix.
> 
> Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Closes: https://lore.kernel.org/linux-mtd/3342163.44csPzL39Z@steina-w/

fixes tag please

with that:
Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Re: [PATCH] Revert "mtd: spi-nor: core: replace dummy buswidth from addr to data"

[Alexander Stein]

Am Mittwoch, 15. Januar 2025, 11:16:52 CET schrieb Pratyush Yadav:
> This reverts commit 98d1fb94ce75f39febd456d6d3cbbe58b6678795. The commit
> uses data nbits instead of addr nbits for dummy phase. This causes a
> regression for all boards where spi-tx-bus-width is smaller than
> spi-rx-bus-width. It is a common pattern for boards to have
> spi-tx-bus-width == 1 and spi-rx-bus-width > 1. The regression causes
> all reads with a dummy phase to become unavailable for such boards,
> leading to a usually slower 0-dummy-cycle read being selected.
> 
> Most controllers' supports_op hooks call spi_mem_default_supports_op().
> In spi_mem_default_supports_op(), spi_mem_check_buswidth() is called to
> check if the buswidths for the op can actually be supported by the
> board's wiring. This wiring information comes from (among other things)
> the spi-{tx,rx}-bus-width DT properties. Based on these properties,
> SPI_TX_* or SPI_RX_* flags are set by of_spi_parse_dt().
> spi_mem_check_buswidth() then uses these flags to make the decision
> whether an op can be supported by the board's wiring (in a way,
> indirectly checking against spi-{rx,tx}-bus-width).
> 
> Now the tricky bit here is that spi_mem_check_buswidth() does:
> 
> 	if (op->dummy.nbytes &&
> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
> 		return false;
> 
> The true argument to spi_check_buswidth_req() means the op is treated as
> a TX op. For a board that has say 1-bit TX and 4-bit RX, a 4-bit dummy
> TX is considered as unsupported, and the op gets rejected.
> 
> The commit being reverted uses the data buswidth for dummy buswidth. So
> for reads, the RX buswidth gets used for the dummy phase, uncovering
> this issue. In reality, a dummy phase is neither RX nor TX. As the name
> suggests, these are just dummy cycles that send or receive no data, and
> thus don't really need to have any buswidth at all.
> 
> Ideally, dummy phases should not be checked against the board's wiring
> capabilities at all, and should only be sanity-checked for having a sane
> buswidth value. Since we are now at rc7 and such a change might
> introduce many unexpected bugs, revert the commit for now. It can be
> sent out later along with the spi_mem_check_buswidth() fix.
> 
> Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Closes: https://lore.kernel.org/linux-mtd/3342163.44csPzL39Z@steina-w/
> Signed-off-by: Pratyush Yadav <pratyush@kernel.org>
> ---
> 
> I have only compile-tested this patch. Alexander, if you can send your Tested-by
> it would be great!

Sure, thanks also for the great and lengthy explanation.
Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> Miquel, since the bug was introduced in v6.13-rc1, I want to have this patch go
> in v6.13. So you would have to send Linus a pull request before he likely
> releases it this Sunday. How do you want to take the patch? Do you want to apply
> it directly to mtd/fixes or should I send out a pull request from a
> spi-nor/fixes branch?
> 
>  drivers/mtd/spi-nor/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 66949d9f0cc5a..b6f374ded390a 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -89,7 +89,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
>  		op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
>  
>  	if (op->dummy.nbytes)
> -		op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
> +		op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
>  
>  	if (op->data.nbytes)
>  		op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH] Revert "mtd: spi-nor: core: replace dummy buswidth from addr to data"

[Pratyush Yadav]

On Wed, Jan 15 2025, Tudor Ambarus wrote:

> On 1/15/25 10:16 AM, Pratyush Yadav wrote:
>> This reverts commit 98d1fb94ce75f39febd456d6d3cbbe58b6678795. The commit
>
> "The commit" on a new paragraph please

ACK, will do when applying.

>
>> uses data nbits instead of addr nbits for dummy phase. This causes a
>> regression for all boards where spi-tx-bus-width is smaller than
>> spi-rx-bus-width. It is a common pattern for boards to have
>> spi-tx-bus-width == 1 and spi-rx-bus-width > 1. The regression causes
>> all reads with a dummy phase to become unavailable for such boards,
>> leading to a usually slower 0-dummy-cycle read being selected.
>> 
>> Most controllers' supports_op hooks call spi_mem_default_supports_op().
>> In spi_mem_default_supports_op(), spi_mem_check_buswidth() is called to
>> check if the buswidths for the op can actually be supported by the
>> board's wiring. This wiring information comes from (among other things)
>> the spi-{tx,rx}-bus-width DT properties. Based on these properties,
>> SPI_TX_* or SPI_RX_* flags are set by of_spi_parse_dt().
>> spi_mem_check_buswidth() then uses these flags to make the decision
>> whether an op can be supported by the board's wiring (in a way,
>> indirectly checking against spi-{rx,tx}-bus-width).
>> 
>> Now the tricky bit here is that spi_mem_check_buswidth() does:
>> 
>> 	if (op->dummy.nbytes &&
>> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
>> 		return false;
>> 
>> The true argument to spi_check_buswidth_req() means the op is treated as
>> a TX op. For a board that has say 1-bit TX and 4-bit RX, a 4-bit dummy
>> TX is considered as unsupported, and the op gets rejected.
>> 
>> The commit being reverted uses the data buswidth for dummy buswidth. So
>> for reads, the RX buswidth gets used for the dummy phase, uncovering
>> this issue. In reality, a dummy phase is neither RX nor TX. As the name
>> suggests, these are just dummy cycles that send or receive no data, and
>> thus don't really need to have any buswidth at all.
>> 
>> Ideally, dummy phases should not be checked against the board's wiring
>> capabilities at all, and should only be sanity-checked for having a sane
>> buswidth value. Since we are now at rc7 and such a change might
>> introduce many unexpected bugs, revert the commit for now. It can be
>> sent out later along with the spi_mem_check_buswidth() fix.
>> 
>> Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>> Closes: https://lore.kernel.org/linux-mtd/3342163.44csPzL39Z@steina-w/
>
> fixes tag please

Hmm, I thought a fixes for a revert might be pointless since we have the
commit hash in the message anyway. I don't have a strong opinion, so
will add the below when applying:

Fixes: 98d1fb94ce75 ("mtd: spi-nor: core: replace dummy buswidth from addr to data")

>
> with that:
> Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Thanks!

-- 
Regards,
Pratyush Yadav

Re: [PATCH] Revert "mtd: spi-nor: core: replace dummy buswidth from addr to data"

[Tudor Ambarus]

On 1/15/25 10:50 AM, Pratyush Yadav wrote:
>> fixes tag please
> Hmm, I thought a fixes for a revert might be pointless since we have the
> commit hash in the message anyway. I don't have a strong opinion, so
> will add the below when applying:

it's useful for scripts searching for fixes of fixes.

Re: [PATCH] Revert "mtd: spi-nor: core: replace dummy buswidth from addr to data"

[Miquel Raynal]

On 15/01/2025 at 10:50:00 GMT, Pratyush Yadav <pratyush@kernel.org> wrote:

> On Wed, Jan 15 2025, Tudor Ambarus wrote:
>
>> On 1/15/25 10:16 AM, Pratyush Yadav wrote:
>>> This reverts commit 98d1fb94ce75f39febd456d6d3cbbe58b6678795. The commit
>>
>> "The commit" on a new paragraph please
>
> ACK, will do when applying.

I can apply the fix on mtd/fixes directly and fire the fixes PR one day
later, but I'll need your v2.

Thanks,
Miquèl