Re: [PATCH 02/16] bus: mhi: core: Add support for registering MHI controllers

[Jeffrey Hugo]

On 1/23/2020 10:05 AM, Jeffrey Hugo wrote:
> On 1/23/2020 4:18 AM, Manivannan Sadhasivam wrote:
> > This commit adds support for registering MHI controller drivers with
> > the MHI stack. MHI controller drivers manages the interaction with the
> > MHI client devices such as the external modems and WiFi chipsets. They
> > are also the MHI bus master in charge of managing the physical link
> > between the host and client device.
> >
> > This is based on the patch submitted by Sujeev Dias:
> > https://lkml.org/lkml/2018/7/9/987
> >
> > Signed-off-by: Sujeev Dias <sdias@xxxxxxxxxxxxxx>
> > Signed-off-by: Siddartha Mohanadoss <smohanad@xxxxxxxxxxxxxx>
> > [jhugo: added static config for controllers and fixed several bugs]
> > Signed-off-by: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>
> > [mani: removed DT dependency, splitted and cleaned up for upstream]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> > Â drivers/bus/KconfigÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
> > Â drivers/bus/MakefileÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 3 +
> > Â drivers/bus/mhi/KconfigÂÂÂÂÂÂÂÂ |Â 14 +
> > Â drivers/bus/mhi/MakefileÂÂÂÂÂÂÂ |ÂÂ 2 +
> > Â drivers/bus/mhi/core/MakefileÂÂ |ÂÂ 3 +
> > Â drivers/bus/mhi/core/init.cÂÂÂÂ | 404 +++++++++++++++++++++++++++++
> > Â drivers/bus/mhi/core/internal.h | 169 ++++++++++++
> > Â include/linux/mhi.hÂÂÂÂÂÂÂÂÂÂÂÂ | 438 ++++++++++++++++++++++++++++++++
> > Â include/linux/mod_devicetable.h |Â 12 +
> > Â 9 files changed, 1046 insertions(+)
> > Â create mode 100644 drivers/bus/mhi/Kconfig
> > Â create mode 100644 drivers/bus/mhi/Makefile
> > Â create mode 100644 drivers/bus/mhi/core/Makefile
> > Â create mode 100644 drivers/bus/mhi/core/init.c
> > Â create mode 100644 drivers/bus/mhi/core/internal.h
> > Â create mode 100644 include/linux/mhi.h
> >
> > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> > index 50200d1c06ea..383934e54786 100644
> > --- a/drivers/bus/Kconfig
> > +++ b/drivers/bus/Kconfig
> > @@ -202,5 +202,6 @@ config DA8XX_MSTPRI
> > ÂÂÂÂÂÂÂ peripherals.
> > Â source "drivers/bus/fsl-mc/Kconfig"
> > +source "drivers/bus/mhi/Kconfig"
> > Â endmenu
> > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> > index 1320bcf9fa9d..05f32cd694a4 100644
> > --- a/drivers/bus/Makefile
> > +++ b/drivers/bus/Makefile
> > @@ -34,3 +34,6 @@ obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)ÂÂÂ += 
> > uniphier-system-bus.o
> > Â obj-$(CONFIG_VEXPRESS_CONFIG)ÂÂÂ += vexpress-config.o
> > Â obj-$(CONFIG_DA8XX_MSTPRI)ÂÂÂ += da8xx-mstpri.o
> > +
> > +# MHI
> > +obj-$(CONFIG_MHI_BUS)ÂÂÂÂÂÂÂ += mhi/
> > diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
> > new file mode 100644
> > index 000000000000..a8bd9bd7db7c
> > --- /dev/null
> > +++ b/drivers/bus/mhi/Kconfig
> > @@ -0,0 +1,14 @@
> > +# SPDX-License-Identifier: GPL-2.0
>
> first time I noticed this, although I suspect this will need to be 
> corrected "everywhere" -
> Per the SPDX website, the "GPL-2.0" label is deprecated. It's 
> replacement is "GPL-2.0-only".
> I think all instances should be updated to "GPL-2.0-only"
>
> > +#
> > +# MHI bus
> > +#
> > +# Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > +#
> > +
> > +config MHI_BUS
> > +ÂÂÂÂÂÂ tristate "Modem Host Interface (MHI) bus"
> > +ÂÂÂÂÂÂ help
> > +ÂÂÂÂ Bus driver for MHI protocol. Modem Host Interface (MHI) is a
> > +ÂÂÂÂ communication protocol used by the host processors to control
> > +ÂÂÂÂ and communicate with modem devices over a high speed peripheral
> > +ÂÂÂÂ bus or shared memory.
> > diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile
> > new file mode 100644
> > index 000000000000..19e6443b72df
> > --- /dev/null
> > +++ b/drivers/bus/mhi/Makefile
> > @@ -0,0 +1,2 @@
> > +# core layer
> > +obj-y += core/
> > diff --git a/drivers/bus/mhi/core/Makefile 
> > b/drivers/bus/mhi/core/Makefile
> > new file mode 100644
> > index 000000000000..2db32697c67f
> > --- /dev/null
> > +++ b/drivers/bus/mhi/core/Makefile
> > @@ -0,0 +1,3 @@
> > +obj-$(CONFIG_MHI_BUS) := mhi.o
> > +
> > +mhi-y := init.o
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > new file mode 100644
> > index 000000000000..5b817ec250e0
> > --- /dev/null
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -0,0 +1,404 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > + *
> > + */
> > +
> > +#define dev_fmt(fmt) "MHI: " fmt
> > +
> > +#include <linux/device.h>
> > +#include <linux/dma-direction.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/list.h>
> > +#include <linux/mhi.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/wait.h>
> > +#include "internal.h"
> > +
> > +static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂ struct mhi_controller_config *config)
> > +{
> > +ÂÂÂ int i, num;
> > +ÂÂÂ struct mhi_event *mhi_event;
> > +ÂÂÂ struct mhi_event_config *event_cfg;
> > +
> > +ÂÂÂ num = config->num_events;
> > +ÂÂÂ mhi_cntrl->total_ev_rings = num;
> > +ÂÂÂ mhi_cntrl->mhi_event = kcalloc(num, sizeof(*mhi_cntrl->mhi_event),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GFP_KERNEL);
> > +ÂÂÂ if (!mhi_cntrl->mhi_event)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ /* Populate event ring */
> > +ÂÂÂ mhi_event = mhi_cntrl->mhi_event;
> > +ÂÂÂ for (i = 0; i < num; i++) {
> > +ÂÂÂÂÂÂÂ event_cfg = &config->event_cfg[i];
> > +
> > +ÂÂÂÂÂÂÂ mhi_event->er_index = i;
> > +ÂÂÂÂÂÂÂ mhi_event->ring.elements = event_cfg->num_elements;
> > +ÂÂÂÂÂÂÂ mhi_event->intmod = event_cfg->irq_moderation_ms;
> > +ÂÂÂÂÂÂÂ mhi_event->irq = event_cfg->irq;
> > +
> > +ÂÂÂÂÂÂÂ if (event_cfg->channel != U32_MAX) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ /* This event ring has a dedicated channel */
> > +ÂÂÂÂÂÂÂÂÂÂÂ mhi_event->chan = event_cfg->channel;
> > +ÂÂÂÂÂÂÂÂÂÂÂ if (mhi_event->chan >= mhi_cntrl->max_chan) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(mhi_cntrl->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Event Ring channel not available\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto error_ev_cfg;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂ mhi_event->mhi_chan =
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &mhi_cntrl->mhi_chan[mhi_event->chan];
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ /* Priority is fixed to 1 for now */
> > +ÂÂÂÂÂÂÂ mhi_event->priority = 1;
> > +
> > +ÂÂÂÂÂÂÂ mhi_event->db_cfg.brstmode = event_cfg->mode;
> > +ÂÂÂÂÂÂÂ if (MHI_INVALID_BRSTMODE(mhi_event->db_cfg.brstmode))
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto error_ev_cfg;
> > +
> > +ÂÂÂÂÂÂÂ mhi_event->data_type = event_cfg->data_type;
> > +
> > +ÂÂÂÂÂÂÂ mhi_event->hw_ring = event_cfg->hardware_event;
> > +ÂÂÂÂÂÂÂ if (mhi_event->hw_ring)
> > +ÂÂÂÂÂÂÂÂÂÂÂ mhi_cntrl->hw_ev_rings++;
> > +ÂÂÂÂÂÂÂ else
> > +ÂÂÂÂÂÂÂÂÂÂÂ mhi_cntrl->sw_ev_rings++;
> > +
> > +ÂÂÂÂÂÂÂ mhi_event->cl_manage = event_cfg->client_managed;
> > +ÂÂÂÂÂÂÂ mhi_event->offload_ev = event_cfg->offload_channel;
> > +ÂÂÂÂÂÂÂ mhi_event++;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ /* We need IRQ for each event ring + additional one for BHI */
> > +ÂÂÂ mhi_cntrl->nr_irqs_req = mhi_cntrl->total_ev_rings + 1;
> > +
> > +ÂÂÂ return 0;
> > +
> > +error_ev_cfg:
> > +
> > +ÂÂÂ kfree(mhi_cntrl->mhi_event);
> > +ÂÂÂ return -EINVAL;
> > +}
> > +
> > +static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂ struct mhi_controller_config *config)
> > +{
> > +ÂÂÂ int i;
> > +ÂÂÂ u32 chan;
> > +ÂÂÂ struct mhi_channel_config *ch_cfg;
> > +
> > +ÂÂÂ mhi_cntrl->max_chan = config->max_channels;
> > +
> > +ÂÂÂ /*
> > +ÂÂÂÂ * The allocation of MHI channels can exceed 32KB in some scenarios,
> > +ÂÂÂÂ * so to avoid any memory possible allocation failures, vzalloc is
> > +ÂÂÂÂ * used here
> > +ÂÂÂÂ */
> > +ÂÂÂ mhi_cntrl->mhi_chan = vzalloc(mhi_cntrl->max_chan *
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(*mhi_cntrl->mhi_chan));
> > +ÂÂÂ if (!mhi_cntrl->mhi_chan)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ INIT_LIST_HEAD(&mhi_cntrl->lpm_chans);
> > +
> > +ÂÂÂ /* Populate channel configurations */
> > +ÂÂÂ for (i = 0; i < config->num_channels; i++) {
> > +ÂÂÂÂÂÂÂ struct mhi_chan *mhi_chan;
> > +
> > +ÂÂÂÂÂÂÂ ch_cfg = &config->ch_cfg[i];
> > +
> > +ÂÂÂÂÂÂÂ chan = ch_cfg->num;
> > +ÂÂÂÂÂÂÂ if (chan >= mhi_cntrl->max_chan) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(mhi_cntrl->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Channel %d not available\n", chan);
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto error_chan_cfg;
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ mhi_chan = &mhi_cntrl->mhi_chan[chan];
> > +ÂÂÂÂÂÂÂ mhi_chan->name = ch_cfg->name;
> > +ÂÂÂÂÂÂÂ mhi_chan->chan = chan;
> > +
> > +ÂÂÂÂÂÂÂ mhi_chan->tre_ring.elements = ch_cfg->num_elements;
> > +ÂÂÂÂÂÂÂ if (!mhi_chan->tre_ring.elements)
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto error_chan_cfg;
> > +
> > +ÂÂÂÂÂÂÂ /*
> > +ÂÂÂÂÂÂÂÂ * For some channels, local ring length should be bigger than
> > +ÂÂÂÂÂÂÂÂ * the transfer ring length due to internal logical channels
> > +ÂÂÂÂÂÂÂÂ * in device. So host can queue much more buffers than transfer
> > +ÂÂÂÂÂÂÂÂ * ring length. Example, RSC channels should have a larger local
> > +ÂÂÂÂÂÂÂÂ * channel length than transfer ring length.
> > +ÂÂÂÂÂÂÂÂ */
> > +ÂÂÂÂÂÂÂ mhi_chan->buf_ring.elements = ch_cfg->local_elements;
> > +ÂÂÂÂÂÂÂ if (!mhi_chan->buf_ring.elements)
> > +ÂÂÂÂÂÂÂÂÂÂÂ mhi_chan->buf_ring.elements = mhi_chan->tre_ring.elements;
> > +ÂÂÂÂÂÂÂ mhi_chan->er_index = ch_cfg->event_ring;
> > +ÂÂÂÂÂÂÂ mhi_chan->dir = ch_cfg->dir;
> > +
> > +ÂÂÂÂÂÂÂ /*
> > +ÂÂÂÂÂÂÂÂ * For most channels, chtype is identical to channel directions.
> > +ÂÂÂÂÂÂÂÂ * So, if it is not defined then assign channel direction to
> > +ÂÂÂÂÂÂÂÂ * chtype
> > +ÂÂÂÂÂÂÂÂ */
> > +ÂÂÂÂÂÂÂ mhi_chan->type = ch_cfg->type;
> > +ÂÂÂÂÂÂÂ if (!mhi_chan->type)
> > +ÂÂÂÂÂÂÂÂÂÂÂ mhi_chan->type = (enum mhi_ch_type)mhi_chan->dir;
> > +
> > +ÂÂÂÂÂÂÂ mhi_chan->ee_mask = ch_cfg->ee_mask;
> > +
> > +ÂÂÂÂÂÂÂ mhi_chan->db_cfg.pollcfg = ch_cfg->pollcfg;
> > +ÂÂÂÂÂÂÂ mhi_chan->xfer_type = ch_cfg->data_type;
> > +
> > +ÂÂÂÂÂÂÂ mhi_chan->lpm_notify = ch_cfg->lpm_notify;
> > +ÂÂÂÂÂÂÂ mhi_chan->offload_ch = ch_cfg->offload_channel;
> > +ÂÂÂÂÂÂÂ mhi_chan->db_cfg.reset_req = ch_cfg->doorbell_mode_switch;
> > +ÂÂÂÂÂÂÂ mhi_chan->pre_alloc = ch_cfg->auto_queue;
> > +ÂÂÂÂÂÂÂ mhi_chan->auto_start = ch_cfg->auto_start;
> > +
> > +ÂÂÂÂÂÂÂ /*
> > +ÂÂÂÂÂÂÂÂ * If MHI host allocates buffers, then the channel direction
> > +ÂÂÂÂÂÂÂÂ * should be DMA_FROM_DEVICE and the buffer type should be
> > +ÂÂÂÂÂÂÂÂ * MHI_BUF_RAW
> > +ÂÂÂÂÂÂÂÂ */
> > +ÂÂÂÂÂÂÂ if (mhi_chan->pre_alloc && (mhi_chan->dir != DMA_FROM_DEVICE ||
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mhi_chan->xfer_type != MHI_BUF_RAW)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(mhi_cntrl->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Invalid channel configuration\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto error_chan_cfg;
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ /*
> > +ÂÂÂÂÂÂÂÂ * Bi-directional and direction less channel must be an
> > +ÂÂÂÂÂÂÂÂ * offload channel
> > +ÂÂÂÂÂÂÂÂ */
> > +ÂÂÂÂÂÂÂ if ((mhi_chan->dir == DMA_BIDIRECTIONAL ||
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ mhi_chan->dir == DMA_NONE) && !mhi_chan->offload_ch) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(mhi_cntrl->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Invalid channel configuration\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto error_chan_cfg;
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ if (!mhi_chan->offload_ch) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ mhi_chan->db_cfg.brstmode = ch_cfg->doorbell;
> > +ÂÂÂÂÂÂÂÂÂÂÂ if (MHI_INVALID_BRSTMODE(mhi_chan->db_cfg.brstmode)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(mhi_cntrl->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Invalid Door bell mode\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto error_chan_cfg;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ mhi_chan->configured = true;
> > +
> > +ÂÂÂÂÂÂÂ if (mhi_chan->lpm_notify)
> > +ÂÂÂÂÂÂÂÂÂÂÂ list_add_tail(&mhi_chan->node, &mhi_cntrl->lpm_chans);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ return 0;
> > +
> > +error_chan_cfg:
> > +ÂÂÂ vfree(mhi_cntrl->mhi_chan);
> > +
> > +ÂÂÂ return -EINVAL;
> > +}
> > +
> > +static int parse_config(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂ struct mhi_controller_config *config)
> > +{
> > +ÂÂÂ int ret;
> > +
> > +ÂÂÂ /* Parse MHI channel configuration */
> > +ÂÂÂ ret = parse_ch_cfg(mhi_cntrl, config);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ /* Parse MHI event configuration */
> > +ÂÂÂ ret = parse_ev_cfg(mhi_cntrl, config);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ goto error_ev_cfg;
> > +
> > +ÂÂÂ mhi_cntrl->timeout_ms = config->timeout_ms;
> > +ÂÂÂ if (!mhi_cntrl->timeout_ms)
> > +ÂÂÂÂÂÂÂ mhi_cntrl->timeout_ms = MHI_TIMEOUT_MS;
> > +
> > +ÂÂÂ mhi_cntrl->bounce_buf = config->use_bounce_buf;
> > +ÂÂÂ mhi_cntrl->buffer_len = config->buf_len;
> > +ÂÂÂ if (!mhi_cntrl->buffer_len)
> > +ÂÂÂÂÂÂÂ mhi_cntrl->buffer_len = MHI_MAX_MTU;
> > +
> > +ÂÂÂ return 0;
> > +
> > +error_ev_cfg:
> > +ÂÂÂ vfree(mhi_cntrl->mhi_chan);
> > +
> > +ÂÂÂ return ret;
> > +}
> > +
> > +int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct mhi_controller_config *config)
> > +{
> > +ÂÂÂ int ret;
> > +ÂÂÂ int i;
> > +ÂÂÂ struct mhi_event *mhi_event;
> > +ÂÂÂ struct mhi_chan *mhi_chan;
> > +ÂÂÂ struct mhi_cmd *mhi_cmd;
> > +ÂÂÂ struct mhi_device *mhi_dev;
> > +

You need a null check on mhi_cntrl right here, otherwise you could cause 
a panic with the following if.

> > +ÂÂÂ if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ if (!mhi_cntrl->status_cb || !mhi_cntrl->link_status)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ ret = parse_config(mhi_cntrl, config);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ mhi_cntrl->mhi_cmd = kcalloc(NR_OF_CMD_RINGS,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(*mhi_cntrl->mhi_cmd), GFP_KERNEL);
> > +ÂÂÂ if (!mhi_cntrl->mhi_cmd) {
> > +ÂÂÂÂÂÂÂ ret = -ENOMEM;
> > +ÂÂÂÂÂÂÂ goto error_alloc_cmd;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ INIT_LIST_HEAD(&mhi_cntrl->transition_list);
> > +ÂÂÂ spin_lock_init(&mhi_cntrl->transition_lock);
> > +ÂÂÂ spin_lock_init(&mhi_cntrl->wlock);
> > +ÂÂÂ init_waitqueue_head(&mhi_cntrl->state_event);
> > +
> > +ÂÂÂ mhi_cmd = mhi_cntrl->mhi_cmd;
> > +ÂÂÂ for (i = 0; i < NR_OF_CMD_RINGS; i++, mhi_cmd++)
> > +ÂÂÂÂÂÂÂ spin_lock_init(&mhi_cmd->lock);
> > +
> > +ÂÂÂ mhi_event = mhi_cntrl->mhi_event;
> > +ÂÂÂ for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
> > +ÂÂÂÂÂÂÂ /* Skip for offload events */
> > +ÂÂÂÂÂÂÂ if (mhi_event->offload_ev)
> > +ÂÂÂÂÂÂÂÂÂÂÂ continue;
> > +
> > +ÂÂÂÂÂÂÂ mhi_event->mhi_cntrl = mhi_cntrl;
> > +ÂÂÂÂÂÂÂ spin_lock_init(&mhi_event->lock);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ mhi_chan = mhi_cntrl->mhi_chan;
> > +ÂÂÂ for (i = 0; i < mhi_cntrl->max_chan; i++, mhi_chan++) {
> > +ÂÂÂÂÂÂÂ mutex_init(&mhi_chan->mutex);
> > +ÂÂÂÂÂÂÂ init_completion(&mhi_chan->completion);
> > +ÂÂÂÂÂÂÂ rwlock_init(&mhi_chan->lock);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ /* Register controller with MHI bus */
> > +ÂÂÂ mhi_dev = mhi_alloc_device(mhi_cntrl);
> > +ÂÂÂ if (IS_ERR(mhi_dev)) {
> > +ÂÂÂÂÂÂÂ dev_err(mhi_cntrl->dev, "Failed to allocate device\n");
> > +ÂÂÂÂÂÂÂ ret = PTR_ERR(mhi_dev);
> > +ÂÂÂÂÂÂÂ goto error_alloc_dev;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;
> > +ÂÂÂ mhi_dev->mhi_cntrl = mhi_cntrl;
> > +ÂÂÂ dev_set_name(&mhi_dev->dev, "%s", mhi_cntrl->name);
> > +
> > +ÂÂÂ /* Init wakeup source */
> > +ÂÂÂ device_init_wakeup(&mhi_dev->dev, true);
> > +
> > +ÂÂÂ ret = device_add(&mhi_dev->dev);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ goto error_add_dev;
> > +
> > +ÂÂÂ mhi_cntrl->mhi_dev = mhi_dev;
> > +
> > +ÂÂÂ return 0;
> > +
> > +error_add_dev:
> > +ÂÂÂ mhi_dealloc_device(mhi_cntrl, mhi_dev);
> > +
> > +error_alloc_dev:
> > +ÂÂÂ kfree(mhi_cntrl->mhi_cmd);
> > +
> > +error_alloc_cmd:
> > +ÂÂÂ vfree(mhi_cntrl->mhi_chan);
> > +ÂÂÂ kfree(mhi_cntrl->mhi_event);
> > +
> > +ÂÂÂ return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(mhi_register_controller);
> > +
> > +void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
> > +{
> > +ÂÂÂ struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
> > +
> > +ÂÂÂ kfree(mhi_cntrl->mhi_cmd);
> > +ÂÂÂ kfree(mhi_cntrl->mhi_event);
> > +ÂÂÂ vfree(mhi_cntrl->mhi_chan);
> > +
> > +ÂÂÂ device_del(&mhi_dev->dev);
> > +ÂÂÂ put_device(&mhi_dev->dev);
> > +}
> > +EXPORT_SYMBOL_GPL(mhi_unregister_controller);
> > +
> > +static void mhi_release_device(struct device *dev)
> > +{
> > +ÂÂÂ struct mhi_device *mhi_dev = to_mhi_device(dev);
> > +
> > +ÂÂÂ if (mhi_dev->ul_chan)
> > +ÂÂÂÂÂÂÂ mhi_dev->ul_chan->mhi_dev = NULL;
> > +
> > +ÂÂÂ if (mhi_dev->dl_chan)
> > +ÂÂÂÂÂÂÂ mhi_dev->dl_chan->mhi_dev = NULL;
> > +
> > +ÂÂÂ kfree(mhi_dev);
> > +}
> > +
> > +struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl)
> > +{
> > +ÂÂÂ struct mhi_device *mhi_dev;
> > +ÂÂÂ struct device *dev;
> > +
> > +ÂÂÂ mhi_dev = kzalloc(sizeof(*mhi_dev), GFP_KERNEL);
> > +ÂÂÂ if (!mhi_dev)
> > +ÂÂÂÂÂÂÂ return ERR_PTR(-ENOMEM);
> > +
> > +ÂÂÂ dev = &mhi_dev->dev;
> > +ÂÂÂ device_initialize(dev);
> > +ÂÂÂ dev->bus = &mhi_bus_type;
> > +ÂÂÂ dev->release = mhi_release_device;
> > +ÂÂÂ dev->parent = mhi_cntrl->dev;
> > +ÂÂÂ mhi_dev->mhi_cntrl = mhi_cntrl;
> > +ÂÂÂ atomic_set(&mhi_dev->dev_wake, 0);
> > +
> > +ÂÂÂ return mhi_dev;
> > +}
> > +
> > +static int mhi_match(struct device *dev, struct device_driver *drv)
> > +{
> > +ÂÂÂ return 0;
> > +};
> > +
> > +struct bus_type mhi_bus_type = {
> > +ÂÂÂ .name = "mhi",
> > +ÂÂÂ .dev_name = "mhi",
> > +ÂÂÂ .match = mhi_match,
> > +};
> > +
> > +static int __init mhi_init(void)
> > +{
> > +ÂÂÂ return bus_register(&mhi_bus_type);
> > +}
> > +
> > +static void __exit mhi_exit(void)
> > +{
> > +ÂÂÂ bus_unregister(&mhi_bus_type);
> > +}
> > +
> > +postcore_initcall(mhi_init);
> > +module_exit(mhi_exit);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MHI Host Interface");
> > diff --git a/drivers/bus/mhi/core/internal.h 
> > b/drivers/bus/mhi/core/internal.h
> > new file mode 100644
> > index 000000000000..21f686d3a140
> > --- /dev/null
> > +++ b/drivers/bus/mhi/core/internal.h
> > @@ -0,0 +1,169 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > + *
> > + */
> > +
> > +#ifndef _MHI_INT_H
> > +#define _MHI_INT_H
> > +
> > +extern struct bus_type mhi_bus_type;
> > +
> > +/* MHI transfer completion events */
> > +enum mhi_ev_ccs {
> > +ÂÂÂ MHI_EV_CC_INVALID = 0x0,
> > +ÂÂÂ MHI_EV_CC_SUCCESS = 0x1,
> > +ÂÂÂ MHI_EV_CC_EOT = 0x2,
> > +ÂÂÂ MHI_EV_CC_OVERFLOW = 0x3,
> > +ÂÂÂ MHI_EV_CC_EOB = 0x4,
> > +ÂÂÂ MHI_EV_CC_OOB = 0x5,
> > +ÂÂÂ MHI_EV_CC_DB_MODE = 0x6,
> > +ÂÂÂ MHI_EV_CC_UNDEFINED_ERR = 0x10,
> > +ÂÂÂ MHI_EV_CC_BAD_TRE = 0x11,
>
> Perhaps a quick comment expanding the "EOT", "EOB", "OOB" acronyms? I 
> feel like those might not be obvious to someone not familiar with the 
> protocol.
>
> > +};
> > +
> > +enum mhi_ch_state {
> > +ÂÂÂ MHI_CH_STATE_DISABLED = 0x0,
> > +ÂÂÂ MHI_CH_STATE_ENABLED = 0x1,
> > +ÂÂÂ MHI_CH_STATE_RUNNING = 0x2,
> > +ÂÂÂ MHI_CH_STATE_SUSPENDED = 0x3,
> > +ÂÂÂ MHI_CH_STATE_STOP = 0x4,
> > +ÂÂÂ MHI_CH_STATE_ERROR = 0x5,
> > +};
> > +
> > +#define MHI_INVALID_BRSTMODE(mode) (mode != MHI_DB_BRST_DISABLE && \
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode != MHI_DB_BRST_ENABLE)
> > +
> > +#define NR_OF_CMD_RINGSÂÂÂÂÂÂÂÂÂÂÂ 1
> > +#define CMD_EL_PER_RINGÂÂÂÂÂÂÂÂÂÂÂ 128
> > +#define PRIMARY_CMD_RINGÂÂÂÂÂÂÂ 0
> > +#define MHI_MAX_MTUÂÂÂÂÂÂÂÂÂÂÂ 0xffff
> > +
> > +enum mhi_er_type {
> > +ÂÂÂ MHI_ER_TYPE_INVALID = 0x0,
> > +ÂÂÂ MHI_ER_TYPE_VALID = 0x1,
> > +};
> > +
> > +enum mhi_ch_ee_mask {
> > +ÂÂÂ MHI_CH_EE_PBL = BIT(MHI_EE_PBL),
>
> MHI_EE_PBL does not appear to be defined. Are you perhaps missing an 
> include?
>
> > +ÂÂÂ MHI_CH_EE_SBL = BIT(MHI_EE_SBL),
> > +ÂÂÂ MHI_CH_EE_AMSS = BIT(MHI_EE_AMSS),
> > +ÂÂÂ MHI_CH_EE_RDDM = BIT(MHI_EE_RDDM),
> > +ÂÂÂ MHI_CH_EE_PTHRU = BIT(MHI_EE_PTHRU),
> > +ÂÂÂ MHI_CH_EE_WFW = BIT(MHI_EE_WFW),
> > +ÂÂÂ MHI_CH_EE_EDL = BIT(MHI_EE_EDL),
> > +};
> > +
> > +struct db_cfg {
> > +ÂÂÂ bool reset_req;
> > +ÂÂÂ bool db_mode;
> > +ÂÂÂ u32 pollcfg;
> > +ÂÂÂ enum mhi_db_brst_mode brstmode;
> > +ÂÂÂ dma_addr_t db_val;
> > +ÂÂÂ void (*process_db)(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct db_cfg *db_cfg, void __iomem *io_addr,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dma_addr_t db_val);
> > +};
> > +
> > +struct mhi_ring {
> > +ÂÂÂ dma_addr_t dma_handle;
> > +ÂÂÂ dma_addr_t iommu_base;
> > +ÂÂÂ u64 *ctxt_wp; /* point to ctxt wp */
> > +ÂÂÂ void *pre_aligned;
> > +ÂÂÂ void *base;
> > +ÂÂÂ void *rp;
> > +ÂÂÂ void *wp;
> > +ÂÂÂ size_t el_size;
> > +ÂÂÂ size_t len;
> > +ÂÂÂ size_t elements;
> > +ÂÂÂ size_t alloc_size;
> > +ÂÂÂ void __iomem *db_addr;
> > +};
> > +
> > +struct mhi_cmd {
> > +ÂÂÂ struct mhi_ring ring;
> > +ÂÂÂ spinlock_t lock;
> > +};
> > +
> > +struct mhi_buf_info {
> > +ÂÂÂ dma_addr_t p_addr;
> > +ÂÂÂ void *v_addr;
> > +ÂÂÂ void *bb_addr;
> > +ÂÂÂ void *wp;
> > +ÂÂÂ size_t len;
> > +ÂÂÂ void *cb_buf;
> > +ÂÂÂ enum dma_data_direction dir;
> > +};
> > +
> > +struct mhi_event {
> > +ÂÂÂ u32 er_index;
> > +ÂÂÂ u32 intmod;
> > +ÂÂÂ u32 irq;
> > +ÂÂÂ int chan; /* this event ring is dedicated to a channel (optional) */
> > +ÂÂÂ u32 priority;
> > +ÂÂÂ enum mhi_er_data_type data_type;
> > +ÂÂÂ struct mhi_ring ring;
> > +ÂÂÂ struct db_cfg db_cfg;
> > +ÂÂÂ bool hw_ring;
> > +ÂÂÂ bool cl_manage;
> > +ÂÂÂ bool offload_ev; /* managed by a device driver */
> > +ÂÂÂ spinlock_t lock;
> > +ÂÂÂ struct mhi_chan *mhi_chan; /* dedicated to channel */
> > +ÂÂÂ struct tasklet_struct task;
> > +ÂÂÂ int (*process_event)(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct mhi_event *mhi_event,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u32 event_quota);
> > +ÂÂÂ struct mhi_controller *mhi_cntrl;
> > +};
> > +
> > +struct mhi_chan {
> > +ÂÂÂ u32 chan;
> > +ÂÂÂ const char *name;
> > +ÂÂÂ /*
> > +ÂÂÂÂ * Important: When consuming, increment tre_ring first and when
> > +ÂÂÂÂ * releasing, decrement buf_ring first. If tre_ring has space, 
> > buf_ring
> > +ÂÂÂÂ * is guranteed to have space so we do not need to check both rings.
> > +ÂÂÂÂ */
> > +ÂÂÂ struct mhi_ring buf_ring;
> > +ÂÂÂ struct mhi_ring tre_ring;
> > +ÂÂÂ u32 er_index;
> > +ÂÂÂ u32 intmod;
> > +ÂÂÂ enum mhi_ch_type type;
> > +ÂÂÂ enum dma_data_direction dir;
> > +ÂÂÂ struct db_cfg db_cfg;
> > +ÂÂÂ enum mhi_ch_ee_mask ee_mask;
> > +ÂÂÂ enum mhi_buf_type xfer_type;
> > +ÂÂÂ enum mhi_ch_state ch_state;
> > +ÂÂÂ enum mhi_ev_ccs ccs;
> > +ÂÂÂ bool lpm_notify;
> > +ÂÂÂ bool configured;
> > +ÂÂÂ bool offload_ch;
> > +ÂÂÂ bool pre_alloc;
> > +ÂÂÂ bool auto_start;
> > +ÂÂÂ int (*gen_tre)(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct mhi_chan *mhi_chan, void *buf, void *cb,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t len, enum mhi_flags flags);
> > +ÂÂÂ int (*queue_xfer)(struct mhi_device *mhi_dev, struct mhi_chan 
> > *mhi_chan,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ void *buf, size_t len, enum mhi_flags mflags);
> > +ÂÂÂ struct mhi_device *mhi_dev;
> > +ÂÂÂ void (*xfer_cb)(struct mhi_device *mhi_dev, struct mhi_result 
> > *result);
> > +ÂÂÂ struct mutex mutex;
> > +ÂÂÂ struct completion completion;
> > +ÂÂÂ rwlock_t lock;
> > +ÂÂÂ struct list_head node;
> > +};
> > +
> > +/* Default MHI timeout */
> > +#define MHI_TIMEOUT_MS (1000)
> > +
> > +struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl);
> > +static inline void mhi_dealloc_device(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct mhi_device *mhi_dev)
> > +{
> > +ÂÂÂ kfree(mhi_dev);
> > +}
> > +
> > +int mhi_destroy_device(struct device *dev, void *data);
> > +void mhi_create_devices(struct mhi_controller *mhi_cntrl);
> > +
> > +#endif /* _MHI_INT_H */
> > diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> > new file mode 100644
> > index 000000000000..69cf9a4b06c7
> > --- /dev/null
> > +++ b/include/linux/mhi.h
> > @@ -0,0 +1,438 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > + *
> > + */
> > +#ifndef _MHI_H_
> > +#define _MHI_H_
> > +
> > +#include <linux/device.h>
> > +#include <linux/dma-direction.h>
> > +#include <linux/mutex.h>
> > +#include <linux/rwlock_types.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock_types.h>
> > +#include <linux/wait.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct mhi_chan;
> > +struct mhi_event;
> > +struct mhi_ctxt;
> > +struct mhi_cmd;
> > +struct mhi_buf_info;
> > +
> > +/**
> > + * enum mhi_callback - MHI callback
> > + * @MHI_CB_IDLE: MHI entered idle state
> > + * @MHI_CB_PENDING_DATA: New data available for client to process
> > + * @MHI_CB_LPM_ENTER: MHI host entered low power mode
> > + * @MHI_CB_LPM_EXIT: MHI host about to exit low power mode
> > + * @MHI_CB_EE_RDDM: MHI device entered RDDM exec env
> > + * @MHI_CB_EE_MISSION_MODE: MHI device entered Mission Mode exec env
> > + * @MHI_CB_SYS_ERROR: MHI device entered error state (may recover)
> > + * @MHI_CB_FATAL_ERROR: MHI device entered fatal error state
> > + */
> > +enum mhi_callback {
> > +ÂÂÂ MHI_CB_IDLE,
> > +ÂÂÂ MHI_CB_PENDING_DATA,
> > +ÂÂÂ MHI_CB_LPM_ENTER,
> > +ÂÂÂ MHI_CB_LPM_EXIT,
> > +ÂÂÂ MHI_CB_EE_RDDM,
> > +ÂÂÂ MHI_CB_EE_MISSION_MODE,
> > +ÂÂÂ MHI_CB_SYS_ERROR,
> > +ÂÂÂ MHI_CB_FATAL_ERROR,
> > +};
> > +
> > +/**
> > + * enum mhi_flags - Transfer flags
> > + * @MHI_EOB: End of buffer for bulk transfer
> > + * @MHI_EOT: End of transfer
> > + * @MHI_CHAIN: Linked transfer
> > + */
> > +enum mhi_flags {
> > +ÂÂÂ MHI_EOB,
> > +ÂÂÂ MHI_EOT,
> > +ÂÂÂ MHI_CHAIN,
> > +};
> > +
> > +/**
> > + * enum mhi_device_type - Device types
> > + * @MHI_DEVICE_XFER: Handles data transfer
> > + * @MHI_DEVICE_TIMESYNC: Use for timesync feature
> > + * @MHI_DEVICE_CONTROLLER: Control device
> > + */
> > +enum mhi_device_type {
> > +ÂÂÂ MHI_DEVICE_XFER,
> > +ÂÂÂ MHI_DEVICE_TIMESYNC,
> > +ÂÂÂ MHI_DEVICE_CONTROLLER,
> > +};
> > +
> > +/**
> > + * enum mhi_ch_type - Channel types
> > + * @MHI_CH_TYPE_INVALID: Invalid channel type
> > + * @MHI_CH_TYPE_OUTBOUND: Outbound channel to the device
> > + * @MHI_CH_TYPE_INBOUND: Inbound channel from the device
> > + * @MHI_CH_TYPE_INBOUND_COALESCED: Coalesced channel for the device 
> > to combine
> > + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ multiple packets and send them as a single
> > + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ large packet to reduce CPU consumption
> > + */
> > +enum mhi_ch_type {
> > +ÂÂÂ MHI_CH_TYPE_INVALID = 0,
> > +ÂÂÂ MHI_CH_TYPE_OUTBOUND = DMA_TO_DEVICE,
> > +ÂÂÂ MHI_CH_TYPE_INBOUND = DMA_FROM_DEVICE,
> > +ÂÂÂ MHI_CH_TYPE_INBOUND_COALESCED = 3,
> > +};
> > +
> > +/**
> > + * enum mhi_ee_type - Execution environment types
> > + * @MHI_EE_PBL: Primary Bootloader
> > + * @MHI_EE_SBL: Secondary Bootloader
> > + * @MHI_EE_AMSS: Modem, aka the primary runtime EE
> > + * @MHI_EE_RDDM: Ram dump download mode
> > + * @MHI_EE_WFW: WLAN firmware mode
> > + * @MHI_EE_PTHRU: Passthrough
> > + * @MHI_EE_EDL: Embedded downloader
> > + */
> > +enum mhi_ee_type {
> > +ÂÂÂ MHI_EE_PBL,
> > +ÂÂÂ MHI_EE_SBL,
> > +ÂÂÂ MHI_EE_AMSS,
> > +ÂÂÂ MHI_EE_RDDM,
> > +ÂÂÂ MHI_EE_WFW,
> > +ÂÂÂ MHI_EE_PTHRU,
> > +ÂÂÂ MHI_EE_EDL,
> > +ÂÂÂ MHI_EE_MAX_SUPPORTED = MHI_EE_EDL,
> > +ÂÂÂ MHI_EE_DISABLE_TRANSITION, /* local EE, not related to mhi spec */
> > +ÂÂÂ MHI_EE_NOT_SUPPORTED,
> > +ÂÂÂ MHI_EE_MAX,
> > +};
> > +
> > +/**
> > + * enum mhi_buf_type - Accepted buffer type for the channel
> > + * @MHI_BUF_RAW: Raw buffer
> > + * @MHI_BUF_SKB: SKB struct
> > + * @MHI_BUF_SCLIST: Scatter-gather list
> > + * @MHI_BUF_NOP: CPU offload channel, host does not accept transfer
> > + * @MHI_BUF_DMA: Receive DMA address mapped by client
> > + * @MHI_BUF_RSC_DMA: RSC type premapped buffer
>
> Maybe its just me, but what is "RSC"?
>
> > + */
> > +enum mhi_buf_type {
> > +ÂÂÂ MHI_BUF_RAW,
> > +ÂÂÂ MHI_BUF_SKB,
> > +ÂÂÂ MHI_BUF_SCLIST,
> > +ÂÂÂ MHI_BUF_NOP,
> > +ÂÂÂ MHI_BUF_DMA,
> > +ÂÂÂ MHI_BUF_RSC_DMA,
> > +};
> > +
> > +/**
> > + * enum mhi_er_data_type - Event ring data types
> > + * @MHI_ER_DATA: Only client data over this ring
> > + * @MHI_ER_CTRL: MHI control data and client data
> > + * @MHI_ER_TSYNC: Time sync events
> > + */
> > +enum mhi_er_data_type {
> > +ÂÂÂ MHI_ER_DATA,
> > +ÂÂÂ MHI_ER_CTRL,
> > +ÂÂÂ MHI_ER_TSYNC,
> > +};
> > +
> > +/**
> > + * enum mhi_db_brst_mode - Doorbell mode
> > + * @MHI_DB_BRST_DISABLE: Burst mode disable
> > + * @MHI_DB_BRST_ENABLE: Burst mode enable
> > + */
> > +enum mhi_db_brst_mode {
> > +ÂÂÂ MHI_DB_BRST_DISABLE = 0x2,
> > +ÂÂÂ MHI_DB_BRST_ENABLE = 0x3,
> > +};
> > +
> > +/**
> > + * struct mhi_channel_config - Channel configuration structure for 
> > controller
> > + * @num: The number assigned to this channel
> > + * @name: The name of this channel
> > + * @num_elements: The number of elements that can be queued to this 
> > channel
> > + * @local_elements: The local ring length of the channel
> > + * @event_ring: The event rung index that services this channel
> > + * @dir: Direction that data may flow on this channel
> > + * @type: Channel type
> > + * @ee_mask: Execution Environment mask for this channel
>
> But the mask defines are in internal.h, so how is a client suposed to 
> know what they are?
>
> > + * @pollcfg: Polling configuration for burst mode. 0 is default.  
> > milliseconds
> > +ÂÂÂÂÂÂÂÂ for UL channels, multiple of 8 ring elements for DL channels
> > + * @data_type: Data type accepted by this channel
> > + * @doorbell: Doorbell mode
> > + * @lpm_notify: The channel master requires low power mode notifications
> > + * @offload_channel: The client manages the channel completely
> > + * @doorbell_mode_switch: Channel switches to doorbell mode on M0 
> > transition
> > + * @auto_queue: Framework will automatically queue buffers for DL 
> > traffic
> > + * @auto_start: Automatically start (open) this channel
> > + */
> > +struct mhi_channel_config {
> > +ÂÂÂ u32 num;
> > +ÂÂÂ char *name;
> > +ÂÂÂ u32 num_elements;
> > +ÂÂÂ u32 local_elements;
> > +ÂÂÂ u32 event_ring;
> > +ÂÂÂ enum dma_data_direction dir;
> > +ÂÂÂ enum mhi_ch_type type;

Why do we have "dir" and "type" when they are the same thing?

> > +ÂÂÂ u32 ee_mask;
> > +ÂÂÂ u32 pollcfg;
> > +ÂÂÂ enum mhi_buf_type data_type;
> > +ÂÂÂ enum mhi_db_brst_mode doorbell;
> > +ÂÂÂ bool lpm_notify;
> > +ÂÂÂ bool offload_channel;
> > +ÂÂÂ bool doorbell_mode_switch;
> > +ÂÂÂ bool auto_queue;
> > +ÂÂÂ bool auto_start;
> > +};
> > +
> > +/**
> > + * struct mhi_event_config - Event ring configuration structure for 
> > controller
> > + * @num_elements: The number of elements that can be queued to this ring
> > + * @irq_moderation_ms: Delay irq for additional events to be aggregated
> > + * @irq: IRQ associated with this ring
> > + * @channel: Dedicated channel number. U32_MAX indicates a 
> > non-dedicated ring
> > + * @mode: Doorbell mode
> > + * @data_type: Type of data this ring will process
> > + * @hardware_event: This ring is associated with hardware channels
> > + * @client_managed: This ring is client managed
> > + * @offload_channel: This ring is associated with an offloaded channel
> > + * @priority: Priority of this ring. Use 1 for now
> > + */
> > +struct mhi_event_config {
> > +ÂÂÂ u32 num_elements;
> > +ÂÂÂ u32 irq_moderation_ms;
> > +ÂÂÂ u32 irq;
> > +ÂÂÂ u32 channel;
> > +ÂÂÂ enum mhi_db_brst_mode mode;
> > +ÂÂÂ enum mhi_er_data_type data_type;
> > +ÂÂÂ bool hardware_event;
> > +ÂÂÂ bool client_managed;
> > +ÂÂÂ bool offload_channel;
> > +ÂÂÂ u32 priority;
> > +};
> > +
> > +/**
> > + * struct mhi_controller_config - Root MHI controller configuration
> > + * @max_channels: Maximum number of channels supported
> > + * @timeout_ms: Timeout value for operations. 0 means use default
> > + * @use_bounce_buf: Use a bounce buffer pool due to limited DDR access
> > + * @m2_no_db: Host is not allowed to ring DB in M2 state
> > + * @buf_len: Size of automatically allocated buffers. 0 means use 
> > default
> > + * @num_channels: Number of channels defined in @ch_cfg
> > + * @ch_cfg: Array of defined channels
> > + * @num_events: Number of event rings defined in @event_cfg
> > + * @event_cfg: Array of defined event rings
> > + */
> > +struct mhi_controller_config {
> > +ÂÂÂ u32 max_channels;
> > +ÂÂÂ u32 timeout_ms;
> > +ÂÂÂ bool use_bounce_buf;
> > +ÂÂÂ bool m2_no_db;
> > +ÂÂÂ u32 buf_len;
> > +ÂÂÂ u32 num_channels;
> > +ÂÂÂ struct mhi_channel_config *ch_cfg;
> > +ÂÂÂ u32 num_events;
> > +ÂÂÂ struct mhi_event_config *event_cfg;
> > +};
> > +
> > +/**
> > + * struct mhi_controller - Master MHI controller structure

Quite a bit of this needs to be initialized by the entity calling 
mhi_register_controller(), but its not clear what.  I'm thinking that 
since we have a config structure, all of that should be copied/moved 
into the config so that the caller of mhi_register_controller() provides 
an empty mhi_controller struct / a populated config struct and receives 
an initialized mhi_controller instance.

> > + * @name: Name of the controller
> > + * @dev: Driver model device node for the controller
> > + * @mhi_dev: MHI device instance for the controller
> > + * @dev_id: Device ID of the controller
> > + * @bus_id: Physical bus instance used by the controller
> > + * @regs: Base address of MHI MMIO register space
> > + * @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
> > + * @sbl_size: SBL image size
> > + * @seg_len: BHIe vector size
> > + * @max_chan: Maximum number of channels the controller supports
> > + * @mhi_chan: Points to the 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
> > + * @nr_irqs_req: Number of IRQs required to operate
> > + * @nr_irqs: Number of IRQ 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_mutex: Mutex for suspend/resume operation
> > + * @pre_init: MHI host needs to do pre-initialization before power up
> > + * @pm_lock: Lock for protecting MHI power management state
> > + * @pm_state: MHI power management state
> > + * @db_access: DB access states
> > + * @ee: MHI device execution environment
> > + * @wake_set: Device wakeup set flag
> > + * @dev_wake: Device wakeup count
> > + * @alloc_size: Total memory allocations size of the controller
> > + * @pending_pkts: Pending packets for the controller
> > + * @transition_list: List of MHI state transitions
> > + * @wlock: Lock for protecting device wakeup
> > + * @M0: M0 state counter for debugging
> > + * @M2: M2 state counter for debugging
> > + * @M3: M3 state counter for debugging
> > + * @M3_FAST: M3 Fast state counter for debugging
> > + * @st_worker: State transition worker
> > + * @fw_worker: Firmware download worker
> > + * @syserr_worker: System error worker
> > + * @state_event: State change event
> > + * @status_cb: CB function to notify various power states to bus master
> > + * @link_status: CB function to query link status of the device
> > + * @wake_get: CB function to assert device wake
> > + * @wake_put: CB function to de-assert device wake
> > + * @wake_toggle: CB function to assert and deasset (toggle) device wake
> > + * @runtime_get: CB function to controller runtime resume
> > + * @runtimet_put: CB function to decrement pm usage
> > + * @lpm_disable: CB function to request disable link level low power 
> > modes
> > + * @lpm_enable: CB function to request enable link level low power 
> > modes again
> > + * @bounce_buf: Use of bounce buffer
> > + * @buffer_len: Bounce buffer length
> > + * @priv_data: Points to bus master's private data
> > + */
> > +struct mhi_controller {
> > +ÂÂÂ const char *name;
> > +ÂÂÂ struct device *dev;
> > +ÂÂÂ struct mhi_device *mhi_dev;
> > +ÂÂÂ u32 dev_id;
> > +ÂÂÂ u32 bus_id;
> > +ÂÂÂ void __iomem *regs;
> > +ÂÂÂ dma_addr_t iova_start;
> > +ÂÂÂ dma_addr_t iova_stop;
> > +ÂÂÂ const char *fw_image;
> > +ÂÂÂ const char *edl_image;
> > +ÂÂÂ bool fbc_download;
> > +ÂÂÂ size_t sbl_size;
> > +ÂÂÂ size_t seg_len;
> > +ÂÂÂ u32 max_chan;
> > +ÂÂÂ struct mhi_chan *mhi_chan;
> > +ÂÂÂ struct list_head lpm_chans;
> > +ÂÂÂ u32 total_ev_rings;
> > +ÂÂÂ u32 hw_ev_rings;
> > +ÂÂÂ u32 sw_ev_rings;
> > +ÂÂÂ u32 nr_irqs_req;
> > +ÂÂÂ u32 nr_irqs;
> > +ÂÂÂ int *irq;
> > +
> > +ÂÂÂ struct mhi_event *mhi_event;
> > +ÂÂÂ struct mhi_cmd *mhi_cmd;
> > +ÂÂÂ struct mhi_ctxt *mhi_ctxt;
> > +
> > +ÂÂÂ u32 timeout_ms;
> > +ÂÂÂ struct mutex pm_mutex;
> > +ÂÂÂ bool pre_init;
> > +ÂÂÂ rwlock_t pm_lock;
> > +ÂÂÂ u32 pm_state;
> > +ÂÂÂ u32 db_access;
> > +ÂÂÂ enum mhi_ee_type ee;
> > +ÂÂÂ bool wake_set;
> > +ÂÂÂ atomic_t dev_wake;
> > +ÂÂÂ atomic_t alloc_size;
> > +ÂÂÂ atomic_t pending_pkts;
> > +ÂÂÂ struct list_head transition_list;
> > +ÂÂÂ spinlock_t transition_lock;
> > +ÂÂÂ spinlock_t wlock;
> > +ÂÂÂ u32 M0, M2, M3, M3_FAST;
> > +ÂÂÂ struct work_struct st_worker;
> > +ÂÂÂ struct work_struct fw_worker;
> > +ÂÂÂ struct work_struct syserr_worker;
> > +ÂÂÂ wait_queue_head_t state_event;
> > +
> > +ÂÂÂ void (*status_cb)(struct mhi_controller *mhi_cntrl, void *priv,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ enum mhi_callback 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);
> > +ÂÂÂ void (*wake_toggle)(struct mhi_controller *mhi_cntrl);
> > +ÂÂÂ 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);
> > +
> > +ÂÂÂ bool bounce_buf;
> > +ÂÂÂ size_t buffer_len;
> > +ÂÂÂ void *priv_data;
> > +};
> > +
> > +/**
> > + * struct mhi_device - Structure representing a MHI device which binds
> > + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ to channels
> > + * @dev: Driver model device node for the MHI device
> > + * @tiocm: Device current terminal settings
> > + * @id: Pointer to MHI device ID struct
> > + * @chan_name: Name of the channel to which the device binds
> > + * @mhi_cntrl: Controller the device belongs to
> > + * @ul_chan: UL channel for the device
> > + * @dl_chan: DL channel for the device
> > + * @dev_wake: Device wakeup counter
> > + * @dev_type: MHI device type
> > + */
> > +struct mhi_device {
> > +ÂÂÂ struct device dev;
> > +ÂÂÂ u32 tiocm;
> > +ÂÂÂ const struct mhi_device_id *id;
> > +ÂÂÂ const char *chan_name;
> > +ÂÂÂ struct mhi_controller *mhi_cntrl;
> > +ÂÂÂ struct mhi_chan *ul_chan;
> > +ÂÂÂ struct mhi_chan *dl_chan;
> > +ÂÂÂ atomic_t dev_wake;
> > +ÂÂÂ enum mhi_device_type dev_type;
> > +};
> > +
> > +/**
> > + * 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 transaction
> > + */
> > +struct mhi_result {
> > +ÂÂÂ void *buf_addr;
> > +ÂÂÂ enum dma_data_direction dir;
> > +ÂÂÂ size_t bytes_xferd;
>
> Desription says this is named "bytes_xfer"
>
> > +ÂÂÂ int transaction_status;
> > +};
> > +
> > +#define to_mhi_device(dev) container_of(dev, struct mhi_device, dev)
> > +
> > +/**
> > + * mhi_controller_set_devdata - Set MHI controller private data
> > + * @mhi_cntrl: MHI controller to set data
> > + */
> > +static inline void mhi_controller_set_devdata(struct mhi_controller 
> > *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void *priv)
> > +{
> > +ÂÂÂ mhi_cntrl->priv_data = priv;
> > +}
> > +
> > +/**
> > + * mhi_controller_get_devdata - Get MHI controller private data
> > + * @mhi_cntrl: MHI controller to get data
> > + */
> > +static inline void *mhi_controller_get_devdata(struct mhi_controller 
> > *mhi_cntrl)
> > +{
> > +ÂÂÂ return mhi_cntrl->priv_data;
> > +}
> > +
> > +/**
> > + * mhi_register_controller - Register MHI controller
> > + * @mhi_cntrl: MHI controller to register
> > + * @config: Configuration to use for the controller
> > + */
> > +int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct mhi_controller_config *config);
> > +
> > +/**
> > + * mhi_unregister_controller - Unregister MHI controller
> > + * @mhi_cntrl: MHI controller to unregister
> > + */
> > +void mhi_unregister_controller(struct mhi_controller *mhi_cntrl);
> > +
> > +#endif /* _MHI_H_ */
> > diff --git a/include/linux/mod_devicetable.h 
> > b/include/linux/mod_devicetable.h
> > index e3596db077dc..be15e997fe39 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -821,4 +821,16 @@ struct wmi_device_id {
> > ÂÂÂÂÂ const void *context;
> > Â };
> > +#define MHI_NAME_SIZE 32
> > +
> > +/**
> > + * struct mhi_device_id - MHI device identification
> > + * @chan: MHI channel name
> > + * @driver_data: driver data;
> > + */
> > +struct mhi_device_id {
> > +ÂÂÂ const char chan[MHI_NAME_SIZE];
> > +ÂÂÂ kernel_ulong_t driver_data;
> > +};
> > +
> > Â #endif /* LINUX_MOD_DEVICETABLE_H */


--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.