[v6,1/6] fpga: dfl: Allow ports without local bar space.

Commit Message

Zhang, Tianfei March 16, 2022, 7:08 a.m. UTC

From: Matthew Gerlach <[e-mail]>

In OFS, each PR slot (AFU) has one port device which include Port
control, Port user clock control and Port errors. In legacy model,
the AFU MMIO space was connected with Port device, so from port
device point of view, there is a bar space associated with this
port device. But in "Multiple VFs per PR slot" model, the AFU MMIO
space was not connected with Port device. The BarID (3bits field) in
PORTn_OFFSET register indicates which PCI bar space associated with
this port device, the value 0b111 (FME_HDR_NO_PORT_BAR) means that
no PCI bar for this port device.

---
v3: add PCI bar number checking with PCI_STD_NUM_BARS.
v2: use FME_HDR_NO_PORT_BAR instead of PCI_STD_NUM_BARS.

Signed-off-by: Matthew Gerlach <[e-mail]>
Signed-off-by: Tianfei Zhang <[e-mail]>
---
 drivers/fpga/dfl-pci.c | 7 +++++++
 drivers/fpga/dfl.h     | 1 +
 2 files changed, 8 insertions(+)

Comments

Wu, Hao March 17, 2022, 2:04 a.m. UTC | #1

> -----Original Message-----
> From: Zhang, Tianfei <[e-mail]>
> Sent: Wednesday, March 16, 2022 3:08 PM
> To: Wu, Hao <[e-mail]>; [e-mail]; [e-mail]; Xu, Yilun
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; Matthew Gerlach <[e-mail]>;
> Zhang, Tianfei <[e-mail]>
> Subject: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> 
> From: Matthew Gerlach <[e-mail]>
> 
> In OFS, each PR slot (AFU) has one port device which include Port
> control, Port user clock control and Port errors. In legacy model,
> the AFU MMIO space was connected with Port device, so from port
> device point of view, there is a bar space associated with this
> port device. But in "Multiple VFs per PR slot" model, the AFU MMIO
> space was not connected with Port device. The BarID (3bits field) in
> PORTn_OFFSET register indicates which PCI bar space associated with
> this port device, the value 0b111 (FME_HDR_NO_PORT_BAR) means that
> no PCI bar for this port device.

The commit message is not matching the change, it's not related to AFU...

Current usage (FME DFL and PORT DFL are not linked together)

FME DFL 
PORT DFL (located by FME's PORTn_OFFSET register, BAR + offset)

Your proposed new usage is (FME DFL and PORT DFL are linked together)

FME DFL -> PORT DFL
So FME's PORTn_OFFSET can be marked, then driver could skip it.

Is my understanding correct? If yes, please update your title and commit
message, and add some comments in code as well.

Again, the change you did in dfl core code, is not only impacting your
OFS device, but also future DFL devices, it's an extension to DFL.

Thanks
Hao

> 
> ---
> v3: add PCI bar number checking with PCI_STD_NUM_BARS.
> v2: use FME_HDR_NO_PORT_BAR instead of PCI_STD_NUM_BARS.
> 
> Signed-off-by: Matthew Gerlach <[e-mail]>
> Signed-off-by: Tianfei Zhang <[e-mail]>
> ---
>  drivers/fpga/dfl-pci.c | 7 +++++++
>  drivers/fpga/dfl.h     | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index 4d68719e608f..2e9abeca3625 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -258,6 +258,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 ||
> +			    bar == FME_HDR_NO_PORT_BAR) {
> +				dev_dbg(&pcidev->dev, "skipping port without
> local BAR space %d\n",
> +					bar);
> +				continue;
> +			}
> +
>  			start = pci_resource_start(pcidev, bar) + offset;
>  			len = pci_resource_len(pcidev, bar) - offset;
> 
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 53572c7aced0..1fd493e82dd8 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -91,6 +91,7 @@
>  #define FME_HDR_PORT_OFST(n)	(0x38 + ((n) * 0x8))
>  #define FME_HDR_BITSTREAM_ID	0x60
>  #define FME_HDR_BITSTREAM_MD	0x68
> +#define FME_HDR_NO_PORT_BAR	7
> 
>  /* FME Fab Capability Register Bitfield */
>  #define FME_CAP_FABRIC_VERID	GENMASK_ULL(7, 0)	/* Fabric
> version ID */
> --
> 2.26.2

