Re: [PATCH v2 1/1] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello Pratyush,

On 14/01/2025 at 16:15:24 GMT, Pratyush Yadav <pratyush@kernel.org> wrote:

>>> --- 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_addr_nbits(proto);
>>> +		op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);

Facing recently a similar issue myself in the SPI NAND world, I believe
we should get rid of the notion of bits when it comes to the dummy
phase. I would appreciate a clarification like "dummy.cycles" which
would typically not require any bus width implications.

...

> Most controller's supports_op hook call spi_mem_default_supports_op(),
> including nxp_fspi_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).

Thanks for the whole explanation, it's pretty clear.

> In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:
>
> 	flash0: flash@0 {
> 		reg = <0>;
> 		compatible = "jedec,spi-nor";
> 		spi-max-frequency = <80000000>;
> 		spi-tx-bus-width = <1>;
> 		spi-rx-bus-width = <4>;
>
> Now the tricky bit here is we do the below in spi_mem_check_buswidth():
>
> 	if (op->dummy.nbytes &&
> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
> 		return false;

May I challenge this entire section? Is there *any* reason to check
anything against dummy cycles wrt the width? Maybe a "can handle x
cycles" check would be interesting though, but I'd go for a different
helper, that is specific to the dummy cycles.

> The "true" parameter here means to "treat the op as TX". Since the
> board only supports 1-bit TX, the 4-bit dummy TX is considered as
> unsupported, and the op gets rejected. In reality, a dummy phase is
> neither a RX nor a TX. We should ideally treat it differently, and
> only check if it is one of 1, 2, 4, or 8, and not test it against the
> board capabilities at all.

...

> Since we are quite late in the cycle, and that changing
> spi_mem_check_buswidth() might cause all sorts of breakages, I think the
> best idea currently would be to revert this patch, and resend it with
> the other changes later.
>
> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
> I think we should send out a fixes PR with a revert. If you agree, I
> will send out a patch and a PR.

Either way I am fine. the -rc cycles are also available for us to
settle. But it's true we can bikeshed a little bit, so feel free to
revert this patch before sending the MR.

Thanks,
Miquèl