[PATCH] bus: mhi: Add MHI PCI support

[PATCH] bus: mhi: Add MHI PCI support

[Loic Poulain]

This is a generic MHI-over-PCI controller driver for MHI only devices
such as QCOM modems. For now it supports registering of Qualcomm SDX55
based PCIe modules. The MHI channels have been extracted from mhi
downstream driver.

This driver is easily extendable to support other MHI PCI devices like
different modem hw or OEM superset.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/Kconfig                      |   7 +
 drivers/bus/mhi/Makefile                     |   1 +
 drivers/bus/mhi/controllers/Makefile         |   1 +
 drivers/bus/mhi/controllers/mhi_pci_common.c | 339 +++++++++++++++++++++++++++
 4 files changed, 348 insertions(+)
 create mode 100644 drivers/bus/mhi/controllers/Makefile
 create mode 100644 drivers/bus/mhi/controllers/mhi_pci_common.c

diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
index e841c10..ef66527 100644
--- a/drivers/bus/mhi/Kconfig
+++ b/drivers/bus/mhi/Kconfig
@@ -20,3 +20,10 @@ config MHI_BUS_DEBUG
 	  Enable debugfs support for use with the MHI transport. Allows
 	  reading and/or modifying some values within the MHI controller
 	  for debug and test purposes.
+
+config MHI_BUS_PCI
+	tristate "Modem Host Interface (MHI) bus over PCI"
+	depends on MHI_BUS
+	depends on PCI
+	help
+	  This driver provides support for MHI controller drivers over PCI.
diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile
index 19e6443..2f1a20c 100644
--- a/drivers/bus/mhi/Makefile
+++ b/drivers/bus/mhi/Makefile
@@ -1,2 +1,3 @@
 # core layer
 obj-y += core/