Zhang, Tianfei March 17, 2022, 7:34 a.m. UTC | #2

> -----Original Message-----
> From: Wu, Hao <[e-mail]>
> Sent: Thursday, March 17, 2022 10:05 AM
> To: Zhang, Tianfei <[e-mail]>; [e-mail];
> [e-mail]; Xu, Yilun <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Cc: [e-mail]; Matthew Gerlach <[e-mail]>
> Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> 
> > -----Original Message-----
> > From: Zhang, Tianfei <[e-mail]>
> > Sent: Wednesday, March 16, 2022 3:08 PM
> > To: Wu, Hao <[e-mail]>; [e-mail]; [e-mail]; Xu,
> > Yilun <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]
> > Cc: [e-mail]; Matthew Gerlach <[e-mail]>;
> > Zhang, Tianfei <[e-mail]>
> > Subject: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> >
> > From: Matthew Gerlach <[e-mail]>
> >
> > In OFS, each PR slot (AFU) has one port device which include Port
> > control, Port user clock control and Port errors. In legacy model, the
> > AFU MMIO space was connected with Port device, so from port device
> > point of view, there is a bar space associated with this port device.
> > But in "Multiple VFs per PR slot" model, the AFU MMIO space was not
> > connected with Port device. The BarID (3bits field) in PORTn_OFFSET
> > register indicates which PCI bar space associated with this port
> > device, the value 0b111 (FME_HDR_NO_PORT_BAR) means that no PCI bar
> > for this port device.
> 
> The commit message is not matching the change, it's not related to AFU...
> 
> Current usage (FME DFL and PORT DFL are not linked together)

This usage is only on Intel PAC N3000 and N5000 card. 
In my understand, the space of Port can put into any PCI bar space. 
In the previous use case, the space of port was located on Bar 2.
For OFS, it allows the port without specific bar space.

> 
> FME DFL
> PORT DFL (located by FME's PORTn_OFFSET register, BAR + offset)
> 
> Your proposed new usage is (FME DFL and PORT DFL are linked together)
> 
> FME DFL -> PORT DFL
> So FME's PORTn_OFFSET can be marked, then driver could skip it.
> 
> Is my understanding correct? If yes, please update your title and commit
> message, and add some comments in code as well.

From DLF perspective, I think it is yes.

How about the title:  "fpga: dfl: Allow Port and FME's DFL link together" ?

I will also add some comments in code.
Here is the new git commit for this patch, any comments? 

In previous FPGA platform like Intel PAC N3000 and N5000, The BarID (3bits field) in PORTn_OFFSET
register indicated which PCI bar space was associated with this port device. In this case, the DFL of Port device
was located in the specific PCI bar space, and then the FME and Port's DFL were not linked. But in OFS, we extend
the usage, it allows the FME and Port's DFL  linked together when there was no local PCI bar space specified by 
the Port device. The value 0b111 (FME_HDR_NO_PORT_BAR) of BarID means that no specific PCI bar space 
was associated with the port device.

> 
> Again, the change you did in dfl core code, is not only impacting your OFS
> device, but also future DFL devices, it's an extension to DFL.

Yes, I agree that is an extended usage.

> 
> Thanks
> Hao
> 
> >
> > ---
> > v3: add PCI bar number checking with PCI_STD_NUM_BARS.
> > v2: use FME_HDR_NO_PORT_BAR instead of PCI_STD_NUM_BARS.
> >
> > Signed-off-by: Matthew Gerlach <[e-mail]>
> > Signed-off-by: Tianfei Zhang <[e-mail]>
> > ---
> >  drivers/fpga/dfl-pci.c | 7 +++++++
> >  drivers/fpga/dfl.h     | 1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
> > 4d68719e608f..2e9abeca3625 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -258,6 +258,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 ||
> > +			    bar == FME_HDR_NO_PORT_BAR) {
> > +				dev_dbg(&pcidev->dev, "skipping port without
> > local BAR space %d\n",
> > +					bar);
> > +				continue;
> > +			}
> > +
> >  			start = pci_resource_start(pcidev, bar) + offset;
> >  			len = pci_resource_len(pcidev, bar) - offset;
> >
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h index
> > 53572c7aced0..1fd493e82dd8 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -91,6 +91,7 @@
> >  #define FME_HDR_PORT_OFST(n)	(0x38 + ((n) * 0x8))
> >  #define FME_HDR_BITSTREAM_ID	0x60
> >  #define FME_HDR_BITSTREAM_MD	0x68
> > +#define FME_HDR_NO_PORT_BAR	7
> >
> >  /* FME Fab Capability Register Bitfield */
> >  #define FME_CAP_FABRIC_VERID	GENMASK_ULL(7, 0)	/* Fabric
> > version ID */
> > --
> > 2.26.2

