[PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[kernel at martin.sperl.org]

> On 23.03.2017, at 13:21, Mark Brown <broonie at kernel.org> wrote:
> 
> On Thu, Mar 23, 2017 at 01:02:57PM +0100, Lukas Wunner wrote:
>> On Thu, Mar 23, 2017 at 11:07:18AM +0000, Mark Brown wrote:
> 
>>> This is going to depend on the software running on the board, I'd expect
>>> it to be something configured at runtime rather than fixed in DT.
> 
>> On the Revolution Pi, the SPI master is hardwired to two KSZ8851 Ethernet
>> controllers which are connected to various fieldbus gateways in a
>> plug-and-play fashion.  Low latencies are needed to achieve a decent
>> cycle rate when reading/writing data to the fieldbus devices.
> 
>> Hence for this use case, the RT priority is mandated by the hardware
>> platform, which is why I put it in the devicetree.  It is needed
>> regardless of which software is running.
> 
> Caring about having low latency access to fieldbus devices is a property
> of the application, and achieving that via the use of RT priority for
> the SPI pump thread is a property of current Linux implementations.
> Neither of these things is a description of the hardware, the SPI pump
> thread bit is already changing (for a very large proportion of use cases
> we only use the thread to shut down devices once they become idle these
> days).

Some observations:
* we use an “inline” spi-pump if there are no messages in the message
  queue - the spi-message-pump-thread is not used in those cases.
  So those spi messages are NOT handled with the proposed RT priority,
  as they run within the context of the “caller” - which may even be
  the priority of userspace if spidev is used.
* the only place where the main message-pump is used is when there are
  spi_messages queued. And then you have a latency waking up the calling
  thread (which typically does not run with RT priority, so it is not
  necessarily woken up immediately by the scheduler either) - that is
  unless the caller use the spi_async with custom callbacks, that do not
  require a wakeup.
* There are a few more things to take into account on the bcm283X:
  * for short SPI messages (<10ms I believe) idle looping is used to get
    the fastest response time without any rescheduling overhead and thus
    to reduce latencies.
  * for long transfers (byte-wise >96 bytes) the driver uses DMA to avoid
    interrupt-scheduling latencies (setup time of the DMA is a bit
    expensive, so 2 interrupts in interrupt-driven mode are acceptable,
    but anything else is handled via DMA to reduce the interrupts to 1 -
    OK, it is actually 2 DMA interrupts, but that is a limitation of the
    DMA framework (requiring INT for RX and TX DMA end - while we only
    really require RX-DMA interrupts, but that is for “ease” of
    releasing memory correctly…)

So a lot of care has already been taken to improve the latencies
of the driver and the framework.

From my experience the main issue is more the side of interrupt-scheduling
and wakeup which this rt patch does not solve - especially if a driver has
to respond as fast as possible to a GPIO change (interrupt request from the
device) this is typically the limiting factor.

I remember I had a similar patch that used a module parameter to control it
instead, but I had dropped it because of all the above as mostly not
necessary (measurements with a logic analyzer my CAN setup did not show any
real improvement).

As a consequence of all this to make it really work you would need RT
priority for:
* calling thread (when using the “inline" message pump)
* spi-message pump (for queued SPI_messages)
* dma-termination-thread (for DMA transfers), as this gets woken
  from DMA interrupt for termination of the DMA transfer,
  which then wakes the thread running the spi-message pump.

> 
>> We could accommodate to setting the RT priority on the command line but
>> we'd like to avoid always having to set it at runtime.  That is already
>> possible by invoking chrt(1) from user space after the SPI master driver
>> has loaded and wouldn't require a kernel patch.
> 
> Right, there's already mechanisms to do this at runtime.
> 
>> Also, if you question setting the RT priority in the devicetree, why
>> was that functionality allowed for pl022 in the first place?
> 
> This was being done via platform data not via device tree, unlike
> platform data device tree should provide a long term stable OS neutral
> ABI.

Well - if it is really a necessity and as the mantra for DT is:
"it should describe the HW only", why not use a module parameter
instead or use chrt() from userspace?

Martin