+obj-y += controllers/
diff --git a/drivers/bus/mhi/controllers/Makefile b/drivers/bus/mhi/controllers/Makefile
new file mode 100644
index 0000000..10cd4f9
--- /dev/null
+++ b/drivers/bus/mhi/controllers/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_MHI_BUS_PCI) := mhi_pci_common.o
diff --git a/drivers/bus/mhi/controllers/mhi_pci_common.c b/drivers/bus/mhi/controllers/mhi_pci_common.c
new file mode 100644
index 0000000..705b283
--- /dev/null
+++ b/drivers/bus/mhi/controllers/mhi_pci_common.c
@@ -0,0 +1,339 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MHI PCI driver - MHI over PCI controller driver
+ *
+ * This module is a generic driver for registering pure MHI-over-PCI devices,
+ * such as PCIe QCOM modems.
+ *
+ * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
+ */
+
+#include <linux/device.h>
+#include <linux/mhi.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+
+#define MHI_PCI_BAR_NUM 0
+
+enum {
+	EV_RING_0,
+	EV_RING_1,
+	EV_RING_2,
+	EV_RING_3,
+};
+
+struct mhi_pci_dev_info {
+	const struct mhi_controller_config *config;
+	const char *name;
+};
+
+#define MHI_CHANNEL_CONFIG_SIMPLE_TX(cnum, cname, elems, event)	\
+	{							\
+		.num = cnum,					\
+		.name = cname,					\
+		.num_elements = elems,				\
+		.event_ring = event,				\
+		.dir = DMA_TO_DEVICE,				\
+		.ee_mask = BIT(MHI_EE_AMSS),			\
+		.pollcfg = 0,					\
+		.doorbell = MHI_DB_BRST_DISABLE,		\
+		.lpm_notify = false,				\
+		.offload_channel = false,			\
+		.doorbell_mode_switch = false,			\
+		.auto_queue = false,				\
+	}							\
+
+#define MHI_CHANNEL_CONFIG_SIMPLE_RX(cnum, cname, elems, event) \
+	{							\
+		.num = cnum,					\
+		.name = cname,					\
+		.num_elements = elems,				\
+		.event_ring = event,				\
+		.dir = DMA_FROM_DEVICE,				\
+		.ee_mask = BIT(MHI_EE_AMSS),			\
+		.pollcfg = 0,					\
+		.doorbell = MHI_DB_BRST_DISABLE,		\
+		.lpm_notify = false,				\
+		.offload_channel = false,			\
+		.doorbell_mode_switch = false,			\
+		.auto_queue = false,				\
+	}
+
+#define MHI_CHANNEL_CONFIG_PREALLOC_RX(cnum, cname, elems, event) \
+	{							\
+		.num = cnum,					\
+		.name = cname,					\
+		.num_elements = elems,				\
+		.event_ring = event,				\
+		.dir = DMA_FROM_DEVICE,				\
+		.ee_mask = BIT(MHI_EE_AMSS),			\
+		.pollcfg = 0,					\
+		.doorbell = MHI_DB_BRST_DISABLE,		\
+		.lpm_notify = false,				\
+		.offload_channel = false,			\
+		.doorbell_mode_switch = false,			\
+		.auto_queue = true,				\
+	}
+
+#define MHI_EVENT_CONFIG_SIMPLE_CTRL(enum)	\
+	{					\
+		.num_elements = 64,		\
+		.irq_moderation_ms = 5,		\
+		.irq = enum,			\
+		.priority = 1,			\
+		.mode = MHI_DB_BRST_DISABLE,	\
+		.data_type = MHI_ER_CTRL,	\
+		.hardware_event = false,	\
+		.client_managed = false,	\
+		.offload_channel = false,	\
+	}
+
+#define MHI_EVENT_CONFIG_SIMPLE_DATA(enum)	\
+	{					\
+		.num_elements = 64,		\
+		.irq_moderation_ms = 5,		\
+		.irq = enum,			\
+		.priority = 1,			\
+		.mode = MHI_DB_BRST_DISABLE,	\
+		.data_type = MHI_ER_DATA,	\
+		.hardware_event = false,	\
+		.client_managed = false,	\
+		.offload_channel = false,	\
+	}
+
+static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = {
+	MHI_CHANNEL_CONFIG_SIMPLE_TX(12, "MBIM", 4, EV_RING_0),
+	MHI_CHANNEL_CONFIG_SIMPLE_RX(13, "MBIM", 4, EV_RING_0),
+	MHI_CHANNEL_CONFIG_SIMPLE_TX(14, "QMI", 4, EV_RING_0),
+	MHI_CHANNEL_CONFIG_SIMPLE_RX(15, "QMI", 4, EV_RING_0),
+	MHI_CHANNEL_CONFIG_SIMPLE_TX(20, "IPCR", 16, EV_RING_0),
+	/* qrtr-mhi expects pre-allocated/pre-queued RX buffers */
+	MHI_CHANNEL_CONFIG_PREALLOC_RX(21, "IPCR", 4, EV_RING_0),
+	MHI_CHANNEL_CONFIG_SIMPLE_TX(34, "IP_SW0", 16, EV_RING_1),
+	MHI_CHANNEL_CONFIG_SIMPLE_RX(35, "IP_SW0", 16, EV_RING_1)
+};
+
+static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {
+	/* first ring is control+data ring */
+	MHI_EVENT_CONFIG_SIMPLE_CTRL(EV_RING_0),
+	/* dedicated ring for IP data */
+	MHI_EVENT_CONFIG_SIMPLE_DATA(EV_RING_1)
+};
+
+static const struct mhi_controller_config modem_qcom_v1_mhi_config = {
+	.max_channels = 128,
+	.timeout_ms = 5000,
+	.num_channels = ARRAY_SIZE(modem_qcom_v1_mhi_channels),
+	.ch_cfg = modem_qcom_v1_mhi_channels,
+	.num_events = ARRAY_SIZE(modem_qcom_v1_mhi_events),
+	.event_cfg = modem_qcom_v1_mhi_events,
+};
+
+static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
+	.name = "qcom-sdx55",
+	.config = &modem_qcom_v1_mhi_config
+};
+
+static const struct pci_device_id mhi_pci_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, 0x0306),
+		.driver_data = (kernel_ulong_t) &mhi_qcom_sdx55_info },
+	{  }
+};
+MODULE_DEVICE_TABLE(pci, mhi_pci_id_table);
+
+static int mhi_pci_read_reg(struct mhi_controller *mhic, void __iomem *addr,
+			    u32 *out)
+{
+	 *out = readl(addr);
+	 return 0;
+}
+
+static void mhi_pci_write_reg(struct mhi_controller *mhic, void __iomem *addr,
+			      u32 val)
+{
+	writel(val, addr);
+}
+
+static void mhi_pci_status_cb(struct mhi_controller *mhi_cntrl,
+			      enum mhi_callback cb)
+{
+	return;
+}
+
+static int mhi_pci_claim(struct mhi_controller *mhic)
+{
+	struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev);
+	int err;
+
+	err = pci_assign_resource(pdev, MHI_PCI_BAR_NUM);
+	if (err) {
+		dev_err(&pdev->dev, "failed to assign pci resource: %d\n", err);
+		return err;
+	}
+
+	err = pcim_enable_device(pdev);
+	if (err) {
+		dev_err(&pdev->dev, "failed to enable pci device: %d\n", err);
+		return err;
+	}
+
+	err = pcim_iomap_regions(pdev, 1 << MHI_PCI_BAR_NUM, pci_name(pdev));
+	if (err) {
+		dev_err(&pdev->dev, "failed to map pci region: %d\n", err);
+		return err;
+	}
+	mhic->regs = pcim_iomap_table(pdev)[MHI_PCI_BAR_NUM];
+
+	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
+	if (err) {
+		dev_err(&pdev->dev, "Cannot set proper DMA mask\n");
+		return err;
+	}
+
+	err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
+	if (err) {
+		dev_err(&pdev->dev, "set consistent dma mask failed\n");
+		return err;
+	}
+
+	pci_set_master(pdev);
+
+	return 0;
+}
+
+static int mhi_pci_get_irqs(struct mhi_controller *mhic,
+			    const struct mhi_controller_config *mhic_config)
+{
+	struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev);
+	int num_vectors, i;
+	int *irq;
+
+	/* Alloc one MSI vector per event ring, ideally... */
+	num_vectors = pci_alloc_irq_vectors(pdev, 1, mhic_config->num_events,
+					    PCI_IRQ_MSI);
+	if (num_vectors < 0) {
+		dev_err(&pdev->dev, "Error allocating MSI vectors %d\n",
+			num_vectors);
+		return num_vectors;
+	}
+
+	if (num_vectors < mhic_config->num_events) {
+		dev_warn(&pdev->dev, "Not enough MSI vectors (%d/%d)\n",
+			 num_vectors, mhic_config->num_events);
+		/* use shared IRQ */
+	}
+
+	irq = devm_kcalloc(&pdev->dev, mhic_config->num_events, sizeof(int),
+			   GFP_KERNEL);
+	if (!irq)
+		return -ENOMEM;
+
+	for (i = 0; i < mhic_config->num_events; i++) {
+		int vector = i >= num_vectors ? (num_vectors - 1) : i;
+
+		irq[i] = pci_irq_vector(pdev, vector);
+	}
+
+	mhic->irq = irq;
+	mhic->nr_irqs = mhic_config->num_events;
+
+	return 0;
+}
+
+static int mhi_pci_runtime_get(struct mhi_controller *mhi_cntrl)
+{
+	/* no PM for now */
+	return 0;
+}
+
+static void mhi_pci_runtime_put(struct mhi_controller *mhi_cntrl)
+{
+	/* no PM for now */
+	return;
+}
+
+static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
+	const struct mhi_controller_config *mhic_config;
+	struct mhi_controller *mhic;
+	int err;
+
+	dev_info(&pdev->dev, "MHI PCI device found: %s\n", info->name);
+
+	mhic = devm_kzalloc(&pdev->dev, sizeof(*mhic), GFP_KERNEL);
+	if (!mhic) {
+		dev_err(&pdev->dev, "failed to allocate mhi controller\n");
+		return -ENOMEM;
+	}
+
+	mhic_config = info->config;
+	mhic->cntrl_dev = &pdev->dev;
+	mhic->iova_start = 0;
+	mhic->iova_stop = 0xffffffff;
+	mhic->fw_image = NULL;
+	mhic->edl_image = NULL;
+
+	mhic->read_reg = mhi_pci_read_reg;
+	mhic->write_reg = mhi_pci_write_reg;
+	mhic->status_cb = mhi_pci_status_cb;
+	mhic->runtime_get = mhi_pci_runtime_get;
+	mhic->runtime_put = mhi_pci_runtime_put;
+
+	err = mhi_pci_claim(mhic);
+	if (err)
+		return err;
+
+	err = mhi_pci_get_irqs(mhic, mhic_config);
+	if (err)
+		return err;
+
+	pci_set_drvdata(pdev, mhic);
+
+	err = mhi_register_controller(mhic, mhic_config);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register MHI controller\n");
+		pci_free_irq_vectors(pdev);
+		return err;
+	}
+
+	/* MHI bus does not power up the controller by default */
+	err = mhi_prepare_for_power_up(mhic);
+	if (err) {
+		dev_err(&pdev->dev, "failed to prepare MHI controller\n");
+		return err;
+	}
+
+	err = mhi_sync_power_up(mhic);
+	if (err) {
+		dev_err(&pdev->dev, "failed to power up MHI controller\n");
+		mhi_unprepare_after_power_down(mhic);
+		mhi_unregister_controller(mhic);
+		return err;
+	}
+
+	return 0;
+}
+
+static void mhi_pci_remove(struct pci_dev *pdev)
+{
+	struct mhi_controller *mhic = pci_get_drvdata(pdev);
+
+	mhi_power_down(mhic, true);
+	mhi_unprepare_after_power_down(mhic);
+	mhi_unregister_controller(mhic);
+}
+
+static struct pci_driver mhi_pci_driver = {
+	.name		= "mhi-pci",
+	.id_table	= mhi_pci_id_table,
+	.probe		= mhi_pci_probe,
+	.remove		= mhi_pci_remove
+};
+module_pci_driver(mhi_pci_driver);
+
+MODULE_AUTHOR("Loic Poulain <loic.poulain@linaro,org>");
+MODULE_DESCRIPTION("mhi-pci");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1");
-- 
2.7.4

