Messages in this thread | | | Date | Wed, 2 Dec 2020 11:06:30 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | RE: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability |
| |
On Wed, 2 Dec 2020, Wu, Hao wrote:
>> Subject: Re: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability >> >> Hi Matthew, >> >> 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 capability >>>> >>>> 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 values >> 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.rst >>>>> 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/dfl- >> fme- >>>>> pr.c) >>>>> could be a reference. >>>>> >>>>> +Location of DFLs on a PCI Device >>>>> +=========================== >>>>> +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 flexibility >>>>> +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 Offset/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 structure. >> >> 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 code > below. So people can find matched information in both code and doc easily. : )
I think this makes sense. I can change the name of the variable to bir to match the documentation.
> > Thanks > Hao >
| |