From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757221AbdLUA6p (ORCPT ); Wed, 20 Dec 2017 19:58:45 -0500 Received: from mail.kernel.org ([198.145.29.99]:33610 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755846AbdLUA6n (ORCPT ); Wed, 20 Dec 2017 19:58:43 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 11B4E2190A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=atull@kernel.org X-Google-Smtp-Source: ACJfBoufFfI50CSvSYo1x0M/KEBoTJ4/EWWlgT/zdZjcEMGaE6yqGLkXbtgh6TjwM7Y4yQuOnoXQzptuSGWCJSAjyII= MIME-Version: 1.0 In-Reply-To: References: <1511764948-20972-1-git-send-email-hao.wu@intel.com> <1511764948-20972-5-git-send-email-hao.wu@intel.com> From: Alan Tull Date: Wed, 20 Dec 2017 18:58:01 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 04/21] fpga: add device feature list support To: Wu Hao 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 Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 20, 2017 at 4:29 PM, Alan Tull wrote: > On Mon, Nov 27, 2017 at 12:42 AM, Wu Hao wrote: > > Hi Hao, > >> + >> +enum port_feature_id { >> + PORT_FEATURE_ID_HEADER = 0x0, >> + PORT_FEATURE_ID_ERROR = 0x1, >> + PORT_FEATURE_ID_UMSG = 0x2, >> + PORT_FEATURE_ID_PR = 0x3, >> + PORT_FEATURE_ID_STP = 0x4, >> + PORT_FEATURE_ID_UAFU = 0x5, >> + PORT_FEATURE_ID_MAX = 0x6, >> +}; >> + >> +#define FME_FEATURE_NUM FME_FEATURE_ID_MAX >> +#define PORT_FEATURE_NUM PORT_FEATURE_ID_MAX >> + >> +#define FPGA_FEATURE_DEV_FME "fpga-dfl-fme" >> +#define FPGA_FEATURE_DEV_PORT "fpga-dfl-port" >> + >> +static inline int feature_platform_data_size(const int num) >> +{ >> + return sizeof(struct feature_platform_data) + >> + num * sizeof(struct feature); >> +} >> + >> +int fpga_port_id(struct platform_device *pdev); >> + >> +static inline int fpga_port_check_id(struct platform_device *pdev, >> + void *pport_id) >> +{ >> + return fpga_port_id(pdev) == *(int *)pport_id; >> +} >> + >> +void __fpga_port_enable(struct platform_device *pdev); >> +int __fpga_port_disable(struct platform_device *pdev); >> + >> +static inline void fpga_port_enable(struct platform_device *pdev) >> +{ >> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev); >> + >> + mutex_lock(&pdata->lock); >> + __fpga_port_enable(pdev); >> + mutex_unlock(&pdata->lock); >> +} >> + >> +static inline int fpga_port_disable(struct platform_device *pdev) >> +{ >> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev); >> + int ret; >> + >> + mutex_lock(&pdata->lock); >> + ret = __fpga_port_disable(pdev); >> + mutex_unlock(&pdata->lock); >> + >> + return ret; >> +} >> + >> +static inline int __fpga_port_reset(struct platform_device *pdev) >> +{ >> + int ret; >> + >> + ret = __fpga_port_disable(pdev); >> + if (ret) >> + return ret; >> + >> + __fpga_port_enable(pdev); >> + >> + return 0; >> +} >> + >> +static inline int fpga_port_reset(struct platform_device *pdev) >> +{ >> + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev); >> + int ret; >> + >> + mutex_lock(&pdata->lock); >> + ret = __fpga_port_reset(pdev); >> + mutex_unlock(&pdata->lock); >> + >> + return ret; >> +} > > I see that the port code is included as part of the enumeration code. > This is not very future-proofed, if a different port needs to be > supported. > > The port is a FPGA fabric based bridge with expanded functionality, > right? So it is similar to the altera freeze bridge, but adds the > ability to reset the fabric and some other features are promised in > the future, IIUC. I still think that the port could be implemented in > the bridge driver .c file instead of being here as part of the > enumeration code. For that to happen, some APIs would need to be > added to the bridge framework and the FPGA region framework. Then the > reset can be requested through a new FPGA region API function. > > The advantage of this is that if this patchset evolves and there is > some other v2 port driver needed, it can be a different driver if it > needs to be. > > If the port reset is really a fabric reset, Actually 'fabric reset' is probably not clear enough. It's resetting the hardware in a partial reconfiguration region, not just resetting the bridge. I'm trying to come up with a term that makes that clear what is getting reset is the contents of the region. > (correct me if I'm > remembering wrongly) then it would be helpful to call it a > fabric_reset. This would be the first bridge driver supporting fabric > reset. I think it won't be the last. > > So what I'm proposing would be added/changed would be: > * move all the bridge code to fpga-dfl-fme-br.c > * add .fabric_reset to bridge ops > * add fpga_bridges_reset to fpga-bridge.c (a new function that goes > through a list of bridges and calls the reset ops if it exists, > ignores the bridges where it doesn't exist) > * add fpga_region_fabric_reset to fpga-region.c. This function gets > the region, gets the bridges, calls fpga_bridges_reset (can steal code > from fpga_region_program_fpga) > * the rest of the patchset can use fpga_region_fabric_reset instead of > fpga_port_reset > > Alan