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

[Pratyush Yadav <pratyush@kernel.org>]

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

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