QSPI multiple thread access

Report here problems in any of ChibiOS components. This forum is NOT for support.
User avatar
FXCoder
Posts: 305
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 136 times
Been thanked: 97 times

Re: QSPI multiple thread access

Postby FXCoder » Sat Jul 11, 2020 6:37 am

Giovanni wrote:I wonder if it is right to have to call (w)spiStart() from outside when SNOR_SHARED_BUS==FALSE.

The most desirable behavior, I think, would be:

SNOR_SHARED_BUS==FALSE -> start the bus on snorStart(), stop it on snorStop.

SNOR_SHARED_BUS==TRUE -> start/stop the bus in each operation, this is for allowing shared SPIs basically.

Perhaps this should be considered a bug because the (W)SPI configuration is passed to the SNOR driver in both cases.
steved wrote:In fact, why would one need to do this, even in a shared configuration? Unless the bus settings change, of course.
The mutex, when enabled, can have the dual purposes of managing access to the bus, and making those accesses thread-safe - as long as checks prior to the mutex don't interfere. (If grouped sequences are needed, they need to be managed at a higher level).

Maybe only enable certain checks if SNOR_SHARED_BUS==FALSE

I suspect this type of issue is going to arise more as Chibi acquires more complex drivers which sit on top of HAL.


Agreed. Anyway here are some further thoughts...
If we were to view the NOR flash as a "system service" versus a "driver" then the question is should that service manage the assigned underlying driver?
It makes sense to me that the "NOR service" manages the underlying driver (SPI or WSPI).
For SPI acquire-start-use-stop-release covers the handling of multiple devices on an SPI bus with differing rates, etc.
i.e. each "service transaction" should be handled by the service acquiring the underlying peripheral, executing the task and then releasing.
In the case of WSPI being used by the "NOR service" then the same sequence makes sense to me.
The overheads of starting/stopping are not substantial and where there are multiple concurrent applications running the acquire-use-release method is highly preferred by me.

QSPI does introduce some twists such as memory mapping.
However, once the "NOR service" enables mapping the safe approach is for the resource (the NOR flash) to remain busy until unmapped and disabled.

Additionally there is a use case where QSPI could be supporting two (possibly different) devices in 1 bit mode (selected by CR:FSEL).
Although the wspi driver does not support managing CR:FSEL at the moment it may make sense for that to be implemented (add a CR field to the config?).
--
Bob

steved
Posts: 743
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 10 times
Been thanked: 111 times

Re: QSPI multiple thread access

Postby steved » Sat Jul 11, 2020 9:15 am

FXCoder wrote:The overheads of starting/stopping are not substantial and where there are multiple concurrent applications running the acquire-use-release method is highly preferred by me.

I see start/stop and acquisition management as two separate tasks.
Given that my current QSPI application does large numbers of reads of small (usually 16-byte) blocks of data, start/stop on every access could introduce a noticeable overhead.
And while the start/stop overhead for QSPI is relatively small, it may not be for some other peripheral whose driver is intended to follow the same model.


FXCoder wrote:QSPI does introduce some twists such as memory mapping.
However, once the "NOR service" enables mapping the safe approach is for the resource (the NOR flash) to remain busy until unmapped and disabled.

Agreed (and in fact implemented in my LLD).


FXCoder wrote:Additionally there is a use case where QSPI could be supporting two (possibly different) devices in 1 bit mode (selected by CR:FSEL).
Although the wspi driver does not support managing CR:FSEL at the moment it may make sense for that to be implemented (add a CR field to the config?).

This seems to be part of the problem with getting to a reasonable solution - QSPI/SPI has so many potential use cases that it's tricky to support the more complex cases without over-complicating the simple cases. Maybe it's possible to come up with, say, two or three models which reflect the main use cases and set all the flags accordingly.

User avatar
Giovanni
Site Admin
Posts: 13217
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 787 times
Been thanked: 681 times
Contact:

Re: QSPI multiple thread access

Postby Giovanni » Tue Aug 11, 2020 5:10 pm

bump

steved
Posts: 743
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 10 times
Been thanked: 111 times

