Re: [PATCH 04/16] fpga: intel: pcie: parse feature list and create platform device for features.

[Alan Tull]

On Wed, Apr 5, 2017 at 6:58 AM, Wu Hao <hao.wu@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>> > From: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxx>
>> > Signed-off-by: Enno Luebbers <enno.luebbers@xxxxxxxxx>
>> > Signed-off-by: Shiva Rao <shiva.rao@xxxxxxxxx>
>> > Signed-off-by: Christopher Rauer <christopher.rauer@xxxxxxxxx>
>> > Signed-off-by: Kang Luwei <luwei.kang@xxxxxxxxx>
>> > Signed-off-by: Zhang Yi <yi.z.zhang@xxxxxxxxx>
>> > Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
>> > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
>> > ---
>> >  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@xxxxxxxxx>
>> > + *   Zhang Yi <yi.z.zhang@xxxxxxxxx>
>> > + *   Wu Hao <hao.wu@xxxxxxxxx>
>> > + *   Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
>> > + *
>> > + * 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.

> 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?

> 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.

> 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.

Alan

>
> Thanks
> Hao
>
>>
>> Alan