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

Issue 709642 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 709252



Sign in to add a comment

ectool motionsense broken on glimmer

Project Member Reported by gwendal@chromium.org, Apr 7 2017

Issue description

Since 4b154c6f95 (cl//226880), ectool expects EC to support EC_CMD_MOTION_SENSE_CMD version 1 or above, but glimmer does not.

In consequence, all commands ectool motionsense ... fails on glimmer (and most likely all baytrail)


 
Summary: ectool motionsense broken on glimmer (was: ectool broken on glimmer)
Owner: gwendal@chromium.org
Status: Assigned (was: Untriaged)
Cc: amstan@chromium.org
Actually, looking with Alex, even kernel 3.10 expects EC_CMD_MOTION_SENSE_CMD version 1 or above to work properly.
We can not change the frequency and other parameters. Luckily, chrome is only interested in reading accel data, which is done via ACPI io memory, no EC command required.

Comment 4 by amstan@chromium.org, Apr 12 2017

Cc: dtor@chromium.org
I was looking at porting the 3.10 driver to 4.4. Eg:
amstan@amstan:~/cros/src/third_party/kernel/v4.4 % cp ../v3.10/drivers/iio/accel/cros_ec_accel.c drivers/iio/accel/cros_ec_accel.c

Seems rather hard, with errors such as:
/mnt/host/source/src/third_party/kernel/v4.4/drivers/iio/accel/cros_ec_accel.c:187:4: error: 'struct cros_ec_device' has no member named 'cmd_read_u8'
/mnt/host/source/src/third_party/kernel/v4.4/drivers/iio/accel/cros_ec_accel.c:613:11: error: too many arguments to function 'iio_kfifo_allocate'
/mnt/host/source/src/third_party/kernel/v4.4/drivers/iio/accel/cros_ec_accel.c:678:12: error: 'struct cros_ec_command' has no member named 'outdata'

The issue is that the cros_ec driver is different upstream due the the rewriting work of Javier. We can't just load our old accel driver and expect it to work with the new cros_ec driver.

While it might be possible to massage it, we should ask ourselves if it's worth it(to have this CHROMIUM: cros-ec-legacy driver that's only enabled on glimmer).

Maybe we should look into what dtor suggested and just make the new sensors driver support protocol version 1.

Comment 5 by amstan@chromium.org, Apr 12 2017

In terms of making the new driver support accel protocol version 1...

Right now the driver responsible for loading all the other ec drivers, stops pretty early on:
[    2.368390] cros-ec-ctl cros-ec-ctl.1.auto: cannot get EC features: 0/1

Because the EC doesn't support the inventory command. Normally it would provide this information:
localhost sys # ectool inventory
EC supported features:
1   : Flash support
5   : LED support
6   : Motion Sensors support
10  : Thermal management support
13  : Host event support
14  : GPIO support
15  : I2C master support
16  : Charger support
17  : Simple Battery support
18  : Smart Battery support
22  : USB Cros Power Delievery support
24  : FIFO for Motion Sensors events support
26  : Host-controlled USB-C SS mux support
27  : Real-time clock support

Then for each feature the driver cares about, it defined new mfd devices and allows the subdrivers(like the cros_ec_sensors, via cros_ec_sensors_register()) to bind to them.

If we wanted to support accel version 1, we would have to somehow assume that it's present (ignoring all the inventory features that we added) and just try it.
#4: current glimmer firmware does not even support version 1, only version 0. As I said in #3, all EC_CMD_MOTION_SENSE_CMD commands sent by the 3.10 driver to the EC currently fails (the driver use version 1): we only communicate via the ACPI interface thru IO registers: it is enough to confirm presence of accels and gather data. It is not for changing frequency or calibration. The 3.10 driver is hard coded to expect 2 accelerometers.
We can add support:
in cros_ec_dev.c: ec_device_probe(): 
after cros_ec_check_features() fails, check EC_MEMMAP_ACC_STATUS_PRESENCE_BIT in EC_MEMMAP_ACC_STATUS: if true, build a 2 mfd_cell array to call a new simple driver, using cros_ec_sensor_platform to present one accel per iio device, hard-coding location. [Having one device per iio would help remove some dead code in chrome to support older statck]. 
By introducing a new driver, we need to alter accel udev script to be sure it creates the link chrome needs.