Re: [PATCH] bus: mhi: Add MHI PCI support

[Jeffrey Hugo]

On 9/23/2020 7:40 AM, Loic Poulain wrote:
> This is a generic MHI-over-PCI controller driver for MHI only devices
> such as QCOM modems. For now it supports registering of Qualcomm SDX55
> based PCIe modules. The MHI channels have been extracted from mhi
> downstream driver.
> 
> This driver is easily extendable to support other MHI PCI devices like
> different modem hw or OEM superset.
> 

Maybe I'm being a bit dense, but what does this "driver" even do?

Ok, it'll bind to SDX55 and "power up" the MHI.  I don't see any 
communication with the device, so I'm not really seeing the value here. 
  Feels like this should be patch 1 of a series, but patches 2 - X are 
"missing".

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

Re: [PATCH] bus: mhi: Add MHI PCI support

[Loic Poulain]

Hi Jeffrey,

On Wed, 23 Sep 2020 at 16:04, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 9/23/2020 7:40 AM, Loic Poulain wrote:
> > This is a generic MHI-over-PCI controller driver for MHI only devices
> > such as QCOM modems. For now it supports registering of Qualcomm SDX55
> > based PCIe modules. The MHI channels have been extracted from mhi
> > downstream driver.
> >
> > This driver is easily extendable to support other MHI PCI devices like
> > different modem hw or OEM superset.
> >
>
> Maybe I'm being a bit dense, but what does this "driver" even do?
>
> Ok, it'll bind to SDX55 and "power up" the MHI.  I don't see any
> communication with the device, so I'm not really seeing the value here.
>   Feels like this should be patch 1 of a series, but patches 2 - X are
> "missing".

