Date:   Mon, 3 Apr 2017 19:10:28 -0700
From:   Moritz Fischer <mdf@...nel.org>
To:     Wu Hao <hao.wu@...el.com>
Cc:     atull@...nel.org, moritz.fischer@...us.com,
        linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org,
        luwei.kang@...el.com, yi.z.zhang@...el.com,
        Tim Whisonant <tim.whisonant@...el.com>,
        Enno Luebbers <enno.luebbers@...el.com>,
        Shiva Rao <shiva.rao@...el.com>,
        Christopher Rauer <christopher.rauer@...el.com>,
        Xiao Guangrong <guangrong.xiao@...ux.intel.com>
Subject: Re: [PATCH 03/16] fpga: intel: add FPGA PCIe device driver

On Thu, Mar 30, 2017 at 08:08:03PM +0800, Wu Hao wrote:
> From: Zhang Yi <yi.z.zhang@...el.com>
> 
> The Intel FPGA device appears as a PCIe device on the system. This patch
> implements the basic framework of the driver for Intel PCIe device which
> locates between CPU and Accelerated Function Units (AFUs).
> 
> Signed-off-by: Tim Whisonant <tim.whisonant@...el.com>
> Signed-off-by: Enno Luebbers <enno.luebbers@...el.com>
> Signed-off-by: Shiva Rao <shiva.rao@...el.com>
> Signed-off-by: Christopher Rauer <christopher.rauer@...el.com>
> Signed-off-by: Zhang Yi <yi.z.zhang@...el.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> Signed-off-by: Wu Hao <hao.wu@...el.com>
> ---
>  drivers/fpga/Kconfig           |   2 +
>  drivers/fpga/Makefile          |   3 +
>  drivers/fpga/intel/Kconfig     |  27 +++++++++
>  drivers/fpga/intel/LICENSE.BSD |  24 ++++++++
>  drivers/fpga/intel/Makefile    |   3 +
>  drivers/fpga/intel/pcie.c      | 129 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 188 insertions(+)
>  create mode 100644 drivers/fpga/intel/Kconfig
>  create mode 100644 drivers/fpga/intel/LICENSE.BSD
>  create mode 100644 drivers/fpga/intel/Makefile
>  create mode 100644 drivers/fpga/intel/pcie.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d99b640..4e49aee 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -69,6 +69,8 @@ config ALTERA_FREEZE_BRIDGE
>  	  isolate one region of the FPGA from the busses while that
>  	  region is being reprogrammed.
>  
> +source "drivers/fpga/intel/Kconfig"
> +
>  endif # FPGA
>  
>  endmenu
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 53a41d2..46f1a5d 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -20,3 +20,6 @@ obj-$(CONFIG_ALTERA_FREEZE_BRIDGE)	+= altera-freeze-bridge.o
>  
>  # High Level Interfaces
>  obj-$(CONFIG_FPGA_REGION)		+= fpga-region.o
> +
> +# Intel FPGA Support
> +obj-$(CONFIG_INTEL_FPGA)		+= intel/
> diff --git a/drivers/fpga/intel/Kconfig b/drivers/fpga/intel/Kconfig
> new file mode 100644
> index 0000000..bf402f3
> --- /dev/null
> +++ b/drivers/fpga/intel/Kconfig
> @@ -0,0 +1,27 @@
> +menuconfig INTEL_FPGA
> +	tristate "Intel(R) FPGA support"
> +	depends on FPGA_DEVICE
> +	help
> +	  Select this option to enable driver support for Intel(R)
> +	  Field-Programmable Gate Array (FPGA) solutions. This driver
> +	  provides interfaces for userspace applications to configure,
> +	  enumerate, open, and access FPGA accelerators on platforms
> +	  equipped with Intel(R) FPGA solutions and enables system
> +	  level management functions such as FPGA reconfiguration,
> +	  power management, and virtualization.
> +
> +	  Say Y if your platform has this technology. Say N if unsure.
> +
> +if INTEL_FPGA
> +
> +config INTEL_FPGA_PCI
> +	tristate "Intel FPGA PCIe Driver"
> +	depends on PCI
> +	help
> +	  This is the driver for the PCIe device which locates between
> +	  CPU and Accelerated Function Units (AFUs) and allows them to
> +	  communicate with each other.
> +
> +	  To compile this as a module, choose M here.
> +
> +endif
> diff --git a/drivers/fpga/intel/LICENSE.BSD b/drivers/fpga/intel/LICENSE.BSD
> new file mode 100644
> index 0000000..309d2b7
> --- /dev/null
> +++ b/drivers/fpga/intel/LICENSE.BSD
> @@ -0,0 +1,24 @@
> +Redistribution and use in source and binary forms, with or without
> +modification, are permitted provided that the following conditions
> +are met:
> +  * Redistributions of source code must retain the above copyright
> +    notice, this list of conditions and the following disclaimer.
> +  * Redistributions in binary form must reproduce the above copyright
> +    notice, this list of conditions and the following disclaimer in
> +    the documentation and/or other materials provided with the
> +    distribution.
> +  * Neither the name of Intel Corporation nor the names of its
> +    contributors may be used to endorse or promote products derived
> +    from this software without specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> diff --git a/drivers/fpga/intel/Makefile b/drivers/fpga/intel/Makefile
> new file mode 100644
> index 0000000..61fd8ea
> --- /dev/null
> +++ b/drivers/fpga/intel/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_INTEL_FPGA_PCI) += intel-fpga-pci.o
> +
> +intel-fpga-pci-objs := pcie.o
> diff --git a/drivers/fpga/intel/pcie.c b/drivers/fpga/intel/pcie.c
> new file mode 100644
> index 0000000..132d9da
> --- /dev/null
> +++ b/drivers/fpga/intel/pcie.c
> @@ -0,0 +1,129 @@
> +/*
> + * Driver for Intel FPGA PCIe device
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Zhang Yi <Yi.Z.Zhang@...el.com>
> + *   Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> + *   Joseph Grecco <joe.grecco@...el.com>
> + *   Enno Luebbers <enno.luebbers@...el.com>
> + *   Tim Whisonant <tim.whisonant@...el.com>
> + *   Ananda Ravuri <ananda.ravuri@...el.com>
> + *   Henry Mitchel <henry.mitchel@...el.com>
> + *
> + * This work is licensed under a dual BSD/GPLv2 license. When using or
> + * redistributing this file, you may do so under either license. See the
> + * LICENSE.BSD file under this directory for the BSD license and see
> + * the COPYING file in the top-level directory for the GPLv2 license.
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/stddef.h>
> +#include <linux/errno.h>
> +#include <linux/aer.h>
> +
> +#define DRV_VERSION	"EXPERIMENTAL VERSION"

Is that a leftover? :)
> +#define DRV_NAME	"intel-fpga-pci"
> +
> +/* PCI Device ID */
> +#define PCIe_DEVICE_ID_PF_INT_5_X	0xBCBD
> +#define PCIe_DEVICE_ID_PF_INT_6_X	0xBCC0
> +#define PCIe_DEVICE_ID_PF_DSC_1_X	0x09C4
> +/* VF Device */
> +#define PCIe_DEVICE_ID_VF_INT_5_X	0xBCBF
> +#define PCIe_DEVICE_ID_VF_INT_6_X	0xBCC1
> +#define PCIe_DEVICE_ID_VF_DSC_1_X	0x09C5
> +
> +static struct pci_device_id cci_pcie_id_tbl[] = {
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_INT_5_X),},
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_INT_5_X),},
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_INT_6_X),},
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_INT_6_X),},
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_DSC_1_X),},
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_DSC_1_X),},
> +	{0,}
> +};
> +MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
> +
> +static
> +int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> +{
> +	int ret;
> +
> +	ret = pci_enable_device(pcidev);
> +	if (ret < 0) {
> +		dev_err(&pcidev->dev, "Failed to enable device %d.\n", ret);
> +		goto exit;
Why not 'return ret' here ?
> +	}
> +
> +	ret = pci_enable_pcie_error_reporting(pcidev);
> +	if (ret && ret != -EINVAL)
> +		dev_info(&pcidev->dev, "PCIE AER unavailable %d.\n", ret);

What if it is EINVAL?

> +
> +	ret = pci_request_regions(pcidev, DRV_NAME);
> +	if (ret) {
> +		dev_err(&pcidev->dev, "Failed to request regions.\n");
> +		goto disable_error_report_exit;
> +	}
> +
> +	pci_set_master(pcidev);
> +	pci_save_state(pcidev);
> +
> +	if (!dma_set_mask(&pcidev->dev, DMA_BIT_MASK(64))) {
> +		dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(64));
> +	} else if (!dma_set_mask(&pcidev->dev, DMA_BIT_MASK(32))) {
> +		dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(32));
> +	} else {
> +		ret = -EIO;
> +		dev_err(&pcidev->dev, "No suitable DMA support available.\n");
> +		goto release_region_exit;
> +	}
> +
> +	/* TODO: create and add the platform device per feature list */
> +	return 0;
> +
> +release_region_exit:
> +	pci_release_regions(pcidev);
> +disable_error_report_exit:
> +	pci_disable_pcie_error_reporting(pcidev);
> +	pci_disable_device(pcidev);
> +exit:
> +	return ret;
If you return as suggested above, this can go away.

> +}
> +
> +static void cci_pci_remove(struct pci_dev *pcidev)
> +{
> +	pci_release_regions(pcidev);
> +	pci_disable_pcie_error_reporting(pcidev);
> +	pci_disable_device(pcidev);
> +}
> +
> +static struct pci_driver cci_pci_driver = {
> +	.name = DRV_NAME,
> +	.id_table = cci_pcie_id_tbl,
> +	.probe = cci_pci_probe,
> +	.remove = cci_pci_remove,
> +};
> +
> +static int __init ccidrv_init(void)
> +{
> +	pr_info("Intel(R) FPGA PCIe Driver: Version %s\n", DRV_VERSION);
> +
> +	return pci_register_driver(&cci_pci_driver);
> +}
> +
> +static void __exit ccidrv_exit(void)
> +{
> +	pci_unregister_driver(&cci_pci_driver);
> +}
> +
> +module_init(ccidrv_init);
> +module_exit(ccidrv_exit);
> +
> +MODULE_DESCRIPTION("Intel FPGA PCIe Device Driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("Dual BSD/GPL");
> -- 
> 2.7.4
> 

Cheers,

Moritz

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)

Powered by blists - more mailing lists