linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alan Tull <atull@kernel.org>
To: Wu Hao <hao.wu@intel.com>
Cc: Moritz Fischer <moritz.fischer@ettus.com>,
	linux-fpga@vger.kernel.org,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"Kang, Luwei" <luwei.kang@intel.com>,
	"Zhang, Yi Z" <yi.z.zhang@intel.com>,
	Xiao Guangrong <guangrong.xiao@linux.intel.com>,
	Tim Whisonant <tim.whisonant@intel.com>,
	Enno Luebbers <enno.luebbers@intel.com>,
	Shiva Rao <shiva.rao@intel.com>,
	Christopher Rauer <christopher.rauer@intel.com>
Subject: Re: [PATCH 04/16] fpga: intel: pcie: parse feature list and create platform device for features.
Date: Tue, 11 Apr 2017 15:21:17 -0500	[thread overview]
Message-ID: <CANk1AXSv2Q978g2m23nCns3ZOuEGBdWc-Y_CyWqivnUSf33P3Q@mail.gmail.com> (raw)
In-Reply-To: <20170405115834.GA3039@hao-dev>

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

> 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

  reply	other threads:[~2017-04-11 20:22 UTC|newest]

Thread overview: 93+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-30 12:08 [PATCH 00/16] Intel FPGA Device Drivers Wu Hao
2017-03-30 12:08 ` [PATCH 01/16] docs: fpga: add a document for Intel FPGA driver overview Wu Hao
2017-03-31 18:24   ` matthew.gerlach
2017-03-31 18:38     ` Alan Tull
2017-04-01 11:16       ` Wu Hao
2017-04-02 14:41         ` Moritz Fischer
2017-04-03 20:44           ` Alan Tull
2017-04-04  5:24             ` Wu Hao
2017-04-04  5:06           ` Wu Hao
2017-04-11 18:02           ` Alan Tull
2017-04-12  3:22             ` Wu, Hao
2017-03-30 12:08 ` [PATCH 02/16] fpga: add FPGA device framework Wu Hao
2017-03-31  6:09   ` Greg KH
2017-03-31  7:48     ` Wu Hao
2017-03-31  9:03       ` Greg KH
2017-03-31 12:19         ` Wu Hao
2017-03-31 19:01       ` matthew.gerlach
2017-04-01 12:18         ` Wu Hao
2017-07-25 21:32           ` Alan Tull
2017-07-26  9:50             ` Wu Hao
2017-07-26 14:20               ` Alan Tull
2017-07-26 22:29                 ` Alan Tull
2017-07-27  4:54                   ` Wu Hao
2017-03-31  6:13   ` Greg KH
     [not found]     ` <82D7661F83C1A047AF7DC287873BF1E167C90F1B@SHSMSX101.ccr.corp.intel.com>