Wu, Hao March 17, 2022, 8:17 a.m. UTC | #3

> > -----Original Message-----
> > From: Wu, Hao <[e-mail]>
> > Sent: Thursday, March 17, 2022 10:05 AM
> > To: Zhang, Tianfei <[e-mail]>; [e-mail];
> > [e-mail]; Xu, Yilun <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]
> > Cc: [e-mail]; Matthew Gerlach <[e-mail]>
> > Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> >
> > > -----Original Message-----
> > > From: Zhang, Tianfei <[e-mail]>
> > > Sent: Wednesday, March 16, 2022 3:08 PM
> > > To: Wu, Hao <[e-mail]>; [e-mail]; [e-mail]; Xu,
> > > Yilun <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > Cc: [e-mail]; Matthew Gerlach <[e-mail]>;
> > > Zhang, Tianfei <[e-mail]>
> > > Subject: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> > >
> > > From: Matthew Gerlach <[e-mail]>
> > >
> > > In OFS, each PR slot (AFU) has one port device which include Port
> > > control, Port user clock control and Port errors. In legacy model, the
> > > AFU MMIO space was connected with Port device, so from port device
> > > point of view, there is a bar space associated with this port device.
> > > But in "Multiple VFs per PR slot" model, the AFU MMIO space was not
> > > connected with Port device. The BarID (3bits field) in PORTn_OFFSET
> > > register indicates which PCI bar space associated with this port
> > > device, the value 0b111 (FME_HDR_NO_PORT_BAR) means that no PCI bar
> > > for this port device.
> >
> > The commit message is not matching the change, it's not related to AFU...
> >
> > Current usage (FME DFL and PORT DFL are not linked together)
> 
> This usage is only on Intel PAC N3000 and N5000 card.
> In my understand, the space of Port can put into any PCI bar space.
> In the previous use case, the space of port was located on Bar 2.
> For OFS, it allows the port without specific bar space.

I didn't understand what you mean. Without your change, existing
driver supports Port in any BAR indicated by PORTn_OFFSET, it's fine
you put Port to BAR 0, or same BAR as FME. What do you mean by
"port without specific bar space"?

