On Fri, Jun 02, 2023 at 08:31:31AM +0900, Damien Le Moal wrote:
> On 6/1/23 23:57, 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>
>
> Looks good, but not knowing this hardware, I only did a style review. I added
> some nits below.
>
> Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
>
>
> > ---
> > drivers/pci/endpoint/functions/Kconfig | 10 +
> > drivers/pci/endpoint/functions/Makefile | 1 +
> > drivers/pci/endpoint/functions/pci-epf-mhi.c | 462 +++++++++++++++++++
> > 3 files changed, 473 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..98f0d96cfd46
> > --- /dev/null
> > +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
[...]
> > +static int pci_epf_mhi_write_to_host(struct mhi_ep_cntrl *mhi_cntrl,
> > + void __iomem *from, u64 to, size_t size)
> > +{
> > + struct pci_epf_mhi *epf_mhi = to_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 = to % SZ_4K;
> > + int ret;
> > +
> > + mutex_lock(&epf_mhi->lock);
> > +
> > + tre_buf = pci_epc_mem_alloc_addr(epc, &tre_phys, size + offset);
> > + if (!tre_buf) {
> > + ret = -ENOMEM;
> > + goto err_unlock;
> > + }
> > +
> > + ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, tre_phys,
> > + to - offset, size + offset);
> > + if (ret) {
> > + pci_epc_mem_free_addr(epc, tre_phys, tre_buf, size + offset);
> > + goto err_unlock;
> > + }
>
> The above 2 hunks are that same as in pci_epf_mhi_read_from_host(). May be pack
> these in a helper function ? E.g. pci_epf_mhi_map() ?
> Note that I have a patch series in the work that does that at the pci_epc API
> level (pci_epc_map() + pci_epc_unmap()) to simplify function drivers.
>
> > +
> > + memcpy_toio(tre_buf + offset, from, 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);
>
> Same here: pci_epf_mhi_unmap() ?
>
Agree with both of the above comments. At have this change in my local tree but
that's not part of this series. Will merge it to this patch itself.
> > +
> > +err_unlock:
> > + mutex_unlock(&epf_mhi->lock);
> > +
> > + return ret;
> > +}
> > +
[...]
> > +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;
>
> This should never happen. If there is a possibility that the EPC core code calls
> bind with epc == NULL, we need to fix that there.
>
Right, this seems to be redundant.
- Mani
--
மணிவண்ணன் சதாசிவம்
