Re: [PATCH v4 7/7] PCI: endpoint: Add PCI Endpoint function driver for MHI bus
From: Manivannan Sadhasivam
Date: Mon May 29 2023 - 13:38:35 EST
On Mon, May 29, 2023 at 03:11:17PM +0200, Lorenzo Pieralisi wrote:
> On Fri, May 19, 2023 at 08:12:15PM +0530, Manivannan Sadhasivam wrote:
> > Add PCI Endpoint driver for the Qualcomm MHI (Modem Host Interface) bus.
> > The driver implements the MHI function over PCI in the endpoint device
> > such as SDX55 modem. The MHI endpoint function driver acts as a
> > controller driver for the MHI Endpoint stack and carries out all PCI
> > related activities like mapping the host memory using iATU, triggering
> > MSIs etc...
> >
> > Reviewed-by: Kishon Vijay Abraham I <kishon@xxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> > drivers/pci/endpoint/functions/Kconfig | 10 +
> > drivers/pci/endpoint/functions/Makefile | 1 +
> > drivers/pci/endpoint/functions/pci-epf-mhi.c | 454 +++++++++++++++++++
> > 3 files changed, 465 insertions(+)
> > create mode 100644 drivers/pci/endpoint/functions/pci-epf-mhi.c
> >
> > diff --git a/drivers/pci/endpoint/functions/Kconfig b/drivers/pci/endpoint/functions/Kconfig
> > index 9fd560886871..f5171b4fabbe 100644
> > --- a/drivers/pci/endpoint/functions/Kconfig
> > +++ b/drivers/pci/endpoint/functions/Kconfig
> > @@ -37,3 +37,13 @@ config PCI_EPF_VNTB
> > between PCI Root Port and PCIe Endpoint.
> >
> > If in doubt, say "N" to disable Endpoint NTB driver.
> > +
> > +config PCI_EPF_MHI
> > + tristate "PCI Endpoint driver for MHI bus"
> > + depends on PCI_ENDPOINT && MHI_BUS_EP
> > + help
> > + Enable this configuration option to enable the PCI Endpoint
> > + driver for Modem Host Interface (MHI) bus in Qualcomm Endpoint
> > + devices such as SDX55.
> > +
> > + If in doubt, say "N" to disable Endpoint driver for MHI bus.
> > diff --git a/drivers/pci/endpoint/functions/Makefile b/drivers/pci/endpoint/functions/Makefile
> > index 5c13001deaba..696473fce50e 100644
> > --- a/drivers/pci/endpoint/functions/Makefile
> > +++ b/drivers/pci/endpoint/functions/Makefile
> > @@ -6,3 +6,4 @@
> > obj-$(CONFIG_PCI_EPF_TEST) += pci-epf-test.o
> > obj-$(CONFIG_PCI_EPF_NTB) += pci-epf-ntb.o
> > obj-$(CONFIG_PCI_EPF_VNTB) += pci-epf-vntb.o
> > +obj-$(CONFIG_PCI_EPF_MHI) += pci-epf-mhi.o
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > new file mode 100644
> > index 000000000000..df924fb10e4d
> > --- /dev/null
> > +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
[...]
> > +#define MHI_EP_CHANNEL_CONFIG_UL(ch_num, ch_name) \
> > + { \
> > + .num = ch_num, \
> > + .name = ch_name, \
> > + .dir = DMA_TO_DEVICE, \
> > + }
> > +
> > +#define MHI_EP_CHANNEL_CONFIG_DL(ch_num, ch_name) \
> > + { \
> > + .num = ch_num, \
> > + .name = ch_name, \
> > + .dir = DMA_FROM_DEVICE, \
> > + }
>
> You can define it as:
>
> #define MHI_EP_CHANNEL_CONFIG(ch_num, ch_name, dir) \
> { \
> .num = ch_num, \
> .name = ch_name, \
> .dir = dir, \
> }
>
> #define MHI_EP_CHANNEL_CONFIG_DL(ch_num, ch_name) \
> MHI_EP_CHANNEL_CONFIG(ch_num, ch_name, DMA_FROM_DEVICE)
>
>
> etc.
Ok.
>
>
> > +
> > +static const struct mhi_ep_channel_config mhi_v1_channels[] = {
> > + MHI_EP_CHANNEL_CONFIG_UL(0, "LOOPBACK"),
> > + MHI_EP_CHANNEL_CONFIG_DL(1, "LOOPBACK"),
> > + MHI_EP_CHANNEL_CONFIG_UL(2, "SAHARA"),
> > + MHI_EP_CHANNEL_CONFIG_DL(3, "SAHARA"),
> > + MHI_EP_CHANNEL_CONFIG_UL(4, "DIAG"),
> > + MHI_EP_CHANNEL_CONFIG_DL(5, "DIAG"),
> > + MHI_EP_CHANNEL_CONFIG_UL(6, "SSR"),
> > + MHI_EP_CHANNEL_CONFIG_DL(7, "SSR"),
> > + MHI_EP_CHANNEL_CONFIG_UL(8, "QDSS"),
> > + MHI_EP_CHANNEL_CONFIG_DL(9, "QDSS"),
> > + MHI_EP_CHANNEL_CONFIG_UL(10, "EFS"),
> > + MHI_EP_CHANNEL_CONFIG_DL(11, "EFS"),
> > + MHI_EP_CHANNEL_CONFIG_UL(12, "MBIM"),
> > + MHI_EP_CHANNEL_CONFIG_DL(13, "MBIM"),
> > + MHI_EP_CHANNEL_CONFIG_UL(14, "QMI"),
> > + MHI_EP_CHANNEL_CONFIG_DL(15, "QMI"),
> > + MHI_EP_CHANNEL_CONFIG_UL(16, "QMI"),
> > + MHI_EP_CHANNEL_CONFIG_DL(17, "QMI"),
> > + MHI_EP_CHANNEL_CONFIG_UL(18, "IP-CTRL-1"),
> > + MHI_EP_CHANNEL_CONFIG_DL(19, "IP-CTRL-1"),
> > + MHI_EP_CHANNEL_CONFIG_UL(20, "IPCR"),
> > + MHI_EP_CHANNEL_CONFIG_DL(21, "IPCR"),
> > + MHI_EP_CHANNEL_CONFIG_UL(32, "DUN"),
> > + MHI_EP_CHANNEL_CONFIG_DL(33, "DUN"),
> > + MHI_EP_CHANNEL_CONFIG_UL(46, "IP_SW0"),
> > + MHI_EP_CHANNEL_CONFIG_DL(47, "IP_SW0"),
> > +};
> > +
> > +static const struct mhi_ep_cntrl_config mhi_v1_config = {
> > + .max_channels = 128,
> > + .num_channels = ARRAY_SIZE(mhi_v1_channels),
> > + .ch_cfg = mhi_v1_channels,
> > + .mhi_version = MHI_VERSION_1_0,
> > +};
> > +
> > +static struct pci_epf_header sdx55_header = {
> > + .vendorid = PCI_VENDOR_ID_QCOM,
> > + .deviceid = 0x0306,
> > + .baseclass_code = PCI_BASE_CLASS_COMMUNICATION,
> > + .subclass_code = PCI_CLASS_COMMUNICATION_MODEM & 0xff,
> > + .interrupt_pin = PCI_INTERRUPT_INTA,
> > +};
> > +
> > +static const struct pci_epf_mhi_ep_info sdx55_info = {
> > + .config = &mhi_v1_config,
> > + .epf_header = &sdx55_header,
> > + .bar_num = BAR_0,
> > + .epf_flags = PCI_BASE_ADDRESS_MEM_TYPE_32,
> > + .msi_count = 32,
> > + .mru = 0x8000,
> > +};
> > +
> > +struct pci_epf_mhi {
> > + const struct pci_epf_mhi_ep_info *info;
> > + struct mhi_ep_cntrl mhi_cntrl;
> > + struct pci_epf *epf;
> > + struct mutex lock;
> > + void __iomem *mmio;
> > + resource_size_t mmio_phys;
> > + u32 mmio_size;
> > + int irq;
> > + bool mhi_registered;
>
> Do we really need this variable ? Can't it be inferred from
> the framework ?
>
Looks like yes. More below.
> > +};
> > +
> > +static int pci_epf_mhi_alloc_map(struct mhi_ep_cntrl *mhi_cntrl, u64 pci_addr,
> > + phys_addr_t *phys_ptr, void __iomem **virt, size_t size)
> > +{
> > + struct pci_epf_mhi *epf_mhi = container_of(mhi_cntrl, struct pci_epf_mhi, mhi_cntrl);
> > + struct pci_epf *epf = epf_mhi->epf;
> > + struct pci_epc *epc = epf_mhi->epf->epc;
> > + size_t offset = pci_addr & (epc->mem->window.page_size - 1);
> > + void __iomem *virt_addr;
> > + phys_addr_t phys_addr;
> > + int ret;
> > +
> > + virt_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, size + offset);
> > + if (!virt_addr)
> > + return -ENOMEM;
> > +
> > + ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr, pci_addr - offset,
> > + size + offset);
> > + if (ret) {
> > + pci_epc_mem_free_addr(epc, phys_addr, virt_addr, size + offset);
> > +
> > + return ret;
> > + }
> > +
> > + *phys_ptr = phys_addr + offset;
> > + *virt = virt_addr + offset;
> > +
> > + return 0;
> > +}
> > +
> > +static void pci_epf_mhi_unmap_free(struct mhi_ep_cntrl *mhi_cntrl, u64 pci_addr,
> > + phys_addr_t phys_addr, void __iomem *virt_addr, size_t size)
> > +{
> > + struct pci_epf_mhi *epf_mhi = container_of(mhi_cntrl, struct pci_epf_mhi, mhi_cntrl);
> > + struct pci_epf *epf = epf_mhi->epf;
> > + struct pci_epc *epc = epf->epc;
> > + size_t offset = pci_addr & (epc->mem->window.page_size - 1);
> > +
> > + pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr - offset);
> > + pci_epc_mem_free_addr(epc, phys_addr - offset, virt_addr - offset, size + offset);
> > +}
> > +
> > +static void pci_epf_mhi_raise_irq(struct mhi_ep_cntrl *mhi_cntrl, u32 vector)
> > +{
> > + struct pci_epf_mhi *epf_mhi = container_of(mhi_cntrl, struct pci_epf_mhi, mhi_cntrl);
> > + struct pci_epf *epf = epf_mhi->epf;
> > + struct pci_epc *epc = epf->epc;
> > +
> > + /*
> > + * Vector is incremented by 1 here as the DWC core will decrement it before
> > + * writing to iATU.
>
> This isn't OK. It is an API, you can't write code explicitly relying on
> the underlying implementation. I assume the API is not well specified,
> that's why we need these tricks ?
>
Well, this is not an API issue but rather an implementation detail of the DWC EP
core driver. The DWC driver expects the interrupt vectors to be 1 based, so it
decrements it before writing to the MSI address:
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-ep.c#n537
> > + */
> > + pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no, PCI_EPC_IRQ_MSI, vector + 1);
> > +}
> > +
> > +static int pci_epf_mhi_read_from_host(struct mhi_ep_cntrl *mhi_cntrl, u64 from, void __iomem *to,
> > + size_t size)
> > +{
> > + struct pci_epf_mhi *epf_mhi = container_of(mhi_cntrl, struct pci_epf_mhi, mhi_cntrl);
> > + struct pci_epf *epf = epf_mhi->epf;
> > + struct pci_epc *epc = epf_mhi->epf->epc;
> > + void __iomem *tre_buf;
> > + phys_addr_t tre_phys;
> > + size_t offset = from % 0x1000;
>
> Explain what 0x1000 represents - make it a macro if that
> helps
>
Ok.
> > + int ret;
> > +
> > + mutex_lock(&epf_mhi->lock);
> > +
> > + tre_buf = pci_epc_mem_alloc_addr(epc, &tre_phys, size + offset);
> > + if (!tre_buf) {
> > + mutex_unlock(&epf_mhi->lock);
> > + return -ENOMEM;
> > + }
> > +
> > + ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, tre_phys, from - offset,
> > + size + offset);
> > + if (ret) {
> > + pci_epc_mem_free_addr(epc, tre_phys, tre_buf, size + offset);
> > + mutex_unlock(&epf_mhi->lock);
>
> I'd prefer a single goto label instead of repeating the code for every
> branch taken but that's up to you.
>
Ok.
> > + return ret;
> > + }
> > +
> > + memcpy_fromio(to, tre_buf + offset, size);
> > +
> > + pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, tre_phys);
> > + pci_epc_mem_free_addr(epc, tre_phys, tre_buf, size + offset);
> > +
> > + mutex_unlock(&epf_mhi->lock);
> > +
> > + return 0;
> > +}
> > +
[...]
> > +static int pci_epf_mhi_link_up(struct pci_epf *epf)
> > +{
> > + struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> > + const struct pci_epf_mhi_ep_info *info = epf_mhi->info;
> > + struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
> > + struct pci_epc *epc = epf->epc;
> > + struct device *dev = &epf->dev;
> > + int ret;
> > +
> > + mhi_cntrl->mmio = epf_mhi->mmio;
> > + mhi_cntrl->irq = epf_mhi->irq;
> > + mhi_cntrl->mru = info->mru;
> > +
> > + /* Assign the struct dev of PCI EP as MHI controller device */
> > + mhi_cntrl->cntrl_dev = epc->dev.parent;
> > + mhi_cntrl->raise_irq = pci_epf_mhi_raise_irq;
> > + mhi_cntrl->alloc_map = pci_epf_mhi_alloc_map;
> > + mhi_cntrl->unmap_free = pci_epf_mhi_unmap_free;
> > + mhi_cntrl->read_from_host = pci_epf_mhi_read_from_host;
> > + mhi_cntrl->write_to_host = pci_epf_mhi_write_to_host;
> > +
> > + /* Register the MHI EP controller */
> > + ret = mhi_ep_register_controller(mhi_cntrl, info->config);
> > + if (ret) {
> > + dev_err(dev, "Failed to register MHI EP controller: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + epf_mhi->mhi_registered = true;
>
> I don't like this. It should be part of mhi_ep_register_controller();
>
Hmm, I could use the "mhi_dev" pointer instead of a dedicated variable.
> > +
> > + return 0;
> > +}
> > +
> > +static int pci_epf_mhi_link_down(struct pci_epf *epf)
> > +{
> > + struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> > + struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
> > +
> > + if (epf_mhi->mhi_registered) {
> > + mhi_ep_power_down(mhi_cntrl);
> > + mhi_ep_unregister_controller(mhi_cntrl);
> > + epf_mhi->mhi_registered = false;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int pci_epf_mhi_bme(struct pci_epf *epf)
> > +{
> > + struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> > + struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
> > + struct device *dev = &epf->dev;
> > + int ret;
> > +
> > + /* Power up the MHI EP stack if link is up and stack is in power down state */
> > + if (!mhi_cntrl->enabled && epf_mhi->mhi_registered) {
> > + ret = mhi_ep_power_up(mhi_cntrl);
> > + if (ret) {
> > + dev_err(dev, "Failed to power up MHI EP: %d\n", ret);
> > + mhi_ep_unregister_controller(mhi_cntrl);
> > + epf_mhi->mhi_registered = false;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int pci_epf_mhi_bind(struct pci_epf *epf)
> > +{
> > + struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> > + struct pci_epc *epc = epf->epc;
> > + struct platform_device *pdev = to_platform_device(epc->dev.parent);
> > + struct device *dev = &epf->dev;
> > + struct resource *res;
> > + int ret;
> > +
> > + if (WARN_ON_ONCE(!epc))
> > + return -EINVAL;
> > +
> > + /* Get MMIO base address from Endpoint controller */
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mmio");
> > + epf_mhi->mmio_phys = res->start;
> > + epf_mhi->mmio_size = resource_size(res);
> > +
> > + epf_mhi->mmio = ioremap_wc(epf_mhi->mmio_phys, epf_mhi->mmio_size);
>
> ioremap_wc(). Why wc mappings ? Please explain.
>
I saw a reference to write combined access requirement for the MMIO region but
couldn't verify it now. Will revert to non_wc mapping.
- Mani
--
மணிவண்ணன் சதாசிவம்