> 
> >
> > FME DFL
> > PORT DFL (located by FME's PORTn_OFFSET register, BAR + offset)
> >
> > Your proposed new usage is (FME DFL and PORT DFL are linked together)
> >
> > FME DFL -> PORT DFL
> > So FME's PORTn_OFFSET can be marked, then driver could skip it.
> >
> > Is my understanding correct? If yes, please update your title and commit
> > message, and add some comments in code as well.
> 
> From DLF perspective, I think it is yes.
> 
> How about the title:  "fpga: dfl: Allow Port and FME's DFL link together" ?

"Allow Port to be linked to FME's DFL" should be better, as we don't
encourage that people to connect FME DFL to Port DFL or any mixed order.

> 
> I will also add some comments in code.
> Here is the new git commit for this patch, any comments?
> 
> In previous FPGA platform like Intel PAC N3000 and N5000, The BarID (3bits field)
> in PORTn_OFFSET
> register indicated which PCI bar space was associated with this port device. In
> this case, the DFL of Port device
> was located in the specific PCI bar space, and then the FME and Port's DFL were
> not linked. But in OFS, we extend
> the usage, it allows the FME and Port's DFL  linked together when there was no
> local PCI bar space specified by
> the Port device. The value 0b111 (FME_HDR_NO_PORT_BAR) of BarID means
> that no specific PCI bar space
> was associated with the port device.

Currently we use PORTn_OFFSET to locate PORT DFLs, and PORT DFLs are not
connected FME DFL. But for some cases (e.g. Intel Open FPGA Stack device), 
PORT DFLs are connected to FME DFL directly, so we don't need to search
PORT DFLs via PORTn_OFFSET again. If BAR value of PORTn_OFFSET is 0x7
(FME_PORT_OFST_BAR_SKIP/INVALID - depends the description added to
DFL spec) then driver will skip searching the DFL for that port.

> 
> >
> > Again, the change you did in dfl core code, is not only impacting your OFS
> > device, but also future DFL devices, it's an extension to DFL.
> 
> Yes, I agree that is an extended usage.

Please make sure related changes documented in DFL spec as well.

> 
> >
> > Thanks
> > Hao
> >
> > >
> > > ---
> > > v3: add PCI bar number checking with PCI_STD_NUM_BARS.
> > > v2: use FME_HDR_NO_PORT_BAR instead of PCI_STD_NUM_BARS.
> > >
> > > Signed-off-by: Matthew Gerlach <[e-mail]>
> > > Signed-off-by: Tianfei Zhang <[e-mail]>
> > > ---
> > >  drivers/fpga/dfl-pci.c | 7 +++++++
> > >  drivers/fpga/dfl.h     | 1 +
> > >  2 files changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
> > > 4d68719e608f..2e9abeca3625 100644
> > > --- a/drivers/fpga/dfl-pci.c
> > > +++ b/drivers/fpga/dfl-pci.c
> > > @@ -258,6 +258,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 ||
> > > +			    bar == FME_HDR_NO_PORT_BAR) {
> > > +				dev_dbg(&pcidev->dev, "skipping port without
> > > local BAR space %d\n",
> > > +					bar);
> > > +				continue;
> > > +			}
> > > +
> > >  			start = pci_resource_start(pcidev, bar) + offset;
> > >  			len = pci_resource_len(pcidev, bar) - offset;
> > >
> > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h index
> > > 53572c7aced0..1fd493e82dd8 100644
> > > --- a/drivers/fpga/dfl.h
> > > +++ b/drivers/fpga/dfl.h
> > > @@ -91,6 +91,7 @@
> > >  #define FME_HDR_PORT_OFST(n)	(0x38 + ((n) * 0x8))
> > >  #define FME_HDR_BITSTREAM_ID	0x60
> > >  #define FME_HDR_BITSTREAM_MD	0x68
> > > +#define FME_HDR_NO_PORT_BAR	7
> > >
> > >  /* FME Fab Capability Register Bitfield */
> > >  #define FME_CAP_FABRIC_VERID	GENMASK_ULL(7, 0)	/* Fabric
> > > version ID */
> > > --
> > > 2.26.2

Zhang, Tianfei March 17, 2022, 8:32 a.m. UTC | #4

