Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification

[Haixu Cui <quic_haixcui@quicinc.com>]

Hi @jrreinhart@google.com,
    Thank you very much for your helpful comments.

    I missed the delay and cs_change_delay parameters. I will add both 
of them, although cs_change_delay can not be set from userspace, but can 
be set in kernel space.


    For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are 
defined in structure spi_device:

    @cs_setup: delay to be introduced by the controller after CS is 
asserted.

    @cs_hold: delay to be introduced by the controller before CS is 
deasserted.

    @cs_inactive: delay to be introduced by the controller after CS is
deasserted. If @cs_change_delay is used from @spi_transfer, then the
two delays will be added up.

    They show the SPI controller ability to control the CS 
assertion/deassertion timing and should not be changed for each transfer 
(because thay can be updated by setting structure spi_transfer or 
structure spi_ioc_transfer). I think it better to define these parameter 
in host OS rather than in guest OS since it's the host OS to operate the 
hardware SPI controller directly. Besides, it can also avoid passing the 
same values from guest to host time after time.

    What's your opinion on this topic? Again, thank you very much.

Best Regards
Haixu Cui

On 6/28/2023 1:06 AM, jrreinhart@google.com wrote:
> Hi Haixu,
>
> > +The \field{word_delay} defines how long to wait between words within
> > one SPI transfer,
> > +in ns unit.
>
> I'm a little surprised to see a word_delay but no frame_delay or
> transfer_delay.
>
> For example, many SPI peripherals require a delay after CS is asserted,
> but before the first SCLK edge, allowing them to prepare to send data.
> (E.g. an ADC might begin taking a sample.)
>
>
> The linux struct spi_transfer has three delay fields:
>
> * @cs_change_delay: delay between cs deassert and assert when
> *ÂÂÂÂÂ @cs_change is set and @spi_transfer is not the last in
> *ÂÂÂÂÂ @spi_message
> * @delay: delay to be introduced after this transfer before
> *ÂÂÂ (optionally) changing the chipselect status, then starting the
> *ÂÂÂ next transfer or completing this @spi_message.
> * @word_delay: inter word delay to be introduced after each word size
> *ÂÂÂ (set by bits_per_word) transmission.
>
> The userspace spidev.h API has only two:
>
> * @delay_usecs: If nonzero, how long to delay after the last bit
> *ÂÂÂÂÂÂ transfer before optionally deselecting the device before the
> *ÂÂÂÂÂÂ next transfer.
> * @word_delay_usecs: If nonzero, how long to wait between words within
> *ÂÂÂÂÂÂ one transfer. This property needs explicit support in the SPI
> *ÂÂÂÂÂÂ controller, otherwise it is silently ignored.
>
> The NXP i.MX RT500 SPI (master) controller, for example, has four
> configurable delays:
>
> - PRE_DELAY: After CS assertion, before first SCLK edge.
> - POST_DELAY: After a transfer, before CS deassertion.
> - FRAME_DELAY: Between frames (which are arbitrarily defined by
>  Â software).
> - TRANSFER_DELAY: Minimum CS deassertion time between transfers.
>
> The NVIDIA Tegra114 SPI controller has:
>
> - nvidia,cs-setup-clk-count: CS setup timing parameter.
> - nvidia,cs-hold-clk-count: CS hold timing parameter.
>
>
> I understand that accurately representing all possible delays might be
> difficult or futile, but I'm curious to understand why, of all the
> possible delays, inter-word delay was chosen for inclusion.
>
> In a microcontroller, delays around CS (de)assertion can be customized
> by using a GPIO -- rather than the hardware SPI block -- to drive the CS
> signal. This way, delays can be inserted where needed. To do so with a
> virtualized SPI controller might prove difficult however, as the virtual
> channel carrying a CS GPIO signal might not be synchronized to the
> channel carrying the SPI data.
>
> Curious to hear your thoughts.
>
> Thanks,
> Jonathon Reinhart