* [PATCH] fpga: dfl: pci: generalize find_dfls_by_vsec()
@ 2021-11-13 22:12 trix
2021-11-15 1:25 ` Xu Yilun
0 siblings, 1 reply; 4+ messages in thread
From: trix @ 2021-11-13 22:12 UTC (permalink / raw)
To: hao.wu, mdf, yilun.xu; +Cc: linux-fpga, linux-kernel, Tom Rix
From: Tom Rix <trix@redhat.com>
find_dfls_by_vsec() is a general dfl function.
Although dfl has multiple vendors, only Intel is supported.
Move vsec and vendor id to an array variable.
Other vendors can append the array to enable their support.
Signed-off-by: Tom Rix <trix@redhat.com>
---
drivers/fpga/dfl-pci.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 4d68719e608f..9dc0815c8274 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -136,19 +136,36 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
return table;
}
-static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
+struct dfl_vsec {
+ u16 vendor;
+ u16 id;
+};
+
+static struct dfl_vsec vsecs[] = {
+ { PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS },
+};
+
+static int find_dfls_by_vsec(struct pci_dev *pcidev,
+ struct dfl_fpga_enum_info *info)
{
u32 bir, offset, vndr_hdr, dfl_cnt, dfl_res;
int dfl_res_off, i, bars, voff = 0;
resource_size_t start, len;
- while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
- vndr_hdr = 0;
- pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
+ for (i = 0; i < ARRAY_SIZE(vsecs); i++) {
+ if (pcidev->vendor != vsecs[i].vendor)
+ continue;
+
+ while ((voff =
+ pci_find_next_ext_capability(pcidev, voff,
+ PCI_EXT_CAP_ID_VNDR))) {
+ vndr_hdr = 0;
+ pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER,
+ &vndr_hdr);
- if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VSEC_ID_INTEL_DFLS &&
- pcidev->vendor == PCI_VENDOR_ID_INTEL)
- break;
+ if (PCI_VNDR_HEADER_ID(vndr_hdr) == vsecs[i].id)
+ break;
+ }
}
if (!voff) {
--
2.26.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] fpga: dfl: pci: generalize find_dfls_by_vsec()
2021-11-13 22:12 [PATCH] fpga: dfl: pci: generalize find_dfls_by_vsec() trix
@ 2021-11-15 1:25 ` Xu Yilun
2021-11-15 18:28 ` Tom Rix
0 siblings, 1 reply; 4+ messages in thread
From: Xu Yilun @ 2021-11-15 1:25 UTC (permalink / raw)
To: trix; +Cc: hao.wu, mdf, linux-fpga, linux-kernel
On Sat, Nov 13, 2021 at 02:12:52PM -0800, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
>
> find_dfls_by_vsec() is a general dfl function.
> Although dfl has multiple vendors, only Intel is supported.
> Move vsec and vendor id to an array variable.
> Other vendors can append the array to enable their support.
As Hao mentioned, DVSEC could be a better solution if DFL should be
present in components by a variety of vendors. This is not finally
determined, but I think we should not add new features for VSEC now.
Thanks,
Yilun
>
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
> drivers/fpga/dfl-pci.c | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index 4d68719e608f..9dc0815c8274 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -136,19 +136,36 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> return table;
> }
>
> -static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
> +struct dfl_vsec {
> + u16 vendor;
> + u16 id;
> +};
> +
> +static struct dfl_vsec vsecs[] = {
> + { PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS },
> +};
> +
> +static int find_dfls_by_vsec(struct pci_dev *pcidev,
> + struct dfl_fpga_enum_info *info)
> {
> u32 bir, offset, vndr_hdr, dfl_cnt, dfl_res;
> int dfl_res_off, i, bars, voff = 0;
> resource_size_t start, len;
>
> - while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
> - vndr_hdr = 0;
> - pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
> + for (i = 0; i < ARRAY_SIZE(vsecs); i++) {
> + if (pcidev->vendor != vsecs[i].vendor)
> + continue;
> +
> + while ((voff =
> + pci_find_next_ext_capability(pcidev, voff,
> + PCI_EXT_CAP_ID_VNDR))) {
> + vndr_hdr = 0;
> + pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER,
> + &vndr_hdr);
>
> - if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VSEC_ID_INTEL_DFLS &&
> - pcidev->vendor == PCI_VENDOR_ID_INTEL)
> - break;
> + if (PCI_VNDR_HEADER_ID(vndr_hdr) == vsecs[i].id)
> + break;
> + }
> }
>
> if (!voff) {
> --
> 2.26.3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fpga: dfl: pci: generalize find_dfls_by_vsec()
2021-11-15 1:25 ` Xu Yilun
@ 2021-11-15 18:28 ` Tom Rix
2021-11-16 2:01 ` Xu Yilun
0 siblings, 1 reply; 4+ messages in thread
From: Tom Rix @ 2021-11-15 18:28 UTC (permalink / raw)
To: Xu Yilun; +Cc: hao.wu, mdf, linux-fpga, linux-kernel
On 11/14/21 5:25 PM, Xu Yilun wrote:
> On Sat, Nov 13, 2021 at 02:12:52PM -0800, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> find_dfls_by_vsec() is a general dfl function.
>> Although dfl has multiple vendors, only Intel is supported.
>> Move vsec and vendor id to an array variable.
>> Other vendors can append the array to enable their support.
> As Hao mentioned, DVSEC could be a better solution if DFL should be
> present in components by a variety of vendors. This is not finally
> determined, but I think we should not add new features for VSEC now.
Can you expand what you mean by this ?
I am considering the n5010 usecase, the vendor is not intel and will go
through this dfl function and always fail.
This is broken.
Either the function should be generalized or moved to an intel specific
call.
Tom
>
> Thanks,
> Yilun
>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>> drivers/fpga/dfl-pci.c | 31 ++++++++++++++++++++++++-------
>> 1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index 4d68719e608f..9dc0815c8274 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -136,19 +136,36 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
>> return table;
>> }
>>
>> -static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
>> +struct dfl_vsec {
>> + u16 vendor;
>> + u16 id;
>> +};
>> +
>> +static struct dfl_vsec vsecs[] = {
>> + { PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS },
>> +};
>> +
>> +static int find_dfls_by_vsec(struct pci_dev *pcidev,
>> + struct dfl_fpga_enum_info *info)
>> {
>> u32 bir, offset, vndr_hdr, dfl_cnt, dfl_res;
>> int dfl_res_off, i, bars, voff = 0;
>> resource_size_t start, len;
>>
>> - while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
>> - vndr_hdr = 0;
>> - pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
>> + for (i = 0; i < ARRAY_SIZE(vsecs); i++) {
>> + if (pcidev->vendor != vsecs[i].vendor)
>> + continue;
>> +
>> + while ((voff =
>> + pci_find_next_ext_capability(pcidev, voff,
>> + PCI_EXT_CAP_ID_VNDR))) {
>> + vndr_hdr = 0;
>> + pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER,
>> + &vndr_hdr);
>>
>> - if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VSEC_ID_INTEL_DFLS &&
>> - pcidev->vendor == PCI_VENDOR_ID_INTEL)
>> - break;
>> + if (PCI_VNDR_HEADER_ID(vndr_hdr) == vsecs[i].id)
>> + break;
>> + }
>> }
>>
>> if (!voff) {
>> --
>> 2.26.3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fpga: dfl: pci: generalize find_dfls_by_vsec()
2021-11-15 18:28 ` Tom Rix
@ 2021-11-16 2:01 ` Xu Yilun
0 siblings, 0 replies; 4+ messages in thread
From: Xu Yilun @ 2021-11-16 2:01 UTC (permalink / raw)
To: Tom Rix; +Cc: hao.wu, mdf, linux-fpga, linux-kernel
On Mon, Nov 15, 2021 at 10:28:32AM -0800, Tom Rix wrote:
>
> On 11/14/21 5:25 PM, Xu Yilun wrote:
> > On Sat, Nov 13, 2021 at 02:12:52PM -0800, trix@redhat.com wrote:
> > > From: Tom Rix <trix@redhat.com>
> > >
> > > find_dfls_by_vsec() is a general dfl function.
> > > Although dfl has multiple vendors, only Intel is supported.
> > > Move vsec and vendor id to an array variable.
> > > Other vendors can append the array to enable their support.
> > As Hao mentioned, DVSEC could be a better solution if DFL should be
> > present in components by a variety of vendors. This is not finally
> > determined, but I think we should not add new features for VSEC now.
>
> Can you expand what you mean by this ?
Using vendor_id-vsec pair is not a good way. It requires a specific VSEC
ID for each vendor to describe DFL. DVSEC is actually the existing
solution for these cases. I expect we switch to DVSEC for DFL and drop
this VSEC solution.
>
> I am considering the n5010 usecase, the vendor is not intel and will go
> through this dfl function and always fail.
Is n5010 using the DFL VSEC, or it just finds DFL by default? Its devId
is supported several month ago and no issue reported so I assume it is
using the default DFL finding. It would be better to stick to it.
Thanks,
Yilun
>
> This is broken.
>
> Either the function should be generalized or moved to an intel specific
> call.
>
> Tom
>
> >
> > Thanks,
> > Yilun
> >
> > > Signed-off-by: Tom Rix <trix@redhat.com>
> > > ---
> > > drivers/fpga/dfl-pci.c | 31 ++++++++++++++++++++++++-------
> > > 1 file changed, 24 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > > index 4d68719e608f..9dc0815c8274 100644
> > > --- a/drivers/fpga/dfl-pci.c
> > > +++ b/drivers/fpga/dfl-pci.c
> > > @@ -136,19 +136,36 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> > > return table;
> > > }
> > > -static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
> > > +struct dfl_vsec {
> > > + u16 vendor;
> > > + u16 id;
> > > +};
> > > +
> > > +static struct dfl_vsec vsecs[] = {
> > > + { PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS },
> > > +};
> > > +
> > > +static int find_dfls_by_vsec(struct pci_dev *pcidev,
> > > + struct dfl_fpga_enum_info *info)
> > > {
> > > u32 bir, offset, vndr_hdr, dfl_cnt, dfl_res;
> > > int dfl_res_off, i, bars, voff = 0;
> > > resource_size_t start, len;
> > > - while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
> > > - vndr_hdr = 0;
> > > - pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
> > > + for (i = 0; i < ARRAY_SIZE(vsecs); i++) {
> > > + if (pcidev->vendor != vsecs[i].vendor)
> > > + continue;
> > > +
> > > + while ((voff =
> > > + pci_find_next_ext_capability(pcidev, voff,
> > > + PCI_EXT_CAP_ID_VNDR))) {
> > > + vndr_hdr = 0;
> > > + pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER,
> > > + &vndr_hdr);
> > > - if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VSEC_ID_INTEL_DFLS &&
> > > - pcidev->vendor == PCI_VENDOR_ID_INTEL)
> > > - break;
> > > + if (PCI_VNDR_HEADER_ID(vndr_hdr) == vsecs[i].id)
> > > + break;
> > > + }
> > > }
> > > if (!voff) {
> > > --
> > > 2.26.3
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-16 2:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-13 22:12 [PATCH] fpga: dfl: pci: generalize find_dfls_by_vsec() trix
2021-11-15 1:25 ` Xu Yilun
2021-11-15 18:28 ` Tom Rix
2021-11-16 2:01 ` Xu Yilun
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.