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

Issue 822362 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature



Sign in to add a comment

ec: Add private argument to irq handler

Project Member Reported by gwendal@chromium.org, Mar 15 2018

Issue description

Today, irq handler only get interrupt signal as argument.

It may be difficult from the signal to get back to a meaningful data structure, like sensor data, [we want to collect exact timestamp when hard interrupt occurs, so we are currently limited to one sensor of a given type per board].

kernel irq handler has an additional argument (private).

I am proposing to add an array for argument:

void* gpio_irq_privates[]

In gpio_interrupt, we would use this array when calling interrupt handler.

However, it cost in ram: I roughly counted the number of interrupts per board with:

find build -name \*RW.smap -exec grep -l gpio_irq_handlers {} \+ | \
xargs -n 1 awk 'BEGIN { s = 0 } 
/gpio_irq_handlers/ { if ( s == 0 ) { b="0x" $1; s=$2 } next } 
/ [Rr] / { if ( s == $2 ) { 
  d=strtonum("0x" $1) - strtonum(b) ; print FILENAME ":" d;  exit 
} }' | \
sort -r -n -t ':' -k 2 | less

There are some number I don't get, but one of the worst case, lux, has 21 interrupts, so we are talking to up to ~100 bytes used in ram.

If we want to be thriftier with ram, we could have a second smaller array, a bitfield and use popcount() to find the right index in gpio_irq_privates[], something like

(1 << i) & gpio_irq_private_map ? 
 gpio_irq_privates(
    popcount(gpio_irq_private_map & ((1 << i+1) - 1))] - 1): 
 NULL)

Given gpio_irq_private_map is a constant, an inline version of popcount can reduce the number of test iteration.

 
all_int.txt
3.5 KB View Download

Comment 1 by amstan@chromium.org, Mar 15 2018

For some more context.

Let's say we're in a system with 2 bmi160, we don't actually have this yet, but let's assume we would add one to scarlet.
* We'll probably have 2 bmi160_drv_data_t defined in the board.c (https://chromium.googlesource.com/chromiumos/platform/ec/+/master/board/scarlet/board.c#363)
* 2 separate gpio interrupt lines hooked up to bmi160_interrupt
https://chromium.googlesource.com/chromiumos/platform/ec/+/master/board/scarlet/gpio.inc#33

The issue is that currently bmi160_interrupt saves the timestampt to a global, having no idea which bmi160_drv_data_t to change (we could have made last_interrupt_timestamp a member of that struct otherwise).
https://chromium.googlesource.com/chromiumos/platform/ec/+/master/driver/accelgyro_bmi160.c#962

Gwendal's suggesting we have bmi160_drv_data_t as a private argument to the bmi160_interrupt.

I disagree a little bit. I agree that we need to somehow correlate the 2, but It would be weird mentioning arguments (like pointers to bmi160_drv_data_t) in gpio.inc.

The only thing we need is some kind of map from gpio_signal to the bmi160_drv_data_t(or whatever that particular sensor uses to store various instances of the sensor). In theory if we have a list of sensor instances on the system (motion_sensors from board.c might do the trick), we could add a gpio_signal either in the motion_sensor_t or bmi160_drv_data_t struct. That way whenever we get the interrupt, we scan all instances of the sensors for which the gpio_signal matches the interrupt argument, and that's the instance we should be using. It's an O(n) search, but n is probably going to be a max of 2.

The other thing to note is that for this to all work: each of the sensors of the same type should have a separate interrupt pin.
Labels: -Pri-2 Pri-3
We don't have to set the argument in gpio.inc, we could do it in board.c, after motion_sensors has been defined.

I agree we can make it motion sensor specific, if a private pointer has no use for any other interrupt handler.
Owner: alevkoy@chromium.org

Sign in to add a comment