On Tue, Jan 14 2025, Miquel Raynal wrote:
> Hello Pratyush,
>
> On 14/01/2025 at 16:15:24 GMT, Pratyush Yadav <pratyush@xxxxxxxxxx> 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.
I agree. All peripheral drivers convert cycles to bytes, and controller
drivers convert them back to cycles. This whole thing should be avoided,
especially since it contains some traps with division truncation.
>
> ...
>
>> 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.
I suppose you would want to sanity check that the cycles are at least
between 1, 2, 4, or 8 (or at the very least not 0).
>
>> 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.
To be clear, since the patch was added in v6.13-rc1 I want to revert it
via a fixes pull request to Linus before he releases v6.13 this week. I
want to fix it in v6.13, not in v6.14.
--
Regards,
Pratyush Yadav
