| Message ID | 1603108844-22286-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] bus: mhi: Add MHI PCI support for WWAN modems | expand |
On 2020-10-19 05:00, 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 for MHI-only devices which have all functionnalities > exposed through MHI channels and accessed by the corresponding MHI > device drivers (no out-of-band communication). > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > --- > v2: - remove useless delay.h include > - remove over-logging on error > - remove controller subdir > - rename to mhi_pci_modem.c > - Fix mhi_pci_probe exit path on error > - expand module description > - drop module version > v3: - Rename to mhi_pci_generic > - Add hardware accelerated IP channel (IPA) > - Added fw/edl names for sdx55m > v4: - Configurable dma width access > - Configurable PCI BAR number (default is 0) > > drivers/bus/mhi/Kconfig | 9 + > drivers/bus/mhi/Makefile | 3 + > drivers/bus/mhi/mhi_pci_generic.c | 336 > ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 348 insertions(+) > create mode 100644 drivers/bus/mhi/mhi_pci_generic.c > > diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig > index e841c10..daa8528 100644 > --- a/drivers/bus/mhi/Kconfig > +++ b/drivers/bus/mhi/Kconfig > @@ -20,3 +20,12 @@ 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_GENERIC > + tristate "MHI PCI controller driver" > + depends on MHI_BUS > + depends on PCI > + help > + This driver provides Modem Host Interface (MHI) PCI controller > driver > + for devices such as Qualcomm SDX55 based PCIe modems. > + > diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile > index 19e6443..d1a4ef3 100644 > --- a/drivers/bus/mhi/Makefile > +++ b/drivers/bus/mhi/Makefile > @@ -1,2 +1,5 @@ > # core layer > obj-y += core/ > + > +obj-$(CONFIG_MHI_BUS_PCI_GENERIC) := mhi_pci_generic.o > + > diff --git a/drivers/bus/mhi/mhi_pci_generic.c > b/drivers/bus/mhi/mhi_pci_generic.c > new file mode 100644 > index 0000000..dcd6c1a > --- /dev/null > +++ b/drivers/bus/mhi/mhi_pci_generic.c > @@ -0,0 +1,336 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * MHI PCI driver - MHI over PCI controller driver > + * > + * This module is a generic driver for registering 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> > + > +#define MHI_PCI_DEFAULT_BAR_NUM 0 > + > +struct mhi_pci_dev_info { > + const struct mhi_controller_config *config; > + const char *name; > + const char *fw; > + const char *edl; > + unsigned int bar_num; > + unsigned int dma_data_width; > +}; > + > +#define MHI_CHANNEL_CONFIG_UL(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, \ > + } \ > + > +#define MHI_CHANNEL_CONFIG_DL(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, \ > + } > + > +#define MHI_EVENT_CONFIG_CTRL(enum) \ > + { \ > + .num_elements = 64, \ > + .irq_moderation_ms = 0, \ > + .irq = (enum) + 1, \ > + .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_DATA(enum) \ > + { \ > + .num_elements = 128, \ > + .irq_moderation_ms = 5, \ > + .irq = (enum) + 1, \ > + .priority = 1, \ > + .mode = MHI_DB_BRST_DISABLE, \ > + .data_type = MHI_ER_DATA, \ > + .hardware_event = false, \ > + .client_managed = false, \ > + .offload_channel = false, \ > + } > + > +#define MHI_EVENT_CONFIG_HW_DATA(enum, cnum) \ > + { \ > + .num_elements = 128, \ > + .irq_moderation_ms = 5, \ > + .irq = (enum) + 1, \ > + .priority = 1, \ > + .mode = MHI_DB_BRST_DISABLE, \ > + .data_type = MHI_ER_DATA, \ > + .hardware_event = true, \ > + .client_managed = false, \ > + .offload_channel = false, \ > + .channel = cnum, \ > + } > + > +static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = > { > + MHI_CHANNEL_CONFIG_UL(12, "MBIM", 4, 0), > + MHI_CHANNEL_CONFIG_DL(13, "MBIM", 4, 0), > + MHI_CHANNEL_CONFIG_UL(14, "QMI", 4, 0), > + MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0), > + MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0), > + MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0), > + MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1), > + MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2), > +}; > + > +static const struct mhi_event_config modem_qcom_v1_mhi_events[] = { > + /* first ring is control+data ring */ > + MHI_EVENT_CONFIG_CTRL(0), > + /* Hardware channels request dedicated hardware event rings */ > + MHI_EVENT_CONFIG_HW_DATA(1, 100), > + MHI_EVENT_CONFIG_HW_DATA(2, 101) > +}; > + > +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-sdx55m", > + .fw = "qcom/sdx55m/sbl1.mbn", > + .edl = "qcom/sdx55m/edl.mbn", > + .config = &modem_qcom_v1_mhi_config, > + .bar_num = MHI_PCI_DEFAULT_BAR_NUM, > + .dma_data_width = 32 > +}; > + > +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, unsigned int > bar_num, > + u64 dma_mask) > +{ > + struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev); > + int err; > + > + err = pci_assign_resource(pdev, bar_num); > + if (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 << 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)[bar_num]; > + > + err = pci_set_dma_mask(pdev, dma_mask); > + if (err) { > + dev_err(&pdev->dev, "Cannot set proper DMA mask\n"); > + return err; > + } > + > + err = pci_set_consistent_dma_mask(pdev, dma_mask); > + 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 nr_vectors, i; > + int *irq; > + > + /* > + * Alloc one MSI vector for BHI + one vector per event ring, > ideally... > + * No explicit pci_free_irq_vectors required, done by pcim_release. > + */ > + mhic->nr_irqs = 1 + mhic_config->num_events; > + > + nr_vectors = pci_alloc_irq_vectors(pdev, 1, mhic->nr_irqs, > PCI_IRQ_MSI); > + if (nr_vectors < 0) { > + dev_err(&pdev->dev, "Error allocating MSI vectors %d\n", > + nr_vectors); > + return nr_vectors; > + } > + > + if (nr_vectors < mhic->nr_irqs) { > + dev_warn(&pdev->dev, "Not enough MSI vectors (%d/%d)\n", > + nr_vectors, mhic_config->num_events); > + /* continue... use shared IRQ */ > + } > + > + irq = devm_kcalloc(&pdev->dev, mhic->nr_irqs, sizeof(int), > GFP_KERNEL); > + if (!irq) > + return -ENOMEM; > + > + for (i = 0; i < mhic->nr_irqs; i++) { > + int vector = i >= nr_vectors ? (nr_vectors - 1) : i; > + > + irq[i] = pci_irq_vector(pdev, vector); > + } > + > + mhic->irq = irq; > + > + 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) > + return -ENOMEM; > + > + mhic_config = info->config; > + mhic->cntrl_dev = &pdev->dev; > + mhic->iova_start = 0; > + mhic->iova_stop = DMA_BIT_MASK(info->dma_data_width); > + mhic->fw_image = info->fw; > + mhic->edl_image = info->edl; > + > + 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, info->bar_num, > DMA_BIT_MASK(info->dma_data_width)); > + 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) > + 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"); > + goto err_unregister; > + } > + > + err = mhi_sync_power_up(mhic); > + if (err) { > + dev_err(&pdev->dev, "failed to power up MHI controller\n"); > + goto err_unprepare; > + } > + > + return 0; > + > +err_unprepare: > + mhi_unprepare_after_power_down(mhic); > + > +err_unregister: > + mhi_unregister_controller(mhic); > + > + return err; > +} > + > +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-generic", > + .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("Modem Host Interface (MHI) PCI controller > driver"); > +MODULE_LICENSE("GPL"); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On Mon, Oct 19, 2020 at 02:00:44PM +0200, 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 for MHI-only devices which have all functionnalities s/functionnalities/functionalities > exposed through MHI channels and accessed by the corresponding MHI > device drivers (no out-of-band communication). > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > v2: - remove useless delay.h include > - remove over-logging on error > - remove controller subdir > - rename to mhi_pci_modem.c > - Fix mhi_pci_probe exit path on error > - expand module description > - drop module version > v3: - Rename to mhi_pci_generic > - Add hardware accelerated IP channel (IPA) > - Added fw/edl names for sdx55m > v4: - Configurable dma width access > - Configurable PCI BAR number (default is 0) > > drivers/bus/mhi/Kconfig | 9 + > drivers/bus/mhi/Makefile | 3 + > drivers/bus/mhi/mhi_pci_generic.c | 336 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 348 insertions(+) > create mode 100644 drivers/bus/mhi/mhi_pci_generic.c > > diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig > index e841c10..daa8528 100644 > --- a/drivers/bus/mhi/Kconfig > +++ b/drivers/bus/mhi/Kconfig > @@ -20,3 +20,12 @@ 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_GENERIC > + tristate "MHI PCI controller driver" > + depends on MHI_BUS > + depends on PCI > + help > + This driver provides Modem Host Interface (MHI) PCI controller driver No need to expand MHI here. It is already done in CONFIG_MHI_BUS. > + for devices such as Qualcomm SDX55 based PCIe modems. > + > diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile > index 19e6443..d1a4ef3 100644 > --- a/drivers/bus/mhi/Makefile > +++ b/drivers/bus/mhi/Makefile > @@ -1,2 +1,5 @@ > # core layer > obj-y += core/ > + > +obj-$(CONFIG_MHI_BUS_PCI_GENERIC) := mhi_pci_generic.o The driver is already under bus/mhi so no need of mhi_ prefix here. > + > diff --git a/drivers/bus/mhi/mhi_pci_generic.c b/drivers/bus/mhi/mhi_pci_generic.c > new file mode 100644 > index 0000000..dcd6c1a > --- /dev/null > +++ b/drivers/bus/mhi/mhi_pci_generic.c > @@ -0,0 +1,336 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * MHI PCI driver - MHI over PCI controller driver > + * > + * This module is a generic driver for registering MHI-over-PCI devices, > + * such as PCIe QCOM modems. > + * > + * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org> Is this driver completely written from scratch? If not then you need to provide the copyright in downstream. > + */ > + > +#include <linux/device.h> > +#include <linux/mhi.h> > +#include <linux/module.h> > +#include <linux/pci.h> > + > +#define MHI_PCI_DEFAULT_BAR_NUM 0 > + > +struct mhi_pci_dev_info { > + const struct mhi_controller_config *config; > + const char *name; > + const char *fw; > + const char *edl; > + unsigned int bar_num; > + unsigned int dma_data_width; > +}; Can you please add Kdoc for this structure? > + > +#define MHI_CHANNEL_CONFIG_UL(cnum, cname, elems, event) \ ch_num, ch_name sounds more apt here... > + { \ > + .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, \ > + } \ > + > +#define MHI_CHANNEL_CONFIG_DL(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, \ > + } > + > +#define MHI_EVENT_CONFIG_CTRL(enum) \ Can you use a better name instead of enum? > + { \ > + .num_elements = 64, \ > + .irq_moderation_ms = 0, \ > + .irq = (enum) + 1, \ > + .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_DATA(enum) \ > + { \ > + .num_elements = 128, \ > + .irq_moderation_ms = 5, \ > + .irq = (enum) + 1, \ > + .priority = 1, \ > + .mode = MHI_DB_BRST_DISABLE, \ > + .data_type = MHI_ER_DATA, \ > + .hardware_event = false, \ > + .client_managed = false, \ > + .offload_channel = false, \ > + } > + > +#define MHI_EVENT_CONFIG_HW_DATA(enum, cnum) \ > + { \ > + .num_elements = 128, \ > + .irq_moderation_ms = 5, \ > + .irq = (enum) + 1, \ > + .priority = 1, \ > + .mode = MHI_DB_BRST_DISABLE, \ > + .data_type = MHI_ER_DATA, \ > + .hardware_event = true, \ > + .client_managed = false, \ > + .offload_channel = false, \ > + .channel = cnum, \ > + } > + > +static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = { > + MHI_CHANNEL_CONFIG_UL(12, "MBIM", 4, 0), > + MHI_CHANNEL_CONFIG_DL(13, "MBIM", 4, 0), > + MHI_CHANNEL_CONFIG_UL(14, "QMI", 4, 0), > + MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0), > + MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0), > + MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0), > + MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1), > + MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2), > +}; > + > +static const struct mhi_event_config modem_qcom_v1_mhi_events[] = { > + /* first ring is control+data ring */ > + MHI_EVENT_CONFIG_CTRL(0), > + /* Hardware channels request dedicated hardware event rings */ > + MHI_EVENT_CONFIG_HW_DATA(1, 100), > + MHI_EVENT_CONFIG_HW_DATA(2, 101) > +}; > + > +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-sdx55m", > + .fw = "qcom/sdx55m/sbl1.mbn", > + .edl = "qcom/sdx55m/edl.mbn", > + .config = &modem_qcom_v1_mhi_config, > + .bar_num = MHI_PCI_DEFAULT_BAR_NUM, > + .dma_data_width = 32 > +}; > + > +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) Nit: Align the wrapped parameter with the start of '(' > +{ > + *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, unsigned int bar_num, s/mhic/mhi_cntrl - Just to be consistent with the core MHI bus. > + u64 dma_mask) > +{ > + struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev); > + int err; > + > + err = pci_assign_resource(pdev, bar_num); > + if (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 << 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)[bar_num]; > + > + err = pci_set_dma_mask(pdev, dma_mask); > + if (err) { > + dev_err(&pdev->dev, "Cannot set proper DMA mask\n"); > + return err; > + } > + > + err = pci_set_consistent_dma_mask(pdev, dma_mask); > + 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 nr_vectors, i; > + int *irq; > + > + /* > + * Alloc one MSI vector for BHI + one vector per event ring, ideally... > + * No explicit pci_free_irq_vectors required, done by pcim_release. > + */ > + mhic->nr_irqs = 1 + mhic_config->num_events; > + > + nr_vectors = pci_alloc_irq_vectors(pdev, 1, mhic->nr_irqs, PCI_IRQ_MSI); > + if (nr_vectors < 0) { > + dev_err(&pdev->dev, "Error allocating MSI vectors %d\n", > + nr_vectors); > + return nr_vectors; > + } > + > + if (nr_vectors < mhic->nr_irqs) { > + dev_warn(&pdev->dev, "Not enough MSI vectors (%d/%d)\n", > + nr_vectors, mhic_config->num_events); > + /* continue... use shared IRQ */ Perhaps add this info to the warning as well? > + } > + > + irq = devm_kcalloc(&pdev->dev, mhic->nr_irqs, sizeof(int), GFP_KERNEL); So you're allocating 'mhic->nr_irqs' even for (nr_vectors < mhic->nr_irqs) case? > + if (!irq) > + return -ENOMEM; > + > + for (i = 0; i < mhic->nr_irqs; i++) { > + int vector = i >= nr_vectors ? (nr_vectors - 1) : i; > + > + irq[i] = pci_irq_vector(pdev, vector); > + } > + > + mhic->irq = irq; > + > + 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; s/mhic_config/mhi_config > + struct mhi_controller *mhic; > + int err; > + > + dev_info(&pdev->dev, "MHI PCI device found: %s\n", info->name); Make it dev_dbg. > + > + mhic = devm_kzalloc(&pdev->dev, sizeof(*mhic), GFP_KERNEL); You should use mhi_alloc_controller() API for allocating the controller struct. but then you need to free it in error paths as well... Thanks, Mani
Hi Mani, On Wed, 21 Oct 2020 at 17:20, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Mon, Oct 19, 2020 at 02:00:44PM +0200, 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 for MHI-only devices which have all functionnalities > > s/functionnalities/functionalities > > > exposed through MHI channels and accessed by the corresponding MHI > > device drivers (no out-of-band communication). > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > --- > > v2: - remove useless delay.h include > > - remove over-logging on error > > - remove controller subdir > > - rename to mhi_pci_modem.c > > - Fix mhi_pci_probe exit path on error > > - expand module description > > - drop module version > > v3: - Rename to mhi_pci_generic > > - Add hardware accelerated IP channel (IPA) > > - Added fw/edl names for sdx55m > > v4: - Configurable dma width access > > - Configurable PCI BAR number (default is 0) > > > > drivers/bus/mhi/Kconfig | 9 + > > drivers/bus/mhi/Makefile | 3 + > > drivers/bus/mhi/mhi_pci_generic.c | 336 ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 348 insertions(+) > > create mode 100644 drivers/bus/mhi/mhi_pci_generic.c > > > > diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig > > index e841c10..daa8528 100644 > > --- a/drivers/bus/mhi/Kconfig > > +++ b/drivers/bus/mhi/Kconfig > > @@ -20,3 +20,12 @@ 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_GENERIC > > + tristate "MHI PCI controller driver" > > + depends on MHI_BUS > > + depends on PCI > > + help > > + This driver provides Modem Host Interface (MHI) PCI controller driver > > No need to expand MHI here. It is already done in CONFIG_MHI_BUS. > > > + for devices such as Qualcomm SDX55 based PCIe modems. > > + > > diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile > > index 19e6443..d1a4ef3 100644 > > --- a/drivers/bus/mhi/Makefile > > +++ b/drivers/bus/mhi/Makefile > > @@ -1,2 +1,5 @@ > > # core layer > > obj-y += core/ > > + > > +obj-$(CONFIG_MHI_BUS_PCI_GENERIC) := mhi_pci_generic.o > > The driver is already under bus/mhi so no need of mhi_ prefix here. > > > + > > diff --git a/drivers/bus/mhi/mhi_pci_generic.c b/drivers/bus/mhi/mhi_pci_generic.c > > new file mode 100644 > > index 0000000..dcd6c1a > > --- /dev/null > > +++ b/drivers/bus/mhi/mhi_pci_generic.c > > @@ -0,0 +1,336 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * MHI PCI driver - MHI over PCI controller driver > > + * > > + * This module is a generic driver for registering MHI-over-PCI devices, > > + * such as PCIe QCOM modems. > > + * > > + * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org> > > Is this driver completely written from scratch? If not then you need to provide > the copyright in downstream. Yes, the code itself has been written from scratch, though some information, like channel IDs come from downstream parts. > > > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/mhi.h> > > +#include <linux/module.h> > > +#include <linux/pci.h> > > + > > +#define MHI_PCI_DEFAULT_BAR_NUM 0 > > + > > +struct mhi_pci_dev_info { > > + const struct mhi_controller_config *config; > > + const char *name; > > + const char *fw; > > + const char *edl; > > + unsigned int bar_num; > > + unsigned int dma_data_width; > > +}; > > Can you please add Kdoc for this structure? sure > > > + > > +#define MHI_CHANNEL_CONFIG_UL(cnum, cname, elems, event) \ > > ch_num, ch_name sounds more apt here... > > > + { \ > > + .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, \ > > + } \ > > + > > +#define MHI_CHANNEL_CONFIG_DL(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, \ > > + } > > + > > +#define MHI_EVENT_CONFIG_CTRL(enum) \ > > Can you use a better name instead of enum? certainly > > > + { \ > > + .num_elements = 64, \ > > + .irq_moderation_ms = 0, \ > > + .irq = (enum) + 1, \ > > + .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_DATA(enum) \ > > + { \ > > + .num_elements = 128, \ > > + .irq_moderation_ms = 5, \ > > + .irq = (enum) + 1, \ > > + .priority = 1, \ > > + .mode = MHI_DB_BRST_DISABLE, \ > > + .data_type = MHI_ER_DATA, \ > > + .hardware_event = false, \ > > + .client_managed = false, \ > > + .offload_channel = false, \ > > + } > > + > > +#define MHI_EVENT_CONFIG_HW_DATA(enum, cnum) \ > > + { \ > > + .num_elements = 128, \ > > + .irq_moderation_ms = 5, \ > > + .irq = (enum) + 1, \ > > + .priority = 1, \ > > + .mode = MHI_DB_BRST_DISABLE, \ > > + .data_type = MHI_ER_DATA, \ > > + .hardware_event = true, \ > > + .client_managed = false, \ > > + .offload_channel = false, \ > > + .channel = cnum, \ > > + } > > + > > +static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = { > > + MHI_CHANNEL_CONFIG_UL(12, "MBIM", 4, 0), > > + MHI_CHANNEL_CONFIG_DL(13, "MBIM", 4, 0), > > + MHI_CHANNEL_CONFIG_UL(14, "QMI", 4, 0), > > + MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0), > > + MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0), > > + MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0), > > + MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1), > > + MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2), > > +}; > > + > > +static const struct mhi_event_config modem_qcom_v1_mhi_events[] = { > > + /* first ring is control+data ring */ > > + MHI_EVENT_CONFIG_CTRL(0), > > + /* Hardware channels request dedicated hardware event rings */ > > + MHI_EVENT_CONFIG_HW_DATA(1, 100), > > + MHI_EVENT_CONFIG_HW_DATA(2, 101) > > +}; > > + > > +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-sdx55m", > > + .fw = "qcom/sdx55m/sbl1.mbn", > > + .edl = "qcom/sdx55m/edl.mbn", > > + .config = &modem_qcom_v1_mhi_config, > > + .bar_num = MHI_PCI_DEFAULT_BAR_NUM, > > + .dma_data_width = 32 > > +}; > > + > > +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) > > Nit: Align the wrapped parameter with the start of '(' > > > +{ > > + *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, unsigned int bar_num, > > s/mhic/mhi_cntrl - Just to be consistent with the core MHI bus. > > > + u64 dma_mask) > > +{ > > + struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev); > > + int err; > > + > > + err = pci_assign_resource(pdev, bar_num); > > + if (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 << 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)[bar_num]; > > + > > + err = pci_set_dma_mask(pdev, dma_mask); > > + if (err) { > > + dev_err(&pdev->dev, "Cannot set proper DMA mask\n"); > > + return err; > > + } > > + > > + err = pci_set_consistent_dma_mask(pdev, dma_mask); > > + 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 nr_vectors, i; > > + int *irq; > > + > > + /* > > + * Alloc one MSI vector for BHI + one vector per event ring, ideally... > > + * No explicit pci_free_irq_vectors required, done by pcim_release. > > + */ > > + mhic->nr_irqs = 1 + mhic_config->num_events; > > + > > + nr_vectors = pci_alloc_irq_vectors(pdev, 1, mhic->nr_irqs, PCI_IRQ_MSI); > > + if (nr_vectors < 0) { > > + dev_err(&pdev->dev, "Error allocating MSI vectors %d\n", > > + nr_vectors); > > + return nr_vectors; > > + } > > + > > + if (nr_vectors < mhic->nr_irqs) { > > + dev_warn(&pdev->dev, "Not enough MSI vectors (%d/%d)\n", > > + nr_vectors, mhic_config->num_events); > > + /* continue... use shared IRQ */ > > Perhaps add this info to the warning as well? > > > + } > > + > > + irq = devm_kcalloc(&pdev->dev, mhic->nr_irqs, sizeof(int), GFP_KERNEL); > > So you're allocating 'mhic->nr_irqs' even for (nr_vectors < mhic->nr_irqs) case? Yes, simply because I want to keep mhi_event_config which contains IRQ IDs is constant (and would like to keep it constant). That allows a simple matching between ring ID and interrupt ID. > > > + if (!irq) > > + return -ENOMEM; > > + > > + for (i = 0; i < mhic->nr_irqs; i++) { > > + int vector = i >= nr_vectors ? (nr_vectors - 1) : i; > > + > > + irq[i] = pci_irq_vector(pdev, vector); > > + } > > + > > + mhic->irq = irq; > > + > > + 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; > > s/mhic_config/mhi_config > > > + struct mhi_controller *mhic; > > + int err; > > + > > + dev_info(&pdev->dev, "MHI PCI device found: %s\n", info->name); > > Make it dev_dbg. > > > + > > + mhic = devm_kzalloc(&pdev->dev, sizeof(*mhic), GFP_KERNEL); > > You should use mhi_alloc_controller() API for allocating the controller struct. > but then you need to free it in error paths as well... TBH, not sure what is the point of using mhi_alloc_controller, since kzalloc allows zeroed allocation as well, and devm is so useful. Regards, Loic
Hi Loic, On Wed, Oct 21, 2020 at 06:06:25PM +0200, Loic Poulain wrote: > Hi Mani, > > On Wed, 21 Oct 2020 at 17:20, Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > > > On Mon, Oct 19, 2020 at 02:00:44PM +0200, 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 for MHI-only devices which have all functionnalities > > > > s/functionnalities/functionalities > > > > > exposed through MHI channels and accessed by the corresponding MHI > > > device drivers (no out-of-band communication). > > > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > > --- > > > v2: - remove useless delay.h include > > > - remove over-logging on error > > > - remove controller subdir > > > - rename to mhi_pci_modem.c > > > - Fix mhi_pci_probe exit path on error > > > - expand module description > > > - drop module version > > > v3: - Rename to mhi_pci_generic > > > - Add hardware accelerated IP channel (IPA) > > > - Added fw/edl names for sdx55m > > > v4: - Configurable dma width access > > > - Configurable PCI BAR number (default is 0) > > > > > > drivers/bus/mhi/Kconfig | 9 + > > > drivers/bus/mhi/Makefile | 3 + > > > drivers/bus/mhi/mhi_pci_generic.c | 336 ++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 348 insertions(+) > > > create mode 100644 drivers/bus/mhi/mhi_pci_generic.c > > > > > > diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig > > > index e841c10..daa8528 100644 > > > --- a/drivers/bus/mhi/Kconfig > > > +++ b/drivers/bus/mhi/Kconfig > > > @@ -20,3 +20,12 @@ 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_GENERIC > > > + tristate "MHI PCI controller driver" > > > + depends on MHI_BUS > > > + depends on PCI > > > + help > > > + This driver provides Modem Host Interface (MHI) PCI controller driver > > > > No need to expand MHI here. It is already done in CONFIG_MHI_BUS. > > > > > + for devices such as Qualcomm SDX55 based PCIe modems. > > > + > > > diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile > > > index 19e6443..d1a4ef3 100644 > > > --- a/drivers/bus/mhi/Makefile > > > +++ b/drivers/bus/mhi/Makefile > > > @@ -1,2 +1,5 @@ > > > # core layer > > > obj-y += core/ > > > + > > > +obj-$(CONFIG_MHI_BUS_PCI_GENERIC) := mhi_pci_generic.o > > > > The driver is already under bus/mhi so no need of mhi_ prefix here. > > > > > + > > > diff --git a/drivers/bus/mhi/mhi_pci_generic.c b/drivers/bus/mhi/mhi_pci_generic.c > > > new file mode 100644 > > > index 0000000..dcd6c1a > > > --- /dev/null > > > +++ b/drivers/bus/mhi/mhi_pci_generic.c > > > @@ -0,0 +1,336 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > +/* > > > + * MHI PCI driver - MHI over PCI controller driver > > > + * > > > + * This module is a generic driver for registering MHI-over-PCI devices, > > > + * such as PCIe QCOM modems. > > > + * > > > + * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org> > > > > Is this driver completely written from scratch? If not then you need to provide > > the copyright in downstream. > > Yes, the code itself has been written from scratch, though some > information, like channel IDs come from downstream parts. > Okay, then it is fine. > > > > > > > + */ [...] > > > + > > > +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 nr_vectors, i; > > > + int *irq; > > > + > > > + /* > > > + * Alloc one MSI vector for BHI + one vector per event ring, ideally... > > > + * No explicit pci_free_irq_vectors required, done by pcim_release. > > > + */ > > > + mhic->nr_irqs = 1 + mhic_config->num_events; > > > + > > > + nr_vectors = pci_alloc_irq_vectors(pdev, 1, mhic->nr_irqs, PCI_IRQ_MSI); > > > + if (nr_vectors < 0) { > > > + dev_err(&pdev->dev, "Error allocating MSI vectors %d\n", > > > + nr_vectors); > > > + return nr_vectors; > > > + } > > > + > > > + if (nr_vectors < mhic->nr_irqs) { > > > + dev_warn(&pdev->dev, "Not enough MSI vectors (%d/%d)\n", > > > + nr_vectors, mhic_config->num_events); > > > + /* continue... use shared IRQ */ > > > > Perhaps add this info to the warning as well? > > > > > + } > > > + > > > + irq = devm_kcalloc(&pdev->dev, mhic->nr_irqs, sizeof(int), GFP_KERNEL); > > > > So you're allocating 'mhic->nr_irqs' even for (nr_vectors < mhic->nr_irqs) case? > > Yes, simply because I want to keep mhi_event_config which contains IRQ > IDs is constant (and would like to keep it constant). > That allows a simple matching between ring ID and interrupt ID. > Okay. > > > > > + if (!irq) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < mhic->nr_irqs; i++) { > > > + int vector = i >= nr_vectors ? (nr_vectors - 1) : i; > > > + > > > + irq[i] = pci_irq_vector(pdev, vector); > > > + } > > > + > > > + mhic->irq = irq; > > > + > > > + 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; > > > > s/mhic_config/mhi_config > > > > > + struct mhi_controller *mhic; > > > + int err; > > > + > > > + dev_info(&pdev->dev, "MHI PCI device found: %s\n", info->name); > > > > Make it dev_dbg. > > > > > + > > > + mhic = devm_kzalloc(&pdev->dev, sizeof(*mhic), GFP_KERNEL); > > > > You should use mhi_alloc_controller() API for allocating the controller struct. > > but then you need to free it in error paths as well... > > TBH, not sure what is the point of using mhi_alloc_controller, since > kzalloc allows > zeroed allocation as well, and devm is so useful. > I agree but the intention is not to depend on the controller drivers to zero out the structure. The stack makes the assumption that the structure is zeroed out, so anything not doing so will create serious issues. Thanks, Mani > Regards, > Loic
diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig index e841c10..daa8528 100644 --- a/drivers/bus/mhi/Kconfig +++ b/drivers/bus/mhi/Kconfig @@ -20,3 +20,12 @@ 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_GENERIC + tristate "MHI PCI controller driver" + depends on MHI_BUS + depends on PCI + help + This driver provides Modem Host Interface (MHI) PCI controller driver + for devices such as Qualcomm SDX55 based PCIe modems. + diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile index 19e6443..d1a4ef3 100644 --- a/drivers/bus/mhi/Makefile +++ b/drivers/bus/mhi/Makefile @@ -1,2 +1,5 @@ # core layer obj-y += core/ + +obj-$(CONFIG_MHI_BUS_PCI_GENERIC) := mhi_pci_generic.o + diff --git a/drivers/bus/mhi/mhi_pci_generic.c b/drivers/bus/mhi/mhi_pci_generic.c new file mode 100644 index 0000000..dcd6c1a --- /dev/null +++ b/drivers/bus/mhi/mhi_pci_generic.c @@ -0,0 +1,336 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * MHI PCI driver - MHI over PCI controller driver + * + * This module is a generic driver for registering 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> + +#define MHI_PCI_DEFAULT_BAR_NUM 0 + +struct mhi_pci_dev_info { + const struct mhi_controller_config *config; + const char *name; + const char *fw; + const char *edl; + unsigned int bar_num; + unsigned int dma_data_width; +}; + +#define MHI_CHANNEL_CONFIG_UL(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, \ + } \ + +#define MHI_CHANNEL_CONFIG_DL(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, \ + } + +#define MHI_EVENT_CONFIG_CTRL(enum) \ + { \ + .num_elements = 64, \ + .irq_moderation_ms = 0, \ + .irq = (enum) + 1, \ + .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_DATA(enum) \ + { \ + .num_elements = 128, \ + .irq_moderation_ms = 5, \ + .irq = (enum) + 1, \ + .priority = 1, \ + .mode = MHI_DB_BRST_DISABLE, \ + .data_type = MHI_ER_DATA, \ + .hardware_event = false, \ + .client_managed = false, \ + .offload_channel = false, \ + } + +#define MHI_EVENT_CONFIG_HW_DATA(enum, cnum) \ + { \ + .num_elements = 128, \ + .irq_moderation_ms = 5, \ + .irq = (enum) + 1, \ + .priority = 1, \ + .mode = MHI_DB_BRST_DISABLE, \ + .data_type = MHI_ER_DATA, \ + .hardware_event = true, \ + .client_managed = false, \ + .offload_channel = false, \ + .channel = cnum, \ + } + +static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = { + MHI_CHANNEL_CONFIG_UL(12, "MBIM", 4, 0), + MHI_CHANNEL_CONFIG_DL(13, "MBIM", 4, 0), + MHI_CHANNEL_CONFIG_UL(14, "QMI", 4, 0), + MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0), + MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0), + MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0), + MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1), + MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2), +}; + +static const struct mhi_event_config modem_qcom_v1_mhi_events[] = { + /* first ring is control+data ring */ + MHI_EVENT_CONFIG_CTRL(0), + /* Hardware channels request dedicated hardware event rings */ + MHI_EVENT_CONFIG_HW_DATA(1, 100), + MHI_EVENT_CONFIG_HW_DATA(2, 101) +}; + +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-sdx55m", + .fw = "qcom/sdx55m/sbl1.mbn", + .edl = "qcom/sdx55m/edl.mbn", + .config = &modem_qcom_v1_mhi_config, + .bar_num = MHI_PCI_DEFAULT_BAR_NUM, + .dma_data_width = 32 +}; + +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, unsigned int bar_num, + u64 dma_mask) +{ + struct pci_dev *pdev = to_pci_dev(mhic->cntrl_dev); + int err; + + err = pci_assign_resource(pdev, bar_num); + if (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 << 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)[bar_num]; + + err = pci_set_dma_mask(pdev, dma_mask); + if (err) { + dev_err(&pdev->dev, "Cannot set proper DMA mask\n"); + return err; + } + + err = pci_set_consistent_dma_mask(pdev, dma_mask); + 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 nr_vectors, i; + int *irq; + + /* + * Alloc one MSI vector for BHI + one vector per event ring, ideally... + * No explicit pci_free_irq_vectors required, done by pcim_release. + */ + mhic->nr_irqs = 1 + mhic_config->num_events; + + nr_vectors = pci_alloc_irq_vectors(pdev, 1, mhic->nr_irqs, PCI_IRQ_MSI); + if (nr_vectors < 0) { + dev_err(&pdev->dev, "Error allocating MSI vectors %d\n", + nr_vectors); + return nr_vectors; + } + + if (nr_vectors < mhic->nr_irqs) { + dev_warn(&pdev->dev, "Not enough MSI vectors (%d/%d)\n", + nr_vectors, mhic_config->num_events); + /* continue... use shared IRQ */ + } + + irq = devm_kcalloc(&pdev->dev, mhic->nr_irqs, sizeof(int), GFP_KERNEL); + if (!irq) + return -ENOMEM; + + for (i = 0; i < mhic->nr_irqs; i++) { + int vector = i >= nr_vectors ? (nr_vectors - 1) : i; + + irq[i] = pci_irq_vector(pdev, vector); + } + + mhic->irq = irq; + + 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) + return -ENOMEM; + + mhic_config = info->config; + mhic->cntrl_dev = &pdev->dev; + mhic->iova_start = 0; + mhic->iova_stop = DMA_BIT_MASK(info->dma_data_width); + mhic->fw_image = info->fw; + mhic->edl_image = info->edl; + + 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, info->bar_num, DMA_BIT_MASK(info->dma_data_width)); + 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) + 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"); + goto err_unregister; + } + + err = mhi_sync_power_up(mhic); + if (err) { + dev_err(&pdev->dev, "failed to power up MHI controller\n"); + goto err_unprepare; + } + + return 0; + +err_unprepare: + mhi_unprepare_after_power_down(mhic); + +err_unregister: + mhi_unregister_controller(mhic); + + return err; +} + +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-generic", + .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("Modem Host Interface (MHI) PCI controller driver"); +MODULE_LICENSE("GPL");
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 for MHI-only devices which have all functionnalities exposed through MHI channels and accessed by the corresponding MHI device drivers (no out-of-band communication). Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- v2: - remove useless delay.h include - remove over-logging on error - remove controller subdir - rename to mhi_pci_modem.c - Fix mhi_pci_probe exit path on error - expand module description - drop module version v3: - Rename to mhi_pci_generic - Add hardware accelerated IP channel (IPA) - Added fw/edl names for sdx55m v4: - Configurable dma width access - Configurable PCI BAR number (default is 0) drivers/bus/mhi/Kconfig | 9 + drivers/bus/mhi/Makefile | 3 + drivers/bus/mhi/mhi_pci_generic.c | 336 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 348 insertions(+) create mode 100644 drivers/bus/mhi/mhi_pci_generic.c