hal_Serial_nor.c

[steved]

All with #define SNOR_SHARED_BUS FALSE
1. The empty macro for bus_acquire needs another parameter:
#define bus_acquire(busp, config)
2. The 'real' bus_acquire() routine calls wspiStart() when required; the empty macro does not. (config is not (void)). So wspi does not start if there's no bus arbitration.

Also, not sure that it matters, but the wspi bus_acquire() and bus_release() are declared as static; those for SPI are not.


Also, in hal_wspi_lld.c (for STM32FXX), wspi_lld_map_flash() the dummy cycle count isn't included in the command:

Code: Select all

  /* Starting memory mapped mode using the passed parameters.*/
  wspip->qspi->DLR = 0;
  wspip->qspi->ABR = 0;
  wspip->qspi->AR  = 0;
  wspip->qspi->CCR = cmdp->cmd | cmdp->cfg |
                     QUADSPI_CCR_DUMMY_CYCLES(cmdp->dummy) |
                     QUADSPI_CCR_FMODE_1 | QUADSPI_CCR_FMODE_0;


[steved]

Also on the subject of QSPI, hal_wspi_lld.c has the following code:

Code: Select all

static void wspi_lld_serve_interrupt(WSPIDriver *wspip) {

  /* Portable WSPI ISR code defined in the high level driver, note, it is
     a macro.*/
  _wspi_isr_code(wspip);

  /* Stop everything, we need to give DMA enough time to complete the ongoing
     operation. Race condition hidden here.*/
  while (dmaStreamGetTransactionSize(wspip->dma) > 0U)
    ;
  dmaStreamDisable(wspip->dma);
}


What would happen if, for example, a Transfer Error Interrupt occurred? Could the DMA get stuck with a non-zero count?

[Giovanni]

Hi,

I see the problem, that would happen only if trying to access addresses above the flash size so basically because a SW bug, it is not a random occurrence.

Lets consider some some possible handling:

1) In case of TEF we call some hook code defaulted to an halt. This would allow for clear detection an open to indirect handling).
2) Propagate the error to the upper layers (this would require some changes to API functions and internal interfaces). Does it make sense that the upper layers have to handle an error originated in upper layers (wrong addresses)? on the other hand it would be a generic "error" mechanism currently missing in the driver model.

BTW, the OSPI driver has the same problem.

Giovanni

[steved]

Personally I would go for a generic "error" mechanism - it's what I do for much of my own code. Looking at the general case, it has two advantages over just stopping the processor:
a) There are scenarios where the caller wishes to know the result of an operation so that it can react accordingly - checking the presence or absence of a chip, testing memory capacity, etc (I realise there are sometimes other ways of doing this). Then the caller can decide on the action.
b) Even if one thread cannot continue processing, it may be desirable that other threads keep going. Particularly relevant to threads whose purpose is peripheral to the prime function of a device.

There are some errors where its very obviously a software bug should they occur, and an assert is a reasonable way to deal with those; however there is a type of error ("range errors", perhaps) where the opportunity to handle themn in the upper layers could be useful.

Possibly a global flag could determine whether errors cause a panic, or alternatively return?

Incidentally, I have a list of several hundred possible error codes (rather specific to my applications, mostly) so that the returned error can be as explicit as possible - greatly helps faultfinding. If you're returning a 16-bit or 32-bit error code anyway, it's not going to matter whether there are three possible return values or 3000.

[Giovanni]

All with #define SNOR_SHARED_BUS FALSE

Also, in hal_wspi_lld.c (for STM32FXX), wspi_lld_map_flash() the dummy cycle count isn't included in the command:

Code: Select all

  /* Starting memory mapped mode using the passed parameters.*/
  wspip->qspi->DLR = 0;
  wspip->qspi->ABR = 0;
  wspip->qspi->AR  = 0;
  wspip->qspi->CCR = cmdp->cmd | cmdp->cfg |
                     QUADSPI_CCR_DUMMY_CYCLES(cmdp->dummy) |
                     QUADSPI_CCR_FMODE_1 | QUADSPI_CCR_FMODE_0;


Hi,

Is the code snippet the correct one? the code in the repository is exactly like that.

Fixed the macro error as bug #1044.

Giovanni

[Giovanni]

Added error handling to the WSPI driver, this required a new callback for the asynchronous API and a return value for the synchronous API.

LLDs have not yet been updated.

Giovanni

[steved]

Is the code snippet the correct one? the code in the repository is exactly like that.
Giovanni

Think I ended up mentioning this in two separate posts; so probably fixed from the other one.

[steved]

Added error handling to the WSPI driver, this required a new callback for the asynchronous API and a return value for the synchronous API.

Excellent - currently I very occasionally get a hang in the WSPI driver which locks up my system; this should help track it down. I am using a different flash chip with my own driver, so it is quite possible that the problem is nothing to do with ChibiOs!

[steved]

Back on looking for this problem (backported Chibi 20 drivers to 19.1.3), and see that the qspi lld needs updating to implement the error handling. Is there any indication of what needs doing? (I've updated the relevant hal_serial_nor routines to pass the error status upwards).

(Note: I'm using the QSPI1 driver in 4-bit mode).

[Giovanni]

I need to look at it again, this is months old.

How is your system hung?

Giovanni