Message-ID: <CANk1AXQBvvozPJqc=ZnfyYizEyY_NqijhD2s1Fi+=+oEC-Hi2Q@mail.gmail.com>
Date:   Mon, 3 Apr 2017 16:44:15 -0500
From:   Alan Tull <atull@...nel.org>
To:     Wu Hao <hao.wu@...el.com>
Cc:     Moritz Fischer <moritz.fischer@...us.com>,
        linux-fpga@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        luwei.kang@...el.com, yi.z.zhang@...el.com,
        Xiao Guangrong <guangrong.xiao@...ux.intel.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>
Subject: Re: [PATCH 04/16] fpga: intel: pcie: parse feature list and create
 platform device for features.

On Thu, Mar 30, 2017 at 7:08 AM, Wu Hao <hao.wu@...el.com> wrote:
> From: Xiao Guangrong <guangrong.xiao@...ux.intel.com>
>
> Device Featuer List structure creates a link list of feature headers
> within the MMIO space to provide an extensiable way of adding features.
>
> The Intel FPGA PCIe driver walks through the feature headers to enumerate
> feature devices, FPGA Management Engine (FME) and FPGA Port for Accelerated
> Function Unit (AFU), and their private sub features. For feature devices,
> it creates the platform devices and linked the private sub features into
> their platform data.
>
> 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: Kang Luwei <luwei.kang@...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/intel/Makefile      |   2 +-
>  drivers/fpga/intel/feature-dev.c | 139 +++++++
>  drivers/fpga/intel/feature-dev.h | 342 ++++++++++++++++
>  drivers/fpga/intel/pcie.c        | 841 ++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 1321 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/fpga/intel/feature-dev.c
>  create mode 100644 drivers/fpga/intel/feature-dev.h
>
> diff --git a/drivers/fpga/intel/Makefile b/drivers/fpga/intel/Makefile
> index 61fd8ea..c029940 100644
> --- a/drivers/fpga/intel/Makefile
> +++ b/drivers/fpga/intel/Makefile
> @@ -1,3 +1,3 @@
>  obj-$(CONFIG_INTEL_FPGA_PCI) += intel-fpga-pci.o
>
> -intel-fpga-pci-objs := pcie.o
> +intel-fpga-pci-objs := pcie.o feature-dev.o
> diff --git a/drivers/fpga/intel/feature-dev.c b/drivers/fpga/intel/feature-dev.c
> new file mode 100644
> index 0000000..6952566
> --- /dev/null
> +++ b/drivers/fpga/intel/feature-dev.c
> @@ -0,0 +1,139 @@
> +/*
> + * Intel FPGA Feature Device Driver
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Kang Luwei <luwei.kang@...el.com>
> + *   Zhang Yi <yi.z.zhang@...el.com>
> + *   Wu Hao <hao.wu@...el.com>
> + *   Xiao Guangrong <guangrong.xiao@...ux.intel.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 "feature-dev.h"
> +
> +void feature_platform_data_add(struct feature_platform_data *pdata,
> +                              int index, const char *name,
> +                              int resource_index, void __iomem *ioaddr)
> +{
> +       WARN_ON(index >= pdata->num);
> +
> +       pdata->features[index].name = name;
> +       pdata->features[index].resource_index = resource_index;
> +       pdata->features[index].ioaddr = ioaddr;
> +}
> +
> +int feature_platform_data_size(int num)
> +{
> +       return sizeof(struct feature_platform_data) +
> +               num * sizeof(struct feature);
> +}
> +
> +struct feature_platform_data *
> +feature_platform_data_alloc_and_init(struct platform_device *dev, int num)
> +{
> +       struct feature_platform_data *pdata;
> +
> +       pdata = kzalloc(feature_platform_data_size(num), GFP_KERNEL);
> +       if (pdata) {
> +               pdata->dev = dev;
> +               pdata->num = num;
> +               mutex_init(&pdata->lock);
> +       }
> +
> +       return pdata;
> +}
> +
> +int fme_feature_num(void)
> +{
> +       return FME_FEATURE_ID_MAX;
> +}
> +
> +int port_feature_num(void)
> +{
> +       return PORT_FEATURE_ID_MAX;
> +}
> +
> +int fpga_port_id(struct platform_device *pdev)
> +{
> +       struct feature_port_header *port_hdr;
> +       struct feature_port_capability capability;
> +
> +       port_hdr = get_feature_ioaddr_by_index(&pdev->dev,
> +                                              PORT_FEATURE_ID_HEADER);
> +       WARN_ON(!port_hdr);
> +
> +       capability.csr = readq(&port_hdr->capability);
> +       return capability.port_number;
> +}
> +EXPORT_SYMBOL_GPL(fpga_port_id);
> +
> +/*
> + * Enable Port by clear the port soft reset bit, which is set by default.
> + * The User AFU is unable to respond to any MMIO access while in reset.
> + * __fpga_port_enable function should only be used after __fpga_port_disable
> + * function.
> + */
> +void __fpga_port_enable(struct platform_device *pdev)
> +{

feature-dev.c is handling enumeration and adding port
enable/disable/etc functions for a specific port device.  I see the
port as a fpga-bridge.  The enumeration code should be separate from
the bridge code.  Especially separate from a very specific bridge low
level device driver implementation, otherwise this becomes obsolete as
soon as you have another port device with a different register
implementation.  Even if you handle that, then this enumeration code
isn't useable by other people who are using fpga-bridge.  The
fpga-bridge framework exists to separate low level things like how to
enable/disable a specific bridge device from upper level code that
knows when to enable/disable it (fpga-region).

Alan

Powered by blists - more mailing lists