From: Matthew Gerlach <[email protected]>
This patchset enhances the definition of the Device Feature Header (DFH) used by
the Device Feature List (DFL) bus and then uses the new enhancements in a uart
driver.
Patch 1 updates the DFL documentation to provide the motivation behind the
enhancements to the definition of the DFH.
Patch 2 moves some of the DFH definitions to include/linux/dfl.h so that
they can be accessed by drivers outside of drivers/fpga.
Patch 3 adds the definitions for DFHv1.
Patch 4 defines and uses a DFHv1 parameter to provide a generic mechanism for
describing MSIX interrupts used by a particular feature instance.
Patch 5 gets the location and size of the feature's register set from DFHv1.
Patch 6 adds a DFL uart driver that makes use of the new features of DFHv1.
Basheer Ahmed Muddebihal (2):
fpga: dfl: Move the DFH definitions
fpga: dfl: Add DFHv1 Register Definitions
Matthew Gerlach (4):
Documentation: fpga: dfl: Add documentation for DFHv1
fpga: dfl: add generic support for MSIX interrupts
fpga: dfl: parse the location of the feature's registers from DFHv1
tty: serial: 8250: add DFL bus driver for Altera 16550.
Documentation/fpga/dfl.rst | 49 ++++++++
drivers/fpga/dfl-afu-main.c | 4 +-
drivers/fpga/dfl.c | 88 ++++++++++++--
drivers/fpga/dfl.h | 24 +---
drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 9 ++
drivers/tty/serial/8250/Makefile | 1 +
include/linux/dfl.h | 79 ++++++++++++-
8 files changed, 402 insertions(+), 29 deletions(-)
create mode 100644 drivers/tty/serial/8250/8250_dfl.c
--
2.25.1
From: Basheer Ahmed Muddebihal <[email protected]>
Moving the DFH register offset and register definitions from
drivers/fpga/dfl.h to include/linux/dfl.h. These definitions
need to be accessed by dfl drivers that are outside of
drivers/fpga.
Signed-off-by: Basheer Ahmed Muddebihal <[email protected]>
Signed-off-by: Matthew Gerlach <[email protected]>
---
v2: remove extra space in commit
use uniform number of digits in constants
don't change copyright date because of removed content
---
drivers/fpga/dfl-afu-main.c | 4 ++--
drivers/fpga/dfl.c | 2 +-
drivers/fpga/dfl.h | 20 +-------------------
include/linux/dfl.h | 33 ++++++++++++++++++++++++++++++++-
4 files changed, 36 insertions(+), 23 deletions(-)
diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index 7f621e96d3b8..c26961ee33db 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -468,8 +468,8 @@ afu_id_show(struct device *dev, struct device_attribute *attr, char *buf)
return -EBUSY;
}
- guidl = readq(base + GUID_L);
- guidh = readq(base + GUID_H);
+ guidl = readq(base + DFH_GUID_L);
+ guidh = readq(base + DFH_GUID_H);
mutex_unlock(&pdata->lock);
return scnprintf(buf, PAGE_SIZE, "%016llx%016llx\n", guidh, guidl);
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..1132f3c10440 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -1163,7 +1163,7 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
* find and parse FIU's child AFU via its NEXT_AFU register.
* please note that only Port has valid NEXT_AFU pointer per spec.
*/
- v = readq(binfo->ioaddr + NEXT_AFU);
+ v = readq(binfo->ioaddr + DFH_NEXT_AFU);
offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
if (offset)
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 06cfcd5e84bb..e620fcb02b5a 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -17,6 +17,7 @@
#include <linux/bitfield.h>
#include <linux/cdev.h>
#include <linux/delay.h>
+#include <linux/dfl.h>
#include <linux/eventfd.h>
#include <linux/fs.h>
#include <linux/interrupt.h>
@@ -53,28 +54,9 @@
#define PORT_FEATURE_ID_UINT 0x12
#define PORT_FEATURE_ID_STP 0x13
-/*
- * Device Feature Header Register Set
- *
- * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
- * For AFUs, they have DFH + GUID as common header registers.
- * For private features, they only have DFH register as common header.
- */
-#define DFH 0x0
-#define GUID_L 0x8
-#define GUID_H 0x10
-#define NEXT_AFU 0x18
-
-#define DFH_SIZE 0x8
-
/* Device Feature Header Register Bitfield */
-#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID */
#define DFH_ID_FIU_FME 0
#define DFH_ID_FIU_PORT 1
-#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
-#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
-#define DFH_EOL BIT_ULL(40) /* End of list */
-#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
#define DFH_TYPE_AFU 1
#define DFH_TYPE_PRIVATE 3
#define DFH_TYPE_FIU 4
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 431636a0dc78..33d167c53b09 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -2,7 +2,7 @@
/*
* Header file for DFL driver and device API
*
- * Copyright (C) 2020 Intel Corporation, Inc.
+ * Copyright (C) 2020-2022 Intel Corporation, Inc.
*/
#ifndef __LINUX_DFL_H
@@ -11,6 +11,37 @@
#include <linux/device.h>
#include <linux/mod_devicetable.h>
+/*
+ * Device Feature Header Register Set
+ *
+ * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
+ * For AFUs, they have DFH + GUID as common header registers.
+ * For private features, they only have DFH register as common header.
+ */
+#define DFH 0x00
+#define DFH_GUID_L 0x08
+#define DFH_GUID_H 0x10
+#define DFH_NEXT_AFU 0x18
+
+/*
+ * DFHv1 Register Offset definitons
+ * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
+ * as common header registers
+ */
+#define DFHv1_CSR_ADDR 0x18 /* CSR Register start address */
+#define DFHv1_CSR_SIZE_GRP 0x20 /* Size of Reg Block and Group/tag */
+#define DFHv1_PARAM_HDR 0x28 /* Optional First Param header */
+#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */
+
+#define DFH_SIZE 0x08
+
+/* Device Feature Header Register Bitfield */
+#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID */
+#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
+#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
+#define DFH_EOL BIT_ULL(40) /* End of list */
+#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
+
/**
* enum dfl_id_type - define the DFL FIU types
*/
--
2.25.1
From: Matthew Gerlach <[email protected]>
Add a Device Feature List (DFL) bus driver for the Altera
16550 implementation of UART.
Signed-off-by: Matthew Gerlach <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
v2: clean up error messages
alphabetize header files
fix 'missing prototype' error by making function static
tried to sort Makefile and Kconfig better
---
drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 9 ++
drivers/tty/serial/8250/Makefile | 1 +
include/linux/dfl.h | 7 ++
4 files changed, 194 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_dfl.c
diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
new file mode 100644
index 000000000000..539ca6138eda
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA UART
+ *
+ * Copyright (C) 2022 Intel Corporation, Inc.
+ *
+ * Authors:
+ * Ananda Ravuri <[email protected]>
+ * Matthew Gerlach <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/dfl.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+
+struct dfl_uart {
+ void __iomem *csr_base;
+ struct device *dev;
+ u64 uart_clk;
+ u64 fifo_len;
+ unsigned int fifo_size;
+ unsigned int reg_shift;
+ unsigned int line;
+};
+
+static int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
+{
+ void __iomem *param_base;
+ int off;
+ u64 v;
+
+ v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
+
+ if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
+ dev_err(dfluart->dev, "missing required DFH parameters\n");
+ return -EINVAL;
+ }
+
+ param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
+
+ off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
+ if (off < 0) {
+ dev_err(dfluart->dev, "missing CLK_FRQ param\n");
+ return -EINVAL;
+ }
+
+ dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
+ dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
+
+ off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
+ if (off < 0) {
+ dev_err(dfluart->dev, "missing FIFO_LEN param\n");
+ return -EINVAL;
+ }
+
+ dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
+ dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
+
+ off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
+ if (off < 0) {
+ dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
+ return -EINVAL;
+ }
+
+ v = readq(param_base + off + DFHv1_PARAM_DATA);
+ dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
+ dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
+ dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
+ dfluart->fifo_size, dfluart->reg_shift);
+
+ return 0;
+}
+
+static int dfl_uart_probe(struct dfl_device *dfl_dev)
+{
+ struct device *dev = &dfl_dev->dev;
+ struct uart_8250_port uart;
+ struct dfl_uart *dfluart;
+ int ret;
+
+ memset(&uart, 0, sizeof(uart));
+
+ dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+ if (!dfluart)
+ return -ENOMEM;
+
+ dfluart->dev = dev;
+
+ dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
+ if (IS_ERR(dfluart->csr_base)) {
+ return PTR_ERR(dfluart->csr_base);
+ }
+
+ ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res));
+
+ devm_iounmap(dev, dfluart->csr_base);
+ devm_release_mem_region(dev, dfl_dev->mmio_res.start, resource_size(&dfl_dev->mmio_res));
+
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed uart feature walk\n");
+
+ dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
+
+ if (dfl_dev->num_irqs == 1)
+ uart.port.irq = dfl_dev->irqs[0];
+
+ switch (dfluart->fifo_len) {
+ case 32:
+ uart.port.type = PORT_ALTR_16550_F32;
+ break;
+
+ case 64:
+ uart.port.type = PORT_ALTR_16550_F64;
+ break;
+
+ case 128:
+ uart.port.type = PORT_ALTR_16550_F128;
+ break;
+
+ default:
+ dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len);
+ return -EINVAL;
+ }
+
+ uart.port.iotype = UPIO_MEM32;
+ uart.port.mapbase = dfl_dev->csr_start;
+ uart.port.mapsize = dfl_dev->csr_size;
+ uart.port.regshift = dfluart->reg_shift;
+ uart.port.uartclk = dfluart->uart_clk;
+ uart.port.flags |= UPF_IOREMAP;
+
+ /* register the port */
+ ret = serial8250_register_8250_port(&uart);
+ if (ret < 0) {
+ dev_err(dev, "unable to register 8250 port %d.\n", ret);
+ return -EINVAL;
+ }
+ dev_info(dev, "serial8250_register_8250_port %d\n", ret);
+ dfluart->line = ret;
+ dev_set_drvdata(dev, dfluart);
+
+ return 0;
+}
+
+static void dfl_uart_remove(struct dfl_device *dfl_dev)
+{
+ struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
+
+ if (dfluart->line > 0)
+ serial8250_unregister_port(dfluart->line);
+}
+
+#define FME_FEATURE_ID_UART 0x24
+
+static const struct dfl_device_id dfl_uart_ids[] = {
+ { FME_ID, FME_FEATURE_ID_UART },
+ { }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
+
+static struct dfl_driver dfl_uart_driver = {
+ .drv = {
+ .name = "dfl-uart",
+ },
+ .id_table = dfl_uart_ids,
+ .probe = dfl_uart_probe,
+ .remove = dfl_uart_remove,
+};
+module_dfl_driver(dfl_uart_driver);
+
+MODULE_DESCRIPTION("DFL Intel UART driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index d0b49e15fbf5..5c6497ce5c12 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -361,6 +361,15 @@ config SERIAL_8250_BCM2835AUX
If unsure, say N.
+config SERIAL_8250_DFL
+ tristate "DFL bus driver for Altera 16550 UART"
+ depends on SERIAL_8250 && FPGA_DFL
+ help
+ This option enables support for a Device Feature List (DFL) bus
+ driver for the Altera 16650 UART. One or more Altera 16650 UARTs
+ can be instantiated in a FPGA and then be discovered during
+ enumeration of the DFL bus.
+
config SERIAL_8250_FSL
bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
depends on SERIAL_8250_CONSOLE
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..32006e0982d1 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE) += 8250_early.o
obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
+obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554) += 8250_exar_st16c554.o
obj-$(CONFIG_SERIAL_8250_HUB6) += 8250_hub6.o
obj-$(CONFIG_SERIAL_8250_FSL) += 8250_fsl.o
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 7d74ef8d1d20..a17aeccc501e 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -67,6 +67,13 @@
#define DFHv1_PARAM_MSIX_STARTV 0x8
#define DFHv1_PARAM_MSIX_NUMV 0xc
+#define DFHv1_PARAM_ID_CLK_FRQ 0x2
+#define DFHv1_PARAM_ID_FIFO_LEN 0x3
+
+#define DFHv1_PARAM_ID_REG_LAYOUT 0x4
+#define DFHv1_PARAM_ID_REG_WIDTH GENMASK_ULL(63, 32)
+#define DFHv1_PARAM_ID_REG_SHIFT GENMASK_ULL(31, 0)
+
/**
* enum dfl_id_type - define the DFL FIU types
*/
--
2.25.1
From: Matthew Gerlach <[email protected]>
Define and use a DFHv1 parameter to add generic support for MSIX
interrupts for DFL devices.
Signed-off-by: Matthew Gerlach <[email protected]>
---
v2: fix kernel doc
clarify use of DFH_VERSION field
---
drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
include/linux/dfl.h | 14 +++++++++++
2 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 1132f3c10440..dfd3f563c92d 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
void __iomem *base = binfo->ioaddr + ofst;
unsigned int i, ibase, inr = 0;
enum dfl_id_type type;
- int virq;
+ int virq, off;
u64 v;
type = feature_dev_id_type(binfo->feature_dev);
/*
* Ideally DFL framework should only read info from DFL header, but
- * current version DFL only provides mmio resources information for
+ * current version, DFHv0, only provides mmio resources information for
* each feature in DFL Header, no field for interrupt resources.
* Interrupt resource information is provided by specific mmio
* registers of each private feature which supports interrupt. So in
* order to parse and assign irq resources, DFL framework has to look
* into specific capability registers of these private features.
*
- * Once future DFL version supports generic interrupt resource
- * information in common DFL headers, the generic interrupt parsing
- * code will be added. But in order to be compatible to old version
+ * DFHv1 supports generic interrupt resource information in DFHv1
+ * parameter blocks. But in order to be compatible to old version
* DFL, the driver may still fall back to these quirks.
*/
if (type == PORT_ID) {
@@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
}
}
+ if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
+ fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
+
+ v = FIELD_GET(DFH_VERSION, readq(base));
+ switch (v) {
+ case 0:
+ break;
+
+ case 1:
+ v = readq(base + DFHv1_CSR_SIZE_GRP);
+ if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
+ off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
+ DFHv1_PARAM_ID_MSIX);
+ if (off >= 0) {
+ ibase = readl(base + DFHv1_PARAM_HDR +
+ off + DFHv1_PARAM_MSIX_STARTV);
+ inr = readl(base + DFHv1_PARAM_HDR +
+ off + DFHv1_PARAM_MSIX_NUMV);
+ dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
+ ibase, inr, fid);
+ }
+ }
+ break;
+
+ default:
+ dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
+ break;
+ }
+ }
+
if (!inr) {
*irq_base = 0;
*nr_irqs = 0;
@@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
}
EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
+int dfl_find_param(void __iomem *base, resource_size_t max, int param)
+{
+ int off = 0;
+ u64 v, next;
+
+ while (off < max) {
+ v = readq(base + off);
+ if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+ return off;
+
+ next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+ if (!next)
+ break;
+
+ off += next;
+ }
+
+ return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(dfl_find_param);
+
static void __exit dfl_fpga_exit(void)
{
dfl_chardev_uinit();
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 1e53468ba8d8..33e21c360671 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -63,6 +63,10 @@
#define DFHv1_PARAM_HDR_VERSION GENMASK_ULL(31, 16) /* Version Param */
#define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 32) /* Offset of next Param */
+#define DFHv1_PARAM_ID_MSIX 0x1
+#define DFHv1_PARAM_MSIX_STARTV 0x8
+#define DFHv1_PARAM_MSIX_NUMV 0xc
+
/**
* enum dfl_id_type - define the DFL FIU types
*/
@@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
module_driver(__dfl_driver, dfl_driver_register, \
dfl_driver_unregister)
+/**
+ * dfl_find_param() - find the offset of the given parameter
+ * @base: base pointer to start of dfl parameters in DFH
+ * @max: maximum offset to search
+ * @param: id of dfl parameter
+ *
+ * Return: positive offset on success, negative error code otherwise.
+ */
+int dfl_find_param(void __iomem *base, resource_size_t max, int param);
+
#endif /* __LINUX_DFL_H */
--
2.25.1
From: Matthew Gerlach <[email protected]>
The location of a feature's registers is explicitly
described in DFHv1 and can be relative to the base of the DFHv1
or an absolute address. Parse the location and pass the information
to DFL driver.
Signed-off-by: Matthew Gerlach <[email protected]>
---
v2: Introduced in v2.
---
drivers/fpga/dfl.c | 26 +++++++++++++++++++++++++-
drivers/fpga/dfl.h | 4 ++++
include/linux/dfl.h | 4 ++++
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index dfd3f563c92d..6fb4f30f93cf 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -381,6 +381,8 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
ddev->feature_id = feature->id;
ddev->revision = feature->revision;
ddev->cdev = pdata->dfl_cdev;
+ ddev->csr_start = feature->csr_start;
+ ddev->csr_size = feature->csr_size;
/* add mmio resource */
parent_res = &pdev->resource[feature->resource_index];
@@ -708,18 +710,25 @@ struct build_feature_devs_info {
* struct dfl_feature_info - sub feature info collected during feature dev build
*
* @fid: id of this sub feature.
+ * @revision: revision of this sub feature
+ * @dfh_version: version of Device Feature Header (DFH)
* @mmio_res: mmio resource of this sub feature.
* @ioaddr: mapped base address of mmio resource.
* @node: node in sub_features linked list.
+ * @csr_start: DFHv1 start of feature registers
+ * @csr_size: DFHv1 size of feature registers
* @irq_base: start of irq index in this sub feature.
* @nr_irqs: number of irqs of this sub feature.
*/
struct dfl_feature_info {
u16 fid;
u8 revision;
+ u8 dfh_version;
struct resource mmio_res;
void __iomem *ioaddr;
struct list_head node;
+ resource_size_t csr_start;
+ resource_size_t csr_size;
unsigned int irq_base;
unsigned int nr_irqs;
};
@@ -797,6 +806,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
feature->dev = fdev;
feature->id = finfo->fid;
feature->revision = finfo->revision;
+ feature->csr_start = finfo->csr_start;
+ feature->csr_size = finfo->csr_size;
/*
* the FIU header feature has some fundamental functions (sriov
@@ -1054,6 +1065,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
{
unsigned int irq_base, nr_irqs;
struct dfl_feature_info *finfo;
+ u8 dfh_version = 0;
u8 revision = 0;
int ret;
u64 v;
@@ -1061,7 +1073,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
if (fid != FEATURE_ID_AFU) {
v = readq(binfo->ioaddr + ofst);
revision = FIELD_GET(DFH_REVISION, v);
-
+ dfh_version = FIELD_GET(DFH_VERSION, v);
/* read feature size and id if inputs are invalid */
size = size ? size : feature_size(v);
fid = fid ? fid : feature_id(v);
@@ -1080,12 +1092,24 @@ create_feature_instance(struct build_feature_devs_info *binfo,
finfo->fid = fid;
finfo->revision = revision;
+ finfo->dfh_version = dfh_version;
finfo->mmio_res.start = binfo->start + ofst;
finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
finfo->mmio_res.flags = IORESOURCE_MEM;
finfo->irq_base = irq_base;
finfo->nr_irqs = nr_irqs;
+ if (dfh_version == 1) {
+ v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+ if (v & DFHv1_CSR_ADDR_REL)
+ finfo->csr_start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+ else
+ finfo->csr_start = binfo->start + ofst + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+
+ v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+ finfo->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
+ }
+
list_add_tail(&finfo->node, &binfo->sub_features);
binfo->feature_num++;
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index e620fcb02b5a..64cedd00dca4 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -217,6 +217,8 @@ struct dfl_feature_irq_ctx {
* this index is used to find its mmio resource from the
* feature dev (platform device)'s resources.
* @ioaddr: mapped mmio resource address.
+ * @csr_start: DFHv1 start of feature registers
+ * @csr_size: DFHv1 size of feature registers
* @irq_ctx: interrupt context list.
* @nr_irqs: number of interrupt contexts.
* @ops: ops of this sub feature.
@@ -229,6 +231,8 @@ struct dfl_feature {
u8 revision;
int resource_index;
void __iomem *ioaddr;
+ resource_size_t csr_start;
+ resource_size_t csr_size;
struct dfl_feature_irq_ctx *irq_ctx;
unsigned int nr_irqs;
const struct dfl_feature_ops *ops;
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 33e21c360671..7d74ef8d1d20 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -84,6 +84,8 @@ enum dfl_id_type {
* @type: type of DFL FIU of the device. See enum dfl_id_type.
* @feature_id: feature identifier local to its DFL FIU type.
* @mmio_res: mmio resource of this dfl device.
+ * @csr_start: DFHv1 start of feature registers
+ * @csr_size: DFHv1 size of feature registers
* @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.
@@ -96,6 +98,8 @@ struct dfl_device {
u16 feature_id;
u8 revision;
struct resource mmio_res;
+ resource_size_t csr_start;
+ resource_size_t csr_size;
int *irqs;
unsigned int num_irqs;
struct dfl_fpga_cdev *cdev;
--
2.25.1
From: Matthew Gerlach <[email protected]>
Add documentation describing the extensions provided by Version
1 of the Device Feature Header (DFHv1).
Signed-off-by: Matthew Gerlach <[email protected]>
---
v2: s/GUILD/GUID/
add picture
---
Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 15b670926084..7c786b75b498 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
driver's id_table.
+Extending the Device Feature Header - DFHv1
+===========================================
+The current 8 bytes of the Device Feature Header, hereafter referred to as
+to DFHv0, provide very little opportunity for the hardware to describe itself
+to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
+to provide increased flexibility and extensibility to hardware designs using
+Device Feature Lists. The list below describes some of the goals behind the
+changes in DFHv1:
+
+* Provide a standardized mechanism for features to describe
+ parameters/capabilities to software.
+* Standardize the use of a GUID for all DFHv1 types.
+* Decouple the location of the DFH from the register space of the feature itself.
+
+Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
+a list of parameter values to a particular feature.
+
+With DFHv0, not all features types contained a GUID. DFHv1 makes the GUID standard
+across all types.
+
+With DFHv0, the register map of a given feature is located immediately following
+the DFHv0 in the memory space. With DFHv1, the location of the feature register
+map can be specified as an offset to the DFHv1 or as an absolute address. The DFHv1
+structure is shown below:
+
+ +-----------------------------------------------------------------------+
+ |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
+ +-----------------------------------------------------------------------+
+ |63 GUID_L 0|
+ +-----------------------------------------------------------------------+
+ |63 GUID_H 0|
+ +-----------------------------------------------------------------------+
+ |63 Address/Offset 1| Rel 0|
+ +-----------------------------------------------------------------------+
+ |63 Size of register set 32|Params 31|30 Group 16|15 Instance 0|
+ +-----------------------------------------------------------------------+
+ |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
+ +-----------------------------------------------------------------------+
+ |63 Parameter Data 0|
+ +-----------------------------------------------------------------------+
+
+ ...
+
+ +-----------------------------------------------------------------------+
+ |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
+ +-----------------------------------------------------------------------+
+ |63 Parameter Data 0|
+ +-----------------------------------------------------------------------+
+
Open discussion
===============
FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
--
2.25.1
From: Basheer Ahmed Muddebihal <[email protected]>
This patch adds the definitions for DFHv1 header and related register
bitfields.
Signed-off-by: Basheer Ahmed Muddebihal <[email protected]>
Signed-off-by: Matthew Gerlach <[email protected]>
---
v2: clean up whitespace and one line comments
---
include/linux/dfl.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 33d167c53b09..1e53468ba8d8 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -40,8 +40,29 @@
#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
#define DFH_EOL BIT_ULL(40) /* End of list */
+#define DFH_VERSION GENMASK_ULL(59, 52) /* DFH version */
#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
+/*
+ * CSR Rel Bit, 1'b0 = relative (offset from feature DFH start),
+ * 1'b1 = absolute (ARM or other non-PCIe use)
+ */
+#define DFHv1_CSR_ADDR_REL BIT_ULL(0)
+
+/* CSR Header Register Bit Definitions */
+#define DFHv1_CSR_ADDR_MASK GENMASK_ULL(63, 1) /* 63:1 of CSR address */
+
+/* CSR SIZE Goup Register Bit Definitions */
+#define DFHv1_CSR_SIZE_GRP_INSTANCE_ID GENMASK_ULL(15, 0) /* Enumeration instantiated IP */
+#define DFHv1_CSR_SIZE_GRP_GROUPING_ID GENMASK_ULL(30, 16) /* Group Features/interfaces */
+#define DFHv1_CSR_SIZE_GRP_HAS_PARAMS BIT_ULL(31) /* Presence of Parameters */
+#define DFHv1_CSR_SIZE_GRP_SIZE GENMASK_ULL(63, 32) /* Size of CSR Block in bytes */
+
+/* PARAM Header Register Bit Definitions */
+#define DFHv1_PARAM_HDR_ID GENMASK_ULL(15, 0) /* Id of this Param */
+#define DFHv1_PARAM_HDR_VERSION GENMASK_ULL(31, 16) /* Version Param */
+#define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 32) /* Offset of next Param */
+
/**
* enum dfl_id_type - define the DFL FIU types
*/
--
2.25.1
On Fri, 23 Sep 2022, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> ---
> v2: fix kernel doc
> clarify use of DFH_VERSION field
> ---
> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
> include/linux/dfl.h | 14 +++++++++++
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 1132f3c10440..dfd3f563c92d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> void __iomem *base = binfo->ioaddr + ofst;
> unsigned int i, ibase, inr = 0;
> enum dfl_id_type type;
> - int virq;
> + int virq, off;
> u64 v;
>
> type = feature_dev_id_type(binfo->feature_dev);
>
> /*
> * Ideally DFL framework should only read info from DFL header, but
> - * current version DFL only provides mmio resources information for
> + * current version, DFHv0, only provides mmio resources information for
> * each feature in DFL Header, no field for interrupt resources.
> * Interrupt resource information is provided by specific mmio
> * registers of each private feature which supports interrupt. So in
> * order to parse and assign irq resources, DFL framework has to look
> * into specific capability registers of these private features.
> *
> - * Once future DFL version supports generic interrupt resource
> - * information in common DFL headers, the generic interrupt parsing
> - * code will be added. But in order to be compatible to old version
> + * DFHv1 supports generic interrupt resource information in DFHv1
> + * parameter blocks. But in order to be compatible to old version
> * DFL, the driver may still fall back to these quirks.
> */
> if (type == PORT_ID) {
> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> }
> }
>
> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +
> + v = FIELD_GET(DFH_VERSION, readq(base));
I'd call this variable version (or ver) if you want to store it but it
would also fit to switch () line so that no extra variable is needed.
> + switch (v) {
> + case 0:
> + break;
> +
> + case 1:
> + v = readq(base + DFHv1_CSR_SIZE_GRP);
Extra space.
> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> + DFHv1_PARAM_ID_MSIX);
> + if (off >= 0) {
I'd reverse these 2 conditions and break when there's nothing to do.
> + ibase = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_STARTV);
> + inr = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_NUMV);
> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> + ibase, inr, fid);
> + }
> + }
> + break;
> +
> + default:
> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> + break;
> + }
> + }
> +
> if (!inr) {
> *irq_base = 0;
> *nr_irqs = 0;
--
i.
On Fri, 23 Sep 2022, Ilpo J?rvinen wrote:
> On Fri, 23 Sep 2022, [email protected] wrote:
>
> > From: Matthew Gerlach <[email protected]>
> >
> > Add documentation describing the extensions provided by Version
> > 1 of the Device Feature Header (DFHv1).
> >
> > Signed-off-by: Matthew Gerlach <[email protected]>
> > ---
> > v2: s/GUILD/GUID/
> > add picture
> > ---
> > Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 49 insertions(+)
> >
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index 15b670926084..7c786b75b498 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
> > driver's id_table.
> >
> >
> > +Extending the Device Feature Header - DFHv1
> > +===========================================
> > +The current 8 bytes of the Device Feature Header, hereafter referred to as
> > +to DFHv0, provide very little opportunity for the hardware to describe itself
> > +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
> > +to provide increased flexibility and extensibility to hardware designs using
> > +Device Feature Lists. The list below describes some of the goals behind the
> > +changes in DFHv1:
> > +
> > +* Provide a standardized mechanism for features to describe
> > + parameters/capabilities to software.
> > +* Standardize the use of a GUID for all DFHv1 types.
> > +* Decouple the location of the DFH from the register space of the feature itself.
> > +
> > +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
> > +a list of parameter values to a particular feature.
> > +
> > +With DFHv0, not all features types contained a GUID. DFHv1 makes the GUID standard
> > +across all types.
> > +
> > +With DFHv0, the register map of a given feature is located immediately following
> > +the DFHv0 in the memory space. With DFHv1, the location of the feature register
> > +map can be specified as an offset to the DFHv1 or as an absolute address. The DFHv1
> > +structure is shown below:
> > +
> > + +-----------------------------------------------------------------------+
> > + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
> > + +-----------------------------------------------------------------------+
> > + |63 GUID_L 0|
> > + +-----------------------------------------------------------------------+
> > + |63 GUID_H 0|
> > + +-----------------------------------------------------------------------+
> > + |63 Address/Offset 1| Rel 0|
> > + +-----------------------------------------------------------------------+
>
> Is something missing here given the layout is claimed (in 2/6) to be:
>
> "DFHv1 Register Offset definitons
> In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA"
>
> ?
Ah, I think I've figured it out, PARAM_HDR + PARAM_DATA combo is repeated
n times (rather than the params being covered by the "PARAM_DATA")?
--
i.
> > + |63 Size of register set 32|Params 31|30 Group 16|15 Instance 0|
> > + +-----------------------------------------------------------------------+
> > + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
> > + +-----------------------------------------------------------------------+
> > + |63 Parameter Data 0|
> > + +-----------------------------------------------------------------------+
> > +
> > + ...
> > +
> > + +-----------------------------------------------------------------------+
> > + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
> > + +-----------------------------------------------------------------------+
> > + |63 Parameter Data 0|
> > + +-----------------------------------------------------------------------+
> > +
> > Open discussion
> > ===============
> > FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> >
>
>
On Fri, 23 Sep 2022, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add documentation describing the extensions provided by Version
> 1 of the Device Feature Header (DFHv1).
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> ---
> v2: s/GUILD/GUID/
> add picture
> ---
> Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 15b670926084..7c786b75b498 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
> driver's id_table.
>
>
> +Extending the Device Feature Header - DFHv1
> +===========================================
> +The current 8 bytes of the Device Feature Header, hereafter referred to as
> +to DFHv0, provide very little opportunity for the hardware to describe itself
> +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
> +to provide increased flexibility and extensibility to hardware designs using
> +Device Feature Lists. The list below describes some of the goals behind the
> +changes in DFHv1:
> +
> +* Provide a standardized mechanism for features to describe
> + parameters/capabilities to software.
> +* Standardize the use of a GUID for all DFHv1 types.
> +* Decouple the location of the DFH from the register space of the feature itself.
> +
> +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
> +a list of parameter values to a particular feature.
> +
> +With DFHv0, not all features types contained a GUID. DFHv1 makes the GUID standard
> +across all types.
> +
> +With DFHv0, the register map of a given feature is located immediately following
> +the DFHv0 in the memory space. With DFHv1, the location of the feature register
> +map can be specified as an offset to the DFHv1 or as an absolute address. The DFHv1
> +structure is shown below:
> +
> + +-----------------------------------------------------------------------+
> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
> + +-----------------------------------------------------------------------+
> + |63 GUID_L 0|
> + +-----------------------------------------------------------------------+
> + |63 GUID_H 0|
> + +-----------------------------------------------------------------------+
> + |63 Address/Offset 1| Rel 0|
> + +-----------------------------------------------------------------------+
Is something missing here given the layout is claimed (in 2/6) to be:
"DFHv1 Register Offset definitons
In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA"
?
> + |63 Size of register set 32|Params 31|30 Group 16|15 Instance 0|
> + +-----------------------------------------------------------------------+
> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
> + +-----------------------------------------------------------------------+
> + |63 Parameter Data 0|
> + +-----------------------------------------------------------------------+
> +
> + ...
> +
> + +-----------------------------------------------------------------------+
> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
> + +-----------------------------------------------------------------------+
> + |63 Parameter Data 0|
> + +-----------------------------------------------------------------------+
> +
> Open discussion
> ===============
> FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
>
--
i.
On Fri, 23 Sep 2022, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> The location of a feature's registers is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address. Parse the location and pass the information
> to DFL driver.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> ---
> v2: Introduced in v2.
> ---
> drivers/fpga/dfl.c | 26 +++++++++++++++++++++++++-
> drivers/fpga/dfl.h | 4 ++++
> include/linux/dfl.h | 4 ++++
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index dfd3f563c92d..6fb4f30f93cf 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -381,6 +381,8 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
> ddev->feature_id = feature->id;
> ddev->revision = feature->revision;
> ddev->cdev = pdata->dfl_cdev;
> + ddev->csr_start = feature->csr_start;
> + ddev->csr_size = feature->csr_size;
>
> /* add mmio resource */
> parent_res = &pdev->resource[feature->resource_index];
> @@ -708,18 +710,25 @@ struct build_feature_devs_info {
> * struct dfl_feature_info - sub feature info collected during feature dev build
> *
> * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
> * @mmio_res: mmio resource of this sub feature.
> * @ioaddr: mapped base address of mmio resource.
> * @node: node in sub_features linked list.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
> * @irq_base: start of irq index in this sub feature.
> * @nr_irqs: number of irqs of this sub feature.
> */
> struct dfl_feature_info {
> u16 fid;
> u8 revision;
> + u8 dfh_version;
> struct resource mmio_res;
> void __iomem *ioaddr;
> struct list_head node;
> + resource_size_t csr_start;
> + resource_size_t csr_size;
> unsigned int irq_base;
> unsigned int nr_irqs;
> };
> @@ -797,6 +806,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> feature->dev = fdev;
> feature->id = finfo->fid;
> feature->revision = finfo->revision;
> + feature->csr_start = finfo->csr_start;
> + feature->csr_size = finfo->csr_size;
>
> /*
> * the FIU header feature has some fundamental functions (sriov
> @@ -1054,6 +1065,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> {
> unsigned int irq_base, nr_irqs;
> struct dfl_feature_info *finfo;
> + u8 dfh_version = 0;
> u8 revision = 0;
> int ret;
> u64 v;
> @@ -1061,7 +1073,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> if (fid != FEATURE_ID_AFU) {
> v = readq(binfo->ioaddr + ofst);
> revision = FIELD_GET(DFH_REVISION, v);
> -
> + dfh_version = FIELD_GET(DFH_VERSION, v);
> /* read feature size and id if inputs are invalid */
> size = size ? size : feature_size(v);
> fid = fid ? fid : feature_id(v);
> @@ -1080,12 +1092,24 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>
> finfo->fid = fid;
> finfo->revision = revision;
> + finfo->dfh_version = dfh_version;
> finfo->mmio_res.start = binfo->start + ofst;
> finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> finfo->mmio_res.flags = IORESOURCE_MEM;
> finfo->irq_base = irq_base;
> finfo->nr_irqs = nr_irqs;
Ordering here seems slightly odd. If finfo would be built before calling
parse_feature_irqs(), it could be passed into there and there would be no
need to:
- read version for second time
- pass irq_base & nr_irqs as pointer parameters
--
i.
On Fri, Sep 23, 2022 at 05:17:45AM -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> Reported-by: kernel test robot <[email protected]>
New feature may not be reported. How?!
--
With Best Regards,
Andy Shevchenko
On Fri, Sep 23, 2022 at 05:17:43AM -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
...
> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +
Unneeded blank line.
> + v = FIELD_GET(DFH_VERSION, readq(base));
> + switch (v) {
This v...
> + case 0:
> + break;
> +
> + case 1:
> + v = readq(base + DFHv1_CSR_SIZE_GRP);
Extra space.
...and this v are semantically different. It's quite hard to deduce their
semantics.
> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> + DFHv1_PARAM_ID_MSIX);
I guess I have suggested to use temporary variable(s) here.
void __iomem *dfhv1 = base + DFHv1...;
void __iomem *nth;
> + if (off >= 0) {
nth = dfhv1 + off;
> + ibase = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_STARTV);
> + inr = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_NUMV);
ibase = readl(nth + DFHv1_PARAM_MSIX_STARTV);
inr = readl(nth + DFHv1_PARAM_MSIX_NUMV);
> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> + ibase, inr, fid);
> + }
> + }
> + break;
> +
> + default:
> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> + break;
> + }
> + }
--
With Best Regards,
Andy Shevchenko
On Fri, 23 Sep 2022, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> ---
> v2: clean up error messages
> alphabetize header files
> fix 'missing prototype' error by making function static
> tried to sort Makefile and Kconfig better
> ---
> drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 9 ++
> drivers/tty/serial/8250/Makefile | 1 +
> include/linux/dfl.h | 7 ++
> 4 files changed, 194 insertions(+)
> create mode 100644 drivers/tty/serial/8250/8250_dfl.c
>
> diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
> new file mode 100644
> index 000000000000..539ca6138eda
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA UART
> + *
> + * Copyright (C) 2022 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Ananda Ravuri <[email protected]>
> + * Matthew Gerlach <[email protected]>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/dfl.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
> +
> +struct dfl_uart {
> + void __iomem *csr_base;
> + struct device *dev;
> + u64 uart_clk;
> + u64 fifo_len;
> + unsigned int fifo_size;
> + unsigned int reg_shift;
Why to make this intermediate storage for these values, wouldn't it be
simpler to just fill them into the uart_port directly?
> + unsigned int line;
> +};
> +
> +static int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
> +{
> + void __iomem *param_base;
> + int off;
> + u64 v;
> +
> + v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
> +
> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> + dev_err(dfluart->dev, "missing required DFH parameters\n");
> + return -EINVAL;
> + }
> +
> + param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
Are all callers of dfl_find_param() expected to run these same checks and
calculations? Perhaps some helper to find param base would be useful and
it could also run those checks.
> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> + if (off < 0) {
> + dev_err(dfluart->dev, "missing CLK_FRQ param\n");
> + return -EINVAL;
> + }
> +
> + dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
> + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
> +
> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> + if (off < 0) {
> + dev_err(dfluart->dev, "missing FIFO_LEN param\n");
> + return -EINVAL;
> + }
> +
> + dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
> + dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
> +
> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> + if (off < 0) {
> + dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
> + return -EINVAL;
> + }
> +
> + v = readq(param_base + off + DFHv1_PARAM_DATA);
> + dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
???
> + dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> + dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
> + dfluart->fifo_size, dfluart->reg_shift);
> +
> + return 0;
> +}
> +
> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
> +{
> + struct device *dev = &dfl_dev->dev;
> + struct uart_8250_port uart;
> + struct dfl_uart *dfluart;
> + int ret;
> +
> + memset(&uart, 0, sizeof(uart));
> +
> + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> + if (!dfluart)
> + return -ENOMEM;
> +
> + dfluart->dev = dev;
> +
> + dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> + if (IS_ERR(dfluart->csr_base)) {
> + return PTR_ERR(dfluart->csr_base);
> + }
No need for braces.
> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
> +{
> + struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
> +
> + if (dfluart->line > 0)
Line 0 is valid uart port. Perhaps you'd never see it here due to how the
8250 driver allocs ports but it would be better to not make this kind of
assumption.
> + serial8250_unregister_port(dfluart->line);
> +}
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 7d74ef8d1d20..a17aeccc501e 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -67,6 +67,13 @@
> #define DFHv1_PARAM_MSIX_STARTV 0x8
> #define DFHv1_PARAM_MSIX_NUMV 0xc
>
> +#define DFHv1_PARAM_ID_CLK_FRQ 0x2
> +#define DFHv1_PARAM_ID_FIFO_LEN 0x3
> +
> +#define DFHv1_PARAM_ID_REG_LAYOUT 0x4
> +#define DFHv1_PARAM_ID_REG_WIDTH GENMASK_ULL(63, 32)
> +#define DFHv1_PARAM_ID_REG_SHIFT GENMASK_ULL(31, 0)
Should UART be included into these names or are they intended to be more
generic parameters (for non-UART uses)?
--
i.
On 9/23/2022 5:17 AM, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> The location of a feature's registers is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address. Parse the location and pass the information
> to DFL driver.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> ---
> v2: Introduced in v2.
> ---
> drivers/fpga/dfl.c | 26 +++++++++++++++++++++++++-
> drivers/fpga/dfl.h | 4 ++++
> include/linux/dfl.h | 4 ++++
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index dfd3f563c92d..6fb4f30f93cf 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -381,6 +381,8 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
> ddev->feature_id = feature->id;
> ddev->revision = feature->revision;
> ddev->cdev = pdata->dfl_cdev;
> + ddev->csr_start = feature->csr_start;
> + ddev->csr_size = feature->csr_size;
>
> /* add mmio resource */
> parent_res = &pdev->resource[feature->resource_index];
> @@ -708,18 +710,25 @@ struct build_feature_devs_info {
> * struct dfl_feature_info - sub feature info collected during feature dev build
> *
> * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
> * @mmio_res: mmio resource of this sub feature.
> * @ioaddr: mapped base address of mmio resource.
> * @node: node in sub_features linked list.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
> * @irq_base: start of irq index in this sub feature.
> * @nr_irqs: number of irqs of this sub feature.
> */
> struct dfl_feature_info {
> u16 fid;
> u8 revision;
> + u8 dfh_version;
> struct resource mmio_res;
> void __iomem *ioaddr;
> struct list_head node;
> + resource_size_t csr_start;
> + resource_size_t csr_size;
> unsigned int irq_base;
> unsigned int nr_irqs;
> };
> @@ -797,6 +806,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> feature->dev = fdev;
> feature->id = finfo->fid;
> feature->revision = finfo->revision;> + feature->csr_start = finfo->csr_start;
> + feature->csr_size = finfo->csr_size;
>
> /*
> * the FIU header feature has some fundamental functions (sriov
> @@ -1054,6 +1065,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> {
> unsigned int irq_base, nr_irqs;
> struct dfl_feature_info *finfo;
> + u8 dfh_version = 0;
> u8 revision = 0;
> int ret;
> u64 v;
> @@ -1061,7 +1073,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> if (fid != FEATURE_ID_AFU) {
> v = readq(binfo->ioaddr + ofst);
> revision = FIELD_GET(DFH_REVISION, v);
> -
> + dfh_version = FIELD_GET(DFH_VERSION, v);
> /* read feature size and id if inputs are invalid */
> size = size ? size : feature_size(v);
> fid = fid ? fid : feature_id(v);
> @@ -1080,12 +1092,24 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>
> finfo->fid = fid;
> finfo->revision = revision;
> + finfo->dfh_version = dfh_version;
> finfo->mmio_res.start = binfo->start + ofst;
> finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> finfo->mmio_res.flags = IORESOURCE_MEM;
> finfo->irq_base = irq_base;
> finfo->nr_irqs = nr_irqs;
>
> + if (dfh_version == 1) {
> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> + if (v & DFHv1_CSR_ADDR_REL)
> + finfo->csr_start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> + else
> + finfo->csr_start = binfo->start + ofst + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> + finfo->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
> + }
> +
> list_add_tail(&finfo->node, &binfo->sub_features);
> binfo->feature_num++;
>
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index e620fcb02b5a..64cedd00dca4 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -217,6 +217,8 @@ struct dfl_feature_irq_ctx {
> * this index is used to find its mmio resource from the
> * feature dev (platform device)'s resources.
> * @ioaddr: mapped mmio resource address.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
> * @irq_ctx: interrupt context list.
> * @nr_irqs: number of interrupt contexts.
> * @ops: ops of this sub feature.
> @@ -229,6 +231,8 @@ struct dfl_feature {
> u8 revision;
> int resource_index;
> void __iomem *ioaddr;
> + resource_size_t csr_start;
> + resource_size_t csr_size;
> struct dfl_feature_irq_ctx *irq_ctx;
> unsigned int nr_irqs;
> const struct dfl_feature_ops *ops;
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 33e21c360671..7d74ef8d1d20 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -84,6 +84,8 @@ enum dfl_id_type {
> * @type: type of DFL FIU of the device. See enum dfl_id_type.
> * @feature_id: feature identifier local to its DFL FIU type.
> * @mmio_res: mmio resource of this dfl device.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
> * @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.
> @@ -96,6 +98,8 @@ struct dfl_device {
> u16 feature_id;
> u8 revision;
> + u8 dfh_version;
This is already parsed as part of info dfl_feature_info, Can we add dfh_version to store it for further use ?
> struct resource mmio_res;
> + resource_size_t csr_start;
> + resource_size_t csr_size;
> int *irqs;
> unsigned int num_irqs;
> struct dfl_fpga_cdev *cdev;
On Fri, Sep 23, 2022 at 05:17:40AM -0700, [email protected] wrote:
> +With DFHv0, the register map of a given feature is located immediately following
> +the DFHv0 in the memory space. With DFHv1, the location of the feature register
> +map can be specified as an offset to the DFHv1 or as an absolute address. The DFHv1
> +structure is shown below:
> +
> + +-----------------------------------------------------------------------+
> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
> + +-----------------------------------------------------------------------+
> + |63 GUID_L 0|
> + +-----------------------------------------------------------------------+
> + |63 GUID_H 0|
> + +-----------------------------------------------------------------------+
> + |63 Address/Offset 1| Rel 0|
> + +-----------------------------------------------------------------------+
> + |63 Size of register set 32|Params 31|30 Group 16|15 Instance 0|
> + +-----------------------------------------------------------------------+
> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
> + +-----------------------------------------------------------------------+
> + |63 Parameter Data 0|
> + +-----------------------------------------------------------------------+
> +
> + ...
> +
> + +-----------------------------------------------------------------------+
> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
> + +-----------------------------------------------------------------------+
> + |63 Parameter Data 0|
> + +-----------------------------------------------------------------------+
> +
For consistency with DFL location diagram (which is above the DFHv1
diagram above), use literal code block instead of table:
---- >8 ----
diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 7c786b75b4988f..db6bff4aee25eb 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -584,7 +584,7 @@ across all types.
With DFHv0, the register map of a given feature is located immediately following
the DFHv0 in the memory space. With DFHv1, the location of the feature register
map can be specified as an offset to the DFHv1 or as an absolute address. The DFHv1
-structure is shown below:
+structure is shown below::
+-----------------------------------------------------------------------+
|63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
Thanks.
--
An old man doll... just what I always wanted! - Clara
On 9/23/22 5:17 AM, [email protected] wrote:
> From: Basheer Ahmed Muddebihal <[email protected]>
>
> Moving the DFH register offset and register definitions from
> drivers/fpga/dfl.h to include/linux/dfl.h. These definitions
> need to be accessed by dfl drivers that are outside of
> drivers/fpga.
This comment does not match what is done.
A move, a change in names and the introduction new defines.
I am not sure if moving these #defines is the best approach, the later
use of the in the uart with FIELD_GET's i think should be wrapped as
functions and these functions exported rather than the #defines.
So split this patch and justify why #defines are added to the user's
includes.
Tom
>
> Signed-off-by: Basheer Ahmed Muddebihal <[email protected]>
> Signed-off-by: Matthew Gerlach <[email protected]>
> ---
> v2: remove extra space in commit
> use uniform number of digits in constants
> don't change copyright date because of removed content
> ---
> drivers/fpga/dfl-afu-main.c | 4 ++--
> drivers/fpga/dfl.c | 2 +-
> drivers/fpga/dfl.h | 20 +-------------------
> include/linux/dfl.h | 33 ++++++++++++++++++++++++++++++++-
> 4 files changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> index 7f621e96d3b8..c26961ee33db 100644
> --- a/drivers/fpga/dfl-afu-main.c
> +++ b/drivers/fpga/dfl-afu-main.c
> @@ -468,8 +468,8 @@ afu_id_show(struct device *dev, struct device_attribute *attr, char *buf)
> return -EBUSY;
> }
>
> - guidl = readq(base + GUID_L);
> - guidh = readq(base + GUID_H);
> + guidl = readq(base + DFH_GUID_L);
> + guidh = readq(base + DFH_GUID_H);
> mutex_unlock(&pdata->lock);
>
> return scnprintf(buf, PAGE_SIZE, "%016llx%016llx\n", guidh, guidl);
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b9aae85ba930..1132f3c10440 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -1163,7 +1163,7 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> * find and parse FIU's child AFU via its NEXT_AFU register.
> * please note that only Port has valid NEXT_AFU pointer per spec.
> */
> - v = readq(binfo->ioaddr + NEXT_AFU);
> + v = readq(binfo->ioaddr + DFH_NEXT_AFU);
>
> offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
> if (offset)
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 06cfcd5e84bb..e620fcb02b5a 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -17,6 +17,7 @@
> #include <linux/bitfield.h>
> #include <linux/cdev.h>
> #include <linux/delay.h>
> +#include <linux/dfl.h>
> #include <linux/eventfd.h>
> #include <linux/fs.h>
> #include <linux/interrupt.h>
> @@ -53,28 +54,9 @@
> #define PORT_FEATURE_ID_UINT 0x12
> #define PORT_FEATURE_ID_STP 0x13
>
> -/*
> - * Device Feature Header Register Set
> - *
> - * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> - * For AFUs, they have DFH + GUID as common header registers.
> - * For private features, they only have DFH register as common header.
> - */
> -#define DFH 0x0
> -#define GUID_L 0x8
> -#define GUID_H 0x10
> -#define NEXT_AFU 0x18
> -
> -#define DFH_SIZE 0x8
> -
> /* Device Feature Header Register Bitfield */
> -#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID */
> #define DFH_ID_FIU_FME 0
> #define DFH_ID_FIU_PORT 1
> -#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
> -#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
> -#define DFH_EOL BIT_ULL(40) /* End of list */
> -#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
> #define DFH_TYPE_AFU 1
> #define DFH_TYPE_PRIVATE 3
> #define DFH_TYPE_FIU 4
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 431636a0dc78..33d167c53b09 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -2,7 +2,7 @@
> /*
> * Header file for DFL driver and device API
> *
> - * Copyright (C) 2020 Intel Corporation, Inc.
> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
> */
>
> #ifndef __LINUX_DFL_H
> @@ -11,6 +11,37 @@
> #include <linux/device.h>
> #include <linux/mod_devicetable.h>
>
> +/*
> + * Device Feature Header Register Set
> + *
> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> + * For AFUs, they have DFH + GUID as common header registers.
> + * For private features, they only have DFH register as common header.
> + */
> +#define DFH 0x00
> +#define DFH_GUID_L 0x08
> +#define DFH_GUID_H 0x10
> +#define DFH_NEXT_AFU 0x18
> +
> +/*
> + * DFHv1 Register Offset definitons
> + * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
> + * as common header registers
> + */
> +#define DFHv1_CSR_ADDR 0x18 /* CSR Register start address */
> +#define DFHv1_CSR_SIZE_GRP 0x20 /* Size of Reg Block and Group/tag */
> +#define DFHv1_PARAM_HDR 0x28 /* Optional First Param header */
> +#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */
> +
> +#define DFH_SIZE 0x08
> +
> +/* Device Feature Header Register Bitfield */
> +#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID */
> +#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
> +#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
> +#define DFH_EOL BIT_ULL(40) /* End of list */
> +#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
> +
> /**
> * enum dfl_id_type - define the DFL FIU types
> */
On Fri, 23 Sep 2022, Ilpo J?rvinen wrote:
> On Fri, 23 Sep 2022, [email protected] wrote:
>
>> From: Matthew Gerlach <[email protected]>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>>
>> Signed-off-by: Matthew Gerlach <[email protected]>
>> ---
>> v2: fix kernel doc
>> clarify use of DFH_VERSION field
>> ---
>> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
>> include/linux/dfl.h | 14 +++++++++++
>> 2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 1132f3c10440..dfd3f563c92d 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> void __iomem *base = binfo->ioaddr + ofst;
>> unsigned int i, ibase, inr = 0;
>> enum dfl_id_type type;
>> - int virq;
>> + int virq, off;
>> u64 v;
>>
>> type = feature_dev_id_type(binfo->feature_dev);
>>
>> /*
>> * Ideally DFL framework should only read info from DFL header, but
>> - * current version DFL only provides mmio resources information for
>> + * current version, DFHv0, only provides mmio resources information for
>> * each feature in DFL Header, no field for interrupt resources.
>> * Interrupt resource information is provided by specific mmio
>> * registers of each private feature which supports interrupt. So in
>> * order to parse and assign irq resources, DFL framework has to look
>> * into specific capability registers of these private features.
>> *
>> - * Once future DFL version supports generic interrupt resource
>> - * information in common DFL headers, the generic interrupt parsing
>> - * code will be added. But in order to be compatible to old version
>> + * DFHv1 supports generic interrupt resource information in DFHv1
>> + * parameter blocks. But in order to be compatible to old version
>> * DFL, the driver may still fall back to these quirks.
>> */
>> if (type == PORT_ID) {
>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> }
>> }
>>
>> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>> +
>> + v = FIELD_GET(DFH_VERSION, readq(base));
>
> I'd call this variable version (or ver) if you want to store it but it
> would also fit to switch () line so that no extra variable is needed.
I will change the v to dfh_ver to be clearer. I want to store the
value because it is used in the default case in the error message. The
error message helps to debug broken FPGA images.
>
>> + switch (v) {
>> + case 0:
>> + break;
>> +
>> + case 1:
>> + v = readq(base + DFHv1_CSR_SIZE_GRP);
>
> Extra space.
I will remove extra space.
>
>> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> + DFHv1_PARAM_ID_MSIX);
>> + if (off >= 0) {
>
> I'd reverse these 2 conditions and break when there's nothing to do.
I'm not sure what you mean by reversing these conditions because a DFHv1
may or may not have parameters (the first condition), and a DFHv1 may have
parameters but may not have a MSI-X parameter (the second condition).
>
>> + ibase = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_STARTV);
>> + inr = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_NUMV);
>> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> + ibase, inr, fid);
>> + }
>> + }
>> + break;
>> +
>> + default:
>> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> + break;
>> + }
>> + }
>> +
>> if (!inr) {
>> *irq_base = 0;
>> *nr_irqs = 0;
>
> --
> i.
>
>
On Fri, 23 Sep 2022, Andy Shevchenko wrote:
> On Fri, Sep 23, 2022 at 05:17:43AM -0700, [email protected] wrote:
>> From: Matthew Gerlach <[email protected]>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>
> ...
>
>> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>
>> +
>
> Unneeded blank line.
I will remove the blank line.
>
>> + v = FIELD_GET(DFH_VERSION, readq(base));
>> + switch (v) {
>
> This v...
>
>> + case 0:
>> + break;
>> +
>> + case 1:
>> + v = readq(base + DFHv1_CSR_SIZE_GRP);
>
> Extra space.
>
> ...and this v are semantically different. It's quite hard to deduce their
> semantics.
I was trying to be consistent with the existing code where the read was
stored in a temporary variable, v, and the FIELD_GET would be used for the
specific field. Will it be sufficiently clear if the v used above is
changed to dfl_ver, and this use of v followed by FIELD_GET remains as is?
>
>> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> + DFHv1_PARAM_ID_MSIX);
>
> I guess I have suggested to use temporary variable(s) here.
>
> void __iomem *dfhv1 = base + DFHv1...;
> void __iomem *nth;
>
>> + if (off >= 0) {
>
> nth = dfhv1 + off;
>
>> + ibase = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_STARTV);
>> + inr = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_NUMV);
>
> ibase = readl(nth + DFHv1_PARAM_MSIX_STARTV);
> inr = readl(nth + DFHv1_PARAM_MSIX_NUMV);
>
>> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> + ibase, inr, fid);
>> + }
>> + }
>> + break;
>> +
>> + default:
>> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> + break;
>> + }
>> + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
On Mon, 26 Sep 2022, [email protected] wrote:
>
>
> On Fri, 23 Sep 2022, Ilpo J?rvinen wrote:
>
> > On Fri, 23 Sep 2022, [email protected] wrote:
> >
> > > From: Matthew Gerlach <[email protected]>
> > >
> > > Define and use a DFHv1 parameter to add generic support for MSIX
> > > interrupts for DFL devices.
> > >
> > > Signed-off-by: Matthew Gerlach <[email protected]>
> > > ---
> > > v2: fix kernel doc
> > > clarify use of DFH_VERSION field
> > > ---
> > > drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
> > > include/linux/dfl.h | 14 +++++++++++
> > > 2 files changed, 69 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > index 1132f3c10440..dfd3f563c92d 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct
> > > build_feature_devs_info *binfo,
> > > void __iomem *base = binfo->ioaddr + ofst;
> > > unsigned int i, ibase, inr = 0;
> > > enum dfl_id_type type;
> > > - int virq;
> > > + int virq, off;
> > > u64 v;
> > >
> > > type = feature_dev_id_type(binfo->feature_dev);
> > >
> > > /*
> > > * Ideally DFL framework should only read info from DFL header, but
> > > - * current version DFL only provides mmio resources information for
> > > + * current version, DFHv0, only provides mmio resources information
> > > for
> > > * each feature in DFL Header, no field for interrupt resources.
> > > * Interrupt resource information is provided by specific mmio
> > > * registers of each private feature which supports interrupt. So in
> > > * order to parse and assign irq resources, DFL framework has to look
> > > * into specific capability registers of these private features.
> > > *
> > > - * Once future DFL version supports generic interrupt resource
> > > - * information in common DFL headers, the generic interrupt parsing
> > > - * code will be added. But in order to be compatible to old version
> > > + * DFHv1 supports generic interrupt resource information in DFHv1
> > > + * parameter blocks. But in order to be compatible to old version
> > > * DFL, the driver may still fall back to these quirks.
> > > */
> > > if (type == PORT_ID) {
> > > @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct
> > > build_feature_devs_info *binfo,
> > > }
> > > }
> > >
> > > + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> > > + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> > > +
> > > + v = FIELD_GET(DFH_VERSION, readq(base));
> >
> > I'd call this variable version (or ver) if you want to store it but it
> > would also fit to switch () line so that no extra variable is needed.
>
> I will change the v to dfh_ver to be clearer. I want to store the value
> because it is used in the default case in the error message. The error
> message helps to debug broken FPGA images.
Right, I missed that (or didn't think it too much and all being called
"v" didn't help either :-)).
> > > + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> > > + off = dfl_find_param(base + DFHv1_PARAM_HDR,
> > > ofst,
> > > + DFHv1_PARAM_ID_MSIX);
> > > + if (off >= 0) {
> >
> > I'd reverse these 2 conditions and break when there's nothing to do.
>
> I'm not sure what you mean by reversing these conditions because a DFHv1 may
> or may not have parameters (the first condition), and a DFHv1 may have
> parameters but may not have a MSI-X parameter (the second condition).
This is what I meant:
if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v))
break;
off = dfl_find_param(...);
if (off < 0)
break;
ibase = ...
--
i.
> > > + ibase = readl(base + DFHv1_PARAM_HDR +
> > > + off +
> > > DFHv1_PARAM_MSIX_STARTV);
> > > + inr = readl(base + DFHv1_PARAM_HDR +
> > > + off +
> > > DFHv1_PARAM_MSIX_NUMV);
> > > + dev_dbg(binfo->dev, "start %d num %d
> > > fid 0x%x\n",
> > > + ibase, inr, fid);
> > > + }
> > > + }
> > > + break;
On Tue, 27 Sep 2022, Ilpo J?rvinen wrote:
> On Mon, 26 Sep 2022, [email protected] wrote:
>
>>
>>
>> On Fri, 23 Sep 2022, Ilpo J?rvinen wrote:
>>
>>> On Fri, 23 Sep 2022, [email protected] wrote:
>>>
>>>> From: Matthew Gerlach <[email protected]>
>>>>
>>>> Define and use a DFHv1 parameter to add generic support for MSIX
>>>> interrupts for DFL devices.
>>>>
>>>> Signed-off-by: Matthew Gerlach <[email protected]>
>>>> ---
>>>> v2: fix kernel doc
>>>> clarify use of DFH_VERSION field
>>>> ---
>>>> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
>>>> include/linux/dfl.h | 14 +++++++++++
>>>> 2 files changed, 69 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>>>> index 1132f3c10440..dfd3f563c92d 100644
>>>> --- a/drivers/fpga/dfl.c
>>>> +++ b/drivers/fpga/dfl.c
>>>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct
>>>> build_feature_devs_info *binfo,
>>>> void __iomem *base = binfo->ioaddr + ofst;
>>>> unsigned int i, ibase, inr = 0;
>>>> enum dfl_id_type type;
>>>> - int virq;
>>>> + int virq, off;
>>>> u64 v;
>>>>
>>>> type = feature_dev_id_type(binfo->feature_dev);
>>>>
>>>> /*
>>>> * Ideally DFL framework should only read info from DFL header, but
>>>> - * current version DFL only provides mmio resources information for
>>>> + * current version, DFHv0, only provides mmio resources information
>>>> for
>>>> * each feature in DFL Header, no field for interrupt resources.
>>>> * Interrupt resource information is provided by specific mmio
>>>> * registers of each private feature which supports interrupt. So in
>>>> * order to parse and assign irq resources, DFL framework has to look
>>>> * into specific capability registers of these private features.
>>>> *
>>>> - * Once future DFL version supports generic interrupt resource
>>>> - * information in common DFL headers, the generic interrupt parsing
>>>> - * code will be added. But in order to be compatible to old version
>>>> + * DFHv1 supports generic interrupt resource information in DFHv1
>>>> + * parameter blocks. But in order to be compatible to old version
>>>> * DFL, the driver may still fall back to these quirks.
>>>> */
>>>> if (type == PORT_ID) {
>>>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct
>>>> build_feature_devs_info *binfo,
>>>> }
>>>> }
>>>>
>>>> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>>>> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>>>> +
>>>> + v = FIELD_GET(DFH_VERSION, readq(base));
>>>
>>> I'd call this variable version (or ver) if you want to store it but it
>>> would also fit to switch () line so that no extra variable is needed.
>>
>> I will change the v to dfh_ver to be clearer. I want to store the value
>> because it is used in the default case in the error message. The error
>> message helps to debug broken FPGA images.
>
> Right, I missed that (or didn't think it too much and all being called
> "v" didn't help either :-)).
>
>>>> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>>>> + off = dfl_find_param(base + DFHv1_PARAM_HDR,
>>>> ofst,
>>>> + DFHv1_PARAM_ID_MSIX);
>>>> + if (off >= 0) {
>>>
>>> I'd reverse these 2 conditions and break when there's nothing to do.
>>
>> I'm not sure what you mean by reversing these conditions because a DFHv1 may
>> or may not have parameters (the first condition), and a DFHv1 may have
>> parameters but may not have a MSI-X parameter (the second condition).
>
> This is what I meant:
>
> if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v))
> break;
>
> off = dfl_find_param(...);
> if (off < 0)
> break;
>
> ibase = ...
I understand now. This is a good suggestion because the resulting
indentation is better.
Thanks,
Matthew
>
>
> --
> i.
>
>
>>>> + ibase = readl(base + DFHv1_PARAM_HDR +
>>>> + off +
>>>> DFHv1_PARAM_MSIX_STARTV);
>>>> + inr = readl(base + DFHv1_PARAM_HDR +
>>>> + off +
>>>> DFHv1_PARAM_MSIX_NUMV);
>>>> + dev_dbg(binfo->dev, "start %d num %d
>>>> fid 0x%x\n",
>>>> + ibase, inr, fid);
>>>> + }
>>>> + }
>>>> + break;
>
On Tue, 27 Sep 2022, [email protected] wrote:
>
>
> On Fri, 23 Sep 2022, Ilpo J?rvinen wrote:
>
> > On Fri, 23 Sep 2022, [email protected] wrote:
> >
> > > From: Matthew Gerlach <[email protected]>
> > >
> > > Add documentation describing the extensions provided by Version
> > > 1 of the Device Feature Header (DFHv1).
> > >
> > > Signed-off-by: Matthew Gerlach <[email protected]>
> > > ---
> > > v2: s/GUILD/GUID/
> > > add picture
> > > ---
> > > Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 49 insertions(+)
> > >
> > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > > index 15b670926084..7c786b75b498 100644
> > > --- a/Documentation/fpga/dfl.rst
> > > +++ b/Documentation/fpga/dfl.rst
> > > @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id
> > > should be added to the
> > > driver's id_table.
> > >
> > >
> > > +Extending the Device Feature Header - DFHv1
> > > +===========================================
> > > +The current 8 bytes of the Device Feature Header, hereafter referred to
> > > as
> > > +to DFHv0, provide very little opportunity for the hardware to describe
> > > itself
> > > +to software. Version 1 of the Device Feature Header (DFHv1) is being
> > > introduced
> > > +to provide increased flexibility and extensibility to hardware designs
> > > using
> > > +Device Feature Lists. The list below describes some of the goals behind
> > > the
> > > +changes in DFHv1:
> > > +
> > > +* Provide a standardized mechanism for features to describe
> > > + parameters/capabilities to software.
> > > +* Standardize the use of a GUID for all DFHv1 types.
> > > +* Decouple the location of the DFH from the register space of the feature
> > > itself.
> > > +
> > > +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to
> > > associate
> > > +a list of parameter values to a particular feature.
> > > +
> > > +With DFHv0, not all features types contained a GUID. DFHv1 makes the
> > > GUID standard
> > > +across all types.
> > > +
> > > +With DFHv0, the register map of a given feature is located immediately
> > > following
> > > +the DFHv0 in the memory space. With DFHv1, the location of the feature
> > > register
> > > +map can be specified as an offset to the DFHv1 or as an absolute address.
> > > The DFHv1
> > > +structure is shown below:
> > > +
> > > +
> > > +-----------------------------------------------------------------------+
> > > + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11
> > > ID 0|
> > > +
> > > +-----------------------------------------------------------------------+
> > > + |63 GUID_L
> > > 0|
> > > +
> > > +-----------------------------------------------------------------------+
> > > + |63 GUID_H
> > > 0|
> > > +
> > > +-----------------------------------------------------------------------+
> > > + |63 Address/Offset 1| Rel
> > > 0|
> > > +
> > > +-----------------------------------------------------------------------+
> >
> > Is something missing here given the layout is claimed (in 2/6) to be:
> >
> > "DFHv1 Register Offset definitons
> > In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA"
> >
> > ?
>
>
> I was hesitant to have a picture because the description would then be in two
> places. I suspect my picture is not clear, but it does line up with the
> offset definitions:
>
> DFH offset 0x0
> GUID offsets 0x8 and 0x10
> CSR_START offset 0x18
> CSR_SIZE offset 0x20
> First PARAM_HDR, if it exists, is 0x28,
> First PARAM_DATA, if it exists, is 0x30.
I already noted in the other email I figured it out. It was thanks to
the offsets in the header how I found out where I had misintepreted
things. I initially had thought PARAM_DATA would be the parameters, both
headers and param data, but then realized that it's PARAM_HDR+PARAM_DATA
which is repeated n times.
I don't think there's need to fix anything in here.
--
i.
On Fri, 23 Sep 2022, Ilpo J?rvinen wrote:
> On Fri, 23 Sep 2022, [email protected] wrote:
>
>> From: Matthew Gerlach <[email protected]>
>>
>> Add documentation describing the extensions provided by Version
>> 1 of the Device Feature Header (DFHv1).
>>
>> Signed-off-by: Matthew Gerlach <[email protected]>
>> ---
>> v2: s/GUILD/GUID/
>> add picture
>> ---
>> Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>>
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 15b670926084..7c786b75b498 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
>> driver's id_table.
>>
>>
>> +Extending the Device Feature Header - DFHv1
>> +===========================================
>> +The current 8 bytes of the Device Feature Header, hereafter referred to as
>> +to DFHv0, provide very little opportunity for the hardware to describe itself
>> +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
>> +to provide increased flexibility and extensibility to hardware designs using
>> +Device Feature Lists. The list below describes some of the goals behind the
>> +changes in DFHv1:
>> +
>> +* Provide a standardized mechanism for features to describe
>> + parameters/capabilities to software.
>> +* Standardize the use of a GUID for all DFHv1 types.
>> +* Decouple the location of the DFH from the register space of the feature itself.
>> +
>> +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
>> +a list of parameter values to a particular feature.
>> +
>> +With DFHv0, not all features types contained a GUID. DFHv1 makes the GUID standard
>> +across all types.
>> +
>> +With DFHv0, the register map of a given feature is located immediately following
>> +the DFHv0 in the memory space. With DFHv1, the location of the feature register
>> +map can be specified as an offset to the DFHv1 or as an absolute address. The DFHv1
>> +structure is shown below:
>> +
>> + +-----------------------------------------------------------------------+
>> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
>> + +-----------------------------------------------------------------------+
>> + |63 GUID_L 0|
>> + +-----------------------------------------------------------------------+
>> + |63 GUID_H 0|
>> + +-----------------------------------------------------------------------+
>> + |63 Address/Offset 1| Rel 0|
>> + +-----------------------------------------------------------------------+
>
> Is something missing here given the layout is claimed (in 2/6) to be:
>
> "DFHv1 Register Offset definitons
> In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA"
>
> ?
I was hesitant to have a picture because the description would then be in
two places. I suspect my picture is not clear, but it does line up with
the offset definitions:
DFH offset 0x0
GUID offsets 0x8 and 0x10
CSR_START offset 0x18
CSR_SIZE offset 0x20
First PARAM_HDR, if it exists, is 0x28,
First PARAM_DATA, if it exists, is 0x30.
>
>> + |63 Size of register set 32|Params 31|30 Group 16|15 Instance 0|
>> + +-----------------------------------------------------------------------+
>> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
>> + +-----------------------------------------------------------------------+
>> + |63 Parameter Data 0|
>> + +-----------------------------------------------------------------------+
>> +
>> + ...
>> +
>> + +-----------------------------------------------------------------------+
>> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
>> + +-----------------------------------------------------------------------+
>> + |63 Parameter Data 0|
>> + +-----------------------------------------------------------------------+
>> +
>> Open discussion
>> ===============
>> FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
>>
>
> --
> i.
>
>
On 2022-09-23 at 05:17:43 -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> ---
> v2: fix kernel doc
> clarify use of DFH_VERSION field
> ---
> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
> include/linux/dfl.h | 14 +++++++++++
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 1132f3c10440..dfd3f563c92d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> void __iomem *base = binfo->ioaddr + ofst;
> unsigned int i, ibase, inr = 0;
> enum dfl_id_type type;
> - int virq;
> + int virq, off;
> u64 v;
>
> type = feature_dev_id_type(binfo->feature_dev);
>
> /*
> * Ideally DFL framework should only read info from DFL header, but
> - * current version DFL only provides mmio resources information for
> + * current version, DFHv0, only provides mmio resources information for
With this patchset, it's not 'current version' anymore.
> * each feature in DFL Header, no field for interrupt resources.
> * Interrupt resource information is provided by specific mmio
> * registers of each private feature which supports interrupt. So in
> * order to parse and assign irq resources, DFL framework has to look
> * into specific capability registers of these private features.
> *
> - * Once future DFL version supports generic interrupt resource
> - * information in common DFL headers, the generic interrupt parsing
> - * code will be added. But in order to be compatible to old version
> + * DFHv1 supports generic interrupt resource information in DFHv1
> + * parameter blocks. But in order to be compatible to old version
> * DFL, the driver may still fall back to these quirks.
> */
> if (type == PORT_ID) {
> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> }
> }
>
> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +
> + v = FIELD_GET(DFH_VERSION, readq(base));
> + switch (v) {
> + case 0:
> + break;
In last version, you mentioned that there will be no quirk for DFLv1, so
how about:
v = FIELD_GET(DFH_VERSION, readq(base));
if (v == 0) {
/* quirks */
} else {
/* parse PARAM MSIX */
}
No need to check specific feature ids again.
Thanks,
Yilun
> +
> + case 1:
> + v = readq(base + DFHv1_CSR_SIZE_GRP);
> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> + DFHv1_PARAM_ID_MSIX);
> + if (off >= 0) {
> + ibase = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_STARTV);
> + inr = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_NUMV);
> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> + ibase, inr, fid);
> + }
> + }
> + break;
> +
> + default:
> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> + break;
> + }
> + }
> +
> if (!inr) {
> *irq_base = 0;
> *nr_irqs = 0;
> @@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
> }
> EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param)
> +{
> + int off = 0;
> + u64 v, next;
> +
> + while (off < max) {
> + v = readq(base + off);
> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> + return off;
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + if (!next)
> + break;
> +
> + off += next;
> + }
> +
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(dfl_find_param);
> +
> static void __exit dfl_fpga_exit(void)
> {
> dfl_chardev_uinit();
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 1e53468ba8d8..33e21c360671 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -63,6 +63,10 @@
> #define DFHv1_PARAM_HDR_VERSION GENMASK_ULL(31, 16) /* Version Param */
> #define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 32) /* Offset of next Param */
>
> +#define DFHv1_PARAM_ID_MSIX 0x1
> +#define DFHv1_PARAM_MSIX_STARTV 0x8
> +#define DFHv1_PARAM_MSIX_NUMV 0xc
> +
> /**
> * enum dfl_id_type - define the DFL FIU types
> */
> @@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> module_driver(__dfl_driver, dfl_driver_register, \
> dfl_driver_unregister)
>
> +/**
> + * dfl_find_param() - find the offset of the given parameter
> + * @base: base pointer to start of dfl parameters in DFH
> + * @max: maximum offset to search
> + * @param: id of dfl parameter
> + *
> + * Return: positive offset on success, negative error code otherwise.
> + */
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);
> +
> #endif /* __LINUX_DFL_H */
> --
> 2.25.1
>
On 2022-09-24 at 06:00:19 -0700, Tom Rix wrote:
>
> On 9/23/22 5:17 AM, [email protected] wrote:
> > From: Basheer Ahmed Muddebihal <[email protected]>
> >
> > Moving the DFH register offset and register definitions from
> > drivers/fpga/dfl.h to include/linux/dfl.h. These definitions
> > need to be accessed by dfl drivers that are outside of
> > drivers/fpga.
>
> This comment does not match what is done.
>
> A move, a change in names and the introduction new defines.
>
> I am not sure if moving these #defines is the best approach, the later use
> of the in the uart with FIELD_GET's i think should be wrapped as functions
> and these functions exported rather than the #defines.
I agree, to play with all domains across kernel, we'd better not expose
too much bus specific details.
Thanks,
Yilun
On 2022-09-23 at 05:17:44 -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> The location of a feature's registers is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address. Parse the location and pass the information
> to DFL driver.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> ---
> v2: Introduced in v2.
> ---
> drivers/fpga/dfl.c | 26 +++++++++++++++++++++++++-
> drivers/fpga/dfl.h | 4 ++++
> include/linux/dfl.h | 4 ++++
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index dfd3f563c92d..6fb4f30f93cf 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -381,6 +381,8 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
> ddev->feature_id = feature->id;
> ddev->revision = feature->revision;
> ddev->cdev = pdata->dfl_cdev;
> + ddev->csr_start = feature->csr_start;
> + ddev->csr_size = feature->csr_size;
>
> /* add mmio resource */
> parent_res = &pdev->resource[feature->resource_index];
> @@ -708,18 +710,25 @@ struct build_feature_devs_info {
> * struct dfl_feature_info - sub feature info collected during feature dev build
> *
> * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
> * @mmio_res: mmio resource of this sub feature.
> * @ioaddr: mapped base address of mmio resource.
> * @node: node in sub_features linked list.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
> * @irq_base: start of irq index in this sub feature.
> * @nr_irqs: number of irqs of this sub feature.
> */
> struct dfl_feature_info {
> u16 fid;
> u8 revision;
> + u8 dfh_version;
> struct resource mmio_res;
> void __iomem *ioaddr;
> struct list_head node;
> + resource_size_t csr_start;
> + resource_size_t csr_size;
> unsigned int irq_base;
> unsigned int nr_irqs;
> };
> @@ -797,6 +806,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> feature->dev = fdev;
> feature->id = finfo->fid;
> feature->revision = finfo->revision;
> + feature->csr_start = finfo->csr_start;
> + feature->csr_size = finfo->csr_size;
>
> /*
> * the FIU header feature has some fundamental functions (sriov
> @@ -1054,6 +1065,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> {
> unsigned int irq_base, nr_irqs;
> struct dfl_feature_info *finfo;
> + u8 dfh_version = 0;
> u8 revision = 0;
> int ret;
> u64 v;
> @@ -1061,7 +1073,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> if (fid != FEATURE_ID_AFU) {
> v = readq(binfo->ioaddr + ofst);
> revision = FIELD_GET(DFH_REVISION, v);
> -
> + dfh_version = FIELD_GET(DFH_VERSION, v);
> /* read feature size and id if inputs are invalid */
> size = size ? size : feature_size(v);
> fid = fid ? fid : feature_id(v);
> @@ -1080,12 +1092,24 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>
> finfo->fid = fid;
> finfo->revision = revision;
> + finfo->dfh_version = dfh_version;
> finfo->mmio_res.start = binfo->start + ofst;
> finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> finfo->mmio_res.flags = IORESOURCE_MEM;
> finfo->irq_base = irq_base;
> finfo->nr_irqs = nr_irqs;
>
> + if (dfh_version == 1) {
> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> + if (v & DFHv1_CSR_ADDR_REL)
> + finfo->csr_start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> + else
> + finfo->csr_start = binfo->start + ofst + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> + finfo->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
> + }
> +
> list_add_tail(&finfo->node, &binfo->sub_features);
> binfo->feature_num++;
>
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index e620fcb02b5a..64cedd00dca4 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -217,6 +217,8 @@ struct dfl_feature_irq_ctx {
> * this index is used to find its mmio resource from the
> * feature dev (platform device)'s resources.
> * @ioaddr: mapped mmio resource address.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
> * @irq_ctx: interrupt context list.
> * @nr_irqs: number of interrupt contexts.
> * @ops: ops of this sub feature.
> @@ -229,6 +231,8 @@ struct dfl_feature {
> u8 revision;
> int resource_index;
> void __iomem *ioaddr;
> + resource_size_t csr_start;
> + resource_size_t csr_size;
> struct dfl_feature_irq_ctx *irq_ctx;
> unsigned int nr_irqs;
> const struct dfl_feature_ops *ops;
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 33e21c360671..7d74ef8d1d20 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -84,6 +84,8 @@ enum dfl_id_type {
> * @type: type of DFL FIU of the device. See enum dfl_id_type.
> * @feature_id: feature identifier local to its DFL FIU type.
> * @mmio_res: mmio resource of this dfl device.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
> * @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.
> @@ -96,6 +98,8 @@ struct dfl_device {
> u16 feature_id;
> u8 revision;
> struct resource mmio_res;
> + resource_size_t csr_start;
> + resource_size_t csr_size;
I think these register start & size info could be stored in
struct resource mmio_res. This is the generic way for the driver to
understand the register layout of the device. And it makes the dfl
driver code easier to be understood by other domains reviewers.
Thanks,
Yilun
> int *irqs;
> unsigned int num_irqs;
> struct dfl_fpga_cdev *cdev;
> --
> 2.25.1
>
On 2022-09-23 at 05:17:45 -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
>
> Signed-off-by: Matthew Gerlach <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> ---
> v2: clean up error messages
> alphabetize header files
> fix 'missing prototype' error by making function static
> tried to sort Makefile and Kconfig better
> ---
> drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 9 ++
> drivers/tty/serial/8250/Makefile | 1 +
> include/linux/dfl.h | 7 ++
> 4 files changed, 194 insertions(+)
> create mode 100644 drivers/tty/serial/8250/8250_dfl.c
>
> diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
> new file mode 100644
> index 000000000000..539ca6138eda
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA UART
> + *
> + * Copyright (C) 2022 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Ananda Ravuri <[email protected]>
> + * Matthew Gerlach <[email protected]>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/dfl.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
> +
> +struct dfl_uart {
> + void __iomem *csr_base;
> + struct device *dev;
> + u64 uart_clk;
> + u64 fifo_len;
> + unsigned int fifo_size;
> + unsigned int reg_shift;
> + unsigned int line;
> +};
> +
> +static int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
> +{
> + void __iomem *param_base;
> + int off;
> + u64 v;
> +
> + v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
> +
> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> + dev_err(dfluart->dev, "missing required DFH parameters\n");
> + return -EINVAL;
> + }
> +
> + param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
> +
> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
I think the parameters of dfl_find_param are too complicated to be used
outside dfl framework (It is OK for dfl internal use).
How about:
dfl_find_param(struct dfl_device *ddev, int param_id)
Thanks,
Yilun
On Sat, 24 Sep 2022, Tom Rix wrote:
>
> On 9/23/22 5:17 AM, [email protected] wrote:
>> From: Basheer Ahmed Muddebihal <[email protected]>
>>
>> Moving the DFH register offset and register definitions from
>> drivers/fpga/dfl.h to include/linux/dfl.h. These definitions
>> need to be accessed by dfl drivers that are outside of
>> drivers/fpga.
>
> This comment does not match what is done.
>
> A move, a change in names and the introduction new defines.
>
> I am not sure if moving these #defines is the best approach, the later use of
> the in the uart with FIELD_GET's i think should be wrapped as functions and
> these functions exported rather than the #defines.
>
> So split this patch and justify why #defines are added to the user's
> includes.
>
> Tom
I agree the original intent "diverged in v2". I will minimize moving and
make helper functions to simplify things.
>
>>
>> Signed-off-by: Basheer Ahmed Muddebihal
>> <[email protected]>
>> Signed-off-by: Matthew Gerlach <[email protected]>
>> ---
>> v2: remove extra space in commit
>> use uniform number of digits in constants
>> don't change copyright date because of removed content
>> ---
>> drivers/fpga/dfl-afu-main.c | 4 ++--
>> drivers/fpga/dfl.c | 2 +-
>> drivers/fpga/dfl.h | 20 +-------------------
>> include/linux/dfl.h | 33 ++++++++++++++++++++++++++++++++-
>> 4 files changed, 36 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
>> index 7f621e96d3b8..c26961ee33db 100644
>> --- a/drivers/fpga/dfl-afu-main.c
>> +++ b/drivers/fpga/dfl-afu-main.c
>> @@ -468,8 +468,8 @@ afu_id_show(struct device *dev, struct device_attribute
>> *attr, char *buf)
>> return -EBUSY;
>> }
>> - guidl = readq(base + GUID_L);
>> - guidh = readq(base + GUID_H);
>> + guidl = readq(base + DFH_GUID_L);
>> + guidh = readq(base + DFH_GUID_H);
>> mutex_unlock(&pdata->lock);
>> return scnprintf(buf, PAGE_SIZE, "%016llx%016llx\n", guidh, guidl);
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index b9aae85ba930..1132f3c10440 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -1163,7 +1163,7 @@ static int parse_feature_fiu(struct
>> build_feature_devs_info *binfo,
>> * find and parse FIU's child AFU via its NEXT_AFU register.
>> * please note that only Port has valid NEXT_AFU pointer per spec.
>> */
>> - v = readq(binfo->ioaddr + NEXT_AFU);
>> + v = readq(binfo->ioaddr + DFH_NEXT_AFU);
>> offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
>> if (offset)
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 06cfcd5e84bb..e620fcb02b5a 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -17,6 +17,7 @@
>> #include <linux/bitfield.h>
>> #include <linux/cdev.h>
>> #include <linux/delay.h>
>> +#include <linux/dfl.h>
>> #include <linux/eventfd.h>
>> #include <linux/fs.h>
>> #include <linux/interrupt.h>
>> @@ -53,28 +54,9 @@
>> #define PORT_FEATURE_ID_UINT 0x12
>> #define PORT_FEATURE_ID_STP 0x13
>> -/*
>> - * Device Feature Header Register Set
>> - *
>> - * For FIUs, they all have DFH + GUID + NEXT_AFU as common header
>> registers.
>> - * For AFUs, they have DFH + GUID as common header registers.
>> - * For private features, they only have DFH register as common header.
>> - */
>> -#define DFH 0x0
>> -#define GUID_L 0x8
>> -#define GUID_H 0x10
>> -#define NEXT_AFU 0x18
>> -
>> -#define DFH_SIZE 0x8
>> -
>> /* Device Feature Header Register Bitfield */
>> -#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID
>> */
>> #define DFH_ID_FIU_FME 0
>> #define DFH_ID_FIU_PORT 1
>> -#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision
>> */
>> -#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH
>> */
>> -#define DFH_EOL BIT_ULL(40) /* End of
>> list */
>> -#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
>> #define DFH_TYPE_AFU 1
>> #define DFH_TYPE_PRIVATE 3
>> #define DFH_TYPE_FIU 4
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 431636a0dc78..33d167c53b09 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -2,7 +2,7 @@
>> /*
>> * Header file for DFL driver and device API
>> *
>> - * Copyright (C) 2020 Intel Corporation, Inc.
>> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
>> */
>> #ifndef __LINUX_DFL_H
>> @@ -11,6 +11,37 @@
>> #include <linux/device.h>
>> #include <linux/mod_devicetable.h>
>> +/*
>> + * Device Feature Header Register Set
>> + *
>> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header
>> registers.
>> + * For AFUs, they have DFH + GUID as common header registers.
>> + * For private features, they only have DFH register as common header.
>> + */
>> +#define DFH 0x00
>> +#define DFH_GUID_L 0x08
>> +#define DFH_GUID_H 0x10
>> +#define DFH_NEXT_AFU 0x18
>> +
>> +/*
>> + * DFHv1 Register Offset definitons
>> + * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR +
>> PARAM_DATA
>> + * as common header registers
>> + */
>> +#define DFHv1_CSR_ADDR 0x18 /* CSR Register start address
>> */
>> +#define DFHv1_CSR_SIZE_GRP 0x20 /* Size of Reg Block and Group/tag */
>> +#define DFHv1_PARAM_HDR 0x28 /* Optional First Param header
>> */
>> +#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param
>> header */
>> +
>> +#define DFH_SIZE 0x08
>> +
>> +/* Device Feature Header Register Bitfield */
>> +#define DFH_ID GENMASK_ULL(11, 0) /* Feature ID
>> */
>> +#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision
>> */
>> +#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH
>> */
>> +#define DFH_EOL BIT_ULL(40) /* End of
>> list */
>> +#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
>> +
>> /**
>> * enum dfl_id_type - define the DFL FIU types
>> */
>
>
On Fri, 30 Sep 2022, Xu Yilun wrote:
> On 2022-09-23 at 05:17:43 -0700, [email protected] wrote:
>> From: Matthew Gerlach <[email protected]>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>>
>> Signed-off-by: Matthew Gerlach <[email protected]>
>> ---
>> v2: fix kernel doc
>> clarify use of DFH_VERSION field
>> ---
>> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
>> include/linux/dfl.h | 14 +++++++++++
>> 2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 1132f3c10440..dfd3f563c92d 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> void __iomem *base = binfo->ioaddr + ofst;
>> unsigned int i, ibase, inr = 0;
>> enum dfl_id_type type;
>> - int virq;
>> + int virq, off;
>> u64 v;
>>
>> type = feature_dev_id_type(binfo->feature_dev);
>>
>> /*
>> * Ideally DFL framework should only read info from DFL header, but
>> - * current version DFL only provides mmio resources information for
>> + * current version, DFHv0, only provides mmio resources information for
>
> With this patchset, it's not 'current version' anymore.
I will update the comment. Thanks.
>
>> * each feature in DFL Header, no field for interrupt resources.
>> * Interrupt resource information is provided by specific mmio
>> * registers of each private feature which supports interrupt. So in
>> * order to parse and assign irq resources, DFL framework has to look
>> * into specific capability registers of these private features.
>> *
>> - * Once future DFL version supports generic interrupt resource
>> - * information in common DFL headers, the generic interrupt parsing
>> - * code will be added. But in order to be compatible to old version
>> + * DFHv1 supports generic interrupt resource information in DFHv1
>> + * parameter blocks. But in order to be compatible to old version
>> * DFL, the driver may still fall back to these quirks.
>> */
>> if (type == PORT_ID) {
>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> }
>> }
>>
>> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>> +
>> + v = FIELD_GET(DFH_VERSION, readq(base));
>> + switch (v) {
>> + case 0:
>> + break;
>
> In last version, you mentioned that there will be no quirk for DFLv1, so
> how about:
>
> v = FIELD_GET(DFH_VERSION, readq(base));
>
> if (v == 0) {
> /* quirks */
> } else {
> /* parse PARAM MSIX */
> }
>
> No need to check specific feature ids again.
With v3 changes I will use a switch state and not need quirks for v1.
>
> Thanks,
> Yilun
>
>> +
>> + case 1:
>> + v = readq(base + DFHv1_CSR_SIZE_GRP);
>> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> + DFHv1_PARAM_ID_MSIX);
>> + if (off >= 0) {
>> + ibase = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_STARTV);
>> + inr = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_NUMV);
>> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> + ibase, inr, fid);
>> + }
>> + }
>> + break;
>> +
>> + default:
>> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> + break;
>> + }
>> + }
>> +
>> if (!inr) {
>> *irq_base = 0;
>> *nr_irqs = 0;
>> @@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>> }
>> EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>>
>> +int dfl_find_param(void __iomem *base, resource_size_t max, int param)
>> +{
>> + int off = 0;
>> + u64 v, next;
>> +
>> + while (off < max) {
>> + v = readq(base + off);
>> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> + return off;
>> +
>> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> + if (!next)
>> + break;
>> +
>> + off += next;
>> + }
>> +
>> + return -ENOENT;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_find_param);
>> +
>> static void __exit dfl_fpga_exit(void)
>> {
>> dfl_chardev_uinit();
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 1e53468ba8d8..33e21c360671 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -63,6 +63,10 @@
>> #define DFHv1_PARAM_HDR_VERSION GENMASK_ULL(31, 16) /* Version Param */
>> #define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 32) /* Offset of next Param */
>>
>> +#define DFHv1_PARAM_ID_MSIX 0x1
>> +#define DFHv1_PARAM_MSIX_STARTV 0x8
>> +#define DFHv1_PARAM_MSIX_NUMV 0xc
>> +
>> /**
>> * enum dfl_id_type - define the DFL FIU types
>> */
>> @@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>> module_driver(__dfl_driver, dfl_driver_register, \
>> dfl_driver_unregister)
>>
>> +/**
>> + * dfl_find_param() - find the offset of the given parameter
>> + * @base: base pointer to start of dfl parameters in DFH
>> + * @max: maximum offset to search
>> + * @param: id of dfl parameter
>> + *
>> + * Return: positive offset on success, negative error code otherwise.
>> + */
>> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);
>> +
>> #endif /* __LINUX_DFL_H */
>> --
>> 2.25.1
>>
>