Re: [PATCH] cros_ec: instantiate properly Intel ISH MCU device
From: Enric Balletbo i Serra
Date: Wed Feb 27 2019 - 09:22:11 EST
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
>