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

[Zhang, Tianfei]

> -----Original Message-----
> From: Wu, Hao <hao.wu@xxxxxxxxx>
> Sent: Thursday, March 17, 2022 4:18 PM
> To: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>; trix@xxxxxxxxxx;
> mdf@xxxxxxxxxx; Xu, Yilun <yilun.xu@xxxxxxxxx>; linux-fpga@xxxxxxxxxxxxxxx;
> linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> rdunlap@xxxxxxxxxxxxx
> Cc: corbet@xxxxxxx; Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> 
> > > -----Original Message-----
> > > From: Wu, Hao <hao.wu@xxxxxxxxx>
> > > Sent: Thursday, March 17, 2022 10:05 AM
> > > To: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>; trix@xxxxxxxxxx;
> > > mdf@xxxxxxxxxx; Xu, Yilun <yilun.xu@xxxxxxxxx>;
> > > linux-fpga@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; rdunlap@xxxxxxxxxxxxx
> > > Cc: corbet@xxxxxxx; Matthew Gerlach
> > > <matthew.gerlach@xxxxxxxxxxxxxxx>
> > > Subject: RE: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>
> > > > Sent: Wednesday, March 16, 2022 3:08 PM
> > > > To: Wu, Hao <hao.wu@xxxxxxxxx>; trix@xxxxxxxxxx; mdf@xxxxxxxxxx;
> > > > Xu, Yilun <yilun.xu@xxxxxxxxx>; linux-fpga@xxxxxxxxxxxxxxx;
> > > > linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > > rdunlap@xxxxxxxxxxxxx
> > > > Cc: corbet@xxxxxxx; Matthew Gerlach
> > > > <matthew.gerlach@xxxxxxxxxxxxxxx>;
> > > > Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>
> > > > Subject: [PATCH v6 1/6] fpga: dfl: Allow ports without local bar space.
> > > >
> > > > From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > > >
> > > > 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 <matthew.gerlach@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Tianfei Zhang <tianfei.zhang@xxxxxxxxx>
> > > > ---
> > > >  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