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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ff522b83-d84a-d76e-0317-e8ca55fb82b8@amd.com>
Date:   Tue, 16 May 2023 09:08:13 +0530
From:   Kishon Vijay Abraham I <Kishon.vijayabraham@....com>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Kishon VijayAbraham I <kvijayab@....com>
Cc:     lpieralisi@...nel.org, kw@...ux.com, kishon@...nel.org,
        bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v3 7/7] PCI: endpoint: Add PCI Endpoint function driver
 for MHI bus



On 5/15/2023 5:06 PM, Manivannan Sadhasivam wrote:
> 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@...aro.org>
>>> ---
>>>    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.

Thank you for clarifying!

For the entire series:

Reviewed-by: Kishon Vijay Abraham I <kishon@...nel.org>
> 
> - Mani
>   
>> Thanks,
>> Kishon
>