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

From: Kishon Vijay Abraham I <kishon@ti.com>
To: Sherry Sun <sherry.sun@nxp.com>
Cc: "bhelgaas@google.com" <bhelgaas@google.com>,
	Jonathan Corbet <corbet@lwn.net>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"jdmason@kudzu.us" <jdmason@kudzu.us>,
	"dave.jiang@intel.com" <dave.jiang@intel.com>,
	"allenbh@gmail.com" <allenbh@gmail.com>,
	"tjoseph@cadence.com" <tjoseph@cadence.com>,
	Rob Herring <robh@kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-ntb@googlegroups.com" <linux-ntb@googlegroups.com>
Subject: Re: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-Transparent Bridge
Date: Mon, 9 Nov 2020 19:51:44 +0530	[thread overview]
Message-ID: <d6d27475-3464-6772-2122-cc194b8ae022@ti.com> (raw)
In-Reply-To: <VI1PR04MB496061EAB6F249F1C394F01092EA0@VI1PR04MB4960.eurprd04.prod.outlook.com>

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.

Regards,
Kishon