Re: [PATCH] cros_ec: instantiate properly Intel ISH MCU device

[Jett Rink]

The diagram you provided is correct.

The ISH device is a MCU that can run general purpose code that is
located on the SoC. Typically the ISH collects sensor data and
processes it before giving it to the AP. The ISH HW can run any RTOS,
and in this scenario we are choosing to run the ECOS RTOS. In doing
so, we have access to the standard cros_ec command interface for
communication between the AP and ISH. This is helpful because we have
sensors drivers (Cros EC IIO Sensors) that depend on the cros_ec
command interface.

The idea behind using a different sysfs path was to ensure that there
weren't any unintended consequences by adding a cros_ec device when
the ISH doesn't support most of the EC type tasks. Here are a few
examples:
 - Mosys tool is specifically trying to talk with an EC:
https://chromium.googlesource.com/chromiumos/platform/mosys/+/152a306a82009cd6ca68760b0902fc03781e0d5d/platform/daisy/ec.c#41
 - The ectool is specifically trying to talk with an EC:
https://chromium.googlesource.com/chromiumos/platform/ec/+/a3c27b39fa69f280a3728a5cf24de57a5d3ccb0d/util/ectool.c#8546

At least on active project, there has already been confusion that the
normal ectool program doesn't work because the EC on the system is 3rd
party and does not support the normal cros_ec interface. This
confusion would compound if the ISH started presenting the cros_ec
interface and the ectool started talking to the ISH instead of the 3rd
party EC. Maybe we just need to modify ectool to make that situation
less confusing, but this is an example of the cross over using the
cros_ish device name is trying to avoid.

At this point though, we will definitely follow the guidance of people
more experienced in kernel design. If using cros_ish as a device name
instead of cros_ec is actually not a good idea, then we can accept
that and move forward and try to see what the fallout is from
userspace tools.

-Jett


