Re: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-Transparent Bridge

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi Sherry,

On 10/11/20 7:55 am, Sherry Sun wrote:
> Hi Kishon,
> 
>> Subject: Re: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-
>> Transparent Bridge
>>
>> Hi Sherry,
>>
>> On 09/11/20 3:07 pm, Sherry Sun wrote:
>>> Hi Kishon,
>>>
>>>> Subject: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-
>>>> Transparent Bridge
>>>>
>>>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>>>
>>>> Add support for EPF PCI-Express Non-Transparent Bridge (NTB) device.
>>>> This driver is platform independent and could be used by any platform
>>>> which have multiple PCIe endpoint instances configured using the pci-epf-
>> ntb driver.
>>>> The driver connnects to the standard NTB sub-system interface. The
>>>> EPF NTB device has configurable number of memory windows (Max 4),
>>>> configurable number of doorbell (Max 32), and configurable number of
>>>> scratch-pad registers.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  drivers/ntb/hw/Kconfig          |   1 +
>>>>  drivers/ntb/hw/Makefile         |   1 +
>>>>  drivers/ntb/hw/epf/Kconfig      |   6 +
>>>>  drivers/ntb/hw/epf/Makefile     |   1 +
>>>>  drivers/ntb/hw/epf/ntb_hw_epf.c | 755
>>>> ++++++++++++++++++++++++++++++++
>>>>  5 files changed, 764 insertions(+)
>>>>  create mode 100644 drivers/ntb/hw/epf/Kconfig  create mode 100644
>>>> drivers/ntb/hw/epf/Makefile  create mode 100644
>>>> drivers/ntb/hw/epf/ntb_hw_epf.c
>>>>
>>>> diff --git a/drivers/ntb/hw/Kconfig b/drivers/ntb/hw/Kconfig index
>>>> e77c587060ff..c325be526b80 100644
>>>> --- a/drivers/ntb/hw/Kconfig
>>>> +++ b/drivers/ntb/hw/Kconfig
>>>> @@ -2,4 +2,5 @@
>>>>  source "drivers/ntb/hw/amd/Kconfig"
>>>>  source "drivers/ntb/hw/idt/Kconfig"
>>>>  source "drivers/ntb/hw/intel/Kconfig"
>>>> +source "drivers/ntb/hw/epf/Kconfig"
>>>>  source "drivers/ntb/hw/mscc/Kconfig"
>>>> diff --git a/drivers/ntb/hw/Makefile b/drivers/ntb/hw/Makefile index
>>>> 4714d6238845..223ca592b5f9 100644
>>>> --- a/drivers/ntb/hw/Makefile
>>>> +++ b/drivers/ntb/hw/Makefile
>>>> @@ -2,4 +2,5 @@
>>>>  obj-$(CONFIG_NTB_AMD)	+= amd/
>>>>  obj-$(CONFIG_NTB_IDT)	+= idt/
>>>>  obj-$(CONFIG_NTB_INTEL)	+= intel/
>>>> +obj-$(CONFIG_NTB_EPF)	+= epf/
>>>>  obj-$(CONFIG_NTB_SWITCHTEC) += mscc/ diff --git
>>>> a/drivers/ntb/hw/epf/Kconfig b/drivers/ntb/hw/epf/Kconfig new file
>>>> mode 100644 index 000000000000..6197d1aab344
>>>> --- /dev/null
>>>> +++ b/drivers/ntb/hw/epf/Kconfig
>>>> @@ -0,0 +1,6 @@
>>>> +config NTB_EPF
>>>> +	tristate "Generic EPF Non-Transparent Bridge support"
>>>> +	depends on m
>>>> +	help
>>>> +	  This driver supports EPF NTB on configurable endpoint.
>>>> +	  If unsure, say N.
>>>> diff --git a/drivers/ntb/hw/epf/Makefile
>>>> b/drivers/ntb/hw/epf/Makefile new file mode 100644 index
>>>> 000000000000..2f560a422bc6
>>>> --- /dev/null
>>>> +++ b/drivers/ntb/hw/epf/Makefile
>>>> @@ -0,0 +1 @@
>>>> +obj-$(CONFIG_NTB_EPF) += ntb_hw_epf.o
>>>> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c
>>>> b/drivers/ntb/hw/epf/ntb_hw_epf.c new file mode 100644 index
>>>> 000000000000..0a144987851a
>>>> --- /dev/null
>>>> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
>>>> @@ -0,0 +1,755 @@
>>> ......
>>>> +static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
>>>> +			    struct pci_dev *pdev)
>>>> +{
>>>> +	struct device *dev = ndev->dev;
>>>> +	int ret;
>>>> +
>>>> +	pci_set_drvdata(pdev, ndev);
>>>> +
>>>> +	ret = pci_enable_device(pdev);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Cannot enable PCI device\n");
>>>> +		goto err_pci_enable;
>>>> +	}
>>>> +
>>>> +	ret = pci_request_regions(pdev, "ntb");
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Cannot obtain PCI resources\n");
>>>> +		goto err_pci_regions;
>>>> +	}
>>>> +
>>>> +	pci_set_master(pdev);
>>>> +
>>>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>>>> +	if (ret) {
>>>> +		ret = dma_set_mask_and_coherent(dev,
>>>> DMA_BIT_MASK(32));
>>>> +		if (ret) {
>>>> +			dev_err(dev, "Cannot set DMA mask\n");
>>>> +			goto err_dma_mask;
>>>> +		}
>>>> +		dev_warn(&pdev->dev, "Cannot DMA highmem\n");
>>>> +	}
>>>> +
>>>> +	ndev->ctrl_reg = pci_iomap(pdev, 0, 0);
>>>
>>> The second parameter of pci_iomap should be ndev->ctrl_reg_bar instead
>> of the hardcode value 0, right?
>>>
>>>> +	if (!ndev->ctrl_reg) {
>>>> +		ret = -EIO;
>>>> +		goto err_dma_mask;
>>>> +	}
>>>> +
>>>> +	ndev->peer_spad_reg = pci_iomap(pdev, 1, 0);
>>>
>>> pci_iomap(pdev, ndev->peer_spad_reg_bar, 0);
>>>
>>>> +	if (!ndev->peer_spad_reg) {
>>>> +		ret = -EIO;
>>>> +		goto err_dma_mask;
>>>> +	}
>>>> +
>>>> +	ndev->db_reg = pci_iomap(pdev, 2, 0);
>>>
>>> pci_iomap(pdev, ndev->db_reg_bar, 0);
>>
>> Good catch. Will fix it and send. Thank you for reviewing.
> 
> You're welcome.
> By the way, since I've studied VOP(virtio over pcie) before, and only recently learned
> about NTB related code. I have some questions about NTB.
> 
> For NTB, in order to make two(or more) different systems to communicate with each other, 
> seems at least three boards are required(two host boards and one board with multiple EP
> instances as bridge), right?

right, this series is about creating NTB bridge by configuring multiple
EP instances in an SoC, however there are also dedicated HW NTB switches
(internally they might as well use multiple EP instances).

> But for VOP, only two boards are needed(one board as host and one board as card) to realize the
> communication between the two systems, so my question is what are the advantages of using NTB? 

NTB is a bridge that facilitates communication between two different
systems. So it by itself will not be source or sink of any data unlike a
normal EP to RP system (or the VOP) which will be source or sink of data.
> Because I think the architecture of NTB seems more complicated. Many thanks!

yeah, I think it enables a different use case all together. Consider you
have two x86 HOST PCs (having RP) and they have to be communicate using
PCIe. NTB can be used in such cases for the two x86 PCs to communicate
with each other over PCIe, which wouldn't be possible without NTB.

Regards,
Kishon