All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Wu, Hao" <hao.wu@intel.com>
To: "Moritz Fischer" <mdf@kernel.org>,
	"Martin Hundebøll" <martin@geanix.com>,
	"Xu, Yilun" <yilun.xu@intel.com>
Cc: "Tom Rix" <trix@redhat.com>, "Jean Delvare" <jdelvare@suse.com>,
	"Guenter Roeck" <linux@roeck-us.net>,
	"Lee Jones" <lee.jones@linaro.org>,
	"Mark Brown" <broonie@kernel.org>,
	"Martin Hundebøll" <mhu@silicom.dk>,
	"linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>
Subject: RE: [PATCH v2 2/5] fpga: dfl: expose feature revision from struct dfl_device
Date: Mon, 28 Jun 2021 03:38:35 +0000	[thread overview]
Message-ID: <DM6PR11MB3819DBB21CA55582FCF645FD85039@DM6PR11MB3819.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YNYt+Dl43zhkjIhI@epycbox.lan>

> On Fri, Jun 25, 2021 at 09:42:10AM +0200, Martin Hundebøll wrote:
> > From: Martin Hundebøll <mhu@silicom.dk>
> >
> > Drivers can make use of the feature field from the DFL header, but
> > shouldn't know about the header structure. To avoid exposing such info,
> > and to reduce the number of reads from the io-mem, the revision is added
> > to struct dfl_device.

DFL driver may need to access DFL header (if device specific fields added in
the future) but at least no need for common fields. It sounds a little confusing
to people, that the purpose of this patch is not to avoid exposing such info.

How about this one:

DFL device drivers have common need of checking revision information from
DFL header, as well as other DFL common information like feature id and type.
So this patch exposes revision information directly via DFL device data structure.
As DFL core code has already read DFL header, so this patch saves additional DFL
header mmio reads from DFL device drivers too.

Other places look good to me.

Thanks
Hao

