MHI initial design review

MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus. This module will allow host to communicate with external devices that support MHI protocol.

Signed-off-by Sujeev Dias <[email protected]>

---
 Documentation/00-INDEX                        |    2 +
 Documentation/devicetree/bindings/bus/mhi.txt |  141 +++
 Documentation/mhi.txt                         |  235 ++++
 drivers/bus/Kconfig                           |   17 +
 drivers/bus/Makefile                          |    1 +
 drivers/bus/mhi/Makefile                      |    8 +
 drivers/bus/mhi/core/Makefile                 |    1 +
 drivers/bus/mhi/core/mhi_boot.c               |  593 ++++++++++

 drivers/bus/mhi/core/mhi_dtr.c                |  177 +++
 drivers/bus/mhi/core/mhi_init.c               | 1290 +++++++++++++++++++++
 drivers/bus/mhi/core/mhi_internal.h           |  732 ++++++++++++
 drivers/bus/mhi/core/mhi_main.c               | 1476 +++++++++++++++++++++++++
 drivers/bus/mhi/core/mhi_pm.c                 | 1177 ++++++++++++++++++++
 include/linux/mhi.h                           |  694 ++++++++++++
 include/linux/mod_devicetable.h               |   11 +
 15 files changed, 6555 insertions(+)

+#ifdef CONFIG_MHI_DEBUG

include/linux/mhi.h new file +17 -0

+
+#define MHI_VERB(fmt, ...) do { \
+		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_VERBOSE) \
+			pr_debug("[D][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+
+#else
+
+#define MHI_VERB(fmt, ...)
+
+#endif
+
+#define MHI_LOG(fmt, ...) do {	\
+		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_INFO) \
+			pr_info("[I][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+

Quoted 5 lines · Sujeev Dias

MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus. This module will allow host to communicate with external devices that support MHI protocol.

Quoted 1 line

Signed-off-by Sujeev Dias <[email protected]>

No one else has ever reviewed this code before? That's not good, please at the very least, have someone else at your company go over it first. I don't want to be the ones having to point out all of the "obvious" issues :)

Quoted 9 lines

---
  Documentation/00-INDEX                        |    2 +
  Documentation/devicetree/bindings/bus/mhi.txt |  141 +++
  Documentation/mhi.txt                         |  235 ++++
  drivers/bus/Kconfig                           |   17 +
  drivers/bus/Makefile                          |    1 +
  drivers/bus/mhi/Makefile                      |    8 +
  drivers/bus/mhi/core/Makefile                 |    1 +
  drivers/bus/mhi/core/mhi_boot.c               |  593 ++++++++++

Quoted 8 lines

  drivers/bus/mhi/core/mhi_dtr.c                |  177 +++
  drivers/bus/mhi/core/mhi_init.c               | 1290 +++++++++++++++++++++
  drivers/bus/mhi/core/mhi_internal.h           |  732 ++++++++++++
  drivers/bus/mhi/core/mhi_main.c               | 1476 +++++++++++++++++++++++++
  drivers/bus/mhi/core/mhi_pm.c                 | 1177 ++++++++++++++++++++
  include/linux/mhi.h                           |  694 ++++++++++++
  include/linux/mod_devicetable.h               |   11 +
  15 files changed, 6555 insertions(+)

And a 6555 line patch is a bit hard to consume all at once. Can't this be split up into much more reviewable chunks? Look at how some of the other new bus subsystems got added to the tree recently. They were submitted in longer patch series, but smaller sized patches individually. That makes things much easier to review.

For example, there is no reason your debugfs stuff needs to be in this initial patch. That should be in a separate one, right? Same for firmware download. Please take the time to break this up into logical steps.

Like my son's math teacher keeps telling him, "show your work, not just an answer at the bottom of the page".

Also, it is required by the DT maintainers to split that file alone up into a separate patch to be even considered for merging.

One thing I can tell you right now that isn't acceptable:

Quoted 1 line

+#ifdef CONFIG_MHI_DEBUG

Don't have a separate config option for debugging. No one will enable
it, which makes it pointless. Everything has to be dynamic these days.

Quoted 17 lines

include/linux/mhi.h new file +17 -0

+
+#define MHI_VERB(fmt, ...) do { \
+		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_VERBOSE) \
+			pr_debug("[D][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+
+#else
+
+#define MHI_VERB(fmt, ...)
+
+#endif
+
+#define MHI_LOG(fmt, ...) do {	\
+		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_INFO) \
+			pr_info("[I][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+

include/linux/mhi.h line 692

Quoted 10 lines

include/linux/mhi.h new file +10 -0

+#define MHI_ERR(fmt, ...) do {	\
+		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_ERROR) \
+			pr_err("[E][%s] " fmt, __func__, ##__VA_ARGS__); \
+} while (0)
+
+#define MHI_CRITICAL(fmt, ...) do { \
+		if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_CRITICAL) \
+			pr_alert("[C][%s] " fmt, __func__, ##__VA_ARGS__); \
+} while (0)
+

And do not roll your own debugging/logging macros. Use what is given to you (dev_info(), dev_err(), dev_dbg()), they are there for a reason. By going around them, you circumvent the whole of the kernel logging infrastructure and declare that your tiny bus is somehow more "special" than it.

And I doubt you want to make such a statement :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks for quick feedback

Quoted 6 lines · Greg Kroah-Hartman

Quoted 5 lines · Sujeev Dias

MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus. This module will allow host to communicate with external devices that support MHI protocol.

Quoted 6 lines

Quoted 1 line

Signed-off-by Sujeev Dias <[email protected]>

No one else has ever reviewed this code before? That's not good, please at the very least, have someone else at your company go over it first. I don't want to be the ones having to point out all of the "obvious" issues :)

This code has gone thru rigorous code review and testing, before I submit next patch I will have multiple people sign off on it.

Intention was to completely compile out MHI_VERB messages because we have those messages in data path. For release build, we wanted to reduce as much mips as possible. However, for debugging these messages are extremely helpful.

I will look into tracepoints...

drivers/bus/mhi/core/mhi_boot.c new file +1 -0

@@ -0,0 +1,593 @@
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved.

Documentation/devicetree/bindings/bus/mhi.txt new file +52 -0

new file mode 100644
index 0000000..ea1b620
@@ -0,0 +1,141 @@
+MHI Host Interface
+
+MHI used by the host to control and communicate with modem over
+high speed peripheral bus.
+
+==============
+Node Structure
+==============
+
+Main node properties:
+
+- mhi,max-channels
+  Usage: required
+  Value type: <u32>
+  Definition: Maximum number of channels supported by this controller
+
+- mhi,chan-cfg
+  Usage: required
+  Value type: Array of <u32>
+  Definition: Array of tuples describe channel configuration.
+       1st element: Physical channel number
+       2nd element: Transfer ring length in elements
+       3rd element: Event ring associated with this channel
+       4th element: Channel direction as defined by enum dma_data_direction
+               1 = UL data transfer
+               2 = DL data transfer
+       5th element: Channel doorbell mode configuration as defined by
+       enum MHI_BRSTMODE
+               2 = burst mode disabled
+               3 = burst mode enabled
+       6th element: mhi doorbell configuration, valid only when burst mode
+       enabled.
+               0 = Use default (device specific) polling configuration
+               For UL channels, value specifies the timer to poll MHI context
+               in milliseconds.
+               For DL channels, the threshold to poll the MHI context
+               in multiple of eight ring element.
+       7th element: Channel execution enviornment as defined by enum MHI_EE
+               1 = Bootloader stage
+               2 = AMSS mode
+       8th element: data transfer type accepted as defined by enum
+       MHI_XFER_TYPE
+               0 = accept cpu address for buffer
+               1 = accept skb
+               2 = accept scatterlist
+               3 = offload channel, does not accept any transfer type
+       9th element: Bitwise configuration settings for the channel
+               Bit mask:
+               BIT(0) : LPM notify, this channel master requre lpm enter/exit
+               notifications.
+               BIT(1) : Offload channel, MHI host only involved in setting up
+               the data pipe. Not involved in active data transfer.

Documentation/devicetree/bindings/bus/mhi.txt new file +7 -0

+Data structures
+---------------
+Host memory : Directly accessed by the host to manage the MHI data structures
+and buffers. The device accesses the host memory over the PCIe interface.
+
+Channel context array : All channel configurations are organized in channel
+context data array.

Documentation/devicetree/bindings/bus/mhi.txt new file +5 -0

