RE: [PATCH v2 1/1] fpga: dfl: afu: harden port enable logic

["Wu, Hao" <hao.wu@intel.com>]

> 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@intel.com>
> >>>>> Sent: Friday, September 18, 2020 2:32 AM
> >>>>> To: mdf@kernel.org; linux-fpga@vger.kernel.org; linux-
> >>>>> kernel@vger.kernel.org
> >>>>> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun
> <yilun.xu@intel.com>;
> >>>>> Wu, Hao <hao.wu@intel.com>; Gerlach, Matthew
> >>>>> <matthew.gerlach@intel.com>; Weight, Russell H
> >>>>> <russell.h.weight@intel.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.

Thanks for this confirmation, then it sounds good to me. I think only Intel
hardware is using this driver now, so if this is confirmed from hardware side,
then we should be safe to take this one.

Hao

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