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

[matthew . gerlach]

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