Comment 7 by gwendal@google.com, Apr 20 2017

Working on a new driver cros-ec-accel-legacy, without EC changes. ETA end of April, so in the meantime, we need glimmer convertible to stay on 3.10.
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 26 2017

Labels: merge-merged-chromeos-4.4
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/3bf98755f9c670c5c10ca05cba22848d65117cb2

commit 3bf98755f9c670c5c10ca05cba22848d65117cb2
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Wed Apr 26 01:40:07 2017

CHROMIUM: iio: accel: Add cros_ec_accel_legacy driver.

Add driver to support older EC firmware that only support deprecated
ec command. Rely on ACPI memory map register to access sensor
information.
Present same interface as the regular cros_ec sensor stack:
- one iio device per accelerometer
- use HTML5 axis definition
- use iio abi units
- accept calibration calls, but do nothing
Chrome can use the same code than regular cros_ec sensor stack to
calculate orientation and lid angle.

BUG= chromium:709642 
TEST=Check sensor values with script:
DIR=/sys/bus/iio/devices/
for i in 0 1 ; do
  cat $DIR/iio:device$i/location
  for j in x y z; do
    echo -n $j: ; cat $DIR/iio:device$i/in_accel_${j}_raw
  done
done
Check chrome goes into tablet mode.

Change-Id: I1856e41cca3993e4f1604b5f6234d813c60b0973
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/483965
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>

[modify] https://crrev.com/3bf98755f9c670c5c10ca05cba22848d65117cb2/drivers/iio/accel/Kconfig
[modify] https://crrev.com/3bf98755f9c670c5c10ca05cba22848d65117cb2/drivers/platform/chrome/cros_ec_dev.c
[modify] https://crrev.com/3bf98755f9c670c5c10ca05cba22848d65117cb2/drivers/iio/accel/Makefile
[add] https://crrev.com/3bf98755f9c670c5c10ca05cba22848d65117cb2/drivers/iio/accel/cros_ec_accel_legacy.c

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/85f58fe31f725f4405444c4cbe7382e7cacca63f

commit 85f58fe31f725f4405444c4cbe7382e7cacca63f
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Wed Apr 26 18:25:59 2017

CHROMIUM: Add support for accelerometers on glimmer

Add cros-ec-accel-legacy as valid cros-ec accelerometer driver.

CQ-DEPEND=CL:483965
BUG= chromium:709642 
TEST=Check sensor values with script:
DIR=/sys/bus/iio/devices/
for i in 0 1 ; do
  cat $DIR/iio:device$i/location
  for j in x y z; do
    echo -n $j: ; cat $DIR/iio:device$i/in_accel_${j}_raw
  done
done
Check /dev/cros-ec-accel links are created properly
Check trigger and access rights.
Check chrome goes into tablet mode when needed.

Change-Id: I1457a18f998eb394aea8fde8f3711467cdab6b49
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/483942
Reviewed-by: Bernie Thompson <bhthompson@chromium.org>

[modify] https://crrev.com/85f58fe31f725f4405444c4cbe7382e7cacca63f/chromeos-base/chromeos-accelerometer-init/files/udev/99-cros-ec-accel.rules
[modify] https://crrev.com/85f58fe31f725f4405444c4cbe7382e7cacca63f/chromeos-base/chromeos-accelerometer-init/files/init/cros-ec-accel.conf
[rename] https://crrev.com/85f58fe31f725f4405444c4cbe7382e7cacca63f/chromeos-base/chromeos-accelerometer-init/chromeos-accelerometer-init-0.0.1-r15.ebuild

Blocking: 709252
The CLs above are more for chromium:709252
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 28 2017

Labels: merge-merged-release-R59-9460.B-chromeos-4.4
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/efad54b2e2c2dc700015a5e7cd89e0122f35eaf6

commit efad54b2e2c2dc700015a5e7cd89e0122f35eaf6
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Fri Apr 28 21:07:08 2017

CHROMIUM: config: x86_64: Add cros_ec_accel_legacy driver

Add support for accelerometer for Glimmer.

BUG= chromium:709642 
TEST=Check sensor values with script:
DIR=/sys/bus/iio/devices/
for i in 0 1 ; do
  cat $DIR/iio:device$i/location
  for j in x y z; do
    echo -n $j: ; cat $DIR/iio:device$i/in_accel_${j}_raw
  done
