Re: [PATCH v7 1/4] SFH: Add maintainers and documentation for AMD SFH based on HID framework

From: Randy Dunlap
Date: Mon Aug 10 2020 - 19:23:50 EST

On 8/10/20 2:30 PM, Sandeep Singh wrote:
> From: Sandeep Singh <sandeep.singh@xxxxxxx>
>
> Add Maintainers for AMD SFH(SENSOR FUSION HUB) Solution and work flow
> document.
>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@xxxxxxx>
> Signed-off-by: Sandeep Singh <sandeep.singh@xxxxxxx>

Hi,
Did you build the documentation?

Oh, I think you would need to add amd-sfh-hid to
Documentation/hid/index.rst first in order to build the documentation.
That change should be part of your patch set.


So please do that and then run "make htmldocs" and look for warnings or errors.

More below:

> ---
> Documentation/hid/amd-sfh-hid.rst | 153 ++++++++++++++++++++++++++++++
> MAINTAINERS | 8 ++
> 2 files changed, 161 insertions(+)
> create mode 100644 Documentation/hid/amd-sfh-hid.rst
>
> diff --git a/Documentation/hid/amd-sfh-hid.rst b/Documentation/hid/amd-sfh-hid.rst
> new file mode 100644
> index 000000000000..0ee9003013f2
> --- /dev/null
> +++ b/Documentation/hid/amd-sfh-hid.rst
> @@ -0,0 +1,153 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +AMD Sensor Fusion Hub:-
> +======================

Underlines (like above) should be at least as long as the heading line,
but heading lines don't need endings on them like ':' or '-', so please
remove those ending characters on all heading lines.


> +AMD Sensor Fusion Hub is part of a SOC starting from Ryzen based platforms.

an SOC

> +The solution is working well on several OEM products. AMD SFH uses HID over PCIe bus.

I would drop that first sentence above.

> +In terms of architecture it resembles ISH .However the major difference is all

resembles ISH. However

> +The hid reports are generated as part of kernel driver.

the HID

> +
> +Block Diagram:-
> +=============
> + -------------------------------
> + | HID User Space Applications |
> + -------------------------------
> +---------------------------------------------
> + ---------------------------------
> + | HID Core |
> + ---------------------------------
> +
> + ---------------------------------
> + | AMD HID Transport |
> + ---------------------------------
> +
> + --------------------------------
> + | AMD HID Client |
> + | with HID Report Generator|
> + --------------------------------
> +
> + --------------------------------
> + | AMD MP2 PCIe Driver |
> + --------------------------------
> +---------------------------------------------
> + -------------------------------
> + | SFH MP2 Processor |
> + --------------------------------
> +
> +
> +AMD HID Transport Layer :-
> +***************************
> +AMD SFH transport is also implemented as a bus. Each client application executing in the AMD MP2
> +is registered as a device on this bus. MP2 which is an ARM® Cortex-M4 core based co-processor to

Incomplete sentence here: MP2 ....

> +x86. The driver, which binds each device (AMD SFH HID driver) identifies the device type and
> +registers with the hid core. Transport drivers attach a constant "struct hid_ll_driver" object with

HID

> +each device. Once a device is registered with HID core, the callbacks provided via this struct are
> +used by HID core to communicate with the device. AMD HID Transport driver implements the synchronouscalls.

implements synchronous calls.

> +
> +AMD HID Client Layer:-
> +**********************
> +This layer is responsible to implement HID request and descriptors. As firmware is OS agnostic,
> +HID client layer fills the HID request structure and descriptors. HID client layer is in complex

is complex

> +in nature as it is interface between MP2 PCIe driver and HID. HID client layer initialized

initializes
I would drop "in nature".

> +the MP2 PCIe driver and holds the instance of MP2 driver.It identified the number of sensors

driver. It identifies

> +connected using MP2-PCIe driver and based on that allocate the DRAM address for each and every

allocates

> +sensor and pass it to MP2-PCIe driver.On enumeration of each sensor, client layer fills out the HID

