This patchset makes it possible to develop independent driver modules
for DFL private features. It also helps to leverage existing kernel
drivers to enable some IP blocks in DFL.
Xu Yilun (2):
fpga: dfl: map feature mmio resources in their own feature drivers
fpga: dfl: create a dfl bus type to support DFL devices
Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
drivers/fpga/dfl-pci.c | 21 +-
drivers/fpga/dfl.c | 435 +++++++++++++++++++++++++++-----
drivers/fpga/dfl.h | 91 ++++++-
4 files changed, 492 insertions(+), 70 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
--
2.7.4
A new bus type "dfl" is introduced for private features which are not
initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
private features could be handled by separate driver modules.
DFL framework will create DFL devices on enumeration. DFL drivers could
be registered on this bus to match these DFL devices. They are matched by
dfl type & feature_id.
Signed-off-by: Xu Yilun <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Signed-off-by: Matthew Gerlach <[email protected]>
Signed-off-by: Russ Weight <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
drivers/fpga/dfl.c | 248 ++++++++++++++++++++++++++++++--
drivers/fpga/dfl.h | 85 +++++++++++
3 files changed, 340 insertions(+), 8 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
new file mode 100644
index 0000000..cd00abc
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-dfl
@@ -0,0 +1,15 @@
+What: /sys/bus/dfl/devices/.../type
+Date: March 2020
+KernelVersion: 5.7
+Contact: Xu Yilun <[email protected]>
+Description: Read-only. It returns type of DFL FIU of the device. Now DFL
+ supports 2 FIU types, 0 for FME, 1 for PORT.
+ Format: 0x%x
+
+What: /sys/bus/dfl/devices/.../feature_id
+Date: March 2020
+KernelVersion: 5.7
+Contact: Xu Yilun <[email protected]>
+Description: Read-only. It returns feature identifier local to its DFL FIU
+ type.
+ Format: 0x%llx
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 7dc6411..93f9d6d 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
* index to dfl_chardevs table. If no chardev support just set devt_type
* as one invalid index (DFL_FPGA_DEVT_MAX).
*/
-enum dfl_id_type {
- FME_ID, /* fme id allocation and mapping */
- PORT_ID, /* port id allocation and mapping */
- DFL_ID_MAX,
-};
-
enum dfl_fpga_devt_type {
DFL_FPGA_DEVT_FME,
DFL_FPGA_DEVT_PORT,
@@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature *feature)
return feature->id == FEATURE_ID_FIU_HEADER;
}
+static const struct dfl_device_id *
+dfl_match_one_device(const struct dfl_device_id *id,
+ struct dfl_device *dfl_dev)
+{
+ if (id->type == dfl_dev->type &&
+ id->feature_id == dfl_dev->feature_id)
+ return id;
+
+ return NULL;
+}
+
+static int dfl_bus_match(struct device *dev, struct device_driver *drv)
+{
+ struct dfl_device *dfl_dev = to_dfl_dev(dev);
+ struct dfl_driver *dfl_drv = to_dfl_drv(drv);
+ const struct dfl_device_id *id_entry = dfl_drv->id_table;
+
+ while (id_entry->feature_id) {
+ if (dfl_match_one_device(id_entry, dfl_dev)) {
+ dfl_dev->id_entry = id_entry;
+ return 1;
+ }
+ id_entry++;
+ }
+
+ return 0;
+}
+
+static int dfl_bus_probe(struct device *dev)
+{
+ struct dfl_device *dfl_dev = to_dfl_dev(dev);
+ struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
+
+ return dfl_drv->probe(dfl_dev);
+}
+
+static int dfl_bus_remove(struct device *dev)
+{
+ struct dfl_device *dfl_dev = to_dfl_dev(dev);
+ struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
+
+ if (dfl_drv->remove)
+ dfl_drv->remove(dfl_dev);
+
+ return 0;
+}
+
+static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+ struct dfl_device *dfl_dev = to_dfl_dev(dev);
+
+ if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
+ dfl_dev->type, dfl_dev->feature_id))
+ return -ENOMEM;
+
+ return 0;
+}
+
+/* show dfl info fields */
+#define dfl_info_attr(field, format_string) \
+static ssize_t \
+field##_show(struct device *dev, struct device_attribute *attr, \
+ char *buf) \
+{ \
+ struct dfl_device *dfl_dev = to_dfl_dev(dev); \
+ \
+ return sprintf(buf, format_string, dfl_dev->field); \
+} \
+static DEVICE_ATTR_RO(field)
+
+dfl_info_attr(type, "0x%x\n");
+dfl_info_attr(feature_id, "0x%llx\n");
+
+static struct attribute *dfl_dev_attrs[] = {
+ &dev_attr_type.attr,
+ &dev_attr_feature_id.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(dfl_dev);
+
+static struct bus_type dfl_bus_type = {
+ .name = "dfl",
+ .match = dfl_bus_match,
+ .probe = dfl_bus_probe,
+ .remove = dfl_bus_remove,
+ .uevent = dfl_bus_uevent,
+ .dev_groups = dfl_dev_groups,
+};
+
+static void release_dfl_dev(struct device *dev)
+{
+ struct dfl_device *dfl_dev = to_dfl_dev(dev);
+
+ release_resource(&dfl_dev->mmio_res);
+ kfree(dfl_dev->irqs);
+ kfree(dfl_dev);
+}
+
+static struct dfl_device *
+dfl_dev_add(struct dfl_feature_platform_data *pdata,
+ struct dfl_feature *feature)
+{
+ struct platform_device *pdev = pdata->dev;
+ struct dfl_device *dfl_dev;
+ int i, ret;
+
+ dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
+ if (!dfl_dev)
+ return ERR_PTR(-ENOMEM);
+
+ dfl_dev->cdev = pdata->dfl_cdev;
+
+ dfl_dev->mmio_res.parent = &pdev->resource[feature->resource_index];
+ dfl_dev->mmio_res.flags = IORESOURCE_MEM;
+ dfl_dev->mmio_res.start =
+ pdev->resource[feature->resource_index].start;
+ dfl_dev->mmio_res.end = pdev->resource[feature->resource_index].end;
+
+ /* then add irq resource */
+ if (feature->nr_irqs) {
+ dfl_dev->irqs = kcalloc(feature->nr_irqs,
+ sizeof(*dfl_dev->irqs), GFP_KERNEL);
+ if (!dfl_dev->irqs) {
+ ret = -ENOMEM;
+ goto free_dfl_dev;
+ }
+
+ for (i = 0; i < feature->nr_irqs; i++)
+ dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
+
+ dfl_dev->num_irqs = feature->nr_irqs;
+ }
+
+ dfl_dev->type = feature_dev_id_type(pdev);
+ dfl_dev->feature_id = (unsigned long long)feature->id;
+
+ dfl_dev->dev.parent = &pdev->dev;
+ dfl_dev->dev.bus = &dfl_bus_type;
+ dfl_dev->dev.release = release_dfl_dev;
+ dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
+ feature->index);
+
+ dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
+ ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev->mmio_res);
+ if (ret) {
+ dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
+ dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
+ goto free_irqs;
+ }
+
+ ret = device_register(&dfl_dev->dev);
+ if (ret) {
+ put_device(&dfl_dev->dev);
+ return ERR_PTR(ret);
+ }
+
+ dev_info(&pdev->dev, "add dfl_dev: %s\n",
+ dev_name(&dfl_dev->dev));
+ return dfl_dev;
+
+free_irqs:
+ kfree(dfl_dev->irqs);
+free_dfl_dev:
+ kfree(dfl_dev);
+ return ERR_PTR(ret);
+}
+
+static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
+{
+ struct dfl_device *dfl_dev;
+ struct dfl_feature *feature;
+
+ dfl_fpga_dev_for_each_feature(pdata, feature) {
+ if (!feature->ioaddr && feature->priv) {
+ dfl_dev = feature->priv;
+ device_unregister(&dfl_dev->dev);
+ feature->priv = NULL;
+ }
+ }
+}
+
+static int dfl_devs_init(struct platform_device *pdev)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+ struct dfl_feature *feature;
+ struct dfl_device *dfl_dev;
+
+ dfl_fpga_dev_for_each_feature(pdata, feature) {
+ if (feature->ioaddr || feature->priv)
+ continue;
+
+ dfl_dev = dfl_dev_add(pdata, feature);
+ if (IS_ERR(dfl_dev)) {
+ dfl_devs_uinit(pdata);
+ return PTR_ERR(dfl_dev);
+ }
+
+ feature->priv = dfl_dev;
+ }
+
+ return 0;
+}
+
+int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
+{
+ if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
+ return -EINVAL;
+
+ dfl_drv->drv.owner = owner;
+ dfl_drv->drv.bus = &dfl_bus_type;
+
+ return driver_register(&dfl_drv->drv);
+}
+EXPORT_SYMBOL(__dfl_driver_register);
+
+void dfl_driver_unregister(struct dfl_driver *dfl_drv)
+{
+ driver_unregister(&dfl_drv->drv);
+}
+EXPORT_SYMBOL(dfl_driver_unregister);
+
/**
* dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
* @pdev: feature device.
@@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
struct dfl_feature *feature;
- dfl_fpga_dev_for_each_feature(pdata, feature)
+ dfl_devs_uinit(pdata);
+
+ dfl_fpga_dev_for_each_feature(pdata, feature) {
if (feature->ops) {
if (feature->ops->uinit)
feature->ops->uinit(pdev, feature);
feature->ops = NULL;
}
+ }
}
EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
@@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,
drv++;
}
+ ret = dfl_devs_init(pdev);
+ if (ret)
+ goto exit;
+
return 0;
exit:
dfl_fpga_dev_feature_uinit(pdev);
@@ -553,6 +776,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
struct dfl_feature_irq_ctx *ctx;
unsigned int i;
+ feature->index = index;
+
/* save resource information for each feature */
feature->dev = fdev;
feature->id = finfo->fid;
@@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
{
int ret;
+ ret = bus_register(&dfl_bus_type);
+ if (ret)
+ return ret;
+
dfl_ids_init();
ret = dfl_chardev_init();
- if (ret)
+ if (ret) {
dfl_ids_destroy();
+ bus_unregister(&dfl_bus_type);
+ }
return ret;
}
@@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
{
dfl_chardev_uinit();
dfl_ids_destroy();
+ bus_unregister(&dfl_bus_type);
}
module_init(dfl_fpga_init);
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index f605c28..d00aa1c 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
*
* @dev: ptr to pdev of the feature device which has the sub feature.
* @id: sub feature id.
+ * @index: unique identifier for an sub feature within the feature device.
+ * It is possible that multiply sub features with same feature id are
+ * listed in one feature device. So an incremental index (start from 0)
+ * is needed to identify each sub feature.
* @resource_index: each sub feature has one mmio resource for its registers.
* this index is used to find its mmio resource from the
* feature dev (platform device)'s reources.
@@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
struct dfl_feature {
struct platform_device *dev;
u64 id;
+ int index;
int resource_index;
void __iomem *ioaddr;
struct dfl_feature_irq_ctx *irq_ctx;
@@ -515,4 +520,84 @@ 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,
+ PORT_ID,
+ DFL_ID_MAX,
+};
+
+/**
+ * struct dfl_device_id - dfl device identifier
+ * @type: Type of DFL FIU of the device. See enum dfl_id_type.
+ * @feature_id: 64 bits feature identifier local to its DFL FIU type.
+ * @driver_data: Driver specific data
+ */
+struct dfl_device_id {
+ unsigned int type;
+ unsigned long long feature_id;
+ unsigned long driver_data;
+};
+
+/**
+ * struct dfl_device - represent an dfl device on dfl bus
+ *
+ * @dev: Generic device interface.
+ * @type: Type of DFL FIU of the device. See enum dfl_id_type.
+ * @feature_id: 64 bits 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;
+ unsigned int type;
+ unsigned long long 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.
+ * @probe: 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);
+ int (*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 */
--
2.7.4
Generally i think this is a good approach.
However I do have concern.
The feature_id in dfl is magic number, similar to pci id but without a vendor id.
Is it possible to add something like a vendor id so different vendors would not have to be so careful to use a unique id ?
This touches some of the matching function of the bus ops. Could there be a way for bus ops to be used so that there could be multiple matching function. Maybe the one in 0002 as a default so users could override it ?
The use case I am worrying about is an OEM. The OEM uses an unclaimed feature_id and supplies their own platform device device driver to match the feature_id. But some later version of the kernel claims this feature_id and the OEM's driver no longer works and since the value comes from pci config space it is difficult to change.
So looking for something like
register_feature_matcher((*bus_match)(struct device *dev, struct device_driver *drv))
static int dfl_bus_match_default(struct device *dev, struct device_driver *drv)
{
struct dfl_device *dfl_dev = to_dfl_dev(dev);
struct dfl_driver *dfl_drv = to_dfl_drv(drv);
const struct dfl_device_id *id_entry = dfl_drv->id_table;
while (id_entry->feature_id) {
if (dfl_match_one_device(id_entry, dfl_dev)) {
dfl_dev->id_entry = id_entry;
return 1;
}
id_entry++;
}
return 0;
}
register_feature_matcher(&dfl_bus_match_default)
static int dfl_bus_match(struct device *dev, struct device_driver *drv)
{
struct dfl_device *dfl_dev = to_dfl_dev(dev);
struct dfl_driver *dfl_drv = to_dfl_drv(drv);
const struct dfl_device_id *id_entry = dfl_drv->id_table;
while (id_entry->feature_id) {
matcher = Loop over matchers()
if (matcher(dev, drv)) {
dfl_dev->id_entry = id_entry;
return 1;
}
id_entry++;
}
return 0;
}
Or maybe use some of the unused bits in the dfl header to add a second vendor-like id and change existing matcher to look feature_id and vendor_like_id.
Tom
On 7/14/20 10:38 PM, Xu Yilun wrote:
> This patchset makes it possible to develop independent driver modules
> for DFL private features. It also helps to leverage existing kernel
> drivers to enable some IP blocks in DFL.
>
> Xu Yilun (2):
> fpga: dfl: map feature mmio resources in their own feature drivers
> fpga: dfl: create a dfl bus type to support DFL devices
>
> Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
> drivers/fpga/dfl-pci.c | 21 +-
> drivers/fpga/dfl.c | 435 +++++++++++++++++++++++++++-----
> drivers/fpga/dfl.h | 91 ++++++-
> 4 files changed, 492 insertions(+), 70 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
>
> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
>
> Generally i think this is a good approach.
>
> However I do have concern.
>
> The feature_id in dfl is magic number, similar to pci id but without a vendor
> id.
>
> Is it possible to add something like a vendor id so different vendors would
> not have to be so careful to use a unique id ?
Hi Tom,
Thanks for the comments.
Here is only one field defined in spec, that is feature id to distinguish one
feature with another one. There is no vendor id here I think, and several
cards are using this for now and seems not possible to change DFH format
for these products. : (
I fully understand the concern is the feature id management, and potential
conflicts there from different vendors. One possible method to resolve this
is managing the ids in a public place (web? Or just the driver header file for
feature id definition), all vendors can request some feature ids, and other
people can see which feature ids have been used so that they can avoid
using conflict ones with other people.
And in the later version DFH, this feature id will be replaced with GUID
I think, so it can resolve the conflict problems from different vendors?
But now, we still need to handle the existing ones. : )
Thanks
Hao
>
> This touches some of the matching function of the bus ops. Could there be a
> way for bus ops to be used so that there could be multiple matching
> function. Maybe the one in 0002 as a default so users could override it ?
>
> The use case I am worrying about is an OEM. The OEM uses an unclaimed
> feature_id and supplies their own platform device device driver to match the
> feature_id. But some later version of the kernel claims this feature_id and
> the OEM's driver no longer works and since the value comes from pci config
> space it is difficult to change.
>
> So looking for something like
>
> register_feature_matcher((*bus_match)(struct device *dev, struct
> device_driver *drv))
>
> static int dfl_bus_match_default(struct device *dev, struct device_driver *drv)
> {
> struct dfl_device *dfl_dev = to_dfl_dev(dev);
> struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> const struct dfl_device_id *id_entry = dfl_drv->id_table;
>
> while (id_entry->feature_id) {
> if (dfl_match_one_device(id_entry, dfl_dev)) {
> dfl_dev->id_entry = id_entry;
> return 1;
> }
> id_entry++;
> }
>
> return 0;
> }
>
> register_feature_matcher(&dfl_bus_match_default)
>
> static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> {
>
> struct dfl_device *dfl_dev = to_dfl_dev(dev);
> struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> const struct dfl_device_id *id_entry = dfl_drv->id_table;
>
> while (id_entry->feature_id) {
>
> matcher = Loop over matchers()
>
> if (matcher(dev, drv)) {
> dfl_dev->id_entry = id_entry;
> return 1;
> }
> id_entry++;
> }
>
> return 0;
> }
>
> Or maybe use some of the unused bits in the dfl header to add a second
> vendor-like id and change existing matcher to look feature_id and
> vendor_like_id.
>
> Tom
>
>
>
> On 7/14/20 10:38 PM, Xu Yilun wrote:
> > This patchset makes it possible to develop independent driver modules
> > for DFL private features. It also helps to leverage existing kernel
> > drivers to enable some IP blocks in DFL.
> >
> > Xu Yilun (2):
> > fpga: dfl: map feature mmio resources in their own feature drivers
> > fpga: dfl: create a dfl bus type to support DFL devices
> >
> > Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
> > drivers/fpga/dfl-pci.c | 21 +-
> > drivers/fpga/dfl.c | 435 +++++++++++++++++++++++++++-----
> > drivers/fpga/dfl.h | 91 ++++++-
> > 4 files changed, 492 insertions(+), 70 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
> >
> -----Original Message-----
> From: Xu, Yilun <[email protected]>
> Sent: Wednesday, July 15, 2020 1:38 PM
> To: [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]; Xu, Yilun <[email protected]>;
> Wu, Hao <[email protected]>; Matthew Gerlach
> <[email protected]>; Weight, Russell H
> <[email protected]>
> Subject: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices
>
> A new bus type "dfl" is introduced for private features which are not
> initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> private features could be handled by separate driver modules.
>
> DFL framework will create DFL devices on enumeration.
Actually these DFL devices are created in AFU/FME driver initialization or real
core DFL code, is my understanding correct?
> DFL drivers could
> be registered on this bus to match these DFL devices. They are matched by
> dfl type & feature_id.
>
> Signed-off-by: Xu Yilun <[email protected]>
> Signed-off-by: Wu Hao <[email protected]>
> Signed-off-by: Matthew Gerlach <[email protected]>
> Signed-off-by: Russ Weight <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
> drivers/fpga/dfl.c | 248 ++++++++++++++++++++++++++++++--
> drivers/fpga/dfl.h | 85 +++++++++++
> 3 files changed, 340 insertions(+), 8 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl
> b/Documentation/ABI/testing/sysfs-bus-dfl
> new file mode 100644
> index 0000000..cd00abc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> @@ -0,0 +1,15 @@
> +What: /sys/bus/dfl/devices/.../type
> +Date: March 2020
> +KernelVersion: 5.7
I guess you need to update the date and target kernel version.
> +Contact: Xu Yilun <[email protected]>
> +Description: Read-only. It returns type of DFL FIU of the device. Now DFL
> + supports 2 FIU types, 0 for FME, 1 for PORT.
> + Format: 0x%x
Or consider just print the string instead here?
> +
> +What: /sys/bus/dfl/devices/.../feature_id
> +Date: March 2020
> +KernelVersion: 5.7
Ditto.
> +Contact: Xu Yilun <[email protected]>
> +Description: Read-only. It returns feature identifier local to its DFL FIU
> + type.
> + Format: 0x%llx
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 7dc6411..93f9d6d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
> * index to dfl_chardevs table. If no chardev support just set devt_type
> * as one invalid index (DFL_FPGA_DEVT_MAX).
> */
> -enum dfl_id_type {
> - FME_ID, /* fme id allocation and mapping */
> - PORT_ID, /* port id allocation and mapping */
> - DFL_ID_MAX,
> -};
> -
> enum dfl_fpga_devt_type {
> DFL_FPGA_DEVT_FME,
> DFL_FPGA_DEVT_PORT,
> @@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature
> *feature)
> return feature->id == FEATURE_ID_FIU_HEADER;
> }
>
> +static const struct dfl_device_id *
> +dfl_match_one_device(const struct dfl_device_id *id,
> + struct dfl_device *dfl_dev)
Why start a new line here, it's just 80 char here. : )
BTW: you can use ddev instead of dfl_dev here for a shorter name.
> +{
> + if (id->type == dfl_dev->type &&
> + id->feature_id == dfl_dev->feature_id)
> + return id;
> +
> + return NULL;
> +}
> +
> +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> + struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> + const struct dfl_device_id *id_entry = dfl_drv->id_table;
> +
> + while (id_entry->feature_id) {
> + if (dfl_match_one_device(id_entry, dfl_dev)) {
> + dfl_dev->id_entry = id_entry;
> + return 1;
> + }
> + id_entry++;
> + }
> +
> + return 0;
> +}
> +
> +static int dfl_bus_probe(struct device *dev)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> + struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> +
> + return dfl_drv->probe(dfl_dev);
> +}
> +
> +static int dfl_bus_remove(struct device *dev)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> + struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> +
> + if (dfl_drv->remove)
> + dfl_drv->remove(dfl_dev);
> +
> + return 0;
> +}
> +
> +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +
> + if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
> + dfl_dev->type, dfl_dev->feature_id))
I see for pci bus, it's using v%08Xd%08X... should we consider adding one
"t" to indicate that value is for type? Will that be simpler to the users?
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +/* show dfl info fields */
> +#define dfl_info_attr(field, format_string) \
> +static ssize_t \
> +field##_show(struct device *dev, struct device_attribute *attr,
> \
> + char *buf) \
> +{ \
> + struct dfl_device *dfl_dev = to_dfl_dev(dev); \
> + \
> + return sprintf(buf, format_string, dfl_dev->field); \
> +} \
> +static DEVICE_ATTR_RO(field)
> +
> +dfl_info_attr(type, "0x%x\n");
> +dfl_info_attr(feature_id, "0x%llx\n");
> +
> +static struct attribute *dfl_dev_attrs[] = {
> + &dev_attr_type.attr,
> + &dev_attr_feature_id.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(dfl_dev);
> +
> +static struct bus_type dfl_bus_type = {
> + .name = "dfl",
> + .match = dfl_bus_match,
> + .probe = dfl_bus_probe,
> + .remove = dfl_bus_remove,
> + .uevent = dfl_bus_uevent,
> + .dev_groups = dfl_dev_groups,
> +};
> +
> +static void release_dfl_dev(struct device *dev)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +
> + release_resource(&dfl_dev->mmio_res);
> + kfree(dfl_dev->irqs);
> + kfree(dfl_dev);
> +}
> +
> +static struct dfl_device *
> +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> + struct dfl_feature *feature)
> +{
> + struct platform_device *pdev = pdata->dev;
> + struct dfl_device *dfl_dev;
> + int i, ret;
> +
> + dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
> + if (!dfl_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dfl_dev->cdev = pdata->dfl_cdev;
> +
> + dfl_dev->mmio_res.parent = &pdev->resource[feature-
> >resource_index];
> + dfl_dev->mmio_res.flags = IORESOURCE_MEM;
> + dfl_dev->mmio_res.start =
> + pdev->resource[feature->resource_index].start;
> + dfl_dev->mmio_res.end = pdev->resource[feature-
> >resource_index].end;
> +
> + /* then add irq resource */
> + if (feature->nr_irqs) {
> + dfl_dev->irqs = kcalloc(feature->nr_irqs,
> + sizeof(*dfl_dev->irqs), GFP_KERNEL);
> + if (!dfl_dev->irqs) {
> + ret = -ENOMEM;
> + goto free_dfl_dev;
> + }
> +
> + for (i = 0; i < feature->nr_irqs; i++)
> + dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
> +
> + dfl_dev->num_irqs = feature->nr_irqs;
> + }
> +
> + dfl_dev->type = feature_dev_id_type(pdev);
> + dfl_dev->feature_id = (unsigned long long)feature->id;
> +
> + dfl_dev->dev.parent = &pdev->dev;
> + dfl_dev->dev.bus = &dfl_bus_type;
> + dfl_dev->dev.release = release_dfl_dev;
> + dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> + feature->index);
Or it's better to have a generic name for the device on the bus.
> +
> + dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
> + ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev-
> >mmio_res);
> + if (ret) {
> + dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> + dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
> + goto free_irqs;
> + }
> +
> + ret = device_register(&dfl_dev->dev);
> + if (ret) {
> + put_device(&dfl_dev->dev);
> + return ERR_PTR(ret);
> + }
> +
> + dev_info(&pdev->dev, "add dfl_dev: %s\n",
> + dev_name(&dfl_dev->dev));
> + return dfl_dev;
> +
> +free_irqs:
> + kfree(dfl_dev->irqs);
> +free_dfl_dev:
> + kfree(dfl_dev);
> + return ERR_PTR(ret);
> +}
> +
> +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> +{
> + struct dfl_device *dfl_dev;
> + struct dfl_feature *feature;
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> + if (!feature->ioaddr && feature->priv) {
> + dfl_dev = feature->priv;
> + device_unregister(&dfl_dev->dev);
> + feature->priv = NULL;
> + }
> + }
> +}
> +
> +static int dfl_devs_init(struct platform_device *pdev)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> + struct dfl_feature *feature;
> + struct dfl_device *dfl_dev;
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> + if (feature->ioaddr || feature->priv)
> + continue;
> +
> + dfl_dev = dfl_dev_add(pdata, feature);
> + if (IS_ERR(dfl_dev)) {
> + dfl_devs_uinit(pdata);
> + return PTR_ERR(dfl_dev);
> + }
> +
> + feature->priv = dfl_dev;
If
> + }
> +
> + return 0;
> +}
> +
> +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> +{
> + if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> + return -EINVAL;
> +
> + dfl_drv->drv.owner = owner;
> + dfl_drv->drv.bus = &dfl_bus_type;
> +
> + return driver_register(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(__dfl_driver_register);
> +
> +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> +{
> + driver_unregister(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(dfl_driver_unregister);
> +
> /**
> * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> * @pdev: feature device.
> @@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct
> platform_device *pdev)
> struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> struct dfl_feature *feature;
>
> - dfl_fpga_dev_for_each_feature(pdata, feature)
> + dfl_devs_uinit(pdata);
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> if (feature->ops) {
> if (feature->ops->uinit)
> feature->ops->uinit(pdev, feature);
> feature->ops = NULL;
> }
> + }
> }
> EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
>
> @@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct
> platform_device *pdev,
> drv++;
> }
>
> + ret = dfl_devs_init(pdev);
> + if (ret)
> + goto exit;
> +
> return 0;
> exit:
> dfl_fpga_dev_feature_uinit(pdev);
> @@ -553,6 +776,8 @@ static int build_info_commit_dev(struct
> build_feature_devs_info *binfo)
> struct dfl_feature_irq_ctx *ctx;
> unsigned int i;
>
> + feature->index = index;
> +
> /* save resource information for each feature */
> feature->dev = fdev;
> feature->id = finfo->fid;
> @@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
> {
> int ret;
>
> + ret = bus_register(&dfl_bus_type);
> + if (ret)
> + return ret;
> +
> dfl_ids_init();
>
> ret = dfl_chardev_init();
> - if (ret)
> + if (ret) {
> dfl_ids_destroy();
> + bus_unregister(&dfl_bus_type);
> + }
>
> return ret;
> }
> @@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
> {
> dfl_chardev_uinit();
> dfl_ids_destroy();
> + bus_unregister(&dfl_bus_type);
> }
>
> module_init(dfl_fpga_init);
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index f605c28..d00aa1c 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
> *
> * @dev: ptr to pdev of the feature device which has the sub feature.
> * @id: sub feature id.
> + * @index: unique identifier for an sub feature within the feature device.
> + * It is possible that multiply sub features with same feature id are
> + * listed in one feature device. So an incremental index (start from 0)
> + * is needed to identify each sub feature.
> * @resource_index: each sub feature has one mmio resource for its
> registers.
> * this index is used to find its mmio resource from the
> * feature dev (platform device)'s reources.
> @@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
> struct dfl_feature {
> struct platform_device *dev;
> u64 id;
> + int index;
> int resource_index;
> void __iomem *ioaddr;
> struct dfl_feature_irq_ctx *irq_ctx;
> @@ -515,4 +520,84 @@ 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,
> + PORT_ID,
> + DFL_ID_MAX,
> +};
> +
> +/**
> + * struct dfl_device_id - dfl device identifier
> + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> + * @driver_data: Driver specific data
> + */
> +struct dfl_device_id {
> + unsigned int type;
> + unsigned long long feature_id;
> + unsigned long driver_data;
Seems not used yet for driver_data, or can be in later patch with real users.
> +};
> +
> +/**
> + * struct dfl_device - represent an dfl device on dfl bus
> + *
> + * @dev: Generic device interface.
> + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> + * @feature_id: 64 bits 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;
> + unsigned int type;
> + unsigned long long 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.
> + * @probe: 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);
> + int (*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
/*
* module_dfl_driver()
> + * 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 */
BTW: maybe it's better to have one patch to add a driver using this bus as an example?
Thanks
Hao
> --
> 2.7.4
On 7/16/20 8:48 PM, Wu, Hao wrote:
>> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
>>
>> Generally i think this is a good approach.
>>
>> However I do have concern.
>>
>> The feature_id in dfl is magic number, similar to pci id but without a vendor
>> id.
>>
>> Is it possible to add something like a vendor id so different vendors would
>> not have to be so careful to use a unique id ?
> Hi Tom,
>
> Thanks for the comments.
>
> Here is only one field defined in spec, that is feature id to distinguish one
> feature with another one. There is no vendor id here I think, and several
> cards are using this for now and seems not possible to change DFH format
> for these products. : (
There looks like some unused bits in the first word, how about
#define DFH_EOL BIT_ULL(40) /* End of list */
+define DFH_VENDOR GENMAKE_ULL(49,41) /* Vendor ID */
#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
And Intel gets id 0.
>
> I fully understand the concern is the feature id management, and potential
> conflicts there from different vendors. One possible method to resolve this
> is managing the ids in a public place (web? Or just the driver header file for
> feature id definition), all vendors can request some feature ids, and other
> people can see which feature ids have been used so that they can avoid
> using conflict ones with other people.
The conflict will come in development when two vendors use the same unpublished feature id.
Also keeping the truth of id's in the kernel source isn't that great because the public kernel always lags development repositories.
>
> And in the later version DFH, this feature id will be replaced with GUID
> I think, so it can resolve the conflict problems from different vendors?
> But now, we still need to handle the existing ones. : )
This is why I proposed needing to generalize the matching.
>
> Thanks
> Hao
>
>> This touches some of the matching function of the bus ops. Could there be a
>> way for bus ops to be used so that there could be multiple matching
>> function. Maybe the one in 0002 as a default so users could override it ?
>>
>> The use case I am worrying about is an OEM. The OEM uses an unclaimed
>> feature_id and supplies their own platform device device driver to match the
>> feature_id. But some later version of the kernel claims this feature_id and
>> the OEM's driver no longer works and since the value comes from pci config
>> space it is difficult to change.
>>
>> So looking for something like
>>
>> register_feature_matcher((*bus_match)(struct device *dev, struct
>> device_driver *drv))
>>
>> static int dfl_bus_match_default(struct device *dev, struct device_driver *drv)
>> {
>> struct dfl_device *dfl_dev = to_dfl_dev(dev);
>> struct dfl_driver *dfl_drv = to_dfl_drv(drv);
>> const struct dfl_device_id *id_entry = dfl_drv->id_table;
>>
>> while (id_entry->feature_id) {
>> if (dfl_match_one_device(id_entry, dfl_dev)) {
>> dfl_dev->id_entry = id_entry;
>> return 1;
>> }
>> id_entry++;
>> }
>>
>> return 0;
>> }
>>
>> register_feature_matcher(&dfl_bus_match_default)
>>
>> static int dfl_bus_match(struct device *dev, struct device_driver *drv)
>> {
>>
>> struct dfl_device *dfl_dev = to_dfl_dev(dev);
>> struct dfl_driver *dfl_drv = to_dfl_drv(drv);
>> const struct dfl_device_id *id_entry = dfl_drv->id_table;
>>
>> while (id_entry->feature_id) {
>>
>> matcher = Loop over matchers()
>>
>> if (matcher(dev, drv)) {
>> dfl_dev->id_entry = id_entry;
>> return 1;
>> }
>> id_entry++;
>> }
>>
>> return 0;
>> }
>>
>> Or maybe use some of the unused bits in the dfl header to add a second
>> vendor-like id and change existing matcher to look feature_id and
>> vendor_like_id.
>>
>> Tom
>>
>>
>>
>> On 7/14/20 10:38 PM, Xu Yilun wrote:
>>> This patchset makes it possible to develop independent driver modules
>>> for DFL private features. It also helps to leverage existing kernel
>>> drivers to enable some IP blocks in DFL.
>>>
>>> Xu Yilun (2):
>>> fpga: dfl: map feature mmio resources in their own feature drivers
>>> fpga: dfl: create a dfl bus type to support DFL devices
>>>
>>> Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
>>> drivers/fpga/dfl-pci.c | 21 +-
>>> drivers/fpga/dfl.c | 435 +++++++++++++++++++++++++++-----
>>> drivers/fpga/dfl.h | 91 ++++++-
>>> 4 files changed, 492 insertions(+), 70 deletions(-)
>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
>>>
More small stuff.
Refactoring for feature_id conflict covered in other email.
Tom
On 7/14/20 10:38 PM, Xu Yilun wrote:
> A new bus type "dfl" is introduced for private features which are not
> initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> private features could be handled by separate driver modules.
>
> DFL framework will create DFL devices on enumeration. DFL drivers could
> be registered on this bus to match these DFL devices. They are matched by
> dfl type & feature_id.
>
> Signed-off-by: Xu Yilun <[email protected]>
> Signed-off-by: Wu Hao <[email protected]>
> Signed-off-by: Matthew Gerlach <[email protected]>
> Signed-off-by: Russ Weight <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
> drivers/fpga/dfl.c | 248 ++++++++++++++++++++++++++++++--
> drivers/fpga/dfl.h | 85 +++++++++++
> 3 files changed, 340 insertions(+), 8 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> new file mode 100644
> index 0000000..cd00abc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> @@ -0,0 +1,15 @@
> +What: /sys/bus/dfl/devices/.../type
> +Date: March 2020
> +KernelVersion: 5.7
5.8
> +Contact: Xu Yilun <[email protected]>
> +Description: Read-only. It returns type of DFL FIU of the device. Now DFL
> + supports 2 FIU types, 0 for FME, 1 for PORT.
> + Format: 0x%x
> +
> +What: /sys/bus/dfl/devices/.../feature_id
> +Date: March 2020
> +KernelVersion: 5.7
> +Contact: Xu Yilun <[email protected]>
> +Description: Read-only. It returns feature identifier local to its DFL FIU
> + type.
> + Format: 0x%llx
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 7dc6411..93f9d6d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
> * index to dfl_chardevs table. If no chardev support just set devt_type
> * as one invalid index (DFL_FPGA_DEVT_MAX).
> */
> -enum dfl_id_type {
> - FME_ID, /* fme id allocation and mapping */
> - PORT_ID, /* port id allocation and mapping */
> - DFL_ID_MAX,
> -};
> -
> enum dfl_fpga_devt_type {
> DFL_FPGA_DEVT_FME,
> DFL_FPGA_DEVT_PORT,
> @@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature *feature)
> return feature->id == FEATURE_ID_FIU_HEADER;
> }
>
> +static const struct dfl_device_id *
> +dfl_match_one_device(const struct dfl_device_id *id,
> + struct dfl_device *dfl_dev)
> +{
> + if (id->type == dfl_dev->type &&
> + id->feature_id == dfl_dev->feature_id)
> + return id;
> +
> + return NULL;
> +}
> +
> +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> + struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> + const struct dfl_device_id *id_entry = dfl_drv->id_table;
Null check ?
> +
> + while (id_entry->feature_id) {
Null check or document table has a sentinel.
> + if (dfl_match_one_device(id_entry, dfl_dev)) {
> + dfl_dev->id_entry = id_entry;
> + return 1;
> + }
> + id_entry++;
> + }
> +
> + return 0;
> +}
> +
> +static int dfl_bus_probe(struct device *dev)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> + struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> +
> + return dfl_drv->probe(dfl_dev);
> +}
> +
> +static int dfl_bus_remove(struct device *dev)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> + struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> +
> + if (dfl_drv->remove)
> + dfl_drv->remove(dfl_dev);
return dfl_drv->remove()
> +
> + return 0;
> +}
> +
> +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +
> + if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
> + dfl_dev->type, dfl_dev->feature_id))
> + return -ENOMEM;
can simplify, change to
return add_uevent_var(...)
> +
> + return 0;
> +}
> +
> +/* show dfl info fields */
> +#define dfl_info_attr(field, format_string) \
> +static ssize_t \
> +field##_show(struct device *dev, struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + struct dfl_device *dfl_dev = to_dfl_dev(dev); \
> + \
> + return sprintf(buf, format_string, dfl_dev->field); \
> +} \
> +static DEVICE_ATTR_RO(field)
> +
> +dfl_info_attr(type, "0x%x\n");
> +dfl_info_attr(feature_id, "0x%llx\n");
> +
> +static struct attribute *dfl_dev_attrs[] = {
> + &dev_attr_type.attr,
> + &dev_attr_feature_id.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(dfl_dev);
> +
> +static struct bus_type dfl_bus_type = {
> + .name = "dfl",
> + .match = dfl_bus_match,
> + .probe = dfl_bus_probe,
> + .remove = dfl_bus_remove,
> + .uevent = dfl_bus_uevent,
> + .dev_groups = dfl_dev_groups,
> +};
> +
> +static void release_dfl_dev(struct device *dev)
> +{
> + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +
> + release_resource(&dfl_dev->mmio_res);
Where is request_resource, shouldn't it be in dfl_dev_add ?
> + kfree(dfl_dev->irqs);
> + kfree(dfl_dev);
> +}
> +
> +static struct dfl_device *
> +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> + struct dfl_feature *feature)
> +{
> + struct platform_device *pdev = pdata->dev;
> + struct dfl_device *dfl_dev;
> + int i, ret;
> +
> + dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
> + if (!dfl_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dfl_dev->cdev = pdata->dfl_cdev;
> +
> + dfl_dev->mmio_res.parent = &pdev->resource[feature->resource_index];
> + dfl_dev->mmio_res.flags = IORESOURCE_MEM;
> + dfl_dev->mmio_res.start =
> + pdev->resource[feature->resource_index].start;
> + dfl_dev->mmio_res.end = pdev->resource[feature->resource_index].end;
> +
> + /* then add irq resource */
> + if (feature->nr_irqs) {
> + dfl_dev->irqs = kcalloc(feature->nr_irqs,
> + sizeof(*dfl_dev->irqs), GFP_KERNEL);
> + if (!dfl_dev->irqs) {
> + ret = -ENOMEM;
> + goto free_dfl_dev;
> + }
> +
> + for (i = 0; i < feature->nr_irqs; i++)
> + dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
> +
> + dfl_dev->num_irqs = feature->nr_irqs;
> + }
> +
> + dfl_dev->type = feature_dev_id_type(pdev);
> + dfl_dev->feature_id = (unsigned long long)feature->id;
> +
> + dfl_dev->dev.parent = &pdev->dev;
> + dfl_dev->dev.bus = &dfl_bus_type;
> + dfl_dev->dev.release = release_dfl_dev;
> + dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> + feature->index);
this can fail
> +
> + dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
> + ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev->mmio_res);
> + if (ret) {
> + dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> + dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
> + goto free_irqs;
> + }
> +
> + ret = device_register(&dfl_dev->dev);
> + if (ret) {
> + put_device(&dfl_dev->dev);
> + return ERR_PTR(ret);
> + }
> +
> + dev_info(&pdev->dev, "add dfl_dev: %s\n",
> + dev_name(&dfl_dev->dev));
> + return dfl_dev;
> +
> +free_irqs:
> + kfree(dfl_dev->irqs);
> +free_dfl_dev:
> + kfree(dfl_dev);
> + return ERR_PTR(ret);
> +}
> +
> +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> +{
> + struct dfl_device *dfl_dev;
> + struct dfl_feature *feature;
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> + if (!feature->ioaddr && feature->priv) {
> + dfl_dev = feature->priv;
> + device_unregister(&dfl_dev->dev);
> + feature->priv = NULL;
> + }
> + }
> +}
> +
> +static int dfl_devs_init(struct platform_device *pdev)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct dfl_feature *feature;
> + struct dfl_device *dfl_dev;
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> + if (feature->ioaddr || feature->priv)
> + continue;
> +
> + dfl_dev = dfl_dev_add(pdata, feature);
> + if (IS_ERR(dfl_dev)) {
> + dfl_devs_uinit(pdata);
> + return PTR_ERR(dfl_dev);
What happens to dfl_dev's that were successful. Need a clean up ?
> + }
> +
> + feature->priv = dfl_dev;
> + }
> +
> + return 0;
> +}
> +
> +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> +{
> + if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> + return -EINVAL;
> +
> + dfl_drv->drv.owner = owner;
> + dfl_drv->drv.bus = &dfl_bus_type;
> +
> + return driver_register(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(__dfl_driver_register);
> +
> +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> +{
> + driver_unregister(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(dfl_driver_unregister);
> +
> /**
> * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> * @pdev: feature device.
> @@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
> struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> struct dfl_feature *feature;
>
> - dfl_fpga_dev_for_each_feature(pdata, feature)
> + dfl_devs_uinit(pdata);
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> if (feature->ops) {
> if (feature->ops->uinit)
> feature->ops->uinit(pdev, feature);
> feature->ops = NULL;
> }
> + }
> }
> EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
>
> @@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> drv++;
> }
>
> + ret = dfl_devs_init(pdev);
> + if (ret)
> + goto exit;
> +
> return 0;
> exit:
> dfl_fpga_dev_feature_uinit(pdev);
> @@ -553,6 +776,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> struct dfl_feature_irq_ctx *ctx;
> unsigned int i;
>
> + feature->index = index;
> +
> /* save resource information for each feature */
> feature->dev = fdev;
> feature->id = finfo->fid;
> @@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
> {
> int ret;
>
> + ret = bus_register(&dfl_bus_type);
> + if (ret)
> + return ret;
> +
> dfl_ids_init();
>
> ret = dfl_chardev_init();
> - if (ret)
> + if (ret) {
> dfl_ids_destroy();
> + bus_unregister(&dfl_bus_type);
> + }
>
> return ret;
> }
> @@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
> {
> dfl_chardev_uinit();
> dfl_ids_destroy();
> + bus_unregister(&dfl_bus_type);
> }
>
> module_init(dfl_fpga_init);
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index f605c28..d00aa1c 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
> *
> * @dev: ptr to pdev of the feature device which has the sub feature.
> * @id: sub feature id.
> + * @index: unique identifier for an sub feature within the feature device.
> + * It is possible that multiply sub features with same feature id are
> + * listed in one feature device. So an incremental index (start from 0)
> + * is needed to identify each sub feature.
> * @resource_index: each sub feature has one mmio resource for its registers.
> * this index is used to find its mmio resource from the
> * feature dev (platform device)'s reources.
> @@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
> struct dfl_feature {
> struct platform_device *dev;
> u64 id;
> + int index;
> int resource_index;
> void __iomem *ioaddr;
> struct dfl_feature_irq_ctx *irq_ctx;
> @@ -515,4 +520,84 @@ 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,
> + PORT_ID,
> + DFL_ID_MAX,
> +};
> +
> +/**
> + * struct dfl_device_id - dfl device identifier
> + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> + * @driver_data: Driver specific data
> + */
> +struct dfl_device_id {
> + unsigned int type;
> + unsigned long long feature_id;
> + unsigned long driver_data;
> +};
> +
> +/**
> + * struct dfl_device - represent an dfl device on dfl bus
> + *
> + * @dev: Generic device interface.
> + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> + * @feature_id: 64 bits 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;
> + unsigned int type;
> + unsigned long long 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.
> + * @probe: 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);
> + int (*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 */
> On 7/16/20 8:48 PM, Wu, Hao wrote:
> >> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
> >>
> >> Generally i think this is a good approach.
> >>
> >> However I do have concern.
> >>
> >> The feature_id in dfl is magic number, similar to pci id but without a
> vendor
> >> id.
> >>
> >> Is it possible to add something like a vendor id so different vendors would
> >> not have to be so careful to use a unique id ?
> > Hi Tom,
> >
> > Thanks for the comments.
> >
> > Here is only one field defined in spec, that is feature id to distinguish one
> > feature with another one. There is no vendor id here I think, and several
> > cards are using this for now and seems not possible to change DFH format
> > for these products. : (
>
> There looks like some unused bits in the first word, how about
>
> #define DFH_EOL BIT_ULL(40) /* End of list */
>
> +define DFH_VENDOR GENMAKE_ULL(49,41) /* Vendor ID */
>
> #define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
>
> And Intel gets id 0.
>
> >
> > I fully understand the concern is the feature id management, and potential
> > conflicts there from different vendors. One possible method to resolve this
> > is managing the ids in a public place (web? Or just the driver header file for
> > feature id definition), all vendors can request some feature ids, and other
> > people can see which feature ids have been used so that they can avoid
> > using conflict ones with other people.
>
> The conflict will come in development when two vendors use the same
> unpublished feature id.
>
> Also keeping the truth of id's in the kernel source isn't that great because the
> public kernel always lags development repositories.
I fully understand your point, and it's a good idea to me, but I am not sure if
we can update the spec for DFHv0 at this moment. Let me check with spec
owner about this. Actually I believe we don't want to add anything in driver
code which is not defined in the spec at all. : (
>
> >
> > And in the later version DFH, this feature id will be replaced with GUID
> > I think, so it can resolve the conflict problems from different vendors?
> > But now, we still need to handle the existing ones. : )
>
> This is why I proposed needing to generalize the matching.
Personally I prefer that we can have standard matching functions per DFH
specs.
Yilun, how do you think about this?
Thanks
Hao
On Mon, Jul 20, 2020 at 05:21:43PM +0800, Wu, Hao wrote:
> > On 7/16/20 8:48 PM, Wu, Hao wrote:
> > >> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
> > >>
> > >> Generally i think this is a good approach.
> > >>
> > >> However I do have concern.
> > >>
> > >> The feature_id in dfl is magic number, similar to pci id but without a
> > vendor
> > >> id.
> > >>
> > >> Is it possible to add something like a vendor id so different vendors would
> > >> not have to be so careful to use a unique id ?
> > > Hi Tom,
> > >
> > > Thanks for the comments.
> > >
> > > Here is only one field defined in spec, that is feature id to distinguish one
> > > feature with another one. There is no vendor id here I think, and several
> > > cards are using this for now and seems not possible to change DFH format
> > > for these products. : (
> >
> > There looks like some unused bits in the first word, how about
> >
> > #define DFH_EOL BIT_ULL(40) /* End of list */
> >
> > +define DFH_VENDOR GENMAKE_ULL(49,41) /* Vendor ID */
> >
> > #define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
> >
> > And Intel gets id 0.
> >
> > >
> > > I fully understand the concern is the feature id management, and potential
> > > conflicts there from different vendors. One possible method to resolve this
> > > is managing the ids in a public place (web? Or just the driver header file for
> > > feature id definition), all vendors can request some feature ids, and other
> > > people can see which feature ids have been used so that they can avoid
> > > using conflict ones with other people.
> >
> > The conflict will come in development when two vendors use the same
> > unpublished feature id.
> >
> > Also keeping the truth of id's in the kernel source isn't that great because the
> > public kernel always lags development repositories.
>
> I fully understand your point, and it's a good idea to me, but I am not sure if
> we can update the spec for DFHv0 at this moment. Let me check with spec
> owner about this. Actually I believe we don't want to add anything in driver
> code which is not defined in the spec at all. : (
>
> >
> > >
> > > And in the later version DFH, this feature id will be replaced with GUID
> > > I think, so it can resolve the conflict problems from different vendors?
> > > But now, we still need to handle the existing ones. : )
> >
> > This is why I proposed needing to generalize the matching.
>
> Personally I prefer that we can have standard matching functions per DFH
> specs.
>
> Yilun, how do you think about this?
I also prefer we don't add anything now before DFL spec is updated.
The idea of extending the DFLv0 is good, but I'm not sure if it is necessary
now. It depends on how the customer is using the card, are they
developing more FME features themselves on current cards and want to make
them public.
I think we could discuss about the strategy. And we could add another
patchset if we finally decide and update the DFLv0 spec.
Thanks
Yilun
>
> Thanks
> Hao
>
On Fri, Jul 17, 2020 at 06:26:37PM +0800, Wu, Hao wrote:
> > -----Original Message-----
> > From: Xu, Yilun <[email protected]>
> > Sent: Wednesday, July 15, 2020 1:38 PM
> > To: [email protected]; [email protected]; linux-
> > [email protected]
> > Cc: [email protected]; [email protected]; Xu, Yilun <[email protected]>;
> > Wu, Hao <[email protected]>; Matthew Gerlach
> > <[email protected]>; Weight, Russell H
> > <[email protected]>
> > Subject: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices
> >
> > A new bus type "dfl" is introduced for private features which are not
> > initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> > private features could be handled by separate driver modules.
> >
> > DFL framework will create DFL devices on enumeration.
>
> Actually these DFL devices are created in AFU/FME driver initialization or real
> core DFL code, is my understanding correct?
Yes I could change the comments.
>
> > DFL drivers could
> > be registered on this bus to match these DFL devices. They are matched by
> > dfl type & feature_id.
> >
> > Signed-off-by: Xu Yilun <[email protected]>
> > Signed-off-by: Wu Hao <[email protected]>
> > Signed-off-by: Matthew Gerlach <[email protected]>
> > Signed-off-by: Russ Weight <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
> > drivers/fpga/dfl.c | 248 ++++++++++++++++++++++++++++++--
> > drivers/fpga/dfl.h | 85 +++++++++++
> > 3 files changed, 340 insertions(+), 8 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl
> > b/Documentation/ABI/testing/sysfs-bus-dfl
> > new file mode 100644
> > index 0000000..cd00abc
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > @@ -0,0 +1,15 @@
> > +What:/sys/bus/dfl/devices/.../type
> > +Date:March 2020
> > +KernelVersion:5.7
>
> I guess you need to update the date and target kernel version.
Yes
>
> > +Contact:Xu Yilun <[email protected]>
> > +Description:Read-only. It returns type of DFL FIU of the device. Now DFL
> > +supports 2 FIU types, 0 for FME, 1 for PORT.
> > +Format: 0x%x
>
> Or consider just print the string instead here?
I think we don't have to. Keeping it align with dfl_device_id.type may
be better.
>
> > +
> > +What:/sys/bus/dfl/devices/.../feature_id
> > +Date:March 2020
> > +KernelVersion:5.7
>
> Ditto.
Yes
>
> > +Contact:Xu Yilun <[email protected]>
> > +Description:Read-only. It returns feature identifier local to its DFL FIU
> > +type.
> > +Format: 0x%llx
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 7dc6411..93f9d6d 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
> > * index to dfl_chardevs table. If no chardev support just set devt_type
> > * as one invalid index (DFL_FPGA_DEVT_MAX).
> > */
> > -enum dfl_id_type {
> > -FME_ID,/* fme id allocation and mapping */
> > -PORT_ID,/* port id allocation and mapping */
> > -DFL_ID_MAX,
> > -};
> > -
> > enum dfl_fpga_devt_type {
> > DFL_FPGA_DEVT_FME,
> > DFL_FPGA_DEVT_PORT,
> > @@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature
> > *feature)
> > return feature->id == FEATURE_ID_FIU_HEADER;
> > }
> >
> > +static const struct dfl_device_id *
> > +dfl_match_one_device(const struct dfl_device_id *id,
> > + struct dfl_device *dfl_dev)
>
> Why start a new line here, it's just 80 char here. : )
> BTW: you can use ddev instead of dfl_dev here for a shorter name.
Yes.
>
> > +{
> > +if (id->type == dfl_dev->type &&
> > + id->feature_id == dfl_dev->feature_id)
> > +return id;
> > +
> > +return NULL;
> > +}
> > +
> > +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> > +const struct dfl_device_id *id_entry = dfl_drv->id_table;
> > +
> > +while (id_entry->feature_id) {
> > +if (dfl_match_one_device(id_entry, dfl_dev)) {
> > +dfl_dev->id_entry = id_entry;
> > +return 1;
> > +}
> > +id_entry++;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +static int dfl_bus_probe(struct device *dev)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> > +
> > +return dfl_drv->probe(dfl_dev);
> > +}
> > +
> > +static int dfl_bus_remove(struct device *dev)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> > +
> > +if (dfl_drv->remove)
> > +dfl_drv->remove(dfl_dev);
> > +
> > +return 0;
> > +}
> > +
> > +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +
> > +if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
> > + dfl_dev->type, dfl_dev->feature_id))
>
> I see for pci bus, it's using v%08Xd%08X... should we consider adding one
> "t" to indicate that value is for type? Will that be simpler to the users?
Yes I could add the tags, maybe "dfl:t%08xf%016llx". So it will not
cause conflicted modalias if a new id field is added.
>
> > +return -ENOMEM;
> > +
> > +return 0;
> > +}
> > +
> > +/* show dfl info fields */
> > +#define dfl_info_attr(field, format_string)\
> > +static ssize_t\
> > +field##_show(struct device *dev, struct device_attribute *attr,
> > \
> > + char *buf)\
> > +{\
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);\
> > +\
> > +return sprintf(buf, format_string, dfl_dev->field);\
> > +}\
> > +static DEVICE_ATTR_RO(field)
> > +
> > +dfl_info_attr(type, "0x%x\n");
> > +dfl_info_attr(feature_id, "0x%llx\n");
> > +
> > +static struct attribute *dfl_dev_attrs[] = {
> > +&dev_attr_type.attr,
> > +&dev_attr_feature_id.attr,
> > +NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(dfl_dev);
> > +
> > +static struct bus_type dfl_bus_type = {
> > +.name= "dfl",
> > +.match= dfl_bus_match,
> > +.probe= dfl_bus_probe,
> > +.remove= dfl_bus_remove,
> > +.uevent= dfl_bus_uevent,
> > +.dev_groups= dfl_dev_groups,
> > +};
> > +
> > +static void release_dfl_dev(struct device *dev)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +
> > +release_resource(&dfl_dev->mmio_res);
> > +kfree(dfl_dev->irqs);
> > +kfree(dfl_dev);
> > +}
> > +
> > +static struct dfl_device *
> > +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> > + struct dfl_feature *feature)
> > +{
> > +struct platform_device *pdev = pdata->dev;
> > +struct dfl_device *dfl_dev;
> > +int i, ret;
> > +
> > +dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
> > +if (!dfl_dev)
> > +return ERR_PTR(-ENOMEM);
> > +
> > +dfl_dev->cdev = pdata->dfl_cdev;
> > +
> > +dfl_dev->mmio_res.parent = &pdev->resource[feature-
> > >resource_index];
> > +dfl_dev->mmio_res.flags = IORESOURCE_MEM;
> > +dfl_dev->mmio_res.start =
> > +pdev->resource[feature->resource_index].start;
> > +dfl_dev->mmio_res.end = pdev->resource[feature-
> > >resource_index].end;
> > +
> > +/* then add irq resource */
> > +if (feature->nr_irqs) {
> > +dfl_dev->irqs = kcalloc(feature->nr_irqs,
> > +sizeof(*dfl_dev->irqs), GFP_KERNEL);
> > +if (!dfl_dev->irqs) {
> > +ret = -ENOMEM;
> > +goto free_dfl_dev;
> > +}
> > +
> > +for (i = 0; i < feature->nr_irqs; i++)
> > +dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
> > +
> > +dfl_dev->num_irqs = feature->nr_irqs;
> > +}
> > +
> > +dfl_dev->type = feature_dev_id_type(pdev);
> > +dfl_dev->feature_id = (unsigned long long)feature->id;
> > +
> > +dfl_dev->dev.parent = &pdev->dev;
> > +dfl_dev->dev.bus = &dfl_bus_type;
> > +dfl_dev->dev.release = release_dfl_dev;
> > +dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> > + feature->index);
>
> Or it's better to have a generic name for the device on the bus.
mm.. It is good suggestion, we should have a unified name for dfl
devices.
How about ("dfl.%d.%d", pdev->id, feature->index)
>
> > +
> > +dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
> > +ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev-
> > >mmio_res);
> > +if (ret) {
> > +dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> > +dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
> > +goto free_irqs;
> > +}
> > +
> > +ret = device_register(&dfl_dev->dev);
> > +if (ret) {
> > +put_device(&dfl_dev->dev);
> > +return ERR_PTR(ret);
> > +}
> > +
> > +dev_info(&pdev->dev, "add dfl_dev: %s\n",
> > + dev_name(&dfl_dev->dev));
> > +return dfl_dev;
> > +
> > +free_irqs:
> > +kfree(dfl_dev->irqs);
> > +free_dfl_dev:
> > +kfree(dfl_dev);
> > +return ERR_PTR(ret);
> > +}
> > +
> > +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> > +{
> > +struct dfl_device *dfl_dev;
> > +struct dfl_feature *feature;
> > +
> > +dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +if (!feature->ioaddr && feature->priv) {
> > +dfl_dev = feature->priv;
> > +device_unregister(&dfl_dev->dev);
> > +feature->priv = NULL;
> > +}
> > +}
> > +}
> > +
> > +static int dfl_devs_init(struct platform_device *pdev)
> > +{
> > +struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> > >dev);
> > +struct dfl_feature *feature;
> > +struct dfl_device *dfl_dev;
> > +
> > +dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +if (feature->ioaddr || feature->priv)
> > +continue;
> > +
> > +dfl_dev = dfl_dev_add(pdata, feature);
> > +if (IS_ERR(dfl_dev)) {
> > +dfl_devs_uinit(pdata);
> > +return PTR_ERR(dfl_dev);
> > +}
> > +
> > +feature->priv = dfl_dev;
>
> If
>
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> > +{
> > +if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> > +return -EINVAL;
> > +
> > +dfl_drv->drv.owner = owner;
> > +dfl_drv->drv.bus = &dfl_bus_type;
> > +
> > +return driver_register(&dfl_drv->drv);
> > +}
> > +EXPORT_SYMBOL(__dfl_driver_register);
> > +
> > +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> > +{
> > +driver_unregister(&dfl_drv->drv);
> > +}
> > +EXPORT_SYMBOL(dfl_driver_unregister);
> > +
> > /**
> > * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> > * @pdev: feature device.
> > @@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct
> > platform_device *pdev)
> > struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> > >dev);
> > struct dfl_feature *feature;
> >
> > -dfl_fpga_dev_for_each_feature(pdata, feature)
> > +dfl_devs_uinit(pdata);
> > +
> > +dfl_fpga_dev_for_each_feature(pdata, feature) {
> > if (feature->ops) {
> > if (feature->ops->uinit)
> > feature->ops->uinit(pdev, feature);
> > feature->ops = NULL;
> > }
> > +}
> > }
> > EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
> >
> > @@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct
> > platform_device *pdev,
> > drv++;
> > }
> >
> > +ret = dfl_devs_init(pdev);
> > +if (ret)
> > +goto exit;
> > +
> > return 0;
> > exit:
> > dfl_fpga_dev_feature_uinit(pdev);
> > @@ -553,6 +776,8 @@ static int build_info_commit_dev(struct
> > build_feature_devs_info *binfo)
> > struct dfl_feature_irq_ctx *ctx;
> > unsigned int i;
> >
> > +feature->index = index;
> > +
> > /* save resource information for each feature */
> > feature->dev = fdev;
> > feature->id = finfo->fid;
> > @@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
> > {
> > int ret;
> >
> > +ret = bus_register(&dfl_bus_type);
> > +if (ret)
> > +return ret;
> > +
> > dfl_ids_init();
> >
> > ret = dfl_chardev_init();
> > -if (ret)
> > +if (ret) {
> > dfl_ids_destroy();
> > +bus_unregister(&dfl_bus_type);
> > +}
> >
> > return ret;
> > }
> > @@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
> > {
> > dfl_chardev_uinit();
> > dfl_ids_destroy();
> > +bus_unregister(&dfl_bus_type);
> > }
> >
> > module_init(dfl_fpga_init);
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index f605c28..d00aa1c 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
> > *
> > * @dev: ptr to pdev of the feature device which has the sub feature.
> > * @id: sub feature id.
> > + * @index: unique identifier for an sub feature within the feature device.
> > + * It is possible that multiply sub features with same feature id are
> > + * listed in one feature device. So an incremental index (start from 0)
> > + * is needed to identify each sub feature.
> > * @resource_index: each sub feature has one mmio resource for its
> > registers.
> > * this index is used to find its mmio resource from the
> > * feature dev (platform device)'s reources.
> > @@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
> > struct dfl_feature {
> > struct platform_device *dev;
> > u64 id;
> > +int index;
> > int resource_index;
> > void __iomem *ioaddr;
> > struct dfl_feature_irq_ctx *irq_ctx;
> > @@ -515,4 +520,84 @@ 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,
> > +PORT_ID,
> > +DFL_ID_MAX,
> > +};
> > +
> > +/**
> > + * struct dfl_device_id - dfl device identifier
> > + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> > + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> > + * @driver_data: Driver specific data
> > + */
> > +struct dfl_device_id {
> > +unsigned int type;
> > +unsigned long long feature_id;
> > +unsigned long driver_data;
>
> Seems not used yet for driver_data, or can be in later patch with real users.
I think we may keep this. Cause modpost also need this struct
dfl_device_id, I think it would be better we don't frequently change the
struct to avoid sync problem between kernel & modpost.
>
> > +};
> > +
> > +/**
> > + * struct dfl_device - represent an dfl device on dfl bus
> > + *
> > + * @dev: Generic device interface.
> > + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> > + * @feature_id: 64 bits 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;
> > +unsigned int type;
> > +unsigned long long 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.
> > + * @probe: 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);
> > +int (*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
>
> /*
> * module_dfl_driver()
Yes
>
> > + * 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 */
>
> BTW: maybe it's better to have one patch to add a driver using this bus as an example?
Yes I can also sent a dfl_n3000_nios driver in next version
Thanks,
Yilun
>
> Thanks
> Hao
>
> > --
> > 2.7.4
On Fri, Jul 17, 2020 at 12:47:14PM -0700, Tom Rix wrote:
> More small stuff.
>
> Refactoring for feature_id conflict covered in other email.
>
> Tom
>
>
> On 7/14/20 10:38 PM, Xu Yilun wrote:
> > A new bus type "dfl" is introduced for private features which are not
> > initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> > private features could be handled by separate driver modules.
> >
> > DFL framework will create DFL devices on enumeration. DFL drivers could
> > be registered on this bus to match these DFL devices. They are matched by
> > dfl type & feature_id.
> >
> > Signed-off-by: Xu Yilun <[email protected]>
> > Signed-off-by: Wu Hao <[email protected]>
> > Signed-off-by: Matthew Gerlach <[email protected]>
> > Signed-off-by: Russ Weight <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
> > drivers/fpga/dfl.c | 248 ++++++++++++++++++++++++++++++--
> > drivers/fpga/dfl.h | 85 +++++++++++
> > 3 files changed, 340 insertions(+), 8 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> > new file mode 100644
> > index 0000000..cd00abc
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > @@ -0,0 +1,15 @@
> > +What: /sys/bus/dfl/devices/.../type
> > +Date: March 2020
> > +KernelVersion: 5.7
> 5.8
Yes, think it should be 5.9 for now.
> > +Contact: Xu Yilun <[email protected]>
> > +Description: Read-only. It returns type of DFL FIU of the device. Now DFL
> > + supports 2 FIU types, 0 for FME, 1 for PORT.
> > + Format: 0x%x
> > +
> > +What: /sys/bus/dfl/devices/.../feature_id
> > +Date: March 2020
> > +KernelVersion: 5.7
> > +Contact: Xu Yilun <[email protected]>
> > +Description: Read-only. It returns feature identifier local to its DFL FIU
> > + type.
> > + Format: 0x%llx
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 7dc6411..93f9d6d 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
> > * index to dfl_chardevs table. If no chardev support just set devt_type
> > * as one invalid index (DFL_FPGA_DEVT_MAX).
> > */
> > -enum dfl_id_type {
> > - FME_ID, /* fme id allocation and mapping */
> > - PORT_ID, /* port id allocation and mapping */
> > - DFL_ID_MAX,
> > -};
> > -
> > enum dfl_fpga_devt_type {
> > DFL_FPGA_DEVT_FME,
> > DFL_FPGA_DEVT_PORT,
> > @@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature *feature)
> > return feature->id == FEATURE_ID_FIU_HEADER;
> > }
> >
> > +static const struct dfl_device_id *
> > +dfl_match_one_device(const struct dfl_device_id *id,
> > + struct dfl_device *dfl_dev)
> > +{
> > + if (id->type == dfl_dev->type &&
> > + id->feature_id == dfl_dev->feature_id)
> > + return id;
> > +
> > + return NULL;
> > +}
> > +
> > +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> > +{
> > + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > + struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> > + const struct dfl_device_id *id_entry = dfl_drv->id_table;
> Null check ?
Yes. Thanks for catching this.
> > +
> > + while (id_entry->feature_id) {
> Null check or document table has a sentinel.
I'll document that table needs a sentinel.
> > + if (dfl_match_one_device(id_entry, dfl_dev)) {
> > + dfl_dev->id_entry = id_entry;
> > + return 1;
> > + }
> > + id_entry++;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int dfl_bus_probe(struct device *dev)
> > +{
> > + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > + struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> > +
> > + return dfl_drv->probe(dfl_dev);
> > +}
> > +
> > +static int dfl_bus_remove(struct device *dev)
> > +{
> > + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > + struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> > +
> > + if (dfl_drv->remove)
> > + dfl_drv->remove(dfl_dev);
> return dfl_drv->remove()
I think we could define void (*remove)(struct dfl_device *dfl_dev) for
dfl_driver.remove
> > +
> > + return 0;
> > +}
> > +
> > +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > +{
> > + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +
> > + if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
> > + dfl_dev->type, dfl_dev->feature_id))
> > + return -ENOMEM;
>
> can simplify, change to
>
> return add_uevent_var(...)
Yes
>
> > +
> > + return 0;
> > +}
> > +
> > +/* show dfl info fields */
> > +#define dfl_info_attr(field, format_string) \
> > +static ssize_t \
> > +field##_show(struct device *dev, struct device_attribute *attr, \
> > + char *buf) \
> > +{ \
> > + struct dfl_device *dfl_dev = to_dfl_dev(dev); \
> > + \
> > + return sprintf(buf, format_string, dfl_dev->field); \
> > +} \
> > +static DEVICE_ATTR_RO(field)
> > +
> > +dfl_info_attr(type, "0x%x\n");
> > +dfl_info_attr(feature_id, "0x%llx\n");
> > +
> > +static struct attribute *dfl_dev_attrs[] = {
> > + &dev_attr_type.attr,
> > + &dev_attr_feature_id.attr,
> > + NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(dfl_dev);
> > +
> > +static struct bus_type dfl_bus_type = {
> > + .name = "dfl",
> > + .match = dfl_bus_match,
> > + .probe = dfl_bus_probe,
> > + .remove = dfl_bus_remove,
> > + .uevent = dfl_bus_uevent,
> > + .dev_groups = dfl_dev_groups,
> > +};
> > +
> > +static void release_dfl_dev(struct device *dev)
> > +{
> > + struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +
> > + release_resource(&dfl_dev->mmio_res);
> Where is request_resource, shouldn't it be in dfl_dev_add ?
insert_resource() is used in dfl_dev_add
> > + kfree(dfl_dev->irqs);
> > + kfree(dfl_dev);
> > +}
> > +
> > +static struct dfl_device *
> > +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> > + struct dfl_feature *feature)
> > +{
> > + struct platform_device *pdev = pdata->dev;
> > + struct dfl_device *dfl_dev;
> > + int i, ret;
> > +
> > + dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
> > + if (!dfl_dev)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + dfl_dev->cdev = pdata->dfl_cdev;
> > +
> > + dfl_dev->mmio_res.parent = &pdev->resource[feature->resource_index];
> > + dfl_dev->mmio_res.flags = IORESOURCE_MEM;
> > + dfl_dev->mmio_res.start =
> > + pdev->resource[feature->resource_index].start;
> > + dfl_dev->mmio_res.end = pdev->resource[feature->resource_index].end;
> > +
> > + /* then add irq resource */
> > + if (feature->nr_irqs) {
> > + dfl_dev->irqs = kcalloc(feature->nr_irqs,
> > + sizeof(*dfl_dev->irqs), GFP_KERNEL);
> > + if (!dfl_dev->irqs) {
> > + ret = -ENOMEM;
> > + goto free_dfl_dev;
> > + }
> > +
> > + for (i = 0; i < feature->nr_irqs; i++)
> > + dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
> > +
> > + dfl_dev->num_irqs = feature->nr_irqs;
> > + }
> > +
> > + dfl_dev->type = feature_dev_id_type(pdev);
> > + dfl_dev->feature_id = (unsigned long long)feature->id;
> > +
> > + dfl_dev->dev.parent = &pdev->dev;
> > + dfl_dev->dev.bus = &dfl_bus_type;
> > + dfl_dev->dev.release = release_dfl_dev;
> > + dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> > + feature->index);
> this can fail
Yes, I could add the check.
> > +
> > + dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
> > + ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev->mmio_res);
> > + if (ret) {
> > + dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> > + dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
> > + goto free_irqs;
> > + }
> > +
> > + ret = device_register(&dfl_dev->dev);
> > + if (ret) {
> > + put_device(&dfl_dev->dev);
> > + return ERR_PTR(ret);
> > + }
> > +
> > + dev_info(&pdev->dev, "add dfl_dev: %s\n",
> > + dev_name(&dfl_dev->dev));
> > + return dfl_dev;
> > +
> > +free_irqs:
> > + kfree(dfl_dev->irqs);
> > +free_dfl_dev:
> > + kfree(dfl_dev);
> > + return ERR_PTR(ret);
> > +}
> > +
> > +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> > +{
> > + struct dfl_device *dfl_dev;
> > + struct dfl_feature *feature;
> > +
> > + dfl_fpga_dev_for_each_feature(pdata, feature) {
> > + if (!feature->ioaddr && feature->priv) {
> > + dfl_dev = feature->priv;
> > + device_unregister(&dfl_dev->dev);
> > + feature->priv = NULL;
> > + }
> > + }
> > +}
> > +
> > +static int dfl_devs_init(struct platform_device *pdev)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + struct dfl_feature *feature;
> > + struct dfl_device *dfl_dev;
> > +
> > + dfl_fpga_dev_for_each_feature(pdata, feature) {
> > + if (feature->ioaddr || feature->priv)
> > + continue;
> > +
> > + dfl_dev = dfl_dev_add(pdata, feature);
> > + if (IS_ERR(dfl_dev)) {
> > + dfl_devs_uinit(pdata);
> > + return PTR_ERR(dfl_dev);
> What happens to dfl_dev's that were successful. Need a clean up ?
> > + }
> > +
> > + feature->priv = dfl_dev;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> > +{
> > + if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> > + return -EINVAL;
> > +
> > + dfl_drv->drv.owner = owner;
> > + dfl_drv->drv.bus = &dfl_bus_type;
> > +
> > + return driver_register(&dfl_drv->drv);
> > +}
> > +EXPORT_SYMBOL(__dfl_driver_register);
> > +
> > +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> > +{
> > + driver_unregister(&dfl_drv->drv);
> > +}
> > +EXPORT_SYMBOL(dfl_driver_unregister);
> > +
> > /**
> > * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> > * @pdev: feature device.
> > @@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
> > struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > struct dfl_feature *feature;
> >
> > - dfl_fpga_dev_for_each_feature(pdata, feature)
> > + dfl_devs_uinit(pdata);
> > +
> > + dfl_fpga_dev_for_each_feature(pdata, feature) {
> > if (feature->ops) {
> > if (feature->ops->uinit)
> > feature->ops->uinit(pdev, feature);
> > feature->ops = NULL;
> > }
> > + }
> > }
> > EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
> >
> > @@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> > drv++;
> > }
> >
> > + ret = dfl_devs_init(pdev);
> > + if (ret)
> > + goto exit;
> > +
> > return 0;
> > exit:
> > dfl_fpga_dev_feature_uinit(pdev);
> > @@ -553,6 +776,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> > struct dfl_feature_irq_ctx *ctx;
> > unsigned int i;
> >
> > + feature->index = index;
> > +
> > /* save resource information for each feature */
> > feature->dev = fdev;
> > feature->id = finfo->fid;
> > @@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
> > {
> > int ret;
> >
> > + ret = bus_register(&dfl_bus_type);
> > + if (ret)
> > + return ret;
> > +
> > dfl_ids_init();
> >
> > ret = dfl_chardev_init();
> > - if (ret)
> > + if (ret) {
> > dfl_ids_destroy();
> > + bus_unregister(&dfl_bus_type);
> > + }
> >
> > return ret;
> > }
> > @@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
> > {
> > dfl_chardev_uinit();
> > dfl_ids_destroy();
> > + bus_unregister(&dfl_bus_type);
> > }
> >
> > module_init(dfl_fpga_init);
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index f605c28..d00aa1c 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
> > *
> > * @dev: ptr to pdev of the feature device which has the sub feature.
> > * @id: sub feature id.
> > + * @index: unique identifier for an sub feature within the feature device.
> > + * It is possible that multiply sub features with same feature id are
> > + * listed in one feature device. So an incremental index (start from 0)
> > + * is needed to identify each sub feature.
> > * @resource_index: each sub feature has one mmio resource for its registers.
> > * this index is used to find its mmio resource from the
> > * feature dev (platform device)'s reources.
> > @@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
> > struct dfl_feature {
> > struct platform_device *dev;
> > u64 id;
> > + int index;
> > int resource_index;
> > void __iomem *ioaddr;
> > struct dfl_feature_irq_ctx *irq_ctx;
> > @@ -515,4 +520,84 @@ 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,
> > + PORT_ID,
> > + DFL_ID_MAX,
> > +};
> > +
> > +/**
> > + * struct dfl_device_id - dfl device identifier
> > + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> > + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> > + * @driver_data: Driver specific data
> > + */
> > +struct dfl_device_id {
> > + unsigned int type;
> > + unsigned long long feature_id;
> > + unsigned long driver_data;
> > +};
> > +
> > +/**
> > + * struct dfl_device - represent an dfl device on dfl bus
> > + *
> > + * @dev: Generic device interface.
> > + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> > + * @feature_id: 64 bits 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;
> > + unsigned int type;
> > + unsigned long long 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.
> > + * @probe: 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);
> > + int (*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 */
> > > +}
> > > +
> > > +dfl_dev->type = feature_dev_id_type(pdev);
> > > +dfl_dev->feature_id = (unsigned long long)feature->id;
> > > +
> > > +dfl_dev->dev.parent = &pdev->dev;
> > > +dfl_dev->dev.bus = &dfl_bus_type;
> > > +dfl_dev->dev.release = release_dfl_dev;
> > > +dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> > > + feature->index);
> >
> > Or it's better to have a generic name for the device on the bus.
>
> mm.. It is good suggestion, we should have a unified name for dfl
> devices.
>
> How about ("dfl.%d.%d", pdev->id, feature->index)
It's quite difficult for people to use related information from these magic
numbers. They are not ids defined in the spec, so just dfl_dev.x with one
unique id seems to be better. If you really need to expose some id
information, maybe you can consider adding some standard sysfs entry
to all dfl_dev, I think that will be easier for users. How do you think?
Thanks
Hao
> > +static int dfl_devs_init(struct platform_device *pdev)
> > +{
> > + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + struct dfl_feature *feature;
> > + struct dfl_device *dfl_dev;
> > +
> > + dfl_fpga_dev_for_each_feature(pdata, feature) {
> > + if (feature->ioaddr || feature->priv)
> > + continue;
> > +
> > + dfl_dev = dfl_dev_add(pdata, feature);
> > + if (IS_ERR(dfl_dev)) {
> > + dfl_devs_uinit(pdata);
> > + return PTR_ERR(dfl_dev);
> What happens to dfl_dev's that were successful. Need a clean up ?
Yes, the already added dfl devices under this pdev will be unregistered
in function dfl_devs_uinit()
> > + }
> > +
> > + feature->priv = dfl_dev;
> > + }
> > +
> > + return 0;
> > +}
On Tue, Jul 21, 2020 at 07:41:27PM +0800, Wu, Hao wrote:
> > > > +}
> > > > +
> > > > +dfl_dev->type = feature_dev_id_type(pdev);
> > > > +dfl_dev->feature_id = (unsigned long long)feature->id;
> > > > +
> > > > +dfl_dev->dev.parent = &pdev->dev;
> > > > +dfl_dev->dev.bus = &dfl_bus_type;
> > > > +dfl_dev->dev.release = release_dfl_dev;
> > > > +dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> > > > + feature->index);
> > >
> > > Or it's better to have a generic name for the device on the bus.
> >
> > mm.. It is good suggestion, we should have a unified name for dfl
> > devices.
> >
> > How about ("dfl.%d.%d", pdev->id, feature->index)
>
> It's quite difficult for people to use related information from these magic
> numbers. They are not ids defined in the spec, so just dfl_dev.x with one
> unique id seems to be better. If you really need to expose some id
> information, maybe you can consider adding some standard sysfs entry
> to all dfl_dev, I think that will be easier for users. How do you think?
I'm fine with the dfl_dev.x solution.
>
> Thanks
> Hao