Received-SPF: pass (google.com: domain of linux-kernel+bounces-51998-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 5 Feb 2024 10:51:57 +0800
From: Xu Yilun <yilun.xu@linux.intel.com>
To: matthew.gerlach@linux.intel.com
Cc: hao.wu@intel.com, trix@redhat.com, mdf@kernel.org, yilun.xu@intel.com,
	linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fpga: dfl: afu: update initialization of port_hdr driver
Message-ID: <ZcBNTdOulDvlIxmY@yilunxu-OptiPlex-7050>
References: <20240122172433.537525-1-matthew.gerlach@linux.intel.com>
 <Za8ibeJc82Xkbpct@yilunxu-OptiPlex-7050>
 <alpine.DEB.2.22.394.2401241106550.77559@sj-4150-psse-sw-opae-dev2>
 <ZbjC501oRClByual@yilunxu-OptiPlex-7050>
 <alpine.DEB.2.22.394.2401300825020.112016@sj-4150-psse-sw-opae-dev2>
 <ZbnTwcomGXOGs9SG@yilunxu-OptiPlex-7050>
 <alpine.DEB.2.22.394.2401311433120.112016@sj-4150-psse-sw-opae-dev2>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.22.394.2401311433120.112016@sj-4150-psse-sw-opae-dev2>

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.
> > > > > 
> > > > 
> > 
> >