hal_Serial_nor.c Topic is solved

Report here problems in any of ChibiOS components. This forum is NOT for support.
steved
Posts: 626
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 5 times
Been thanked: 87 times

hal_Serial_nor.c  Topic is solved

Postby steved » Mon Jul 15, 2019 1:36 pm

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
Posts: 626
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 5 times
Been thanked: 87 times

Re: hal_Serial_nor.c

Postby steved » Fri Jul 19, 2019 10:02 pm

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?

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

Re: hal_Serial_nor.c

Postby Giovanni » Sun Aug 11, 2019 7:40 am

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
Posts: 626
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 5 times
Been thanked: 87 times

Re: hal_Serial_nor.c

Postby steved » Sun Aug 11, 2019 3:14 pm

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.

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

Re: hal_Serial_nor.c

Postby Giovanni » Sat Oct 05, 2019 7:40 am

steved wrote: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

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

Re: hal_Serial_nor.c

Postby Giovanni » Sat Oct 05, 2019 8:06 am

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
Posts: 626
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 5 times
Been thanked: 87 times

Re: hal_Serial_nor.c

Postby steved » Sat Oct 05, 2019 8:32 am

Giovanni wrote: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
Posts: 626
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 5 times
Been thanked: 87 times

Re: hal_Serial_nor.c

Postby steved » Sat Oct 05, 2019 8:34 am

Giovanni wrote: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!


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 2 guests