| Message ID | 20220214112619.219761-4-tianfei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add Intel OFS support for DFL driver | expand |
On 2/14/22 3:26 AM, Tianfei zhang wrote: > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > From a fpga partial reconfiguration standpoint, a port > may not be connected any local BAR space. The port could > be connected to a different PCIe Physical Function (PF) or > Virtual Function (VF), in which case another driver instance > would manage the endpoint. It is not clear if this is part of iofs or a bug fix. > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> > --- > drivers/fpga/dfl-pci.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c > index 4d68719e608f..8abd9b408403 100644 > --- a/drivers/fpga/dfl-pci.c > +++ b/drivers/fpga/dfl-pci.c > @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev, > v = readq(base + FME_HDR_CAP); > port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); > > + dev_info(&pcidev->dev, "port_num = %d\n", port_num); > WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); > > for (i = 0; i < port_num; i++) { > @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct pci_dev *pcidev, > */ > bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); > offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); > + if (bar >= PCI_STD_NUM_BARS) { Is bar set to a better magic number that pci_std_num_bars ? maybe 0xff's How do you tell between this case and broken hw ? Move up a line and skip getting an offset that will not be used. > + dev_info(&pcidev->dev, "skipping port without local BAR space %d\n", > + bar); > + continue; > + } else { > + dev_info(&pcidev->dev, "BAR %d offset %u\n", bar, offset); > + } > start = pci_resource_start(pcidev, bar) + offset; > len = pci_resource_len(pcidev, bar) - offset; > Is similar logic needed for else-if (port) block below this ? Tom
> Subject: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space. > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > From a fpga partial reconfiguration standpoint, a port > may not be connected any local BAR space. The port could > be connected to a different PCIe Physical Function (PF) or > Virtual Function (VF), in which case another driver instance > would manage the endpoint. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> > --- > drivers/fpga/dfl-pci.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c > index 4d68719e608f..8abd9b408403 100644 > --- a/drivers/fpga/dfl-pci.c > +++ b/drivers/fpga/dfl-pci.c > @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev, > v = readq(base + FME_HDR_CAP); > port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); > > + dev_info(&pcidev->dev, "port_num = %d\n", port_num); Do we really need this info here? in FME there is one sysfs interface for port num. > WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); > > for (i = 0; i < port_num; i++) { > @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct pci_dev *pcidev, > */ > bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); > offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); > + if (bar >= PCI_STD_NUM_BARS) { > + dev_info(&pcidev->dev, "skipping port without > local BAR space %d\n", > + bar); > + continue; Is this change for IOFS? From patch #1, we have FME and PORT on PF0, so we should have a BAR for PORT on PF0, is my understanding correct? Thanks Hao > + } else { > + dev_info(&pcidev->dev, "BAR %d offset %u\n", > bar, offset); > + } > start = pci_resource_start(pcidev, bar) + offset; > len = pci_resource_len(pcidev, bar) - offset; > > -- > 2.17.1
> -----Original Message----- > From: Tom Rix <trix@redhat.com> > Sent: Tuesday, February 15, 2022 11:06 PM > To: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>; > mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; linux-fpga@vger.kernel.org; > linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: corbet@lwn.net; Matthew Gerlach <matthew.gerlach@linux.intel.com> > Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space. > > > On 2/14/22 3:26 AM, Tianfei zhang wrote: > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > > > From a fpga partial reconfiguration standpoint, a port may not be > > connected any local BAR space. The port could be connected to a > > different PCIe Physical Function (PF) or Virtual Function (VF), in > > which case another driver instance would manage the endpoint. > > It is not clear if this is part of iofs or a bug fix. This is the new implementation/feature of IOFS. On IOFS support multiple methods to access the AFU. 1. Legacy Model. This is used for N3000 and N5000 card. In this model the entire AFU region is a unit of PR, and there is a Port device connected to this AFU. On DFL perspective, there is "Next AFU" point to the AFU, and the "BarID" is the PCIe Bar ID of AFU. In this model, we can use the AFU APIs to access the entire AFU resource, like MMIO. 2. Micro-Personas in AFU. IOFS intruding new model for PR and AFU access. Micro-Personas allow the RTL developer to designate their own AFU-defined PR regions. In this model the unit of PR is not the entire AFU, instead the unit of PR can be any size block or blocks inside the AFU. 3. Multiple VFs per PR slot. In this method, we can instance multiple VFs over SRIOV for one PR slot, and access the AFU resource by different VFs in virtualization usage. In this case, the Port device would not connected to AFU (the BarID of Port device should be set to invalid), so this patch want to support this use model. > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> > > --- > > drivers/fpga/dfl-pci.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index > > 4d68719e608f..8abd9b408403 100644 > > --- a/drivers/fpga/dfl-pci.c > > +++ b/drivers/fpga/dfl-pci.c > > @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev, > > v = readq(base + FME_HDR_CAP); > > port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); > > > > + dev_info(&pcidev->dev, "port_num = %d\n", port_num); > > WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); > > > > for (i = 0; i < port_num; i++) { > > @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct pci_dev *pcidev, > > */ > > bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); > > offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); > > + if (bar >= PCI_STD_NUM_BARS) { > > Is bar set to a better magic number that pci_std_num_bars ? maybe 0xff's > > How do you tell between this case and broken hw ? Yes, I agree that magic number is better, Currently the RTL using PCI_STD_NUM_BARS for an invalid PCIe bar number. > > Move up a line and skip getting an offset that will not be used. Yes, this line is not necessary, I will remove it on next version patch. > > > + dev_info(&pcidev->dev, "skipping port without > local BAR space %d\n", > > + bar); > > + continue; > > + } else { > > + dev_info(&pcidev->dev, "BAR %d offset %u\n", > bar, offset); > > + } > > start = pci_resource_start(pcidev, bar) + offset; > > len = pci_resource_len(pcidev, bar) - offset; > > > > Is similar logic needed for else-if (port) block below this ? I think, the else-if is not necessary. I will remove it on next version patch. > > Tom
On 2/17/22 11:31 PM, Zhang, Tianfei wrote: > >> -----Original Message----- >> From: Tom Rix <trix@redhat.com> >> Sent: Tuesday, February 15, 2022 11:06 PM >> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>; >> mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; linux-fpga@vger.kernel.org; >> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org >> Cc: corbet@lwn.net; Matthew Gerlach <matthew.gerlach@linux.intel.com> >> Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space. >> >> >> On 2/14/22 3:26 AM, Tianfei zhang wrote: >>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> >>> From a fpga partial reconfiguration standpoint, a port may not be >>> connected any local BAR space. The port could be connected to a >>> different PCIe Physical Function (PF) or Virtual Function (VF), in >>> which case another driver instance would manage the endpoint. >> It is not clear if this is part of iofs or a bug fix. > This is the new implementation/feature of IOFS. > On IOFS support multiple methods to access the AFU. > 1. Legacy Model. This is used for N3000 and N5000 card. > In this model the entire AFU region is a unit of PR, and there is a Port device connected to this AFU. > On DFL perspective, there is "Next AFU" point to the AFU, and the "BarID" is the PCIe Bar ID of AFU. > In this model, we can use the AFU APIs to access the entire AFU resource, like MMIO. > 2. Micro-Personas in AFU. > IOFS intruding new model for PR and AFU access. > Micro-Personas allow the RTL developer to designate their own AFU-defined PR regions. > In this model the unit of PR is not the entire AFU, instead > the unit of PR can be any size block or blocks inside the AFU. > 3. Multiple VFs per PR slot. > In this method, we can instance multiple VFs over SRIOV for one PR slot, and access the AFU resource > by different VFs in virtualization usage. In this case, the Port device would not connected to AFU (the BarID of Port device > should be set to invalid), so this patch want to support this use model. What I am looking for is how the older cards using (my term) dfl 1 will still work with dfl 2 and vice versa. No where do I see a version check for dfl 2 nor a pci id check so either this just works or backward compatibility has not be considered. Please add a backward compatibility section to the doc patch > >>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> >>> --- >>> drivers/fpga/dfl-pci.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index >>> 4d68719e608f..8abd9b408403 100644 >>> --- a/drivers/fpga/dfl-pci.c >>> +++ b/drivers/fpga/dfl-pci.c >>> @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev, >>> v = readq(base + FME_HDR_CAP); >>> port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); >>> >>> + dev_info(&pcidev->dev, "port_num = %d\n", port_num); >>> WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); >>> >>> for (i = 0; i < port_num; i++) { >>> @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct pci_dev *pcidev, >>> */ >>> bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); >>> offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); >>> + if (bar >= PCI_STD_NUM_BARS) { >> Is bar set to a better magic number that pci_std_num_bars ? maybe 0xff's >> >> How do you tell between this case and broken hw ? > Yes, I agree that magic number is better, Currently the RTL using PCI_STD_NUM_BARS for an invalid PCIe bar number. How do you tell between this case and broken hw ? Tom >> Move up a line and skip getting an offset that will not be used. > Yes, this line is not necessary, I will remove it on next version patch. > >>> + dev_info(&pcidev->dev, "skipping port without >> local BAR space %d\n", >>> + bar); >>> + continue; >>> + } else { >>> + dev_info(&pcidev->dev, "BAR %d offset %u\n", >> bar, offset); >>> + } >>> start = pci_resource_start(pcidev, bar) + offset; >>> len = pci_resource_len(pcidev, bar) - offset; >>> >> Is similar logic needed for else-if (port) block below this ? > I think, the else-if is not necessary. I will remove it on next version patch. >> Tom
> -----Original Message----- > From: Wu, Hao <hao.wu@intel.com> > Sent: Wednesday, February 16, 2022 11:38 AM > To: Zhang, Tianfei <tianfei.zhang@intel.com>; trix@redhat.com; > mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; linux-fpga@vger.kernel.org; > linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: corbet@lwn.net; Matthew Gerlach <matthew.gerlach@linux.intel.com> > Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space. > > > Subject: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space. > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > > > From a fpga partial reconfiguration standpoint, a port may not be > > connected any local BAR space. The port could be connected to a > > different PCIe Physical Function (PF) or Virtual Function (VF), in > > which case another driver instance would manage the endpoint. > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> > > --- > > drivers/fpga/dfl-pci.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index > > 4d68719e608f..8abd9b408403 100644 > > --- a/drivers/fpga/dfl-pci.c > > +++ b/drivers/fpga/dfl-pci.c > > @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev, > > v = readq(base + FME_HDR_CAP); > > port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); > > > > + dev_info(&pcidev->dev, "port_num = %d\n", port_num); > > Do we really need this info here? in FME there is one sysfs interface for port > num. I think it is not necessary, I will remove it. > > > WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); > > > > for (i = 0; i < port_num; i++) { > > @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct pci_dev *pcidev, > > */ > > bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); > > offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); > > + if (bar >= PCI_STD_NUM_BARS) { > > + dev_info(&pcidev->dev, "skipping port without > > local BAR space %d\n", > > + bar); > > + continue; > > Is this change for IOFS? From patch #1, we have FME and PORT on PF0, so we > should have a BAR for PORT on PF0, is my understanding correct? Yes, we have a Port device on each PR slot in IOFS, but for " Multiple VFs per PR slot" model, the Port device would not connected to AFU/PR slot (the BarID of Port device should be set to invalid), and we just can access PR slot/AFU resource via VFs. > > Thanks > Hao > > > + } else { > > + dev_info(&pcidev->dev, "BAR %d offset %u\n", > > bar, offset); > > + } > > start = pci_resource_start(pcidev, bar) + offset; > > len = pci_resource_len(pcidev, bar) - offset; > > > > -- > > 2.17.1
On Fri, 18 Feb 2022, Tom Rix wrote: > > On 2/17/22 11:31 PM, Zhang, Tianfei wrote: >> >>> -----Original Message----- >>> From: Tom Rix <trix@redhat.com> >>> Sent: Tuesday, February 15, 2022 11:06 PM >>> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>; >>> mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; >>> linux-fpga@vger.kernel.org; >>> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org >>> Cc: corbet@lwn.net; Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar >>> space. >>> >>> >>> On 2/14/22 3:26 AM, Tianfei zhang wrote: >>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>>> >>>> From a fpga partial reconfiguration standpoint, a port may not be >>>> connected any local BAR space. The port could be connected to a >>>> different PCIe Physical Function (PF) or Virtual Function (VF), in >>>> which case another driver instance would manage the endpoint. >>> It is not clear if this is part of iofs or a bug fix. >> This is the new implementation/feature of IOFS. >> On IOFS support multiple methods to access the AFU. >> 1. Legacy Model. This is used for N3000 and N5000 card. >> In this model the entire AFU region is a unit of PR, and there is a Port >> device connected to this AFU. >> On DFL perspective, there is "Next AFU" point to the AFU, and the "BarID" >> is the PCIe Bar ID of AFU. >> In this model, we can use the AFU APIs to access the entire AFU resource, >> like MMIO. >> 2. Micro-Personas in AFU. >> IOFS intruding new model for PR and AFU access. >> Micro-Personas allow the RTL developer to designate their own AFU-defined >> PR regions. >> In this model the unit of PR is not the entire AFU, instead >> the unit of PR can be any size block or blocks inside the AFU. >> 3. Multiple VFs per PR slot. >> In this method, we can instance multiple VFs over SRIOV for one PR slot, >> and access the AFU resource >> by different VFs in virtualization usage. In this case, the Port device >> would not connected to AFU (the BarID of Port device >> should be set to invalid), so this patch want to support this use model. > > What I am looking for is how the older cards using (my term) dfl 1 will still > work with dfl 2 and vice versa. > > No where do I see a version check for dfl 2 nor a pci id check so either this > just works or backward compatibility has not be considered. > > Please add a backward compatibility section to the doc patch > >> >>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> >>>> --- >>>> drivers/fpga/dfl-pci.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index >>>> 4d68719e608f..8abd9b408403 100644 >>>> --- a/drivers/fpga/dfl-pci.c >>>> +++ b/drivers/fpga/dfl-pci.c >>>> @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev >>>> *pcidev, >>>> v = readq(base + FME_HDR_CAP); >>>> port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); >>>> >>>> + dev_info(&pcidev->dev, "port_num = %d\n", port_num); >>>> WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); >>>> >>>> for (i = 0; i < port_num; i++) { >>>> @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct pci_dev >>>> *pcidev, >>>> */ >>>> bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); >>>> offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); >>>> + if (bar >= PCI_STD_NUM_BARS) { >>> Is bar set to a better magic number that pci_std_num_bars ? maybe 0xff's >>> >>> How do you tell between this case and broken hw ? >> Yes, I agree that magic number is better, Currently the RTL using >> PCI_STD_NUM_BARS for an invalid PCIe bar number. > > How do you tell between this case and broken hw ? > > Tom The field, FME_PORT_OFST_BAR_ID, is a three bit field, which is pretty common for BARs on PCI. PCI_STD_NUM_BARS is defined as 6. Current HW implementations are filing this field with the value, 7, which is close to the suggestion of 0xff's. How about we define the following magic number? #define NO_LOCAL_PORT_BAR 7 We should also change the dev_info to be a dev_dbg and more precise to something like the following: if (bar == NO_LOCAL_PORT_BAR) { dev_dbg(&pcidev->dev, "No local port BAR space.\n"); continue; } > >>> Move up a line and skip getting an offset that will not be used. >> Yes, this line is not necessary, I will remove it on next version patch. >> >>>> + dev_info(&pcidev->dev, "skipping port without >>> local BAR space %d\n", >>>> + bar); >>>> + continue; >>>> + } else { >>>> + dev_info(&pcidev->dev, "BAR %d offset %u\n", >>> bar, offset); >>>> + } >>>> start = pci_resource_start(pcidev, bar) + offset; >>>> len = pci_resource_len(pcidev, bar) - offset; >>>> >>> Is similar logic needed for else-if (port) block below this ? >> I think, the else-if is not necessary. I will remove it on next version >> patch. >>> Tom > >
On 2/21/22 9:22 AM, matthew.gerlach@linux.intel.com wrote: > > > On Fri, 18 Feb 2022, Tom Rix wrote: > >> >> On 2/17/22 11:31 PM, Zhang, Tianfei wrote: >>> >>>> -----Original Message----- >>>> From: Tom Rix <trix@redhat.com> >>>> Sent: Tuesday, February 15, 2022 11:06 PM >>>> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao >>>> <hao.wu@intel.com>; >>>> mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; >>>> linux-fpga@vger.kernel.org; >>>> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org >>>> Cc: corbet@lwn.net; Matthew Gerlach <matthew.gerlach@linux.intel.com> >>>> Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no >>>> local bar space. >>>> >>>> >>>> On 2/14/22 3:26 AM, Tianfei zhang wrote: >>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>>>> >>>>> From a fpga partial reconfiguration standpoint, a port may not be >>>>> connected any local BAR space. The port could be connected to a >>>>> different PCIe Physical Function (PF) or Virtual Function (VF), in >>>>> which case another driver instance would manage the endpoint. >>>> It is not clear if this is part of iofs or a bug fix. >>> This is the new implementation/feature of IOFS. >>> On IOFS support multiple methods to access the AFU. >>> 1. Legacy Model. This is used for N3000 and N5000 card. >>> In this model the entire AFU region is a unit of PR, and there is a >>> Port device connected to this AFU. >>> On DFL perspective, there is "Next AFU" point to the AFU, and the >>> "BarID" is the PCIe Bar ID of AFU. >>> In this model, we can use the AFU APIs to access the entire AFU >>> resource, like MMIO. >>> 2. Micro-Personas in AFU. >>> IOFS intruding new model for PR and AFU access. >>> Micro-Personas allow the RTL developer to designate their own >>> AFU-defined PR regions. >>> In this model the unit of PR is not the entire AFU, instead >>> the unit of PR can be any size block or blocks inside the AFU. >>> 3. Multiple VFs per PR slot. >>> In this method, we can instance multiple VFs over SRIOV for one PR >>> slot, and access the AFU resource >>> by different VFs in virtualization usage. In this case, the Port >>> device would not connected to AFU (the BarID of Port device >>> should be set to invalid), so this patch want to support this use >>> model. >> >> What I am looking for is how the older cards using (my term) dfl 1 >> will still work with dfl 2 and vice versa. >> >> No where do I see a version check for dfl 2 nor a pci id check so >> either this just works or backward compatibility has not be considered. >> >> Please add a backward compatibility section to the doc patch >> >>> >>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> >>>>> --- >>>>> drivers/fpga/dfl-pci.c | 8 ++++++++ >>>>> 1 file changed, 8 insertions(+) >>>>> >>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index >>>>> 4d68719e608f..8abd9b408403 100644 >>>>> --- a/drivers/fpga/dfl-pci.c >>>>> +++ b/drivers/fpga/dfl-pci.c >>>>> @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev >>>>> *pcidev, >>>>> v = readq(base + FME_HDR_CAP); >>>>> port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); >>>>> >>>>> + dev_info(&pcidev->dev, "port_num = %d\n", port_num); >>>>> WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); >>>>> >>>>> for (i = 0; i < port_num; i++) { >>>>> @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct >>>>> pci_dev *pcidev, >>>>> */ >>>>> bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); >>>>> offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); >>>>> + if (bar >= PCI_STD_NUM_BARS) { >>>> Is bar set to a better magic number that pci_std_num_bars ? maybe >>>> 0xff's >>>> >>>> How do you tell between this case and broken hw ? >>> Yes, I agree that magic number is better, Currently the RTL using >>> PCI_STD_NUM_BARS for an invalid PCIe bar number. >> >> How do you tell between this case and broken hw ? >> >> Tom > > The field, FME_PORT_OFST_BAR_ID, is a three bit field, which is pretty > common for BARs on PCI. PCI_STD_NUM_BARS is defined as 6. Current HW > implementations are filing this field with the value, 7, which is > close to the suggestion of 0xff's. How about we define the following > magic number? > #define NO_LOCAL_PORT_BAR 7 > > We should also change the dev_info to be a dev_dbg and more precise to > something like the following: > > if (bar == NO_LOCAL_PORT_BAR) { > dev_dbg(&pcidev->dev, "No local port BAR space.\n"); > continue; > } What I am looking for is way generally is to tell if this is an old framework or a new one. Maybe a flag and/or version added to dfl_fpga_cdev on probing ? (The meaning of released_port_num likely needs to change there as well) So in this case you could check if this was the new framework before doing the bar check. Similar checks other places where ofs stuff is being fit it. My concern is the fitting in without checking will break the old stuff. And why I wanted to see a probing writeup in the dfl.rst doc Tom > >> >>>> Move up a line and skip getting an offset that will not be used. >>> Yes, this line is not necessary, I will remove it on next version >>> patch. >>> >>>>> + dev_info(&pcidev->dev, "skipping port without >>>> local BAR space %d\n", >>>>> + bar); >>>>> + continue; >>>>> + } else { >>>>> + dev_info(&pcidev->dev, "BAR %d offset %u\n", >>>> bar, offset); >>>>> + } >>>>> start = pci_resource_start(pcidev, bar) + offset; >>>>> len = pci_resource_len(pcidev, bar) - offset; >>>>> >>>> Is similar logic needed for else-if (port) block below this ? >>> I think, the else-if is not necessary. I will remove it on next >>> version patch. >>>> Tom >> >> >
> -----Original Message----- > From: Tom Rix <trix@redhat.com> > Sent: Tuesday, February 22, 2022 1:52 AM > To: matthew.gerlach@linux.intel.com > Cc: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>; > mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; linux-fpga@vger.kernel.org; > linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; corbet@lwn.net > Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no local bar space. > > > On 2/21/22 9:22 AM, matthew.gerlach@linux.intel.com wrote: > > > > > > On Fri, 18 Feb 2022, Tom Rix wrote: > > > >> > >> On 2/17/22 11:31 PM, Zhang, Tianfei wrote: > >>> > >>>> -----Original Message----- > >>>> From: Tom Rix <trix@redhat.com> > >>>> Sent: Tuesday, February 15, 2022 11:06 PM > >>>> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao > >>>> <hao.wu@intel.com>; mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; > >>>> linux-fpga@vger.kernel.org; linux-doc@vger.kernel.org; > >>>> linux-kernel@vger.kernel.org > >>>> Cc: corbet@lwn.net; Matthew Gerlach > >>>> <matthew.gerlach@linux.intel.com> > >>>> Subject: Re: [PATCH v1 3/7] fpga: dfl: Allow for ports with no > >>>> local bar space. > >>>> > >>>> > >>>> On 2/14/22 3:26 AM, Tianfei zhang wrote: > >>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > >>>>> > >>>>> From a fpga partial reconfiguration standpoint, a port may not > >>>>> be connected any local BAR space. The port could be connected to > >>>>> a different PCIe Physical Function (PF) or Virtual Function (VF), > >>>>> in which case another driver instance would manage the endpoint. > >>>> It is not clear if this is part of iofs or a bug fix. > >>> This is the new implementation/feature of IOFS. > >>> On IOFS support multiple methods to access the AFU. > >>> 1. Legacy Model. This is used for N3000 and N5000 card. > >>> In this model the entire AFU region is a unit of PR, and there is a > >>> Port device connected to this AFU. > >>> On DFL perspective, there is "Next AFU" point to the AFU, and the > >>> "BarID" is the PCIe Bar ID of AFU. > >>> In this model, we can use the AFU APIs to access the entire AFU > >>> resource, like MMIO. > >>> 2. Micro-Personas in AFU. > >>> IOFS intruding new model for PR and AFU access. > >>> Micro-Personas allow the RTL developer to designate their own > >>> AFU-defined PR regions. > >>> In this model the unit of PR is not the entire AFU, instead the unit > >>> of PR can be any size block or blocks inside the AFU. > >>> 3. Multiple VFs per PR slot. > >>> In this method, we can instance multiple VFs over SRIOV for one PR > >>> slot, and access the AFU resource by different VFs in virtualization > >>> usage. In this case, the Port device would not connected to AFU (the > >>> BarID of Port device should be set to invalid), so this patch want > >>> to support this use model. > >> > >> What I am looking for is how the older cards using (my term) dfl 1 > >> will still work with dfl 2 and vice versa. > >> > >> No where do I see a version check for dfl 2 nor a pci id check so > >> either this just works or backward compatibility has not be considered. > >> > >> Please add a backward compatibility section to the doc patch > >> > >>> > >>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > >>>>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> > >>>>> --- > >>>>> drivers/fpga/dfl-pci.c | 8 ++++++++ > >>>>> 1 file changed, 8 insertions(+) > >>>>> > >>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index > >>>>> 4d68719e608f..8abd9b408403 100644 > >>>>> --- a/drivers/fpga/dfl-pci.c > >>>>> +++ b/drivers/fpga/dfl-pci.c > >>>>> @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev > >>>>> *pcidev, > >>>>> v = readq(base + FME_HDR_CAP); > >>>>> port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); > >>>>> > >>>>> + dev_info(&pcidev->dev, "port_num = %d\n", port_num); > >>>>> WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); > >>>>> > >>>>> for (i = 0; i < port_num; i++) { @@ -258,6 +259,13 @@ > >>>>> static int find_dfls_by_default(struct pci_dev *pcidev, > >>>>> */ > >>>>> bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); > >>>>> offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); > >>>>> + if (bar >= PCI_STD_NUM_BARS) { > >>>> Is bar set to a better magic number that pci_std_num_bars ? maybe > >>>> 0xff's > >>>> > >>>> How do you tell between this case and broken hw ? > >>> Yes, I agree that magic number is better, Currently the RTL using > >>> PCI_STD_NUM_BARS for an invalid PCIe bar number. > >> > >> How do you tell between this case and broken hw ? > >> > >> Tom > > > > The field, FME_PORT_OFST_BAR_ID, is a three bit field, which is pretty > > common for BARs on PCI. PCI_STD_NUM_BARS is defined as 6. Current HW > > implementations are filing this field with the value, 7, which is > > close to the suggestion of 0xff's. How about we define the following > > magic number? > > #define NO_LOCAL_PORT_BAR 7 > > > > We should also change the dev_info to be a dev_dbg and more precise to > > something like the following: > > > > if (bar == NO_LOCAL_PORT_BAR) { > > dev_dbg(&pcidev->dev, "No local port BAR space.\n"); > > continue; > > } > > What I am looking for is way generally is to tell if this is an old framework or a > new one. > > Maybe a flag and/or version added to dfl_fpga_cdev on probing ? I am agree add " features" in dfl_fpga_cdev on probing, for example: struct dfl_fpga_cdev { ... #define DFL_FEAT_xxxx (1<<0) u64 features; }; > > (The meaning of released_port_num likely needs to change there as well) > > So in this case you could check if this was the new framework before doing the > bar check. > > Similar checks other places where ofs stuff is being fit it. > > My concern is the fitting in without checking will break the old stuff. > > And why I wanted to see a probing writeup in the dfl.rst doc > > Tom > > > > >> > >>>> Move up a line and skip getting an offset that will not be used. > >>> Yes, this line is not necessary, I will remove it on next version > >>> patch. > >>> > >>>>> + dev_info(&pcidev->dev, "skipping port without > >>>> local BAR space %d\n", > >>>>> + bar); > >>>>> + continue; > >>>>> + } else { > >>>>> + dev_info(&pcidev->dev, "BAR %d offset %u\n", > >>>> bar, offset); > >>>>> + } > >>>>> start = pci_resource_start(pcidev, bar) + offset; > >>>>> len = pci_resource_len(pcidev, bar) - offset; > >>>>> > >>>> Is similar logic needed for else-if (port) block below this ? > >>> I think, the else-if is not necessary. I will remove it on next > >>> version patch. > >>>> Tom > >> > >> > >
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index 4d68719e608f..8abd9b408403 100644 --- a/drivers/fpga/dfl-pci.c +++ b/drivers/fpga/dfl-pci.c @@ -243,6 +243,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev, v = readq(base + FME_HDR_CAP); port_num = FIELD_GET(FME_CAP_NUM_PORTS, v); + dev_info(&pcidev->dev, "port_num = %d\n", port_num); WARN_ON(port_num > MAX_DFL_FPGA_PORT_NUM); for (i = 0; i < port_num; i++) { @@ -258,6 +259,13 @@ static int find_dfls_by_default(struct pci_dev *pcidev, */ bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v); offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v); + if (bar >= PCI_STD_NUM_BARS) { + dev_info(&pcidev->dev, "skipping port without local BAR space %d\n", + bar); + continue; + } else { + dev_info(&pcidev->dev, "BAR %d offset %u\n", bar, offset); + } start = pci_resource_start(pcidev, bar) + offset; len = pci_resource_len(pcidev, bar) - offset;