+void mhi_init_debugfs(struct mhi_controller *mhi_cntrl)
+{
+       struct dentry *dentry;
+       char node[32];
+

Documentation/devicetree/bindings/bus/mhi.txt new file +6 -0

+
+#ifdef CONFIG_MHI_DEBUG
+
+#define MHI_ASSERT(cond, msg) do { \
+       if (cond) \
+               panic(msg); \

Documentation/devicetree/bindings/bus/mhi.txt new file +6 -0

+};
+
+/**
+ * struct mhi_device - mhi device structure associated bind to channel
+ * @dev: Device associated with the channels
+ * @mtu: Maximum # of bytes controller support

Documentation/devicetree/bindings/bus/mhi.txt new file +19 -0

+
+#ifdef CONFIG_MHI_DEBUG
+
+#define MHI_VERB(fmt, ...) do { \
+               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_VERBOSE) \
+                       pr_debug("[D][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+
+#else
+
+#define MHI_VERB(fmt, ...)
+
+#endif
+
+#define MHI_LOG(fmt, ...) do { \
+               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_INFO) \
+                       pr_info("[I][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+

include/linux/mod_devicetable.h +0 -0

index 7d361be..1e11e30 100644
@@ -734,4 +734,15 @@ struct tb_service_id {
 #define TBSVC_MATCH_PROTOCOL_VERSION   0x0004
 #define TBSVC_MATCH_PROTOCOL_REVISION  0x0008
 

Quoted 58 lines · Sujeev Dias

Documentation/devicetree/bindings/bus/mhi.txt new file +52 -0

new file mode 100644
index 0000000..ea1b620
@@ -0,0 +1,141 @@
+MHI Host Interface
+
+MHI used by the host to control and communicate with modem over
+high speed peripheral bus.
+
+==============
+Node Structure
+==============
+
+Main node properties:
+
+- mhi,max-channels
+  Usage: required
+  Value type: <u32>
+  Definition: Maximum number of channels supported by this controller
+
+- mhi,chan-cfg
+  Usage: required
+  Value type: Array of <u32>
+  Definition: Array of tuples describe channel configuration.
+       1st element: Physical channel number
+       2nd element: Transfer ring length in elements
+       3rd element: Event ring associated with this channel
+       4th element: Channel direction as defined by enum dma_data_direction
+               1 = UL data transfer
+               2 = DL data transfer
+       5th element: Channel doorbell mode configuration as defined by
+       enum MHI_BRSTMODE
+               2 = burst mode disabled
+               3 = burst mode enabled
+       6th element: mhi doorbell configuration, valid only when burst mode
+       enabled.
+               0 = Use default (device specific) polling configuration
+               For UL channels, value specifies the timer to poll MHI context
+               in milliseconds.
+               For DL channels, the threshold to poll the MHI context
+               in multiple of eight ring element.
+       7th element: Channel execution enviornment as defined by enum MHI_EE
+               1 = Bootloader stage
+               2 = AMSS mode
+       8th element: data transfer type accepted as defined by enum
+       MHI_XFER_TYPE
+               0 = accept cpu address for buffer
+               1 = accept skb
+               2 = accept scatterlist
+               3 = offload channel, does not accept any transfer type
+       9th element: Bitwise configuration settings for the channel
+               Bit mask:
+               BIT(0) : LPM notify, this channel master requre lpm enter/exit
+               notifications.
+               BIT(1) : Offload channel, MHI host only involved in setting up
+               the data pipe. Not involved in active data transfer.

Documentation/devicetree/bindings/bus/mhi.txt line 62

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +10 -0

+               BIT(2) : Must switch to doorbell mode whenever MHI M0 state
+               transition happens.
+               BIT(3) : MHI bus driver pre-allocate buffer for this channel.
+               If set, clients not allowed to queue buffers. Valid only for DL
+               direction.
+
+- mhi,chan-names
+  Usage: required
+  Value type: Array of <string>
+  Definition: Channel names configured in mhi,chan-cfg.

I think the top-level structure would be better done by requiring a child for each channel and moving the mhi,chan-cfg into the children nodes, either as separate properties, or some of the fields combined into a 'reg' property.

Documentation/devicetree/bindings/bus/mhi.txt line 92

Quoted 4 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +4 -0

+- mhi,fw-name
+  Usage: optional
+  Value type: <string>
+  Definition: Firmware image name to upload

The device tree should have no knowledge of a firmware file name.

What you should do instead is to have the driver generate the file name from the "compatible" property. You could use a lookup table in the driver, or have some algorithmic way of doing that, but you want the driver to have some form of intelligence to let it pick a different firmware image, e.g. when you have added new capabilities to the driver and the firmware but want to use an existing device tree.

Quoted 7 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +7 -0

+Data structures
+---------------
+Host memory : Directly accessed by the host to manage the MHI data structures
+and buffers. The device accesses the host memory over the PCIe interface.
+
+Channel context array : All channel configurations are organized in channel
+context data array.

Documentation/devicetree/bindings/bus/mhi.txt line 83

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +10 -0

+
+struct __packed mhi_chan_ctxt;
+struct mhi_ctxt.chan_ctxt;
+
+Transfer rings : Used by host to schedule work items for a channel and organized
+as a circular queue of transfer descriptors (TD).
+
+struct __packed mhi_tre;
+struct mhi_chan.tre_ring;
+

This looks like you reinvented virtio. What are the reasons for not just using virtio directly as we do for other modem interfaces?

Documentation/devicetree/bindings/bus/mhi.txt line 93

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +10 -0

+static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
+                          void *buf,
+                          size_t size)
+{
+       u32 tx_status, val;
+       int i, ret;
+       void __iomem *base = mhi_cntrl->bhi;
+       rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
+       dma_addr_t phys = dma_map_single(mhi_cntrl->dev, buf, size,
+                                        DMA_TO_DEVICE);

Please don't name a dma address 'phys', it's very confusing ;-)

Documentation/devicetree/bindings/bus/mhi.txt line 94

Quoted 1 line

Documentation/devicetree/bindings/bus/mhi.txt new file +1 -0

+struct mhi_bus mhi_bus;

One global instance? I suspec this duplicates data that is already in the bus_type structure, so just use that instead.

Quoted 5 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +5 -0

+void mhi_init_debugfs(struct mhi_controller *mhi_cntrl)
+{
+       struct dentry *dentry;
+       char node[32];
+

Documentation/devicetree/bindings/bus/mhi.txt line 109

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +10 -0

+       if (!mhi_cntrl->parent)
+               return;
+
+       snprintf(node, sizeof(node), "%04x_%02u:%02u.%02u",
+                mhi_cntrl->dev_id, mhi_cntrl->domain, mhi_cntrl->bus,
+                mhi_cntrl->slot);
+
+       dentry = debugfs_create_dir(node, mhi_cntrl->parent);
+       if (IS_ERR_OR_NULL(dentry))
+               return;

Using IS_ERR_OR_NULL() just means that you have misunderstood the interface, you want IS_ERR() here.

Documentation/devicetree/bindings/bus/mhi.txt line 112

Quoted 3 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +3 -0

+#if defined(CONFIG_OF)
+static int of_parse_ev_cfg(struct mhi_controller *mhi_cntrl,
+                          struct device_node *of_node)

Why the #ifdef? Do you ever build this on architectures that don't support CONFIG_OF yet? Which ones?

Documentation/devicetree/bindings/bus/mhi.txt line 123

Quoted 11 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +11 -0

+struct __packed mhi_event_ctxt {
+       u32 reserved : 8;
+       u32 intmodc : 8;
+       u32 intmodt : 16;
+       u32 ertype;
+       u32 msivec;
+       u64 rbase;
+       u64 rlen;
+       u64 rp;
+       u64 wp;
+};

I would generally only mark those members as __packed that don't already have natural alignment, such as

struct mhi_event_ctxt {
        u32 reserved : 8;
        u32 intmodc : 8;
        u32 intmodt : 16;
        u32 ertype;
        u32 msivec;
        u64 rbase __packed __aligned(4);
        u64 rlen__packed __aligned(4);
        u64 rp __packed __aligned(4);
        u64 wp__packed __aligned(4);
};

This clarifies where the hardware designers screwed up without forcing bytewise access to the structures.

Quoted 6 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +6 -0

+
+#ifdef CONFIG_MHI_DEBUG
+
+#define MHI_ASSERT(cond, msg) do { \
+       if (cond) \
+               panic(msg); \

Documentation/devicetree/bindings/bus/mhi.txt line 139

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +10 -0

+} while (0)
+
+#else
+
+#define MHI_ASSERT(cond, msg) do { \
+       if (cond) { \
+               MHI_ERR(msg); \
+               WARN_ON(cond); \
+       } \
+} while (0)

Remove these.

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 6 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +6 -0

+struct mhi_controller {
+       struct list_head node;
+
+       /* device node for iommu ops */
+       struct device *dev;
+       struct device_node *of_node;

Hmm, shouldn't the mhi_controller itself be a device that is a child of ->dev? It feels like

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +10 -0

+       /* mmio base */
+       void __iomem *regs;
+       void __iomem *bhi;
+       void __iomem *wake_db;
+
+       /* device topology */
+       u32 dev_id;
+       u32 domain;
+       u32 bus;
+       u32 slot;

These are even stranger. This looks PCI specific, but if you have a pci_dev then you know all of them while for the case you don't have a pci_dev, they don't make sense.

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 11 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +11 -0

+       /* kernel log level */
+       enum MHI_DEBUG_LEVEL klog_lvl;
+
+       /* private log level controller driver to set */
+       enum MHI_DEBUG_LEVEL log_lvl;
+
+       /* controller specific data */
+       void *priv_data;
+       void *log_buf;
+       struct dentry *dentry;
+       struct dentry *parent;

All the debugfs stuff is a bit worrying. It seems so pervasive in the driver that it's hard to tell if any of it is actually used for more than just debugging. It might be better to split that out of the series and add back the debug files one at a time in separate patches that each explain why the files are still required. If you have successfully debugged a problem with one
file, it may have seemed useful but maybe it's not needed for any future bugs.

You might also be able to get more value out of tracepoints than debugfs files.

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +10 -0

+};
+
+/**
+ * struct mhi_device - mhi device structure associated bind to channel
+ * @dev: Device associated with the channels
+ * @mtu: Maximum # of bytes controller support
+ * @ul_chan_id: MHI channel id for UL transfer
+ * @dl_chan_id: MHI channel id for DL transfer
+ * @priv: Driver private data
+ */

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 6 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +6 -0

