Re: [PATCH v3 00/25] Add initial support for MHI endpoint stack

Alex Elder
2022-02-15T20:01:49Z

On 2/12/22 12:20 PM, Manivannan Sadhasivam wrote:
> Hello,
>
> This series adds initial support for the Qualcomm specific Modem Host Interface
> (MHI) bus in endpoint devices like SDX55 modems. The MHI bus in endpoint devices
> communicates with the MHI bus in host machines like x86 over any physical bus
> like PCIe. The MHI host support is already in mainline [1] and been used by PCIe
> based modems and WLAN devices running vendor code (downstream).

Maybe "running (downstream) vendor code".

I have a few general comments, which I'll mention here.

- This description goes out of its way to say MHI *could* be used over
almost any transport, and PCIe just happens to be one of them. The
reality is, you are only supporting it over PCIe, and as far as I
know you have no plans to go beyond that. Even if you did, I think
it should be clearer that you are doing MHI support over PCIe, even
though other options are possible (and could be incorporated in the
future).
- The first two patches are bug fixes; I think you should send those
out right away, without waiting for the entire series to get accepted.
- But ideally, can we get a "Tested-by" tag on these first?
- The next several, maybe up to patch 7, are also sort of preparatory
for the "real" code you're adding. Maybe those could be sent out
early/separately too, knowing that the end goal is to get the MHI
endpoint support accepted.
- Given the endianness issues (which I pointed out last time, but
which seem to be addressed), are you able to test this code using
a host that has different endianness than the modem CPU (SDX55)?
- Paul Davey seems to have the ability to test this.
- I have a few very minor suggestions in the wording below.
- You really need a picture to make it easier to see at a glance what
the hardware model is. Here's one I did at one point, but it also
includes the IPA in it (which is the FUUUUTURE!!!). The SDX55 AP
controls the PCIe endpoint.

.................... ..................................
: "Intel host" : : "SDX55" :
: : : ------------ :
: : : | SDX55 AP | :
: : : ------------ :
: : | | : | :
: -------- (root complex) |P| (endpoint) ------- --------- :
: | Host |----------------|C|------------| IPA |-------| Modem | :
: -------- : |I| : ------- --------- :
:..................: |e| :................................:
| |

Something this picture does not show is that the transfer,
command and event rings (and buffers) reside in host memory,
while information *about* those rings (size, location, and
current read/write pointers) reside in PCIe memory.

> Overview
> ========
>
> This series aims at adding the MHI support in the endpoint devices with the goal

This series adds the MHI support...

> of getting data connectivity using the mainline kernel running on the modems.
> Modems here refer to the combination of an APPS processor (Cortex A grade) and
> a baseband processor (DSP). The MHI bus is located in the APPS processor and it
> transfers data packets from the baseband processor to the host machine.
>
> The MHI Endpoint (MHI EP) stack proposed here is inspired by the downstream
> code written by Qualcomm. But the complete stack is mostly re-written to adapt
> to the "bus" framework and made it modular so that it can work with the upstream

...framework to make it modular, so that...

> subsystems like "PCI Endpoint". The code structure of the MHI endpoint stack
> follows the MHI host stack to maintain uniformity.
>
> With this initial MHI EP stack (along with few other drivers), we can establish
> the network interface between host and endpoint over the MHI software channels
> (IP_SW0) and can do things like IP forwarding, SSH, etc...
>
> Stack Organization
> ==================
>
> The MHI EP stack has the concept of controller and device drivers as like the
> MHI host stack. The MHI EP controller driver can be a PCI Endpoint Function
> driver and the MHI device driver can be a MHI EP Networking driver or QRTR
> driver. The MHI EP controller driver is tied to the PCI Endpoint subsystem and
> handles all bus related activities like mapping the host memory, raising IRQ,
> passing link specific events etc... The MHI EP networking driver is tied to the
> Networking stack and handles all networking related activities like
> sending/receiving the SKBs from netdev, statistics collection etc...
>
> This series only contains the MHI EP code, whereas the PCIe EPF driver and MHI
> EP Networking drivers are not yet submitted and can be found here [2]. Though
> the MHI EP stack doesn't have the build time dependency, it cannot function
> without them.
>
> Test setup
> ==========
>
> This series has been tested on Telit FN980 TLB board powered by Qualcomm SDX55
> (a.k.a X55 modem) and Qualcomm SM8450 based dev board.
>
> For testing the stability and performance, networking tools such as iperf, ssh
> and ping are used.
>
> Limitations
> ===========
>
> We are not _yet_ there to get the data packets from the modem as that involves
> the Qualcomm IP Accelerator (IPA) integration with MHI endpoint stack. But we
> are planning to add support for it in the coming days.

s/days/months/

And now I'm going to send this, along with my comments on the first
half of the patches. I'll keep going on the rest after that.

-Alex

