Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752685AbdDCVpG (ORCPT ); Mon, 3 Apr 2017 17:45:06 -0400 Received: from mail.kernel.org ([198.145.29.136]:58852 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752439AbdDCVpE (ORCPT ); Mon, 3 Apr 2017 17:45:04 -0400 MIME-Version: 1.0 In-Reply-To: <1490875696-15145-5-git-send-email-hao.wu@intel.com> References: <1490875696-15145-1-git-send-email-hao.wu@intel.com> <1490875696-15145-5-git-send-email-hao.wu@intel.com> From: Alan Tull Date: Mon, 3 Apr 2017 16:44:15 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 04/16] fpga: intel: pcie: parse feature list and create platform device for features. To: Wu Hao Cc: Moritz Fischer , linux-fpga@vger.kernel.org, linux-kernel , luwei.kang@intel.com, yi.z.zhang@intel.com, Xiao Guangrong , Tim Whisonant , Enno Luebbers , Shiva Rao , Christopher Rauer Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5311 Lines: 141 On Thu, Mar 30, 2017 at 7:08 AM, Wu Hao wrote: > From: Xiao Guangrong > > 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 > Signed-off-by: Enno Luebbers > Signed-off-by: Shiva Rao > Signed-off-by: Christopher Rauer > Signed-off-by: Kang Luwei > Signed-off-by: Zhang Yi > Signed-off-by: Xiao Guangrong > Signed-off-by: Wu Hao > --- > 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 > + * Zhang Yi > + * Wu Hao > + * Xiao Guangrong > + * > + * 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). Alan