Re: [PATCH v5 9/9] PCI: endpoint: Add PCI Endpoint function driver for MHI bus

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Looks good, but not knowing this hardware, I only did a style review. I added
> some nits below.
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 
> 
> > ---
> >  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

-- 
மணிவண்ணன் சதாசிவம்