>
> References
> ==========
>
> MHI bus: https://www.kernel.org/doc/html/latest/mhi/mhi.html
> Linaro connect presentation around this topic: https://connect.linaro.org/resources/lvc21f/lvc21f-222/
>
> Thanks,
> Mani
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bus/mhi
> [2] https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=tracking-qcomlt-sdx55-drivers
>
> Changes in v3:
>
> * Splitted the patch 20/23 into two.
> * Fixed the error handling in patch 21/23.
> * Removed spurious change in patch 01/23.
> * Added check for xfer callbacks in client driver probe.
>
> Changes in v2:
>
> v2 mostly addresses the issues seen while testing the stack on SM8450 that is a
> SMP platform and also incorporates the review comments from Alex.
>
> Major changes are:
>
> * Added a cleanup patch for getting rid of SHIFT macros and used the bitfield
> operations.
> * Added the endianess patches that were submitted to MHI list and used the
> endianess conversion in EP patches also.
> * Added support for multiple event rings.
> * Fixed the MSI generation based on the event ring index.
> * Fixed the doorbell list handling by making use of list splice and not locking
> the entire list manipulation.
> * Added new APIs for wrapping the reading and writing to host memory (Dmitry).
> * Optimized the read_channel and queue_skb function logics.
> * Added Hemant's R-o-b tag.
>
> Manivannan Sadhasivam (23):
> bus: mhi: Move host MHI code to "host" directory
> bus: mhi: Move common MHI definitions out of host directory
> bus: mhi: Make mhi_state_str[] array static inline and move to
> common.h
> bus: mhi: Cleanup the register definitions used in headers
> bus: mhi: Get rid of SHIFT macros and use bitfield operations
> bus: mhi: ep: Add support for registering MHI endpoint controllers
> bus: mhi: ep: Add support for registering MHI endpoint client drivers
> bus: mhi: ep: Add support for creating and destroying MHI EP devices
> bus: mhi: ep: Add support for managing MMIO registers
> bus: mhi: ep: Add support for ring management
> bus: mhi: ep: Add support for sending events to the host
> bus: mhi: ep: Add support for managing MHI state machine
> bus: mhi: ep: Add support for processing MHI endpoint interrupts
> bus: mhi: ep: Add support for powering up the MHI endpoint stack
> bus: mhi: ep: Add support for powering down the MHI endpoint stack
> bus: mhi: ep: Add support for handling MHI_RESET
> bus: mhi: ep: Add support for handling SYS_ERR condition
> bus: mhi: ep: Add support for processing command ring
> bus: mhi: ep: Add support for reading from the host
> bus: mhi: ep: Add support for processing transfer ring
> bus: mhi: ep: Add support for queueing SKBs to the host
> bus: mhi: ep: Add support for suspending and resuming channels
> bus: mhi: ep: Add uevent support for module autoloading
>
> Paul Davey (2):
> bus: mhi: Fix pm_state conversion to string
> bus: mhi: Fix MHI DMA structure endianness
>
> drivers/bus/Makefile | 2 +-
> drivers/bus/mhi/Kconfig | 28 +-
> drivers/bus/mhi/Makefile | 9 +-
> drivers/bus/mhi/common.h | 319 ++++
> drivers/bus/mhi/ep/Kconfig | 10 +
> drivers/bus/mhi/ep/Makefile | 2 +
> drivers/bus/mhi/ep/internal.h | 254 ++++
> drivers/bus/mhi/ep/main.c | 1601 +++++++++++++++++++++
> drivers/bus/mhi/ep/mmio.c | 274 ++++
> drivers/bus/mhi/ep/ring.c | 267 ++++
> drivers/bus/mhi/ep/sm.c | 174 +++
> drivers/bus/mhi/host/Kconfig | 31 +
> drivers/bus/mhi/{core => host}/Makefile | 4 +-
> drivers/bus/mhi/{core => host}/boot.c | 17 +-
> drivers/bus/mhi/{core => host}/debugfs.c | 40 +-
> drivers/bus/mhi/{core => host}/init.c | 123 +-
> drivers/bus/mhi/{core => host}/internal.h | 427 +-----
> drivers/bus/mhi/{core => host}/main.c | 46 +-
> drivers/bus/mhi/{ => host}/pci_generic.c | 0
> drivers/bus/mhi/{core => host}/pm.c | 36 +-
> include/linux/mhi_ep.h | 293 ++++
> include/linux/mod_devicetable.h | 2 +
> scripts/mod/file2alias.c | 10 +
> 23 files changed, 3442 insertions(+), 527 deletions(-)
> create mode 100644 drivers/bus/mhi/common.h
> create mode 100644 drivers/bus/mhi/ep/Kconfig
> create mode 100644 drivers/bus/mhi/ep/Makefile
> create mode 100644 drivers/bus/mhi/ep/internal.h
> create mode 100644 drivers/bus/mhi/ep/main.c
> create mode 100644 drivers/bus/mhi/ep/mmio.c
> create mode 100644 drivers/bus/mhi/ep/ring.c
> create mode 100644 drivers/bus/mhi/ep/sm.c
> create mode 100644 drivers/bus/mhi/host/Kconfig
> rename drivers/bus/mhi/{core => host}/Makefile (54%)
> rename drivers/bus/mhi/{core => host}/boot.c (96%)
> rename drivers/bus/mhi/{core => host}/debugfs.c (90%)
> rename drivers/bus/mhi/{core => host}/init.c (93%)
> rename drivers/bus/mhi/{core => host}/internal.h (50%)
> rename drivers/bus/mhi/{core => host}/main.c (98%)
> rename drivers/bus/mhi/{ => host}/pci_generic.c (100%)
> rename drivers/bus/mhi/{core => host}/pm.c (97%)
> create mode 100644 include/linux/mhi_ep.h
>