+struct mhi_device {
+       struct device dev;
+       u32 dev_id;
+       u32 domain;
+       u32 bus;
+       u32 slot;

Again, shouldn't the pci stuff be part of the parent or grandparent device?

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 2 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +2 -0

+
+#if defined(CONFIG_MHI_BUS)

In what situation would you include this header without enabling CONFIG_MHI_BUS?

Quoted 19 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +19 -0

+
+#ifdef CONFIG_MHI_DEBUG
+
+#define MHI_VERB(fmt, ...) do { \
+               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_VERBOSE) \
+                       pr_debug("[D][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+
+#else
+
+#define MHI_VERB(fmt, ...)
+
+#endif
+
+#define MHI_LOG(fmt, ...) do { \
+               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_INFO) \
+                       pr_info("[I][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt new file +10 -0

+#define MHI_ERR(fmt, ...) do { \
+               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_ERROR) \
+                       pr_err("[E][%s] " fmt, __func__, ##__VA_ARGS__); \
+} while (0)
+
+#define MHI_CRITICAL(fmt, ...) do { \
+               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_CRITICAL) \
+                       pr_alert("[C][%s] " fmt, __func__, ##__VA_ARGS__); \
+} while (0)
+

Again, these all should be removed. Just use dev_err() etc.

Quoted 8 lines

include/linux/mod_devicetable.h +0 -0