Well, yes and no. On MHI controller side point-of-view, the driver is
quite complete, since its only goal is to implement the MHI transport
layer and to register the available channels. The PCI driver is really
no expected to do more (contrary to ath11k), everything else will be
implemented by MHI device drivers at upper level. I agree most of them
are not yet implemented (only qrtr-mhi for IPCR channel), but I'm
currently working on this.

Regards,
Loic

Re: [PATCH] bus: mhi: Add MHI PCI support

[Jeffrey Hugo]

On 9/23/2020 8:35 AM, Loic Poulain wrote:
> Hi Jeffrey,
> 
> On Wed, 23 Sep 2020 at 16:04, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>>
>> On 9/23/2020 7:40 AM, Loic Poulain wrote:
>>> This is a generic MHI-over-PCI controller driver for MHI only devices
>>> such as QCOM modems. For now it supports registering of Qualcomm SDX55
>>> based PCIe modules. The MHI channels have been extracted from mhi
>>> downstream driver.
>>>
>>> This driver is easily extendable to support other MHI PCI devices like
>>> different modem hw or OEM superset.
>>>
>>
>> Maybe I'm being a bit dense, but what does this "driver" even do?
>>
>> Ok, it'll bind to SDX55 and "power up" the MHI.  I don't see any
>> communication with the device, so I'm not really seeing the value here.
>>    Feels like this should be patch 1 of a series, but patches 2 - X are
>> "missing".
> 
> Well, yes and no. On MHI controller side point-of-view, the driver is
> quite complete, since its only goal is to implement the MHI transport
> layer and to register the available channels. The PCI driver is really
> no expected to do more (contrary to ath11k), everything else will be
> implemented by MHI device drivers at upper level. I agree most of them
> are not yet implemented (only qrtr-mhi for IPCR channel), but I'm
> currently working on this.

Hmm.  I guess I wonder why the functionality provided by this patch 
isn't incorporated into some other driver.  I see why its not really a 
great idea to pick a random client driver (like ipc router) and push the 
responsibility into them.  However, do we hook up these external modems 
to remoteproc?  If we are managing the devices somewhere else (for FW 
loading, SSR, etc), then it would seem like a good place for this 
functionality.

In summary, this feels like a shim driver that is a driver for driver's 
sake, which doesn't feel like the right design.  I don't think I have a 
better suggestion through.  Since I don't have a concrete "this should 
be done some other way" I won't be offended if you ignore this "complaint".

However, assuming you continue with this approach, I think this change 
needs more documentation.  At a minimum I think what you just explained 
to me needs to be in the commit text.

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

Re: [PATCH] bus: mhi: Add MHI PCI support

[Loic Poulain]

On Wed, 23 Sep 2020 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 9/23/2020 8:35 AM, Loic Poulain wrote:
> > Hi Jeffrey,
> >
> > On Wed, 23 Sep 2020 at 16:04, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> >>
> >> On 9/23/2020 7:40 AM, Loic Poulain wrote:
> >>> This is a generic MHI-over-PCI controller driver for MHI only devices
> >>> such as QCOM modems. For now it supports registering of Qualcomm SDX55
> >>> based PCIe modules. The MHI channels have been extracted from mhi
> >>> downstream driver.
> >>>
> >>> This driver is easily extendable to support other MHI PCI devices like
> >>> different modem hw or OEM superset.
> >>>
> >>
> >> Maybe I'm being a bit dense, but what does this "driver" even do?
> >>
> >> Ok, it'll bind to SDX55 and "power up" the MHI.  I don't see any
> >> communication with the device, so I'm not really seeing the value here.
> >>    Feels like this should be patch 1 of a series, but patches 2 - X are
> >> "missing".
> >
> > Well, yes and no. On MHI controller side point-of-view, the driver is
> > quite complete, since its only goal is to implement the MHI transport
> > layer and to register the available channels. The PCI driver is really
> > no expected to do more (contrary to ath11k), everything else will be
> > implemented by MHI device drivers at upper level. I agree most of them
> > are not yet implemented (only qrtr-mhi for IPCR channel), but I'm
> > currently working on this.
>
> Hmm.  I guess I wonder why the functionality provided by this patch
> isn't incorporated into some other driver.  I see why its not really a
> great idea to pick a random client driver (like ipc router) and push the
> responsibility into them.  However, do we hook up these external modems
> to remoteproc?  If we are managing the devices somewhere else (for FW
> loading, SSR, etc), then it would seem like a good place for this
> functionality.

I understand what you mean, but here this driver is a (logical)
controller driver for the MHI bus, in the same way as e.g. I2C adapter
driver is a controller for the I2C bus, then MHI device drivers (e.g.
ipc router) implement the device function/class like e.g. I2C client
driver (though you could argue that MHI channels are not fully
independant devices).

The device is not managed from somewhere else and the firmware loading
is a generic piece of the MHI subsystem/bus. Moreover there is
currently no modem/wwan subsystem in Linux, so there is not really
something else to register with... at least for now.

> In summary, this feels like a shim driver that is a driver for driver's
> sake, which doesn't feel like the right design.  I don't think I have a
> better suggestion through.  Since I don't have a concrete "this should
> be done some other way" I won't be offended if you ignore this "complaint".
>
> However, assuming you continue with this approach, I think this change
> needs more documentation.  At a minimum I think what you just explained
> to me needs to be in the commit text.

