Message-ID: <20170405115834.GA3039@hao-dev>
Date:   Wed, 5 Apr 2017 19:58:34 +0800
From:   Wu Hao <hao.wu@...el.com>
To:     Alan Tull <atull@...nel.org>
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 Mon, Apr 03, 2017 at 04:44:15PM -0500, Alan Tull wrote:
> 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).

Hi Alan

The major purpose of feature-dev is to create infrastructure for feature
device. Please refer to patch 7.  It abstracts feature device common
data structures and functions in feature-dev.c for FME and AFU now (but
may be more in the future). The reason we add port enable/disable/etc
fuctions there for code reuse. e.g FME driver needs port enable/disable
for PR (in the future, to implement the fpga bridge enable_set function),
AFU driver needs similar code to implement reset interface for user space
application, PCIe driver needs port enable to make AFU MMIO region valid,
then it can access Device Feature Header inside this AFU MMIO during
enumeration. Other fpga_port_* are all for code reuse purpose too. So we
should not put these function in the same feature-dev.c but a seperated
file?

Thanks
Hao

> 
> Alan

Powered by blists - more mailing lists