Received-SPF: pass (google.com: domain of linux-kernel+bounces-37602-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 24 Jan 2024 11:40:05 -0800 (PST)
From: matthew.gerlach@linux.intel.com
To: Xu Yilun <yilun.xu@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
In-Reply-To: <Za8ibeJc82Xkbpct@yilunxu-OptiPlex-7050>
Message-ID: <alpine.DEB.2.22.394.2401241106550.77559@sj-4150-psse-sw-opae-dev2>
References: <20240122172433.537525-1-matthew.gerlach@linux.intel.com> <Za8ibeJc82Xkbpct@yilunxu-OptiPlex-7050>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed


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