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

[Sherry Sun]

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@xxxxxx>
> >>
> >> 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@xxxxxx>
> >> ---
> >>  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?
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? 
Because I think the architecture of NTB seems more complicated. Many thanks!

Best regards
Sherry

> 
> Regards,
> Kishon