diff mbox series

[v2,02/16] bus: mhi: core: Add support for registering MHI controllers

Message ID 20200131135009.31477-3-manivannan.sadhasivam@linaro.org
State New
Headers show
Series Add MHI bus support | expand

Commit Message

Manivannan Sadhasivam Jan. 31, 2020, 1:49 p.m. UTC
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@codeaurora.org>
Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
[jhugo: added static config for controllers and fixed several bugs]
Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
[mani: removed DT dependency, splitted and cleaned up for upstream]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 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     | 407 +++++++++++++++++++++++++++++++
 drivers/bus/mhi/core/internal.h | 163 +++++++++++++
 include/linux/mhi.h             | 420 ++++++++++++++++++++++++++++++++
 include/linux/mod_devicetable.h |  12 +
 9 files changed, 1025 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

Comments

Greg Kroah-Hartman Feb. 6, 2020, 4:57 p.m. UTC | #1
On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
> --- /dev/null
> +++ b/drivers/bus/mhi/core/init.c
> @@ -0,0 +1,407 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> + *
> + */
> +
> +#define dev_fmt(fmt) "MHI: " fmt

This should not be needed, right?  The bus/device name should give you
all you need here from what I can tell.  So why is this needed?

thanks,

greg k-h
Jeffrey Hugo Feb. 6, 2020, 5:08 p.m. UTC | #2
On 1/31/2020 6:49 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@codeaurora.org>
> Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> [jhugo: added static config for controllers and fixed several bugs]
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> [mani: removed DT dependency, splitted and cleaned up for upstream]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> +/**
> + * 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,
> +};

Time sync support was removed, no?  I don't see MHI_DEVICE_TIMESYNC used 
anywhere in the code.

> +/**
> + * 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,
> +};

Time sync support was removed, no?  I don't see MHI_ER_TSYNC used 
anywhere in the code.

> +/**
> + * struct mhi_controller - Master MHI controller structure
> + * @dev: Driver model device node for the controller (required)
> + * @mhi_dev: MHI device instance for the controller
> + * @regs: Base address of MHI MMIO register space (required)
> + * @iova_start: IOMMU starting address for data (required)
> + * @iova_stop: IOMMU stop address for data (required)
> + * @fw_image: Firmware image name for normal booting (required)
> + * @edl_image: Firmware image name for emergency download mode (optional)
> + * @sbl_size: SBL image size downloaded through BHIe (optional)
> + * @seg_len: BHIe vector size (optional)
> + * @mhi_chan: Points to the channel configuration table
> + * @lpm_chans: List of channels that require LPM notifications
> + * @irq: base irq # to request (required)
> + * @max_chan: Maximum number of channels the controller supports
> + * @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 (optional)
> + * @nr_irqs: Number of IRQ allocated by bus master (required)
> + * @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
> + * @pm_mutex: Mutex for suspend/resume operation
> + * @pm_lock: Lock for protecting MHI power management state
> + * @timeout_ms: Timeout in ms for state transitions
> + * @pm_state: MHI power management state
> + * @db_access: DB access states
> + * @ee: MHI device execution environment
> + * @dev_wake: Device wakeup count
> + * @pending_pkts: Pending packets for the controller
> + * @transition_list: List of MHI state transitions
> + * @transition_lock: Lock for protecting MHI state transition list
> + * @wlock: Lock for protecting device wakeup
> + * @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 power states of the device (required)
> + * @link_status: CB function to query link status of the device (required)
> + * @wake_get: CB function to assert device wake (optional)
> + * @wake_put: CB function to de-assert device wake (optional)
> + * @wake_toggle: CB function to assert and de-assert device wake (optional)
> + * @runtime_get: CB function to controller runtime resume (required)
> + * @runtimet_put: CB function to decrement pm usage (required)
> + * @buffer_len: Bounce buffer length
> + * @bounce_buf: Use of bounce buffer
> + * @fbc_download: MHI host needs to do complete image transfer (optional)
> + * @pre_init: MHI host needs to do pre-initialization before power up
> + * @wake_set: Device wakeup set flag
> + */

