Re: [PATCH v3 7/7] PCI: endpoint: Add PCI Endpoint function driver for MHI bus

[Manivannan Sadhasivam]

Hi Kishon,

On Mon, May 15, 2023 at 01:37:28PM +0530, Kishon VijayAbraham I wrote:
> Hi Manivannan,
> 
> On 3/14/2023 10:16 AM, 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...
> > 
> > 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..03e7f42663b3
> > --- /dev/null
> > +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> .
> .
> <snip>
> .
> .
> > +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;
> > +	}
> 
> Any reason for delaying registration with MHI bus till link up? Since after
> linkup, the host can start using the device, this should be doing minimal
> stuff IMHO. Or is there further handshake between the host side MHI driver
> and the endpoint side MHI driver?
> 

Yes, there are further handshakes required between ep and host before the host
can start using MHI and this is done during mhi_ep_power_up(). Moreover,
registering the controller during link_up event allows us to do cleanup properly
when the link goes down.

- Mani
 
> Thanks,
> Kishon

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