> -----Original Message-----
> From: Wu, Hao <[e-mail]>
> Sent: Thursday, March 17, 2022 4:18 PM
> To: Zhang, Tianfei <[e-mail]>; [e-mail];
> [e-mail]; Xu, Yilun <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Cc: [e-mail]; Matthew Gerlach <[e-mail]>
> Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> 
> > > -----Original Message-----
> > > From: Wu, Hao <[e-mail]>
> > > Sent: Thursday, March 17, 2022 10:05 AM
> > > To: Zhang, Tianfei <[e-mail]>; [e-mail];
> > > [e-mail]; Xu, Yilun <[e-mail]>;
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]
> > > Cc: [e-mail]; Matthew Gerlach
> > > <[e-mail]>
> > > Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Tianfei <[e-mail]>
> > > > Sent: Wednesday, March 16, 2022 3:08 PM
> > > > To: Wu, Hao <[e-mail]>; [e-mail]; [e-mail];
> > > > Xu, Yilun <[e-mail]>; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]
> > > > Cc: [e-mail]; Matthew Gerlach
> > > > <[e-mail]>;
> > > > Zhang, Tianfei <[e-mail]>
> > > > Subject: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> > > >
> > > > From: Matthew Gerlach <[e-mail]>
> > > >
> > > > In OFS, each PR slot (AFU) has one port device which include Port
> > > > control, Port user clock control and Port errors. In legacy model,
> > > > the AFU MMIO space was connected with Port device, so from port
> > > > device point of view, there is a bar space associated with this port device.
> > > > But in "Multiple VFs per PR slot" model, the AFU MMIO space was
> > > > not connected with Port device. The BarID (3bits field) in
> > > > PORTn_OFFSET register indicates which PCI bar space associated
> > > > with this port device, the value 0b111 (FME_HDR_NO_PORT_BAR) means
> > > > that no PCI bar for this port device.
> > >
> > > The commit message is not matching the change, it's not related to AFU...
> > >
> > > Current usage (FME DFL and PORT DFL are not linked together)
> >
> > This usage is only on Intel PAC N3000 and N5000 card.
> > In my understand, the space of Port can put into any PCI bar space.
> > In the previous use case, the space of port was located on Bar 2.
> > For OFS, it allows the port without specific bar space.
> 
> I didn't understand what you mean. Without your change, existing driver
> supports Port in any BAR indicated by PORTn_OFFSET, it's fine you put Port to
> BAR 0, or same BAR as FME. What do you mean by "port without specific bar
> space"?

"port with specific bar space" means that the port has a dedicated bar space, including the DFL, AFU, this is use 
case in N3000/N5000 card.

"port without specific bar space" means the port without specific bar space, and the Port linked with FME for DFL
perspective.

> 
> >
> > >
> > > FME DFL
> > > PORT DFL (located by FME's PORTn_OFFSET register, BAR + offset)
> > >
> > > Your proposed new usage is (FME DFL and PORT DFL are linked
> > > together)
> > >
> > > FME DFL -> PORT DFL
> > > So FME's PORTn_OFFSET can be marked, then driver could skip it.
> > >
> > > Is my understanding correct? If yes, please update your title and
> > > commit message, and add some comments in code as well.
> >
> > From DLF perspective, I think it is yes.
> >
> > How about the title:  "fpga: dfl: Allow Port and FME's DFL link together" ?
> 
> "Allow Port to be linked to FME's DFL" should be better, as we don't encourage
> that people to connect FME DFL to Port DFL or any mixed order.

Looks good.

> 
> >
> > I will also add some comments in code.
> > Here is the new git commit for this patch, any comments?
> >
> > In previous FPGA platform like Intel PAC N3000 and N5000, The BarID
> > (3bits field) in PORTn_OFFSET register indicated which PCI bar space
> > was associated with this port device. In this case, the DFL of Port
> > device was located in the specific PCI bar space, and then the FME and
> > Port's DFL were not linked. But in OFS, we extend the usage, it allows
> > the FME and Port's DFL  linked together when there was no local PCI
> > bar space specified by the Port device. The value 0b111
> > (FME_HDR_NO_PORT_BAR) of BarID means that no specific PCI bar space
> > was associated with the port device.
> 
> Currently we use PORTn_OFFSET to locate PORT DFLs, and PORT DFLs are not
> connected FME DFL. But for some cases (e.g. Intel Open FPGA Stack device),
> PORT DFLs are connected to FME DFL directly, so we don't need to search PORT
> DFLs via PORTn_OFFSET again. If BAR value of PORTn_OFFSET is 0x7
> (FME_PORT_OFST_BAR_SKIP/INVALID - depends the description added to DFL
> spec) then driver will skip searching the DFL for that port.

It is good for me.

> 
> >
> > >
> > > Again, the change you did in dfl core code, is not only impacting
> > > your OFS device, but also future DFL devices, it's an extension to DFL.
> >
> > Yes, I agree that is an extended usage.
> 
> Please make sure related changes documented in DFL spec as well.

I will add it on documentation.

> 
> >
> > >
> > > Thanks
> > > Hao
> > >
> > > >
> > > > ---
> > > > v3: add PCI bar number checking with PCI_STD_NUM_BARS.
> > > > v2: use FME_HDR_NO_PORT_BAR instead of PCI_STD_NUM_BARS.
> > > >
> > > > Signed-off-by: Matthew Gerlach <[e-mail]>
> > > > Signed-off-by: Tianfei Zhang <[e-mail]>
> > > > ---
> > > >  drivers/fpga/dfl-pci.c | 7 +++++++
> > > >  drivers/fpga/dfl.h     | 1 +
> > > >  2 files changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index
> > > > 4d68719e608f..2e9abeca3625 100644
> > > > --- a/drivers/fpga/dfl-pci.c
> > > > +++ b/drivers/fpga/dfl-pci.c
> > > > @@ -258,6 +258,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 ||
> > > > +			    bar == FME_HDR_NO_PORT_BAR) {
> > > > +				dev_dbg(&pcidev->dev, "skipping port without
> > > > local BAR space %d\n",
> > > > +					bar);
> > > > +				continue;
> > > > +			}
> > > > +
> > > >  			start = pci_resource_start(pcidev, bar) + offset;
> > > >  			len = pci_resource_len(pcidev, bar) - offset;
> > > >
> > > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h index
> > > > 53572c7aced0..1fd493e82dd8 100644
> > > > --- a/drivers/fpga/dfl.h
> > > > +++ b/drivers/fpga/dfl.h
> > > > @@ -91,6 +91,7 @@
> > > >  #define FME_HDR_PORT_OFST(n)	(0x38 + ((n) * 0x8))
> > > >  #define FME_HDR_BITSTREAM_ID	0x60
> > > >  #define FME_HDR_BITSTREAM_MD	0x68
> > > > +#define FME_HDR_NO_PORT_BAR	7
> > > >
> > > >  /* FME Fab Capability Register Bitfield */
> > > >  #define FME_CAP_FABRIC_VERID	GENMASK_ULL(7, 0)	/* Fabric
> > > > version ID */
> > > > --
> > > > 2.26.2