I'm happy the optional/required is listed.  However if I look at this 
from the perspective of someone writing a controller for the first time, 
I'm not confident the required/optional annotations are clear enough. 
Perhaps a quick sentance explaining that those annotations indicate 
fields which must be populated prior to mhi_register_controller() ?

> +
> +/**
> + * struct mhi_device - Structure representing a MHI device which binds
> + *                     to channels
> + * @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: Driver model device node for the MHI device
> + * @dev_type: MHI device type
> + * @tiocm: Device current terminal settings
> + * @dev_wake: Device wakeup counter
> + */
> +struct mhi_device {
> +	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;
> +	struct device dev;
> +	enum mhi_device_type dev_type;
> +	u32 tiocm;

This was for the old ioctl support, right?  I don't see it used anywhere.

> +	u32 dev_wake;
> +};
Manivannan Sadhasivam Feb. 11, 2020, 6:41 p.m. UTC | #3
Hi Greg,

On Thu, Feb 06, 2020 at 05:57:55PM +0100, Greg KH wrote:
> On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
> > --- /dev/null
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -0,0 +1,407 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > + *
> > + */
> > +
> > +#define dev_fmt(fmt) "MHI: " fmt
> 
> This should not be needed, right?  The bus/device name should give you
> all you need here from what I can tell.  So why is this needed?
> 

The log will have only the device name as like PCI-E. But that won't specify
where the error is coming from. Having "MHI" prefix helps the users to
quickly identify that the error is coming from MHI stack.

Thanks,
Mani

> thanks,
> 
> greg k-h
Manivannan Sadhasivam Feb. 11, 2020, 7:11 p.m. UTC | #4
Hi Greg,

On Thu, Feb 06, 2020 at 05:56:06PM +0100, Greg KH wrote:
> On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
> > +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;
> 
> That looks really odd.  Why didn't you just drop the reference you
> should have grabbed here for this pointer?  You did properly increment
> it when you saved it, right?  :)
> 

Well, there is no reference count (kref) exist for mhi_dev. And we really
needed to NULL the mhi_dev to avoid any dangling reference to it. The reason for
not having kref is that, each mhi_dev will be used by maximum of 2 channels
only. So thought that refcounting is not needed. Please correct me if I'm
wrong.

Thanks,
Mani

> > +
> > +	if (mhi_dev->dl_chan)
> > +		mhi_dev->dl_chan->mhi_dev = NULL;
> 
> Same here.
> 
> thanks,
> 
> greg k-h
Greg Kroah-Hartman Feb. 11, 2020, 7:22 p.m. UTC | #5
On Wed, Feb 12, 2020 at 12:41:47AM +0530, Manivannan Sadhasivam wrote:
> Hi Greg,
> 
> On Thu, Feb 06, 2020 at 05:56:06PM +0100, Greg KH wrote:
> > On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
> > > +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;
> > 
> > That looks really odd.  Why didn't you just drop the reference you
> > should have grabbed here for this pointer?  You did properly increment
> > it when you saved it, right?  :)
> > 
> 
> Well, there is no reference count (kref) exist for mhi_dev.