I will.

Thanks,
Loic

Re: [PATCH] bus: mhi: Add MHI PCI support

[Bjorn Andersson]

On Wed 23 Sep 08:40 CDT 2020, Loic Poulain wrote:

> This is a generic MHI-over-PCI controller driver for MHI only devices
> such as QCOM modems. For now it supports registering of Qualcomm SDX55
> based PCIe modules. The MHI channels have been extracted from mhi
> downstream driver.
> 
> This driver is easily extendable to support other MHI PCI devices like
> different modem hw or OEM superset.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
[..]
> diff --git a/drivers/bus/mhi/controllers/mhi_pci_common.c b/drivers/bus/mhi/controllers/mhi_pci_common.c
> new file mode 100644
> index 0000000..705b283
> --- /dev/null
> +++ b/drivers/bus/mhi/controllers/mhi_pci_common.c

ath11k is also "PCI" and "MHI" but this isn't "common" code for it, so
perhaps "mhi_pci_generic.c" or "mhi_pci_modem.c"?

> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MHI PCI driver - MHI over PCI controller driver
> + *
> + * This module is a generic driver for registering pure MHI-over-PCI devices,
> + * such as PCIe QCOM modems.
> + *
> + * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/mhi.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/delay.h>

Can you confirm that you need delay.h?

> +
> +#define MHI_PCI_BAR_NUM 0
> +
> +enum {
> +	EV_RING_0,
> +	EV_RING_1,
> +	EV_RING_2,
> +	EV_RING_3,

Maybe you can just use the numbers 0-3 instead?

> +};
> +
[..]
> +static int mhi_pci_claim(struct mhi_controller *mhic)
> +{
> +	struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev);
> +	int err;
> +
> +	err = pci_assign_resource(pdev, MHI_PCI_BAR_NUM);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to assign pci resource: %d\n", err);

Afaict all code paths of pci_assign_resource() will log already.

> +		return err;
> +	}
> +
> +	err = pcim_enable_device(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to enable pci device: %d\n", err);
> +		return err;
> +	}
> +
> +	err = pcim_iomap_regions(pdev, 1 << MHI_PCI_BAR_NUM, pci_name(pdev));
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to map pci region: %d\n", err);
> +		return err;
> +	}
> +	mhic->regs = pcim_iomap_table(pdev)[MHI_PCI_BAR_NUM];
> +
> +	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> +	if (err) {
> +		dev_err(&pdev->dev, "Cannot set proper DMA mask\n");
> +		return err;
> +	}
> +
> +	err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
> +	if (err) {
> +		dev_err(&pdev->dev, "set consistent dma mask failed\n");
> +		return err;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	return 0;
> +}
[..]
> +static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
> +	const struct mhi_controller_config *mhic_config;
> +	struct mhi_controller *mhic;
> +	int err;
> +
> +	dev_info(&pdev->dev, "MHI PCI device found: %s\n", info->name);
> +
> +	mhic = devm_kzalloc(&pdev->dev, sizeof(*mhic), GFP_KERNEL);
> +	if (!mhic) {
> +		dev_err(&pdev->dev, "failed to allocate mhi controller\n");

kzalloc() will log errors as well.

> +		return -ENOMEM;
> +	}
> +
> +	mhic_config = info->config;
> +	mhic->cntrl_dev = &pdev->dev;
> +	mhic->iova_start = 0;
> +	mhic->iova_stop = 0xffffffff;
> +	mhic->fw_image = NULL;
> +	mhic->edl_image = NULL;
> +
> +	mhic->read_reg = mhi_pci_read_reg;
> +	mhic->write_reg = mhi_pci_write_reg;
> +	mhic->status_cb = mhi_pci_status_cb;
> +	mhic->runtime_get = mhi_pci_runtime_get;
> +	mhic->runtime_put = mhi_pci_runtime_put;
> +
> +	err = mhi_pci_claim(mhic);
> +	if (err)
> +		return err;
> +
> +	err = mhi_pci_get_irqs(mhic, mhic_config);
> +	if (err)
> +		return err;
> +
> +	pci_set_drvdata(pdev, mhic);
> +
> +	err = mhi_register_controller(mhic, mhic_config);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to register MHI controller\n");

Afaict all errors that occurs if you pass valid data to this function
will print an error message already...

> +		pci_free_irq_vectors(pdev);
> +		return err;
> +	}
> +
> +	/* MHI bus does not power up the controller by default */
> +	err = mhi_prepare_for_power_up(mhic);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to prepare MHI controller\n");

mhi_unregister_controller()?

> +		return err;
> +	}
> +
> +	err = mhi_sync_power_up(mhic);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to power up MHI controller\n");
> +		mhi_unprepare_after_power_down(mhic);
> +		mhi_unregister_controller(mhic);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mhi_pci_remove(struct pci_dev *pdev)
> +{
> +	struct mhi_controller *mhic = pci_get_drvdata(pdev);
> +
> +	mhi_power_down(mhic, true);
> +	mhi_unprepare_after_power_down(mhic);
> +	mhi_unregister_controller(mhic);
> +}
> +
> +static struct pci_driver mhi_pci_driver = {
> +	.name		= "mhi-pci",
> +	.id_table	= mhi_pci_id_table,
> +	.probe		= mhi_pci_probe,
> +	.remove		= mhi_pci_remove
> +};
> +module_pci_driver(mhi_pci_driver);
> +
> +MODULE_AUTHOR("Loic Poulain <loic.poulain@linaro,org>");
> +MODULE_DESCRIPTION("mhi-pci");

