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: G1iMg5FlOW9xJBM7Ml1G4yQAqXX9afwDR4I9/yCchtekc9LUboObcojkIyYCiGN4oeGuOX/aqM
 omvJulFKVJJw==
IronPort-SDR: xipLtQ21wLXY74r9VYCrPKpT231DoznjDz4Cs9gDAq3XM6drd5WkX6nEhO9VDCyFaafDVxJmix
 txwfPHapyOcw==
From:   "Wu, Hao" <hao.wu@intel.com>
To:     Moritz Fischer <mdf@kernel.org>,
        "matthew.gerlach@linux.intel.com" <matthew.gerlach@linux.intel.com>
CC:     "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>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "corbet@lwn.net" <corbet@lwn.net>
Subject: RE: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability
Thread-Topic: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability
Thread-Index: AQHWwnpOqjhoM37MKU6ujI2Rfo8RvKndD8WwgARhwwCAATDggIAA+WKA
Date:   Wed, 2 Dec 2020 09:56:26 +0000
Message-ID: <DM6PR11MB38191CF23533B7C1B44B56EA85F30@DM6PR11MB3819.namprd11.prod.outlook.com>
References: <20201124155658.700976-1-matthew.gerlach@linux.intel.com>
 <20201124155658.700976-3-matthew.gerlach@linux.intel.com>
 <DM6PR11MB38191D8C5E27E6E04B8DAA1A85F70@DM6PR11MB3819.namprd11.prod.outlook.com>
 <alpine.DEB.2.22.394.2011301614040.1050045@rhweight-WRK1>
 <X8aR36hGoV9SsPDw@archbook>
In-Reply-To: <X8aR36hGoV9SsPDw@archbook>
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: 2422c29e-674c-4690-10e3-08d896a88896
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Dec 2020 09:56:26.2213
 (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: /pSO3W5z2MwDRk/3WiPoYJnszqdt7TdzQsKM9DpxXL074qOZr4ffTmes6FIbMMgYZ3COYCvunC4r+K6qznj9Qw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB1418
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH v3 2/2] fpga: dfl: look for vendor specific capabilit=
y
>=20
> Hi Matthew,
>=20
> On Mon, Nov 30, 2020 at 04:45:20PM -0800,
> matthew.gerlach@linux.intel.com wrote:
> >
> >
> > On Sat, 28 Nov 2020, Wu, Hao wrote:
> >
> > > > Subject: [PATCH v3 2/2] fpga: dfl: look for vendor specific capabil=
ity
> > >
> > > Maybe we can change the title a little bit, what about
> > > fpga: dfl-pci: locate DFLs by PCIe vendor specific capability
> > >
> > > >
> > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > >
> > > > A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
> > > > specific capability can be used to specify the start of a
> > > > number of DFLs.
> > >
> > > A PCIe vendor specific extended capability is introduced by Intel to
> > > specify the start of a number of DFLs.
> >
> > Your suggestion is more precise.
> > >
> > >
> > > >
> > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > ---
> > > > v3: Add text and ascii art to documentation.
> > > >     Ensure not to exceed PCIe config space in loop.
> > > >
> > > > v2: Update documentation for clarity.
> > > >     Clean up  macro names.
> > > >     Use GENMASK.
> > > >     Removed spurious blank lines.
> > > >     Changed some calls from dev_info to dev_dbg.
> > > >     Specifically check for VSEC not found, -ENODEV.
> > > >     Ensure correct pci vendor id.
> > > >     Remove check for page alignment.
> > > >     Rename find_dfl_in_cfg to find_dfls_by_vsec.
> > > >     Initialize target memory of pci_read_config_dword to invalid va=
lues
> before
> > > > use.
> > > > ---
> > > >  Documentation/fpga/dfl.rst | 25 +++++++++++
> > > >  drivers/fpga/dfl-pci.c     | 91
> +++++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 115 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rs=
t
> > > > index 0404fe6ffc74..fa0da884a818 100644
> > > > --- a/Documentation/fpga/dfl.rst
> > > > +++ b/Documentation/fpga/dfl.rst
> > > > @@ -501,6 +501,31 @@ Developer only needs to provide a sub feature
> > > > driver with matched feature id.
> > > >  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/d=
fl-
> fme-
> > > > pr.c)
> > > >  could be a reference.
> > > >
> > > > +Location of DFLs on a PCI Device
> > > > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
> > > > +There are two ways of locating DFLs on a PCI Device.  The original
> > >
> > > I found this new VSEC is only for PCIe device, correct? If so, let's =
make
> > > sure descriptions are accurate. E.g. default method for all devices
> > > and a new method for PCIe device.
> >
> > Yes, the default method can be used with PCI and PCIe device, and the
> VSEC
> > approach is PCIe, only.  Documentation can be made more precise.
> >
> > >
> > > > +method assumed the start of the first DFL to offset 0 of bar 0.
> > > > +If the first node of the DFL is an FME, then further DFLs
> > > > +in the port(s) are specified in FME header registers.
> > > > +Alternatively, a vendor specific capability structure can be used =
to
> Maybe: a vendor specific extended capability (VSEC) ...
> > > > +specify the location of all the DFLs on the device, providing flex=
ibility
> > > > +for the type of starting node in the DFL.  Intel has reserved the
> > > > +VSEC ID of 0x43 for this purpose.  The vendor specific
> > > > +data begins with a 4 byte vendor specific register for the number =
of
> DFLs
> > > > followed 4 byte
> > > > +Offset/BIR vendor specific registers for each DFL. Bits 2:0 of Off=
set/BIR
> > > > register
> > >
> > > Do we have a defined register name here? or it's named as Offset/BIR
> register?
> > > Sounds a little wired, and I see you defined it as DFLS_RES?
> >
> > The Offset/BIR terminology is also used in the MSI-X capability structu=
re.
>=20
> Yeah, this intuitively made sense to me having worked with PCIe :)

I just feel that it's better to use the same register name defined in the c=
ode
below. So people can find matched information in both code and doc easily. =
: )

Thanks
Hao