[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <mafs0cygo5p2f.fsf@kernel.org>
Date: Wed, 15 Jan 2025 10:50:00 +0000
From: Pratyush Yadav <pratyush@...nel.org>
To: Tudor Ambarus <tudor.ambarus@...aro.org>
Cc: Pratyush Yadav <pratyush@...nel.org>, Michael Walle
<mwalle@...nel.org>, Miquel Raynal <miquel.raynal@...tlin.com>, Richard
Weinberger <richard@....at>, Vignesh Raghavendra <vigneshr@...com>,
Cheng Ming Lin <linchengming884@...il.com>, Alexander Stein
<alexander.stein@...tq-group.com>, linux-kernel@...r.kernel.org,
linux-mtd@...ts.infradead.org, alvinzhou@...c.com.tw,
leoyu@...c.com.tw, Cheng Ming Lin <chengminglin@...c.com.tw>
Subject: Re: [PATCH] Revert "mtd: spi-nor: core: replace dummy buswidth from
addr to data"
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@...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@...aro.org>
Thanks!
--
Regards,
Pratyush Yadav
Powered by blists - more mailing lists