Re: [PATCH 03/32] spi: dw: Fix driving MOSI low while recieving

[Sean Anderson <seanga2@xxxxxxxxx>]

On 11/9/20 2:19 PM, Serge Semin wrote:
> On Mon, Nov 09, 2020 at 02:14:22PM +0000, Mark Brown wrote:
>> On Mon, Nov 09, 2020 at 08:47:10AM -0500, Sean Anderson wrote:
>>> On 11/9/20 8:29 AM, Mark Brown wrote:
>>>> On Sat, Nov 07, 2020 at 05:13:51PM +0900, Damien Le Moal wrote:
>>>>
> 
>>>>> The resting state of MOSI is high when nothing is driving it. If we
>>>>> drive it low while recieving, it looks like we are transmitting 0x00
>>>>> instead of transmitting nothing. This can confuse slaves (like SD cards)
>>>>> which allow new commands to be sent over MOSI while they are returning
>>>>> data over MISO. The return of MOSI from 0 to 1 at the end of recieving
>>>>> a byte can look like a start bit and a transmission bit to an SD card.
> 
> Yeah, that's what we've also experienced on our systems. We've worked
> around the problem in exactly the same way as you have. But we haven't
> dared to send it out as the solution seemed a bit hackish.

Well, the way it is now is equally wrong, since it is driving the line
low.

>>>>
>>>> If client devices are interpreting the transmitted data then I would
>>>> expect the drivers for that hardware to be ensuring that whatever we
>>>> transmit matches what the device is expecting.  We shouldn't be putting
>>>> a hack in a particular controller driver to paper over things, that will
>>>> mean that the device will break when used with other controllers and if
>>>> different devices have different requirements then obviously we can't
>>>> satisfy them.  There is not meaningfully a general specification for SPI
>>>> which says what happens when signals are idle, it's all specific to the
>>>> client device.
>>>>
>>>> In this case it also looks like the controller hardware requires
>>>> transmit data and therefore should be setting SPI_MUST_TX and just
>>>> removing the in driver default anyway, though that will have no effect
>>>> one way or anther on the issue you're seeing.
>>
> 
>>> There is a recieve-only mode, but it is not used by this driver. Perhaps
>>> it should be.
>>
>> I'd expect it'd perform better, especially on systems that are
>> apparently struggling for CPU bandwidth like yours seems to.
> 
> CPU-wise. RO-mode won't help in that case. Moreover it will be even
> more errors-prone for the systems with small CPU bandwidth. As I said
> the Receive-only mode will make the SPI controller automatically
> receiving data from the SPI bus and putting it into the Rx FIFO. If
> CPU is either busy with something else or too slow in fetching the
> data from the Rx FIFO, the FIFO will be eventually overflown with
> data, which we need to avoid at all cost.
> 
> As I see it the Receive-only mode is only acceptable in the next two
> situations:
> 
> 1) Rx-only DMA. But only if the DMA-engine and system bus are fast
> enough to fetch the incoming data on time. (Note for example in our
> system some DWC DMA-engine channels don't work well with the DW APB
> SSI working with full-speed, so we had to set constraints on the DWC
> DMA channels being used in conjunction with the DW APB SSI
> controller.)
> 
> 2) Rx-only with atomic CPU utilization. In order to make sure that the
> CPU keeps up with fetching the data from the Rx FIFO, we have to
> disable the local CPU IRQs while performing the Rx-only transfers, so
> to prevent the Rx FIFO overflow while the CPU is doing something else.
> Needless to say that such approach should be utilized only as a last
> resort, if we have no choice but to run the Receive-only transfers.
> Because locking the CPU for God knows how much time may cause the
> system interactivity degradation. For instance, a possible use-case of
> that design is when the controller is communicating with the
> SPI-devices with native DW APB SSI chip-select attached. BTW You can
> also find that design implemented in the kernel 5.10 spi-dw-core.c
> driver in context of the SPI-memory operations (with my last patches
> merged in). In particular I had to use it to handle the CPU-based
> EEPROM-read mode.
> 
> So in all other cases for normal CPU-based SPI-transfers when
> GPIO-based chip-select is available the safest solution would be to
> use a normal Push-Pull mode. In this case we have no risk in getting
> the Rx FIFO overflow unless there is a bug in the code, which is
> fixable anyway.
> 
> Getting back to the patch. In fact I don't really see how the
> Receive-only mode will help us with solving the problem noted in the
> patch log.

Shouldn't it put MOSI into High-Z like when the device is idle? The
issue is mainly that the idle state and RX state are different.

> As Mark said the problem with the Tx data on Rx-only
> transfers should be fixed on the client side. If an subordinate
> SPI-device needs a specific value to be received in that case, then
> that value should be somehow provided to the SPI-controller anyway.
> So the native Rx-only mode of the DW APB SSI controller won't help.
> Currently it's possible to be done only by executing a Full-duplex
> SPI-transfer with the Tx-buffer being pre-initialized with that
> value.
> 
> Another possible solution for the problem would be to fix the SPI core
> so aside with tx_buf being set to the NULL-pointer, a client driver
> would provide a default level or some specific value being put to the
> SPI bus on Rx-only transfers. If an SPI-controller is capable of
> satisfying the request, then it will accept the transfer. If it's not,
> then the SPI core may try to convert the Rx-only transfer into the
> Full-duplex transfer with the Tx-buffer being initialized with the
> requested level.

This is probably the most general solution; is there an existing way to
specify this sort of thing?

--Sean