New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 807504 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 611549



Sign in to add a comment

ec: stm32: do most of host command processing out of IRQ.

Project Member Reported by gwendal@chromium.org, Jan 31 2018

Issue description

Alex (amstan@) while working on tightening timestamping of sensor information found that the precisioon is hampered by the spi host command processing in spi_event(): receiving the whole packet is done on interrupt.

The current workaround is to call gpio_interrupt() while under the interrupt in wait_for_bytes(), allowing the sensor interrupt to be processed. 

We can do better:

host_packet_receive() could be done in TASK_ID_HOSTCMD: the args should be the raw buffer. We know only one command can be sent by the AP.
We will do crc calculation and other checks outside the irq.

wait_for_bytes() is dependent of the AP. If while sending the command over SPI the AP is interrupted, the ec will busy wait.

One solution could be to enable SPI RXNEIE (receive buffer not empty interrupt) to every 8/16 bit waiting for the buffer to be complete to trigger TASK_ID_HOSTCMD.

 
Cc: drinkcat@chromium.org

Comment 2 by sha...@chromium.org, Jan 31 2018

I think it can be done, but:

- The host has expectation that the EC will begin to output EC_SPI_PROCESSING soon (next SPI clock cycles, I think) after the full host command is received (host command length is known from length parameter). So, the EC needs to quickly change its output data in any solution.

- One corner case is that the host may assert CS and take a long time to even get to sending the host command length parameter. Before we receive length, I think we need to trigger an interrupt or task wake on every Rx byte. After we receive length, we can trigger an interrupt after length (+/- overhead) bytes have been received, or CS has been de-asserted.
From what I read in [RM0091 Reference manual : STM32F0x1/STM32F0x2/STM32F0x8 advanced ARM-based 32-bit MCUs]

the SPI subsystem will interrupt every 1 or 2 bytes only:

[27.5.9 SPI status flags]"
Rx buffer not empty (RXNE)

The RXNE flag is set depending on the FRXTH bit value in the SPIx_CR2 register:
• If FRXTH is set, RXNE goes high and stays high until the RXFIFO level is greater or equal to 1/4 (8-bit).
• If FRXTH is cleared, RXNE goes high and stays high until the RXFIFO level is greater than or equal to 1/2 (16-bit).
An interrupt can be generated if the RXNEIE bit in the SPIx_CR2 register is set.
The RXNE is cleared by hardware automatically when the above conditions are no longer true.

- In consequence, we will always interrupt on 1 or 2 bytes, I did not find a way to set the length to interrupt on. We will interpret the stream as it come and raise an event in TASK_ID_HOSTCMD once we have the full message. The hope is more than 2 bytes have been dma'ed when the interrupt is processed, to prevent too many interruptions.


Comment 4 by amstan@chromium.org, Feb 16 2018

Cc: aaboagye@chromium.org sjg@google.com
This currently blocks https://b.corp.google.com/issues/67743747. Because of this you cannot rely on the EC to give you less than 100us jitter when recording timestamps of events (instead it might be 8ms+).

For my uses I would like to commit the gpio_interrupt() call in wait_for_bytes(). That should cover most of the problems I see. I don't think it's a good idea to do such risky major changes to this spi driver otherwise only because of my "minor" project. I'll have a CL soon for people that really want to object.

Comment 5 by sjg@chromium.org, Feb 20 2018

Cc: -sjg@google.com sjg@chromium.org

Comment 6 by sjg@chromium.org, Feb 20 2018

I mentioned this to Randall some years ago, and wrote a prototype for snow (at he time). It processed most commands from the interrupt handler. Those that it could not, it did later.

There are some challenges, but I hope something can come of this. IMO the current code is not very efficient.

Sign in to add a comment