done

Change-Id: I483e340f83ebcbb61a9828b32ea9d7ab57f79f33
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/483964
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
(cherry picked from commit f85f0c104dad52a85b386b49b10f55116867fcd4)
Reviewed-on: https://chromium-review.googlesource.com/490829

[modify] https://crrev.com/efad54b2e2c2dc700015a5e7cd89e0122f35eaf6/chromeos/config/x86_64/common.config

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 28 2017

Labels: merge-merged-release-R59-9460.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/2292d7429f3ec10cc9a4252fa877b597276859b1

commit 2292d7429f3ec10cc9a4252fa877b597276859b1
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Fri Apr 28 21:07:09 2017

CHROMIUM: Add support for accelerometers on glimmer

Add cros-ec-accel-legacy as valid cros-ec accelerometer driver.

CQ-DEPEND=CL:483965
BUG= chromium:709642 
TEST=Check sensor values with script:
DIR=/sys/bus/iio/devices/
for i in 0 1 ; do
  cat $DIR/iio:device$i/location
  for j in x y z; do
    echo -n $j: ; cat $DIR/iio:device$i/in_accel_${j}_raw
  done
done
Check /dev/cros-ec-accel links are created properly
Check trigger and access rights.
Check chrome goes into tablet mode when needed.

Change-Id: I1457a18f998eb394aea8fde8f3711467cdab6b49
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/483942
Reviewed-by: Bernie Thompson <bhthompson@chromium.org>
(cherry picked from commit 85f58fe31f725f4405444c4cbe7382e7cacca63f)
Reviewed-on: https://chromium-review.googlesource.com/490847

[modify] https://crrev.com/2292d7429f3ec10cc9a4252fa877b597276859b1/chromeos-base/chromeos-accelerometer-init/files/udev/99-cros-ec-accel.rules
[modify] https://crrev.com/2292d7429f3ec10cc9a4252fa877b597276859b1/chromeos-base/chromeos-accelerometer-init/files/init/cros-ec-accel.conf
[rename] https://crrev.com/2292d7429f3ec10cc9a4252fa877b597276859b1/chromeos-base/chromeos-accelerometer-init/chromeos-accelerometer-init-0.0.1-r15.ebuild

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/83d7d925177b0cb734db954464401a498a222fe2

commit 83d7d925177b0cb734db954464401a498a222fe2
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Fri Apr 28 21:07:13 2017

CHROMIUM: iio: accel: Add cros_ec_accel_legacy driver.

Add driver to support older EC firmware that only support deprecated
ec command. Rely on ACPI memory map register to access sensor
information.
Present same interface as the regular cros_ec sensor stack:
- one iio device per accelerometer
- use HTML5 axis definition
- use iio abi units
- accept calibration calls, but do nothing
Chrome can use the same code than regular cros_ec sensor stack to
calculate orientation and lid angle.

BUG= chromium:709642 
TEST=Check sensor values with script:
DIR=/sys/bus/iio/devices/
for i in 0 1 ; do
  cat $DIR/iio:device$i/location
  for j in x y z; do
    echo -n $j: ; cat $DIR/iio:device$i/in_accel_${j}_raw
  done
done
Check chrome goes into tablet mode.

Change-Id: I1856e41cca3993e4f1604b5f6234d813c60b0973
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/483965
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
(cherry picked from commit 3bf98755f9c670c5c10ca05cba22848d65117cb2)
Reviewed-on: https://chromium-review.googlesource.com/490828

[modify] https://crrev.com/83d7d925177b0cb734db954464401a498a222fe2/drivers/iio/accel/Kconfig
[modify] https://crrev.com/83d7d925177b0cb734db954464401a498a222fe2/drivers/platform/chrome/cros_ec_dev.c
[modify] https://crrev.com/83d7d925177b0cb734db954464401a498a222fe2/drivers/iio/accel/Makefile
[add] https://crrev.com/83d7d925177b0cb734db954464401a498a222fe2/drivers/iio/accel/cros_ec_accel_legacy.c

Can this be closed?
Status: Fixed (was: Assigned)
We are not fixing ectool motionsense on glimmer, but now, the sensors with the current EC show up in 4.4.
Project Member