Re: QSPI multiple thread access

Postby steved » Fri Aug 14, 2020 5:35 pm

Not quite sure whether the 'bump' was for a specific point, but I've tried to pull together some of the discussions and thoughts into one post. No doubt missed (or confused) a few things.

Some of the complications arise due to supporting both SPI and WSPI drivers. This gives us three (or maybe four) use cases:
  • SPI (shared bus assumed?)
  • WSPI exclusive bus
  • WSPI shared bus
Note: If I've understood things right, a shared bus could (at least for SPI) be supporting multiple NOR flash devices as well as non-flash devices.


We potentially need two types of arbitration:
  • bus driver level (managing access to shared resource)
  • SNOR driver level (for thread safety)

If I've thought things through OK, for SPI/QSPI at least, a single mutex at the LLD level will achieve both purposes (as long as the higher level driver code is reentrant).

Driver Configuration & Management
=================================
While it seems logical that the SNOR driver be treated as a service, and manage the underlying hardware driver, the hardware driver may be supporting non-SNOR devices as well, which makes it more difficult.
I'm also not clear why the QSPI device initialisation is done via the bus_acquire() routine, rather than by a call from elsewhere. As a minimum, it would be good if bus_acquire() would accept a NULL pointer for the config, meaning "leave unchanged" (again, this removes a little overhead, by eliminating the calculation of the current configuration structure)

Driver Start/Stop & Acquisition
===============================
There are use cases for at least three scenarios:
  • Start once; acquire-use-release
  • acquire-start-use-stop-release
  • acquire, start, use stop, release as separate functions
The last case is for 'grouped' use, which is necessarily under the control of the application.

Thread-Safe Drivers
===================
I'd like to see all drivers have the option to be thread-safe. I realise that this can be done externally, but it is another layer of indirection, even with inline functions. Supporting within the drivers ensures consistency
(N.B. Some drivers appear to be thread-safe by design - serial? - so it would be consistent to support thread-safety on all).
If grouping is required, the application disables the inbuilt mutex, and does its own thing.
If enabled, the driver is passed a pointer to a mutex in the configuration block, and initialises it in the start() routine.

Driver Asserts
==============
Drivers should continue to assert within an API function if the call is incapable of ever being satisfied (state = [uninitialised, stopped...]).
If arbitration is enabled, an API call when the driver is busy will either return an error (e.g. erase in progress) or wait on a mutex.
If arbitration isn't enabled, though - that's probably a programming error so an assert (Something that should have been handled at a higher level, but wasn't?)

General comments
================
The names bus_acquire() and bus_release() seem to be rather generic, and risk clashes with application routines. (But because of their use with both SPI and WSPI, what's a good name? SNOR_bus_acquire()?)


hal_serial_nor.c - suggested code improvements
==============================================
(might apply to other drivers as well)

In several places there's code of the form:

Code: Select all

  osalDbgAssert((devp->state == FLASH_READY) || (devp->state == FLASH_ERASE),
                "invalid state");

  if (devp->state == FLASH_ERASE) {
    return FLASH_BUSY_ERASING;
  }

If the two lines are swapped, the assert can be simplified, probably making the code smaller and faster:

Code: Select all

  if (devp->state == FLASH_ERASE) {
    return FLASH_BUSY_ERASING;
  }
  osalDbgAssert((devp->state == FLASH_READY),
                "invalid state");


Note: If we move to allowing through multiple states (as suggested by both Bob and myself) then for efficiency you could do the code this way:

Code: Select all

#define FLASH_VALID_STATES      ((1 << FLASH_READY) | (1 << FLASH_READ) | (1 << FLASH_PGM) | (1 << FLASH_ERASE))

  osalDbgAssert((((1 << devp->state) & FLASH_VALID_STATES) > 0),
                "invalid state");

User avatar
Giovanni
Site Admin
Posts: 13217
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 787 times
Been thanked: 681 times
Contact:

Re: QSPI multiple thread access

Postby Giovanni » Fri Aug 14, 2020 6:09 pm

Just a reminder for myself that this topic was still pending.

Giovanni


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 6 guests