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

Issue 602832 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

ec: Consider removing cros_ec_readmem / CROS_EC_DEV_IOCRDMEM IOCTL

Project Member Reported by sha...@chromium.org, Apr 12 2016

Issue description

- CROS_EC_DEV_IOCRDMEM is not supported by non-LPC platforms
- CROS_EC_DEV_IOCRDMEM is redundant and can be replaced (with slightly less efficiency on LPC platforms) with CROS_EC_DEV_IOCXCMD + EC_CMD_READ_MEMMAP cmd
- flashrom and mosys don't even use it -- it's only used in the ec comm-dev library (eg. for ectool)
 
The IOCTL can probably go.  Mapped memory needs to stick around on x86 systems, since it's used by ACPI.

Owner: gwendal@chromium.org
Status: Started (was: Untriaged)
Slight complication.
When using LPC transport, the command EC_CMD_READ_MEMMAP is compiled out in the EC.

The way ectool is working:
- if dev interface present (which is always true on 3.14+ kernels)
  - if LPC transport, 
    use CROS_EC_DEV_IOCRDMEM ioctl for reading memory.
    use CROS_EC_DEV_IOCXCMD for commands.
  - else
    use CROS_EC_DEV_IOCXCMD for everything
- else
  use direct LPC access for everything. 
  Using direct access for commands is deprecated for 3.14+ kernel,
  the kernel is also sending commands.


If propose the following:
- if dev interface present (which is always true on 3.14+ kernels)
  -if CROS_EC_DEV_IOCXCMD + EC_CMD_READ_MEMMAP works,
     use it
  -else
     use direct LPC access.
...

This method allows removal of CROS_EC_DEV_IOCRDMEM ioctl, but requires an assumption that the EC is frugal.

Comment 4 by sha...@chromium.org, Apr 13 2016

EC_CMD_READ_MEMMAP should be supported by all production ECs going back to link.

Do we have any kernels in production today where the cros-dev interface is supported, CROS_EC_DEV_IOCRDMEM is supported, but CROS_EC_DEV_IOCXCMD is not?

Comment 5 by gwendal@google.com, Apr 13 2016

EC_CMD_READ_MEMMAP is not supported if LPC is enabled.
See 
https://cs.corp.google.com/chromeos_public/src/platform/ec/common/host_command.c?rcl=52f7b3a0c842810d119b63894ce5c12d88363b5c&l=478

If ros-dev interface is supported, CROS_EC_DEV_IOCXCMD is as well.
I sent a CL to remove CROS_EC_DEV_IOCRDMEM support.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/de45353bbdf0c6c4afb88420d3cb2e182eae7450

commit de45353bbdf0c6c4afb88420d3cb2e182eae7450
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Wed Apr 13 19:34:08 2016

ectool: Remove CROS_EC_DEV_IOCRDMEM

On !LPC EC, we can read memory via CROS_EC_DEV_IOCXCMD ioctl,
using command EC_CMD_READ_MEMMAP.
On platform that supports direct memory access (lpc), we access
the memory directly, bypassing the ioctl.

BUG= chromium:602832 
TEST=On gnawty and veyron, check 'ectool battery' works.
Verify that gnawty use io mapped registers.
BRANCH=none

Change-Id: I9bfcddcf450bf8df63ead78e1df97dd7392289e6
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/338853
Reviewed-by: Randall Spangler <rspangler@chromium.org>

[modify] https://crrev.com/de45353bbdf0c6c4afb88420d3cb2e182eae7450/util/comm-host.c
[modify] https://crrev.com/de45353bbdf0c6c4afb88420d3cb2e182eae7450/util/comm-host.h
[modify] https://crrev.com/de45353bbdf0c6c4afb88420d3cb2e182eae7450/util/comm-lpc.c
[modify] https://crrev.com/de45353bbdf0c6c4afb88420d3cb2e182eae7450/util/comm-dev.c

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 18 2016

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

commit c0567473579e03598a15d4601b184f516944735b
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Wed Apr 13 16:55:39 2016

cros_ec: Remove unused ioctl CROS_EC_DEV_IOCRDMEM

This ioctl is a duplicate of CROS_EC_DEV_IOCXCMD with command
EC_CMD_READ_MEMMAP.

BUG= chromium:602832 
CQ-DEPEND=I9bfcddcf450bf8df63ead78e1df97dd7392289e6
TEST=On gnawty, check 'ectool battery' works.

Change-Id: I81a8d0dcb0d4a7b0a142e17753a18369d4099bb0
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/338861
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Shawn N <shawnn@chromium.org>

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

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/e54d1284ffbd1d7c3b1f77a2a696ce275c527de4

commit e54d1284ffbd1d7c3b1f77a2a696ce275c527de4
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Fri Apr 22 18:09:33 2016

ectool: Revert "ectool: Remove CROS_EC_DEV_IOCRDMEM"

CROS_EC_DEV_IOCRDMEM must be used on architecture where legacy IO mapped
registers are accessed inderectly via EMI. The kernel is taking care of
the translation.

TEST=Check on reks that we need to use the IOCTL.
BUG=chrome-os-partner:52550, chromium:602832 
BRANCH=none

This reverts commit de45353bbdf0 ("ectool: Remove CROS_EC_DEV_IOCRDMEM").

Change-Id: I8efad56df90c58c25bdc9ccd70a508547e629a77
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/340348
Reviewed-by: Shawn N <shawnn@chromium.org>

[modify] https://crrev.com/e54d1284ffbd1d7c3b1f77a2a696ce275c527de4/util/comm-host.c
[modify] https://crrev.com/e54d1284ffbd1d7c3b1f77a2a696ce275c527de4/util/comm-host.h
[modify] https://crrev.com/e54d1284ffbd1d7c3b1f77a2a696ce275c527de4/util/comm-lpc.c
[modify] https://crrev.com/e54d1284ffbd1d7c3b1f77a2a696ce275c527de4/util/comm-dev.c

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 23 2016

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

commit 35a1e4401dc5e49ad3db9ad581816296808637fa
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Fri Apr 22 18:04:30 2016

CHROMIUM: Revert "cros_ec: Remove unused ioctl CROS_EC_DEV_IOCRDMEM"

Removing CROS_EC_DEV_IOCRDMEM is not possible.
On some architecture reading legacy IO mapped memory register is done
through EMI indirect access.
To make it transparent from the user space, re-add the ioctl.

TEST=Compile.
BUG=chrome-os-partner:52550, chromium:602832 

This reverts commit c0567473579e ("cros_ec: Remove unused ioctl
CROS_EC_DEV_IOCRDMEM")

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

[modify] https://crrev.com/35a1e4401dc5e49ad3db9ad581816296808637fa/drivers/platform/chrome/cros_ec_dev.h
[modify] https://crrev.com/35a1e4401dc5e49ad3db9ad581816296808637fa/drivers/platform/chrome/cros_ec_dev.c

Status: WontFix (was: Started)
CROS_EC_DEV_IOCRDMEM is needed after all.

Sign in to add a comment