[PATCH 0/4] DFL Module support

[PATCH 0/4] DFL Module support

[Moritz Fischer]

Hi Greg,

as requested resend of pull request as patches.

Note: I put this on a separate branch so Maintainers can pull the stable
tag to allow them to apply DFL based drivers through their trees
(krzk@ had asked for this when Xu intially posted his patchset, since
there was a driver for drivers/memory that uses this series).

Thanks,

Moritz

Xu Yilun (4):
  fpga: dfl: fix the definitions of type & feature_id for dfl devices
  fpga: dfl: move dfl_device_id to mod_devicetable.h
  fpga: dfl: add dfl bus support to MODULE_DEVICE_TABLE()
  fpga: dfl: move dfl bus related APIs to include/linux/dfl.h

 MAINTAINERS                       |  1 +
 drivers/fpga/dfl.c                |  4 +-
 drivers/fpga/dfl.h                | 85 +-----------------------------
 include/linux/dfl.h               | 86 +++++++++++++++++++++++++++++++
 include/linux/mod_devicetable.h   | 24 +++++++++
 scripts/mod/devicetable-offsets.c |  4 ++
 scripts/mod/file2alias.c          | 13 +++++
 7 files changed, 131 insertions(+), 86 deletions(-)
 create mode 100644 include/linux/dfl.h

-- 
2.29.2

[PATCH 1/4] fpga: dfl: fix the definitions of type & feature_id for dfl devices

[Moritz Fischer]

From: Xu Yilun <yilun.xu@intel.com>

The value of the field dfl_device.type comes from the 12 bits register
field DFH_ID according to DFL spec. So this patch changes the definition
of the type field to u16.

Also it is not necessary to illustrate the valid bits of the type field
in comments. Instead we should explicitly define the possible values in
the enumeration type for it, because they are shared by hardware spec.
We should not let the compiler decide these values.

Similar changes are also applied to dfl_device.feature_id.

This patch also fixed the MODALIAS format according to the changes
above.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/dfl.c |  3 +--
 drivers/fpga/dfl.h | 14 +++++++-------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b450870b75ed..5a6ba3b2fa05 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct dfl_device *ddev = to_dfl_dev(dev);
 
-	/* The type has 4 valid bits and feature_id has 12 valid bits */
-	return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
+	return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",
 			      ddev->type, ddev->feature_id);
 }
 
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 5dc758f655b7..ac373b1fcff9 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
  * enum dfl_id_type - define the DFL FIU types
  */
 enum dfl_id_type {
-	FME_ID,
-	PORT_ID,
+	FME_ID = 0,
+	PORT_ID = 1,
 	DFL_ID_MAX,
 };
 
 /**
  * struct dfl_device_id -  dfl device identifier
- * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type.
- * @feature_id: contains 12 bits feature identifier local to its DFL FIU type.
+ * @type: DFL FIU type of the device. See enum dfl_id_type.
+ * @feature_id: feature identifier local to its DFL FIU type.
  * @driver_data: driver specific data.
  */
 struct dfl_device_id {
-	u8 type;
+	u16 type;
 	u16 feature_id;
 	unsigned long driver_data;
 };