Then something is wrong with your model :(

You can't save pointers off to things without reference counting, that
is going to cause you real problems.  See the coding style document for
all the details.

> And we really needed to NULL the mhi_dev to avoid any dangling
> reference to it.

Again, that's not how to do this correctly.

> The reason for not having kref is that, each mhi_dev will be used by
> maximum of 2 channels only. So thought that refcounting is not needed.
> Please correct me if I'm wrong.

Please read section 11 of Documentation/process/coding-style.rst

thanks,

greg k-h
Manivannan Sadhasivam Feb. 13, 2020, 3:20 p.m. UTC | #6
On Tue, Feb 11, 2020 at 11:20:55AM -0800, Greg KH wrote:
> On Wed, Feb 12, 2020 at 12:11:30AM +0530, Manivannan Sadhasivam wrote:
> > Hi Greg,
> > 
> > On Thu, Feb 06, 2020 at 05:57:55PM +0100, Greg KH wrote:
> > > On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
> > > > --- /dev/null
> > > > +++ b/drivers/bus/mhi/core/init.c
> > > > @@ -0,0 +1,407 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > > > + *
> > > > + */
> > > > +
> > > > +#define dev_fmt(fmt) "MHI: " fmt
> > > 
> > > This should not be needed, right?  The bus/device name should give you
> > > all you need here from what I can tell.  So why is this needed?
> > > 
> > 
> > The log will have only the device name as like PCI-E. But that won't specify
> > where the error is coming from. Having "MHI" prefix helps the users to
> > quickly identify that the error is coming from MHI stack.
> 
> If the driver binds properly to the device, the name of the driver will
> be there in the message, so I suggest using that please.
> 
> No need for this prefix...
> 

So the driver name will be in the log but that won't help identifying where
the log is coming from. This is more important for MHI since it reuses the
`struct device` of the transport device like PCI-E. For instance, below is
the log without MHI prefix:

[   47.355582] ath11k_pci 0000:01:00.0: Requested to power on
[   47.355724] ath11k_pci 0000:01:00.0: Power on setup success

As you can see, this gives the assumption that the log is coming from the
ath11k_pci driver. But the reality is, it is coming from MHI bus.

With the prefix added, we will get below:

[   47.355582] ath11k_pci 0000:01:00.0: MHI: Requested to power on
[   47.355724] ath11k_pci 0000:01:00.0: MHI: Power on setup success

IMO, the prefix will give users a clear idea of logs and that will be very
useful for debugging.

Hope this clarifies.

Thanks,
Mani

> thanks,
> 
> greg k-h
Manivannan Sadhasivam Feb. 13, 2020, 3:48 p.m. UTC | #7
Hi Greg,

On Thu, Feb 13, 2020 at 07:34:18AM -0800, Greg KH wrote:
> On Thu, Feb 13, 2020 at 08:50:13PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Feb 11, 2020 at 11:20:55AM -0800, Greg KH wrote:
> > > On Wed, Feb 12, 2020 at 12:11:30AM +0530, Manivannan Sadhasivam wrote:
> > > > Hi Greg,
> > > > 
> > > > On Thu, Feb 06, 2020 at 05:57:55PM +0100, Greg KH wrote:
> > > > > On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/bus/mhi/core/init.c
> > > > > > @@ -0,0 +1,407 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > > > > > + *
> > > > > > + */
> > > > > > +
> > > > > > +#define dev_fmt(fmt) "MHI: " fmt
> > > > > 
> > > > > This should not be needed, right?  The bus/device name should give you
> > > > > all you need here from what I can tell.  So why is this needed?
> > > > > 
> > > > 
> > > > The log will have only the device name as like PCI-E. But that won't specify
> > > > where the error is coming from. Having "MHI" prefix helps the users to
> > > > quickly identify that the error is coming from MHI stack.
> > > 
> > > If the driver binds properly to the device, the name of the driver will
> > > be there in the message, so I suggest using that please.
> > > 
> > > No need for this prefix...
> > > 
> > 
> > So the driver name will be in the log but that won't help identifying where
> > the log is coming from. This is more important for MHI since it reuses the
> > `struct device` of the transport device like PCI-E. For instance, below is
> > the log without MHI prefix:
> > 
> > [   47.355582] ath11k_pci 0000:01:00.0: Requested to power on
> > [   47.355724] ath11k_pci 0000:01:00.0: Power on setup success
> > 
> > As you can see, this gives the assumption that the log is coming from the
> > ath11k_pci driver. But the reality is, it is coming from MHI bus.
> 
> Then you should NOT be trying to "reuse" a struct device.
> 
> > With the prefix added, we will get below:
> > 
> > [   47.355582] ath11k_pci 0000:01:00.0: MHI: Requested to power on
> > [   47.355724] ath11k_pci 0000:01:00.0: MHI: Power on setup success
> > 
> > IMO, the prefix will give users a clear idea of logs and that will be very
> > useful for debugging.
> > 
> > Hope this clarifies.
> 
> Don't try to reuse struct devices, if you are a bus, have your own
> devices as that's the correct way to do things.
> 

I assumed that the buses relying on a different physical interface for the
actual communication can reuse the `struct device`. I can see that the MOXTET
bus driver already doing it. It reuses the `struct device` of SPI.

And this assumption has deep rooted in MHI bus design.

Thanks,
Mani

> thanks,
> 
> greg k-h
Manivannan Sadhasivam Feb. 17, 2020, 11:53 a.m. UTC | #8
On Mon, Feb 17, 2020 at 12:45:15PM +0100, Greg KH wrote:
> On Mon, Feb 17, 2020 at 10:57:43AM +0530, Manivannan Sadhasivam wrote:
> > Hi Greg,
> > 
> > On Thu, Feb 13, 2020 at 07:53:02AM -0800, Greg KH wrote:
> > > On Thu, Feb 13, 2020 at 09:18:09PM +0530, Manivannan Sadhasivam wrote:
> > > > Hi Greg,
> > > > 
> > > > On Thu, Feb 13, 2020 at 07:34:18AM -0800, Greg KH wrote:
> > > > > On Thu, Feb 13, 2020 at 08:50:13PM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Tue, Feb 11, 2020 at 11:20:55AM -0800, Greg KH wrote:
> > > > > > > On Wed, Feb 12, 2020 at 12:11:30AM +0530, Manivannan Sadhasivam wrote:
> > > > > > > > Hi Greg,
> > > > > > > > 
> > > > > > > > On Thu, Feb 06, 2020 at 05:57:55PM +0100, Greg KH wrote:
> > > > > > > > > On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/drivers/bus/mhi/core/init.c
> > > > > > > > > > @@ -0,0 +1,407 @@
> > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > +/*
> > > > > > > > > > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > > > > > > > > > + *
> > > > > > > > > > + */
> > > > > > > > > > +
> > > > > > > > > > +#define dev_fmt(fmt) "MHI: " fmt
> > > > > > > > > 
> > > > > > > > > This should not be needed, right?  The bus/device name should give you
> > > > > > > > > all you need here from what I can tell.  So why is this needed?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > The log will have only the device name as like PCI-E. But that won't specify
> > > > > > > > where the error is coming from. Having "MHI" prefix helps the users to
> > > > > > > > quickly identify that the error is coming from MHI stack.
> > > > > > > 
> > > > > > > If the driver binds properly to the device, the name of the driver will
> > > > > > > be there in the message, so I suggest using that please.
> > > > > > > 
> > > > > > > No need for this prefix...
> > > > > > > 
> > > > > > 
> > > > > > So the driver name will be in the log but that won't help identifying where
> > > > > > the log is coming from. This is more important for MHI since it reuses the
> > > > > > `struct device` of the transport device like PCI-E. For instance, below is
> > > > > > the log without MHI prefix:
> > > > > > 
> > > > > > [   47.355582] ath11k_pci 0000:01:00.0: Requested to power on
> > > > > > [   47.355724] ath11k_pci 0000:01:00.0: Power on setup success
> > > > > > 
> > > > > > As you can see, this gives the assumption that the log is coming from the
> > > > > > ath11k_pci driver. But the reality is, it is coming from MHI bus.
> > > > > 
> > > > > Then you should NOT be trying to "reuse" a struct device.
> > > > > 
> > > > > > With the prefix added, we will get below:
> > > > > > 
> > > > > > [   47.355582] ath11k_pci 0000:01:00.0: MHI: Requested to power on
> > > > > > [   47.355724] ath11k_pci 0000:01:00.0: MHI: Power on setup success
> > > > > > 
> > > > > > IMO, the prefix will give users a clear idea of logs and that will be very
> > > > > > useful for debugging.
> > > > > > 
> > > > > > Hope this clarifies.
> > > > > 
> > > > > Don't try to reuse struct devices, if you are a bus, have your own
> > > > > devices as that's the correct way to do things.
> > > > > 
> > > > 
> > > > I assumed that the buses relying on a different physical interface for the
> > > > actual communication can reuse the `struct device`. I can see that the MOXTET
> > > > bus driver already doing it. It reuses the `struct device` of SPI.
> > > 
> > > How can you reuse anything?
> > > 
> > > > And this assumption has deep rooted in MHI bus design.
> > > 
> > > Maybe I do not understand what this is at all, but a device can only be
> > > on one "bus" at a time.  How is that being broken here?
> > > 
> > 
> > Let me share some insight on how it is being used:
> > 
> > The MHI bus sits on top of the actual physical bus like PCI-E and requires
> > the physical bus for doing activities like allocating I/O virtual address,
> > runtime PM etc... The part which gets tied to the PCI-E from MHI is called MHI
> > controller driver. This MHI controller driver is also the actual PCI-E driver
> > managing the device.
> > 
> > For instance, we have QCA6390 PCI-E WLAN device. For this device, there is a
> > ath11k PCI-E driver and the same driver also registers as a MHI controller and
> > acts as a MHI controller driver. This is where I referred to reusing the PCI-E
> > struct device. It's not that MHI bus itself is reusing the PCI-E struct device
> > but we need the PCI-E device pointer to do above mentioned IOVA, PM operations
> > in some places. One of the usage is below:
> > 
> > ```
> > void *buf = dma_alloc_coherent(mhi_cntrl->dev, size, dma_handle, gfp);
> > ```
> > 
> > There was some discussion about it here: https://lkml.org/lkml/2020/1/27/21
> > 
> > The MHI bus itself has the struct device and it is the child of the physical
> > bus (PCI-E in this case).
> > 
> > Now coming to your actual question of why using a custom "MHI" prefix for
> > dev_ APIs. I agree that if we use the struct device of MHI bus it is not at all
> > needed. The fact that we are using "mhi_cntrl->dev" (which points to PCI-E dev)
> > is what confusing and it can be avoided.
> 
> You should also rename "dev" there, as that is really a "pci device".
> So use the real pci device and name it as "parent_pci" or something like
> that, so we know just by looking at it as to what it really is.
> 
> Especially as traditionally "->dev" is the device structure for _this_
> device, not another one.
> 

Okay, sure. I'll rename it as "pdev" to indicate it as the parent device.

Thanks,
Mani

> thanks,
> 
> greg k-h
Manivannan Sadhasivam Feb. 17, 2020, 1:04 p.m. UTC | #9
On Mon, Feb 17, 2020 at 12:59:30PM +0100, Greg KH wrote:
> On Mon, Feb 17, 2020 at 10:57:43AM +0530, Manivannan Sadhasivam wrote:
> > Hi Greg,
> > 
> > On Thu, Feb 13, 2020 at 07:53:02AM -0800, Greg KH wrote:
> > > On Thu, Feb 13, 2020 at 09:18:09PM +0530, Manivannan Sadhasivam wrote:
> > > > Hi Greg,
> > > > 
> > > > On Thu, Feb 13, 2020 at 07:34:18AM -0800, Greg KH wrote:
> > > > > On Thu, Feb 13, 2020 at 08:50:13PM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Tue, Feb 11, 2020 at 11:20:55AM -0800, Greg KH wrote:
> > > > > > > On Wed, Feb 12, 2020 at 12:11:30AM +0530, Manivannan Sadhasivam wrote:
> > > > > > > > Hi Greg,
> > > > > > > > 
> > > > > > > > On Thu, Feb 06, 2020 at 05:57:55PM +0100, Greg KH wrote:
> > > > > > > > > On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/drivers/bus/mhi/core/init.c
> > > > > > > > > > @@ -0,0 +1,407 @@
> > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > +/*
> > > > > > > > > > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > > > > > > > > > + *
> > > > > > > > > > + */
> > > > > > > > > > +
> > > > > > > > > > +#define dev_fmt(fmt) "MHI: " fmt
> > > > > > > > > 
> > > > > > > > > This should not be needed, right?  The bus/device name should give you
> > > > > > > > > all you need here from what I can tell.  So why is this needed?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > The log will have only the device name as like PCI-E. But that won't specify
> > > > > > > > where the error is coming from. Having "MHI" prefix helps the users to
> > > > > > > > quickly identify that the error is coming from MHI stack.
> > > > > > > 
> > > > > > > If the driver binds properly to the device, the name of the driver will
> > > > > > > be there in the message, so I suggest using that please.
> > > > > > > 
> > > > > > > No need for this prefix...
> > > > > > > 
> > > > > > 
> > > > > > So the driver name will be in the log but that won't help identifying where
> > > > > > the log is coming from. This is more important for MHI since it reuses the
> > > > > > `struct device` of the transport device like PCI-E. For instance, below is
> > > > > > the log without MHI prefix:
> > > > > > 
> > > > > > [   47.355582] ath11k_pci 0000:01:00.0: Requested to power on
> > > > > > [   47.355724] ath11k_pci 0000:01:00.0: Power on setup success
> > > > > > 
> > > > > > As you can see, this gives the assumption that the log is coming from the
> > > > > > ath11k_pci driver. But the reality is, it is coming from MHI bus.
> > > > > 
> > > > > Then you should NOT be trying to "reuse" a struct device.
> > > > > 
> > > > > > With the prefix added, we will get below:
> > > > > > 
> > > > > > [   47.355582] ath11k_pci 0000:01:00.0: MHI: Requested to power on
> > > > > > [   47.355724] ath11k_pci 0000:01:00.0: MHI: Power on setup success
> > > > > > 
> > > > > > IMO, the prefix will give users a clear idea of logs and that will be very
> > > > > > useful for debugging.
> > > > > > 
> > > > > > Hope this clarifies.
> > > > > 
> > > > > Don't try to reuse struct devices, if you are a bus, have your own
> > > > > devices as that's the correct way to do things.
> > > > > 
> > > > 
> > > > I assumed that the buses relying on a different physical interface for the
> > > > actual communication can reuse the `struct device`. I can see that the MOXTET
> > > > bus driver already doing it. It reuses the `struct device` of SPI.
> > > 
> > > How can you reuse anything?
> > > 
> > > > And this assumption has deep rooted in MHI bus design.
> > > 
> > > Maybe I do not understand what this is at all, but a device can only be
> > > on one "bus" at a time.  How is that being broken here?
> > > 
> > 
> > Let me share some insight on how it is being used:
> > 
> > The MHI bus sits on top of the actual physical bus like PCI-E and requires
> > the physical bus for doing activities like allocating I/O virtual address,
> > runtime PM etc... The part which gets tied to the PCI-E from MHI is called MHI
> > controller driver. This MHI controller driver is also the actual PCI-E driver
> > managing the device.
> > 
> > For instance, we have QCA6390 PCI-E WLAN device. For this device, there is a
> > ath11k PCI-E driver and the same driver also registers as a MHI controller and
> > acts as a MHI controller driver. This is where I referred to reusing the PCI-E
> > struct device. It's not that MHI bus itself is reusing the PCI-E struct device
> > but we need the PCI-E device pointer to do above mentioned IOVA, PM operations
> > in some places. One of the usage is below:
> > 
> > ```
> > void *buf = dma_alloc_coherent(mhi_cntrl->dev, size, dma_handle, gfp);
> > ```
> 
> Wait, why do you need to call this with the parent dev?  Why not with
> your struct device?  What does the parent pointer have that yours does
> not?  Is it not correctly having whatever dma attributes the parent has
> set properly for your device as well?  If not, why not just fix that and
> then _your_ device can be doing the allocation?
> 

This is _one_ of the usecases of the parent dev. We are also using it to manage
the runtime PM operations of the physical device (pcie) when the MHI stack goes
into respective states. For instance,

```
        if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
                mhi_cntrl->runtime_get(mhi_cntrl);
                mhi_cntrl->runtime_put(mhi_cntrl);
        }
```

These runtime_put() and runtime_get() are the callbacks to be provided by the
controller drivers for managing its runtime PM states.

Also, the MHI devices for the channels will be created later on after the
controller probe, so at that time we need this parent dev to set the MHI device
parent:

```
struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl)
{
...
dev->parent = mhi_cntrl->dev;
...
```

Hence, having the parent dev pointer really helps.

Thanks,
Mani
> thanks,
> 
> greg k-h
diff mbox series

Patch

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
+#
+# 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..a00f1547d252
--- /dev/null
+++ b/drivers/bus/mhi/core/init.c
@@ -0,0 +1,407 @@ 
+// 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;
+
+	if (!mhi_cntrl)
+		return -EINVAL;
+
+	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", dev_name(mhi_cntrl->dev));
+
+	/* 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;
+	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..86a07be2e038
--- /dev/null
+++ b/drivers/bus/mhi/core/internal.h
@@ -0,0 +1,163 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
+ *
+ */
+
+#ifndef _MHI_INT_H
+#define _MHI_INT_H
+
+#include <linux/mhi.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, /* End of transfer event */
+	MHI_EV_CC_OVERFLOW = 0x3,
+	MHI_EV_CC_EOB = 0x4, /* End of block event */
+	MHI_EV_CC_OOB = 0x5, /* Out of block event */
+	MHI_EV_CC_DB_MODE = 0x6,
+	MHI_EV_CC_UNDEFINED_ERR = 0x10,
+	MHI_EV_CC_BAD_TRE = 0x11,
+};
+
+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,
+};
+
+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 {
+	void *v_addr;
+	void *bb_addr;
+	void *wp;
+	void *cb_buf;
+	dma_addr_t p_addr;
+	size_t len;
+	enum dma_data_direction dir;
+};
+
+struct mhi_event {
+	struct mhi_controller *mhi_cntrl;
+	struct mhi_chan *mhi_chan; /* dedicated to channel */
+	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;
+	struct tasklet_struct task;
+	spinlock_t lock;
+	int (*process_event)(struct mhi_controller *mhi_cntrl,
+			     struct mhi_event *mhi_event,
+			     u32 event_quota);
+	bool hw_ring;
+	bool cl_manage;
+	bool offload_ev; /* managed by a device driver */
+};
+
+struct mhi_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 chan;
+	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;
+	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;
+	bool lpm_notify;
+	bool configured;
+	bool offload_ch;
+	bool pre_alloc;
+	bool auto_start;
+	bool wake_capable;
+};
+
+/* 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)
+{
+	put_device(&mhi_dev->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..02330236ced8
--- /dev/null
+++ b/include/linux/mhi.h
@@ -0,0 +1,420 @@ 
+/* 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: Reserved Side Coalesced (RSC) type premapped buffer
+ */
+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_ch_ee_mask - Execution environment mask for channel
+ * @MHI_CH_EE_PBL: Allow channel to be used in PBL EE
+ * @MHI_CH_EE_SBL: Allow channel to be used in SBL EE
+ * @MHI_CH_EE_AMSS: Allow channel to be used in AMSS EE
+ * @MHI_CH_EE_RDDM: Allow channel to be used in RDDM EE
+ * @MHI_CH_EE_PTHRU: Allow channel to be used in PTHRU EE
+ * @MHI_CH_EE_WFW: Allow channel to be used in WFW EE
+ * @MHI_CH_EE_EDL: Allow channel to be used in EDL EE
+ */
+enum mhi_ch_ee_mask {
+	MHI_CH_EE_PBL = BIT(MHI_EE_PBL),
+	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),
+};
+
+/**
+ * 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
+ * @name: The name of this channel
+ * @num: The number assigned to 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
+ * @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 {
+	char *name;
+	u32 num;
+	u32 num_elements;
+	u32 local_elements;
+	u32 event_ring;
+	enum dma_data_direction dir;
+	enum mhi_ch_type type;
+	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
+ * @priority: Priority of this ring. Use 1 for now
+ * @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
+ */
+struct mhi_event_config {
+	u32 num_elements;
+	u32 irq_moderation_ms;
+	u32 irq;
+	u32 channel;
+	u32 priority;
+	enum mhi_db_brst_mode mode;
+	enum mhi_er_data_type data_type;
+	bool hardware_event;
+	bool client_managed;
+	bool offload_channel;
+};
+
+/**
+ * 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
+ * @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
+ * @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
+ */
+struct mhi_controller_config {
+	u32 max_channels;
+	u32 timeout_ms;
+	u32 buf_len;
+	u32 num_channels;
+	struct mhi_channel_config *ch_cfg;
+	u32 num_events;
+	struct mhi_event_config *event_cfg;
+	bool use_bounce_buf;
+	bool m2_no_db;
+};
+
+/**
+ * struct mhi_controller - Master MHI controller structure
+ * @dev: Driver model device node for the controller (required)
+ * @mhi_dev: MHI device instance for the controller
+ * @regs: Base address of MHI MMIO register space (required)
+ * @iova_start: IOMMU starting address for data (required)
+ * @iova_stop: IOMMU stop address for data (required)
+ * @fw_image: Firmware image name for normal booting (required)
+ * @edl_image: Firmware image name for emergency download mode (optional)
+ * @sbl_size: SBL image size downloaded through BHIe (optional)
+ * @seg_len: BHIe vector size (optional)
+ * @mhi_chan: Points to the channel configuration table
+ * @lpm_chans: List of channels that require LPM notifications
+ * @irq: base irq # to request (required)
+ * @max_chan: Maximum number of channels the controller supports
+ * @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 (optional)
+ * @nr_irqs: Number of IRQ allocated by bus master (required)
+ * @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
+ * @pm_mutex: Mutex for suspend/resume operation
+ * @pm_lock: Lock for protecting MHI power management state
+ * @timeout_ms: Timeout in ms for state transitions
+ * @pm_state: MHI power management state
+ * @db_access: DB access states
+ * @ee: MHI device execution environment
+ * @dev_wake: Device wakeup count
+ * @pending_pkts: Pending packets for the controller
+ * @transition_list: List of MHI state transitions
+ * @transition_lock: Lock for protecting MHI state transition list
+ * @wlock: Lock for protecting device wakeup
+ * @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 power states of the device (required)
+ * @link_status: CB function to query link status of the device (required)
+ * @wake_get: CB function to assert device wake (optional)
+ * @wake_put: CB function to de-assert device wake (optional)
+ * @wake_toggle: CB function to assert and de-assert device wake (optional)
+ * @runtime_get: CB function to controller runtime resume (required)
+ * @runtimet_put: CB function to decrement pm usage (required)
+ * @buffer_len: Bounce buffer length
+ * @bounce_buf: Use of bounce buffer
+ * @fbc_download: MHI host needs to do complete image transfer (optional)
+ * @pre_init: MHI host needs to do pre-initialization before power up
+ * @wake_set: Device wakeup set flag
+ */
+struct mhi_controller {
+	struct device *dev;
+	struct mhi_device *mhi_dev;
+	void __iomem *regs;
+	dma_addr_t iova_start;
+	dma_addr_t iova_stop;
+	const char *fw_image;
+	const char *edl_image;
+	size_t sbl_size;
+	size_t seg_len;
+	struct mhi_chan *mhi_chan;
+	struct list_head lpm_chans;
+	int *irq;
+	u32 max_chan;
+	u32 total_ev_rings;
+	u32 hw_ev_rings;
+	u32 sw_ev_rings;
+	u32 nr_irqs_req;
+	u32 nr_irqs;
+
+	struct mhi_event *mhi_event;
+	struct mhi_cmd *mhi_cmd;
+	struct mhi_ctxt *mhi_ctxt;
+
+	struct mutex pm_mutex;
+	rwlock_t pm_lock;
+	u32 timeout_ms;
+	u32 pm_state;
+	u32 db_access;
+	enum mhi_ee_type ee;
+	atomic_t dev_wake;
+	atomic_t pending_pkts;
+	struct list_head transition_list;
+	spinlock_t transition_lock;
+	spinlock_t wlock;
+	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, enum mhi_callback cb);
+	int (*link_status)(struct mhi_controller *mhi_cntrl);
+	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 (*runtime_put)(struct mhi_controller *mhi_cntrl);
+
+	size_t buffer_len;
+	bool bounce_buf;
+	bool fbc_download;
+	bool pre_init;
+	bool wake_set;
+};
+
+/**
+ * struct mhi_device - Structure representing a MHI device which binds
+ *                     to channels
+ * @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: Driver model device node for the MHI device
+ * @dev_type: MHI device type
+ * @tiocm: Device current terminal settings
+ * @dev_wake: Device wakeup counter
+ */
+struct mhi_device {
+	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;
+	struct device dev;
+	enum mhi_device_type dev_type;
+	u32 tiocm;
+	u32 dev_wake;
+};
+
+/**
+ * struct mhi_result - Completed buffer information
+ * @buf_addr: Address of data buffer
+ * @bytes_xferd: # of bytes transferred
+ * @dir: Channel direction
+ * @transaction_status: Status of last transaction
+ */
+struct mhi_result {
+	void *buf_addr;
+	size_t bytes_xferd;
+	enum dma_data_direction dir;
+	int transaction_status;
+};
+
+#define to_mhi_device(dev) container_of(dev, struct mhi_device, dev)
+
+/**
+ * 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 */