[PATCH] cros_ec: instantiate properly Intel ISH MCU device

[PATCH] cros_ec: instantiate properly Intel ISH MCU device

[Rushikesh S Kadam]

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'.

Signed-off-by: Rushikesh S Kadam <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

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

[Enric Balletbo i Serra]

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
> 

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@collabora.com> 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@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
> >

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

[Enric Balletbo i Serra]

Hi Jett,

Many thanks for the explanation.

On 27/2/19 16:13, Jett Rink wrote:
> 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.
> 

IMHO the problem with names is that is not really scalable, right now, we have

/sys/class/chromeos/cros_ec
/dev/cros_ec

and

/sys/class/chromeos/cros_pd
/dev/cros_pd

but looks like, apart from this, we will have

/sys/class/chromeos/cros_ish
/dev/cros_ish

And a lot more: cros_fp, cros_tp, cros_scp, etc


I was thinking in a solution more scalable where all the cros-ec are enumerated
by an index, so is more generic. So in a system with 2 cros-ec you'll have

/sys/class/chromeos/cros_ec0
/dev/cros_ec0

/sys/class/chromeos/cros_ec1
/dev/cros_ec1

...

In such case, from userspace point of view, when you open the device you can
send the EC_CMD_GET_FEATURES and know which EC is under the device.

One of the problems I see is that some old ECs (cros_pd I guess) doesn't
implement that command, in such case maybe we can continue using the name to
differentiate from other ECs.

I'm unsure yet of the impact of this change though, so I'd like to listen
Guenter and Benson opinions here :)

Will this solution work for you? Do you see any problem?


> 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@collabora.com> 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@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
>>>

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

[Gwendal Grignou]

On Wed, Feb 27, 2019 at 10:08 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Jett,
>
> Many thanks for the explanation.
>
> On 27/2/19 16:13, Jett Rink wrote:
> > 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
That's the default, we can override the name with --name option.
> >
> > 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.
> >
>
> IMHO the problem with names is that is not really scalable, right now, we have
>
> /sys/class/chromeos/cros_ec
> /dev/cros_ec
>
> and
>
> /sys/class/chromeos/cros_pd
> /dev/cros_pd
>
> but looks like, apart from this, we will have
>
> /sys/class/chromeos/cros_ish
> /dev/cros_ish
>
> And a lot more: cros_fp, cros_tp, cros_scp, etc
>
>
> I was thinking in a solution more scalable where all the cros-ec are enumerated
> by an index, so is more generic. So in a system with 2 cros-ec you'll have
>
> /sys/class/chromeos/cros_ec0
> /dev/cros_ec0
>
> /sys/class/chromeos/cros_ec1
> /dev/cros_ec1
>
> ...
>
> In such case, from userspace point of view, when you open the device you can
> send the EC_CMD_GET_FEATURES and know which EC is under the device.
>
> One of the problems I see is that some old ECs (cros_pd I guess) doesn't
> implement that command, in such case maybe we can continue using the name to
> differentiate from other ECs.
>
> I'm unsure yet of the impact of this change though, so I'd like to listen
> Guenter and Benson opinions here :)

You're right, the cros_ names are based on what the EC provides.
cros_ec for generic EC, fp, tp for fingerprint, touch pad
respectively.
ish is for standalone sensor hub [it does not have to be Intel Sensor Hub].

ChromeOS user space tool are using the cros_XX names directly, like
biod is using cros_fp.

I agree the number of standalone EC in the system is increasing, but
it is still bounded.

Gwendal.



>
> Will this solution work for you? Do you see any problem?
>
>
> > 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@collabora.com> 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@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
> >>>

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

[Enric Balletbo Serra]

Hi Gwendal,

