Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755108AbdDEOOA (ORCPT ); Wed, 5 Apr 2017 10:14:00 -0400 Received: from mga04.intel.com ([192.55.52.120]:25695 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753663AbdDEONz (ORCPT ); Wed, 5 Apr 2017 10:13:55 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,278,1486454400"; d="scan'208";a="85054485" Date: Wed, 5 Apr 2017 22:09:16 +0800 From: Wu Hao To: Alan Tull 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 Subject: Re: [PATCH 04/16] fpga: intel: pcie: parse feature list and create platform device for features. Message-ID: <20170405140916.GC3039@hao-dev> References: <1490875696-15145-1-git-send-email-hao.wu@intel.com> <1490875696-15145-5-git-send-email-hao.wu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3578 Lines: 75 On Tue, Apr 04, 2017 at 05:09:23PM -0500, Alan Tull wrote: > 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. > > > > I'm still looking at this code and it's pretty new to me, but I think > it would be desirable and not really hard to separate the code > that enumerates from all the fixed feature code. So pcie.c could see > that there is a pci device out there that it knows has these memory > mapped enumeration structs. So it goes and reads the structs, parses > them, and knows what drivers to probe. The FME and AFU and other > fpga device drivers could register their guids with the framework > and be discoverable in that way. > > That way if you need to implement a different FME or anything else, it > could be added with a new guid to this framework and would get > enumerated. I'm thinking of the future and of more general usability > of this code. > > Then the enumeration code wouldn't have to be 'intel' code or even > code dedicated to FME's and AFU's. Any FPGA with a PCIe > port that has the right id's could have this struct and use this > enumeration method. Actually if the parse* enumeration code could be in a > separate file as helper functions for the pcie code, this stuff would > be structured for future support this of the same framework on > embedded FPGA devices. > Hi Alan Thank you very much for the review and comments. Actually I am not sure if the 'Device Feature List' is designed for common usage or not, but per current implementation of Port/AFU and FME, they did not use the exact same way for enumeration. e.g PCIe driver reads register under Port to know the AFU MMIO region size. So for each module, it has its own method to enumerate and prepare the resource for its platform device. Other developers may not be able to use them for a new module. >From the whole device's point of view, do enumeration for all modules is still a device specific thing. e.g 'Device Feature List' of the FME is in PCI BAR0, but the location of Port/AFU's 'Device Feature List' is not linked to FME's Device Feature List, but given (PCI BAR + offset) by a FME register. So the process of enumeration may be totally different in another device with different module. I don't expect FME can be used without PCIE or Port/AFU now, as it ties to them so closely. e.g give PCI Bar info for port, registers to support PCI SRIOV function. And so does PCIe and AFU driver, e.g PCIe driver is not only handling the enumeration, but also manage ports and access FME registers for SRIOV function support (sriov related code is not included in this patch set). If we have any PCIE FPGA has FME, then we can reuse this Intel FPGA driver directly, but if no FME, then most code can't be reused. I fully understand your point for code reuse and agree with you that parse* enumeration code could be in a common place as helper function. But for others, I still have concern due to current hardware implementation. Anyway I will continue consideration on this. Thanks Hao > Alan