@@ -543,7 +543,7 @@ struct dfl_device_id {
  * @dev: generic device interface.
  * @id: id of the dfl device.
  * @type: type of DFL FIU of the device. See enum dfl_id_type.
- * @feature_id: 16 bits feature identifier local to its DFL FIU type.
+ * @feature_id: feature identifier local to its DFL FIU type.
  * @mmio_res: mmio resource of this dfl device.
  * @irqs: list of Linux IRQ numbers of this dfl device.
  * @num_irqs: number of IRQs supported by this dfl device.
@@ -553,7 +553,7 @@ struct dfl_device_id {
 struct dfl_device {
 	struct device dev;
 	int id;
-	u8 type;
+	u16 type;
 	u16 feature_id;
 	struct resource mmio_res;
 	int *irqs;
-- 
2.29.2

[PATCH 2/4] fpga: dfl: move dfl_device_id to mod_devicetable.h

[Moritz Fischer]

From: Xu Yilun <yilun.xu@intel.com>

In order to support MODULE_DEVICE_TABLE() for dfl device driver, this
patch moves struct dfl_device_id to mod_devicetable.h

Some brief description for DFL (Device Feature List) is added to make
the DFL known to the whole kernel.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Acked-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/dfl.h              | 13 +------------
 include/linux/mod_devicetable.h | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index ac373b1fcff9..549c7900dcfd 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -22,6 +22,7 @@
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/uuid.h>
@@ -525,18 +526,6 @@ enum dfl_id_type {
 	DFL_ID_MAX,
 };
 
-/**
- * struct dfl_device_id -  dfl device identifier
- * @type: DFL FIU type of the device. See enum dfl_id_type.
- * @feature_id: feature identifier local to its DFL FIU type.
- * @driver_data: driver specific data.
- */
-struct dfl_device_id {
-	u16 type;
-	u16 feature_id;
-	unsigned long driver_data;
-};
-
 /**
  * struct dfl_device - represent an dfl device on dfl bus
  *
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..e4870e5d3ea8 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -838,4 +838,28 @@ struct mhi_device_id {
 	kernel_ulong_t driver_data;
 };
 
+/*
+ * DFL (Device Feature List)
+ *
+ * DFL defines a linked list of feature headers within the device MMIO space to
+ * provide an extensible way of adding features. Software can walk through these
+ * predefined data structures to enumerate features. It is now used in the FPGA.
+ * See Documentation/fpga/dfl.rst for more information.
+ *
+ * The dfl bus type is introduced to match the individual feature devices (dfl
+ * devices) for specific dfl drivers.
+ */
+
+/**
+ * struct dfl_device_id -  dfl device identifier
+ * @type: DFL FIU type of the device. See enum dfl_id_type.
+ * @feature_id: feature identifier local to its DFL FIU type.
+ * @driver_data: driver specific data.
+ */
+struct dfl_device_id {
+	__u16 type;
+	__u16 feature_id;
+	unsigned long driver_data;
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
-- 
2.29.2

[PATCH 3/4] fpga: dfl: add dfl bus support to MODULE_DEVICE_TABLE()

[Moritz Fischer]

From: Xu Yilun <yilun.xu@intel.com>

Device Feature List (DFL) is a linked list of feature headers within the
device MMIO space. It is used by FPGA to enumerate multiple sub features
within it. Each feature can be uniquely identified by DFL type and
feature id, which can be read out from feature headers.

A dfl bus helps DFL framework modularize DFL device drivers for
different sub features. The dfl bus matches its devices and drivers by
DFL type and feature id.

This patch adds dfl bus support to MODULE_DEVICE_TABLE() by adding info
about struct dfl_device_id in devicetable-offsets.c and add a dfl entry
point in file2alias.c.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Acked-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 scripts/mod/devicetable-offsets.c |  4 ++++
 scripts/mod/file2alias.c          | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 27007c18e754..d8350eea6d1a 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -243,5 +243,9 @@ int main(void)
 	DEVID(mhi_device_id);
 	DEVID_FIELD(mhi_device_id, chan);
 
+	DEVID(dfl_device_id);
+	DEVID_FIELD(dfl_device_id, type);
+	DEVID_FIELD(dfl_device_id, feature_id);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2417dd1dee33..8a438c94dcd9 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1368,6 +1368,18 @@ static int do_mhi_entry(const char *filename, void *symval, char *alias)
 	return 1;
 }
 
+/* Looks like: dfl:tNfN */
+static int do_dfl_entry(const char *filename, void *symval, char *alias)
+{
+	DEF_FIELD(symval, dfl_device_id, type);
+	DEF_FIELD(symval, dfl_device_id, feature_id);
+
+	sprintf(alias, "dfl:t%04Xf%04X", type, feature_id);
+
+	add_wildcard(alias);
+	return 1;
+}
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
@@ -1442,6 +1454,7 @@ static const struct devtable devtable[] = {
 	{"tee", SIZE_tee_client_device_id, do_tee_entry},
 	{"wmi", SIZE_wmi_device_id, do_wmi_entry},
 	{"mhi", SIZE_mhi_device_id, do_mhi_entry},
+	{"dfl", SIZE_dfl_device_id, do_dfl_entry},
 };
 
 /* Create MODULE_ALIAS() statements.
-- 
2.29.2

[PATCH 4/4] fpga: dfl: move dfl bus related APIs to include/linux/dfl.h

[Moritz Fischer]

From: Xu Yilun <yilun.xu@intel.com>

Now the dfl drivers could be made as independent modules and put in
different folders according to their functionalities. In order for
scattered dfl device drivers to include dfl bus APIs, move the
dfl bus APIs to a new header file in the public folder.

[mdf@kernel.org: Fixed up header guards to match filename]
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Acked-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 MAINTAINERS         |  1 +
 drivers/fpga/dfl.c  |  1 +
 drivers/fpga/dfl.h  | 72 -------------------------------------
 include/linux/dfl.h | 86 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 72 deletions(-)
 create mode 100644 include/linux/dfl.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e73636b75f29..9bbb378308a8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6886,6 +6886,7 @@ S:	Maintained
 F:	Documentation/ABI/testing/sysfs-bus-dfl
 F:	Documentation/fpga/dfl.rst
 F:	drivers/fpga/dfl*
+F:	include/linux/dfl.h
 F:	include/uapi/linux/fpga-dfl.h
 
 FPGA MANAGER FRAMEWORK
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 5a6ba3b2fa05..511b20ff35a3 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -10,6 +10,7 @@
  *   Wu Hao <hao.wu@intel.com>
  *   Xiao Guangrong <guangrong.xiao@linux.intel.com>
  */
+#include <linux/dfl.h>
 #include <linux/fpga-dfl.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 549c7900dcfd..2b82c96ba56c 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -517,76 +517,4 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
 			       struct dfl_feature *feature,
 			       unsigned long arg);
 
-/**
- * enum dfl_id_type - define the DFL FIU types
- */
-enum dfl_id_type {
-	FME_ID = 0,
-	PORT_ID = 1,
-	DFL_ID_MAX,
-};
-
-/**
- * struct dfl_device - represent an dfl device on dfl bus
- *
- * @dev: generic device interface.
- * @id: id of the dfl device.
- * @type: type of DFL FIU of the device. See enum dfl_id_type.
- * @feature_id: feature identifier local to its DFL FIU type.
- * @mmio_res: mmio resource of this dfl device.
- * @irqs: list of Linux IRQ numbers of this dfl device.
- * @num_irqs: number of IRQs supported by this dfl device.
- * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
- * @id_entry: matched id entry in dfl driver's id table.
- */
-struct dfl_device {
-	struct device dev;
-	int id;
-	u16 type;
-	u16 feature_id;
-	struct resource mmio_res;
-	int *irqs;
-	unsigned int num_irqs;
-	struct dfl_fpga_cdev *cdev;
-	const struct dfl_device_id *id_entry;
-};
-
-/**
- * struct dfl_driver - represent an dfl device driver
- *
- * @drv: driver model structure.
- * @id_table: pointer to table of device IDs the driver is interested in.
- *	      { } member terminated.
- * @probe: mandatory callback for device binding.
- * @remove: callback for device unbinding.
- */
-struct dfl_driver {
-	struct device_driver drv;
-	const struct dfl_device_id *id_table;
-
-	int (*probe)(struct dfl_device *dfl_dev);
-	void (*remove)(struct dfl_device *dfl_dev);
-};
-
-#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
-#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
-
-/*
- * use a macro to avoid include chaining to get THIS_MODULE.
- */
-#define dfl_driver_register(drv) \
-	__dfl_driver_register(drv, THIS_MODULE)
-int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
-void dfl_driver_unregister(struct dfl_driver *dfl_drv);
-
-/*
- * module_dfl_driver() - Helper macro for drivers that don't do
- * anything special in module init/exit.  This eliminates a lot of
- * boilerplate.  Each module may only use this macro once, and
- * calling it replaces module_init() and module_exit().
- */
-#define module_dfl_driver(__dfl_driver) \
-	module_driver(__dfl_driver, dfl_driver_register, \
-		      dfl_driver_unregister)
-
 #endif /* __FPGA_DFL_H */
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
new file mode 100644
index 000000000000..6cc10982351a
--- /dev/null
+++ b/include/linux/dfl.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for DFL driver and device API
+ *
+ * Copyright (C) 2020 Intel Corporation, Inc.
+ */
+
+#ifndef __LINUX_DFL_H
+#define __LINUX_DFL_H
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+
+/**
+ * enum dfl_id_type - define the DFL FIU types
+ */
+enum dfl_id_type {
+	FME_ID = 0,
+	PORT_ID = 1,
+	DFL_ID_MAX,
+};
+
+/**
+ * struct dfl_device - represent an dfl device on dfl bus
+ *
+ * @dev: generic device interface.
+ * @id: id of the dfl device.
+ * @type: type of DFL FIU of the device. See enum dfl_id_type.
+ * @feature_id: feature identifier local to its DFL FIU type.
+ * @mmio_res: mmio resource of this dfl device.
+ * @irqs: list of Linux IRQ numbers of this dfl device.
+ * @num_irqs: number of IRQs supported by this dfl device.
+ * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
+ * @id_entry: matched id entry in dfl driver's id table.
+ */
+struct dfl_device {
+	struct device dev;
+	int id;
+	u16 type;
+	u16 feature_id;
+	struct resource mmio_res;
+	int *irqs;
+	unsigned int num_irqs;
+	struct dfl_fpga_cdev *cdev;
+	const struct dfl_device_id *id_entry;
+};
+
+/**
+ * struct dfl_driver - represent an dfl device driver
+ *
+ * @drv: driver model structure.
+ * @id_table: pointer to table of device IDs the driver is interested in.
+ *	      { } member terminated.
+ * @probe: mandatory callback for device binding.
+ * @remove: callback for device unbinding.
+ */
+struct dfl_driver {
+	struct device_driver drv;
+	const struct dfl_device_id *id_table;
+
+	int (*probe)(struct dfl_device *dfl_dev);
+	void (*remove)(struct dfl_device *dfl_dev);
+};
+
+#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
+#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
+
+/*
+ * use a macro to avoid include chaining to get THIS_MODULE.
+ */
+#define dfl_driver_register(drv) \
+	__dfl_driver_register(drv, THIS_MODULE)
+int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
+void dfl_driver_unregister(struct dfl_driver *dfl_drv);
+
+/*
+ * module_dfl_driver() - Helper macro for drivers that don't do
+ * anything special in module init/exit.  This eliminates a lot of
+ * boilerplate.  Each module may only use this macro once, and
+ * calling it replaces module_init() and module_exit().
+ */
+#define module_dfl_driver(__dfl_driver) \
+	module_driver(__dfl_driver, dfl_driver_register, \
+		      dfl_driver_unregister)
+
+#endif /* __LINUX_DFL_H */
-- 
2.29.2

Re: [PATCH 2/4] fpga: dfl: move dfl_device_id to mod_devicetable.h

[Greg KH]

On Mon, Nov 02, 2020 at 11:21:02PM -0800, Moritz Fischer wrote:
> From: Xu Yilun <yilun.xu@intel.com>
> 
> In order to support MODULE_DEVICE_TABLE() for dfl device driver, this
> patch moves struct dfl_device_id to mod_devicetable.h
> 
> Some brief description for DFL (Device Feature List) is added to make
> the DFL known to the whole kernel.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Acked-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  drivers/fpga/dfl.h              | 13 +------------
>  include/linux/mod_devicetable.h | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index ac373b1fcff9..549c7900dcfd 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -22,6 +22,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/iopoll.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/uuid.h>
> @@ -525,18 +526,6 @@ enum dfl_id_type {
>  	DFL_ID_MAX,
>  };
>  
> -/**
> - * struct dfl_device_id -  dfl device identifier
> - * @type: DFL FIU type of the device. See enum dfl_id_type.
> - * @feature_id: feature identifier local to its DFL FIU type.
> - * @driver_data: driver specific data.
> - */
> -struct dfl_device_id {
> -	u16 type;
> -	u16 feature_id;
> -	unsigned long driver_data;
> -};
> -
>  /**
>   * struct dfl_device - represent an dfl device on dfl bus
>   *
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5b08a473cdba..e4870e5d3ea8 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -838,4 +838,28 @@ struct mhi_device_id {
>  	kernel_ulong_t driver_data;
>  };
>  
> +/*
> + * DFL (Device Feature List)
> + *
> + * DFL defines a linked list of feature headers within the device MMIO space to
> + * provide an extensible way of adding features. Software can walk through these
> + * predefined data structures to enumerate features. It is now used in the FPGA.
> + * See Documentation/fpga/dfl.rst for more information.
> + *
> + * The dfl bus type is introduced to match the individual feature devices (dfl
> + * devices) for specific dfl drivers.
> + */
> +
> +/**
> + * struct dfl_device_id -  dfl device identifier
> + * @type: DFL FIU type of the device. See enum dfl_id_type.
> + * @feature_id: feature identifier local to its DFL FIU type.
> + * @driver_data: driver specific data.
> + */
> +struct dfl_device_id {
> +	__u16 type;
> +	__u16 feature_id;
> +	unsigned long driver_data;

This is the wrong type for driver_data now that it goes to userspace :(

{sigh}

Re: [PATCH 4/4] fpga: dfl: move dfl bus related APIs to include/linux/dfl.h

[Greg KH]

On Mon, Nov 02, 2020 at 11:21:04PM -0800, Moritz Fischer wrote:
> From: Xu Yilun <yilun.xu@intel.com>
> 
> Now the dfl drivers could be made as independent modules and put in
> different folders according to their functionalities. In order for
> scattered dfl device drivers to include dfl bus APIs, move the
> dfl bus APIs to a new header file in the public folder.
> 
> [mdf@kernel.org: Fixed up header guards to match filename]
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Acked-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  MAINTAINERS         |  1 +
>  drivers/fpga/dfl.c  |  1 +
>  drivers/fpga/dfl.h  | 72 -------------------------------------
>  include/linux/dfl.h | 86 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 88 insertions(+), 72 deletions(-)
>  create mode 100644 include/linux/dfl.h

Why move this if there is no in-kernel users?

greg k-h

Re: [PATCH 1/4] fpga: dfl: fix the definitions of type & feature_id for dfl devices

[Greg KH]

On Mon, Nov 02, 2020 at 11:21:01PM -0800, Moritz Fischer wrote:
> From: Xu Yilun <yilun.xu@intel.com>
> 
> The value of the field dfl_device.type comes from the 12 bits register
> field DFH_ID according to DFL spec. So this patch changes the definition
> of the type field to u16.
> 
> Also it is not necessary to illustrate the valid bits of the type field
> in comments. Instead we should explicitly define the possible values in
> the enumeration type for it, because they are shared by hardware spec.
> We should not let the compiler decide these values.
> 
> Similar changes are also applied to dfl_device.feature_id.
> 
> This patch also fixed the MODALIAS format according to the changes
> above.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  drivers/fpga/dfl.c |  3 +--
>  drivers/fpga/dfl.h | 14 +++++++-------
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b450870b75ed..5a6ba3b2fa05 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct dfl_device *ddev = to_dfl_dev(dev);
>  
> -	/* The type has 4 valid bits and feature_id has 12 valid bits */
> -	return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
> +	return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",

Why drop the comment and not just fix it up instead?

>  			      ddev->type, ddev->feature_id);
>  }
>  
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 5dc758f655b7..ac373b1fcff9 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>   * enum dfl_id_type - define the DFL FIU types
>   */
>  enum dfl_id_type {
> -	FME_ID,
> -	PORT_ID,
> +	FME_ID = 0,
> +	PORT_ID = 1,
>  	DFL_ID_MAX,
>  };
>  
>  /**
>   * struct dfl_device_id -  dfl device identifier
> - * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type.
> - * @feature_id: contains 12 bits feature identifier local to its DFL FIU type.
> + * @type: DFL FIU type of the device. See enum dfl_id_type.
> + * @feature_id: feature identifier local to its DFL FIU type.
>   * @driver_data: driver specific data.
>   */
>  struct dfl_device_id {
> -	u8 type;
> +	u16 type;

Why isn't this enum dfl_id_type?

>  	u16 feature_id;
>  	unsigned long driver_data;
>  };
> @@ -543,7 +543,7 @@ struct dfl_device_id {
>   * @dev: generic device interface.
>   * @id: id of the dfl device.
>   * @type: type of DFL FIU of the device. See enum dfl_id_type.
> - * @feature_id: 16 bits feature identifier local to its DFL FIU type.
> + * @feature_id: feature identifier local to its DFL FIU type.

But feature_id is still 16 bits, why change this?


>   * @mmio_res: mmio resource of this dfl device.
>   * @irqs: list of Linux IRQ numbers of this dfl device.
>   * @num_irqs: number of IRQs supported by this dfl device.
> @@ -553,7 +553,7 @@ struct dfl_device_id {
>  struct dfl_device {
>  	struct device dev;
>  	int id;
> -	u8 type;
> +	u16 type;

why isn't this an enum as well?

greg k-h

Re: [PATCH 1/4] fpga: dfl: fix the definitions of type & feature_id for dfl devices

[Xu Yilun]

Hi Greg:

On Tue, Nov 03, 2020 at 08:44:52AM +0100, Greg KH wrote:
> On Mon, Nov 02, 2020 at 11:21:01PM -0800, Moritz Fischer wrote:
> > From: Xu Yilun <yilun.xu@intel.com>
> > 
> > The value of the field dfl_device.type comes from the 12 bits register
> > field DFH_ID according to DFL spec. So this patch changes the definition
> > of the type field to u16.
> > 
> > Also it is not necessary to illustrate the valid bits of the type field
> > in comments. Instead we should explicitly define the possible values in
> > the enumeration type for it, because they are shared by hardware spec.
> > We should not let the compiler decide these values.
> > 
> > Similar changes are also applied to dfl_device.feature_id.
> > 
> > This patch also fixed the MODALIAS format according to the changes
> > above.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Reviewed-by: Tom Rix <trix@redhat.com>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >  drivers/fpga/dfl.c |  3 +--
> >  drivers/fpga/dfl.h | 14 +++++++-------
> >  2 files changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index b450870b75ed..5a6ba3b2fa05 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  {
> >  	struct dfl_device *ddev = to_dfl_dev(dev);
> >  
> > -	/* The type has 4 valid bits and feature_id has 12 valid bits */
> > -	return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
> > +	return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",
> 
> Why drop the comment and not just fix it up instead?

Previously in Pull request for 5.10, you suggested that don't care too much about
the exact data width.

 "Who cares it's only 4 bits, we have room in the kernel to have it be a full 8
  bits or 32 or whatever"

So I changed the type of 'type' & 'feature_id' all to u16 and deleted all the
comments about the width of them.

> 
> >  			      ddev->type, ddev->feature_id);
> >  }
> >  
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 5dc758f655b7..ac373b1fcff9 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
> >   * enum dfl_id_type - define the DFL FIU types
> >   */
> >  enum dfl_id_type {
> > -	FME_ID,
> > -	PORT_ID,
> > +	FME_ID = 0,
> > +	PORT_ID = 1,
> >  	DFL_ID_MAX,
> >  };
> >  
> >  /**
> >   * struct dfl_device_id -  dfl device identifier
> > - * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type.
> > - * @feature_id: contains 12 bits feature identifier local to its DFL FIU type.
> > + * @type: DFL FIU type of the device. See enum dfl_id_type.
> > + * @feature_id: feature identifier local to its DFL FIU type.
> >   * @driver_data: driver specific data.
> >   */
> >  struct dfl_device_id {
> > -	u8 type;
> > +	u16 type;
> 
> Why isn't this enum dfl_id_type?

We are going to move the two fields to mod_devicetable.h, they
will be eventually changed to __u16, so I use u16 here.

> 
> >  	u16 feature_id;
> >  	unsigned long driver_data;
> >  };
> > @@ -543,7 +543,7 @@ struct dfl_device_id {
> >   * @dev: generic device interface.
> >   * @id: id of the dfl device.
> >   * @type: type of DFL FIU of the device. See enum dfl_id_type.
> > - * @feature_id: 16 bits feature identifier local to its DFL FIU type.
> > + * @feature_id: feature identifier local to its DFL FIU type.
> 
> But feature_id is still 16 bits, why change this?

As I mentioned before, I mean not to emphasize on the bitfield width.

> 
> 
> >   * @mmio_res: mmio resource of this dfl device.
> >   * @irqs: list of Linux IRQ numbers of this dfl device.
> >   * @num_irqs: number of IRQs supported by this dfl device.
> > @@ -553,7 +553,7 @@ struct dfl_device_id {
> >  struct dfl_device {
> >  	struct device dev;
> >  	int id;
> > -	u8 type;
> > +	u16 type;
> 
> why isn't this an enum as well?

Same as before, the 'type' should be sync with dfl_device_id.type.

Thanks,
Yilun

> 
> greg k-h

Re: [PATCH 2/4] fpga: dfl: move dfl_device_id to mod_devicetable.h

[Xu Yilun]

On Tue, Nov 03, 2020 at 08:40:53AM +0100, Greg KH wrote:
> On Mon, Nov 02, 2020 at 11:21:02PM -0800, Moritz Fischer wrote:
> > From: Xu Yilun <yilun.xu@intel.com>
> > 
> > In order to support MODULE_DEVICE_TABLE() for dfl device driver, this
> > patch moves struct dfl_device_id to mod_devicetable.h
> > 
> > Some brief description for DFL (Device Feature List) is added to make
> > the DFL known to the whole kernel.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > Reviewed-by: Tom Rix <trix@redhat.com>
> > Acked-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >  drivers/fpga/dfl.h              | 13 +------------
> >  include/linux/mod_devicetable.h | 24 ++++++++++++++++++++++++
> >  2 files changed, 25 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index ac373b1fcff9..549c7900dcfd 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -22,6 +22,7 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/uuid.h>
> > @@ -525,18 +526,6 @@ enum dfl_id_type {
> >  	DFL_ID_MAX,
> >  };
> >  
> > -/**
> > - * struct dfl_device_id -  dfl device identifier
> > - * @type: DFL FIU type of the device. See enum dfl_id_type.
> > - * @feature_id: feature identifier local to its DFL FIU type.
> > - * @driver_data: driver specific data.
> > - */
> > -struct dfl_device_id {
> > -	u16 type;
> > -	u16 feature_id;
> > -	unsigned long driver_data;
> > -};
> > -
> >  /**
> >   * struct dfl_device - represent an dfl device on dfl bus
> >   *
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index 5b08a473cdba..e4870e5d3ea8 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -838,4 +838,28 @@ struct mhi_device_id {
> >  	kernel_ulong_t driver_data;
> >  };
> >  
> > +/*
> > + * DFL (Device Feature List)
> > + *
> > + * DFL defines a linked list of feature headers within the device MMIO space to
> > + * provide an extensible way of adding features. Software can walk through these
> > + * predefined data structures to enumerate features. It is now used in the FPGA.
> > + * See Documentation/fpga/dfl.rst for more information.
> > + *
> > + * The dfl bus type is introduced to match the individual feature devices (dfl
> > + * devices) for specific dfl drivers.
> > + */
> > +
> > +/**
> > + * struct dfl_device_id -  dfl device identifier
> > + * @type: DFL FIU type of the device. See enum dfl_id_type.
> > + * @feature_id: feature identifier local to its DFL FIU type.
> > + * @driver_data: driver specific data.
> > + */
> > +struct dfl_device_id {
> > +	__u16 type;
> > +	__u16 feature_id;
> > +	unsigned long driver_data;
> 
> This is the wrong type for driver_data now that it goes to userspace :(
> 
> {sigh}

Thanks for the catching, I should change it to kernel_ulong_t.

Thanks,
Yilun

Re: [PATCH 4/4] fpga: dfl: move dfl bus related APIs to include/linux/dfl.h

[Xu Yilun]

On Tue, Nov 03, 2020 at 08:43:07AM +0100, Greg KH wrote:
> On Mon, Nov 02, 2020 at 11:21:04PM -0800, Moritz Fischer wrote:
> > From: Xu Yilun <yilun.xu@intel.com>
> > 
> > Now the dfl drivers could be made as independent modules and put in
> > different folders according to their functionalities. In order for
> > scattered dfl device drivers to include dfl bus APIs, move the
> > dfl bus APIs to a new header file in the public folder.
> > 
> > [mdf@kernel.org: Fixed up header guards to match filename]
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Reviewed-by: Tom Rix <trix@redhat.com>
> > Acked-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >  MAINTAINERS         |  1 +
> >  drivers/fpga/dfl.c  |  1 +
> >  drivers/fpga/dfl.h  | 72 -------------------------------------
> >  include/linux/dfl.h | 86 +++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 88 insertions(+), 72 deletions(-)
> >  create mode 100644 include/linux/dfl.h
> 
> Why move this if there is no in-kernel users?

The DFL emif driver in driver/memory is the first user, see:

https://lore.kernel.org/linux-fpga/20201027105545.GB20676@kozik-lap/T/#m6b72f043ecf266c6305bf43db88cddcaf3f9f73d

It is not in this patchset, but the memory controller maintainer is already
acked this patch.

Thanks,
Yilun

> 
> greg k-h

Re: [PATCH 4/4] fpga: dfl: move dfl bus related APIs to include/linux/dfl.h

[Greg KH]

On Tue, Nov 03, 2020 at 06:43:17PM +0800, Xu Yilun wrote:
> On Tue, Nov 03, 2020 at 08:43:07AM +0100, Greg KH wrote:
> > On Mon, Nov 02, 2020 at 11:21:04PM -0800, Moritz Fischer wrote:
> > > From: Xu Yilun <yilun.xu@intel.com>
> > > 
> > > Now the dfl drivers could be made as independent modules and put in
> > > different folders according to their functionalities. In order for
> > > scattered dfl device drivers to include dfl bus APIs, move the
> > > dfl bus APIs to a new header file in the public folder.
> > > 
> > > [mdf@kernel.org: Fixed up header guards to match filename]
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > Reviewed-by: Tom Rix <trix@redhat.com>
> > > Acked-by: Wu Hao <hao.wu@intel.com>
> > > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > > ---
> > >  MAINTAINERS         |  1 +
> > >  drivers/fpga/dfl.c  |  1 +
> > >  drivers/fpga/dfl.h  | 72 -------------------------------------
> > >  include/linux/dfl.h | 86 +++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 88 insertions(+), 72 deletions(-)
> > >  create mode 100644 include/linux/dfl.h
> > 
> > Why move this if there is no in-kernel users?
> 
> The DFL emif driver in driver/memory is the first user, see:
> 
> https://lore.kernel.org/linux-fpga/20201027105545.GB20676@kozik-lap/T/#m6b72f043ecf266c6305bf43db88cddcaf3f9f73d
> 
> It is not in this patchset, but the memory controller maintainer is already
> acked this patch.

How am I, or anyone else, supposed to know this?

Again, don't include patches that are not actually used, that's a huge
red flag to any reviewer and it just makes them grumpy and sad and
less-likely to ever want to review code from the submitter again...

{sigh}

greg k-h

Re: [PATCH 4/4] fpga: dfl: move dfl bus related APIs to include/linux/dfl.h

[Xu Yilun]

On Tue, Nov 03, 2020 at 12:08:17PM +0100, Greg KH wrote:
> On Tue, Nov 03, 2020 at 06:43:17PM +0800, Xu Yilun wrote:
> > On Tue, Nov 03, 2020 at 08:43:07AM +0100, Greg KH wrote:
> > > On Mon, Nov 02, 2020 at 11:21:04PM -0800, Moritz Fischer wrote:
> > > > From: Xu Yilun <yilun.xu@intel.com>
> > > > 
> > > > Now the dfl drivers could be made as independent modules and put in
> > > > different folders according to their functionalities. In order for
> > > > scattered dfl device drivers to include dfl bus APIs, move the
> > > > dfl bus APIs to a new header file in the public folder.
> > > > 
> > > > [mdf@kernel.org: Fixed up header guards to match filename]
> > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > Reviewed-by: Tom Rix <trix@redhat.com>
> > > > Acked-by: Wu Hao <hao.wu@intel.com>
> > > > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > > > ---
> > > >  MAINTAINERS         |  1 +
> > > >  drivers/fpga/dfl.c  |  1 +
> > > >  drivers/fpga/dfl.h  | 72 -------------------------------------
> > > >  include/linux/dfl.h | 86 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 88 insertions(+), 72 deletions(-)
> > > >  create mode 100644 include/linux/dfl.h
> > > 
> > > Why move this if there is no in-kernel users?
> > 
> > The DFL emif driver in driver/memory is the first user, see:
> > 
> > https://lore.kernel.org/linux-fpga/20201027105545.GB20676@kozik-lap/T/#m6b72f043ecf266c6305bf43db88cddcaf3f9f73d
> > 
> > It is not in this patchset, but the memory controller maintainer is already
> > acked this patch.
> 
> How am I, or anyone else, supposed to know this?
> 
> Again, don't include patches that are not actually used, that's a huge
> red flag to any reviewer and it just makes them grumpy and sad and
> less-likely to ever want to review code from the submitter again...

Sorry, I didn't explain it clearly.

The DFL emif driver was in this patchset before v11. All the maintainers
are on the maillist, see the v10:
https://lore.kernel.org/linux-fpga/20201015162812.GA251058@epycbox.lan/T/#m64f61e205ed0ea1fd65e30e24bea55da40f8881d

In v10, the first 4 patches are DFL bus related, the same as you can see
in this pull request. The 5th is the dfl-n3000-nios driver and the 6th is
the dfl-emif, they are the in-kernel users of dfl bus.

In v11, I didn't include the first 4 patches because Moritz has already
queued them to his branch, I just send the remaining ones in order to
save time for the reviewers.

Thanks,
Yilun

> 
> {sigh}
> 
> greg k-h