Re: [PATCH v4 3/4] fpga: dfl: add basic support DFHv1

[matthew . gerlach]

On Sat, 29 Oct 2022, matthew.gerlach@xxxxxxxxxxxxxxx wrote:

> > >  	if (!finfo)
> > >  		return -ENOMEM;
> > >
> > > +	if (dfh_psize > 0) {
> > > +		memcpy_fromio(finfo->params,
> > > +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, 
> > > dfh_psize);
> > > +		finfo->param_size = dfh_psize;
> > > +	}
> > > +
> > >  	finfo->fid = fid;
> > >  	finfo->revision = revision;
> > > -	finfo->mmio_res.start = binfo->start + ofst;
> > > -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > > +	finfo->dfh_version = dfh_ver;
> > >  	finfo->mmio_res.flags = IORESOURCE_MEM;
> > > -	finfo->irq_base = irq_base;
> > > -	finfo->nr_irqs = nr_irqs;
> > > +	if (dfh_ver == 1) {
> > > +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> > > +		if (v & DFHv1_CSR_ADDR_REL)
> > > +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> > > +		else
> > > +			finfo->mmio_res.start = binfo->start + ofst +
> > > +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, 
> > > v);
> > > +
> > > +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> > > +		finfo->mmio_res.end = finfo->mmio_res.start +
> > > +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 
> > > 1;
> >
> > So for dflv1, no feature header resource for dfl_device, is it a problem
> > for dfl_uio? Does userspace driver need the raw feature header?
> These are two very good questions.  The dfl_uio driver question is 
> particularly relevent because user space is looking at the GUIDs.

In the case of dfl_uio driver, user space will definitely want to look at 
the feature header for the GUID and the parameters.  Since DFHv1 can have 
the DFH header and the feature registers in non-contiguous memory 
locations, a resource for the dfl_device will be required.  In earlier
revisions of this patch set, a second resource was added called csr_res 
pointing to the feature's register while mmio_res pointed at the header.
Do we just need better names or do we need an array of named resources?

> > > +	} else {
> > > +		finfo->mmio_res.start = binfo->start + ofst;
> > > +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > > +	}
> > > +
> > > +	ret = parse_feature_irqs(binfo, ofst, finfo);
> > > +	if (ret) {
> > > +		kfree(finfo);
> > > +		return ret;
> > > +	}