2017-03-31 13:31       ` Wu Hao
2017-03-31 14:10         ` Greg KH
2017-04-01 11:36           ` Wu Hao
2017-03-30 12:08 ` [PATCH 03/16] fpga: intel: add FPGA PCIe device driver Wu Hao
2017-04-04  2:10   ` Moritz Fischer
2017-04-05 13:14     ` Wu, Hao
2017-03-30 12:08 ` [PATCH 04/16] fpga: intel: pcie: parse feature list and create platform device for features Wu Hao
2017-04-03 21:44   ` Alan Tull
2017-04-05 11:58     ` Wu Hao
2017-04-11 20:21       ` Alan Tull [this message]
2017-04-13  4:12         ` Wu, Hao
2017-04-04  2:44   ` Moritz Fischer
2017-04-05 12:57     ` Wu Hao
2017-04-04 22:09   ` Alan Tull
2017-04-05 14:09     ` Wu Hao
2017-05-04 15:13   ` Li, Yi
2017-05-05  3:03     ` Wu Hao
2017-03-30 12:08 ` [PATCH 05/16] fpga: intel: pcie: add chardev support for feature devices Wu Hao
2017-03-30 12:08 ` [PATCH 06/16] fpga: intel: pcie: adds fpga_for_each_port callback for fme device Wu Hao
2017-03-30 12:08 ` [PATCH 07/16] fpga: intel: add feature device infrastructure Wu Hao
2017-03-30 12:08 ` [PATCH 08/16] fpga: intel: add FPGA Management Engine driver basic framework Wu Hao
2017-03-30 12:08 ` [PATCH 09/16] fpga: intel: fme: add header sub feature support Wu Hao
2017-03-30 12:08 ` [PATCH 10/16] fpga: intel: fme: add FPGA_GET_API_VERSION/CHECK_EXTENSION ioctls support Wu Hao
2017-03-30 12:08 ` [PATCH 11/16] fpga: intel: fme: add partial reconfiguration sub feature support Wu Hao
2017-03-30 20:30   ` Alan Tull
2017-03-31  4:11     ` Xiao Guangrong
2017-03-31  8:50       ` Wu Hao
2017-04-03 20:26         ` Alan Tull
2017-04-04  5:25           ` Wu Hao
2017-03-31 19:10   ` Alan Tull
2017-04-01 11:08     ` Wu Hao
2017-04-03 16:30       ` Alan Tull
2017-04-04  6:05         ` Wu Hao
2017-04-04 22:37           ` Alan Tull
2017-04-05 11:40             ` Wu, Hao
2017-04-05 15:26               ` Alan Tull
2017-04-05 15:39                 ` Alan Tull
2017-04-06 10:57                   ` Wu Hao
2017-04-06 19:27                     ` Alan Tull
2017-04-07  5:56                       ` Wu Hao
2017-03-31 23:45   ` kbuild test robot
2017-04-01  1:12   ` kbuild test robot
2017-04-03 21:24   ` Alan Tull
2017-04-03 22:49     ` matthew.gerlach
2017-04-04  6:48       ` Wu Hao
2017-04-04  6:28     ` Wu Hao
2017-03-30 12:08 ` [PATCH 12/16] fpga: intel: add FPGA Accelerated Function Unit driver basic framework Wu Hao
2017-03-30 12:08 ` [PATCH 13/16] fpga: intel: afu: add header sub feature support Wu Hao
2017-03-30 12:08 ` [PATCH 14/16] fpga: intel: afu add FPGA_GET_API_VERSION/CHECK_EXTENSION ioctls support Wu Hao
2017-03-30 12:08 ` [PATCH 15/16] fpga: intel: afu: add user afu sub feature support Wu Hao
2017-03-30 12:08 ` [PATCH 16/16] fpga: intel: afu: add FPGA_PORT_DMA_MAP/UNMAP ioctls support Wu Hao
2017-04-01  0:00   ` kbuild test robot
2017-04-01  1:33   ` kbuild test robot
2017-03-30 17:17 ` [PATCH 00/16] Intel FPGA Device Drivers Moritz Fischer
2017-04-06 20:27 ` Jerome Glisse
2017-04-11 19:38   ` Luebbers, Enno
2017-04-12 13:29     ` Jerome Glisse
2017-04-12 14:46       ` Moritz Fischer
2017-04-12 15:37         ` Jerome Glisse
2017-04-14 19:48           ` Luebbers, Enno
2017-04-14 20:49             ` Jerome Glisse
2017-04-17 15:35               ` Alan Tull
2017-04-17 15:57                 ` Jerome Glisse
2017-04-17 16:22                   ` Alan Tull
2017-04-17 17:15                     ` Jerome Glisse
2017-04-18 13:36                   ` Alan Cox
2017-04-18 14:59                     ` Jerome Glisse
2017-04-25 20:02                       ` One Thousand Gnomes
2017-05-01 16:41                         ` Jerome Glisse

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANk1AXSv2Q978g2m23nCns3ZOuEGBdWc-Y_CyWqivnUSf33P3Q@mail.gmail.com \
    --to=atull@kernel.org \
    --cc=christopher.rauer@intel.com \
    --cc=enno.luebbers@intel.com \
    --cc=guangrong.xiao@linux.intel.com \
    --cc=hao.wu@intel.com \
    --cc=linux-fpga@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luwei.kang@intel.com \
    --cc=moritz.fischer@ettus.com \
    --cc=shiva.rao@intel.com \
    --cc=tim.whisonant@intel.com \
    --cc=yi.z.zhang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).