Missatge de Gwendal Grignou <gwendal@chromium.org> del dia dc., 27 de
febr. 2019 a les 19:37:
>
> On Wed, Feb 27, 2019 at 10:08 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
> >
> > Hi Jett,
> >
> > Many thanks for the explanation.
> >
> > On 27/2/19 16:13, Jett Rink wrote:
> > > 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
> That's the default, we can override the name with --name option.
> > >
> > > 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.
> > >
> >
> > IMHO the problem with names is that is not really scalable, right now, we have
> >
> > /sys/class/chromeos/cros_ec
> > /dev/cros_ec
> >
> > and
> >
> > /sys/class/chromeos/cros_pd
> > /dev/cros_pd
> >
> > but looks like, apart from this, we will have
> >
> > /sys/class/chromeos/cros_ish
> > /dev/cros_ish
> >
> > And a lot more: cros_fp, cros_tp, cros_scp, etc
> >
> >
> > I was thinking in a solution more scalable where all the cros-ec are enumerated
> > by an index, so is more generic. So in a system with 2 cros-ec you'll have
> >
> > /sys/class/chromeos/cros_ec0
> > /dev/cros_ec0
> >
> > /sys/class/chromeos/cros_ec1
> > /dev/cros_ec1
> >
> > ...
> >
> > In such case, from userspace point of view, when you open the device you can
> > send the EC_CMD_GET_FEATURES and know which EC is under the device.
> >
> > One of the problems I see is that some old ECs (cros_pd I guess) doesn't
> > implement that command, in such case maybe we can continue using the name to
> > differentiate from other ECs.
> >
> > I'm unsure yet of the impact of this change though, so I'd like to listen
> > Guenter and Benson opinions here :)
>
> You're right, the cros_ names are based on what the EC provides.
> cros_ec for generic EC, fp, tp for fingerprint, touch pad
> respectively.
> ish is for standalone sensor hub [it does not have to be Intel Sensor Hub].
>

Thanks for the explanation. I didn't know that and I assumed the 'i'
was for 'intel', maybe would be good call cros_ssh or cros_sh to avoid
confusion?

> ChromeOS user space tool are using the cros_XX names directly, like
> biod is using cros_fp.
>
> I agree the number of standalone EC in the system is increasing, but
> it is still bounded.

Let me ask another question :)

I understand different names based on what the EC provides. Is this
also the case for the cros_scp? In other words, will cros_ec and
cros_scp co-exist and provide different functionalities than ec, fp,
tp or standalone sensor hub?

Thanks,
Enric

> Gwendal.
>
>
>
> >
> > Will this solution work for you? Do you see any problem?
> >
> >
> > > 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@collabora.com> 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@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
> > >>>

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

[Andy Shevchenko]

On Thu, Feb 28, 2019 at 12:05:35AM +0100, Enric Balletbo Serra wrote:
> Missatge de Gwendal Grignou <gwendal@chromium.org> del dia dc., 27 de
> febr. 2019 a les 19:37:
> > On Wed, Feb 27, 2019 at 10:08 AM Enric Balletbo i Serra
> > <enric.balletbo@collabora.com> wrote:
> > > On 27/2/19 16:13, Jett Rink wrote:

> > You're right, the cros_ names are based on what the EC provides.
> > cros_ec for generic EC, fp, tp for fingerprint, touch pad
> > respectively.
> > ish is for standalone sensor hub [it does not have to be Intel Sensor Hub].
> >
> 
> Thanks for the explanation. I didn't know that and I assumed the 'i'
> was for 'intel', maybe would be good call cros_ssh or cros_sh to avoid
> confusion?

i is for Integrated. There is no confusion.
It seems you misread what was written in [] above.

-- 
With Best Regards,
Andy Shevchenko

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

[Enric Balletbo Serra]

Hi Andy,
Missatge de Andy Shevchenko <andriy.shevchenko@intel.com> del dia dj.,
28 de febr. 2019 a les 8:57:
>
> On Thu, Feb 28, 2019 at 12:05:35AM +0100, Enric Balletbo Serra wrote:
> > Missatge de Gwendal Grignou <gwendal@chromium.org> del dia dc., 27 de
> > febr. 2019 a les 19:37:
> > > On Wed, Feb 27, 2019 at 10:08 AM Enric Balletbo i Serra
> > > <enric.balletbo@collabora.com> wrote:
> > > > On 27/2/19 16:13, Jett Rink wrote:
>
> > > You're right, the cros_ names are based on what the EC provides.
> > > cros_ec for generic EC, fp, tp for fingerprint, touch pad
> > > respectively.
> > > ish is for standalone sensor hub [it does not have to be Intel Sensor Hub].
> > >
> >
> > Thanks for the explanation. I didn't know that and I assumed the 'i'
> > was for 'intel', maybe would be good call cros_ssh or cros_sh to avoid
> > confusion?
>
> i is for Integrated. There is no confusion.