On Wed, Feb 27, 2019 at 7:22 AM Enric Balletbo i Serra
<enric.balletbo@xxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 26/2/19 18:21, Jett Rink wrote:
> > We are specifically wanting userspace applications to not worry/confuse cros_ish
> > with a normal cros_ec. Adding an attribute instead of changing the path would
> > make it easy for userspace application to forget to check properly before
> > accessing the ish as an EC when it shouldn't.
> >
> > On Mon, Feb 25, 2019 at 4:37 PM Guenter Roeck <groeck@xxxxxxxxxx
> > <mailto:groeck@xxxxxxxxxx>> wrote:
> >
> >     On Mon, Feb 25, 2019 at 3:22 PM Jett Rink <jettrink@xxxxxxxxxxxx
> >     <mailto:jettrink@xxxxxxxxxxxx>> wrote:
> >
> >         A cros_ec and cros_ish device could both be present on the same system.
> >         We want to change the device path to ensure that drivers/code further up
> >         the stack does not get confuse the ISH with as an EC.
> >
> >         The ISH device can export a similar sysfs interface as they both use the
> >         same command interface for communication (although they will use
> >         different transport layers). The common cros code will detect certain
> >         EC_FEATURES and enable the correct subsystem based on the level of
> >         functionality the device supports. In the case of the ISH, the sensor
> >         subsystem will be enabled.
> >
> >     Seems to me it would make more sense to handle that difference with a sysfs
> >     attribute (instead of forcing each userspace application to support multiple
> >     sysfs paths).
> >
>
> Is still unclear to me what's that ISH device, so I'd appreciate if you can give
> some more background. Trying to understand the topology, makes sense that block
> diagram to you?
>
>
>        ---------------------------
>       |  User Space Applications  |
>        ---------------------------
>
> ----------------IIO ABI----------------
>
>        -----------------------------
>       |  CrOS EC IIO Sensor Drivers |
>        -----------------------------
>
>        --------------------------
>       |  CrOS EC over ISH Driver |
>        --------------------------
>
> ---------------- OS ------------------
>
>        --------------------------
>       |     CrOS EC Firmware     |
>        --------------------------
>
>        --------------------------
>       | ISH Hardware/Firmware    |
>        --------------------------
>
> So I'm right assuming that this CrOS EC will implement only the sensor features?
>
> And then, the system will have another CrOS EC implementing other features like
> RTC, USBPD-charger, etc?
>
> Apart from the sensors features, will the CrOS EC ISH implement other features?
>
> I'm a bit worried about the increasing way to use a particular name for
> different CrOS EC, actually we have only cros_ec and cros_pd. But in the
> chromeos kernels there is /dev/cros_fp, /dev/cros_tp, /dev/cros_ish,
> /dev/cros_scp and who knows how many more in the future. So I'm wondering if
> wouldn't be better use standard names, i.e /dev/cros_ec0, /dev/cros_ec1, etc. as
> userspace, for those cases, should be able to query the EC_FEATURE_ISH/FP/TP/SCP
> and know against which EC the device is attached.
>
> Cheers,
> Enric
>
> >     Guenter
> >
> >
> >         -Jett
> >
> >         On Sun, Feb 24, 2019 at 4:03 PM Guenter Roeck <groeck@xxxxxxxxxx
> >         <mailto:groeck@xxxxxxxxxx>> wrote:
> >
> >             On Sun, Feb 24, 2019 at 1:14 AM Rushikesh S Kadam
> >             <rushikesh.s.kadam@xxxxxxxxx <mailto:rushikesh.s.kadam@xxxxxxxxx>>
> >             wrote:
> >
> >                 Intel Integrated Sensor Hub (ISH) is also a MCU running EC
> >                 having feature bit EC_FEATURE_ISH. Instantiate it as a special
> >                 CrOS EC device with device name 'cros_ish'.
> >
> >
> >             The type of MCU doesn't really have to be reflected in the sysfs
> >             directory path. cros_ec uses different
> >             MCUs over time.
> >
> >             Will the new path exist in parallel with cros_ec (in other words,
> >             will there also be a stand-alone EC in the same system) ? Does it
> >             have different or the same sysfs attributes as cros_ec ?
> >
> >             Also,, what is the impact on userspace ?
> >
> >             Thanks,
> >             Guenter
> >
> >                 Signed-off-by: Rushikesh S Kadam <rushikesh.s.kadam@xxxxxxxxx
> >                 <mailto:rushikesh.s.kadam@xxxxxxxxx>>
> >                 ---
> >                  drivers/mfd/cros_ec_dev.c            | 10 ++++++++++
> >                  include/linux/mfd/cros_ec.h          |  1 +
> >                  include/linux/mfd/cros_ec_commands.h |  2 ++
> >                  3 files changed, 13 insertions(+)
> >
> >                 diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> >                 index 2d0fee4..be499b8 100644
> >                 --- a/drivers/mfd/cros_ec_dev.c
> >                 +++ b/drivers/mfd/cros_ec_dev.c
> >                 @@ -414,6 +414,16 @@ static int ec_device_probe(struct
> >                 platform_device *pdev)
> >                         device_initialize(&ec->class_dev);
> >                         cdev_init(&ec->cdev, &fops);
> >
> >                 +       /* check whether this is actually a Intel ISH rather
> >                 than an EC */
> >                 +       if (cros_ec_check_features(ec, EC_FEATURE_ISH)) {
> >                 +               dev_info(dev, "Intel ISH MCU detected.\n");
> >                 +               /*
> >                 +                * Help userspace differentiating ECs from ISH MCU,
> >                 +                * regardless of the probing order.
> >                 +                */
> >                 +               ec_platform->ec_name = CROS_EC_DEV_ISH_NAME;
> >                 +       }
> >                 +
> >                         /*
> >                          * Add the class device
> >                          * Link to the character device for creating the /dev entry
> >                 diff --git a/include/linux/mfd/cros_ec.h
> >                 b/include/linux/mfd/cros_ec.h
> >                 index de8b588..00c5765 100644
> >                 --- a/include/linux/mfd/cros_ec.h
> >                 +++ b/include/linux/mfd/cros_ec.h
> >                 @@ -24,6 +24,7 @@
> >
> >                  #define CROS_EC_DEV_NAME "cros_ec"
> >                  #define CROS_EC_DEV_PD_NAME "cros_pd"
> >                 +#define CROS_EC_DEV_ISH_NAME "cros_ish"
> >
> >                  /*
> >                   * The EC is unresponsive for a time after a reboot command.  Add a
> >                 diff --git a/include/linux/mfd/cros_ec_commands.h
> >                 b/include/linux/mfd/cros_ec_commands.h
> >                 index fc91082..9276c3c 100644
> >                 --- a/include/linux/mfd/cros_ec_commands.h
> >                 +++ b/include/linux/mfd/cros_ec_commands.h
> >                 @@ -856,6 +856,8 @@ enum ec_feature_code {
> >                         EC_FEATURE_RTC = 27,
> >                         /* EC supports CEC commands */
> >                         EC_FEATURE_CEC = 35,
> >                 +       /* The MCU is an Intel Integrated Sensor Hub */
> >                 +       EC_FEATURE_ISH = 40,
> >                  };
> >
> >                  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> >                 --
> >                 1.9.1
> >