passes driver. On

> +Descriptor structure and HID input report structure. HID Feature report structure can be optional.

structure is optional.

> +The report descriptor structure varies sensor to sensor. Now on enumeration client layer does
> +two major things

things:
(at least for punctuation; I don't know if that's sufficient for ReST markup.)

> +1. Register the HID sensor client to virtual bus (Platform driver) and bind it.
> +2. Probes the AMD HID transport driver. Which in turns register device to the core.

driver, which in turn registers

> +
> +AMD MP2 PCIe layer:-
> +********************
> +MP2 PCIe Layer is responsible for making all transaction with the firmware over PCIe.

transactions

> +The connection establishment between firmware and PCIe happens here.
> +
> +The communication between X86 and MP2 is spilt into three parts.

split

> +1. Command Transfer => C2P Mailbox Register are used

Registers are used

> +2. Data Transfer => DRAM
> +3. Supported sensor info => P2C Register
> +
> +Commands are sent to MP2 using C2P Mail Box registers. These C2P registers are common between x86

Mailbox C2P registers X86

Use Mailbox and X86 as above for consistency.

> +and MP2. Writing into C2P Message register generate interrupt to MP2. The client layer allocates
> +the physical memory and send the same to MP2 for data transfer. MP2 firmware uses HUBIF interface

What is HUBIF interface? do we care?

> +to access DRAM memory. For Firmware always write minimum 32 bytes into DRAM.So it is expected that

firmware DRAM. So

> +driver shall allocate minimum 32 bytes DRAM space.
> +
> +Enumeration and Probing flow:-
> +*****************************
> + HID AMD AMD AMD -PCIe MP2
> + Core Transport Client layer layer FW
> + | | | | |
> + | | | on Boot Driver Loaded |
> + | | | | |
> + | | | MP2-PCIe Int |
> + | | | | |
> + | | |---Get Number of sensors-> | |
> + | | | Read P2C |
> + | | | Register |
> + | | | | |
> + | | | Loop(for No of Sensors) | |
> + | | |----------------------| | |
> + | | | Create HID Descriptor| | |
> + | | | Create Input report | | |
> + | | | Descriptor Map | | |
> + | | | the MP2 FW Index to | | |
> + | | | HID Index | | |
> + | | | Allocate the DRAM | Enable |
> + | | | address | Sensors |
> + | | |----------------------| | |
> + | | HID transport| | Enable |
> + | |<--Probe------| |---Sensor CMD--> |
> + | | Create the | | |
> + | | HID device | | |
> + | | (MFD) | | |
> + | | by Populating| | |
> + | | the HID | | |
> + | | ll_driver | | |
> + | HID | | | |
> + | add | | | |
> + |Device | | | |
> + |<------------- | | | |
> +
> +
> +Data Flow from Application to the AMD SFH Driver:-
> +*************************************************
> +
> + | | | | |
> +Get | | | | |
> +Input | | | | |
> +Report | | | | |
> +---> | | | | |
> + |HID_req | | | |
> + |get_report | | | |
> + |------------->| | | |
> + | | HID_get_input| | |
> + | | report | | |
> + | |------------->|------------------------| | |
> + | | | Read the DRAM data for| | |
> + | | | requested sensor and | | |
> + | | | create the HID input | | |
> + | | | report | | |
> + | | |------------------------| | |
> + | |Data received | | |
> + | | in HID report| | |
> + To |<-------------|<-------------| | |
> +Applications | | | |
> +<-------| | | | |
> +
> +
> +Data Flow from AMD SFH Driver to Application:-
> +**********************************************
> + | | | | |
> + | | |------------------------| | |
> + | | |Periodically Read | | |
> + | | |the data for all | | |
> + | | |enumerated sensors | | |
> + | | |from the dram and create| | |
> + | | | HID Input reports | | |
> + | | |------------------------| | |
> + | |HID Input | | |
> + | |Input report | | |
> + <----submit to Application-----| | |
> + | | | | |


thanks.
--
~Randy