Ok, thanks for the clarification. Now that is clear to me I'll send
some reviews of this patch in a minute.

Best regards,
Enric

> It seems you misread what was written in [] above.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

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

[Enric Balletbo Serra]

Hi,

Based on the discussion we had, only some few comments.

Missatge de Rushikesh S Kadam <rushikesh.s.kadam@intel.com> del dia
dg., 24 de febr. 2019 a les 10:15:
>
> 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'.
>
> Signed-off-by: Rushikesh S Kadam <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 ++

Better if you use the "mfd: cros_ec:" prefix in the next version so
it's clear that this should go through the MFD tree.

>  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 */

"Check whether this is actually an Integrated Sensor Hub (ISH) rather
than an EC"?

> +       if (cros_ec_check_features(ec, EC_FEATURE_ISH)) {
> +               dev_info(dev, "Intel ISH MCU detected.\n");

 dev_info(dev, "CrOS 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,

Just a note that we will have a trivial conflict to solve with
https://lkml.org/lkml/2019/2/27/723

>  };
>
>  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> --
> 1.9.1
>

Best regards,
Enric

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

[Gwendal Grignou]

On Thu, Feb 28, 2019 at 4:18 AM Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
>
> Hi Andy,
> Missatge de Andy Shevchenko <andriy.shevchenko@intel.com> del dia dj.,
> 28 de febr. 2019 a les 8:57:
> >
> > On Thu, Feb 28, 2019 at 12:05:35AM +0100, Enric Balletbo Serra wrote:
> > > Missatge de Gwendal Grignou <gwendal@chromium.org> del dia dc., 27 de
> > > febr. 2019 a les 19:37:
> > > > On Wed, Feb 27, 2019 at 10:08 AM Enric Balletbo i Serra
> > > > <enric.balletbo@collabora.com> wrote:
> > > > > On 27/2/19 16:13, Jett Rink wrote:
> >
> > > > You're right, the cros_ names are based on what the EC provides.
> > > > cros_ec for generic EC, fp, tp for fingerprint, touch pad
> > > > respectively.
> > > > ish is for standalone sensor hub [it does not have to be Intel Sensor Hub].
> > > >
> > >
> > > Thanks for the explanation. I didn't know that and I assumed the 'i'
> > > was for 'intel', maybe would be good call cros_ssh or cros_sh to avoid
> > > confusion?
> >
> > i is for Integrated. There is no confusion.
>
> Ok, thanks for the clarification. Now that is clear to me I'll send
> some reviews of this patch in a minute.
Regarding cros_scp, it may coexist with cros_ec or be standalone. It
is not clear yet.

Gwendal.
>
> Best regards,
> Enric
>
> > It seems you misread what was written in [] above.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

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

[Rushikesh S Kadam]

Thanks Enric, Jett, Gwendal & Andy for taking timeout to review
the patch & provide detailed inputs.

Couple of acks below.


On Thu, Feb 28, 2019 at 01:33:09PM +0100, Enric Balletbo Serra wrote:
> Hi,
> 
> Based on the discussion we had, only some few comments.
> 
> Missatge de Rushikesh S Kadam <rushikesh.s.kadam@intel.com> del dia
> dg., 24 de febr. 2019 a les 10:15:
> >
> > 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'.
> >
> > Signed-off-by: Rushikesh S Kadam <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 ++
> 
> Better if you use the "mfd: cros_ec:" prefix in the next version so
> it's clear that this should go through the MFD tree.

Ok, will do.

> 
> >  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 */
> 
> "Check whether this is actually an Integrated Sensor Hub (ISH) rather
> than an EC"?

Ack.

> 
> > +       if (cros_ec_check_features(ec, EC_FEATURE_ISH)) {
> > +               dev_info(dev, "Intel ISH MCU detected.\n");
> 
>  dev_info(dev, "CrOS ISH MCU detected.\n"); ?

Ack.

> 
> > +               /*
> > +                * 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,
> 
> Just a note that we will have a trivial conflict to solve with
> https://lkml.org/lkml/2019/2/27/723

Thanks

Rushikesh

> 
> >  };
> >
> >  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> > --
> > 1.9.1
> >
> 
> Best regards,
> Enric

--