From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Cyrus-Session-Id: sloti22d1t05-2219703-1520398165-2-12429888020239996524 X-Sieve: CMU Sieve 3.0 X-Spam-known-sender: no X-Spam-score: 0.0 X-Spam-hits: BAYES_00 -1.9, HEADER_FROM_DIFFERENT_DOMAINS 0.25, ME_NOAUTH 0.01, RCVD_IN_DNSWL_HI -5, T_RP_MATCHES_RCVD -0.01, LANGUAGES en, BAYES_USED global, SA_VERSION 3.4.0 X-Spam-source: IP='209.132.180.67', Host='vger.kernel.org', Country='CN', FromHeader='com', MailFrom='org' X-Spam-charsets: plain='us-ascii' X-Resolved-to: greg@kroah.com X-Delivered-to: greg@kroah.com X-Mail-from: linux-api-owner@vger.kernel.org ARC-Seal: i=1; a=rsa-sha256; cv=none; d=messagingengine.com; s=arctest; t=1520398164; b=DOH7TieKmI5IkVi6HxsSVJ3y869wY4im7SWYYmtIGQqHY/x hKh2lnOoZ2BGp8TRR87Cq02+HScKsqDA+qfTVCOZHN/7DjU3aFrKF0/7MF8U8WxT isgz8PirAUuwRb/SlGRoNzN75H4La3Qgnyojf8rF223ttOvq9Q2a7p23SXmvtDe1 crjKFlkRuQdMfgJCY97+a3lNGodFZxDg+2JPY3cIkLId08ImyViI5kTiXo+nxHn8 PrK0Ncvc7UJJM6cThvsRqMeXGeXFU77AsUysFtMPJDOIIUM+Nd3byivn05b/0539 bEgdehHoPL0NvHDwrUx0uu50N9BPyBIBLLS75Wg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=date:from:to:cc:subject:message-id :references:mime-version:content-type:in-reply-to:sender :list-id; s=arctest; t=1520398164; bh=DuszvEP/mspPnsXEDBp0KIG+lR EMedDOdZ9Czwoyow0=; b=ShiB5HxVyccycep4MX5zuIZxMMK/W7i2X7IjyGMne3 ueErSX2MrTi0Mz1yEujKarxtvay3hUbNwnwVX++5USe5xd8/FC9B01ArKUSIPACQ +HpSQUO4MWGtGksoJaC90jXgxaDn0c4xBEiI9l5ZVaXccbM5UsTBiRq3lhgqCz7e /urWdci34lghkP6m9YT/AFu10cwL50bVF8BjMcypID8hC0AQ4jFiIKc9Mz6nH1vv S+f9r2571zrgz9Bz1Vg82cIoeAdWNF2VoGopvyKgc5Q5qN9/PvFt3RNupwmv102h vtNP6o30SD8Dx7zEkdauKfj89aIgyRO67VteajySUJrg== ARC-Authentication-Results: i=1; mx4.messagingengine.com; arc=none (no signatures found); dkim=none (no signatures found); dmarc=none (p=none,has-list-id=yes,d=none) header.from=intel.com; iprev=pass policy.iprev=209.132.180.67 (vger.kernel.org); spf=none smtp.mailfrom=linux-api-owner@vger.kernel.org smtp.helo=vger.kernel.org; x-aligned-from=fail; x-category=clean score=-100 state=0; x-ptr=pass x-ptr-helo=vger.kernel.org x-ptr-lookup=vger.kernel.org; x-return-mx=pass smtp.domain=vger.kernel.org smtp.result=pass smtp_org.domain=kernel.org smtp_org.result=pass smtp_is_org_domain=no header.domain=intel.com header.result=pass header_is_org_domain=yes Authentication-Results: mx4.messagingengine.com; arc=none (no signatures found); dkim=none (no signatures found); dmarc=none (p=none,has-list-id=yes,d=none) header.from=intel.com; iprev=pass policy.iprev=209.132.180.67 (vger.kernel.org); spf=none smtp.mailfrom=linux-api-owner@vger.kernel.org smtp.helo=vger.kernel.org; x-aligned-from=fail; x-category=clean score=-100 state=0; x-ptr=pass x-ptr-helo=vger.kernel.org x-ptr-lookup=vger.kernel.org; x-return-mx=pass smtp.domain=vger.kernel.org smtp.result=pass smtp_org.domain=kernel.org smtp_org.result=pass smtp_is_org_domain=no header.domain=intel.com header.result=pass header_is_org_domain=yes Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933036AbeCGEtK (ORCPT ); Tue, 6 Mar 2018 23:49:10 -0500 Received: from mga09.intel.com ([134.134.136.24]:1834 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753978AbeCGEtJ (ORCPT ); Tue, 6 Mar 2018 23:49:09 -0500 X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,434,1515484800"; d="scan'208";a="180544645" Date: Wed, 7 Mar 2018 12:39:20 +0800 From: Wu Hao To: Alan Tull Cc: Moritz Fischer , linux-fpga@vger.kernel.org, linux-kernel , linux-api@vger.kernel.org, "Kang, Luwei" , "Zhang, Yi Z" , Tim Whisonant , Enno Luebbers , Shiva Rao , Christopher Rauer , Xiao Guangrong Subject: Re: [PATCH v4 14/24] fpga: dfl: fme: add partial reconfiguration sub feature support Message-ID: <20180307043920.GA6907@hao-dev> References: <1518513893-4719-1-git-send-email-hao.wu@intel.com> <1518513893-4719-15-git-send-email-hao.wu@intel.com> <20180306020838.GB32057@hao-dev> 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-api-owner@vger.kernel.org X-Mailing-List: linux-api@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Tue, Mar 06, 2018 at 12:29:35PM -0600, Alan Tull wrote: > On Mon, Mar 5, 2018 at 8:08 PM, Wu Hao wrote: > > On Mon, Mar 05, 2018 at 04:46:02PM -0600, Alan Tull wrote: > >> On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao wrote: > >> > >> Hi Hao, > > > > Hi Alan, > > > > Thanks for the comments. > > > >> > >> We are going to want to be able use different FPGA managers with this > >> framework. The different manager may be part of a different FME in > >> fabric or it may be a hardware FPGA manager. Fortunately, at this > >> point now the changes, noted below, to get there are pretty small. > > > > Yes, understand these could be some cases that FME having different PR > > hardware. > > > > Or supporting reduced FME plus hardware-based FPGA manager. > > Just to re-emphasize, the basic intent of the FPGA manager subsystem > in the first place is to have FPGA managers separate from higher level > frameworks so that the higher level frameworks will be able to able to > use different FPGAs. > > >> > +/** > >> > + * fpga_fme_create_mgr - create fpga mgr platform device as child device > >> > + * > >> > + * @pdata: fme platform_device's pdata > >> > + * > >> > + * Return: mgr platform device if successful, and error code otherwise. > >> > + */ > >> > +static struct platform_device * > >> > +fpga_fme_create_mgr(struct feature_platform_data *pdata) > >> > +{ > >> > + struct platform_device *mgr, *fme = pdata->dev; > >> > + struct feature *feature; > >> > + struct resource res; > >> > + struct resource *pres; > >> > + int ret = -ENOMEM; > >> > + > >> > + feature = get_feature_by_id(&pdata->dev->dev, FME_FEATURE_ID_PR_MGMT); > >> > + if (!feature) > >> > + return ERR_PTR(-ENODEV); > >> > + > >> > + /* > >> > + * Each FME has only one fpga-mgr, so allocate platform device using > >> > + * the same FME platform device id. > >> > + */ > >> > + mgr = platform_device_alloc(FPGA_DFL_FME_MGR, fme->id); > >> > >> At this point, the framework is assuming all FME's include the same > >> FPGA manager device which would use the driver in dfl-fme-mgr.c. > >> > >> I'm thinking of two cases where the manager isn't the same as a > >> dfl-fme-mgr.c manager are a bit different: > >> > >> (1) a FME-based FPGA manager, but different implementation, different > >> registers. The constraint is that the port implementation has to be > >> similar enough to use the rest of the base FME code. I am wondering > >> if the FPGA manager can be added to the DFL. At this point, the DFL > >> would drive which FPGA manager is alloc'd. That way the user gets to > >> use all this code in dfl-fme-pr.c but with their FPGA manager. > > > > Actually I'm not sure how this will be implemented in the hardware in the > > future, but from my understanding, there may be several methods to add this > > support (a different PR hardware) to FME. > > > > 1) introduce a new PR sub feature to FME. > > driver knows it by different feature id, and create different fpga > > manager platform device, but may not be able to reuse dfl-fme-pr.c. > > What would prevent reusing dfl-fme-pr.c? It looks like this is 98% of > the way there and only needs a way of knowing which FPGA manager > driver to alloc here. I am hoping that a new PR sub feature could be > added and that dfl-fme-pr.c can be reused. It depeneds on how hardware implements it. : ) I agree that if it follows the same way of current PR sub feature, then we could reuse the dfl-fme-pr.c for sure. > > > > > 2) add a different PR hardware support to current PR sub feature. > > It requires hardware to add registers to indicate this is a different > > PR hardware than dfl-fme-mgr.c, and its register region information > > (e.g location and size). Then this dfl-fme-pr.c driver could read > > these information from hardware and create a different fpga manager > > platform device with correct resources. > > So this dfl-fme-pr.c would have to know where some ID registers are > and the enumeration gets messier. Some of the enumeration would be > DFL and some would be from information that is not in the DFL headers. > The DFL should contain the knowledge of which mgr to use. Actually I don't know how hardware will implement this in the future, but I just listed my ideas here. Per my understanding, driver (reuse dfl-fme-pr.c) needs some more information to decide which platform device to create (for fpga manager). 1) introduce a new PR sub feature. Then it has a different private feature id in DFH (Device Feature Header). driver could use this id to create a different platform device. 2) introduce some registers inside the current PR sub feature. Then driver could read these registers to know which platform device to create for fpga manager. I think either 1) or 2) will only require small changes to current driver code, I don't have any concern on supporting different PR hardware. :) I understand your concern on case 2), everybody who wants to reuse dfl-fme-pr.c needs to implement ID register which is not defined by DFL, so I guess we should suggest 1). > > > > > I think current driver framework could support both cases above for sure. > > > >> > >> (2) a FPGA manager that can be added by device tree in the case of a > >> platform that is using device tree. I think this will be pretty > >> simple and can be done later when someone is actually bringing this > >> framework up on a FPGA running under device tree. I'm thinking that > >> the base DFL device that reads the dfl data from hardware can have a > >> DT property that points to the FPGA manager. That manager can be > >> saved somewhere handy like the pdata and passed down to this code, > >> which realizes it can use that existing device and doesn't need to > >> alloc a platform device. But again, that's probably best done later. > > > > Sure, we can discuss further when we really need it. Actually per my > > understanding, if hardware describes itself clearly, we may not have to > > use DT for fpga manager, as driver could create it automatically based > > on information read from hardware. :) > > DT exists for busses that don't have that kind of discovery. For a > concrete example, consider how the Arria10 driver (socfpga-a10.c) > probe function is getting its two mmio spaces and clock. I see. If we have to create fpga mgr using DT some cases, it makes sense that we just link the existing one instead. > > > > >> > >> > + if (!mgr) > >> > + return ERR_PTR(ret); > >> > + > >> > + mgr->dev.parent = &fme->dev; > >> > + > >> > + pres = platform_get_resource(fme, IORESOURCE_MEM, > >> > + feature->resource_index); > >> > + if (!pres) { > >> > + ret = -ENODEV; > >> > + goto create_mgr_err; > >> > + } > >> > + > >> > + memset(&res, 0, sizeof(struct resource)); > >> > + > >> > + res.start = pres->start; > >> > + res.end = pres->end; > >> > + res.name = pres->name; > >> > + res.flags = IORESOURCE_MEM; > >> > + > >> > + ret = platform_device_add_resources(mgr, &res, 1); > >> > + if (ret) > >> > + goto create_mgr_err; > >> > + > >> > + ret = platform_device_add(mgr); > >> > + if (ret) > >> > + goto create_mgr_err; > >> > + > >> > + return mgr; > >> > + > >> > +create_mgr_err: > >> > + platform_device_put(mgr); > >> > + return ERR_PTR(ret); > >> > +} > > >> > diff --git a/drivers/fpga/dfl-fme-pr.h b/drivers/fpga/dfl-fme-pr.h > >> > new file mode 100644 > >> > index 0000000..11bd001 > >> > --- /dev/null > >> > +++ b/drivers/fpga/dfl-fme-pr.h > >> > @@ -0,0 +1,113 @@ > >> > +/* SPDX-License-Identifier: GPL-2.0 */ > >> > +/* > >> > + * Header file for FPGA Management Engine (FME) Partial Reconfiguration Driver > >> > + * > >> > + * Copyright (C) 2017 Intel Corporation, Inc. > >> > + * > >> > + * Authors: > >> > + * Kang Luwei > >> > + * Xiao Guangrong > >> > + * Wu Hao > >> > + * Joseph Grecco > >> > + * Enno Luebbers > >> > + * Tim Whisonant > >> > + * Ananda Ravuri > >> > + * Henry Mitchel > >> > + */ > >> > + > >> > +#ifndef __DFL_FME_PR_H > >> > +#define __DFL_FME_PR_H > >> > + > >> > +#include > >> > + > >> > +/** > >> > + * struct fme_region - FME fpga region data structure > >> > + * > >> > + * @region: platform device of the FPGA region. > >> > + * @node: used to link fme_region to a list. > >> > + * @port_id: indicate which port this region connected to. > >> > + */ > >> > +struct fme_region { > >> > + struct platform_device *region; > >> > + struct list_head node; > >> > + int port_id; > >> > +}; > >> > + > >> > +/** > >> > + * struct fme_region_pdata - platform data for FME region platform device. > >> > + * > >> > + * @mgr: platform device of the FPGA manager. > >> > + * @br: platform device of the FPGA bridge. > >> > + * @region_id: region id (same as port_id). > >> > + */ > >> > +struct fme_region_pdata { > >> > + struct platform_device *mgr; > >> > + struct platform_device *br; > >> > + int region_id; > >> > +}; > >> > + > >> > +/** > >> > + * struct fme_bridge - FME fpga bridge data structure > >> > + * > >> > + * @br: platform device of the FPGA bridge. > >> > + * @node: used to link fme_bridge to a list. > >> > + */ > >> > +struct fme_bridge { > >> > + struct platform_device *br; > >> > + struct list_head node; > >> > +}; > >> > + > >> > +/** > >> > + * struct fme_bridge_pdata - platform data for FME bridge platform device. > >> > + * > >> > + * @port: platform device of the port feature dev. > >> > + */ > >> > +struct fme_br_pdata { > >> > + struct platform_device *port; > >> > +}; > >> > + > >> > +#define FPGA_DFL_FME_MGR "dfl-fme-mgr" > >> > +#define FPGA_DFL_FME_BRIDGE "dfl-fme-bridge" > >> > +#define FPGA_DFL_FME_REGION "dfl-fme-region" > >> > + > >> > >> Everything in dfl-fme-pr.h up to this point is good and general and > >> should remain in dfl-fme-pr.h. The register #defines for this FME's > >> FPGA manager device below should be associated with the FPGA manager > >> driver. Sorry if the way I stated that in the v3 review wasn't clear. > > > > Actually I put the PR sub feature register set definitions in this header > > file (dfl-fme-pr.h), because it's possible the driver (dfl-fme-pr.c) of > > this PR sub feature access some of the registers in the future. e.g read > > some PR sub feature registers to create different fpga manager platform > > devices as I mentioned above. > > That sounds like a workaround. Since you're adding a new method of > enumeration, you should use that new method of enumeration to choose > which FPGA manager is being used. Otherwise we are ending up with > multi-level enumeration, i.e. look at the DFL and then look at a > specific register location in the device. > > > > > I have to say this is only future consideration, and in this dfl-fme-pr.c > > driver there is no code to access below registers currently. I can move all > > of them to dfl-fme-mgr.h or dfl-fme-mgr.c in the next version if this is > > preferred. : ) > > That sounds good. That makes the mgr driver its own separate thing > which is what is supposed to happen in this framework. Sure, will fix this. Thanks Hao > > > > >> > >> > +/* FME Partial Reconfiguration Sub Feature Register Set */ > >> > +#define FME_PR_DFH 0x0 > >> > +#define FME_PR_CTRL 0x8 > >> > +#define FME_PR_STS 0x10 > >> > +#define FME_PR_DATA 0x18 > >> > +#define FME_PR_ERR 0x20 > >> > +#define FME_PR_INTFC_ID_H 0xA8 > >> > +#define FME_PR_INTFC_ID_L 0xB0 > >> > + > >> > +/* FME PR Control Register Bitfield */ > >> > +#define FME_PR_CTRL_PR_RST BIT(0) /* Reset PR engine */ > >> > +#define FME_PR_CTRL_PR_RSTACK BIT(4) /* Ack for PR engine reset */ > >> > +#define FME_PR_CTRL_PR_RGN_ID GENMASK_ULL(9, 7) /* PR Region ID */ > >> > +#define FME_PR_CTRL_PR_START BIT(12) /* Start to request for PR service */ > >> > +#define FME_PR_CTRL_PR_COMPLETE BIT(13) /* PR data push complete notification */ > >> > + > >> > +/* FME PR Status Register Bitfield */ > >> > +/* Number of available entries in HW queue inside the PR engine. */ > >> > +#define FME_PR_STS_PR_CREDIT GENMASK_ULL(8, 0) > >> > +#define FME_PR_STS_PR_STS BIT(16) /* PR operation status */ > >> > +#define FME_PR_STS_PR_STS_IDLE 0 > >> > +#define FME_PR_STS_PR_CTRLR_STS GENMASK_ULL(22, 20) /* Controller status */ > >> > +#define FME_PR_STS_PR_HOST_STS GENMASK_ULL(27, 24) /* PR host status */ > >> > + > >> > +/* FME PR Data Register Bitfield */ > >> > +/* PR data from the raw-binary file. */ > >> > +#define FME_PR_DATA_PR_DATA_RAW GENMASK_ULL(32, 0) > >> > + > >> > +/* FME PR Error Register */ > >> > +/* PR Operation errors detected. */ > >> > +#define FME_PR_ERR_OPERATION_ERR BIT(0) > >> > +/* CRC error detected. */ > >> > +#define FME_PR_ERR_CRC_ERR BIT(1) > >> > +/* Incompatible PR bitstream detected. */ > >> > +#define FME_PR_ERR_INCOMPATIBLE_BS BIT(2) > >> > +/* PR data push protocol violated. */ > >> > +#define FME_PR_ERR_PROTOCOL_ERR BIT(3) > >> > +/* PR data fifo overflow error detected */ > >> > +#define FME_PR_ERR_FIFO_OVERFLOW BIT(4) > >> > >> This stuff is specific to this FPGA manager device, so it should > >> either be in the dfl-fme-mgr.c or in a dfl-fme-mgr.h > > > > same as above, I can fix this in the next version. > > > > Thanks > > Hao > > Thanks, > Alan