On Wed, Feb 27, 2019 at 10:08 AM Enric Balletbo i Serra <enric.balle...@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.balle...@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 <gro...@google.com > >>> <mailto:gro...@google.com>> wrote: > >>> > >>> On Mon, Feb 25, 2019 at 3:22 PM Jett Rink <jettr...@chromium.org > >>> <mailto:jettr...@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 <gro...@google.com > >>> <mailto:gro...@google.com>> wrote: > >>> > >>> On Sun, Feb 24, 2019 at 1:14 AM Rushikesh S Kadam > >>> <rushikesh.s.ka...@intel.com > >>> <mailto:rushikesh.s.ka...@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.ka...@intel.com > >>> <mailto:rushikesh.s.ka...@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 > >>>