Zhang, Tianfei April 13, 2022, 9:13 a.m. UTC | #5

> -----Original Message-----
> From: Zhang, Tianfei <[e-mail]>
> Sent: Thursday, March 17, 2022 4:33 PM
> To: Wu, Hao <[e-mail]>; [e-mail]; [e-mail]; Xu, Yilun
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; Matthew Gerlach <[e-mail]>
> Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> 
> 
> 
> > -----Original Message-----
> > From: Wu, Hao <[e-mail]>
> > Sent: Thursday, March 17, 2022 4:18 PM
> > To: Zhang, Tianfei <[e-mail]>; [e-mail];
> > [e-mail]; Xu, Yilun <[e-mail]>;
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]
> > Cc: [e-mail]; Matthew Gerlach <[e-mail]>
> > Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> >
> > > > -----Original Message-----
> > > > From: Wu, Hao <[e-mail]>
> > > > Sent: Thursday, March 17, 2022 10:05 AM
> > > > To: Zhang, Tianfei <[e-mail]>; [e-mail];
> > > > [e-mail]; Xu, Yilun <[e-mail]>;
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]
> > > > Cc: [e-mail]; Matthew Gerlach
> > > > <[e-mail]>
> > > > Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhang, Tianfei <[e-mail]>
> > > > > Sent: Wednesday, March 16, 2022 3:08 PM
> > > > > To: Wu, Hao <[e-mail]>; [e-mail]; [e-mail];
> > > > > Xu, Yilun <[e-mail]>; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]
> > > > > Cc: [e-mail]; Matthew Gerlach
> > > > > <[e-mail]>;
> > > > > Zhang, Tianfei <[e-mail]>
> > > > > Subject: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> > > > >
> > > > > From: Matthew Gerlach <[e-mail]>
> > > > >
> > > > > In OFS, each PR slot (AFU) has one port device which include
> > > > > Port control, Port user clock control and Port errors. In legacy
> > > > > model, the AFU MMIO space was connected with Port device, so
> > > > > from port device point of view, there is a bar space associated with this
> port device.
> > > > > But in "Multiple VFs per PR slot" model, the AFU MMIO space was
> > > > > not connected with Port device. The BarID (3bits field) in
> > > > > PORTn_OFFSET register indicates which PCI bar space associated
> > > > > with this port device, the value 0b111 (FME_HDR_NO_PORT_BAR)
> > > > > means that no PCI bar for this port device.
> > > >
> > > > The commit message is not matching the change, it's not related to AFU...
> > > >
> > > > Current usage (FME DFL and PORT DFL are not linked together)
> > >
> > > This usage is only on Intel PAC N3000 and N5000 card.
> > > In my understand, the space of Port can put into any PCI bar space.
> > > In the previous use case, the space of port was located on Bar 2.
> > > For OFS, it allows the port without specific bar space.
> >
> > I didn't understand what you mean. Without your change, existing
> > driver supports Port in any BAR indicated by PORTn_OFFSET, it's fine
> > you put Port to BAR 0, or same BAR as FME. What do you mean by "port
> > without specific bar space"?
> 
> "port with specific bar space" means that the port has a dedicated bar space,
> including the DFL, AFU, this is use case in N3000/N5000 card.
> 
> "port without specific bar space" means the port without specific bar space, and
> the Port linked with FME for DFL perspective.
> 
> >
> > >
> > > >
> > > > FME DFL
> > > > PORT DFL (located by FME's PORTn_OFFSET register, BAR + offset)
> > > >
> > > > Your proposed new usage is (FME DFL and PORT DFL are linked
> > > > together)
> > > >
> > > > FME DFL -> PORT DFL
> > > > So FME's PORTn_OFFSET can be marked, then driver could skip it.
> > > >
> > > > Is my understanding correct? If yes, please update your title and
> > > > commit message, and add some comments in code as well.
> > >
> > > From DLF perspective, I think it is yes.
> > >
> > > How about the title:  "fpga: dfl: Allow Port and FME's DFL link together" ?
> >
> > "Allow Port to be linked to FME's DFL" should be better, as we don't
> > encourage that people to connect FME DFL to Port DFL or any mixed order.
> 
> Looks good.
> 
> >
> > >
> > > I will also add some comments in code.
> > > Here is the new git commit for this patch, any comments?
> > >
> > > In previous FPGA platform like Intel PAC N3000 and N5000, The BarID
> > > (3bits field) in PORTn_OFFSET register indicated which PCI bar space
> > > was associated with this port device. In this case, the DFL of Port
> > > device was located in the specific PCI bar space, and then the FME
> > > and Port's DFL were not linked. But in OFS, we extend the usage, it
> > > allows the FME and Port's DFL  linked together when there was no
> > > local PCI bar space specified by the Port device. The value 0b111
> > > (FME_HDR_NO_PORT_BAR) of BarID means that no specific PCI bar space
> > > was associated with the port device.
> >
> > Currently we use PORTn_OFFSET to locate PORT DFLs, and PORT DFLs are
> > not connected FME DFL. But for some cases (e.g. Intel Open FPGA Stack
> > device), PORT DFLs are connected to FME DFL directly, so we don't need
> > to search PORT DFLs via PORTn_OFFSET again. If BAR value of
> > PORTn_OFFSET is 0x7 (FME_PORT_OFST_BAR_SKIP/INVALID - depends the
> > description added to DFL
> > spec) then driver will skip searching the DFL for that port.
>

I like to split the patch to a sperate patch, so this patchset will divide into 2, one for port bar, one for VF creation.

Patch

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 4d68719e608f..2e9abeca3625 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -258,6 +258,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 ||
+			    bar == FME_HDR_NO_PORT_BAR) {
+				dev_dbg(&pcidev->dev, "skipping port without local BAR space %d\n",
+					bar);
+				continue;
+			}
+
 			start = pci_resource_start(pcidev, bar) + offset;
 			len = pci_resource_len(pcidev, bar) - offset;
 
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 53572c7aced0..1fd493e82dd8 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -91,6 +91,7 @@ 
 #define FME_HDR_PORT_OFST(n)	(0x38 + ((n) * 0x8))
 #define FME_HDR_BITSTREAM_ID	0x60
 #define FME_HDR_BITSTREAM_MD	0x68
+#define FME_HDR_NO_PORT_BAR	7
 
 /* FME Fab Capability Register Bitfield */
 #define FME_CAP_FABRIC_VERID	GENMASK_ULL(7, 0)	/* Fabric version ID */