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