Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: tC+LTUL2Q4y116HvgyNx2XTXsmnG45umzfQ3ge3jT4e1Jje9ow0yr1WRfLzJB5F+iv4kykB11s
 spRs/BiMMF6w==
IronPort-SDR: mE4Fgxi8Y2+l3L6Nm6Du/1Pbod5qsAxEXNgObThgAh2LUQSk/0saSHZT/WpnwTi43hadYSjnKQ
 cpHnk00j2fow==
From:   "Wu, Hao" <hao.wu@intel.com>
To:     "matthew.gerlach@linux.intel.com" <matthew.gerlach@linux.intel.com>,
        "Weight, Russell H" <russell.h.weight@intel.com>
CC:     "mdf@kernel.org" <mdf@kernel.org>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "trix@redhat.com" <trix@redhat.com>,
        "lgoncalv@redhat.com" <lgoncalv@redhat.com>,
        "Xu, Yilun" <yilun.xu@intel.com>,
        "Gerlach, Matthew" <matthew.gerlach@intel.com>
Subject: RE: [PATCH v2 1/1] fpga: dfl: afu: harden port enable logic
Thread-Topic: [PATCH v2 1/1] fpga: dfl: afu: harden port enable logic
Thread-Index: AQHWjSD4LeMXUYhU00yzPOyYE3T3saltlsAAgNiPhQCAANXVMIAA5ZOAgAAGsoCAACpaYA==
Date:   Thu, 4 Feb 2021 01:55:22 +0000
Message-ID: <DM6PR11MB38197E81243A179AED35F20C85B39@DM6PR11MB3819.namprd11.prod.outlook.com>
References: <20200917183219.3603-1-russell.h.weight@intel.com>
 <DM6PR11MB38194BD85854B598F1CD97C5853F0@DM6PR11MB3819.namprd11.prod.outlook.com>
 <8ab0e288-97f0-d167-50f0-624e05d77944@intel.com>
 <DM6PR11MB3819BC4BFE16A9CBE185EB1185B49@DM6PR11MB3819.namprd11.prod.outlook.com>
 <25ada056-e591-4a6d-2e0e-704b099d00bf@intel.com>
 <alpine.DEB.2.22.394.2102031504300.1186805@rhweight-WRK1>
In-Reply-To: <alpine.DEB.2.22.394.2102031504300.1186805@rhweight-WRK1>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f5601a63-25e6-4915-aef0-08d8c8afef2c
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Feb 2021 01:55:22.9090
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: soXW0/x+MuXE48KgCE/EuL7KPUa6vhOX0hPUVTI6x8VgKgUpMYhXkzX8hyi+VWaVXpQp55x00Tj88JZgL5ZPOg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB3657
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> On Wed, 3 Feb 2021, Russ Weight wrote:
>=20
> >
> >
> > 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 pa=
tch than
> >>> others, since we haven't seen any issues with current products. Pleas=
e 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 =3D 0. Change
> >>>>> __afu_port_enable() to guarantee that the enable process
> >>>>> is complete by polling for ACK =3D=3D 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 po=
rt
> >>>> 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 al=
l
> >>>> 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 wa=
it
> >>> 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=3D1
> >>>> 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=3D0
> >>>> This sequence ensures that outstanding transactions are suitably flu=
shed
> and
> >>>> that the FIM minimum reset pulse width is respected. Failing to foll=
ow 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 sce=
narios:
> >>>>
> >>>>   * Different platform can take variable period of time to get out o=
f 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 r=
eason
> >>> 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 m=
ake
> >> sure new changes introduced in new version spec, don't break old produ=
cts
> >> as we are sharing the same driver. and we are not sure if in the futur=
e some
> >> new products but still uses old specs, and then things may be broken i=
f 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 su=
ggest
> >> 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 clea=
r the
> > ACK is something we could have (should have?) been doing all along. The
> timing
>=20
> 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 si=
de,
then we should be safe to take this one.

Hao

>=20
> Matthew Gerlach
>=20
> > hasn't been an issue for the current PAC products, as proven by our tes=
ting.
> > However, with OFS we cannot anticipate what the timing will be for cust=
omer
> > 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 install=
ed. I
> > cleared port errors on these cards as a quick check, and the reset comp=
letes
> > without hanging - which indicates that the ACK bit is in fact getting c=
leared.
> > So there is not need for any device-specific conditional statements her=
e.
> >
> > - Russ
> >
> >>
> >> Hao
> >
> >