* [PATCH] add the FPGA Device Feature List (DFL) EMIF support
@ 2020-09-08 8:27 Xu Yilun
2020-09-08 9:03 ` Krzysztof Kozlowski
[not found] ` <1599553645-26928-2-git-send-email-yilun.xu@intel.com>
0 siblings, 2 replies; 8+ messages in thread
From: Xu Yilun @ 2020-09-08 8:27 UTC (permalink / raw)
To: krzk, mdf, linux-kernel
Cc: trix, yilun.xu, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu
This patch depend on the patchsets: "Modularization of DFL private
feature drivers" & "add dfl bus support to MODULE_DEVICE_TABLE()"
https://lore.kernel.org/linux-fpga/1599488581-16386-1-git-send-email-yilun.xu@intel.com/
The driver supports the EMIF controller on Intel Programmable
Acceleration Card (PAC). The controller manages the on-board memory of
the PCIe card.
Xu Yilun (1):
memory: dfl-emif: add the DFL EMIF private feature driver
.../ABI/testing/sysfs-bus-dfl-devices-emif | 25 +++
drivers/memory/Kconfig | 9 +
drivers/memory/Makefile | 2 +
drivers/memory/dfl-emif.c | 211 +++++++++++++++++++++
4 files changed, 247 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
create mode 100644 drivers/memory/dfl-emif.c
--
2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] add the FPGA Device Feature List (DFL) EMIF support
2020-09-08 8:27 [PATCH] add the FPGA Device Feature List (DFL) EMIF support Xu Yilun
@ 2020-09-08 9:03 ` Krzysztof Kozlowski
2020-09-09 7:45 ` Xu Yilun
[not found] ` <1599553645-26928-2-git-send-email-yilun.xu@intel.com>
1 sibling, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-08 9:03 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-kernel, trix, matthew.gerlach, russell.h.weight,
lgoncalv, hao.wu
On Tue, Sep 08, 2020 at 04:27:24PM +0800, Xu Yilun wrote:
> This patch depend on the patchsets: "Modularization of DFL private
> feature drivers" & "add dfl bus support to MODULE_DEVICE_TABLE()"
The need for bus I understand but why it depends on the "Modularization
of DFL private feature drivers"?
Anyway I will need a stable tag with mentioned dependencies or this will
wait for the next cycle.
Best regards,
Krzysztof
>
> https://lore.kernel.org/linux-fpga/1599488581-16386-1-git-send-email-yilun.xu@intel.com/
>
> The driver supports the EMIF controller on Intel Programmable
> Acceleration Card (PAC). The controller manages the on-board memory of
> the PCIe card.
>
> Xu Yilun (1):
> memory: dfl-emif: add the DFL EMIF private feature driver
>
> .../ABI/testing/sysfs-bus-dfl-devices-emif | 25 +++
> drivers/memory/Kconfig | 9 +
> drivers/memory/Makefile | 2 +
> drivers/memory/dfl-emif.c | 211 +++++++++++++++++++++
> 4 files changed, 247 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> create mode 100644 drivers/memory/dfl-emif.c
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver
[not found] ` <1599553645-26928-2-git-send-email-yilun.xu@intel.com>
@ 2020-09-08 10:01 ` Krzysztof Kozlowski
2020-09-11 8:42 ` Xu Yilun
2020-09-11 16:13 ` Wu, Hao
1 sibling, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-08 10:01 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-kernel, trix, matthew.gerlach, russell.h.weight,
lgoncalv, hao.wu
On Tue, Sep 08, 2020 at 04:27:25PM +0800, Xu Yilun wrote:
> This driver is for the EMIF private feature implemented under FPGA
> Device Feature List (DFL) framework. It is used to expose memory
> interface status information as well as memory clearing control.
>
> The purpose of memory clearing block is to zero out all private memory
> when FPGA is to be reprogrammed. This gives users a reliable method to
> prevent potential data leakage.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> .../ABI/testing/sysfs-bus-dfl-devices-emif | 25 +++
> drivers/memory/Kconfig | 9 +
> drivers/memory/Makefile | 2 +
> drivers/memory/dfl-emif.c | 211 +++++++++++++++++++++
> 4 files changed, 247 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> create mode 100644 drivers/memory/dfl-emif.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> new file mode 100644
> index 0000000..33d557e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> @@ -0,0 +1,25 @@
> +What: /sys/bus/dfl/devices/dfl_dev.X/infX_cal_fail
> +Date: Sep 2020
> +KernelVersion: 5.10
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Read-only. It indicates if the calibration is failed on this
> + memory interface. "1" for calibration failure, "0" for OK.
"if the calibration failed"
> + Format: %u
> +
> +What: /sys/bus/dfl/devices/dfl_dev.X/infX_init_done
> +Date: Sep 2020
> +KernelVersion: 5.10
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Read-only. It indicates if the initialization is complete on
> + this memory interface. "1" for initialization complete, "0"
> + for not yet.
> + Format: %u
"if the initialization completed"
> +
> +What: /sys/bus/dfl/devices/dfl_dev.X/infX_clear
> +Date: Sep 2020
> +KernelVersion: 5.10
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Write-only. Writing "1" to this file will zero out all memory
> + data in this memory interface. Writing other values are
> + invalid.
"Writing of other values is invalid."
> + Format: %u
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 8072204..fb0858f 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -215,6 +215,15 @@ config STM32_FMC2_EBI
> devices (like SRAM, ethernet adapters, FPGAs, LCD displays, ...) on
> SOCs containing the FMC2 External Bus Interface.
>
> +config FPGA_DFL_EMIF
> + tristate "DFL EMIF private feature support"
> + depends on FPGA_DFL && HAS_IOMEM
Cannot be compile tested without FPGA_DFL?
> + help
> + This driver is for the EMIF private feature implemented under
> + FPGA Device Feature List (DFL) framework. It is used to expose
> + memory interface status information as well as memory clearing
> + control.
Don't add new entries in Makefiles and Kconfig at the end of file, but
rather find appropriate place. Since in Makefile this is next to
CONFIG_TI_EMIF_SRAM, add it also next to it in Kconfig.
> +
> source "drivers/memory/samsung/Kconfig"
> source "drivers/memory/tegra/Kconfig"
>
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index e71cf7b..0afbf39 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -39,3 +39,5 @@ $(obj)/ti-emif-asm-offsets.h: $(obj)/emif-asm-offsets.s FORCE
>
> targets += emif-asm-offsets.s
> clean-files += ti-emif-asm-offsets.h
> +
> +obj-$(CONFIG_FPGA_DFL_EMIF) += dfl-emif.o
> diff --git a/drivers/memory/dfl-emif.c b/drivers/memory/dfl-emif.c
> new file mode 100644
> index 0000000..442137f
> --- /dev/null
> +++ b/drivers/memory/dfl-emif.c
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for DFL EMIF private feature
> + *
> + * Copyright (C) 2020 Intel Corporation, Inc.
> + *
> + */
> +#include <linux/bitfield.h>
> +#include <linux/errno.h>
> +#include <linux/fpga/dfl-bus.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +#include <linux/stddef.h>
Why do you need this header?
> +#include <linux/types.h>
> +
> +#define EMIF_STAT 0x8
> +#define EMIF_STAT_INIT_DONE_SFT 0
> +#define EMIF_STAT_CALC_FAIL_SFT 8
> +#define EMIF_STAT_CLEAR_BUSY_SFT 16
> +#define EMIF_CTRL 0x10
> +#define EMIF_CTRL_CLEAR_EN_SFT 0
> +#define EMIF_CTRL_CLEAR_EN_MSK GENMASK_ULL(3, 0)
> +
> +#define EMIF_POLL_INVL 10000 /* us */
> +#define EMIF_POLL_TIMEOUT 5000000 /* us */
> +
> +struct dfl_emif {
> + struct device *dev;
> + void __iomem *base;
> + spinlock_t lock; /* Serialises access to EMIF_CTRL reg */
> +};
> +
> +struct emif_attr {
> + struct device_attribute attr;
> + u32 shift;
> + u32 index;
> +};
> +
> +#define to_emif_attr(dev_attr) \
> + container_of(dev_attr, struct emif_attr, attr)
> +
> +static ssize_t emif_state_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct emif_attr *eattr = to_emif_attr(attr);
> + struct dfl_emif *de = dev_get_drvdata(dev);
> + u64 val;
> +
> + val = readq(de->base + EMIF_STAT);
> +
> + return sprintf(buf, "%u\n",
> + !!(val & BIT_ULL(eattr->shift + eattr->index)));
> +}
> +
> +static ssize_t emif_clear_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct emif_attr *eattr = to_emif_attr(attr);
> + struct dfl_emif *de = dev_get_drvdata(dev);
> + u64 clear_busy_msk, clear_en_msk, val;
> + void __iomem *base = de->base;
> +
> + if (!sysfs_streq(buf, "1"))
> + return -EINVAL;
> +
> + clear_busy_msk = BIT_ULL(EMIF_STAT_CLEAR_BUSY_SFT + eattr->index);
> + clear_en_msk = BIT_ULL(EMIF_CTRL_CLEAR_EN_SFT + eattr->index);
> +
> + spin_lock(&de->lock);
> + /* The CLEAR_EN field is WO, but other fields are RW */
> + val = readq(base + EMIF_CTRL);
> + val &= ~EMIF_CTRL_CLEAR_EN_MSK;
> + val |= clear_en_msk;
> + writeq(val, base + EMIF_CTRL);
> + spin_unlock(&de->lock);
> +
> + if (readq_poll_timeout(base + EMIF_STAT, val,
> + !(val & clear_busy_msk),
> + EMIF_POLL_INVL, EMIF_POLL_TIMEOUT)) {
> + dev_err(de->dev, "timeout, fail to clear\n");
> + return -ETIMEDOUT;
> + }
> +
> + return count;
> +}
> +
> +#define emif_state_attr(_name, _shift, _index) \
> + struct emif_attr emif_attr_##inf##_index##_##_name = \
> + { .attr = __ATTR(inf##_index##_##_name, 0444, \
> + emif_state_show, NULL), \
> + .shift = (_shift), .index = (_index) }
> +
> +#define emif_clear_attr(_index) \
> + struct emif_attr emif_attr_##inf##_index##_clear = \
> + { .attr = __ATTR(inf##_index##_clear, 0200, \
> + NULL, emif_clear_store), \
> + .index = (_index) }
> +
> +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 0);
> +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 1);
> +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 2);
> +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 3);
> +
> +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 0);
> +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 1);
> +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 2);
> +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 3);
> +
> +emif_clear_attr(0);
> +emif_clear_attr(1);
> +emif_clear_attr(2);
> +emif_clear_attr(3);
> +
> +static struct attribute *dfl_emif_attrs[] = {
> + &emif_attr_inf0_init_done.attr.attr,
> + &emif_attr_inf0_cal_fail.attr.attr,
> + &emif_attr_inf0_clear.attr.attr,
> +
> + &emif_attr_inf1_init_done.attr.attr,
> + &emif_attr_inf1_cal_fail.attr.attr,
> + &emif_attr_inf1_clear.attr.attr,
> +
> + &emif_attr_inf2_init_done.attr.attr,
> + &emif_attr_inf2_cal_fail.attr.attr,
> + &emif_attr_inf2_clear.attr.attr,
> +
> + &emif_attr_inf3_init_done.attr.attr,
> + &emif_attr_inf3_cal_fail.attr.attr,
> + &emif_attr_inf3_clear.attr.attr,
> +
> + NULL,
> +};
> +
> +static umode_t dfl_emif_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + struct dfl_emif *de = dev_get_drvdata(kobj_to_dev(kobj));
> + struct emif_attr *eattr = container_of(attr, struct emif_attr,
> + attr.attr);
> + u64 val;
> +
> + /*
> + * This device supports upto 4 memory interfaces, but not all
> + * interfaces are used on different platforms. The read out value of
> + * CLEAN_EN field (which is a bitmap) could tell how many interfaces
> + * are available.
> + */
> + val = FIELD_GET(EMIF_CTRL_CLEAR_EN_MSK, readq(de->base + EMIF_CTRL));
> +
> + return (val & BIT_ULL(eattr->index)) ? attr->mode : 0;
> +}
> +
> +static const struct attribute_group dfl_emif_group = {
> + .is_visible = dfl_emif_visible,
> + .attrs = dfl_emif_attrs,
> +};
> +
> +static const struct attribute_group *dfl_emif_groups[] = {
> + &dfl_emif_group,
> + NULL,
> +};
> +
> +static int dfl_emif_probe(struct dfl_device *dfl_dev)
> +{
> + struct device *dev = &dfl_dev->dev;
> + struct dfl_emif *de;
> +
> + de = devm_kzalloc(dev, sizeof(*de), GFP_KERNEL);
> + if (!de)
> + return -ENOMEM;
> +
> + de->base = devm_ioremap_resource(&dfl_dev->dev, &dfl_dev->mmio_res);
You just stored device under *dev.
> + if (IS_ERR(de->base)) {
> + dev_err(dev, "get mem resource fail!\n");
No need for printing error. Core will handle it.
> + return PTR_ERR(de->base);
> + }
> +
> + de->dev = dev;
> + spin_lock_init(&de->lock);
> + dev_set_drvdata(dev, de);
> +
> + return 0;
> +}
> +
> +#define FME_FEATURE_ID_EMIF 0x9
Put defines at the beginning of file.
> +
> +static const struct dfl_device_id dfl_emif_ids[] = {
> + { FME_ID, FME_FEATURE_ID_EMIF },
> + { }
> +};
> +
> +static struct dfl_driver dfl_emif_driver = {
> + .drv = {
> + .name = "dfl-emif",
> + .dev_groups = dfl_emif_groups,
> + },
> + .id_table = dfl_emif_ids,
> + .probe = dfl_emif_probe,
> +};
> +
> +module_dfl_driver(dfl_emif_driver);
> +
> +MODULE_DEVICE_TABLE(dfl, dfl_emif_ids);
This always goes next to the table.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] add the FPGA Device Feature List (DFL) EMIF support
2020-09-08 9:03 ` Krzysztof Kozlowski
@ 2020-09-09 7:45 ` Xu Yilun
0 siblings, 0 replies; 8+ messages in thread
From: Xu Yilun @ 2020-09-09 7:45 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mdf, linux-kernel, trix, matthew.gerlach, russell.h.weight,
lgoncalv, hao.wu, yilun.xu
On Tue, Sep 08, 2020 at 11:03:35AM +0200, Krzysztof Kozlowski wrote:
> On Tue, Sep 08, 2020 at 04:27:24PM +0800, Xu Yilun wrote:
> > This patch depend on the patchsets: "Modularization of DFL private
> > feature drivers" & "add dfl bus support to MODULE_DEVICE_TABLE()"
>
> The need for bus I understand but why it depends on the "Modularization
> of DFL private feature drivers"?
Sorry, maybe the titles of the two Patch 0 make confusion.
The patchset "Modularization of DFL private feature drivers" implements
the dfl bus.
The "add dfl bus support to MODULE_DEVICE_TABLE()" adds the support for
dfl driver module autoloading by changing script/mod. It creates the
dfl-bus.h head file that would be used in this driver.
>
> Anyway I will need a stable tag with mentioned dependencies or this will
> wait for the next cycle.
OK. Maybe I sent it a little earlier. I could wait until the dependencies
are applied.
Thanks,
Yilun
>
> Best regards,
> Krzysztof
>
>
> >
> > https://lore.kernel.org/linux-fpga/1599488581-16386-1-git-send-email-yilun.xu@intel.com/
> >
> > The driver supports the EMIF controller on Intel Programmable
> > Acceleration Card (PAC). The controller manages the on-board memory of
> > the PCIe card.
> >
> > Xu Yilun (1):
> > memory: dfl-emif: add the DFL EMIF private feature driver
> >
> > .../ABI/testing/sysfs-bus-dfl-devices-emif | 25 +++
> > drivers/memory/Kconfig | 9 +
> > drivers/memory/Makefile | 2 +
> > drivers/memory/dfl-emif.c | 211 +++++++++++++++++++++
> > 4 files changed, 247 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > create mode 100644 drivers/memory/dfl-emif.c
> >
> > --
> > 2.7.4
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver
2020-09-08 10:01 ` [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver Krzysztof Kozlowski
@ 2020-09-11 8:42 ` Xu Yilun
2020-09-11 9:03 ` Krzysztof Kozlowski
0 siblings, 1 reply; 8+ messages in thread
From: Xu Yilun @ 2020-09-11 8:42 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: mdf, linux-kernel, trix, matthew.gerlach, russell.h.weight,
lgoncalv, hao.wu, yilun.xu
Sorry I missed one comments, see inline.
On Tue, Sep 08, 2020 at 12:01:02PM +0200, Krzysztof Kozlowski wrote:
> On Tue, Sep 08, 2020 at 04:27:25PM +0800, Xu Yilun wrote:
> > This driver is for the EMIF private feature implemented under FPGA
> > Device Feature List (DFL) framework. It is used to expose memory
> > interface status information as well as memory clearing control.
> >
> > The purpose of memory clearing block is to zero out all private memory
> > when FPGA is to be reprogrammed. This gives users a reliable method to
> > prevent potential data leakage.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > ---
> > .../ABI/testing/sysfs-bus-dfl-devices-emif | 25 +++
> > drivers/memory/Kconfig | 9 +
> > drivers/memory/Makefile | 2 +
> > drivers/memory/dfl-emif.c | 211 +++++++++++++++++++++
> > 4 files changed, 247 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > create mode 100644 drivers/memory/dfl-emif.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > new file mode 100644
> > index 0000000..33d557e
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > @@ -0,0 +1,25 @@
> > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_cal_fail
> > +Date: Sep 2020
> > +KernelVersion: 5.10
> > +Contact: Xu Yilun <yilun.xu@intel.com>
> > +Description: Read-only. It indicates if the calibration is failed on this
> > + memory interface. "1" for calibration failure, "0" for OK.
>
> "if the calibration failed"
>
> > + Format: %u
> > +
> > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_init_done
> > +Date: Sep 2020
> > +KernelVersion: 5.10
> > +Contact: Xu Yilun <yilun.xu@intel.com>
> > +Description: Read-only. It indicates if the initialization is complete on
> > + this memory interface. "1" for initialization complete, "0"
> > + for not yet.
> > + Format: %u
>
> "if the initialization completed"
>
> > +
> > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_clear
> > +Date: Sep 2020
> > +KernelVersion: 5.10
> > +Contact: Xu Yilun <yilun.xu@intel.com>
> > +Description: Write-only. Writing "1" to this file will zero out all memory
> > + data in this memory interface. Writing other values are
> > + invalid.
>
> "Writing of other values is invalid."
>
> > + Format: %u
> > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> > index 8072204..fb0858f 100644
> > --- a/drivers/memory/Kconfig
> > +++ b/drivers/memory/Kconfig
> > @@ -215,6 +215,15 @@ config STM32_FMC2_EBI
> > devices (like SRAM, ethernet adapters, FPGAs, LCD displays, ...) on
> > SOCs containing the FMC2 External Bus Interface.
> >
> > +config FPGA_DFL_EMIF
> > + tristate "DFL EMIF private feature support"
> > + depends on FPGA_DFL && HAS_IOMEM
>
> Cannot be compile tested without FPGA_DFL?
We need this FPGA_DFL dependency. The driver will use the
__dfl_driver_register(), which is defined in drivers/fpga/dfl.c, and
FPGA_DFL enables the compiling of dfl.c
Thanks,
Yilun
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver
2020-09-11 8:42 ` Xu Yilun
@ 2020-09-11 9:03 ` Krzysztof Kozlowski
[not found] ` <20200911144356.GA11294@yilunxu-OptiPlex-7050>
0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-11 9:03 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-kernel, trix, matthew.gerlach, russell.h.weight,
lgoncalv, hao.wu
On Fri, 11 Sep 2020 at 10:46, Xu Yilun <yilun.xu@intel.com> wrote:
>
> Sorry I missed one comments, see inline.
>
> On Tue, Sep 08, 2020 at 12:01:02PM +0200, Krzysztof Kozlowski wrote:
> > On Tue, Sep 08, 2020 at 04:27:25PM +0800, Xu Yilun wrote:
> > > This driver is for the EMIF private feature implemented under FPGA
> > > Device Feature List (DFL) framework. It is used to expose memory
> > > interface status information as well as memory clearing control.
> > >
> > > The purpose of memory clearing block is to zero out all private memory
> > > when FPGA is to be reprogrammed. This gives users a reliable method to
> > > prevent potential data leakage.
> > >
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > > ---
> > > .../ABI/testing/sysfs-bus-dfl-devices-emif | 25 +++
> > > drivers/memory/Kconfig | 9 +
> > > drivers/memory/Makefile | 2 +
> > > drivers/memory/dfl-emif.c | 211 +++++++++++++++++++++
> > > 4 files changed, 247 insertions(+)
> > > create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > > create mode 100644 drivers/memory/dfl-emif.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > > new file mode 100644
> > > index 0000000..33d557e
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > > @@ -0,0 +1,25 @@
> > > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_cal_fail
> > > +Date: Sep 2020
> > > +KernelVersion: 5.10
> > > +Contact: Xu Yilun <yilun.xu@intel.com>
> > > +Description: Read-only. It indicates if the calibration is failed on this
> > > + memory interface. "1" for calibration failure, "0" for OK.
> >
> > "if the calibration failed"
> >
> > > + Format: %u
> > > +
> > > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_init_done
> > > +Date: Sep 2020
> > > +KernelVersion: 5.10
> > > +Contact: Xu Yilun <yilun.xu@intel.com>
> > > +Description: Read-only. It indicates if the initialization is complete on
> > > + this memory interface. "1" for initialization complete, "0"
> > > + for not yet.
> > > + Format: %u
> >
> > "if the initialization completed"
> >
> > > +
> > > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_clear
> > > +Date: Sep 2020
> > > +KernelVersion: 5.10
> > > +Contact: Xu Yilun <yilun.xu@intel.com>
> > > +Description: Write-only. Writing "1" to this file will zero out all memory
> > > + data in this memory interface. Writing other values are
> > > + invalid.
> >
> > "Writing of other values is invalid."
> >
> > > + Format: %u
> > > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> > > index 8072204..fb0858f 100644
> > > --- a/drivers/memory/Kconfig
> > > +++ b/drivers/memory/Kconfig
> > > @@ -215,6 +215,15 @@ config STM32_FMC2_EBI
> > > devices (like SRAM, ethernet adapters, FPGAs, LCD displays, ...) on
> > > SOCs containing the FMC2 External Bus Interface.
> > >
> > > +config FPGA_DFL_EMIF
> > > + tristate "DFL EMIF private feature support"
> > > + depends on FPGA_DFL && HAS_IOMEM
> >
> > Cannot be compile tested without FPGA_DFL?
>
> We need this FPGA_DFL dependency. The driver will use the
> __dfl_driver_register(), which is defined in drivers/fpga/dfl.c, and
> FPGA_DFL enables the compiling of dfl.c
Yeah but the DFL headers provide empty stubs for such case, don't
they? If they don't, probably they should.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver
[not found] ` <20200911144356.GA11294@yilunxu-OptiPlex-7050>
@ 2020-09-11 15:16 ` Krzysztof Kozlowski
0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-11 15:16 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-kernel, trix, matthew.gerlach, russell.h.weight,
lgoncalv, hao.wu
On Fri, 11 Sep 2020 at 16:48, Xu Yilun <yilun.xu@intel.com> wrote:
>
> On Fri, Sep 11, 2020 at 11:03:50AM +0200, Krzysztof Kozlowski wrote:
> > On Fri, 11 Sep 2020 at 10:46, Xu Yilun <yilun.xu@intel.com> wrote:
> > >
> > > Sorry I missed one comments, see inline.
> > >
> > > On Tue, Sep 08, 2020 at 12:01:02PM +0200, Krzysztof Kozlowski wrote:
> > > > On Tue, Sep 08, 2020 at 04:27:25PM +0800, Xu Yilun wrote:
> > > > > This driver is for the EMIF private feature implemented under FPGA
> > > > > Device Feature List (DFL) framework. It is used to expose memory
> > > > > interface status information as well as memory clearing control.
> > > > >
> > > > > The purpose of memory clearing block is to zero out all private memory
> > > > > when FPGA is to be reprogrammed. This gives users a reliable method to
> > > > > prevent potential data leakage.
> > > > >
> > > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > > > > ---
> > > > > .../ABI/testing/sysfs-bus-dfl-devices-emif | 25 +++
> > > > > drivers/memory/Kconfig | 9 +
> > > > > drivers/memory/Makefile | 2 +
> > > > > drivers/memory/dfl-emif.c | 211 +++++++++++++++++++++
> > > > > 4 files changed, 247 insertions(+)
> > > > > create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > > > > create mode 100644 drivers/memory/dfl-emif.c
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > > > > new file mode 100644
> > > > > index 0000000..33d557e
> > > > > --- /dev/null
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > > > > @@ -0,0 +1,25 @@
> > > > > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_cal_fail
> > > > > +Date: Sep 2020
> > > > > +KernelVersion: 5.10
> > > > > +Contact: Xu Yilun <yilun.xu@intel.com>
> > > > > +Description: Read-only. It indicates if the calibration is failed on this
> > > > > + memory interface. "1" for calibration failure, "0" for OK.
> > > >
> > > > "if the calibration failed"
> > > >
> > > > > + Format: %u
> > > > > +
> > > > > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_init_done
> > > > > +Date: Sep 2020
> > > > > +KernelVersion: 5.10
> > > > > +Contact: Xu Yilun <yilun.xu@intel.com>
> > > > > +Description: Read-only. It indicates if the initialization is complete on
> > > > > + this memory interface. "1" for initialization complete, "0"
> > > > > + for not yet.
> > > > > + Format: %u
> > > >
> > > > "if the initialization completed"
> > > >
> > > > > +
> > > > > +What: /sys/bus/dfl/devices/dfl_dev.X/infX_clear
> > > > > +Date: Sep 2020
> > > > > +KernelVersion: 5.10
> > > > > +Contact: Xu Yilun <yilun.xu@intel.com>
> > > > > +Description: Write-only. Writing "1" to this file will zero out all memory
> > > > > + data in this memory interface. Writing other values are
> > > > > + invalid.
> > > >
> > > > "Writing of other values is invalid."
> > > >
> > > > > + Format: %u
> > > > > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> > > > > index 8072204..fb0858f 100644
> > > > > --- a/drivers/memory/Kconfig
> > > > > +++ b/drivers/memory/Kconfig
> > > > > @@ -215,6 +215,15 @@ config STM32_FMC2_EBI
> > > > > devices (like SRAM, ethernet adapters, FPGAs, LCD displays, ...) on
> > > > > SOCs containing the FMC2 External Bus Interface.
> > > > >
> > > > > +config FPGA_DFL_EMIF
> > > > > + tristate "DFL EMIF private feature support"
> > > > > + depends on FPGA_DFL && HAS_IOMEM
> > > >
> > > > Cannot be compile tested without FPGA_DFL?
> > >
> > > We need this FPGA_DFL dependency. The driver will use the
> > > __dfl_driver_register(), which is defined in drivers/fpga/dfl.c, and
> > > FPGA_DFL enables the compiling of dfl.c
> >
> > Yeah but the DFL headers provide empty stubs for such case, don't
> > they? If they don't, probably they should.
>
> The DFL headers don't provide the empty stubs, why they should? A DFL
> driver should not be selected when the DFL framework & DFL bus is not
> enabled in system.
Good point, this is not a consumer API. Then the config entry looks fine.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver
[not found] ` <1599553645-26928-2-git-send-email-yilun.xu@intel.com>
2020-09-08 10:01 ` [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver Krzysztof Kozlowski
@ 2020-09-11 16:13 ` Wu, Hao
1 sibling, 0 replies; 8+ messages in thread
From: Wu, Hao @ 2020-09-11 16:13 UTC (permalink / raw)
To: Xu, Yilun, krzk, mdf, linux-kernel
Cc: trix, matthew.gerlach, Weight, Russell H, lgoncalv
> Subject: [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver
>
> This driver is for the EMIF private feature implemented under FPGA
> Device Feature List (DFL) framework. It is used to expose memory
> interface status information as well as memory clearing control.
>
> The purpose of memory clearing block is to zero out all private memory
> when FPGA is to be reprogrammed. This gives users a reliable method to
> prevent potential data leakage.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> .../ABI/testing/sysfs-bus-dfl-devices-emif | 25 +++
> drivers/memory/Kconfig | 9 +
> drivers/memory/Makefile | 2 +
> drivers/memory/dfl-emif.c | 211 +++++++++++++++++++++
> 4 files changed, 247 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> create mode 100644 drivers/memory/dfl-emif.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> new file mode 100644
> index 0000000..33d557e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> @@ -0,0 +1,25 @@
> +What: /sys/bus/dfl/devices/dfl_dev.X/infX_cal_fail
> +Date: Sep 2020
> +KernelVersion: 5.10
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Read-only. It indicates if the calibration is failed on this
> + memory interface. "1" for calibration failure, "0" for OK.
> + Format: %u
> +
> +What: /sys/bus/dfl/devices/dfl_dev.X/infX_init_done
> +Date: Sep 2020
> +KernelVersion: 5.10
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Read-only. It indicates if the initialization is complete on
> + this memory interface. "1" for initialization complete, "0"
> + for not yet.
> + Format: %u
> +
> +What: /sys/bus/dfl/devices/dfl_dev.X/infX_clear
> +Date: Sep 2020
> +KernelVersion: 5.10
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Write-only. Writing "1" to this file will zero out all memory
> + data in this memory interface. Writing other values are
> + invalid.
> + Format: %u
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 8072204..fb0858f 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -215,6 +215,15 @@ config STM32_FMC2_EBI
> devices (like SRAM, ethernet adapters, FPGAs, LCD displays, ...) on
> SOCs containing the FMC2 External Bus Interface.
>
> +config FPGA_DFL_EMIF
> + tristate "DFL EMIF private feature support"
Yilun,
I see you are submitting several different DFL device driver now, then do you
think if it's better to have some unified style of the naming?
> + depends on FPGA_DFL && HAS_IOMEM
> + help
> + This driver is for the EMIF private feature implemented under
> + FPGA Device Feature List (DFL) framework. It is used to expose
> + memory interface status information as well as memory clearing
> + control.
> +
> source "drivers/memory/samsung/Kconfig"
> source "drivers/memory/tegra/Kconfig"
>
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index e71cf7b..0afbf39 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -39,3 +39,5 @@ $(obj)/ti-emif-asm-offsets.h: $(obj)/emif-asm-offsets.s
> FORCE
>
> targets += emif-asm-offsets.s
> clean-files += ti-emif-asm-offsets.h
> +
> +obj-$(CONFIG_FPGA_DFL_EMIF) += dfl-emif.o
> diff --git a/drivers/memory/dfl-emif.c b/drivers/memory/dfl-emif.c
> new file mode 100644
> index 0000000..442137f
> --- /dev/null
> +++ b/drivers/memory/dfl-emif.c
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for DFL EMIF private feature
> + *
> + * Copyright (C) 2020 Intel Corporation, Inc.
> + *
> + */
> +#include <linux/bitfield.h>
> +#include <linux/errno.h>
> +#include <linux/fpga/dfl-bus.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
> +
> +#define EMIF_STAT 0x8
> +#define EMIF_STAT_INIT_DONE_SFT 0
> +#define EMIF_STAT_CALC_FAIL_SFT 8
> +#define EMIF_STAT_CLEAR_BUSY_SFT 16
> +#define EMIF_CTRL 0x10
> +#define EMIF_CTRL_CLEAR_EN_SFT 0
> +#define EMIF_CTRL_CLEAR_EN_MSK GENMASK_ULL(3, 0)
> +
> +#define EMIF_POLL_INVL 10000 /* us */
> +#define EMIF_POLL_TIMEOUT 5000000 /* us */
> +
> +struct dfl_emif {
> + struct device *dev;
> + void __iomem *base;
> + spinlock_t lock; /* Serialises access to EMIF_CTRL reg */
> +};
> +
> +struct emif_attr {
> + struct device_attribute attr;
> + u32 shift;
> + u32 index;
> +};
> +
> +#define to_emif_attr(dev_attr) \
> + container_of(dev_attr, struct emif_attr, attr)
> +
> +static ssize_t emif_state_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct emif_attr *eattr = to_emif_attr(attr);
> + struct dfl_emif *de = dev_get_drvdata(dev);
> + u64 val;
> +
> + val = readq(de->base + EMIF_STAT);
> +
> + return sprintf(buf, "%u\n",
> + !!(val & BIT_ULL(eattr->shift + eattr->index)));
> +}
> +
> +static ssize_t emif_clear_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct emif_attr *eattr = to_emif_attr(attr);
> + struct dfl_emif *de = dev_get_drvdata(dev);
> + u64 clear_busy_msk, clear_en_msk, val;
> + void __iomem *base = de->base;
> +
> + if (!sysfs_streq(buf, "1"))
> + return -EINVAL;
> +
> + clear_busy_msk = BIT_ULL(EMIF_STAT_CLEAR_BUSY_SFT + eattr-
> >index);
> + clear_en_msk = BIT_ULL(EMIF_CTRL_CLEAR_EN_SFT + eattr->index);
> +
> + spin_lock(&de->lock);
> + /* The CLEAR_EN field is WO, but other fields are RW */
> + val = readq(base + EMIF_CTRL);
> + val &= ~EMIF_CTRL_CLEAR_EN_MSK;
> + val |= clear_en_msk;
> + writeq(val, base + EMIF_CTRL);
> + spin_unlock(&de->lock);
Should this lock protect below status polling? Is it possible another clear
happening right after this spin unlock and cause a different value to
EMIF_CTRL before below status polling?
Thanks
Hao
> +
> + if (readq_poll_timeout(base + EMIF_STAT, val,
> + !(val & clear_busy_msk),
> + EMIF_POLL_INVL, EMIF_POLL_TIMEOUT)) {
> + dev_err(de->dev, "timeout, fail to clear\n");
> + return -ETIMEDOUT;
> + }
> +
> + return count;
> +}
> +
> +#define emif_state_attr(_name, _shift, _index)
> \
> + struct emif_attr emif_attr_##inf##_index##_##_name =
> \
> + { .attr = __ATTR(inf##_index##_##_name, 0444,
> \
> + emif_state_show, NULL), \
> + .shift = (_shift), .index = (_index) }
> +
> +#define emif_clear_attr(_index)
> \
> + struct emif_attr emif_attr_##inf##_index##_clear = \
> + { .attr = __ATTR(inf##_index##_clear, 0200, \
> + NULL, emif_clear_store), \
> + .index = (_index) }
> +
> +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 0);
> +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 1);
> +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 2);
> +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 3);
> +
> +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 0);
> +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 1);
> +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 2);
> +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 3);
> +
> +emif_clear_attr(0);
> +emif_clear_attr(1);
> +emif_clear_attr(2);
> +emif_clear_attr(3);
> +
> +static struct attribute *dfl_emif_attrs[] = {
> + &emif_attr_inf0_init_done.attr.attr,
> + &emif_attr_inf0_cal_fail.attr.attr,
> + &emif_attr_inf0_clear.attr.attr,
> +
> + &emif_attr_inf1_init_done.attr.attr,
> + &emif_attr_inf1_cal_fail.attr.attr,
> + &emif_attr_inf1_clear.attr.attr,
> +
> + &emif_attr_inf2_init_done.attr.attr,
> + &emif_attr_inf2_cal_fail.attr.attr,
> + &emif_attr_inf2_clear.attr.attr,
> +
> + &emif_attr_inf3_init_done.attr.attr,
> + &emif_attr_inf3_cal_fail.attr.attr,
> + &emif_attr_inf3_clear.attr.attr,
> +
> + NULL,
> +};
> +
> +static umode_t dfl_emif_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + struct dfl_emif *de = dev_get_drvdata(kobj_to_dev(kobj));
> + struct emif_attr *eattr = container_of(attr, struct emif_attr,
> + attr.attr);
> + u64 val;
> +
> + /*
> + * This device supports upto 4 memory interfaces, but not all
> + * interfaces are used on different platforms. The read out value of
> + * CLEAN_EN field (which is a bitmap) could tell how many interfaces
> + * are available.
> + */
> + val = FIELD_GET(EMIF_CTRL_CLEAR_EN_MSK, readq(de->base +
> EMIF_CTRL));
> +
> + return (val & BIT_ULL(eattr->index)) ? attr->mode : 0;
> +}
> +
> +static const struct attribute_group dfl_emif_group = {
> + .is_visible = dfl_emif_visible,
> + .attrs = dfl_emif_attrs,
> +};
> +
> +static const struct attribute_group *dfl_emif_groups[] = {
> + &dfl_emif_group,
> + NULL,
> +};
> +
> +static int dfl_emif_probe(struct dfl_device *dfl_dev)
> +{
> + struct device *dev = &dfl_dev->dev;
> + struct dfl_emif *de;
> +
> + de = devm_kzalloc(dev, sizeof(*de), GFP_KERNEL);
> + if (!de)
> + return -ENOMEM;
> +
> + de->base = devm_ioremap_resource(&dfl_dev->dev, &dfl_dev-
> >mmio_res);
> + if (IS_ERR(de->base)) {
> + dev_err(dev, "get mem resource fail!\n");
> + return PTR_ERR(de->base);
> + }
> +
> + de->dev = dev;
> + spin_lock_init(&de->lock);
> + dev_set_drvdata(dev, de);
> +
> + return 0;
> +}
> +
> +#define FME_FEATURE_ID_EMIF 0x9
> +
> +static const struct dfl_device_id dfl_emif_ids[] = {
> + { FME_ID, FME_FEATURE_ID_EMIF },
> + { }
> +};
> +
> +static struct dfl_driver dfl_emif_driver = {
> + .drv = {
> + .name = "dfl-emif",
> + .dev_groups = dfl_emif_groups,
> + },
> + .id_table = dfl_emif_ids,
> + .probe = dfl_emif_probe,
> +};
> +
> +module_dfl_driver(dfl_emif_driver);
> +
> +MODULE_DEVICE_TABLE(dfl, dfl_emif_ids);
> +MODULE_DESCRIPTION("DFL EMIF driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-11 16:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 8:27 [PATCH] add the FPGA Device Feature List (DFL) EMIF support Xu Yilun
2020-09-08 9:03 ` Krzysztof Kozlowski
2020-09-09 7:45 ` Xu Yilun
[not found] ` <1599553645-26928-2-git-send-email-yilun.xu@intel.com>
2020-09-08 10:01 ` [PATCH] memory: dfl-emif: add the DFL EMIF private feature driver Krzysztof Kozlowski
2020-09-11 8:42 ` Xu Yilun
2020-09-11 9:03 ` Krzysztof Kozlowski
[not found] ` <20200911144356.GA11294@yilunxu-OptiPlex-7050>
2020-09-11 15:16 ` Krzysztof Kozlowski
2020-09-11 16:13 ` Wu, Hao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).