Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v9 2/8] Updated documentation
To:     mail@richard-neumann.de, nehal-bakulchandra.shah@amd.com,
        sandeep.singh@amd.com, corbet@lwn.net, jikos@kernel.org,
        benjamin.tissoires@redhat.com, linux-input@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20210123134716.13414-1-mail@richard-neumann.de>
 <20210123134716.13414-3-mail@richard-neumann.de>
From:   Randy Dunlap <rdunlap@infradead.org>
Message-ID: <650cef6a-6e7c-8fe9-ef48-a0efacfcf9f8@infradead.org>
Date:   Thu, 28 Jan 2021 17:25:51 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <20210123134716.13414-3-mail@richard-neumann.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/23/21 5:47 AM, mail@richard-neumann.de wrote:
> From: Richard Neumann <mail@richard-neumann.de>
> 
> Updated documentation of the AMD Sensor Fusion Hub.
> 
> Signed-off-by: Richard Neumann <mail@richard-neumann.de>
> ---
>  Documentation/hid/amd-sfh-hid.rst | 275 ++++++++++++++----------------
>  1 file changed, 131 insertions(+), 144 deletions(-)
> 
> diff --git a/Documentation/hid/amd-sfh-hid.rst b/Documentation/hid/amd-sfh-hid.rst
> index 1f2fe29ccd4f..d68ba2b85d1e 100644
> --- a/Documentation/hid/amd-sfh-hid.rst
> +++ b/Documentation/hid/amd-sfh-hid.rst
> @@ -1,145 +1,132 @@

[snip deletions]

> +========================================
> +Kernel drivers: amd-sfh-pci, amd-sfh-hid
> +========================================
> +
> +Supported adapters:
> +  * AMD Sensor Fusion Hub PCIe interface
> +
> +Datasheet: not publicly available.
> +
> +Authors:
> +        - Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> +        - Nehal Bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
> +        - Sandeep Singh <sandeep.singh@amd.com>
> +        - Richard Neumann <mail@richard-neumann.de>
> +
> +Description
> +===========
> +The AMD Sensor Fushion Hub (SFH) is part of a SOC on Ryzen-based platforms.

                  Fusion

> +The SFH uses HID over PCIe bus. In terms of architecture it is much more

                                                            it resembles the

> +resmebles like ISH. However the major difference is, that currently HID reports

   ISH. However, the major difference is that currently HID reports

> +are being generated within the kernel driver.
> +
> +Block Diagram
> +-------------
> +.. code-block:: none
> +
> +    +-------------------------------+
> +    |  HID User Space Applications  |
> +    +-------------------------------+
> +    =================================
> +    +-------------------------------+
> +    |      HID low-level driver     |
> +    |   with HID report generator   |
> +    +-------------------------------+
> +
> +    +-------------------------------+
> +    |      HID platform driver      |
> +    +-------------------------------+
> +
> +    +-------------------------------+
> +    |      AMD SFH PCIe driver      |
> +    +-------------------------------+
> +    =================================
> +    +-------------------------------+
> +    |       SFH MP2 Processor       |
> +    +-------------------------------+
> +
> +HID low-level driver
>  --------------------

[snip deletions]

> +The driver is conceived in a multi-layer architecture.
> +The level closest to the applications is the HID low-level (LL) driver,
> +which implements the functions defined by the hid-core API to manage the
> +respective HID devices and process reports.
> +Therefor, the HID-LL-driver starts and stops the sensors as needed by invoking
> +the exposed functions from the PCI driver (see below) and creates DMA mappings
> +to access the DRAM of the PCI device to retrieve feature and input reports
> +from it.
> +
> +HID platform driver (`amd-sfh-hid`)
> +-----------------------------------
> +The aforementioned HID devices are being managed, i.e. created on probing and
> +destroyed on removing, by the platform driver. It is being loaded through ACPI

                           better:                It is loaded through ACPI

> +table matching if the PCI driver was loaded successfully.
> +It determines the HID devices to be created on startup using the connected
> +sensors bitmask retrieved by invoking the respective function of the PCI driver.
> +On some systems the firmware does not provide the information about sensors
> +connected to the SFH device. In this case, the detected sensors can be manually
> +overridden by setting the driver's module parameter `sensor_mask=<int>`.
> +
> +PCI device driver (`amd-sfh-pci`)
> +---------------------------------
> +The PCI driver is responsible for making all transaction with the chip's

                                                transactions

> +firmware over PCI-e.
> +The sensors are being started and stopped respectively by writing commands

   The sensors are started and stopped

> +and, where applicable, DRAM addresses to certain device registers.
> +The sensor's input report data can then be accessed by accessing the DRAM
> +through DMA-mapped virtual addresses. Commands are sent to the device using C2P
> +mail box registers. These C2P registers are mapped in PCIe address space.

what is C2P?

> +Writing into the device message registers generates interrupts. The device's
> +firmware uses DRAM interface registers to indirectly access DRAM memory. It is
> +recommended to always write a minimum of 32 bytes into the DRAM.
> +
> +Driver loading
> +--------------
> +
> ++------------+-----------------+----------------------+
> +| PCI driver | Platform driver | HID low-level driver |
> ++============+=================+======================+
> +| Loaded at boot time if       | Used by spawned HIDs |
> +| device is present.           |                      |
> ++------------------------------+----------------------+
> +
> +Data flow table
> +---------------
> +.. code-block:: none
> +
> +                                                 +===============================================+
> +    +============+        Get sensor mask        |                Platform driver                |
> +    | PCI driver | <---------------------------- +===============================================+
> +    +============+    of available HID devices   | * Probe HID devices according to sensor mask. |
> +          ^                                      | * Start periodical polling from DRAM.         |

                                                              periodic

> +          |                                      +-----------------------------------------------+
> + Start / stop sensor on                                                 |
> + respective HID requsts.                                                |

                   requests.

> +          |                +==============================+             |
> +          |                |        HID ll-driver         |             |
> +          +--------------- +==============================+ <-----------+
> +                           | Provide reports as requested |
> +                           | by hid-code.                 |
> +                           +------------------------------+
> +
> +Quirks
> +------
> +On some systems, the sensor hub has not been programmed with information about
> +the sensors active on the device. This results in no sensors bein activated and

                                                                being

> +no HID devices being spawned by the driver. To manually active the respective

                                                           activate

> +sensors, you can load the module `amd-sfh-hid` with the kernel parameter
> +`sensor_mask=<int>`. The available sensors are currently:
> +

How about adding a "value" column and an example?

> ++----------------------+----------+
> +|        sensor        |   mask   |  value
> ++======================+==========+
> +| accelerometer        |  BIT(0)  |    1
> +| gyroscope            |  BIT(1)  |    2
> +| magnetometer         |  BIT(2)  |    4
> +| ambient light sensor |  BIT(19) | 524288
> ++----------------------+----------+

The values are additive, so to enable the gyroscope and the
ambient light sensor, use a value of 524290.

> +
> +To enable e.g. only the accelerometer:
> +
> +    $ cat /etc/modprobe.d/amd_sfh.conf
> +    options amd_sfh_hid sensor_mask=1
> 


HTH.
-- 
~Randy