> >
> > Signed-off-by: Martin Hundebøll <mhu@silicom.dk>
> > ---
> >
> > Changes since v1:
> >  * This patch replaces the previous patch 2 and exposes the feature
> >    revision through struct dfl_device instead of a helper reading from
> >    io-mem
> >
> >  drivers/fpga/dfl.c  | 27 +++++++++++++++++----------
> >  drivers/fpga/dfl.h  |  1 +
> >  include/linux/dfl.h |  1 +
> >  3 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 511b20ff35a3..9381c579d1cd 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -381,6 +381,7 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
> >
> >  	ddev->type = feature_dev_id_type(pdev);
> >  	ddev->feature_id = feature->id;
> > +	ddev->revision = feature->revision;
> >  	ddev->cdev = pdata->dfl_cdev;
> >
> >  	/* add mmio resource */
> > @@ -717,6 +718,7 @@ struct build_feature_devs_info {
> >   */
> >  struct dfl_feature_info {
> >  	u16 fid;
> > +	u8 rev;
> >  	struct resource mmio_res;
> >  	void __iomem *ioaddr;
> >  	struct list_head node;
> > @@ -796,6 +798,7 @@ static int build_info_commit_dev(struct
> build_feature_devs_info *binfo)
> >  		/* save resource information for each feature */
> >  		feature->dev = fdev;
> >  		feature->id = finfo->fid;
> > +		feature->revision = finfo->rev;
> >
> >  		/*
> >  		 * the FIU header feature has some fundamental functions
> (sriov
> > @@ -910,19 +913,17 @@ static void build_info_free(struct
> build_feature_devs_info *binfo)
> >  	devm_kfree(binfo->dev, binfo);
> >  }
> >
> > -static inline u32 feature_size(void __iomem *start)
> > +static inline u32 feature_size(u64 value)
> >  {
> > -	u64 v = readq(start + DFH);
> > -	u32 ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> > +	u32 ofst = FIELD_GET(DFH_NEXT_HDR_OFST, value);
> >  	/* workaround for private features with invalid size, use 4K instead */
> >  	return ofst ? ofst : 4096;
> >  }
> >
> > -static u16 feature_id(void __iomem *start)
> > +static u16 feature_id(u64 value)
> >  {
> > -	u64 v = readq(start + DFH);
> > -	u16 id = FIELD_GET(DFH_ID, v);
> > -	u8 type = FIELD_GET(DFH_TYPE, v);
> > +	u16 id = FIELD_GET(DFH_ID, value);
> > +	u8 type = FIELD_GET(DFH_TYPE, value);
> >
> >  	if (type == DFH_TYPE_FIU)
> >  		return FEATURE_ID_FIU_HEADER;
> > @@ -1021,10 +1022,15 @@ create_feature_instance(struct
> build_feature_devs_info *binfo,
> >  	unsigned int irq_base, nr_irqs;
> >  	struct dfl_feature_info *finfo;
> >  	int ret;
> > +	u8 rev;
> > +	u64 v;
> > +
> > +	v = readq(binfo->ioaddr + ofst);
> > +	rev = FIELD_GET(DFH_REVISION, v);
> >
> >  	/* read feature size and id if inputs are invalid */
> > -	size = size ? size : feature_size(binfo->ioaddr + ofst);
> > -	fid = fid ? fid : feature_id(binfo->ioaddr + ofst);
> > +	size = size ? size : feature_size(v);
> > +	fid = fid ? fid : feature_id(v);
> >
> >  	if (binfo->len - ofst < size)
> >  		return -EINVAL;
> > @@ -1038,6 +1044,7 @@ create_feature_instance(struct
> build_feature_devs_info *binfo,
> >  		return -ENOMEM;
> >
> >  	finfo->fid = fid;
> > +	finfo->rev = rev;
> >  	finfo->mmio_res.start = binfo->start + ofst;
> >  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> >  	finfo->mmio_res.flags = IORESOURCE_MEM;
> > @@ -1166,7 +1173,7 @@ static int parse_feature_private(struct
> build_feature_devs_info *binfo,
> >  {
> >  	if (!is_feature_dev_detected(binfo)) {
> >  		dev_err(binfo->dev, "the private feature 0x%x does not belong
> to any AFU.\n",
> > -			feature_id(binfo->ioaddr + ofst));
> > +			feature_id(readq(binfo->ioaddr + ofst)));
> >  		return -EINVAL;
> >  	}
> >
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 2b82c96ba56c..422157cfd742 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -243,6 +243,7 @@ struct dfl_feature_irq_ctx {
> >  struct dfl_feature {
> >  	struct platform_device *dev;
> >  	u16 id;
> > +	u8 revision;
> >  	int resource_index;
> >  	void __iomem *ioaddr;
> >  	struct dfl_feature_irq_ctx *irq_ctx;
> > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > index 6cc10982351a..431636a0dc78 100644
> > --- a/include/linux/dfl.h
> > +++ b/include/linux/dfl.h
> > @@ -38,6 +38,7 @@ struct dfl_device {
> >  	int id;
> >  	u16 type;
> >  	u16 feature_id;
> > +	u8 revision;
> >  	struct resource mmio_res;
> >  	int *irqs;
> >  	unsigned int num_irqs;
> > --
> > 2.31.0
> >
> Looks good to me, any concerns from Intel folks?
> 
> - Moritz

  reply	other threads:[~2021-06-28  3:38 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-25  7:42 [PATCH v2 0/5] fpga/mfd/hwmon: Initial support for Silicom N5010 PAC Martin Hundebøll
2021-06-25  7:42 ` [PATCH v2 1/5] fpga: dfl: pci: add device IDs for Silicom N501x PAC cards Martin Hundebøll
2021-06-25 18:43   ` Moritz Fischer
2021-06-25  7:42 ` [PATCH v2 2/5] fpga: dfl: expose feature revision from struct dfl_device Martin Hundebøll
2021-06-25 19:26   ` Moritz Fischer
2021-06-28  3:38     ` Wu, Hao [this message]
2021-06-25  7:42 ` [PATCH v2 3/5] spi: spi-altera-dfl: support n5010 feature revision Martin Hundebøll
2021-06-28  5:58   ` Xu Yilun
2021-06-28 17:39   ` Moritz Fischer
2021-06-29 11:35     ` Mark Brown
2021-06-29 11:49     ` Martin Hundebøll
2021-06-29 14:37       ` Wu, Hao
2021-06-29 22:30         ` matthew.gerlach
2021-06-25  7:42 ` [PATCH v2 4/5] mfd: intel-m10-bmc: add n5010 variant Martin Hundebøll
2021-06-25 18:45   ` Moritz Fischer
2021-06-29  1:39     ` Xu Yilun
2021-06-28  5:59   ` Xu Yilun
2021-06-28 10:33     ` Lee Jones
2021-06-30 10:57   ` Lee Jones
2021-06-25  7:42 ` [PATCH v2 5/5] hwmon: intel-m10-bmc-hwmon: add n5010 sensors Martin Hundebøll
2021-06-28  6:00   ` Xu Yilun
2021-06-28 14:11     ` Guenter Roeck
2021-06-28 16:35   ` Guenter Roeck
2021-06-28 17:28     ` Moritz Fischer
2021-06-29  1:40       ` Xu Yilun

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM6PR11MB3819DBB21CA55582FCF645FD85039@DM6PR11MB3819.namprd11.prod.outlook.com \
    --to=hao.wu@intel.com \
    --cc=broonie@kernel.org \
    --cc=jdelvare@suse.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-fpga@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=martin@geanix.com \
    --cc=mdf@kernel.org \
    --cc=mhu@silicom.dk \
    --cc=trix@redhat.com \
    --cc=yilun.xu@intel.com \
    --subject='RE: [PATCH v2 2/5] fpga: dfl: expose feature revision from struct dfl_device' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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.