| Message ID | 1581476197-25854-1-git-send-email-Sandeep.Singh@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | SFH: Add Support for AMD Sensor Fusion Hub | expand |
Hi, On 2/12/20 3:56 AM, Sandeep Singh wrote: > From: Sandeep Singh <sandeep.singh@amd.com> > > AMD SFH(Sensor Fusion Hub) is HID based driver.SFH FW > is part of MP2 processor (MP2 which is an ARM® Cortex-M4 > core based co-processor to x86) and it runs on MP2 where > in driver resides on X86.The driver functionalities are > divided into three parts:- > > 1: amd-mp2-pcie:- This module will communicate with MP2 FW and > provide that data into DRAM. > 2: Client driver :- This part for driver will use dram data and > convert that data into HID format based on > HID reports. > 3: Transport driver :- This part of driver will communicate with > HID core. Communication between devices and > HID core is mostly done via HID reports > > In terms of architecture it is much more reassembles like > ISH(Intel Integrated Sensor Hub). However the major difference > is all the hid reports are generated as part of kernel driver. > AMD SFH driver taken reference from ISH in terms of > design and functionalities at fewer location. > > AMD sensor fusion Hub is part of a SOC 17h family based platforms. > The solution is working well on several OEM products. > AMD SFH uses HID over PCIe bus. I started looking at this patch because of the phoronix' news item on it. First of all I want to say that it is great that AMD is working on getting the Sensor Fusion Hub supported on Linux and that you are working on a driver for this. But, I've taken a quick look, mainly at the "[PATCH v3 5/5] SFH: Create HID report to Enable support of AMD sensor fusion Hub (SFH)" patch. AFAIK with the Intel ISH the sensor-hub itself is actually providing HID descriptors and HID input reports. Looking at the AMD code, that does not seem to be the case, it seems the values come directly from the AMD sensor-hub without being in any HID specific form, e.g.: +u8 get_input_report(int sensor_idx, int report_id, + u8 *input_report, u32 *sensor_virt_addr) +{ + u8 report_size = 0; + struct accel3_input_report acc_input; + struct gyro_input_report gyro_input; + struct magno_input_report magno_input; + struct als_input_report als_input; + + if (!sensor_virt_addr || !input_report) + return report_size; + + switch (sensor_idx) { + case ACCEL_IDX: /* accel */ + acc_input.common_property.report_id = report_id; + acc_input.common_property.sensor_state = + HID_USAGE_SENSOR_STATE_READY_ENUM; + acc_input.common_property.event_type = + HID_USAGE_SENSOR_EVENT_DATA_UPDATED_ENUM; + acc_input.in_accel_x_value = (int)sensor_virt_addr[0] / + AMD_SFH_FIRMWARE_MULTIPLIER; + acc_input.in_accel_y_value = (int)sensor_virt_addr[1] / + AMD_SFH_FIRMWARE_MULTIPLIER; + acc_input.in_accel_z_value = (int)sensor_virt_addr[2] / + AMD_SFH_FIRMWARE_MULTIPLIER; + memcpy(input_report, &acc_input, sizeof(acc_input)); + report_size = sizeof(acc_input); + break; And the descriptors are hardcoded in the driver so as to fake a HID device. So going through the HID subsystem seems like an unnecessary detour, which just makes things needlessly complex and harder to debug (and extend). The HID devices which the current patch-set is creating ultimately will result in a number of devices being created under /sys/bus/iio/devices And this are the devices which userspace uses to get the sensor data. IMHO instead of going through the HID subsys the AMD Sensor Fusion Hub driver should simply register 4 (*) iio-devices itself and directly pass the data through at the iio subsys level rather then going the long way around by creating a fake HID device which then gets attached to by the hid-sensor driver to ultimately create the same iio-devices. There are examples of e.g. various iio accel drivers under: drivers/iio/accel/ you could start with a simple driver supporting just the accelerometer bits and then extend things from there. Benjamin, Jiri, Jonathan, what is your take on this? Regards, Hans *) One for accel, gyra, magneto and light each > Sandeep Singh (5): > SFH: Add maintainers and documentation for AMD SFH based on HID > framework > SFH: PCI driver to add support of AMD sensor fusion Hub using HID > framework > SFH: Transport Driver to add support of AMD Sensor Fusion Hub (SFH) > SFH: Add debugfs support to AMD Sensor Fusion Hub > SFH: Create HID report to Enable support of AMD sensor fusion Hub > (SFH) > > Changes since v1: > -Fix auto build test warnings > -Fix warnings captured using smatch > -Changes suggested by Dan Carpenter > > Links of the review comments for v1: > [1] https://patchwork.kernel.org/patch/11325163/ > [2] https://patchwork.kernel.org/patch/11325167/ > [3] https://patchwork.kernel.org/patch/11325171/ > [4] https://patchwork.kernel.org/patch/11325187/ > > > Changes since v2: > -Debugfs divided into another patch > -Fix some cosmetic changes > -Fix for review comments > Reported and Suggested by:- Srinivas Pandruvada > > Links of the review comments for v2: > [1] https://patchwork.kernel.org/patch/11355491/ > [2] https://patchwork.kernel.org/patch/11355495/ > [3] https://patchwork.kernel.org/patch/11355499/ > [4] https://patchwork.kernel.org/patch/11355503/ > > > Documentation/hid/amd-sfh-hid.rst | 160 +++++ > MAINTAINERS | 8 + > drivers/hid/Kconfig | 2 + > drivers/hid/Makefile | 1 + > drivers/hid/amd-sfh-hid/Kconfig | 20 + > drivers/hid/amd-sfh-hid/Makefile | 17 + > drivers/hid/amd-sfh-hid/amd_mp2_pcie.c | 243 ++++++++ > drivers/hid/amd-sfh-hid/amd_mp2_pcie.h | 177 ++++++ > drivers/hid/amd-sfh-hid/amdsfh-debugfs.c | 250 ++++++++ > drivers/hid/amd-sfh-hid/amdsfh-debugfs.h | 14 + > drivers/hid/amd-sfh-hid/amdsfh-hid-client.c | 260 +++++++++ > drivers/hid/amd-sfh-hid/amdsfh-hid.c | 179 ++++++ > drivers/hid/amd-sfh-hid/amdsfh-hid.h | 85 +++ > .../hid_descriptor/amd_sfh_hid_descriptor.c | 275 +++++++++ > .../hid_descriptor/amd_sfh_hid_descriptor.h | 125 ++++ > .../hid_descriptor/amd_sfh_hid_report_descriptor.h | 642 +++++++++++++++++++++ > 16 files changed, 2458 insertions(+) > create mode 100644 Documentation/hid/amd-sfh-hid.rst > create mode 100644 drivers/hid/amd-sfh-hid/Kconfig > create mode 100644 drivers/hid/amd-sfh-hid/Makefile > create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.c > create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.h > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.c > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.h > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid-client.c > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.c > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.h > create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_descriptor.c > create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_descriptor.h > create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_descriptor.h >
Hi, On Wed, Feb 12, 2020 at 3:45 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 2/12/20 3:56 AM, Sandeep Singh wrote: > > From: Sandeep Singh <sandeep.singh@amd.com> > > > > AMD SFH(Sensor Fusion Hub) is HID based driver.SFH FW > > is part of MP2 processor (MP2 which is an ARM® Cortex-M4 > > core based co-processor to x86) and it runs on MP2 where > > in driver resides on X86.The driver functionalities are > > divided into three parts:- > > > > 1: amd-mp2-pcie:- This module will communicate with MP2 FW and > > provide that data into DRAM. > > 2: Client driver :- This part for driver will use dram data and > > convert that data into HID format based on > > HID reports. > > 3: Transport driver :- This part of driver will communicate with > > HID core. Communication between devices and > > HID core is mostly done via HID reports > > > > In terms of architecture it is much more reassembles like > > ISH(Intel Integrated Sensor Hub). However the major difference > > is all the hid reports are generated as part of kernel driver. > > AMD SFH driver taken reference from ISH in terms of > > design and functionalities at fewer location. > > > > AMD sensor fusion Hub is part of a SOC 17h family based platforms. > > The solution is working well on several OEM products. > > AMD SFH uses HID over PCIe bus. > > I started looking at this patch because of the phoronix' news item on it. > > First of all I want to say that it is great that AMD is working on > getting the Sensor Fusion Hub supported on Linux and that you are > working on a driver for this. Yep, couldn't agree more :) > > But, I've taken a quick look, mainly at the > "[PATCH v3 5/5] SFH: Create HID report to Enable support of AMD sensor fusion Hub (SFH)" > patch. > > AFAIK with the Intel ISH the sensor-hub itself is actually providing > HID descriptors and HID input reports. > > Looking at the AMD code, that does not seem to be the case, it seems > the values come directly from the AMD sensor-hub without being in any > HID specific form, e.g.: > > +u8 get_input_report(int sensor_idx, int report_id, > + u8 *input_report, u32 *sensor_virt_addr) > +{ > + u8 report_size = 0; > + struct accel3_input_report acc_input; > + struct gyro_input_report gyro_input; > + struct magno_input_report magno_input; > + struct als_input_report als_input; > + > + if (!sensor_virt_addr || !input_report) > + return report_size; > + > + switch (sensor_idx) { > + case ACCEL_IDX: /* accel */ > + acc_input.common_property.report_id = report_id; > + acc_input.common_property.sensor_state = > + HID_USAGE_SENSOR_STATE_READY_ENUM; > + acc_input.common_property.event_type = > + HID_USAGE_SENSOR_EVENT_DATA_UPDATED_ENUM; > + acc_input.in_accel_x_value = (int)sensor_virt_addr[0] / > + AMD_SFH_FIRMWARE_MULTIPLIER; > + acc_input.in_accel_y_value = (int)sensor_virt_addr[1] / > + AMD_SFH_FIRMWARE_MULTIPLIER; > + acc_input.in_accel_z_value = (int)sensor_virt_addr[2] / > + AMD_SFH_FIRMWARE_MULTIPLIER; > + memcpy(input_report, &acc_input, sizeof(acc_input)); > + report_size = sizeof(acc_input); > + break; > > And the descriptors are hardcoded in the driver so as to fake a HID > device. > > So going through the HID subsystem seems like an unnecessary detour, > which just makes things needlessly complex and harder to debug > (and extend). > > The HID devices which the current patch-set is creating ultimately > will result in a number of devices being created under > > /sys/bus/iio/devices > > And this are the devices which userspace uses to get the sensor data. > > IMHO instead of going through the HID subsys the AMD Sensor Fusion Hub > driver should simply register 4 (*) iio-devices itself and directly > pass the data through at the iio subsys level rather then going the > long way around by creating a fake HID device which then gets > attached to by the hid-sensor driver to ultimately create the same > iio-devices. > > There are examples of e.g. various iio accel drivers under: > drivers/iio/accel/ you could start with a simple driver supporting > just the accelerometer bits and then extend things from there. > > Benjamin, Jiri, Jonathan, what is your take on this? Hard to say without knowing AMD roadmap for that. If they intend to have an ISH-like approach in the end with reports and descriptors provided by the firmwares, then it makes sense to keep this architecture for the first revision of devices. If not, then yes, this is probably overkill compared to what needs to be done. Sandeep, can you explain to us why you think using HID is the best way? On a side note, I don't necessarily like patch 4/5 with the debugfs interface. It's adding a kernel API for no gain, and we should already have the debug API available in the various subsystems involved. Cheers, Benjamin > > Regards, > > Hans > > > *) One for accel, gyra, magneto and light each > > > > Sandeep Singh (5): > > SFH: Add maintainers and documentation for AMD SFH based on HID > > framework > > SFH: PCI driver to add support of AMD sensor fusion Hub using HID > > framework > > SFH: Transport Driver to add support of AMD Sensor Fusion Hub (SFH) > > SFH: Add debugfs support to AMD Sensor Fusion Hub > > SFH: Create HID report to Enable support of AMD sensor fusion Hub > > (SFH) > > > > Changes since v1: > > -Fix auto build test warnings > > -Fix warnings captured using smatch > > -Changes suggested by Dan Carpenter > > > > Links of the review comments for v1: > > [1] https://patchwork.kernel.org/patch/11325163/ > > [2] https://patchwork.kernel.org/patch/11325167/ > > [3] https://patchwork.kernel.org/patch/11325171/ > > [4] https://patchwork.kernel.org/patch/11325187/ > > > > > > Changes since v2: > > -Debugfs divided into another patch > > -Fix some cosmetic changes > > -Fix for review comments > > Reported and Suggested by:- Srinivas Pandruvada > > > > Links of the review comments for v2: > > [1] https://patchwork.kernel.org/patch/11355491/ > > [2] https://patchwork.kernel.org/patch/11355495/ > > [3] https://patchwork.kernel.org/patch/11355499/ > > [4] https://patchwork.kernel.org/patch/11355503/ > > > > > > Documentation/hid/amd-sfh-hid.rst | 160 +++++ > > MAINTAINERS | 8 + > > drivers/hid/Kconfig | 2 + > > drivers/hid/Makefile | 1 + > > drivers/hid/amd-sfh-hid/Kconfig | 20 + > > drivers/hid/amd-sfh-hid/Makefile | 17 + > > drivers/hid/amd-sfh-hid/amd_mp2_pcie.c | 243 ++++++++ > > drivers/hid/amd-sfh-hid/amd_mp2_pcie.h | 177 ++++++ > > drivers/hid/amd-sfh-hid/amdsfh-debugfs.c | 250 ++++++++ > > drivers/hid/amd-sfh-hid/amdsfh-debugfs.h | 14 + > > drivers/hid/amd-sfh-hid/amdsfh-hid-client.c | 260 +++++++++ > > drivers/hid/amd-sfh-hid/amdsfh-hid.c | 179 ++++++ > > drivers/hid/amd-sfh-hid/amdsfh-hid.h | 85 +++ > > .../hid_descriptor/amd_sfh_hid_descriptor.c | 275 +++++++++ > > .../hid_descriptor/amd_sfh_hid_descriptor.h | 125 ++++ > > .../hid_descriptor/amd_sfh_hid_report_descriptor.h | 642 +++++++++++++++++++++ > > 16 files changed, 2458 insertions(+) > > create mode 100644 Documentation/hid/amd-sfh-hid.rst > > create mode 100644 drivers/hid/amd-sfh-hid/Kconfig > > create mode 100644 drivers/hid/amd-sfh-hid/Makefile > > create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.c > > create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.h > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.c > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.h > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid-client.c > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.c > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.h > > create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_descriptor.c > > create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_descriptor.h > > create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_descriptor.h > > >
On Thu, 2020-02-13 at 15:56 +0100, Benjamin Tissoires wrote: > Hi, > > On Wed, Feb 12, 2020 at 3:45 PM Hans de Goede <hdegoede@redhat.com> > wrote: > > Hi, > > > > On 2/12/20 3:56 AM, Sandeep Singh wrote: > > > From: Sandeep Singh <sandeep.singh@amd.com> > > > > > > AMD SFH(Sensor Fusion Hub) is HID based driver.SFH FW > > > is part of MP2 processor (MP2 which is an ARM® Cortex-M4 > > > core based co-processor to x86) and it runs on MP2 where > > > in driver resides on X86.The driver functionalities are > > > divided into three parts:- > > > > > > 1: amd-mp2-pcie:- This module will communicate with MP2 FW > > > and > > > provide that data into DRAM. > > > 2: Client driver :- This part for driver will use dram data > > > and > > > convert that data into HID format based > > > on > > > HID reports. > > > 3: Transport driver :- This part of driver will communicate with > > > HID core. Communication between devices > > > and > > > HID core is mostly done via HID reports > > > > > > In terms of architecture it is much more reassembles like > > > ISH(Intel Integrated Sensor Hub). However the major difference > > > is all the hid reports are generated as part of kernel driver. > > > AMD SFH driver taken reference from ISH in terms of > > > design and functionalities at fewer location. > > > > > > AMD sensor fusion Hub is part of a SOC 17h family based > > > platforms. > > > The solution is working well on several OEM products. > > > AMD SFH uses HID over PCIe bus. > > > > I started looking at this patch because of the phoronix' news item > > on it. > > > > First of all I want to say that it is great that AMD is working on > > getting the Sensor Fusion Hub supported on Linux and that you are > > working on a driver for this. > > Yep, couldn't agree more :) > > > But, I've taken a quick look, mainly at the > > "[PATCH v3 5/5] SFH: Create HID report to Enable support of AMD > > sensor fusion Hub (SFH)" > > patch. > > > > AFAIK with the Intel ISH the sensor-hub itself is actually > > providing > > HID descriptors and HID input reports. > > > > Looking at the AMD code, that does not seem to be the case, it > > seems > > the values come directly from the AMD sensor-hub without being in > > any > > HID specific form, e.g.: > > > > +u8 get_input_report(int sensor_idx, int report_id, > > + u8 *input_report, u32 *sensor_virt_addr) > > +{ > > + u8 report_size = 0; > > + struct accel3_input_report acc_input; > > + struct gyro_input_report gyro_input; > > + struct magno_input_report magno_input; > > + struct als_input_report als_input; > > + > > + if (!sensor_virt_addr || !input_report) > > + return report_size; > > + > > + switch (sensor_idx) { > > + case ACCEL_IDX: /* accel */ > > + acc_input.common_property.report_id = report_id; > > + acc_input.common_property.sensor_state = > > + HID_USAGE_SENSOR_STATE_READ > > Y_ENUM; > > + acc_input.common_property.event_type = > > + HID_USAGE_SENSOR_EVENT_DATA_UPDATED > > _ENUM; > > + acc_input.in_accel_x_value = > > (int)sensor_virt_addr[0] / > > + AMD_SFH_FIRMWARE_MU > > LTIPLIER; > > + acc_input.in_accel_y_value = > > (int)sensor_virt_addr[1] / > > + AMD_SFH_FIRMWARE_MU > > LTIPLIER; > > + acc_input.in_accel_z_value > > = (int)sensor_virt_addr[2] / > > + AMD_SFH_FIRMWARE_MU > > LTIPLIER; > > + memcpy(input_report, &acc_input, > > sizeof(acc_input)); > > + report_size = sizeof(acc_input); > > + break; > > > > And the descriptors are hardcoded in the driver so as to fake a HID > > device. > > > > So going through the HID subsystem seems like an unnecessary > > detour, > > which just makes things needlessly complex and harder to debug > > (and extend). > > > > The HID devices which the current patch-set is creating ultimately > > will result in a number of devices being created under > > > > /sys/bus/iio/devices > > > > And this are the devices which userspace uses to get the sensor > > data. > > > > IMHO instead of going through the HID subsys the AMD Sensor Fusion > > Hub > > driver should simply register 4 (*) iio-devices itself and directly > > pass the data through at the iio subsys level rather then going the > > long way around by creating a fake HID device which then gets > > attached to by the hid-sensor driver to ultimately create the same > > iio-devices. > > > > There are examples of e.g. various iio accel drivers under: > > drivers/iio/accel/ you could start with a simple driver supporting > > just the accelerometer bits and then extend things from there. > > > > Benjamin, Jiri, Jonathan, what is your take on this? > > Hard to say without knowing AMD roadmap for that. If they intend to > have an ISH-like approach in the end with reports and descriptors > provided by the firmwares, then it makes sense to keep this > architecture for the first revision of devices. > If not, then yes, this is probably overkill compared to what needs to > be done. > I suggested this approach to follow something like Chrome-OS EC based hub, but looks like in longer run this may come from firmware. That's why they may have decided. Thanks, Srinivas > Sandeep, can you explain to us why you think using HID is the best > way? > > On a side note, I don't necessarily like patch 4/5 with the debugfs > interface. It's adding a kernel API for no gain, and we should > already > have the debug API available in the various subsystems involved. > > Cheers, > Benjamin > > > > > > Regards, > > > > Hans > > > > > > *) One for accel, gyra, magneto and light each > > > > > > > Sandeep Singh (5): > > > SFH: Add maintainers and documentation for AMD SFH based on > > > HID > > > framework > > > SFH: PCI driver to add support of AMD sensor fusion Hub using > > > HID > > > framework > > > SFH: Transport Driver to add support of AMD Sensor Fusion Hub > > > (SFH) > > > SFH: Add debugfs support to AMD Sensor Fusion Hub > > > SFH: Create HID report to Enable support of AMD sensor fusion > > > Hub > > > (SFH) > > > > > > Changes since v1: > > > -Fix auto build test warnings > > > -Fix warnings captured using smatch > > > -Changes suggested by Dan Carpenter > > > > > > Links of the review comments for v1: > > > [1] https://patchwork.kernel.org/patch/11325163/ > > > [2] https://patchwork.kernel.org/patch/11325167/ > > > [3] https://patchwork.kernel.org/patch/11325171/ > > > [4] https://patchwork.kernel.org/patch/11325187/ > > > > > > > > > Changes since v2: > > > -Debugfs divided into another patch > > > -Fix some cosmetic changes > > > -Fix for review comments > > > Reported and Suggested by:- Srinivas Pandruvada > > > > > > Links of the review comments for v2: > > > [1] https://patchwork.kernel.org/patch/11355491/ > > > [2] https://patchwork.kernel.org/patch/11355495/ > > > [3] https://patchwork.kernel.org/patch/11355499/ > > > [4] https://patchwork.kernel.org/patch/11355503/ > > > > > > > > > Documentation/hid/amd-sfh-hid.rst | 160 +++++ > > > MAINTAINERS | 8 + > > > drivers/hid/Kconfig | 2 + > > > drivers/hid/Makefile | 1 + > > > drivers/hid/amd-sfh-hid/Kconfig | 20 + > > > drivers/hid/amd-sfh-hid/Makefile | 17 + > > > drivers/hid/amd-sfh-hid/amd_mp2_pcie.c | 243 > > > ++++++++ > > > drivers/hid/amd-sfh-hid/amd_mp2_pcie.h | 177 ++++++ > > > drivers/hid/amd-sfh-hid/amdsfh-debugfs.c | 250 > > > ++++++++ > > > drivers/hid/amd-sfh-hid/amdsfh-debugfs.h | 14 + > > > drivers/hid/amd-sfh-hid/amdsfh-hid-client.c | 260 > > > +++++++++ > > > drivers/hid/amd-sfh-hid/amdsfh-hid.c | 179 ++++++ > > > drivers/hid/amd-sfh-hid/amdsfh-hid.h | 85 +++ > > > .../hid_descriptor/amd_sfh_hid_descriptor.c | 275 > > > +++++++++ > > > .../hid_descriptor/amd_sfh_hid_descriptor.h | 125 ++++ > > > .../hid_descriptor/amd_sfh_hid_report_descriptor.h | 642 > > > +++++++++++++++++++++ > > > 16 files changed, 2458 insertions(+) > > > create mode 100644 Documentation/hid/amd-sfh-hid.rst > > > create mode 100644 drivers/hid/amd-sfh-hid/Kconfig > > > create mode 100644 drivers/hid/amd-sfh-hid/Makefile > > > create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.c > > > create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.h > > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.c > > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.h > > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid-client.c > > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.c > > > create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.h > > > create mode 100644 drivers/hid/amd-sfh- > > > hid/hid_descriptor/amd_sfh_hid_descriptor.c > > > create mode 100644 drivers/hid/amd-sfh- > > > hid/hid_descriptor/amd_sfh_hid_descriptor.h > > > create mode 100644 drivers/hid/amd-sfh- > > > hid/hid_descriptor/amd_sfh_hid_report_descriptor.h > > >
Hi On 2/14/2020 10:10 AM, Srinivas Pandruvada wrote: > On Thu, 2020-02-13 at 15:56 +0100, Benjamin Tissoires wrote: >> Hi, >> >> On Wed, Feb 12, 2020 at 3:45 PM Hans de Goede <hdegoede@redhat.com> >> wrote: >>> Hi, >>> >>> On 2/12/20 3:56 AM, Sandeep Singh wrote: >>>> From: Sandeep Singh <sandeep.singh@amd.com> >>>> >>>> AMD SFH(Sensor Fusion Hub) is HID based driver.SFH FW >>>> is part of MP2 processor (MP2 which is an ARM® Cortex-M4 >>>> core based co-processor to x86) and it runs on MP2 where >>>> in driver resides on X86.The driver functionalities are >>>> divided into three parts:- >>>> >>>> 1: amd-mp2-pcie:- This module will communicate with MP2 FW >>>> and >>>> provide that data into DRAM. >>>> 2: Client driver :- This part for driver will use dram data >>>> and >>>> convert that data into HID format based >>>> on >>>> HID reports. >>>> 3: Transport driver :- This part of driver will communicate with >>>> HID core. Communication between devices >>>> and >>>> HID core is mostly done via HID reports >>>> >>>> In terms of architecture it is much more reassembles like >>>> ISH(Intel Integrated Sensor Hub). However the major difference >>>> is all the hid reports are generated as part of kernel driver. >>>> AMD SFH driver taken reference from ISH in terms of >>>> design and functionalities at fewer location. >>>> >>>> AMD sensor fusion Hub is part of a SOC 17h family based >>>> platforms. >>>> The solution is working well on several OEM products. >>>> AMD SFH uses HID over PCIe bus. >>> I started looking at this patch because of the phoronix' news item >>> on it. >>> >>> First of all I want to say that it is great that AMD is working on >>> getting the Sensor Fusion Hub supported on Linux and that you are >>> working on a driver for this. Thanks for the valuable input. >> But, I've taken a quick look, mainly at the >> "[PATCH v3 5/5] SFH: Create HID report to Enable support of AMD >> sensor fusion Hub (SFH)" >> patch. >> >> AFAIK with the Intel ISH the sensor-hub itself is actually >> providing >> HID descriptors and HID input reports. >> >> Looking at the AMD code, that does not seem to be the case, it >> seems >> the values come directly from the AMD sensor-hub without being in >> any >> HID specific form, e.g.: >> >> +u8 get_input_report(int sensor_idx, int report_id, >> + u8 *input_report, u32 *sensor_virt_addr) >> +{ >> + u8 report_size = 0; >> + struct accel3_input_report acc_input; >> + struct gyro_input_report gyro_input; >> + struct magno_input_report magno_input; >> + struct als_input_report als_input; >> + >> + if (!sensor_virt_addr || !input_report) >> + return report_size; >> + >> + switch (sensor_idx) { >> + case ACCEL_IDX: /* accel */ >> + acc_input.common_property.report_id = report_id; >> + acc_input.common_property.sensor_state = >> + HID_USAGE_SENSOR_STATE_READ >> Y_ENUM; >> + acc_input.common_property.event_type = >> + HID_USAGE_SENSOR_EVENT_DATA_UPDATED >> _ENUM; >> + acc_input.in_accel_x_value = >> (int)sensor_virt_addr[0] / >> + AMD_SFH_FIRMWARE_MU >> LTIPLIER; >> + acc_input.in_accel_y_value = >> (int)sensor_virt_addr[1] / >> + AMD_SFH_FIRMWARE_MU >> LTIPLIER; >> + acc_input.in_accel_z_value >> = (int)sensor_virt_addr[2] / >> + AMD_SFH_FIRMWARE_MU >> LTIPLIER; >> + memcpy(input_report, &acc_input, >> sizeof(acc_input)); >> + report_size = sizeof(acc_input); >> + break; >> >> And the descriptors are hardcoded in the driver so as to fake a HID >> device. >> >> So going through the HID subsystem seems like an unnecessary >> detour, >> which just makes things needlessly complex and harder to debug >> (and extend). >> >> The HID devices which the current patch-set is creating ultimately >> will result in a number of devices being created under >> >> /sys/bus/iio/devices >> >> And this are the devices which userspace uses to get the sensor >> data. >> >> IMHO instead of going through the HID subsys the AMD Sensor Fusion >> Hub >> driver should simply register 4 (*) iio-devices itself and directly >> pass the data through at the iio subsys level rather then going the >> long way around by creating a fake HID device which then gets >> attached to by the hid-sensor driver to ultimately create the same >> iio-devices. >> >> There are examples of e.g. various iio accel drivers under: >> drivers/iio/accel/ you could start with a simple driver supporting >> just the accelerometer bits and then extend things from there. >> >> Benjamin, Jiri, Jonathan, what is your take on this? >> Hard to say without knowing AMD roadmap for that. If they intend to >> have an ISH-like approach in the end with reports and descriptors >> provided by the firmwares, then it makes sense to keep this >> architecture for the first revision of devices. >> If not, then yes, this is probably overkill compared to what needs to >> be done. >> > I suggested this approach to follow something like Chrome-OS EC based > hub, but looks like in longer run this may come from firmware. That's > why they may have decided. > > Thanks, > Srinivas > > >> Sandeep, can you explain to us why you think using HID is the best >> way? >> >> On a side note, I don't necessarily like patch 4/5 with the debugfs >> interface. It's adding a kernel API for no gain, and we should >> already >> have the debug API available in the various subsystems involved. >> >> Cheers, >> Benjamin Yes today, the HID Reports are getting generated in driver. But, we would like to have HID based driver as we may go for HID based firmware in future . Hence keeping that in mind current AMD SFH design. So, kindly consider our design w.r.t HID for this patch series. For the debugfs part,currently it is really handy for us to debug raw values coming from firmware.But if guys feel that it is not necessary, we can remove it. Thanks Nehal Shah
Hi, On 2/14/20 11:04 AM, Shah, Nehal-bakulchandra wrote: > Hi > > On 2/14/2020 10:10 AM, Srinivas Pandruvada wrote: >> On Thu, 2020-02-13 at 15:56 +0100, Benjamin Tissoires wrote: >>> Hi, >>> >>> On Wed, Feb 12, 2020 at 3:45 PM Hans de Goede <hdegoede@redhat.com> >>> wrote: >>>> Hi, >>>> >>>> On 2/12/20 3:56 AM, Sandeep Singh wrote: >>>>> From: Sandeep Singh <sandeep.singh@amd.com> >>>>> >>>>> AMD SFH(Sensor Fusion Hub) is HID based driver.SFH FW >>>>> is part of MP2 processor (MP2 which is an ARM® Cortex-M4 >>>>> core based co-processor to x86) and it runs on MP2 where >>>>> in driver resides on X86.The driver functionalities are >>>>> divided into three parts:- >>>>> >>>>> 1: amd-mp2-pcie:- This module will communicate with MP2 FW >>>>> and >>>>> provide that data into DRAM. >>>>> 2: Client driver :- This part for driver will use dram data >>>>> and >>>>> convert that data into HID format based >>>>> on >>>>> HID reports. >>>>> 3: Transport driver :- This part of driver will communicate with >>>>> HID core. Communication between devices >>>>> and >>>>> HID core is mostly done via HID reports >>>>> >>>>> In terms of architecture it is much more reassembles like >>>>> ISH(Intel Integrated Sensor Hub). However the major difference >>>>> is all the hid reports are generated as part of kernel driver. >>>>> AMD SFH driver taken reference from ISH in terms of >>>>> design and functionalities at fewer location. >>>>> >>>>> AMD sensor fusion Hub is part of a SOC 17h family based >>>>> platforms. >>>>> The solution is working well on several OEM products. >>>>> AMD SFH uses HID over PCIe bus. >>>> I started looking at this patch because of the phoronix' news item >>>> on it. >>>> >>>> First of all I want to say that it is great that AMD is working on >>>> getting the Sensor Fusion Hub supported on Linux and that you are >>>> working on a driver for this. > Thanks for the valuable input. >>> But, I've taken a quick look, mainly at the >>> "[PATCH v3 5/5] SFH: Create HID report to Enable support of AMD >>> sensor fusion Hub (SFH)" >>> patch. >>> >>> AFAIK with the Intel ISH the sensor-hub itself is actually >>> providing >>> HID descriptors and HID input reports. >>> >>> Looking at the AMD code, that does not seem to be the case, it >>> seems >>> the values come directly from the AMD sensor-hub without being in >>> any >>> HID specific form, e.g.: >>> >>> +u8 get_input_report(int sensor_idx, int report_id, >>> + u8 *input_report, u32 *sensor_virt_addr) >>> +{ >>> + u8 report_size = 0; >>> + struct accel3_input_report acc_input; >>> + struct gyro_input_report gyro_input; >>> + struct magno_input_report magno_input; >>> + struct als_input_report als_input; >>> + >>> + if (!sensor_virt_addr || !input_report) >>> + return report_size; >>> + >>> + switch (sensor_idx) { >>> + case ACCEL_IDX: /* accel */ >>> + acc_input.common_property.report_id = report_id; >>> + acc_input.common_property.sensor_state = >>> + HID_USAGE_SENSOR_STATE_READ >>> Y_ENUM; >>> + acc_input.common_property.event_type = >>> + HID_USAGE_SENSOR_EVENT_DATA_UPDATED >>> _ENUM; >>> + acc_input.in_accel_x_value = >>> (int)sensor_virt_addr[0] / >>> + AMD_SFH_FIRMWARE_MU >>> LTIPLIER; >>> + acc_input.in_accel_y_value = >>> (int)sensor_virt_addr[1] / >>> + AMD_SFH_FIRMWARE_MU >>> LTIPLIER; >>> + acc_input.in_accel_z_value >>> = (int)sensor_virt_addr[2] / >>> + AMD_SFH_FIRMWARE_MU >>> LTIPLIER; >>> + memcpy(input_report, &acc_input, >>> sizeof(acc_input)); >>> + report_size = sizeof(acc_input); >>> + break; >>> >>> And the descriptors are hardcoded in the driver so as to fake a HID >>> device. >>> >>> So going through the HID subsystem seems like an unnecessary >>> detour, >>> which just makes things needlessly complex and harder to debug >>> (and extend). >>> >>> The HID devices which the current patch-set is creating ultimately >>> will result in a number of devices being created under >>> >>> /sys/bus/iio/devices >>> >>> And this are the devices which userspace uses to get the sensor >>> data. >>> >>> IMHO instead of going through the HID subsys the AMD Sensor Fusion >>> Hub >>> driver should simply register 4 (*) iio-devices itself and directly >>> pass the data through at the iio subsys level rather then going the >>> long way around by creating a fake HID device which then gets >>> attached to by the hid-sensor driver to ultimately create the same >>> iio-devices. >>> >>> There are examples of e.g. various iio accel drivers under: >>> drivers/iio/accel/ you could start with a simple driver supporting >>> just the accelerometer bits and then extend things from there. >>> >>> Benjamin, Jiri, Jonathan, what is your take on this? >>> Hard to say without knowing AMD roadmap for that. If they intend to >>> have an ISH-like approach in the end with reports and descriptors >>> provided by the firmwares, then it makes sense to keep this >>> architecture for the first revision of devices. >>> If not, then yes, this is probably overkill compared to what needs to >>> be done. >>> >> I suggested this approach to follow something like Chrome-OS EC based >> hub, but looks like in longer run this may come from firmware. That's >> why they may have decided. >> >> Thanks, >> Srinivas >> >> >>> Sandeep, can you explain to us why you think using HID is the best >>> way? >>> >>> On a side note, I don't necessarily like patch 4/5 with the debugfs >>> interface. It's adding a kernel API for no gain, and we should >>> already >>> have the debug API available in the various subsystems involved. >>> >>> Cheers, >>> Benjamin > > Yes today, the HID Reports are getting generated in driver. But, we would like to have HID based driver as we may go for HID based firmware in future . Hence keeping that in mind current AMD SFH design. > > So, kindly consider our design w.r.t HID for this patch series. If the plan is to move to using HID in the future and the HID maintainers (Benjamin and Jiri) are ok with the current approach then I'm fine with the current approach too. Regards, Hans
On Fri, Feb 14, 2020 at 11:04 AM Shah, Nehal-bakulchandra <nehal-bakulchandra.shah@amd.com> wrote: > > Hi > > On 2/14/2020 10:10 AM, Srinivas Pandruvada wrote: > > On Thu, 2020-02-13 at 15:56 +0100, Benjamin Tissoires wrote: > >> Hi, > >> > >> On Wed, Feb 12, 2020 at 3:45 PM Hans de Goede <hdegoede@redhat.com> > >> wrote: > >>> Hi, > >>> > >>> On 2/12/20 3:56 AM, Sandeep Singh wrote: > >>>> From: Sandeep Singh <sandeep.singh@amd.com> > >>>> > >>>> AMD SFH(Sensor Fusion Hub) is HID based driver.SFH FW > >>>> is part of MP2 processor (MP2 which is an ARM® Cortex-M4 > >>>> core based co-processor to x86) and it runs on MP2 where > >>>> in driver resides on X86.The driver functionalities are > >>>> divided into three parts:- > >>>> > >>>> 1: amd-mp2-pcie:- This module will communicate with MP2 FW > >>>> and > >>>> provide that data into DRAM. > >>>> 2: Client driver :- This part for driver will use dram data > >>>> and > >>>> convert that data into HID format based > >>>> on > >>>> HID reports. > >>>> 3: Transport driver :- This part of driver will communicate with > >>>> HID core. Communication between devices > >>>> and > >>>> HID core is mostly done via HID reports > >>>> > >>>> In terms of architecture it is much more reassembles like > >>>> ISH(Intel Integrated Sensor Hub). However the major difference > >>>> is all the hid reports are generated as part of kernel driver. > >>>> AMD SFH driver taken reference from ISH in terms of > >>>> design and functionalities at fewer location. > >>>> > >>>> AMD sensor fusion Hub is part of a SOC 17h family based > >>>> platforms. > >>>> The solution is working well on several OEM products. > >>>> AMD SFH uses HID over PCIe bus. > >>> I started looking at this patch because of the phoronix' news item > >>> on it. > >>> > >>> First of all I want to say that it is great that AMD is working on > >>> getting the Sensor Fusion Hub supported on Linux and that you are > >>> working on a driver for this. > Thanks for the valuable input. > >> But, I've taken a quick look, mainly at the > >> "[PATCH v3 5/5] SFH: Create HID report to Enable support of AMD > >> sensor fusion Hub (SFH)" > >> patch. > >> > >> AFAIK with the Intel ISH the sensor-hub itself is actually > >> providing > >> HID descriptors and HID input reports. > >> > >> Looking at the AMD code, that does not seem to be the case, it > >> seems > >> the values come directly from the AMD sensor-hub without being in > >> any > >> HID specific form, e.g.: > >> > >> +u8 get_input_report(int sensor_idx, int report_id, > >> + u8 *input_report, u32 *sensor_virt_addr) > >> +{ > >> + u8 report_size = 0; > >> + struct accel3_input_report acc_input; > >> + struct gyro_input_report gyro_input; > >> + struct magno_input_report magno_input; > >> + struct als_input_report als_input; > >> + > >> + if (!sensor_virt_addr || !input_report) > >> + return report_size; > >> + > >> + switch (sensor_idx) { > >> + case ACCEL_IDX: /* accel */ > >> + acc_input.common_property.report_id = report_id; > >> + acc_input.common_property.sensor_state = > >> + HID_USAGE_SENSOR_STATE_READ > >> Y_ENUM; > >> + acc_input.common_property.event_type = > >> + HID_USAGE_SENSOR_EVENT_DATA_UPDATED > >> _ENUM; > >> + acc_input.in_accel_x_value = > >> (int)sensor_virt_addr[0] / > >> + AMD_SFH_FIRMWARE_MU > >> LTIPLIER; > >> + acc_input.in_accel_y_value = > >> (int)sensor_virt_addr[1] / > >> + AMD_SFH_FIRMWARE_MU > >> LTIPLIER; > >> + acc_input.in_accel_z_value > >> = (int)sensor_virt_addr[2] / > >> + AMD_SFH_FIRMWARE_MU > >> LTIPLIER; > >> + memcpy(input_report, &acc_input, > >> sizeof(acc_input)); > >> + report_size = sizeof(acc_input); > >> + break; > >> > >> And the descriptors are hardcoded in the driver so as to fake a HID > >> device. > >> > >> So going through the HID subsystem seems like an unnecessary > >> detour, > >> which just makes things needlessly complex and harder to debug > >> (and extend). > >> > >> The HID devices which the current patch-set is creating ultimately > >> will result in a number of devices being created under > >> > >> /sys/bus/iio/devices > >> > >> And this are the devices which userspace uses to get the sensor > >> data. > >> > >> IMHO instead of going through the HID subsys the AMD Sensor Fusion > >> Hub > >> driver should simply register 4 (*) iio-devices itself and directly > >> pass the data through at the iio subsys level rather then going the > >> long way around by creating a fake HID device which then gets > >> attached to by the hid-sensor driver to ultimately create the same > >> iio-devices. > >> > >> There are examples of e.g. various iio accel drivers under: > >> drivers/iio/accel/ you could start with a simple driver supporting > >> just the accelerometer bits and then extend things from there. > >> > >> Benjamin, Jiri, Jonathan, what is your take on this? > >> Hard to say without knowing AMD roadmap for that. If they intend to > >> have an ISH-like approach in the end with reports and descriptors > >> provided by the firmwares, then it makes sense to keep this > >> architecture for the first revision of devices. > >> If not, then yes, this is probably overkill compared to what needs to > >> be done. > >> > > I suggested this approach to follow something like Chrome-OS EC based > > hub, but looks like in longer run this may come from firmware. That's > > why they may have decided. > > > > Thanks, > > Srinivas > > > > > >> Sandeep, can you explain to us why you think using HID is the best > >> way? > >> > >> On a side note, I don't necessarily like patch 4/5 with the debugfs > >> interface. It's adding a kernel API for no gain, and we should > >> already > >> have the debug API available in the various subsystems involved. > >> > >> Cheers, > >> Benjamin > > Yes today, the HID Reports are getting generated in driver. But, we would like to have HID based driver as we may go for HID based firmware in future . Hence keeping that in mind current AMD SFH design. > > So, kindly consider our design w.r.t HID for this patch series. OK, that's good enough for me. Jiri, are you fine with that too? > > For the debugfs part,currently it is really handy for us to debug raw values coming from firmware.But if guys feel that it is not necessary, we can remove it. > 2 problems here: - patch 3/5 references this debugfs interface which is only added in 4/5. - you are creating a new sysfs set of file for debug purpose only, but as soon as we start shipping those, some other people will find it more convenient to use that directly instead or IIO, and you won't be able to change anything there. So I would strongly advocate against having this debugfs, and suggest you to: - either keep this debugfs as a downstream patch - either play with eBPF or kprobes to retrieve the same information without changing the kernel. For reference, I recently tried to replicate the hidraw functionality with eBPF[0] without changing the kernel code, and it was working. Well, there was no filtering on the source of the HID event, but still, I got the same data directly from the kernel just by adding instrumentation in a couple of functions. Cheers, Benjamin [0] https://gitlab.freedesktop.org/bentiss/hid-tools/snippets/875
Hi On 2/14/2020 4:41 PM, Benjamin Tissoires wrote: > On Fri, Feb 14, 2020 at 11:04 AM Shah, Nehal-bakulchandra > <nehal-bakulchandra.shah@amd.com> wrote: >> Hi >> >> On 2/14/2020 10:10 AM, Srinivas Pandruvada wrote: >>> On Thu, 2020-02-13 at 15:56 +0100, Benjamin Tissoires wrote: >>>> Hi, >>>> >>>> On Wed, Feb 12, 2020 at 3:45 PM Hans de Goede <hdegoede@redhat.com> >>>> wrote: >>>>> Hi, >>>>> >>>>> On 2/12/20 3:56 AM, Sandeep Singh wrote: >>>>>> From: Sandeep Singh <sandeep.singh@amd.com> >>>>>> >>>>>> AMD SFH(Sensor Fusion Hub) is HID based driver.SFH FW >>>>>> is part of MP2 processor (MP2 which is an ARM® Cortex-M4 >>>>>> core based co-processor to x86) and it runs on MP2 where >>>>>> in driver resides on X86.The driver functionalities are >>>>>> divided into three parts:- >>>>>> >>>>>> 1: amd-mp2-pcie:- This module will communicate with MP2 FW >>>>>> and >>>>>> provide that data into DRAM. >>>>>> 2: Client driver :- This part for driver will use dram data >>>>>> and >>>>>> convert that data into HID format based >>>>>> on >>>>>> HID reports. >>>>>> 3: Transport driver :- This part of driver will communicate with >>>>>> HID core. Communication between devices >>>>>> and >>>>>> HID core is mostly done via HID reports >>>>>> >>>>>> In terms of architecture it is much more reassembles like >>>>>> ISH(Intel Integrated Sensor Hub). However the major difference >>>>>> is all the hid reports are generated as part of kernel driver. >>>>>> AMD SFH driver taken reference from ISH in terms of >>>>>> design and functionalities at fewer location. >>>>>> >>>>>> AMD sensor fusion Hub is part of a SOC 17h family based >>>>>> platforms. >>>>>> The solution is working well on several OEM products. >>>>>> AMD SFH uses HID over PCIe bus. >>>>> I started looking at this patch because of the phoronix' news item >>>>> on it. >>>>> >>>>> First of all I want to say that it is great that AMD is working on >>>>> getting the Sensor Fusion Hub supported on Linux and that you are >>>>> working on a driver for this. >> Thanks for the valuable input. >>>> But, I've taken a quick look, mainly at the >>>> "[PATCH v3 5/5] SFH: Create HID report to Enable support of AMD >>>> sensor fusion Hub (SFH)" >>>> patch. >>>> >>>> AFAIK with the Intel ISH the sensor-hub itself is actually >>>> providing >>>> HID descriptors and HID input reports. >>>> >>>> Looking at the AMD code, that does not seem to be the case, it >>>> seems >>>> the values come directly from the AMD sensor-hub without being in >>>> any >>>> HID specific form, e.g.: >>>> >>>> +u8 get_input_report(int sensor_idx, int report_id, >>>> + u8 *input_report, u32 *sensor_virt_addr) >>>> +{ >>>> + u8 report_size = 0; >>>> + struct accel3_input_report acc_input; >>>> + struct gyro_input_report gyro_input; >>>> + struct magno_input_report magno_input; >>>> + struct als_input_report als_input; >>>> + >>>> + if (!sensor_virt_addr || !input_report) >>>> + return report_size; >>>> + >>>> + switch (sensor_idx) { >>>> + case ACCEL_IDX: /* accel */ >>>> + acc_input.common_property.report_id = report_id; >>>> + acc_input.common_property.sensor_state = >>>> + HID_USAGE_SENSOR_STATE_READ >>>> Y_ENUM; >>>> + acc_input.common_property.event_type = >>>> + HID_USAGE_SENSOR_EVENT_DATA_UPDATED >>>> _ENUM; >>>> + acc_input.in_accel_x_value = >>>> (int)sensor_virt_addr[0] / >>>> + AMD_SFH_FIRMWARE_MU >>>> LTIPLIER; >>>> + acc_input.in_accel_y_value = >>>> (int)sensor_virt_addr[1] / >>>> + AMD_SFH_FIRMWARE_MU >>>> LTIPLIER; >>>> + acc_input.in_accel_z_value >>>> = (int)sensor_virt_addr[2] / >>>> + AMD_SFH_FIRMWARE_MU >>>> LTIPLIER; >>>> + memcpy(input_report, &acc_input, >>>> sizeof(acc_input)); >>>> + report_size = sizeof(acc_input); >>>> + break; >>>> >>>> And the descriptors are hardcoded in the driver so as to fake a HID >>>> device. >>>> >>>> So going through the HID subsystem seems like an unnecessary >>>> detour, >>>> which just makes things needlessly complex and harder to debug >>>> (and extend). >>>> >>>> The HID devices which the current patch-set is creating ultimately >>>> will result in a number of devices being created under >>>> >>>> /sys/bus/iio/devices >>>> >>>> And this are the devices which userspace uses to get the sensor >>>> data. >>>> >>>> IMHO instead of going through the HID subsys the AMD Sensor Fusion >>>> Hub >>>> driver should simply register 4 (*) iio-devices itself and directly >>>> pass the data through at the iio subsys level rather then going the >>>> long way around by creating a fake HID device which then gets >>>> attached to by the hid-sensor driver to ultimately create the same >>>> iio-devices. >>>> >>>> There are examples of e.g. various iio accel drivers under: >>>> drivers/iio/accel/ you could start with a simple driver supporting >>>> just the accelerometer bits and then extend things from there. >>>> >>>> Benjamin, Jiri, Jonathan, what is your take on this? >>>> Hard to say without knowing AMD roadmap for that. If they intend to >>>> have an ISH-like approach in the end with reports and descriptors >>>> provided by the firmwares, then it makes sense to keep this >>>> architecture for the first revision of devices. >>>> If not, then yes, this is probably overkill compared to what needs to >>>> be done. >>>> >>> I suggested this approach to follow something like Chrome-OS EC based >>> hub, but looks like in longer run this may come from firmware. That's >>> why they may have decided. >>> >>> Thanks, >>> Srinivas >>> >>> >>>> Sandeep, can you explain to us why you think using HID is the best >>>> way? >>>> >>>> On a side note, I don't necessarily like patch 4/5 with the debugfs >>>> interface. It's adding a kernel API for no gain, and we should >>>> already >>>> have the debug API available in the various subsystems involved. >>>> >>>> Cheers, >>>> Benjamin >> Yes today, the HID Reports are getting generated in driver. But, we would like to have HID based driver as we may go for HID based firmware in future . Hence keeping that in mind current AMD SFH design. >> >> So, kindly consider our design w.r.t HID for this patch series. > OK, that's good enough for me. Jiri, are you fine with that too? > >> For the debugfs part,currently it is really handy for us to debug raw values coming from firmware.But if guys feel that it is not necessary, we can remove it. >> > 2 problems here: > - patch 3/5 references this debugfs interface which is only added in 4/5. > - you are creating a new sysfs set of file for debug purpose only, but > as soon as we start shipping those, some other people will find it > more convenient to use that directly instead or IIO, and you won't be > able to change anything there. > > So I would strongly advocate against having this debugfs, and suggest you to: > - either keep this debugfs as a downstream patch > - either play with eBPF or kprobes to retrieve the same information > without changing the kernel. > > For reference, I recently tried to replicate the hidraw functionality > with eBPF[0] without changing the kernel code, and it was working. > Well, there was no filtering on the source of the HID event, but > still, I got the same data directly from the kernel just by adding > instrumentation in a couple of functions. > > Cheers, > Benjamin If Jiri is Ok, then we will push the next patch as per suggestion here i.e. taking out debugfs. Thanks Nehal Shah >
On Wed, 19 Feb 2020, Shah, Nehal-bakulchandra wrote: > > 2 problems here: > > - patch 3/5 references this debugfs interface which is only added in 4/5. > > - you are creating a new sysfs set of file for debug purpose only, but > > as soon as we start shipping those, some other people will find it > > more convenient to use that directly instead or IIO, and you won't be > > able to change anything there. > > > > So I would strongly advocate against having this debugfs, and suggest you to: > > - either keep this debugfs as a downstream patch > > - either play with eBPF or kprobes to retrieve the same information > > without changing the kernel. > > > > For reference, I recently tried to replicate the hidraw functionality > > with eBPF[0] without changing the kernel code, and it was working. > > Well, there was no filtering on the source of the HID event, but > > still, I got the same data directly from the kernel just by adding > > instrumentation in a couple of functions. Wow. Complete eBPF supremacy is really coming shortly apparently :) > If Jiri is Ok, then we will push the next patch as per suggestion here > i.e. taking out debugfs. I agree with that. Thanks,
From: Sandeep Singh <sandeep.singh@amd.com> AMD SFH(Sensor Fusion Hub) is HID based driver.SFH FW is part of MP2 processor (MP2 which is an ARM® Cortex-M4 core based co-processor to x86) and it runs on MP2 where in driver resides on X86.The driver functionalities are divided into three parts:- 1: amd-mp2-pcie:- This module will communicate with MP2 FW and provide that data into DRAM. 2: Client driver :- This part for driver will use dram data and convert that data into HID format based on HID reports. 3: Transport driver :- This part of driver will communicate with HID core. Communication between devices and HID core is mostly done via HID reports In terms of architecture it is much more reassembles like ISH(Intel Integrated Sensor Hub). However the major difference is all the hid reports are generated as part of kernel driver. AMD SFH driver taken reference from ISH in terms of design and functionalities at fewer location. AMD sensor fusion Hub is part of a SOC 17h family based platforms. The solution is working well on several OEM products. AMD SFH uses HID over PCIe bus. Sandeep Singh (5): SFH: Add maintainers and documentation for AMD SFH based on HID framework SFH: PCI driver to add support of AMD sensor fusion Hub using HID framework SFH: Transport Driver to add support of AMD Sensor Fusion Hub (SFH) SFH: Add debugfs support to AMD Sensor Fusion Hub SFH: Create HID report to Enable support of AMD sensor fusion Hub (SFH) Changes since v1: -Fix auto build test warnings -Fix warnings captured using smatch -Changes suggested by Dan Carpenter Links of the review comments for v1: [1] https://patchwork.kernel.org/patch/11325163/ [2] https://patchwork.kernel.org/patch/11325167/ [3] https://patchwork.kernel.org/patch/11325171/ [4] https://patchwork.kernel.org/patch/11325187/ Changes since v2: -Debugfs divided into another patch -Fix some cosmetic changes -Fix for review comments Reported and Suggested by:- Srinivas Pandruvada Links of the review comments for v2: [1] https://patchwork.kernel.org/patch/11355491/ [2] https://patchwork.kernel.org/patch/11355495/ [3] https://patchwork.kernel.org/patch/11355499/ [4] https://patchwork.kernel.org/patch/11355503/ Documentation/hid/amd-sfh-hid.rst | 160 +++++ MAINTAINERS | 8 + drivers/hid/Kconfig | 2 + drivers/hid/Makefile | 1 + drivers/hid/amd-sfh-hid/Kconfig | 20 + drivers/hid/amd-sfh-hid/Makefile | 17 + drivers/hid/amd-sfh-hid/amd_mp2_pcie.c | 243 ++++++++ drivers/hid/amd-sfh-hid/amd_mp2_pcie.h | 177 ++++++ drivers/hid/amd-sfh-hid/amdsfh-debugfs.c | 250 ++++++++ drivers/hid/amd-sfh-hid/amdsfh-debugfs.h | 14 + drivers/hid/amd-sfh-hid/amdsfh-hid-client.c | 260 +++++++++ drivers/hid/amd-sfh-hid/amdsfh-hid.c | 179 ++++++ drivers/hid/amd-sfh-hid/amdsfh-hid.h | 85 +++ .../hid_descriptor/amd_sfh_hid_descriptor.c | 275 +++++++++ .../hid_descriptor/amd_sfh_hid_descriptor.h | 125 ++++ .../hid_descriptor/amd_sfh_hid_report_descriptor.h | 642 +++++++++++++++++++++ 16 files changed, 2458 insertions(+) create mode 100644 Documentation/hid/amd-sfh-hid.rst create mode 100644 drivers/hid/amd-sfh-hid/Kconfig create mode 100644 drivers/hid/amd-sfh-hid/Makefile create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.c create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.h create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.c create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.h create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid-client.c create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.c create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.h create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_descriptor.c create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_descriptor.h create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_descriptor.h