Please expand this

> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1");

And please drop the version, as this is likely not going to be updated
consistently.

Regards,
Bjorn

Re: [PATCH] bus: mhi: Add MHI PCI support

[Jeffrey Hugo]

On 9/23/2020 9:17 AM, Bjorn Andersson wrote:
> On Wed 23 Sep 08:40 CDT 2020, Loic Poulain wrote:
> 
>> This is a generic MHI-over-PCI controller driver for MHI only devices
>> such as QCOM modems. For now it supports registering of Qualcomm SDX55
>> based PCIe modules. The MHI channels have been extracted from mhi
>> downstream driver.
>>
>> This driver is easily extendable to support other MHI PCI devices like
>> different modem hw or OEM superset.
>>
>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>> ---
> [..]
>> diff --git a/drivers/bus/mhi/controllers/mhi_pci_common.c b/drivers/bus/mhi/controllers/mhi_pci_common.c
>> new file mode 100644
>> index 0000000..705b283
>> --- /dev/null
>> +++ b/drivers/bus/mhi/controllers/mhi_pci_common.c
> 
> ath11k is also "PCI" and "MHI" but this isn't "common" code for it, so
> perhaps "mhi_pci_generic.c" or "mhi_pci_modem.c"?

It occurs to me from seeing Bjorn's comment, GregKH had a pretty hard 
objection to creating a directory for the mhi_uci driver (more or less 
its pointless to have a directory for a single file, wait until there 
are a mess of files that need to be managed), and this patch is doing 
the same thing.  I would suggest dropping the "controllers" directory, 
and just putting this in the root mhi directory.

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

Re: [PATCH] bus: mhi: Add MHI PCI support

[Loic Poulain]

Hi Bjorn,

On Wed, 23 Sep 2020 at 17:17, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Wed 23 Sep 08:40 CDT 2020, Loic Poulain wrote:
>
> > This is a generic MHI-over-PCI controller driver for MHI only devices
> > such as QCOM modems. For now it supports registering of Qualcomm SDX55
> > based PCIe modules. The MHI channels have been extracted from mhi
> > downstream driver.
> >
> > This driver is easily extendable to support other MHI PCI devices like
> > different modem hw or OEM superset.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > ---
> [..]
> > diff --git a/drivers/bus/mhi/controllers/mhi_pci_common.c b/drivers/bus/mhi/controllers/mhi_pci_common.c
> > new file mode 100644
> > index 0000000..705b283
> > --- /dev/null
> > +++ b/drivers/bus/mhi/controllers/mhi_pci_common.c
>
> ath11k is also "PCI" and "MHI" but this isn't "common" code for it, so
> perhaps "mhi_pci_generic.c" or "mhi_pci_modem.c"?

Right, mhi_pci_modem looks good.

>
> > @@ -0,0 +1,339 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * MHI PCI driver - MHI over PCI controller driver
> > + *
> > + * This module is a generic driver for registering pure MHI-over-PCI devices,
> > + * such as PCIe QCOM modems.
> > + *
> > + * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/mhi.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/delay.h>
>
> Can you confirm that you need delay.h?

You're right, don't use any delay function in that version.

>
> > +
> > +#define MHI_PCI_BAR_NUM 0
> > +
> > +enum {
> > +     EV_RING_0,
> > +     EV_RING_1,
> > +     EV_RING_2,
> > +     EV_RING_3,
>
> Maybe you can just use the numbers 0-3 instead?

ack.

>
> > +};
> > +
> [..]
> > +static int mhi_pci_claim(struct mhi_controller *mhic)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev);
> > +     int err;
> > +
> > +     err = pci_assign_resource(pdev, MHI_PCI_BAR_NUM);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to assign pci resource: %d\n", err);
>
> Afaict all code paths of pci_assign_resource() will log already.
>

ok, thanks.