Comment 17 by bugdroid1@chromium.org, May 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/05842b0e6433314a3a0abe755f1ffcb77ce96f94

commit 05842b0e6433314a3a0abe755f1ffcb77ce96f94
Author: Guenter Roeck <groeck@chromium.org>
Date: Tue May 02 18:45:39 2017

FIXUP: CHROMIUM: iio: accel: Add cros_ec_accel_legacy driver

Fix the following build warnings.

drivers/iio/accel/cros_ec_accel_legacy.c:223:6-12: WARNING:
	Unsigned expression compared with zero: status < 0

drivers/iio/accel/cros_ec_accel_legacy.c:307:13: warning:
	no previous prototype for cros_ec_accel_legacy_capture

BUG= chromium:709642 
TEST=Build with W=1

Change-Id: I0d09ce476afb0afdb7f253045912f7fa44624766
Signed-off-by: Guenter Roeck <groeck@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/490747
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

[modify] https://crrev.com/05842b0e6433314a3a0abe755f1ffcb77ce96f94/drivers/iio/accel/cros_ec_accel_legacy.c

Project Member

Comment 18 by bugdroid1@chromium.org, May 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/b16b2f6897b5a53627d46499a68e4a160388372f

commit b16b2f6897b5a53627d46499a68e4a160388372f
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Sat May 13 08:04:43 2017

init: Exclude cros-ec-accel AND cros-ec-accel-legacy devices

These devices are already claimed by cros-ec-accel.conf.
Adding * to the match to include cros-ec-accel-legacy.

BUG=b:38189383, chromium:709642 
TEST=Check accelerometer is matched in time on eve.

Change-Id: I6d695fc9cee6a023f6ed5dbc2f098a6c9c05db3a
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/502172
Tested-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/b16b2f6897b5a53627d46499a68e4a160388372f/init/upstart/udev-trigger.conf

Project Member

Comment 19 by bugdroid1@chromium.org, May 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/df8c5bb29df16ba386d80b687935208dd8b51c8a

commit df8c5bb29df16ba386d80b687935208dd8b51c8a
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Sat May 13 17:56:43 2017

init: Exclude cros-ec-accel AND cros-ec-accel-legacy devices

These devices are already claimed by cros-ec-accel.conf.
Adding * to the match to include cros-ec-accel-legacy.

BUG=b:38189383, chromium:709642 
TEST=Check accelerometer is matched in time on eve.

Change-Id: I6d695fc9cee6a023f6ed5dbc2f098a6c9c05db3a
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/502172
Tested-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
(cherry picked from commit b16b2f6897b5a53627d46499a68e4a160388372f)
Reviewed-on: https://chromium-review.googlesource.com/505423

[modify] https://crrev.com/df8c5bb29df16ba386d80b687935208dd8b51c8a/init/upstart/udev-trigger.conf

Project Member

Comment 20 by bugdroid1@chromium.org, Nov 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/f2b141a242e59017dbc774dc916748670a41da0b

commit f2b141a242e59017dbc774dc916748670a41da0b
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Thu Nov 16 06:37:51 2017

FIXUP: CHROMIUM: iio: accel: Add cros_ec_accel_legacy driver

Because we read the LPC memory directly, we would read the primary EC
while setting up the downstream EC, incorrectly re-discovering the
accelerometers connected to the primary EC and associating them with the
secondary EC.

BUG= chromium:709642 
TEST=On Samus with 4.4:
Before on the secondary EC (cros_pd), at
/sys/devices/.../PNP0C09:00/GOOG0004:00/cros-ec-ctl.2.auto/
We have chromeos, cros-ec-accel-legacy.0, cros-ec-accel-legacy.1
and cros-usb-pd-charger.
In iio/devices, we have cros-ec-accel and cros-ec-accel-legacy pointing
to the same accelerometer.

Now, only we have only chromeos and cros-usb-pd-charger.

Change-Id: I5f9b8c026ee0a9700d7e31a1c46357763c17b634
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/772807
Reviewed-by: Guenter Roeck <groeck@chromium.org>

[modify] https://crrev.com/f2b141a242e59017dbc774dc916748670a41da0b/drivers/platform/chrome/cros_ec_dev.c

Sign in to add a comment