Date:   Wed, 3 Feb 2021 15:07:17 -0800 (PST)
From:   matthew.gerlach@...ux.intel.com
To:     Russ Weight <russell.h.weight@...el.com>
cc:     "Wu, Hao" <hao.wu@...el.com>, "mdf@...nel.org" <mdf@...nel.org>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "trix@...hat.com" <trix@...hat.com>,
        "lgoncalv@...hat.com" <lgoncalv@...hat.com>,
        "Xu, Yilun" <yilun.xu@...el.com>,
        "Gerlach, Matthew" <matthew.gerlach@...el.com>
Subject: Re: [PATCH v2 1/1] fpga: dfl: afu: harden port enable logic



On Wed, 3 Feb 2021, Russ Weight wrote:

>
>
> On 2/3/21 1:28 AM, Wu, Hao wrote:
>>> Subject: Re: [PATCH v2 1/1] fpga: dfl: afu: harden port enable logic
>>>
>>> Sorry for the delay on this patch. It seemed like a lower priority patch than
>>> others, since we haven't seen any issues with current products. Please my
>>> responses inline.
>>>
>>> On 9/17/20 7:08 PM, Wu, Hao wrote:
>>>>> -----Original Message-----
>>>>> From: Russ Weight <russell.h.weight@...el.com>
>>>>> Sent: Friday, September 18, 2020 2:32 AM
>>>>> To: mdf@...nel.org; linux-fpga@...r.kernel.org; linux-
>>>>> kernel@...r.kernel.org
>>>>> Cc: trix@...hat.com; lgoncalv@...hat.com; Xu, Yilun <yilun.xu@...el.com>;
>>>>> Wu, Hao <hao.wu@...el.com>; Gerlach, Matthew
>>>>> <matthew.gerlach@...el.com>; Weight, Russell H
>>>>> <russell.h.weight@...el.com>
>>>>> Subject: [PATCH v2 1/1] fpga: dfl: afu: harden port enable logic
>>>>>
>>>>> Port enable is not complete until ACK = 0. Change
>>>>> __afu_port_enable() to guarantee that the enable process
>>>>> is complete by polling for ACK == 0.
>>>> The description of this port reset ack bit is
>>>>
>>>> " After initiating a Port soft reset, SW should monitor this bit. HW
>>>> will set this bit when all outstanding requests initiated by this port
>>>> have been drained, and the minimum soft reset pulse width has
>>>> elapsed. "
>>>>
>>>> But no description about what to do when clearing a Port soft reset
>>>> to enable the port.
>>>>
>>>> So we need to understand clearly on why we need this change
>>>> (e.g. what may happen without this change), and will it apply for all
>>>> existing DFL devices and future ones, or just for one specific card.
>>>> Could you please help? : )
>>> I touched bases with the hardware engineers. The recommendation to wait
>>> for ACK to be cleared is new with OFS and is documented in the latest
>>> OFS specification as follows (see step #4):
>>>
>>>> 3.7.1 AFU Soft Resets
>>>> Software may cause a soft reset to be issued to the AFU as follows:
>>>> 1. Assert the PortSoftReset field of the PORT_CONTROL register
>>>> 2. Wait for the Port to acknowledge the soft reset by monitoring the
>>>> PortSoftResetAck field of the PORT_CONTROL register, i.e.
>>> PortSoftResetAck=1
>>>> 3. Deasserting the PortSoftReset field
>>>> 4. Wait for the Port to acknowledge the soft reset de-assertion by monitoring
>>> the
>>>> PortSoftResetAck field of the PORT_CONTROL register, i.e.
>>> PortSoftResetAck=0
>>>> This sequence ensures that outstanding transactions are suitably flushed and
>>>> that the FIM minimum reset pulse width is respected. Failing to follow this
>>>> sequence leaves the AFU in an undefined state.
>>> The OFS specification has not been posted publicly, yet.
>>>
>>> Also, this is how it was explained to me:
>>>
>>>> In most scenario, port will be able to get out of reset soon enough
>>>> when SW releases the port reset, especially on all the PAC products
>>>> which have been verified before release.
>>>>
>>>> Polling for HW to clear the ACK is meant to handle the following scenarios:
>>>>
>>>>   * Different platform can take variable period of time to get out of reset
>>>>   * Bug in the HW that hold the port in reset
>>> So this change is not required for the currently released PAC cards,
>>> but it is needed for OFS based products. I don't think there is any reason
>>> to hold off on the patch, as it is still valid for current products.
>> As you know, this driver is used for different cards, and we need to make
>> sure new changes introduced in new version spec, don't break old products
>> as we are sharing the same driver. and we are not sure if in the future some
>> new products but still uses old specs, and then things may be broken if the
>> driver which always perform new flow. Another method is that introduce 1
>> bit in hardware register to tell the driver to perform the additional steps,
>> then it can avoid impacts to the old products. If this can't be done, then
>> we at least need to verify this change on all existing hardware and suggest
>> users to follow new spec only.
>
> According to the HW engineers, the RTL implementation has not changed; it is
> the same as the RTL in the current PAC products. Polling for HW to clear the
> ACK is something we could have (should have?) been doing all along. The timing

I also confirmed with HW engineers.  The original specification was 
not precise.  The code should have been doing this all along.

Matthew Gerlach

> hasn't been an issue for the current PAC products, as proven by our testing.
> However, with OFS we cannot anticipate what the timing will be for customer
> designed products, so the specification is calling out this requirement as a
> precaution.
>
> I am using a development machine that has the older PAC devices installed. I
> cleared port errors on these cards as a quick check, and the reset completes
> without hanging - which indicates that the ACK bit is in fact getting cleared.
> So there is not need for any device-specific conditional statements here.
>
> - Russ
>
>>
>> Hao
>
>

Powered by blists - more mailing lists