> > +             return err;
> > +     }
> > +
> > +     err = pcim_enable_device(pdev);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to enable pci device: %d\n", err);
> > +             return err;
> > +     }
> > +
> > +     err = pcim_iomap_regions(pdev, 1 << MHI_PCI_BAR_NUM, pci_name(pdev));
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to map pci region: %d\n", err);
> > +             return err;
> > +     }
> > +     mhic->regs = pcim_iomap_table(pdev)[MHI_PCI_BAR_NUM];
> > +
> > +     err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> > +     if (err) {
> > +             dev_err(&pdev->dev, "Cannot set proper DMA mask\n");
> > +             return err;
> > +     }
> > +
> > +     err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
> > +     if (err) {
> > +             dev_err(&pdev->dev, "set consistent dma mask failed\n");
> > +             return err;
> > +     }
> > +
> > +     pci_set_master(pdev);
> > +
> > +     return 0;
> > +}
> [..]
> > +static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +     const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
> > +     const struct mhi_controller_config *mhic_config;
> > +     struct mhi_controller *mhic;
> > +     int err;
> > +
> > +     dev_info(&pdev->dev, "MHI PCI device found: %s\n", info->name);
> > +
> > +     mhic = devm_kzalloc(&pdev->dev, sizeof(*mhic), GFP_KERNEL);
> > +     if (!mhic) {
> > +             dev_err(&pdev->dev, "failed to allocate mhi controller\n");
>
> kzalloc() will log errors as well.
>
> > +             return -ENOMEM;
> > +     }
> > +
> > +     mhic_config = info->config;
> > +     mhic->cntrl_dev = &pdev->dev;
> > +     mhic->iova_start = 0;
> > +     mhic->iova_stop = 0xffffffff;
> > +     mhic->fw_image = NULL;
> > +     mhic->edl_image = NULL;
> > +
> > +     mhic->read_reg = mhi_pci_read_reg;
> > +     mhic->write_reg = mhi_pci_write_reg;
> > +     mhic->status_cb = mhi_pci_status_cb;
> > +     mhic->runtime_get = mhi_pci_runtime_get;
> > +     mhic->runtime_put = mhi_pci_runtime_put;
> > +
> > +     err = mhi_pci_claim(mhic);
> > +     if (err)
> > +             return err;
> > +
> > +     err = mhi_pci_get_irqs(mhic, mhic_config);
> > +     if (err)
> > +             return err;
> > +
> > +     pci_set_drvdata(pdev, mhic);
> > +
> > +     err = mhi_register_controller(mhic, mhic_config);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to register MHI controller\n");
>
> Afaict all errors that occurs if you pass valid data to this function
> will print an error message already...
>
> > +             pci_free_irq_vectors(pdev);
> > +             return err;
> > +     }
> > +
> > +     /* MHI bus does not power up the controller by default */
> > +     err = mhi_prepare_for_power_up(mhic);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to prepare MHI controller\n");
>
> mhi_unregister_controller()?

Indeed, thanks.

>
> > +             return err;
> > +     }
> > +
> > +     err = mhi_sync_power_up(mhic);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to power up MHI controller\n");
> > +             mhi_unprepare_after_power_down(mhic);
> > +             mhi_unregister_controller(mhic);
> > +             return err;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void mhi_pci_remove(struct pci_dev *pdev)
> > +{
> > +     struct mhi_controller *mhic = pci_get_drvdata(pdev);
> > +
> > +     mhi_power_down(mhic, true);
> > +     mhi_unprepare_after_power_down(mhic);
> > +     mhi_unregister_controller(mhic);
> > +}
> > +
> > +static struct pci_driver mhi_pci_driver = {
> > +     .name           = "mhi-pci",
> > +     .id_table       = mhi_pci_id_table,
> > +     .probe          = mhi_pci_probe,
> > +     .remove         = mhi_pci_remove
> > +};
> > +module_pci_driver(mhi_pci_driver);
> > +
> > +MODULE_AUTHOR("Loic Poulain <loic.poulain@linaro,org>");
> > +MODULE_DESCRIPTION("mhi-pci");
>
> Please expand this

will do.

>
> > +MODULE_LICENSE("GPL");
> > +MODULE_VERSION("1");
>
> And please drop the version, as this is likely not going to be updated
> consistently.
>
> Regards,
> Bjorn

Re: [PATCH] bus: mhi: Add MHI PCI support

[Manivannan Sadhasivam]

On Wed, Sep 23, 2020 at 06:31:09PM +0200, Loic Poulain wrote:
> Hi Bjorn,
> 
> On Wed, 23 Sep 2020 at 17:17, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 23 Sep 08:40 CDT 2020, Loic Poulain wrote:
> >
> > > This is a generic MHI-over-PCI controller driver for MHI only devices
> > > such as QCOM modems. For now it supports registering of Qualcomm SDX55
> > > based PCIe modules. The MHI channels have been extracted from mhi
> > > downstream driver.
> > >
> > > This driver is easily extendable to support other MHI PCI devices like
> > > different modem hw or OEM superset.
> > >
> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > ---
> > [..]
> > > diff --git a/drivers/bus/mhi/controllers/mhi_pci_common.c b/drivers/bus/mhi/controllers/mhi_pci_common.c
> > > new file mode 100644
> > > index 0000000..705b283
> > > --- /dev/null
> > > +++ b/drivers/bus/mhi/controllers/mhi_pci_common.c
> >
> > ath11k is also "PCI" and "MHI" but this isn't "common" code for it, so
> > perhaps "mhi_pci_generic.c" or "mhi_pci_modem.c"?
> 
> Right, mhi_pci_modem looks good.
> 

Please use "mhi_pci_generic". This driver is not specific to modems...

Thanks,
Mani

