Message-ID: <BE8371DA886269458E0220A16DC1F8277D6240B2@SHSMSX101.ccr.corp.intel.com>
Date:   Thu, 13 Apr 2017 04:12:49 +0000
From:   "Wu, Hao" <hao.wu@...el.com>
To:     'Alan Tull' <atull@...nel.org>,
        "Luebbers, Enno" <enno.luebbers@...el.com>
CC:     Moritz Fischer <moritz.fischer@...us.com>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Kang, Luwei" <luwei.kang@...el.com>,
        "Zhang, Yi Z" <yi.z.zhang@...el.com>,
        Xiao Guangrong <guangrong.xiao@...ux.intel.com>,
        "Whisonant, Tim" <tim.whisonant@...el.com>,
        "Rao, Shiva" <shiva.rao@...el.com>,
        "Rauer, Christopher" <christopher.rauer@...el.com>
Subject: RE: [PATCH 04/16] fpga: intel: pcie: parse feature list and create
 platform device for features.

> On Wed, Apr 5, 2017 at 6:58 AM, Wu Hao <hao.wu@...el.com> wrote:
> > 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),
> 
> We partly discussed this elsewhere.  If the port is implemented as a
> fpga-bridge and controlled by an fpga-region, that handles the
> enable/disable during PR.
>

Yes.
 
> > AFU driver needs similar code to implement reset interface for user space
> > application,
> 
> I would have thought that you would only want to reset the PR region
> right when PR has been completed. Or is there a reason to reset the
> port separate from programming the PR region?
> 

Yes, there are several places, 
1 ) after power on (e.g system reboot), the port is in reset state by default,
It needs to clear port reset before using the accelerator. User doesn't
need to do PR every time after system reboot.
2 ) Port has one sub feature for error reporting (related code is not
Included in this patch set), in order to clear some Port errors (e.g
recover from deep thermal throttling state), port reset is needed for
the sequence required by HW spec.
3 ) port_reset requested by user space application, I think Enno could
comment more on this.

> > PCIe driver needs port enable to make AFU MMIO region valid,
> > then it can access Device Feature Header inside this AFU MMIO during
> > enumeration.
> 
> If the port/fpga-bridge is controlled by a fpga-region, then the
> bridge is enabled once PR is finished.  As long as the AFU code knows
> that the fpga-region has been successfully programmed, it knows that
> the region can be accessed for the Device Function Header.
>

As mentioned above, users may not want to do PR every time after
system reboot. They can clear the reset and use the accelerators
directly if everything already there.

> > 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?
> 
> What is the plan for this code when you need to support a different
> FME in the future? For instance, on a new FPGA device.  I am
> suggesting that we need to figure out how to make the port a
> fpga-bridge and have it a separate module and separate out the
> enumeration code from anything else.  So that this same code can be
> used with different bridges.
> 
> I've said elsewhere that I'm hoping that this enumeration scheme can
> be the central part here and be expandable so that people can use
> other fpga-mgr's and fpga-bridges with it.  That will make this code
> reusable for future generations of your same project as will as other
> FPGA projects.
> 

For fpga-bridge, as I mentioned in some other emails, we can create
and manage them in FME. As in virtualization case, PF PCI device
could turn all its Ports/Accelerators to different VF PCI devices. So
PF PCI device may only have 1 FME but no Port/Accelerators, but it
still needs to provide PR functions to all the ports which have already
been turned to VFs and passed through to different virtual machines.

For enumeration code, I fully understand your point, but I feel
it's difficult to generalize after considered several methods but no
good solution so far, as hardware design and implementation is
quite device specific, the enumeration does not only handle DFLs,
but also needs to arrange device specific resources which are
indicated by these DFLs to create platform device.

But we can put DFLs parse function to some place as helper
function if any other drivers need them. Any other suggestions
on this will be appreciated a lot.

For the support to new FPGA devices, so far I am not sure what
will the future generations of Intel FPGAs and other FPGAs
exactly going to be, but we can try extend current PCIe driver to
support new version FME and Port, or even new modules. But
it's possible we may have totally different hardware in the future
then we have to write new drivers.

Thanks
Hao

> Alan
> 
> >
> > Thanks
> > Hao
> >
> >>
> >> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists