Message ID | 20240122172433.537525-1-matthew.gerlach@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-33806-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:805:b0:101:a8e8:374 with SMTP id e5csp12191dyi; Mon, 22 Jan 2024 13:26:15 -0800 (PST) X-Google-Smtp-Source: AGHT+IGrJGyviNhfT2o6MrQnNkn8jGee5aPqD3e2bXYQmLIukcwTZ0+z5/DWE0aesXJUKNKs0DCV X-Received: by 2002:a17:907:760e:b0:a2c:2437:101c with SMTP id jx14-20020a170907760e00b00a2c2437101cmr2377747ejc.13.1705958775705; Mon, 22 Jan 2024 13:26:15 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1705958775; cv=pass; d=google.com; s=arc-20160816; b=wntqegaIpoMMwBOhXWjc55nS30O5cE2ZmXMnq1f/9MaLRRA9cHunZA12P8SSbZiScR CsxHzSF0e1wxHCsNQ7quAdnF8UJJ6oQlE1Bj82x1/DZRyUWqeYzycHHcWmFGJjjVs5Z6 h7EXXRY0jIjwmWvXeroCQ0CtBnYEsEQsBjz4Q22NHKSGH5Iemath2MCDTjP1iSqu0Zso cOf310aJ55qlyMHu8W4R4Z38CrrdJ6Np/iCZqQOAAuV8q1ESpbanOOsKzW6r+JdZXO6H 5bORHH1z4iuqQgojb/3C12Ej1yepVZWQXgmKMklTDKgnFQEDyNGmZjk2iMo2BY3i3B8k XB7g== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=y4VFiwtw9rxDA2XrtJsXUpAuybVxqKTG4u6ahEZl2r8=; fh=BQSex8sXkxkjVEn1pKefhBTZBbfExckE2Rh7vRJdeEs=; b=Z6BcqOquqWvOEBB7+bw1F0u/op13Jl1k6KkqMT6VGIiSL4l3swvL4dogs8qAO/0Hyd TMp8c2Q3tYorPCL5Lx4OB64HzmW4oMQlHKsiVULp6wWCDJqg/6A4dloo4d9SOuMJeA18 gzmPoqIy8KoSAqshn+1wdYig2QxnnjJWA77Y3ZWOWVaVdoWH/tzMLEzoaG646YjRJMf0 mvH5lQcpBOkvcSFYcnH9RR2JCETjU2ueF8Dln65qplMBlpDRrJV0dl8b9bOZsMt9VEwr idYW3wX1c6bx2POfcx1Mm8lpzCtTt7TIG1ttmi7UlEGoueiUeluCBNpL3Po9VpEQ88Cs VtgA== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=mVwJlG54; arc=pass (i=1 dkim=pass dkdomain=intel.com dmarc=pass fromdomain=linux.intel.com); spf=pass (google.com: domain of linux-kernel+bounces-33806-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-33806-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id l24-20020a170906a41800b00a2d06893ab2si9603199ejz.291.2024.01.22.13.26.15 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 22 Jan 2024 13:26:15 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-33806-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=mVwJlG54; arc=pass (i=1 dkim=pass dkdomain=intel.com dmarc=pass fromdomain=linux.intel.com); spf=pass (google.com: domain of linux-kernel+bounces-33806-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-33806-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 376951F2EF0D for <ouuuleilei@gmail.com>; Mon, 22 Jan 2024 18:02:38 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 7F65E6168E; Mon, 22 Jan 2024 17:24:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="mVwJlG54" Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B5841612FE; Mon, 22 Jan 2024 17:24:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.15 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705944291; cv=none; b=ISbi4beL71C3q6xTOOmE7RNb2XReoHyzHoa4v8eaxJFY0brX8zBS8p92G0erK5XTK/dBo8PEaYEdwC5Wkj7gsShBYHnFiMyXD2yVvfuoq/IG1vDfwwKRtrNLIdqjzZfjUxBiLQUpMBR9Hqpvza4ZzvQw2KyyWkdFYAf9BZRnhmM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705944291; c=relaxed/simple; bh=NPdtl0n426XRja32Jxqcz/56e80Ebxi5CwNkSRmx1Zk=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version; b=STcdt+5KNPNXEyjsYHpl1amxBvm9LolurrskT8wlNCyVZhxvrAjDZhCJFJndURHfokqADYgAZqr1U6wWC0mkWaYAk0MWg+ABfeMm4vvCmYLpmHNVht+C6B1Bk2zJoWtODlnOdij9db+MLzn1gPb/xLJjj6qzGRKWQm2tKqSYGZE= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=none smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=mVwJlG54; arc=none smtp.client-ip=198.175.65.15 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1705944290; x=1737480290; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=NPdtl0n426XRja32Jxqcz/56e80Ebxi5CwNkSRmx1Zk=; b=mVwJlG543WZag8bwwyjTtnkcvC+0fE3F2BBjU2dBSxnSFQBFD6f+p4JA w3szMwh24AmgNrd6n3ARBgfnAygXk05pqXjOJj/hXwxeQQRi2XNLPLoSm 3bQkfT4TmivPI8BGk/1pbUAFTgvmoFtop4eGUU1f/phbC9BDWBBlzj0Ds gj4QtJ5JiNBkCWhfXLYM2ux8I4vkm8xCsPz7kFxRtDOa5ceta8OFZ8tbl 1ia7TU4N2qt1TtWY/Eo//YC6YNnvz8FWy2CWRQV7UiA2dNEvOXsjqEUuM 5dpPvw1eRO8Yxv+OYAVpv7VGPeptzd0nY1BAV4Is7Zt3z5hFSXqzMYoTC Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10961"; a="1150981" X-IronPort-AV: E=Sophos;i="6.05,211,1701158400"; d="scan'208";a="1150981" Received: from orviesa005.jf.intel.com ([10.64.159.145]) by orvoesa107.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jan 2024 09:24:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.05,211,1701158400"; d="scan'208";a="1263457" Received: from sj-2308-osc3.sj.intel.com ([10.233.115.64]) by orviesa005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jan 2024 09:24:49 -0800 From: Matthew Gerlach <matthew.gerlach@linux.intel.com> To: hao.wu@intel.com, trix@redhat.com, mdf@kernel.org, yilun.xu@intel.com, linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Matthew Gerlach <matthew.gerlach@linux.intel.com> Subject: [PATCH] fpga: dfl: afu: update initialization of port_hdr driver Date: Mon, 22 Jan 2024 09:24:33 -0800 Message-Id: <20240122172433.537525-1-matthew.gerlach@linux.intel.com> X-Mailer: git-send-email 2.34.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788827428938905588 X-GMAIL-MSGID: 1788827428938905588 |
Series |
fpga: dfl: afu: update initialization of port_hdr driver
|
|
Commit Message
Matthew Gerlach
Jan. 22, 2024, 5:24 p.m. UTC
Revision 2 of the Device Feature List (DFL) Port feature has
slightly different requirements than revision 1. Revision 2
does not need the port to reset at driver startup. In fact,
performing a port reset during driver initialization can cause
driver race conditions when the port is connected to a different
PCIe Physical Function (PF) than the management PF performing
the actual port reset.
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
drivers/fpga/dfl-afu-main.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
Comments
On Mon, Jan 22, 2024 at 09:24:33AM -0800, Matthew Gerlach wrote: > Revision 2 of the Device Feature List (DFL) Port feature has > slightly different requirements than revision 1. Revision 2 > does not need the port to reset at driver startup. In fact, Please help illustrate what's the difference between Revision 1 & 2, and why revision 2 needs not. > performing a port reset during driver initialization can cause > driver race conditions when the port is connected to a different Please reorganize this part, in this description there seems be a software racing bug and the patch is a workaround. But the fact is port reset shouldn't been done for a new HW. BTW: Is there a way to tell whether the port is connected to a different PF? Any guarantee that revision 3, 4 ... would need a port reset or not? Thanks, Yilun > PCIe Physical Function (PF) than the management PF performing > the actual port reset. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > drivers/fpga/dfl-afu-main.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c > index c0a75ca360d6..7d7f80cd264f 100644 > --- a/drivers/fpga/dfl-afu-main.c > +++ b/drivers/fpga/dfl-afu-main.c > @@ -417,7 +417,18 @@ static const struct attribute_group port_hdr_group = { > static int port_hdr_init(struct platform_device *pdev, > struct dfl_feature *feature) > { > - port_reset(pdev); > + void __iomem *base; > + u8 rev; > + > + base = dfl_get_feature_ioaddr_by_id(&pdev->dev, PORT_FEATURE_ID_HEADER); > + > + rev = dfl_feature_revision(base); > + > + if (rev < 2) > + port_reset(pdev); > + > + if (rev > 2) > + dev_info(&pdev->dev, "unexpected port feature revision, %u\n", rev); Remove the print. It is indicating an error but the function returns OK. Thanks, Yilun > > return 0; > } > -- > 2.34.1 > >
On Tue, 23 Jan 2024, Xu Yilun wrote: > On Mon, Jan 22, 2024 at 09:24:33AM -0800, Matthew Gerlach wrote: >> Revision 2 of the Device Feature List (DFL) Port feature has >> slightly different requirements than revision 1. Revision 2 >> does not need the port to reset at driver startup. In fact, > > Please help illustrate what's the difference between Revision 1 & 2, and > why revision 2 needs not. I will update the commit message to clarify the differences between revision 1 and 2. > >> performing a port reset during driver initialization can cause >> driver race conditions when the port is connected to a different > > Please reorganize this part, in this description there seems be a > software racing bug and the patch is a workaround. But the fact is port > reset shouldn't been done for a new HW. Reorganizing the commit message a bit will help to clarify why port reset should not be performed during driver initialization with revision 2 of the hardware. > > BTW: Is there a way to tell whether the port is connected to a different > PF? Any guarantee that revision 3, 4 ... would need a port reset or not? The use of revision 2 of the port_hdr IP block indicates that the port can be connected multiple PFs, but there is nothing explicitly stating which PFs the port is connected to. It is hard to predict the requirements and implementation of a future revision of an IP block. If a requirement of a future revision is to work with existing software, then the future revision would not require a port reset at driver initialization. > > Thanks, > Yilun > >> PCIe Physical Function (PF) than the management PF performing >> the actual port reset. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> drivers/fpga/dfl-afu-main.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c >> index c0a75ca360d6..7d7f80cd264f 100644 >> --- a/drivers/fpga/dfl-afu-main.c >> +++ b/drivers/fpga/dfl-afu-main.c >> @@ -417,7 +417,18 @@ static const struct attribute_group port_hdr_group = { >> static int port_hdr_init(struct platform_device *pdev, >> struct dfl_feature *feature) >> { >> - port_reset(pdev); >> + void __iomem *base; >> + u8 rev; >> + >> + base = dfl_get_feature_ioaddr_by_id(&pdev->dev, PORT_FEATURE_ID_HEADER); >> + >> + rev = dfl_feature_revision(base); >> + >> + if (rev < 2) >> + port_reset(pdev); >> + >> + if (rev > 2) >> + dev_info(&pdev->dev, "unexpected port feature revision, %u\n", rev); > > Remove the print. It is indicating an error but the function returns OK. The message is intended to be informational, but I'll remove it because it could be confusing. > > Thanks, > Yilun Thanks for the feedback. > >> >> return 0; >> } >> -- >> 2.34.1 >> >> >
On Wed, Jan 24, 2024 at 11:40:05AM -0800, matthew.gerlach@linux.intel.com wrote: > > > On Tue, 23 Jan 2024, Xu Yilun wrote: > > > On Mon, Jan 22, 2024 at 09:24:33AM -0800, Matthew Gerlach wrote: > > > Revision 2 of the Device Feature List (DFL) Port feature has > > > slightly different requirements than revision 1. Revision 2 > > > does not need the port to reset at driver startup. In fact, > > > > Please help illustrate what's the difference between Revision 1 & 2, and > > why revision 2 needs not. > > I will update the commit message to clarify the differences between revision > 1 and 2. > > > > > > performing a port reset during driver initialization can cause > > > driver race conditions when the port is connected to a different > > > > Please reorganize this part, in this description there seems be a > > software racing bug and the patch is a workaround. But the fact is port > > reset shouldn't been done for a new HW. > > Reorganizing the commit message a bit will help to clarify why port reset > should not be performed during driver initialization with revision 2 of the > hardware. > > > > > BTW: Is there a way to tell whether the port is connected to a different > > PF? Any guarantee that revision 3, 4 ... would need a port reset or not? > > The use of revision 2 of the port_hdr IP block indicates that the port can > be connected multiple PFs, but there is nothing explicitly stating which PFs Sorry, I mean any specific indicator other than enumerate the revision number? As you said below, checking revision number may not make further things right, then you need to amend code each time. Thanks, Yilun > the port is connected to. > > It is hard to predict the requirements and implementation of a future > revision of an IP block. If a requirement of a future revision is to work > with existing software, then the future revision would not require a port > reset at driver initialization. >
On Tue, 30 Jan 2024, Xu Yilun wrote: > On Wed, Jan 24, 2024 at 11:40:05AM -0800, matthew.gerlach@linux.intel.com wrote: >> >> >> On Tue, 23 Jan 2024, Xu Yilun wrote: >> >>> On Mon, Jan 22, 2024 at 09:24:33AM -0800, Matthew Gerlach wrote: >>>> Revision 2 of the Device Feature List (DFL) Port feature has >>>> slightly different requirements than revision 1. Revision 2 >>>> does not need the port to reset at driver startup. In fact, >>> >>> Please help illustrate what's the difference between Revision 1 & 2, and >>> why revision 2 needs not. >> >> I will update the commit message to clarify the differences between revision >> 1 and 2. >> >>> >>>> performing a port reset during driver initialization can cause >>>> driver race conditions when the port is connected to a different >>> >>> Please reorganize this part, in this description there seems be a >>> software racing bug and the patch is a workaround. But the fact is port >>> reset shouldn't been done for a new HW. >> >> Reorganizing the commit message a bit will help to clarify why port reset >> should not be performed during driver initialization with revision 2 of the >> hardware. >> >>> >>> BTW: Is there a way to tell whether the port is connected to a different >>> PF? Any guarantee that revision 3, 4 ... would need a port reset or not? >> >> The use of revision 2 of the port_hdr IP block indicates that the port can >> be connected multiple PFs, but there is nothing explicitly stating which PFs > > Sorry, I mean any specific indicator other than enumerate the revision > number? As you said below, checking revision number may not make further > things right, then you need to amend code each time. Using a revision number to indicate the level of functionality for a particular IP block seems to be a widely used approach. What other indicator of functionality level did you have in mind? The revision number of an IP block would change when new functionality is added to an IP block or the behavior of the IP block changes. It would be expected that SW might need to change in order to use the new functionality or to handle the change in behavior of the IP block. Ideally the new revision of an IP block would be compatible with existing SW, but that cannot be guaranteed. Thanks, Matthew > > Thanks, > Yilun > >> the port is connected to. >> >> It is hard to predict the requirements and implementation of a future >> revision of an IP block. If a requirement of a future revision is to work >> with existing software, then the future revision would not require a port >> reset at driver initialization. >> >
On Tue, Jan 30, 2024 at 09:13:56AM -0800, matthew.gerlach@linux.intel.com wrote: > > > On Tue, 30 Jan 2024, Xu Yilun wrote: > > > On Wed, Jan 24, 2024 at 11:40:05AM -0800, matthew.gerlach@linux.intel.com wrote: > > > > > > > > > On Tue, 23 Jan 2024, Xu Yilun wrote: > > > > > > > On Mon, Jan 22, 2024 at 09:24:33AM -0800, Matthew Gerlach wrote: > > > > > Revision 2 of the Device Feature List (DFL) Port feature has > > > > > slightly different requirements than revision 1. Revision 2 > > > > > does not need the port to reset at driver startup. In fact, > > > > > > > > Please help illustrate what's the difference between Revision 1 & 2, and > > > > why revision 2 needs not. > > > > > > I will update the commit message to clarify the differences between revision > > > 1 and 2. > > > > > > > > > > > > performing a port reset during driver initialization can cause > > > > > driver race conditions when the port is connected to a different > > > > > > > > Please reorganize this part, in this description there seems be a > > > > software racing bug and the patch is a workaround. But the fact is port > > > > reset shouldn't been done for a new HW. > > > > > > Reorganizing the commit message a bit will help to clarify why port reset > > > should not be performed during driver initialization with revision 2 of the > > > hardware. > > > > > > > > > > > BTW: Is there a way to tell whether the port is connected to a different > > > > PF? Any guarantee that revision 3, 4 ... would need a port reset or not? > > > > > > The use of revision 2 of the port_hdr IP block indicates that the port can > > > be connected multiple PFs, but there is nothing explicitly stating which PFs > > > > Sorry, I mean any specific indicator other than enumerate the revision > > number? As you said below, checking revision number may not make further > > things right, then you need to amend code each time. > > Using a revision number to indicate the level of functionality for a > particular IP block seems to be a widely used approach. What other indicator If you still want to make the existing driver work, some capability indication would have more compatibility. That's more reasonable approach. Or you need to change existing behavior for each new revision, that's not actually widely used. > of functionality level did you have in mind? I'm not trying to make the design. You tell me. If finally no indicator could be used, we have to use revision number. That's OK but make SW work harder, so I'm asking if anything could be done to avoid that. > > The revision number of an IP block would change when new functionality is > added to an IP block or the behavior of the IP block changes. It would be > expected that SW might need to change in order to use the new functionality > or to handle the change in behavior of the IP block. Ideally the new > revision of an IP block would be compatible with existing SW, but that > cannot be guaranteed. People make the IP block, and be compatible should be the concern if it want upstream support. Thanks, Yilun > > Thanks, > Matthew > > > > > Thanks, > > Yilun > > > > > the port is connected to. > > > > > > It is hard to predict the requirements and implementation of a future > > > revision of an IP block. If a requirement of a future revision is to work > > > with existing software, then the future revision would not require a port > > > reset at driver initialization. > > > > >
On Wed, 31 Jan 2024, Xu Yilun wrote: > On Tue, Jan 30, 2024 at 09:13:56AM -0800, matthew.gerlach@linux.intel.com wrote: >> >> >> On Tue, 30 Jan 2024, Xu Yilun wrote: >> >>> On Wed, Jan 24, 2024 at 11:40:05AM -0800, matthew.gerlach@linux.intel.com wrote: >>>> >>>> >>>> On Tue, 23 Jan 2024, Xu Yilun wrote: >>>> >>>>> On Mon, Jan 22, 2024 at 09:24:33AM -0800, Matthew Gerlach wrote: >>>>>> Revision 2 of the Device Feature List (DFL) Port feature has >>>>>> slightly different requirements than revision 1. Revision 2 >>>>>> does not need the port to reset at driver startup. In fact, >>>>> >>>>> Please help illustrate what's the difference between Revision 1 & 2, and >>>>> why revision 2 needs not. >>>> >>>> I will update the commit message to clarify the differences between revision >>>> 1 and 2. >>>> >>>>> >>>>>> performing a port reset during driver initialization can cause >>>>>> driver race conditions when the port is connected to a different >>>>> >>>>> Please reorganize this part, in this description there seems be a >>>>> software racing bug and the patch is a workaround. But the fact is port >>>>> reset shouldn't been done for a new HW. >>>> >>>> Reorganizing the commit message a bit will help to clarify why port reset >>>> should not be performed during driver initialization with revision 2 of the >>>> hardware. >>>> >>>>> >>>>> BTW: Is there a way to tell whether the port is connected to a different >>>>> PF? Any guarantee that revision 3, 4 ... would need a port reset or not? >>>> >>>> The use of revision 2 of the port_hdr IP block indicates that the port can >>>> be connected multiple PFs, but there is nothing explicitly stating which PFs >>> >>> Sorry, I mean any specific indicator other than enumerate the revision >>> number? As you said below, checking revision number may not make further >>> things right, then you need to amend code each time. >> >> Using a revision number to indicate the level of functionality for a >> particular IP block seems to be a widely used approach. What other indicator > > If you still want to make the existing driver work, some capability indication > would have more compatibility. That's more reasonable approach. Or you > need to change existing behavior for each new revision, that's not > actually widely used. I understand some capability indication would be better for compatibility implementation. A revision number change is not as explicit or precise as capability lists. > >> of functionality level did you have in mind? > > I'm not trying to make the design. You tell me. One could use parameter blocks introduced in version 1 of the Device Feature Header (DFH), or capability registers could be added the IP block. In this particular case it seems the least impact to upstreamed software is to keep the DFH and the register map unchanged, except for an incremented revision number field. > > If finally no indicator could be used, we have to use revision number. That's > OK but make SW work harder, so I'm asking if anything could be done to > avoid that. In this case, I don't think anything else can be done without bigger impacts to the SW. > >> >> The revision number of an IP block would change when new functionality is >> added to an IP block or the behavior of the IP block changes. It would be >> expected that SW might need to change in order to use the new functionality >> or to handle the change in behavior of the IP block. Ideally the new >> revision of an IP block would be compatible with existing SW, but that >> cannot be guaranteed. > > People make the IP block, and be compatible should be the concern if it > want upstream support. Agreed, and making sure some capability mechanism exists when an IP is created would be a great start. Thanks, Matthew > > Thanks, > Yilun > >> >> Thanks, >> Matthew >> >>> >>> Thanks, >>> Yilun >>> >>>> the port is connected to. >>>> >>>> It is hard to predict the requirements and implementation of a future >>>> revision of an IP block. If a requirement of a future revision is to work >>>> with existing software, then the future revision would not require a port >>>> reset at driver initialization. >>>> >>> > >
On Wed, Jan 31, 2024 at 03:53:23PM -0800, matthew.gerlach@linux.intel.com wrote: > > > On Wed, 31 Jan 2024, Xu Yilun wrote: > > > On Tue, Jan 30, 2024 at 09:13:56AM -0800, matthew.gerlach@linux.intel.com wrote: > > > > > > > > > On Tue, 30 Jan 2024, Xu Yilun wrote: > > > > > > > On Wed, Jan 24, 2024 at 11:40:05AM -0800, matthew.gerlach@linux.intel.com wrote: > > > > > > > > > > > > > > > On Tue, 23 Jan 2024, Xu Yilun wrote: > > > > > > > > > > > On Mon, Jan 22, 2024 at 09:24:33AM -0800, Matthew Gerlach wrote: > > > > > > > Revision 2 of the Device Feature List (DFL) Port feature has > > > > > > > slightly different requirements than revision 1. Revision 2 > > > > > > > does not need the port to reset at driver startup. In fact, > > > > > > > > > > > > Please help illustrate what's the difference between Revision 1 & 2, and > > > > > > why revision 2 needs not. > > > > > > > > > > I will update the commit message to clarify the differences between revision > > > > > 1 and 2. > > > > > > > > > > > > > > > > > > performing a port reset during driver initialization can cause > > > > > > > driver race conditions when the port is connected to a different > > > > > > > > > > > > Please reorganize this part, in this description there seems be a > > > > > > software racing bug and the patch is a workaround. But the fact is port > > > > > > reset shouldn't been done for a new HW. > > > > > > > > > > Reorganizing the commit message a bit will help to clarify why port reset > > > > > should not be performed during driver initialization with revision 2 of the > > > > > hardware. > > > > > > > > > > > > > > > > > BTW: Is there a way to tell whether the port is connected to a different > > > > > > PF? Any guarantee that revision 3, 4 ... would need a port reset or not? > > > > > > > > > > The use of revision 2 of the port_hdr IP block indicates that the port can > > > > > be connected multiple PFs, but there is nothing explicitly stating which PFs > > > > > > > > Sorry, I mean any specific indicator other than enumerate the revision > > > > number? As you said below, checking revision number may not make further > > > > things right, then you need to amend code each time. > > > > > > Using a revision number to indicate the level of functionality for a > > > particular IP block seems to be a widely used approach. What other indicator > > > > If you still want to make the existing driver work, some capability indication > > would have more compatibility. That's more reasonable approach. Or you > > need to change existing behavior for each new revision, that's not > > actually widely used. > > I understand some capability indication would be better for compatibility > implementation. A revision number change is not as explicit or precise as > capability lists. > > > > > > of functionality level did you have in mind? > > > > I'm not trying to make the design. You tell me. > > One could use parameter blocks introduced in version 1 of the Device Feature > Header (DFH), or capability registers could be added the IP block. > In this particular case it seems the least impact to upstreamed software is > to keep the DFH and the register map unchanged, except for an incremented > revision number field. > > > > > If finally no indicator could be used, we have to use revision number. That's > > OK but make SW work harder, so I'm asking if anything could be done to > > avoid that. > > In this case, I don't think anything else can be done without bigger impacts > to the SW. Changing the existing SW is not a problem, repeat the same change every time is a problem. So if we make sure port reset is no longer needed after version 1, then this patch is OK. Otherwise, please re-evaluate. Thanks, Yilun > > > > > > > > > The revision number of an IP block would change when new functionality is > > > added to an IP block or the behavior of the IP block changes. It would be > > > expected that SW might need to change in order to use the new functionality > > > or to handle the change in behavior of the IP block. Ideally the new > > > revision of an IP block would be compatible with existing SW, but that > > > cannot be guaranteed. > > > > People make the IP block, and be compatible should be the concern if it > > want upstream support. > > Agreed, and making sure some capability mechanism exists when an IP is > created would be a great start. > > Thanks, > Matthew > > > > > Thanks, > > Yilun > > > > > > > > Thanks, > > > Matthew > > > > > > > > > > > Thanks, > > > > Yilun > > > > > > > > > the port is connected to. > > > > > > > > > > It is hard to predict the requirements and implementation of a future > > > > > revision of an IP block. If a requirement of a future revision is to work > > > > > with existing software, then the future revision would not require a port > > > > > reset at driver initialization. > > > > > > > > > > > > >
On Mon, 5 Feb 2024, Xu Yilun wrote: > On Wed, Jan 31, 2024 at 03:53:23PM -0800, matthew.gerlach@linux.intel.com wrote: >> >> >> On Wed, 31 Jan 2024, Xu Yilun wrote: >> >>> On Tue, Jan 30, 2024 at 09:13:56AM -0800, matthew.gerlach@linux.intel.com wrote: >>>> >>>> >>>> On Tue, 30 Jan 2024, Xu Yilun wrote: >>>> >>>>> On Wed, Jan 24, 2024 at 11:40:05AM -0800, matthew.gerlach@linux.intel.com wrote: >>>>>> >>>>>> >>>>>> On Tue, 23 Jan 2024, Xu Yilun wrote: >>>>>> >>>>>>> On Mon, Jan 22, 2024 at 09:24:33AM -0800, Matthew Gerlach wrote: >>>>>>>> Revision 2 of the Device Feature List (DFL) Port feature has >>>>>>>> slightly different requirements than revision 1. Revision 2 >>>>>>>> does not need the port to reset at driver startup. In fact, >>>>>>> >>>>>>> Please help illustrate what's the difference between Revision 1 & 2, and >>>>>>> why revision 2 needs not. >>>>>> >>>>>> I will update the commit message to clarify the differences between revision >>>>>> 1 and 2. >>>>>> >>>>>>> >>>>>>>> performing a port reset during driver initialization can cause >>>>>>>> driver race conditions when the port is connected to a different >>>>>>> >>>>>>> Please reorganize this part, in this description there seems be a >>>>>>> software racing bug and the patch is a workaround. But the fact is port >>>>>>> reset shouldn't been done for a new HW. >>>>>> >>>>>> Reorganizing the commit message a bit will help to clarify why port reset >>>>>> should not be performed during driver initialization with revision 2 of the >>>>>> hardware. >>>>>> >>>>>>> >>>>>>> BTW: Is there a way to tell whether the port is connected to a different >>>>>>> PF? Any guarantee that revision 3, 4 ... would need a port reset or not? >>>>>> >>>>>> The use of revision 2 of the port_hdr IP block indicates that the port can >>>>>> be connected multiple PFs, but there is nothing explicitly stating which PFs >>>>> >>>>> Sorry, I mean any specific indicator other than enumerate the revision >>>>> number? As you said below, checking revision number may not make further >>>>> things right, then you need to amend code each time. >>>> >>>> Using a revision number to indicate the level of functionality for a >>>> particular IP block seems to be a widely used approach. What other indicator >>> >>> If you still want to make the existing driver work, some capability indication >>> would have more compatibility. That's more reasonable approach. Or you >>> need to change existing behavior for each new revision, that's not >>> actually widely used. >> >> I understand some capability indication would be better for compatibility >> implementation. A revision number change is not as explicit or precise as >> capability lists. >> >>> >>>> of functionality level did you have in mind? >>> >>> I'm not trying to make the design. You tell me. >> >> One could use parameter blocks introduced in version 1 of the Device Feature >> Header (DFH), or capability registers could be added the IP block. >> In this particular case it seems the least impact to upstreamed software is >> to keep the DFH and the register map unchanged, except for an incremented >> revision number field. >> >>> >>> If finally no indicator could be used, we have to use revision number. That's >>> OK but make SW work harder, so I'm asking if anything could be done to >>> avoid that. >> >> In this case, I don't think anything else can be done without bigger impacts >> to the SW. > > Changing the existing SW is not a problem, repeat the same change every time > is a problem. So if we make sure port reset is no longer needed after > version 1, then this patch is OK. Otherwise, please re-evaluate. The initial port reset will no longer be after version 1. The requirement for the initial state of the logic in side the port to be valid will remain. Thanks, Matthew > > Thanks, > Yilun > >> >>> >>>> >>>> The revision number of an IP block would change when new functionality is >>>> added to an IP block or the behavior of the IP block changes. It would be >>>> expected that SW might need to change in order to use the new functionality >>>> or to handle the change in behavior of the IP block. Ideally the new >>>> revision of an IP block would be compatible with existing SW, but that >>>> cannot be guaranteed. >>> >>> People make the IP block, and be compatible should be the concern if it >>> want upstream support. >> >> Agreed, and making sure some capability mechanism exists when an IP is >> created would be a great start. >> >> Thanks, >> Matthew >> >>> >>> Thanks, >>> Yilun >>> >>>> >>>> Thanks, >>>> Matthew >>>> >>>>> >>>>> Thanks, >>>>> Yilun >>>>> >>>>>> the port is connected to. >>>>>> >>>>>> It is hard to predict the requirements and implementation of a future >>>>>> revision of an IP block. If a requirement of a future revision is to work >>>>>> with existing software, then the future revision would not require a port >>>>>> reset at driver initialization. >>>>>> >>>>> >>> >>> >
diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c index c0a75ca360d6..7d7f80cd264f 100644 --- a/drivers/fpga/dfl-afu-main.c +++ b/drivers/fpga/dfl-afu-main.c @@ -417,7 +417,18 @@ static const struct attribute_group port_hdr_group = { static int port_hdr_init(struct platform_device *pdev, struct dfl_feature *feature) { - port_reset(pdev); + void __iomem *base; + u8 rev; + + base = dfl_get_feature_ioaddr_by_id(&pdev->dev, PORT_FEATURE_ID_HEADER); + + rev = dfl_feature_revision(base); + + if (rev < 2) + port_reset(pdev); + + if (rev > 2) + dev_info(&pdev->dev, "unexpected port feature revision, %u\n", rev); return 0; }