> >
> > > @@ -0,0 +1,339 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * MHI PCI driver - MHI over PCI controller driver
> > > + *
> > > + * This module is a generic driver for registering pure MHI-over-PCI devices,
> > > + * such as PCIe QCOM modems.
> > > + *
> > > + * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/mhi.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/delay.h>
> >
> > Can you confirm that you need delay.h?
> 
> You're right, don't use any delay function in that version.
> 
> >
> > > +
> > > +#define MHI_PCI_BAR_NUM 0
> > > +
> > > +enum {
> > > +     EV_RING_0,
> > > +     EV_RING_1,
> > > +     EV_RING_2,
> > > +     EV_RING_3,
> >
> > Maybe you can just use the numbers 0-3 instead?
> 
> ack.
> 
> >
> > > +};
> > > +
> > [..]
> > > +static int mhi_pci_claim(struct mhi_controller *mhic)
> > > +{
> > > +     struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev);
> > > +     int err;
> > > +
> > > +     err = pci_assign_resource(pdev, MHI_PCI_BAR_NUM);
> > > +     if (err) {
> > > +             dev_err(&pdev->dev, "failed to assign pci resource: %d\n", err);
> >
> > Afaict all code paths of pci_assign_resource() will log already.
> >
> 
> ok, thanks.
> 
> > > +             return err;
> > > +     }
> > > +
> > > +     err = pcim_enable_device(pdev);
> > > +     if (err) {
> > > +             dev_err(&pdev->dev, "failed to enable pci device: %d\n", err);
> > > +             return err;
> > > +     }
> > > +
> > > +     err = pcim_iomap_regions(pdev, 1 << MHI_PCI_BAR_NUM, pci_name(pdev));
> > > +     if (err) {
> > > +             dev_err(&pdev->dev, "failed to map pci region: %d\n", err);
> > > +             return err;
> > > +     }
> > > +     mhic->regs = pcim_iomap_table(pdev)[MHI_PCI_BAR_NUM];
> > > +
> > > +     err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> > > +     if (err) {
> > > +             dev_err(&pdev->dev, "Cannot set proper DMA mask\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
> > > +     if (err) {
> > > +             dev_err(&pdev->dev, "set consistent dma mask failed\n");
> > > +             return err;
> > > +     }
> > > +
> > > +     pci_set_master(pdev);
> > > +
> > > +     return 0;
> > > +}
> > [..]
> > > +static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > +{
> > > +     const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
> > > +     const struct mhi_controller_config *mhic_config;
> > > +     struct mhi_controller *mhic;
> > > +     int err;
> > > +
> > > +     dev_info(&pdev->dev, "MHI PCI device found: %s\n", info->name);
> > > +
> > > +     mhic = devm_kzalloc(&pdev->dev, sizeof(*mhic), GFP_KERNEL);
> > > +     if (!mhic) {
> > > +             dev_err(&pdev->dev, "failed to allocate mhi controller\n");
> >
> > kzalloc() will log errors as well.
> >
> > > +             return -ENOMEM;
> > > +     }
> > > +
> > > +     mhic_config = info->config;
> > > +     mhic->cntrl_dev = &pdev->dev;
> > > +     mhic->iova_start = 0;
> > > +     mhic->iova_stop = 0xffffffff;
> > > +     mhic->fw_image = NULL;
> > > +     mhic->edl_image = NULL;
> > > +
> > > +     mhic->read_reg = mhi_pci_read_reg;
> > > +     mhic->write_reg = mhi_pci_write_reg;
> > > +     mhic->status_cb = mhi_pci_status_cb;
> > > +     mhic->runtime_get = mhi_pci_runtime_get;
> > > +     mhic->runtime_put = mhi_pci_runtime_put;
> > > +
> > > +     err = mhi_pci_claim(mhic);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     err = mhi_pci_get_irqs(mhic, mhic_config);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     pci_set_drvdata(pdev, mhic);
> > > +
> > > +     err = mhi_register_controller(mhic, mhic_config);
> > > +     if (err) {
> > > +             dev_err(&pdev->dev, "failed to register MHI controller\n");
> >
> > Afaict all errors that occurs if you pass valid data to this function
> > will print an error message already...
> >
> > > +             pci_free_irq_vectors(pdev);
> > > +             return err;
> > > +     }
> > > +
> > > +     /* MHI bus does not power up the controller by default */
> > > +     err = mhi_prepare_for_power_up(mhic);
> > > +     if (err) {
> > > +             dev_err(&pdev->dev, "failed to prepare MHI controller\n");
> >
> > mhi_unregister_controller()?
> 
> Indeed, thanks.
> 
> >
> > > +             return err;
> > > +     }
> > > +
> > > +     err = mhi_sync_power_up(mhic);
> > > +     if (err) {
> > > +             dev_err(&pdev->dev, "failed to power up MHI controller\n");
> > > +             mhi_unprepare_after_power_down(mhic);
> > > +             mhi_unregister_controller(mhic);
> > > +             return err;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void mhi_pci_remove(struct pci_dev *pdev)
> > > +{
> > > +     struct mhi_controller *mhic = pci_get_drvdata(pdev);
> > > +
> > > +     mhi_power_down(mhic, true);
> > > +     mhi_unprepare_after_power_down(mhic);
> > > +     mhi_unregister_controller(mhic);
> > > +}
> > > +
> > > +static struct pci_driver mhi_pci_driver = {
> > > +     .name           = "mhi-pci",
> > > +     .id_table       = mhi_pci_id_table,
> > > +     .probe          = mhi_pci_probe,
> > > +     .remove         = mhi_pci_remove
> > > +};
> > > +module_pci_driver(mhi_pci_driver);
> > > +
> > > +MODULE_AUTHOR("Loic Poulain <loic.poulain@linaro,org>");
> > > +MODULE_DESCRIPTION("mhi-pci");
> >
> > Please expand this
> 
> will do.
> 
> >
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_VERSION("1");
> >
> > And please drop the version, as this is likely not going to be updated
> > consistently.
> >
> > Regards,
> > Bjorn