On Thu, May 03, 2018 at 04:14:48PM -0500, Alan Tull wrote:

On Tue, May 1, 2018 at 9:50 PM, Wu Hao [off-list ref] wrote:

quoted

Hi All,

Here is v5 patch-series adding drivers for FPGA DFL devices.

This patch series provides a common framework to support FPGA Device Feature
List (DFL), and also feature dev drivers under this DFL framework to provide
interfaces for userspace applications to configure, enumerate, open, and
access FPGA accelerators on DFL based FPGA device and enables system level
management functions such as FPGA partial reconfiguration, power management
and virtualization.

Hi Hao,

Hi Alan,

Thanks for your review on this patch set.

I've started looking at your v5 here.  It's 5212 lines of code, so
it's not tiny.

I see a lot of renaming from 'fpga' to 'dfl', thanks for doing that.

The main thing that stands out at first look is a new method of
registering port ops.  It would be better if the fpga bridge framework
were expanded or changed somehow to do that.  It's like we have two
bridge frameworks now.

I only limit this change in DFL framework, because introduce a port ops
here to DFL framework is to resolve the hard dependency between DFL
driver modules (e.g DFL FME and port), this sounds like a DFL specific
thing, and I am not sure if there is some real need or benefit for other
non-DFL module, so i didn't modify the common fpga bridge code.

I see that there isn't a way of specifying which FPGA mgr driver to
use, so dfl-fme-mgr.c is tied to dfl-fme-pr.c.  It would be better if
the DFL could specify which fpga manager driver to use.

Hm.. I think we discussed this against v4. It could be a different sub
feature with different id to support different PR hardware.  In that
case, we could still reuse sub feature driver provided by dfl-fme-pr.c
(specify a new id which reuses the same pr_mgmt_ops)

        {
                 .id = FME_FEATURE_ID_PR_MGMT,
                 .ops = &pr_mgmt_ops,
        },

Inside dfl-fme-pr.c, it could parse the different id to create different
platform devices for different FPGA mgrs.

Could we leave this a later change? as i don't have a different hardware
like this at this moment (maybe future) and I don't see current framework
block us on reusing these code.

Maybe I'm repeating myself here, the biggest question I'm asking
region, how much of this code can be reused?  I would hope that all of
it could except for the fpga manager/bridge drivers.  That was the
intent of the original FPGA framework.  

I fully understand that it's better to make code reusable, this is what
we are trying to do in these patchsets. But it's still not very clear that
who others or which products will use this DFL framework and how, what's
the difficulty they will face and what kind of help or changes we could
provide. so maybe we could defer some work when people really need it, if
there is no hard blocking issue on architecture level.

I'll have more time next week to look in more depth.

Looking forward to your feedback. Thanks again for your review. :)

Hao

Alan