index 7d361be..1e11e30 100644
@@ -734,4 +734,15 @@ struct tb_service_id {
  #define TBSVC_MATCH_PROTOCOL_VERSION   0x0004
  #define TBSVC_MATCH_PROTOCOL_REVISION  0x0008
 

include/linux/mod_devicetable.h struct tb_service_id { · line 746

Quoted 10 lines

include/linux/mod_devicetable.h +10 -0

+
+/**
+ * struct mhi_device_id - MHI device identification
+ * @chan: MHI channel name
+ * @driver_data: driver data;
+ */
+struct mhi_device_id {
+       const char *chan;
+       kernel_ulong_t driver_data;
+};

I think you have to use a fixed-length string here, otherwise module autoloading cannot work.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus. This module will allow host to communicate with external devices that support MHI protocol.

Hi!

Quoted 4 lines

MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus. This module will allow host to communicate with external devices that support MHI protocol.

I have Motorola Droid 4 cellphone here, with Qualcomm GSM modem. But it talks over serial line and USB. I guess MHI is not applicable to my hardware?

Documentation/mhi.txt line 211

Quoted 5 lines

Documentation/mhi.txt new file +5 -0

+MHI Devices
+-----------
+Logical device that bind to maximum of two physical MHI channels. Once MHI is in
+powered on state, each supported channel by controller will be allocated as a
+mhi_device.

What kind of protocol is running over MHI? I guess its not AT commands. QMI? Or something entirely different?

Pavel

drivers/bus/Kconfig +12 -0

+	  Enabling this module will allow host to communicate with external
+	  devices that support MHI protocol.
+
+config MHI_DEBUG
+	 bool "MHI debug support"
+	 depends on MHI_BUS
+	 help
+	   Say yes here to enable debugging support in the MHI transport
+	   and individual MHI client drivers. This option will impact
+	   throughput as individual MHI packets and state transitions
+	   will be logged.
+
 source "drivers/bus/fsl-mc/Kconfig"
 
 endmenu

QCOM PCIe based modems uses MHI as the communication protocol. MHI control driver is the bus master for such modems. As the bus master driver, it oversees power management operations such as suspend, resume, powering on and off the device.

+- compatible

Documentation/devicetree/bindings/bus/mhi_qcom.txt new file +14 -0

+  Usage: required
+  Value type: <string>
+  Definition: "qcom,mhi"
+
+- qcom,pci-dev-id
+  Usage: optional
+  Value type: <u32>
+  Definition: PCIe device id of external modem to bind. If not set, any
+       device is compatible with this node.
+
+- qcom,pci-domain
+  Usage: required
+  Value type: <u32>
+  Definition: PCIe root complex external modem connected to

+- qcom,smmu-cfg

Documentation/devicetree/bindings/bus/mhi_qcom.txt new file +4 -0

+  Usage: required
+  Value type: <u32>
+  Definition: Required SMMU configuration bitmask for PCIe bus.
+       BIT mask:

+static struct pci_driver mhi_pcie_driver;

drivers/bus/mhi/controllers/mhi_qcom.c new file +25 -0

+static int mhi_platform_probe(struct platform_device *pdev)
+{
+       struct mhi_controller *mhi_cntrl;
+       struct mhi_dev *mhi_dev;
+       struct device_node *of_node = pdev->dev.of_node;
+       u64 addr_win[2];
+       int ret;
+
+       if (!of_node)
+               return -ENODEV;
+
+       mhi_cntrl = mhi_alloc_controller(sizeof(*mhi_dev));
+       if (!mhi_cntrl)
+               return -ENOMEM;
+
+       mhi_dev = mhi_controller_get_devdata(mhi_cntrl);
+
+       /* get pci bus topology for this node */
+       ret = of_property_read_u32(of_node, "qcom,pci-dev-id",
+                                  &mhi_cntrl->dev_id);
+       if (ret)
+               mhi_cntrl->dev_id = PCI_ANY_ID;
+
+       ret = of_property_read_u32(of_node, "qcom,pci-domain",
+                                  &mhi_cntrl->domain);

Quoted 25 lines · Sujeev Dias

QCOM PCIe based modems uses MHI as the communication protocol. MHI control driver is the bus master for such modems. As the bus master driver, it oversees power management operations such as suspend, resume, powering on and off the device.

+- compatible

Documentation/devicetree/bindings/bus/mhi_qcom.txt new file +19 -0

+  Usage: required
+  Value type: <string>
+  Definition: "qcom,mhi"
+
+- qcom,pci-dev-id
+  Usage: optional
+  Value type: <u32>
+  Definition: PCIe device id of external modem to bind. If not set, any
+       device is compatible with this node.
+
+- qcom,pci-domain
+  Usage: required
+  Value type: <u32>
+  Definition: PCIe root complex external modem connected to
+
+- qcom,pci-bus
+  Usage: required
+  Value type: <u32>
+  Definition: PCIe bus external modem connected to

Documentation/devicetree/bindings/bus/mhi_qcom.txt line 36

Quoted 5 lines

Documentation/devicetree/bindings/bus/mhi_qcom.txt new file +5 -0

+
+- qcom,pci-slot
+  Usage: required
+  Value type: <u32>
+  Definition: PCIe slot as assigned by pci framework to external modem

These don't seem to make any sense: You seem to have access to a regular pci_device already, so why do you need to duplicate the information about it in DT?

Quoted 5 lines

+- qcom,smmu-cfg

Documentation/devicetree/bindings/bus/mhi_qcom.txt new file +4 -0

+  Usage: required
+  Value type: <u32>
+  Definition: Required SMMU configuration bitmask for PCIe bus.
+       BIT mask:

Documentation/devicetree/bindings/bus/mhi_qcom.txt line 52

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi_qcom.txt new file +10 -0

+       BIT(0) : Attach address mapping to endpoint device
+       BIT(1) : Set attribute S1_BYPASS
+       BIT(2) : Set attribute FAST
+       BIT(3) : Set attribute ATOMIC
+       BIT(4) : Set attribute FORCE_COHERENT
+
+- qcom,addr-win
+  Usage: required if SMMU S1 translation is enabled
+  Value type: Array of <u64>
+  Definition: Pair of values describing iova start and stop address

Why do you need these? Can't that be handled by the PCI layer?

Documentation/devicetree/bindings/bus/mhi_qcom.txt line 57

Quoted 4 lines

+- qcom,msm-bus,name

Documentation/devicetree/bindings/bus/mhi_qcom.txt new file +3 -0

+  Usage: required
+  Value type: <string>
+  Definition: string representing the bus scale client name to register

This probably belongs into a separate binding for the bus scale driver, right?

Quoted 1 line

+static struct pci_driver mhi_pcie_driver;

Please try to reorder the symbols to avoid forward declarations.

Quoted 30 lines

drivers/bus/mhi/controllers/mhi_qcom.c new file +30 -0

+static int mhi_platform_probe(struct platform_device *pdev)
+{
+       struct mhi_controller *mhi_cntrl;
+       struct mhi_dev *mhi_dev;
+       struct device_node *of_node = pdev->dev.of_node;
+       u64 addr_win[2];
+       int ret;
+
+       if (!of_node)
+               return -ENODEV;
+
+       mhi_cntrl = mhi_alloc_controller(sizeof(*mhi_dev));
+       if (!mhi_cntrl)
+               return -ENOMEM;
+
+       mhi_dev = mhi_controller_get_devdata(mhi_cntrl);
+
+       /* get pci bus topology for this node */
+       ret = of_property_read_u32(of_node, "qcom,pci-dev-id",
+                                  &mhi_cntrl->dev_id);
+       if (ret)
+               mhi_cntrl->dev_id = PCI_ANY_ID;
+
+       ret = of_property_read_u32(of_node, "qcom,pci-domain",
+                                  &mhi_cntrl->domain);
+       if (ret)
+               goto error_probe;
+
+       ret = of_property_read_u32(of_node, "qcom,pci-bus", &mhi_cntrl->bus);
+       if (ret)

drivers/bus/mhi/controllers/mhi_qcom.c line 568

Quoted 5 lines

drivers/bus/mhi/controllers/mhi_qcom.c new file +5 -0

+               goto error_probe;
+
+       ret = of_property_read_u32(of_node, "qcom,pci-slot", &mhi_cntrl->slot);
+       if (ret)
+               goto error_probe;

Please explain what you are trying to do here, why do you register two device drivers? It looks like they both refer to the same hardware, so why isn't it sufficient to have the pci_driver?

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

MHI based net device driver is used for transferring IP traffic between host and modem. Driver allows clients to transfer data using standard network interface.

Signed-off-by Sujeev Dias <[email protected]>

---
 Documentation/devicetree/bindings/bus/mhi.txt |  36 ++
 drivers/bus/Kconfig                           |   1 +
 drivers/bus/mhi/Makefile                      |   2 +-
 drivers/bus/mhi/devices/Kconfig               |  10 +
 drivers/bus/mhi/devices/Makefile              |   1 +
 drivers/bus/mhi/devices/mhi_netdev.c          | 893 ++++++++++++++++++++++++++

 6 files changed, 942 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bus/mhi/devices/Kconfig
 create mode 100644 drivers/bus/mhi/devices/Makefile
 create mode 100644 drivers/bus/mhi/devices/mhi_netdev.c

Documentation/devicetree/bindings/bus/mhi.txt +25 -0

index ea1b620..172ae7b 100644
@@ -139,3 +139,39 @@ mhi_controller {
                <driver specific properties>
        };
 };
+
+================
+Children Devices
+================
+
+MHI netdev properties
+
+- mhi,chan
+  Usage: required
+  Value type: <string>
+  Definition: Channel name MHI netdev support
 
+- mhi,mru
+  Usage: required
+  Value type: <u32>
+  Definition: Largest packet size interface can receive in bytes.
+
+- mhi,interface-name
+  Usage: optional
+  Value type: <string>
+  Definition: Interface name to be given so clients can identify it
+
+- mhi,recycle-buf
+  Usage: optional
+  Value type: <bool>
+  Definition: Set true if interface support recycling buffers.
 

Documentation/devicetree/bindings/bus/mhi.txt +15 -0

+#define MHI_NETDEV_DRIVER_NAME "mhi_netdev"
+#define WATCHDOG_TIMEOUT (30 * HZ)
+
+#ifdef CONFIG_MHI_DEBUG
+
+#define MHI_ASSERT(cond, msg) do { \
+       if (cond) \
+               panic(msg); \
+} while (0)
+
+#define MSG_VERB(fmt, ...) do { \
+       if (mhi_netdev->msg_lvl <= MHI_MSG_LVL_VERBOSE) \
+               pr_err("[D][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+

Documentation/devicetree/bindings/bus/mhi.txt +5 -0

+struct mhi_stats {
+       u32 rx_int;
+       u32 tx_full;
+       u32 tx_pkts;
+       u32 rx_budget_overflow;

Documentation/devicetree/bindings/bus/mhi.txt +17 -0

+struct mhi_netdev {
+       int alias;
+       struct mhi_device *mhi_dev;
+       spinlock_t rx_lock;
+       bool enabled;
+       rwlock_t pm_lock; /* state change lock */
+       int (*rx_queue)(struct mhi_netdev *mhi_netdev, gfp_t gfp_t);
+       struct work_struct alloc_work;
+       int wake;
+
+       struct sk_buff_head rx_allocated;
+
+       u32 mru;
+       const char *interface_name;
+       struct napi_struct napi;
+       struct net_device *ndev;
+       struct sk_buff *frag_skb;

Documentation/devicetree/bindings/bus/mhi.txt +11 -0

+static int mhi_netdev_alloc_skb(struct mhi_netdev *mhi_netdev, gfp_t gfp_t)
+{
+       u32 cur_mru = mhi_netdev->mru;
+       struct mhi_device *mhi_dev = mhi_netdev->mhi_dev;
+       struct mhi_skb_priv *skb_priv;
+       int ret;
+       struct sk_buff *skb;
+       int no_tre = mhi_get_no_free_descriptors(mhi_dev, DMA_FROM_DEVICE);
+       int i;
+
+       for (i = 0; i < no_tre; i++) {

Documentation/devicetree/bindings/bus/mhi.txt +9 -0

+static int mhi_netdev_poll(struct napi_struct *napi, int budget)
+{
+       struct net_device *dev = napi->dev;
+       struct mhi_netdev_priv *mhi_netdev_priv = netdev_priv(dev);
+       struct mhi_netdev *mhi_netdev = mhi_netdev_priv->mhi_netdev;
+       struct mhi_device *mhi_dev = mhi_netdev->mhi_dev;
+       int rx_work = 0;
+       int ret;
+

Documentation/devicetree/bindings/bus/mhi.txt +24 -0

+static int mhi_netdev_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+       struct mhi_netdev_priv *mhi_netdev_priv = netdev_priv(dev);
+       struct mhi_netdev *mhi_netdev = mhi_netdev_priv->mhi_netdev;
+       struct mhi_device *mhi_dev = mhi_netdev->mhi_dev;
+       int res = 0;
+       struct mhi_skb_priv *tx_priv;
+
+       MSG_VERB("Entered\n");
+
+       tx_priv = (struct mhi_skb_priv *)(skb->cb);
+       tx_priv->mhi_netdev = mhi_netdev;
+       read_lock_bh(&mhi_netdev->pm_lock);
+
+       if (unlikely(!mhi_netdev->enabled)) {
+               /* Only reason interface could be disabled and we get data
+                * is due to an SSR. We do not want to stop the queue and
+                * return error. Instead we will flush all the uplink packets
+                * and return successful
+                */
+               res = NETDEV_TX_OK;
+               dev_kfree_skb_any(skb);
+               goto mhi_xmit_exit;
+       }

Documentation/devicetree/bindings/bus/mhi.txt +30 -0

+static void mhi_netdev_create_debugfs(struct mhi_netdev *mhi_netdev)
+{
+       char node_name[32];
+       int i;
+       const umode_t mode = 0600;
+       struct dentry *file;
+       struct mhi_device *mhi_dev = mhi_netdev->mhi_dev;
+
+       const struct {
+               char *name;
+               u32 *ptr;
+       } debugfs_table[] = {
+               {
+                       "rx_int",
+                       &mhi_netdev->stats.rx_int
+               },
+               {
+                       "tx_full",
+                       &mhi_netdev->stats.tx_full
+               },
+               {
+                       "tx_pkts",
+                       &mhi_netdev->stats.tx_pkts
+               },
+               {
+                       "rx_budget_overflow",
+                       &mhi_netdev->stats.rx_budget_overflow
+               },
+               {
+                       "rx_fragmentation",

Documentation/devicetree/bindings/bus/mhi.txt +9 -0

+static int mhi_netdev_probe(struct mhi_device *mhi_dev,
+                           const struct mhi_device_id *id)
+{
+       int ret;
+       struct mhi_netdev *mhi_netdev;
+       struct device_node *of_node = mhi_dev->dev.of_node;
+       char node_name[32];
+
+       if (!of_node)

Quoted 4 lines · Sujeev Dias

MHI based net device driver is used for transferring IP traffic between host and modem. Driver allows clients to transfer data using standard network interface.

Quoted 8 lines

Signed-off-by Sujeev Dias <[email protected]>

---
  Documentation/devicetree/bindings/bus/mhi.txt |  36 ++
  drivers/bus/Kconfig                           |   1 +
  drivers/bus/mhi/Makefile                      |   2 +-
  drivers/bus/mhi/devices/Kconfig               |  10 +
  drivers/bus/mhi/devices/Makefile              |   1 +
  drivers/bus/mhi/devices/mhi_netdev.c          | 893 ++++++++++++++++++++++++++

Network drivers go into drivers/net/, not into the bus subsystem. You also need to Cc the netdev mailing list for review.

Quoted 39 lines

  6 files changed, 942 insertions(+), 1 deletion(-)

create mode 100644 drivers/bus/mhi/devices/Kconfig create mode 100644 drivers/bus/mhi/devices/Makefile create mode 100644 drivers/bus/mhi/devices/mhi_netdev.c

Documentation/devicetree/bindings/bus/mhi.txt +25 -0

index ea1b620..172ae7b 100644
@@ -139,3 +139,39 @@ mhi_controller {
                 <driver specific properties>
         };
  };
+
+================
+Children Devices
+================
+
+MHI netdev properties
+
+- mhi,chan
+  Usage: required
+  Value type: <string>
+  Definition: Channel name MHI netdev support
 
+- mhi,mru
+  Usage: required
+  Value type: <u32>
+  Definition: Largest packet size interface can receive in bytes.
+
+- mhi,interface-name
+  Usage: optional
+  Value type: <string>
+  Definition: Interface name to be given so clients can identify it
+
+- mhi,recycle-buf
+  Usage: optional
+  Value type: <bool>
+  Definition: Set true if interface support recycling buffers.

Documentation/devicetree/bindings/bus/mhi.txt mhi_controller { · line 177

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt +9 -0

 
+========
+Example:
+========
+
+mhi_rmnet@0 {
+       mhi,chan = "IP_HW0";
+       mhi,interface-name = "rmnet_mhi";
+       mhi,mru = <0x4000>;
+};

Who defines the "IP_HW0" and "rmnet_mhi" strings?

Shouldn't there be a "compatible" property?

Quoted 19 lines

Documentation/devicetree/bindings/bus/mhi.txt +19 -0

+#define MHI_NETDEV_DRIVER_NAME "mhi_netdev"
+#define WATCHDOG_TIMEOUT (30 * HZ)
+
+#ifdef CONFIG_MHI_DEBUG
+
+#define MHI_ASSERT(cond, msg) do { \
+       if (cond) \
+               panic(msg); \
+} while (0)
+
+#define MSG_VERB(fmt, ...) do { \
+       if (mhi_netdev->msg_lvl <= MHI_MSG_LVL_VERBOSE) \
+               pr_err("[D][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)
+
+#define MSG_LOG(fmt, ...) do { \
+       if (mhi_netdev->msg_lvl <= MHI_MSG_LVL_INFO) \
+               pr_err("[I][%s] " fmt, __func__, ##__VA_ARGS__);\
+} while (0)

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 6 lines

Documentation/devicetree/bindings/bus/mhi.txt +6 -0

+
+#define MSG_ERR(fmt, ...) do { \
+       if (mhi_netdev->msg_lvl <= MHI_MSG_LVL_ERROR) \
+               pr_err("[E][%s] " fmt, __func__, ##__VA_ARGS__); \
+} while (0)
+

Please remove these macros and use the normal kernel helpers we have.

Quoted 5 lines

Documentation/devicetree/bindings/bus/mhi.txt +5 -0

+struct mhi_stats {
+       u32 rx_int;
+       u32 tx_full;
+       u32 tx_pkts;
+       u32 rx_budget_overflow;

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt +10 -0

+       u32 rx_frag;
+       u32 alloc_failed;
+};
+
+/* important: do not exceed sk_buf->cb (48 bytes) */
+struct mhi_skb_priv {
+       void *buf;
+       size_t size;
+       struct mhi_netdev *mhi_netdev;
+};

Shouldn't all three members already be part of an skb?

You initialize them as

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 5 lines

Documentation/devicetree/bindings/bus/mhi.txt +5 -0

+       u32 cur_mru = mhi_netdev->mru;
+               skb_priv = (struct mhi_skb_priv *)skb->cb;
+               skb_priv->buf = skb->data;
+               skb_priv->size = cur_mru;
+               skb_priv->mhi_netdev = mhi_netdev;

so I think you can remove the structure completely.

Quoted 17 lines

Documentation/devicetree/bindings/bus/mhi.txt +17 -0

+struct mhi_netdev {
+       int alias;
+       struct mhi_device *mhi_dev;
+       spinlock_t rx_lock;
+       bool enabled;
+       rwlock_t pm_lock; /* state change lock */
+       int (*rx_queue)(struct mhi_netdev *mhi_netdev, gfp_t gfp_t);
+       struct work_struct alloc_work;
+       int wake;
+
+       struct sk_buff_head rx_allocated;
+
+       u32 mru;
+       const char *interface_name;
+       struct napi_struct napi;
+       struct net_device *ndev;
+       struct sk_buff *frag_skb;

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt +10 -0

+       bool recycle_buf;
+
+       struct mhi_stats stats;
+       struct dentry *dentry;
+       enum MHI_DEBUG_LEVEL msg_lvl;
+};
+
+struct mhi_netdev_priv {
+       struct mhi_netdev *mhi_netdev;
+};

Please kill this intermediate structure and use the mhi_netdev directly.

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 2 lines

Documentation/devicetree/bindings/bus/mhi.txt +2 -0

+static struct mhi_driver mhi_netdev_driver;
+static void mhi_netdev_create_debugfs(struct mhi_netdev *mhi_netdev);

Better remove forward declarations and sort the symbols in their natural order.

Quoted 17 lines

Documentation/devicetree/bindings/bus/mhi.txt +17 -0

+static int mhi_netdev_alloc_skb(struct mhi_netdev *mhi_netdev, gfp_t gfp_t)
+{
+       u32 cur_mru = mhi_netdev->mru;
+       struct mhi_device *mhi_dev = mhi_netdev->mhi_dev;
+       struct mhi_skb_priv *skb_priv;
+       int ret;
+       struct sk_buff *skb;
+       int no_tre = mhi_get_no_free_descriptors(mhi_dev, DMA_FROM_DEVICE);
+       int i;
+
+       for (i = 0; i < no_tre; i++) {
+               skb = alloc_skb(cur_mru, gfp_t);
+               if (!skb)
+                       return -ENOMEM;
+
+               read_lock_bh(&mhi_netdev->pm_lock);
+               if (unlikely(!mhi_netdev->enabled)) {

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 4 lines

Documentation/devicetree/bindings/bus/mhi.txt +4 -0

+                       MSG_ERR("Interface not enabled\n");
+                       ret = -EIO;
+                       goto error_queue;
+               }

You shouldn't normally get here when the device is disabled, I think you can remove the pm_lock and enabled members and instead make sure the netdev itself is stopped at that point.

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 4 lines

Documentation/devicetree/bindings/bus/mhi.txt +4 -0

+               spin_lock_bh(&mhi_netdev->rx_lock);
+               ret = mhi_queue_transfer(mhi_dev, DMA_FROM_DEVICE, skb,
+                                        skb_priv->size, MHI_EOT);
+               spin_unlock_bh(&mhi_netdev->rx_lock);

What does this spinlock protect?

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 8 lines

Documentation/devicetree/bindings/bus/mhi.txt +8 -0

+static void mhi_netdev_alloc_work(struct work_struct *work)
+{
+       struct mhi_netdev *mhi_netdev = container_of(work, struct mhi_netdev,
+                                                  alloc_work);
+       /* sleep about 1 sec and retry, that should be enough time
+        * for system to reclaim freed memory back.
+        */
+       const int sleep_ms =  1000;

That is a long time to wait for in the middle of a work function!

Have you tested that it's actually sufficient to make the driver survive an out-of-memory situation?

If you do need it at all, maybe use a delayed work queue that reschedules itself rather than retrying in the work queue.

Quoted 12 lines

Documentation/devicetree/bindings/bus/mhi.txt +12 -0

+static int mhi_netdev_poll(struct napi_struct *napi, int budget)
+{
+       struct net_device *dev = napi->dev;
+       struct mhi_netdev_priv *mhi_netdev_priv = netdev_priv(dev);
+       struct mhi_netdev *mhi_netdev = mhi_netdev_priv->mhi_netdev;
+       struct mhi_device *mhi_dev = mhi_netdev->mhi_dev;
+       int rx_work = 0;
+       int ret;
+
+       MSG_VERB("Entered\n");
+
+       read_lock_bh(&mhi_netdev->pm_lock);

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 7 lines

Documentation/devicetree/bindings/bus/mhi.txt +7 -0

+
+       if (!mhi_netdev->enabled) {
+               MSG_LOG("interface is disabled!\n");
+               napi_complete(napi);
+               read_unlock_bh(&mhi_netdev->pm_lock);
+               return 0;
+       }

Again, this shouldn't be required.

Quoted 24 lines

Documentation/devicetree/bindings/bus/mhi.txt +24 -0

+static int mhi_netdev_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+       struct mhi_netdev_priv *mhi_netdev_priv = netdev_priv(dev);
+       struct mhi_netdev *mhi_netdev = mhi_netdev_priv->mhi_netdev;
+       struct mhi_device *mhi_dev = mhi_netdev->mhi_dev;
+       int res = 0;
+       struct mhi_skb_priv *tx_priv;
+
+       MSG_VERB("Entered\n");
+
+       tx_priv = (struct mhi_skb_priv *)(skb->cb);
+       tx_priv->mhi_netdev = mhi_netdev;
+       read_lock_bh(&mhi_netdev->pm_lock);
+
+       if (unlikely(!mhi_netdev->enabled)) {
+               /* Only reason interface could be disabled and we get data
+                * is due to an SSR. We do not want to stop the queue and
+                * return error. Instead we will flush all the uplink packets
+                * and return successful
+                */
+               res = NETDEV_TX_OK;
+               dev_kfree_skb_any(skb);
+               goto mhi_xmit_exit;
+       }

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt +10 -0

+
+       res = mhi_queue_transfer(mhi_dev, DMA_TO_DEVICE, skb, skb->len,
+                                MHI_EOT);
+       if (res) {
+               MSG_VERB("Failed to queue with reason:%d\n", res);
+               netif_stop_queue(dev);
+               mhi_netdev->stats.tx_full++;
+               res = NETDEV_TX_BUSY;
+               goto mhi_xmit_exit;
+       }

You don't seem to have any limit on the number of queued transfers here. Since this is for a modem, it seems absolutely essential that you keep track of how many packets have been queued but not sent over the air yet.

Please add the necessary calls to netdev_sent_queue() here and netdev_completed_queue() in the mhi_netdev_xfer_ul_cb callback respectively to prevent uncontrolled buffer bloat.

Can you clarify whether mhi_netdev_xfer_ul_cb() is called after the data has been read by the modem and queued again in another buffer, or if it only gets called after we know that the packet has been sent over the air?

Quoted 7 lines

Documentation/devicetree/bindings/bus/mhi.txt +7 -0

+static int mhi_netdev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+       int rc = 0;
+
+       switch (cmd) {
+       default:
+               /* don't fail any IOCTL right now */

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 6 lines

Documentation/devicetree/bindings/bus/mhi.txt +6 -0

+               rc = 0;
+               break;
+       }
+
+       return rc;
+}

That looks wrong. You should return an error for any unknown ioctl, which is the default behavior you get without an ioctl function.

Quoted 30 lines

Documentation/devicetree/bindings/bus/mhi.txt +30 -0

+static void mhi_netdev_create_debugfs(struct mhi_netdev *mhi_netdev)
+{
+       char node_name[32];
+       int i;
+       const umode_t mode = 0600;
+       struct dentry *file;
+       struct mhi_device *mhi_dev = mhi_netdev->mhi_dev;
+
+       const struct {
+               char *name;
+               u32 *ptr;
+       } debugfs_table[] = {
+               {
+                       "rx_int",
+                       &mhi_netdev->stats.rx_int
+               },
+               {
+                       "tx_full",
+                       &mhi_netdev->stats.tx_full
+               },
+               {
+                       "tx_pkts",
+                       &mhi_netdev->stats.tx_pkts
+               },
+               {
+                       "rx_budget_overflow",
+                       &mhi_netdev->stats.rx_budget_overflow
+               },
+               {
+                       "rx_fragmentation",

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 10 lines

Documentation/devicetree/bindings/bus/mhi.txt +10 -0

+                       &mhi_netdev->stats.rx_frag
+               },
+               {
+                       "alloc_failed",
+                       &mhi_netdev->stats.alloc_failed
+               },
+               {
+                       NULL, NULL
+               },
+       };

Please use the regular network statistics interfaces for this, don't roll your own.

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 7 lines

Documentation/devicetree/bindings/bus/mhi.txt +7 -0

+       /* Both tx & rx client handle contain same device info */
+       snprintf(node_name, sizeof(node_name), "%s_%04x_%02u.%02u.%02u_%u",
+                mhi_netdev->interface_name, mhi_dev->dev_id, mhi_dev->domain,
+                mhi_dev->bus, mhi_dev->slot, mhi_netdev->alias);
+
+       if (IS_ERR_OR_NULL(dentry))
+               return;

IS_ERR_OR_NULL() is basically always a bug. debugfs returns NULL only on success when it is completely disabled, which you were trying to handle separately.

Quoted 12 lines

Documentation/devicetree/bindings/bus/mhi.txt +12 -0

+static int mhi_netdev_probe(struct mhi_device *mhi_dev,
+                           const struct mhi_device_id *id)
+{
+       int ret;
+       struct mhi_netdev *mhi_netdev;
+       struct device_node *of_node = mhi_dev->dev.of_node;
+       char node_name[32];
+
+       if (!of_node)
+               return -ENODEV;
+
+       mhi_netdev = devm_kzalloc(&mhi_dev->dev, sizeof(*mhi_netdev),

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 7 lines

Documentation/devicetree/bindings/bus/mhi.txt +7 -0

+                                 GFP_KERNEL);
+       if (!mhi_netdev)
+               return -ENOMEM;
+
+       mhi_netdev->alias = of_alias_get_id(of_node, "mhi_netdev");
+       if (mhi_netdev->alias < 0)
+               return -ENODEV;

Where is that alias documented?

Documentation/devicetree/bindings/bus/mhi.txt

Quoted 4 lines

Documentation/devicetree/bindings/bus/mhi.txt +4 -0

+       ret = of_property_read_string(of_node, "mhi,interface-name",
+                                     &mhi_netdev->interface_name);
+       if (ret)
+               mhi_netdev->interface_name = mhi_netdev_driver.driver.name;

Don't couple the Linux interface name to what the hardware is called.

Arnd

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This module allows user space clients to transfer data between external modem and host using standard file operations.

Signed-off-by Sujeev Dias <[email protected]>

---
 drivers/bus/mhi/devices/Kconfig   |   9 +
 drivers/bus/mhi/devices/Makefile  |   1 +
 drivers/bus/mhi/devices/mhi_uci.c | 662 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 672 insertions(+)
 create mode 100644 drivers/bus/mhi/devices/mhi_uci.c

drivers/bus/mhi/devices/Kconfig +0 -0

index 40f964d..83b9673 100644
@@ -7,4 +7,13 @@ config MHI_NETDEV
          MHI based net device driver for transferring IP traffic
          between host and modem. By enabling this driver, clients

This is the initial skeleton driver for mhi bus stack. MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus. This module will allow host to communicate with external devices that support MHI protocol.

Signed-off-by Sujeev Dias <[email protected]>

Reviewed-by Tony Truong <[email protected]>

Signed-off-by Siddartha Mohanadoss <[email protected]>

---
 Documentation/00-INDEX                        |   2 +
 Documentation/devicetree/bindings/bus/mhi.txt | 258 ++++++++++++
 Documentation/mhi.txt                         | 235 +++++++++++
 drivers/bus/Kconfig                           |   8 +
 drivers/bus/Makefile                          |   1 +
 drivers/bus/mhi/Makefile                      |   6 +
 drivers/bus/mhi/core/Makefile                 |   1 +
 drivers/bus/mhi/core/mhi_init.c               | 538 ++++++++++++++++++++++++++
 drivers/bus/mhi/core/mhi_internal.h           | 238 ++++++++++++
 drivers/bus/mhi/core/mhi_main.c               | 122 ++++++
 include/linux/mhi.h                           | 341 ++++++++++++++++
 include/linux/mod_devicetable.h               |  12 +
 12 files changed, 1762 insertions(+)

 create mode 100644 Documentation/devicetree/bindings/bus/mhi.txt
 create mode 100644 Documentation/mhi.txt
 create mode 100644 drivers/bus/mhi/Makefile
 create mode 100644 drivers/bus/mhi/core/Makefile
 create mode 100644 drivers/bus/mhi/core/mhi_init.c
 create mode 100644 drivers/bus/mhi/core/mhi_internal.h
 create mode 100644 drivers/bus/mhi/core/mhi_main.c
 create mode 100644 include/linux/mhi.h

This is the initial skeleton driver for mhi bus stack. MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus. This module will allow host to communicate with external devices that support MHI protocol.

Signed-off-by Sujeev Dias <[email protected]>

Reviewed-by Tony Truong <[email protected]>

Signed-off-by Siddartha Mohanadoss <[email protected]>

---
 Documentation/00-INDEX                        |   2 +
 Documentation/devicetree/bindings/bus/mhi.txt | 258 ++++++++++++
 Documentation/mhi.txt                         | 235 +++++++++++

+++ b/drivers/bus/mhi/core/mhi_main.c

drivers/bus/mhi/core/mhi_main.c new file +15 -0

@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/list.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/skbuff.h>

include/linux/mhi.h new file +24 -0

+ * @iova_start: IOMMU starting address for data
+ * @iova_stop: IOMMU stop address for data
+ * @fw_image: Firmware image name for normal booting
+ * @edl_image: Firmware image name for emergency download mode
+ * @fbc_download: MHI host needs to do complete image transfer
+ * @rddm_size: RAM dump size that host should allocate for debugging purpose
+ * @sbl_size: SBL image size
+ * @seg_len: BHIe vector size
+ * @fbc_image: Points to firmware image buffer
+ * @rddm_image: Points to RAM dump buffer
+ * @max_chan: Maximum number of channels controller support
+ * @mhi_chan: Points to channel configuration table
+ * @lpm_chans: List of channels that require LPM notifications
+ * @total_ev_rings: Total # of event rings allocated
+ * @hw_ev_rings: Number of hardware event rings
+ * @sw_ev_rings: Number of software event rings
+ * @msi_required: Number of msi required to operate
+ * @msi_allocated: Number of msi allocated by bus master
+ * @irq: base irq # to request
+ * @mhi_event: MHI event ring configurations table
+ * @mhi_cmd: MHI command ring configurations table
+ * @mhi_ctxt: MHI device context, shared memory between host and device
+ * @timeout_ms: Timeout in ms for state transitions
+ * @pm_state: Power management state

include/linux/mhi.h new file +113 -0

+ */
+struct mhi_controller {
+	struct list_head node;
+	struct mhi_device *mhi_dev;
+
+	/* device node for iommu ops */
+	struct device *dev;
+	struct device_node *of_node;
+
+	/* mmio base */
+	void __iomem *regs;
+
+	/* device topology */
+	u32 dev_id;
+	u32 domain;
+	u32 bus;
+	u32 slot;
+
+	/* addressing window */
+	dma_addr_t iova_start;
+	dma_addr_t iova_stop;
+
+	/* fw images */
+	const char *fw_image;
+	const char *edl_image;
+
+	/* mhi host manages downloading entire fbc images */
+	bool fbc_download;
+	size_t rddm_size;
+	size_t sbl_size;
+	size_t seg_len;
+	u32 session_id;
+	u32 sequence_id;
+
+	/* physical channel config data */
+	u32 max_chan;
+	struct mhi_chan *mhi_chan;
+	struct list_head lpm_chans; /* these chan require lpm notification */
+
+	/* physical event config data */
+	u32 total_ev_rings;
+	u32 hw_ev_rings;
+	u32 sw_ev_rings;
+	u32 msi_required;
+	u32 msi_allocated;
+	int *irq; /* interrupt table */
+	struct mhi_event *mhi_event;
+
+	/* cmd rings */
+	struct mhi_cmd *mhi_cmd;
+
+	/* mhi context (shared with device) */
+	struct mhi_ctxt *mhi_ctxt;
+
+	u32 timeout_ms;
+
+	/* caller should grab pm_mutex for suspend/resume operations */
+	struct mutex pm_mutex;
+	bool pre_init;
+	rwlock_t pm_lock;
+	u32 pm_state;
+	u32 ee;
+	u32 dev_state;
+	bool wake_set;
+	atomic_t dev_wake;
+	atomic_t alloc_size;
+	struct list_head transition_list;
+	spinlock_t transition_lock;
+	spinlock_t wlock;
+
+	/* debug counters */
+	u32 M0, M2, M3;
+
+	/* worker for different state transitions */
+	struct work_struct st_worker;
+	struct work_struct fw_worker;
+	struct work_struct syserr_worker;
+	wait_queue_head_t state_event;
+
+	/* shadow functions */
+	void (*status_cb)(struct mhi_controller *mhi_cntrl, void *priv,
+			  enum MHI_CB cb);
+	int (*link_status)(struct mhi_controller *mhi_cntrl, void *priv);
+	void (*wake_get)(struct mhi_controller *mhi_cntrl, bool override);
+	void (*wake_put)(struct mhi_controller *mhi_cntrl, bool override);
+	int (*runtime_get)(struct mhi_controller *mhi_cntrl, void *priv);
+	void (*runtime_put)(struct mhi_controller *mhi_cntrl, void *priv);
+	void (*lpm_disable)(struct mhi_controller *mhi_cntrl, void *priv);
+	void (*lpm_enable)(struct mhi_controller *mhi_cntrl, void *priv);
+	int (*map_single)(struct mhi_controller *mhi_cntrl,
+			  struct mhi_buf_info *buf);
+	void (*unmap_single)(struct mhi_controller *mhi_cntrl,
+			     struct mhi_buf_info *buf);
+
+	/* channel to control DTR messaging */
+	struct mhi_device *dtr_dev;
+
+	/* bounce buffer settings */
+	bool bounce_buf;
+	size_t buffer_len;
+
+	/* controller specific data */
+	void *priv_data;
+	void *log_buf;
+	struct dentry *dentry;
+	struct dentry *parent;
+};
+
+/**
+ * struct mhi_device - mhi device structure associated bind to channel
+ * @dev: Device associated with the channels
+ * @mtu: Maximum # of bytes controller support
+ * @ul_chan_id: MHI channel id for UL transfer

include/linux/mhi.h new file +23 -0

+};
+
+/**
+ * struct mhi_result - Completed buffer information
+ * @buf_addr: Address of data buffer
+ * @dir: Channel direction
+ * @bytes_xfer: # of bytes transferred
+ * @transaction_status: Status of last trasnferred
+ */
+struct mhi_result {
+	void *buf_addr;
+	enum dma_data_direction dir;
+	size_t bytes_xferd;
+	int transaction_status;
+};
+
+/**
+ * struct mhi_driver - mhi driver information
+ * @id_table: NULL terminated channel ID names
+ * @ul_xfer_cb: UL data transfer callback
+ * @dl_xfer_cb: DL data transfer callback
+ * @status_cb: Asynchronous status callback
+ */

This is the initial skeleton driver for mhi bus stack. MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus. This module will allow host to communicate with external devices that support MHI protocol.

Signed-off-by Sujeev Dias <[email protected]>

Reviewed-by Tony Truong <[email protected]>

Signed-off-by Siddartha Mohanadoss <[email protected]>

---
 Documentation/00-INDEX                        |   2 +
 Documentation/devicetree/bindings/bus/mhi.txt | 258 ++++++++++++

 Documentation/mhi.txt                         | 235 +++++++++++
 drivers/bus/Kconfig                           |   8 +
 drivers/bus/Makefile                          |   1 +
 drivers/bus/mhi/Makefile                      |   6 +
 drivers/bus/mhi/core/Makefile                 |   1 +
 drivers/bus/mhi/core/mhi_init.c               | 538 ++++++++++++++++++++++++++
 drivers/bus/mhi/core/mhi_internal.h           | 238 ++++++++++++
 drivers/bus/mhi/core/mhi_main.c               | 122 ++++++
 include/linux/mhi.h                           | 341 ++++++++++++++++
 include/linux/mod_devicetable.h               |  12 +
 12 files changed, 1762 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/mhi.txt
 create mode 100644 Documentation/mhi.txt
 create mode 100644 drivers/bus/mhi/Makefile
 create mode 100644 drivers/bus/mhi/core/Makefile
 create mode 100644 drivers/bus/mhi/core/mhi_init.c
 create mode 100644 drivers/bus/mhi/core/mhi_internal.h
 create mode 100644 drivers/bus/mhi/core/mhi_main.c
 create mode 100644 include/linux/mhi.h

Documentation/devicetree/bindings/bus/mhi.txt new file +2 -0

new file mode 100644
index 0000000..19deb84
@@ -0,0 +1,258 @@
+MHI Host Interface
+

Documentation/devicetree/bindings/bus/mhi.txt new file +11 -0

+
+- mhi,buffer-len
+  Usage: optional
+  Value type: <bool>
+  Definition: MHI automatically pre-allocate buffers for some channel.
+	Set the length of buffer size to allocate. If not default
+	size MHI_MAX_MTU will be used.
+
+============================
+mhi channel node properties:
+============================

Documentation/devicetree/bindings/bus/mhi.txt new file +75 -0

+
+- mhi,num-elements
+  Usage: optional
+  Value type: <u32>
+  Definition: Number of elements transfer ring support
+
+- mhi,event-ring
+  Usage: required
+  Value type: <u32>
+  Definition: Event ring index associated with this channel
+
+- mhi,chan-dir
+  Usage: required
+  Value type: <u32>
+  Definition: Channel direction as defined by enum dma_data_direction
+	0 = Bidirectional data transfer
+	1 = UL data transfer
+	2 = DL data transfer
+	3 = No direction, not a regular data transfer channel
+
+- mhi,ee
+  Usage: required
+  Value type: <u32>
+  Definition: Channel execution enviornment as defined by enum MHI_EE
+	1 = Bootloader stage
+	2 = AMSS mode
+
+- mhi,pollcfg
+  Usage: optional
+  Value type: <u32>
+  Definition: MHI poll configuration, valid only when burst mode is enabled
+	0 = Use default (device specific) polling configuration
+	For UL channels, value specifies the timer to poll MHI context in
+	milliseconds.
+	For DL channels, the threshold to poll the MHI context in multiple of
+	eight ring element.
+
+- mhi,data-type
+  Usage: required
+  Value type: <u32>
+  Definition: Data transfer type accepted as defined by enum MHI_XFER_TYPE
+	0 = accept cpu address for buffer
+	1 = accept skb
+	2 = accept scatterlist
+	3 = offload channel, does not accept any transfer type
+
+- mhi,doorbell-mode
+  Usage: required
+  Value type: <u32>
+  Definition: Channel doorbell mode configuration as defined by enum
+	MHI_BRSTMODE
+	2 = burst mode disabled
+	3 = burst mode enabled
+
+- mhi,lpm-notify
+  Usage: optional
+  Value type: <bool>
+  Definition: This channel master require low power mode enter and exit
+  notifications from mhi bus master.
+
+- mhi,offload-chan
+  Usage: optional
+  Value type: <bool>
+  Definition: Client managed channel, MHI host only involved in setting up
+	the data path, not involved in active data path.
+
+- mhi,db-mode-switch
+  Usage: optional
+  Value type: <bool>
+  Definition: Must switch to doorbell mode whenever MHI M0 state transition
+	happens.
+
+- mhi,auto-queue
+  Usage: optional
+  Value type: <bool>

Documentation/devicetree/bindings/bus/mhi.txt new file +50 -0

+
+==========================
+mhi event node properties:
+==========================
+
+- mhi,num-elements
+  Usage: required
+  Value type: <u32>
+  Definition: Number of elements event ring support
+
+- mhi,intmod
+  Usage: required
+  Value type: <u32>
+  Definition: interrupt moderation time in ms
+
+- mhi,msi
+  Usage: required
+  Value type: <u32>
+  Definition: MSI associated with this event ring
+
+- mhi,chan
+  Usage: optional
+  Value type: <u32>
+  Definition: Dedicated channel number, if it's a dedicated event ring
+
+- mhi,priority
+  Usage: required
+  Value type: <u32>
+  Definition: Event ring priority, set to 1 for now
+
+- mhi,brstmode
+  Usage: required
+  Value type: <u32>
+  Definition: Event doorbell mode configuration as defined by
+	enum MHI_BRSTMODE
+		2 = burst mode disabled
+		3 = burst mode enabled
+
+- mhi,data-type
+  Usage: optional
+  Value type: <u32>
+  Definition: Type of data this event ring will process as defined
+	by enum mhi_er_data_type
+		0 = process data packets (default)
+		1 = process mhi control packets
+
+- mhi,hw-ev
+  Usage: optional
+  Value type: <bool>
+  Definition: Event ring associated with hardware channels

This is the initial skeleton driver for mhi bus stack. MHI Host Interface is a communication protocol to be used by the host to control and communcate with modem over a high speed peripheral bus.

This module will allow host to communicate with external devices that support MHI protocol.

Signed-off-by Sujeev Dias <[email protected]>

Reviewed-by Tony Truong <[email protected]>

Signed-off-by Siddartha Mohanadoss <[email protected]>

---
 Documentation/00-INDEX                        |   2 +
 Documentation/devicetree/bindings/bus/mhi.txt | 258 ++++++++++++
 Documentation/mhi.txt                         | 235 +++++++++++
 drivers/bus/Kconfig                           |   8 +
 drivers/bus/Makefile                          |   1 +
 drivers/bus/mhi/Makefile                      |   6 +
 drivers/bus/mhi/core/Makefile                 |   1 +
 drivers/bus/mhi/core/mhi_init.c               | 538 ++++++++++++++++++++++++++
 drivers/bus/mhi/core/mhi_internal.h           | 238 ++++++++++++
 drivers/bus/mhi/core/mhi_main.c               | 122 ++++++
 include/linux/mhi.h                           | 341 ++++++++++++++++
 include/linux/mod_devicetable.h               |  12 +
 12 files changed, 1762 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/mhi.txt
 create mode 100644 Documentation/mhi.txt
 create mode 100644 drivers/bus/mhi/Makefile
 create mode 100644 drivers/bus/mhi/core/Makefile
 create mode 100644 drivers/bus/mhi/core/mhi_init.c
 create mode 100644 drivers/bus/mhi/core/mhi_internal.h
 create mode 100644 drivers/bus/mhi/core/mhi_main.c
 create mode 100644 include/linux/mhi.h

Documentation/devicetree/bindings/bus/mhi.txt new file +0 -0

new file mode 100644
index 0000000..19deb84

Documentation/devicetree/bindings/bus/mhi.txt new file +59 -0

+high speed peripheral bus.
+
+==============
+Node Structure
+==============
+
+Main node properties:
+
+- mhi,max-channels
+  Usage: required
+  Value type: <u32>
+  Definition: Maximum number of channels supported by this controller
+
+- mhi,timeout
+  Usage: optional
+  Value type: <u32>
+  Definition: Maximum timeout in ms wait for state and cmd completion
+
+- mhi,use-bb
+  Usage: optional
+  Value type: <bool>
+  Definition: Set true, if PCIe controller does not have full access to host
+	DDR, and we're using a dedicated memory pool like cma, or
+	carveout pool. Pool must support atomic allocation.
+
+- mhi,buffer-len
+  Usage: optional
+  Value type: <bool>
+  Definition: MHI automatically pre-allocate buffers for some channel.
+	Set the length of buffer size to allocate. If not default
+	size MHI_MAX_MTU will be used.
+
+============================
+mhi channel node properties:
+============================
+
+- reg
+  Usage: required
+  Value type: <u32>
+  Definition: physical channel number
+
+- label
+  Usage: required
+  Value type: <string>
+  Definition: given name for the channel
+
+- mhi,num-elements
+  Usage: optional
+  Value type: <u32>
+  Definition: Number of elements transfer ring support
+
+- mhi,event-ring
+  Usage: required
+  Value type: <u32>
+  Definition: Event ring index associated with this channel
+
+- mhi,chan-dir
+  Usage: required
+  Value type: <u32>

Documentation/devicetree/bindings/bus/mhi.txt new file +51 -0

+	1 = Bootloader stage
+	2 = AMSS mode
+
+- mhi,pollcfg
+  Usage: optional
+  Value type: <u32>
+  Definition: MHI poll configuration, valid only when burst mode is enabled
+	0 = Use default (device specific) polling configuration
+	For UL channels, value specifies the timer to poll MHI context in
+	milliseconds.
+	For DL channels, the threshold to poll the MHI context in multiple of
+	eight ring element.
+
+- mhi,data-type
+  Usage: required
+  Value type: <u32>
+  Definition: Data transfer type accepted as defined by enum MHI_XFER_TYPE
+	0 = accept cpu address for buffer
+	1 = accept skb
+	2 = accept scatterlist
+	3 = offload channel, does not accept any transfer type
+
+- mhi,doorbell-mode
+  Usage: required
+  Value type: <u32>
+  Definition: Channel doorbell mode configuration as defined by enum
+	MHI_BRSTMODE
+	2 = burst mode disabled
+	3 = burst mode enabled
+
+- mhi,lpm-notify
+  Usage: optional
+  Value type: <bool>
+  Definition: This channel master require low power mode enter and exit
+  notifications from mhi bus master.
+
+- mhi,offload-chan
+  Usage: optional
+  Value type: <bool>
+  Definition: Client managed channel, MHI host only involved in setting up
+	the data path, not involved in active data path.
+
+- mhi,db-mode-switch
+  Usage: optional
+  Value type: <bool>
+  Definition: Must switch to doorbell mode whenever MHI M0 state transition
+	happens.
+
+- mhi,auto-queue
+  Usage: optional
+  Value type: <bool>

Documentation/devicetree/bindings/bus/mhi.txt new file +125 -0

+
+==========================
+mhi event node properties:
+==========================
+
+- mhi,num-elements
+  Usage: required
+  Value type: <u32>
+  Definition: Number of elements event ring support
+
+- mhi,intmod
+  Usage: required
+  Value type: <u32>
+  Definition: interrupt moderation time in ms
+
+- mhi,msi
+  Usage: required
+  Value type: <u32>
+  Definition: MSI associated with this event ring
+
+- mhi,chan
+  Usage: optional
+  Value type: <u32>
+  Definition: Dedicated channel number, if it's a dedicated event ring
+
+- mhi,priority
+  Usage: required
+  Value type: <u32>
+  Definition: Event ring priority, set to 1 for now
+
+- mhi,brstmode
+  Usage: required
+  Value type: <u32>
+  Definition: Event doorbell mode configuration as defined by
+	enum MHI_BRSTMODE
+		2 = burst mode disabled
+		3 = burst mode enabled
+
+- mhi,data-type
+  Usage: optional
+  Value type: <u32>
+  Definition: Type of data this event ring will process as defined
+	by enum mhi_er_data_type
+		0 = process data packets (default)
+		1 = process mhi control packets
+
+- mhi,hw-ev
+  Usage: optional
+  Value type: <bool>
+  Definition: Event ring associated with hardware channels
+
+- mhi,client-manage
+  Usage: optional
+  Value type: <bool>
+  Definition: Client manages the event ring (use by napi_poll)
+
+- mhi,offload
+  Usage: optional
+  Value type: <bool>
+  Definition: Event ring associated with offload channel
+
+
+Children node properties:
+
+MHI drivers that require DT can add driver specific information as a child node.
+
+- mhi,chan
+  Usage: Required
+  Value type: <string>
+  Definition: Channel name
+
+========
+Example:
+========
+mhi_controller {
+	mhi,max-channels = <105>;
+
+	mhi_chan@0 {
+		reg = <0>;
+		label = "LOOPBACK";
+		mhi,num-elements = <64>;
+		mhi,event-ring = <2>;
+		mhi,chan-dir = <1>;
+		mhi,data-type = <0>;
+		mhi,doorbell-mode = <2>;
+		mhi,ee = <2>;
+	};
+
+	mhi_chan@1 {
+		reg = <1>;
+		label = "LOOPBACK";
+		mhi,num-elements = <64>;
+		mhi,event-ring = <2>;
+		mhi,chan-dir = <2>;
+		mhi,data-type = <0>;
+		mhi,doorbell-mode = <2>;
+		mhi,ee = <2>;
+	};
+
+	mhi_event@0 {
+		mhi,num-elements = <32>;
+		mhi,intmod = <1>;
+		mhi,msi = <1>;
+		mhi,chan = <0>;
+		mhi,priority = <1>;
+		mhi,bstmode = <2>;
+		mhi,data-type = <1>;
+	};
+
+	mhi_event@1 {
+		mhi,num-elements = <256>;
+		mhi,intmod = <1>;
+		mhi,msi = <2>;
+		mhi,chan = <0>;
+		mhi,priority = <1>;
+		mhi,bstmode = <2>;
+	};
+
+	mhi,timeout = <500>;
+
+	children_node {
+		mhi,chan = "LOOPBACK"
+		<driver specific properties>
+	};
+};

Documentation/mhi.txt new file +0 -0

new file mode 100644
index 0000000..1c501f1

Documentation/mhi.txt new file +6 -0

+Data transfers are invoked from the device.
+
+All data structures used by MHI are in the host system memory. Using PCIe
+interface, the device accesses those data structures. MHI data structures and
+data buffers in the host system memory regions are mapped for device.
+

Documentation/mhi.txt new file +48 -0

+
+MHI control registers : Access to MHI configurations registers
+(struct mhi_controller.regs).
+
+MHI BHI register: Boot host interface registers (struct mhi_controller.bhi) used
+for firmware download before MHI initialization.
+
+Channel db array : Doorbell registers (struct mhi_chan.tre_ring.db_addr) used by
+host to notify device there is new work to do.
+
+Event db array : Associated with event context array
+(struct mhi_event.ring.db_addr), host uses to notify device free events are
+available.
+
+Data structures
+---------------
+Host memory : Directly accessed by the host to manage the MHI data structures
+and buffers. The device accesses the host memory over the PCIe interface.
+
+Channel context array : All channel configurations are organized in channel
+context data array.
+
+struct __packed mhi_chan_ctxt;
+struct mhi_ctxt.chan_ctxt;
+
+Transfer rings : Used by host to schedule work items for a channel and organized
+as a circular queue of transfer descriptors (TD).
+
+struct __packed mhi_tre;
+struct mhi_chan.tre_ring;
+
+Event context array : All event configurations are organized in event context
+data array.
+
+struct mhi_ctxt.er_ctxt;
+struct __packed mhi_event_ctxt;
+
+Event rings: Used by device to send completion and state transition messages to
+host
+
+struct mhi_event.ring;
+struct __packed mhi_tre;
+
+Command context array: All command configurations are organized in command
+context data array.
+
+struct __packed mhi_cmd_ctxt;
+struct mhi_ctxt.cmd_ctxt;

Documentation/mhi.txt new file +5 -0

+6. Ring the associated channel DB.
+
+Event rings
+-----------
+Events from the device to host are organized in event rings and defined in event