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

Enric Balletbo i Serra
2019-02-27T14:22:11Z

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@google.com
> <mailto:groeck@google.com>> wrote:
>
> On Mon, Feb 25, 2019 at 3:22 PM Jett Rink <jettrink@chromium.org
> <mailto:jettrink@chromium.org>> 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@google.com
> <mailto:groeck@google.com>> wrote:
>
> On Sun, Feb 24, 2019 at 1:14 AM Rushikesh S Kadam
> <rushikesh.s.kadam@intel.com <mailto:rushikesh.s.kadam@intel.com>>
> 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@intel.com
> <mailto:rushikesh.s.kadam@intel.com>>
> ---
>  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
>