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 adds the definitions for DFHv1.
Patch 3 adds basic support DFHv1. It provides a generic mechanism for
describing MSIX interrupts used by a particular feature instance, and
it gets the location and size of the feature's register set from DFHv1.
Patch 4 adds a DFL uart driver that makes use of the new features of DFHv1.
Basheer Ahmed Muddebihal (1):
fpga: dfl: Add DFHv1 Register Definitions
Matthew Gerlach (3):
Documentation: fpga: dfl: Add documentation for DFHv1
fpga: dfl: add basic support DFHv1
tty: serial: 8250: add DFL bus driver for Altera 16550.
Documentation/fpga/dfl.rst | 96 ++++++++++++
drivers/fpga/dfl.c | 234 +++++++++++++++++++++++------
drivers/fpga/dfl.h | 38 +++++
drivers/tty/serial/8250/8250_dfl.c | 149 ++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 12 ++
drivers/tty/serial/8250/Makefile | 1 +
include/linux/dfl.h | 15 ++
7 files changed, 496 insertions(+), 49 deletions(-)
create mode 100644 drivers/tty/serial/8250/8250_dfl.c
--
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]>
---
v4: use dev_err_probe() everywhere that is appropriate
clean up noise
change error messages to use the word, unsupported
tried again to sort Makefile and KConfig better
reorder probe function for easier error handling
use new dfh_find_param API
v3: use passed in location of registers
use cleaned up functions for parsing parameters
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 | 149 +++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 12 +++
drivers/tty/serial/8250/Makefile | 1 +
3 files changed, 162 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..f02f0ba2a565
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,149 @@
+// 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 {
+ int line;
+};
+
+static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
+{
+ struct device *dev = &dfl_dev->dev;
+ u64 v, fifo_len, reg_width;
+ u64 *p;
+
+ p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
+ if (!p)
+ return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
+
+ uart->port.uartclk = *p;
+ dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
+
+ p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
+ if (!p)
+ return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
+
+ fifo_len = *p;
+ dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
+
+ switch (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:
+ return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
+ }
+
+ p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
+ if (!p)
+ return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
+
+ v = *p;
+ uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
+ reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
+
+ dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n", reg_width, uart->port.regshift);
+
+ switch (reg_width) {
+ case 4:
+ uart->port.iotype = UPIO_MEM32;
+ break;
+
+ case 2:
+ uart->port.iotype = UPIO_MEM16;
+ break;
+
+ default:
+ return dev_err_probe(dev, -EINVAL, "unsupported reg_width %lld\n", reg_width);
+ }
+
+ 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));
+ uart.port.flags = UPF_IOREMAP;
+ uart.port.mapbase = dfl_dev->mmio_res.start;
+ uart.port.mapsize = resource_size(&dfl_dev->mmio_res);
+
+ ret = dfl_uart_get_params(dfl_dev, &uart);
+
+ 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];
+
+ dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+ if (!dfluart)
+ return -ENOMEM;
+
+ dfluart->line = serial8250_register_8250_port(&uart);
+ if (dfluart->line < 0)
+ return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
+
+ 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);
+
+ 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..4efc8ee51c18 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -370,6 +370,18 @@ config SERIAL_8250_FSL
erratum for Freescale 16550 UARTs in the 8250 driver. It also
enables support for ACPI enumeration.
+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.
+
+ To compile this driver as a module, chose M here: the
+ module will be called 8250_dfl.
+
config SERIAL_8250_DW
tristate "Support for Synopsys DesignWare 8250 quirks"
depends on SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..65bc6ad4dd01 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -28,6 +28,7 @@ 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
obj-$(CONFIG_SERIAL_8250_MEN_MCB) += 8250_men_mcb.o
+obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
obj-$(CONFIG_SERIAL_8250_DW) += 8250_dw.o
obj-$(CONFIG_SERIAL_8250_EM) += 8250_em.o
obj-$(CONFIG_SERIAL_8250_IOC3) += 8250_ioc3.o
--
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]>
---
v4: Remove marketing speak and separate v0 and v1 descriptions.
Fix errors reported by "make htmldocs".
v3: no change
v2: s/GUILD/GUID/
add picture
---
Documentation/fpga/dfl.rst | 96 ++++++++++++++++++++++++++++++++++++++
1 file changed, 96 insertions(+)
diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 15b670926084..12365be435a8 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -561,6 +561,102 @@ new DFL feature via UIO direct access, its feature id should be added to the
driver's id_table.
+Device Feature Header - Version 0
+===========================================
+The format of Version 0 of a Device Feature Header (DFH) is shown below::
+
+ +-----------------------------------------------------------------------+
+ |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
+ +-----------------------------------------------------------------------+
+ |63 GUID_L 0| 0x08
+ +-----------------------------------------------------------------------+
+ |63 GUID_H 0| 0x10
+ +-----------------------------------------------------------------------+
+
+Offset 0x00
+Type - The type of DFH (e.g. FME, AFU, or private feature).
+DFH VER - The version of the DFH.
+Rsvd - Currently unused.
+EOL - Set if this DFH is the end of the Device Feature List (DFL).
+Next - The offset of the next DFH in the DFL from the start of the DFH.
+If EOL is set, Next refers to size of mmio for last feature in the list.
+ID - If Type field is 'private feature', then ID of the private feature.
+
+Offset 0x08
+GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
+if Type is FME or AFU.
+
+Offset 0x10
+GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
+if Type is FME or AFU.
+
+
+Device Feature Header - Version 1
+===========================================
+The format of Version 1 of a Device Feature Header (DFH) is shown below::
+
+ +-----------------------------------------------------------------------+
+ |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
+ +-----------------------------------------------------------------------+
+ |63 GUID_L 0| 0x08
+ +-----------------------------------------------------------------------+
+ |63 GUID_H 0| 0x10
+ +-----------------------------------------------------------------------+
+ |63 Address/Offset 1| Rel 0| 0x18
+ +-----------------------------------------------------------------------+
+ |63 Reg Size 32|Params 31|30 Group 16|15 Instance 0| 0x20
+ +-----------------------------------------------------------------------+
+ |63 Next 34|RSV33|EOP32|31 Param Version 16|15 Param ID 0| 0x28
+ +-----------------------------------------------------------------------+
+ |63 Parameter Data 0| 0x30
+ +-----------------------------------------------------------------------+
+
+ ...
+
+ +-----------------------------------------------------------------------+
+ |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
+ +-----------------------------------------------------------------------+
+ |63 Parameter Data 0|
+ +-----------------------------------------------------------------------+
+
+Offset 0x00
+Type - The type of DFH (e.g. FME, AFU, or private feature).
+DFH VER - The version of the DFH.
+Rsvd - Currently unused.
+EOL - Set if this DFH is the end of the Device Feature List (DFL).
+Next - The offset of the next DFH in the DFL from the start of the DFH.
+If EOL is set, Next refers to size of mmio for last feature in the list.
+ID - If Type field is 'private feature', then ID of the private feature.
+
+Offset 0x08
+GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
+
+Offset 0x10
+GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
+if Type is FME or AFU.
+
+Offset 0x18
+Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
+absolute address for the location of the feature's registers.
+If Rel bit is clear, then the feature's registers start at the
+offset from the start of the DFH.
+
+Offset 0x20
+Reg Size - Size of feature's register set.
+Params - Set if DFH has one or more parameter blocks.
+Group - Id of group if feature is part of a group.
+Instance - Id of instance of feature within a group.
+
+Offset 0x28 if feature has parameters
+Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
+If EOP set, size of last parameter.
+Param Version - Version of Param ID.
+Param ID - ID of parameter.
+
+Offset 0x30
+Parameter Data - Parameter data whose size and format is defined by version
+and ID of the parameter.
+
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]>
---
v4: s/MSIX/MSI_X/g
move kerneldoc to implementation
don't change copyright date
v3:
keep DFHv1 definitions "hidden" in drivers/fpga/dfl.h
v2: clean up whitespace and one line comments
remove extra space in commit
use uniform number of digits in constants
don't change copyright date because of removed content
dfl.h s/MSIX/MSI_X/g move kerneldoc
---
drivers/fpga/dfl.h | 33 +++++++++++++++++++++++++++++++++
include/linux/dfl.h | 11 +++++++++++
2 files changed, 44 insertions(+)
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 06cfcd5e84bb..45e6e1359a67 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -74,11 +74,44 @@
#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 */
#define DFH_TYPE_AFU 1
#define DFH_TYPE_PRIVATE 3
#define DFH_TYPE_FIU 4
+/*
+ * 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 */
+
+/*
+ * 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 */
+#define DFHv1_PARAM_HDR_NEXT_EOL BIT_ULL(0)
+#define DFHv1_PARAM_HDR_NEXT_MASK GENMASK_ULL(1, 0)
+#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */
+
/* Next AFU Register Bitfield */
#define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 431636a0dc78..fea9e16d35b6 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -11,6 +11,17 @@
#include <linux/device.h>
#include <linux/mod_devicetable.h>
+#define DFHv1_PARAM_ID_MSI_X 0x1
+#define DFHv1_PARAM_MSI_X_STARTV 0x0
+#define DFHv1_PARAM_MSI_X_NUMV 0x4
+
+#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]>
Add generic support for MSI-X interrupts for DFL devices.
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]>
---
v4: s/MSIX/MSI_X
move kernel doc to implementation
use structure assignment
fix decode of absolute address
clean up comment in parse_feature_irqs
remove use of csr_res
v3: remove unneeded blank line
use clearer variable name
pass finfo into parse_feature_irqs()
refactor code for better indentation
use switch statement for irq parsing
squash in code parsing register location
v2: fix kernel doc
clarify use of DFH_VERSION field
---
drivers/fpga/dfl.c | 234 ++++++++++++++++++++++++++++++++++----------
drivers/fpga/dfl.h | 5 +
include/linux/dfl.h | 4 +
3 files changed, 194 insertions(+), 49 deletions(-)
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..37f995e66436 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
if (ddev->mmio_res.parent)
release_resource(&ddev->mmio_res);
+ kfree(ddev->params);
+
ida_free(&dfl_device_ida, ddev->id);
kfree(ddev->irqs);
kfree(ddev);
@@ -380,7 +382,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
ddev->type = feature_dev_id_type(pdev);
ddev->feature_id = feature->id;
ddev->revision = feature->revision;
+ ddev->dfh_version = feature->dfh_version;
ddev->cdev = pdata->dfl_cdev;
+ if (feature->param_size) {
+ ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
+ if (!ddev->params) {
+ ret = -ENOMEM;
+ goto put_dev;
+ }
+ ddev->param_size = feature->param_size;
+ }
/* add mmio resource */
parent_res = &pdev->resource[feature->resource_index];
@@ -708,20 +719,27 @@ 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.
* @irq_base: start of irq index in this sub feature.
* @nr_irqs: number of irqs of this sub feature.
+ * @param_size: size DFH parameters.
+ * @params: DFH parameter data.
*/
struct dfl_feature_info {
u16 fid;
u8 revision;
+ u8 dfh_version;
struct resource mmio_res;
void __iomem *ioaddr;
struct list_head node;
unsigned int irq_base;
unsigned int nr_irqs;
+ unsigned int param_size;
+ u64 params[];
};
static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
@@ -797,7 +815,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
feature->dev = fdev;
feature->id = finfo->fid;
feature->revision = finfo->revision;
+ feature->dfh_version = finfo->dfh_version;
+ if (finfo->param_size) {
+ feature->params = devm_kmemdup(binfo->dev,
+ finfo->params, finfo->param_size,
+ GFP_KERNEL);
+ if (!feature->params)
+ return -ENOMEM;
+
+ feature->param_size = finfo->param_size;
+ }
/*
* the FIU header feature has some fundamental functions (sriov
* set, port enable/disable) needed for the dfl bus device and
@@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
return 0;
}
+static void *find_param(void *base, resource_size_t max, int param)
+{
+ int off = 0;
+ u64 v, next;
+
+ while (off < max) {
+ v = *(u64 *)(base + off);
+ if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+ return base + off + DFHv1_PARAM_DATA;
+
+ next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+ off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
+ if (next & DFHv1_PARAM_HDR_NEXT_EOL)
+ break;
+
+ }
+
+ return NULL;
+}
+
+/**
+ * dfh_find_param() - find data for the given parameter id
+ * @dfl_dev: dfl device
+ * @param: id of dfl parameter
+ *
+ * Return: pointer to parameter data on success, NULL otherwise.
+ */
+void *dfh_find_param(struct dfl_device *dfl_dev, int param)
+{
+ return find_param(dfl_dev->params, dfl_dev->param_size, param);
+}
+EXPORT_SYMBOL_GPL(dfh_find_param);
+
static int parse_feature_irqs(struct build_feature_devs_info *binfo,
- resource_size_t ofst, u16 fid,
- unsigned int *irq_base, unsigned int *nr_irqs)
+ resource_size_t ofst, struct dfl_feature_info *finfo)
{
void __iomem *base = binfo->ioaddr + ofst;
unsigned int i, ibase, inr = 0;
+ void *params = finfo->params;
enum dfl_id_type type;
+ u16 fid = finfo->fid;
int virq;
+ u32 *p;
u64 v;
- type = feature_dev_id_type(binfo->feature_dev);
+ switch (finfo->dfh_version) {
+ case 0:
+ /*
+ * DFHv0 only provides mmio resource information for each feature
+ * in the DFL header. There is no generic interrupt information.
+ * Instead, features with interrupt functionality provide
+ * the information in feature specific registers.
+ */
+ type = feature_dev_id_type(binfo->feature_dev);
+ if (type == PORT_ID) {
+ switch (fid) {
+ case PORT_FEATURE_ID_UINT:
+ v = readq(base + PORT_UINT_CAP);
+ ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
+ inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+ break;
+ case PORT_FEATURE_ID_ERROR:
+ v = readq(base + PORT_ERROR_CAP);
+ ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
+ inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+ break;
+ }
+ } else if (type == FME_ID) {
+ switch (fid) {
+ case FME_FEATURE_ID_GLOBAL_ERR:
+ v = readq(base + FME_ERROR_CAP);
+ ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
+ inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
+ break;
+ }
+ }
+ break;
- /*
- * Ideally DFL framework should only read info from DFL header, but
- * current version DFL 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
- * DFL, the driver may still fall back to these quirks.
- */
- if (type == PORT_ID) {
- switch (fid) {
- case PORT_FEATURE_ID_UINT:
- v = readq(base + PORT_UINT_CAP);
- ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
- inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+ case 1:
+ /*
+ * DFHv1 provides interrupt resource information in DFHv1
+ * parameter blocks.
+ */
+ if (!finfo->param_size)
break;
- case PORT_FEATURE_ID_ERROR:
- v = readq(base + PORT_ERROR_CAP);
- ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
- inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+
+ p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
+ if (!p)
break;
- }
- } else if (type == FME_ID) {
- if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
- v = readq(base + FME_ERROR_CAP);
- ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
- inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
- }
+
+ ibase = *p++;
+ inr = *p;
+ break;
+
+ default:
+ dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
+ break;
}
if (!inr) {
- *irq_base = 0;
- *nr_irqs = 0;
+ finfo->irq_base = 0;
+ finfo->nr_irqs = 0;
return 0;
}
@@ -1006,12 +1086,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
}
}
- *irq_base = ibase;
- *nr_irqs = inr;
+ finfo->irq_base = ibase;
+ finfo->nr_irqs = inr;
return 0;
}
+static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
+{
+ int size = 0;
+ u64 v, next;
+
+ if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
+ readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
+ return 0;
+
+ while (size + DFHv1_PARAM_HDR < max) {
+ v = readq(dfh_base + DFHv1_PARAM_HDR + size);
+
+ next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+ if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
+ return -EINVAL;
+
+ size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
+
+ if (next & DFHv1_PARAM_HDR_NEXT_EOL)
+ return size;
+ }
+
+ return -ENOENT;
+}
+
/*
* when create sub feature instances, for private features, it doesn't need
* to provide resource size and feature id as they could be read from DFH
@@ -1023,39 +1128,70 @@ static int
create_feature_instance(struct build_feature_devs_info *binfo,
resource_size_t ofst, resource_size_t size, u16 fid)
{
- unsigned int irq_base, nr_irqs;
struct dfl_feature_info *finfo;
+ int dfh_psize = 0;
u8 revision = 0;
+ u8 dfh_ver = 0;
int ret;
u64 v;
if (fid != FEATURE_ID_AFU) {
v = readq(binfo->ioaddr + ofst);
revision = FIELD_GET(DFH_REVISION, v);
-
+ dfh_ver = 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);
+ if (dfh_ver == 1) {
+ dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
+ if (dfh_psize < 0) {
+ dev_err(binfo->dev,
+ "failed to read size of DFHv1 parameters %d\n",
+ dfh_psize);
+ return dfh_psize;
+ }
+ dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
+ }
}
if (binfo->len - ofst < size)
return -EINVAL;
- ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
- if (ret)
- return ret;
-
- finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
+ finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
if (!finfo)
return -ENOMEM;
+ if (dfh_psize > 0) {
+ memcpy_fromio(finfo->params,
+ binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
+ finfo->param_size = dfh_psize;
+ }
+
finfo->fid = fid;
finfo->revision = revision;
- finfo->mmio_res.start = binfo->start + ofst;
- finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+ finfo->dfh_version = dfh_ver;
finfo->mmio_res.flags = IORESOURCE_MEM;
- finfo->irq_base = irq_base;
- finfo->nr_irqs = nr_irqs;
+ if (dfh_ver == 1) {
+ v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+ if (v & DFHv1_CSR_ADDR_REL)
+ finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
+ else
+ finfo->mmio_res.start = binfo->start + ofst +
+ FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+
+ v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+ finfo->mmio_res.end = finfo->mmio_res.start +
+ FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
+ } else {
+ finfo->mmio_res.start = binfo->start + ofst;
+ finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+ }
+
+ ret = parse_feature_irqs(binfo, ofst, finfo);
+ if (ret) {
+ kfree(finfo);
+ return ret;
+ }
list_add_tail(&finfo->node, &binfo->sub_features);
binfo->feature_num++;
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 45e6e1359a67..1ea7f40c1af6 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -273,11 +273,14 @@ struct dfl_feature_irq_ctx {
* @ops: ops of this sub feature.
* @ddev: ptr to the dfl device of this sub feature.
* @priv: priv data of this feature.
+ * @param_size: size of dfh parameters
+ * @params: point to memory copy of dfh parameters
*/
struct dfl_feature {
struct platform_device *dev;
u16 id;
u8 revision;
+ u8 dfh_version;
int resource_index;
void __iomem *ioaddr;
struct dfl_feature_irq_ctx *irq_ctx;
@@ -285,6 +288,8 @@ struct dfl_feature {
const struct dfl_feature_ops *ops;
struct dfl_device *ddev;
void *priv;
+ unsigned int param_size;
+ void *params;
};
#define FEATURE_DEV_ID_UNUSED (-1)
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index fea9e16d35b6..d00787e870b7 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -50,9 +50,12 @@ struct dfl_device {
u16 type;
u16 feature_id;
u8 revision;
+ u8 dfh_version;
struct resource mmio_res;
int *irqs;
unsigned int num_irqs;
+ unsigned int param_size;
+ void *params;
struct dfl_fpga_cdev *cdev;
const struct dfl_device_id *id_entry;
};
@@ -95,4 +98,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
module_driver(__dfl_driver, dfl_driver_register, \
dfl_driver_unregister)
+void *dfh_find_param(struct dfl_device *dfl_dev, int param);
#endif /* __LINUX_DFL_H */
--
2.25.1
On Thu, Oct 20, 2022 at 02:26:09PM -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add generic support for MSI-X interrupts for DFL devices.
>
> 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.
...
> +static void *find_param(void *base, resource_size_t max, int param)
Why base can't be u64 * to begin with?
> +{
> + int off = 0;
> + u64 v, next;
> +
> + while (off < max) {
Maybe you need a comment somewhere to tell that the caller guarantees that max
won't provoke OOB accesses.
> + v = *(u64 *)(base + off);
Okay, if offset is not multiple of at least 4, how do you guarantee no
exception on the architectures with disallowed misaligned accesses?
Making base to be u64 * solves this, but you need to take care to provide
offset in terms of u64 words.
> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> + return base + off + DFHv1_PARAM_DATA;
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> + break;
> +
> + }
> +
> + return NULL;
> +}
...
> + /*
> + * DFHv0 only provides mmio resource information for each feature
MMIO
> + * in the DFL header. There is no generic interrupt information.
> + * Instead, features with interrupt functionality provide
> + * the information in feature specific registers.
> + */
...
> + if (!finfo->param_size)
> break;
This is redundant as it's implied by find_param().
> + p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
> + if (!p)
> break;
...
> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> + int size = 0;
> + u64 v, next;
> +
> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> + readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> + return 0;
> +
> + while (size + DFHv1_PARAM_HDR < max) {
> + v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
> + return -EINVAL;
> +
> + size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +
> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> + return size;
These 3 looks like they deserve different fields and hence separate FIELD_GET()
will return exactly what we need without additional masking, right?
> + }
> +
> + return -ENOENT;
> +}
...
> + if (dfh_psize > 0) {
Isn't this implied by memcpy_fromio()? I mean if it's 0, nothing bad will
happen if you call the above directly.
> + memcpy_fromio(finfo->params,
> + binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> + finfo->param_size = dfh_psize;
> + }
...
> finfo->mmio_res.flags = IORESOURCE_MEM;
> + if (dfh_ver == 1) {
> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> + if (v & DFHv1_CSR_ADDR_REL)
> + finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> + else
> + finfo->mmio_res.start = binfo->start + ofst +
> + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> + finfo->mmio_res.end = finfo->mmio_res.start +
> + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> + } else {
> + finfo->mmio_res.start = binfo->start + ofst;
> + finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> + }
You may define
resource_size_t start, end;
locally and simplify above quite a bit.
...
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);
+ Blank line.
> #endif /* __LINUX_DFL_H */
--
With Best Regards,
Andy Shevchenko
On Thu, Oct 20, 2022 at 02:26:10PM -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
...
> +#include <linux/bitfield.h>
Where is device.h?
> +#include <linux/dfl.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
User?
> +#include <linux/kernel.h>
Try to use what is really needed, yet this one may be still needed for
something like ARRAY_SIZE().
> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
Missed types.h.
...
> + ret = dfl_uart_get_params(dfl_dev, &uart);
> +
Redundant blank line.
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed uart feature walk\n");
--
With Best Regards,
Andy Shevchenko
On Thu, Oct 20, 2022 at 02:26:07PM -0700, [email protected] wrote:
> +Device Feature Header - Version 0
> +===========================================
> +The format of Version 0 of a Device Feature Header (DFH) is shown below::
> +
> + +-----------------------------------------------------------------------+
> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> + +-----------------------------------------------------------------------+
> + |63 GUID_L 0| 0x08
> + +-----------------------------------------------------------------------+
> + |63 GUID_H 0| 0x10
> + +-----------------------------------------------------------------------+
> +
> +Offset 0x00
> +Type - The type of DFH (e.g. FME, AFU, or private feature).
> +DFH VER - The version of the DFH.
> +Rsvd - Currently unused.
> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +Next - The offset of the next DFH in the DFL from the start of the DFH.
> +If EOL is set, Next refers to size of mmio for last feature in the list.
> +ID - If Type field is 'private feature', then ID of the private feature.
> +
> +Offset 0x08
> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +Offset 0x10
> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +
> +Device Feature Header - Version 1
> +===========================================
> +The format of Version 1 of a Device Feature Header (DFH) is shown below::
> +
> + +-----------------------------------------------------------------------+
> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> + +-----------------------------------------------------------------------+
> + |63 GUID_L 0| 0x08
> + +-----------------------------------------------------------------------+
> + |63 GUID_H 0| 0x10
> + +-----------------------------------------------------------------------+
> + |63 Address/Offset 1| Rel 0| 0x18
> + +-----------------------------------------------------------------------+
> + |63 Reg Size 32|Params 31|30 Group 16|15 Instance 0| 0x20
> + +-----------------------------------------------------------------------+
> + |63 Next 34|RSV33|EOP32|31 Param Version 16|15 Param ID 0| 0x28
> + +-----------------------------------------------------------------------+
> + |63 Parameter Data 0| 0x30
> + +-----------------------------------------------------------------------+
> +
> + ...
> +
> + +-----------------------------------------------------------------------+
> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
> + +-----------------------------------------------------------------------+
> + |63 Parameter Data 0|
> + +-----------------------------------------------------------------------+
> +
> +Offset 0x00
> +Type - The type of DFH (e.g. FME, AFU, or private feature).
> +DFH VER - The version of the DFH.
> +Rsvd - Currently unused.
> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +Next - The offset of the next DFH in the DFL from the start of the DFH.
> +If EOL is set, Next refers to size of mmio for last feature in the list.
> +ID - If Type field is 'private feature', then ID of the private feature.
> +
> +Offset 0x08
> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
> +
> +Offset 0x10
> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +Offset 0x18
> +Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
> +absolute address for the location of the feature's registers.
> +If Rel bit is clear, then the feature's registers start at the
> +offset from the start of the DFH.
> +
> +Offset 0x20
> +Reg Size - Size of feature's register set.
> +Params - Set if DFH has one or more parameter blocks.
> +Group - Id of group if feature is part of a group.
> +Instance - Id of instance of feature within a group.
> +
> +Offset 0x28 if feature has parameters
> +Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
> +If EOP set, size of last parameter.
> +Param Version - Version of Param ID.
> +Param ID - ID of parameter.
> +
> +Offset 0x30
> +Parameter Data - Parameter data whose size and format is defined by version
> +and ID of the parameter.
> +
The offset fields list should be formatted with nested list (with
prose improv):
---- >8 ----
diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 12365be435a812..9c19ee62d4ac44 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -573,22 +573,27 @@ The format of Version 0 of a Device Feature Header (DFH) is shown below::
|63 GUID_H 0| 0x10
+-----------------------------------------------------------------------+
-Offset 0x00
-Type - The type of DFH (e.g. FME, AFU, or private feature).
-DFH VER - The version of the DFH.
-Rsvd - Currently unused.
-EOL - Set if this DFH is the end of the Device Feature List (DFL).
-Next - The offset of the next DFH in the DFL from the start of the DFH.
-If EOL is set, Next refers to size of mmio for last feature in the list.
-ID - If Type field is 'private feature', then ID of the private feature.
+The fields are:
-Offset 0x08
-GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
-if Type is FME or AFU.
+ * Offset 0x00
-Offset 0x10
-GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
-if Type is FME or AFU.
+ * Type - The type of DFH (e.g. FME, AFU, or private feature).
+ * DFH VER - The version of the DFH.
+ * Rsvd - Currently unused.
+ * EOL - Set if this DFH is the end of the Device Feature List (DFL).
+
+ * Next - The offset of the next DFH in the DFL from the start of the DFH.
+ If EOL is set, Next refers to size of mmio for last feature in the list.
+
+ * ID - Private feature ID if Type is private feature.
+
+ * Offset 0x08
+
+ * GUID_L - Least significant half of a 128-bit GUID if Type is FME or AFU.
+
+ * Offset 0x10
+
+ * GUID_H - Most significant half of a 128-bit GUID if Type if FME or AFU.
Device Feature Header - Version 1
@@ -619,43 +624,53 @@ The format of Version 1 of a Device Feature Header (DFH) is shown below::
|63 Parameter Data 0|
+-----------------------------------------------------------------------+
-Offset 0x00
-Type - The type of DFH (e.g. FME, AFU, or private feature).
-DFH VER - The version of the DFH.
-Rsvd - Currently unused.
-EOL - Set if this DFH is the end of the Device Feature List (DFL).
-Next - The offset of the next DFH in the DFL from the start of the DFH.
-If EOL is set, Next refers to size of mmio for last feature in the list.
-ID - If Type field is 'private feature', then ID of the private feature.
+The fields are:
-Offset 0x08
-GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
+ * Offset 0x00
-Offset 0x10
-GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
-if Type is FME or AFU.
+ * Type - The type of DFH (e.g. FME, AFU, or private feature).
+ * DFH VER - The version of the DFH.
+ * Rsvd - Currently unused.
+ * EOL - Set if this DFH is the end of the Device Feature List (DFL).
-Offset 0x18
-Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
-absolute address for the location of the feature's registers.
-If Rel bit is clear, then the feature's registers start at the
-offset from the start of the DFH.
+ * Next - The offset of the next DFH in the DFL from the start of the DFH.
+ If EOL is set, Next refers to size of mmio for last feature in the list.
-Offset 0x20
-Reg Size - Size of feature's register set.
-Params - Set if DFH has one or more parameter blocks.
-Group - Id of group if feature is part of a group.
-Instance - Id of instance of feature within a group.
+ * ID - Private feature ID if Type is private feature.
-Offset 0x28 if feature has parameters
-Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
-If EOP set, size of last parameter.
-Param Version - Version of Param ID.
-Param ID - ID of parameter.
+ * Offset 0x08
-Offset 0x30
-Parameter Data - Parameter data whose size and format is defined by version
-and ID of the parameter.
+ * GUID_L - Least significant half of a 128-bit GUID if Type is FME or APU.
+
+ * Offset 0x10
+
+ * GUID_H - Most significant half of a 128-bit GUID if Type is FME or AFU.
+
+ * Offset 0x18
+
+ * Address/Offset - If Rel bit is set, upper 63 bits of a 16-bit aligned
+ absolute address for the location of feature registers; otherwise
+ registers of the feature start at the offset from the start of the DFH.
+
+ * Offset 0x20
+
+ * Reg Size - Size of register set of the feature.
+ * Params - Set if DFH has one or more parameter blocks.
+ * Group - ID of group if the feature is part of a group.
+ * Instance - ID of instance of the feature within a group.
+
+ * Offset 0x28 (if the feature has parameters)
+
+ * Next - Upper 30 bits of a 32-bit aligned offset to the next parameter
+ block. If EOP is set, size of last parameter.
+
+ * Param Version - Version of Param ID.
+ * Param ID - ID of parameter.
+
+ * Offset 0x30 (if the feature has parameters)
+
+ * Parameter Data - Parameter data whose size and format is defined by
+ version and ID of the parameter.
Open discussion
===============
Thanks.
--
An old man doll... just what I always wanted! - Clara
On Thu, Oct 20, 2022 at 02:26:10PM -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]>
As per the rules of how Intel developers are supposed to send out
patches, I can't take this. Please work with the Intel kernel developer
team to get this properly reviewed first, and the correct reviews,
before sending it out again.
thanks,
greg k-h
On Thu, 20 Oct 2022, [email protected] wrote:
> 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]>
> ---
> v4: s/MSIX/MSI_X/g
> move kerneldoc to implementation
> don't change copyright date
>
> v3:
> keep DFHv1 definitions "hidden" in drivers/fpga/dfl.h
>
> v2: clean up whitespace and one line comments
> remove extra space in commit
> use uniform number of digits in constants
> don't change copyright date because of removed content
>
> dfl.h s/MSIX/MSI_X/g move kerneldoc
> ---
> drivers/fpga/dfl.h | 33 +++++++++++++++++++++++++++++++++
> include/linux/dfl.h | 11 +++++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 06cfcd5e84bb..45e6e1359a67 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -74,11 +74,44 @@
> #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 */
> #define DFH_TYPE_AFU 1
> #define DFH_TYPE_PRIVATE 3
> #define DFH_TYPE_FIU 4
>
> +/*
> + * 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 */
> +
> +/*
> + * 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 */
SIZE -> SZ would remove two letters w/o loss of info (remember to
rename the offset too if you make this change).
> +/* 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 */
> +#define DFHv1_PARAM_HDR_NEXT_EOL BIT_ULL(0)
> +#define DFHv1_PARAM_HDR_NEXT_MASK GENMASK_ULL(1, 0)
> +#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */
> +
> /* Next AFU Register Bitfield */
> #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
>
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 431636a0dc78..fea9e16d35b6 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -11,6 +11,17 @@
> #include <linux/device.h>
> #include <linux/mod_devicetable.h>
>
> +#define DFHv1_PARAM_ID_MSI_X 0x1
> +#define DFHv1_PARAM_MSI_X_STARTV 0x0
> +#define DFHv1_PARAM_MSI_X_NUMV 0x4
> +
> +#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)
Any particular reason why MSI_X parameters are given as offsets and
these REG_LAYOUT ones as bitfields (both are 32-bit)?
The naming here would indicate that DFHv1_PARAM_ID_REG_WIDTH is one of the
parameters but it's part of param data instead. I suppose you'd want
DFHv1_PARAM_REG_LAYOUT_WIDTH instead.
--
i.
On Fri, 21 Oct 2022, Ilpo J?rvinen wrote:
> On Thu, 20 Oct 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]>
> > ---
> > v4: Remove marketing speak and separate v0 and v1 descriptions.
> > Fix errors reported by "make htmldocs".
> >
> > v3: no change
> >
> > v2: s/GUILD/GUID/
> > add picture
> > ---
> > Documentation/fpga/dfl.rst | 96 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 96 insertions(+)
> >
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index 15b670926084..12365be435a8 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -561,6 +561,102 @@ new DFL feature via UIO direct access, its feature id should be added to the
> > driver's id_table.
> >
> >
> > +Device Feature Header - Version 0
> > +===========================================
> > +The format of Version 0 of a Device Feature Header (DFH) is shown below::
> > +
> > + +-----------------------------------------------------------------------+
> > + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> > + +-----------------------------------------------------------------------+
> > + |63 GUID_L 0| 0x08
> > + +-----------------------------------------------------------------------+
> > + |63 GUID_H 0| 0x10
> > + +-----------------------------------------------------------------------+
> > +
> > +Offset 0x00
> > +Type - The type of DFH (e.g. FME, AFU, or private feature).
> > +DFH VER - The version of the DFH.
> > +Rsvd - Currently unused.
> > +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> > +Next - The offset of the next DFH in the DFL from the start of the DFH.
> > +If EOL is set, Next refers to size of mmio for last feature in the list.
> > +ID - If Type field is 'private feature', then ID of the private feature.
> > +
> > +Offset 0x08
> > +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
> > +if Type is FME or AFU.
> > +
> > +Offset 0x10
> > +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> > +if Type is FME or AFU.
> > +
> > +
> > +Device Feature Header - Version 1
> > +===========================================
>
> While this is structurally better than the previous one. I'd still include
> at least one paragraph about the purpose. Something along these lines
> (picked from v3 + edited the marketing speak/v0 compare away from it):
>
> Version 1 of the Device Feature Header (DFHv1) provides flexibility and
> extensibility to hardware designs using Device Feature Lists. It is a
> standardized mechanism for features to describe parameters/capabilities to
> software.
>
> With DFHv1:
> * GUID is mandatory for all types
> * The register space of the feature is decoupled from the location of the DFH
> * A list of parameter values associates to a particular feature.
>
> After that, the header itself makes much more sense already.
>
> > +The format of Version 1 of a Device Feature Header (DFH) is shown below::
> > +
> > + +-----------------------------------------------------------------------+
> > + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> > + +-----------------------------------------------------------------------+
> > + |63 GUID_L 0| 0x08
> > + +-----------------------------------------------------------------------+
> > + |63 GUID_H 0| 0x10
> > + +-----------------------------------------------------------------------+
> > + |63 Address/Offset 1| Rel 0| 0x18
>
> Should this mention it's addr/offs of registers? As is it's bit hard to
> figure out from the diagram w/o the extra description. I think you have
> plenty of space for adding that extra bit of info.
>
> > + +-----------------------------------------------------------------------+
> > + |63 Reg Size 32|Params 31|30 Group 16|15 Instance 0| 0x20
> > + +-----------------------------------------------------------------------+
> > + |63 Next 34|RSV33|EOP32|31 Param Version 16|15 Param ID 0| 0x28
> > + +-----------------------------------------------------------------------+
> > + |63 Parameter Data 0| 0x30
> > + +-----------------------------------------------------------------------+
> > +
> > + ...
> > +
> > + +-----------------------------------------------------------------------+
> > + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
Copy-paste error on the next field.
--
i.
> > + +-----------------------------------------------------------------------+
> > + |63 Parameter Data 0|
> > + +-----------------------------------------------------------------------+
> > +
> > +Offset 0x00
> > +Type - The type of DFH (e.g. FME, AFU, or private feature).
> > +DFH VER - The version of the DFH.
> > +Rsvd - Currently unused.
> > +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> > +Next - The offset of the next DFH in the DFL from the start of the DFH.
> > +If EOL is set, Next refers to size of mmio for last feature in the list.
> > +ID - If Type field is 'private feature', then ID of the private feature.
> > +
> > +Offset 0x08
> > +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
> > +
> > +Offset 0x10
> > +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> > +if Type is FME or AFU.
>
> A copy-paste error?
>
> > +
> > +Offset 0x18
> > +Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
> > +absolute address for the location of the feature's registers.
> > +If Rel bit is clear, then the feature's registers start at the
> > +offset from the start of the DFH.
> > +
> > +Offset 0x20
> > +Reg Size - Size of feature's register set.
> > +Params - Set if DFH has one or more parameter blocks.
> > +Group - Id of group if feature is part of a group.
> > +Instance - Id of instance of feature within a group.
> > +
> > +Offset 0x28 if feature has parameters
> > +Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
> > +If EOP set, size of last parameter.
> > +Param Version - Version of Param ID.
> > +Param ID - ID of parameter.
> > +
> > +Offset 0x30
> > +Parameter Data - Parameter data whose size and format is defined by version
> > +and ID of the parameter.
>
> I'd reverse the order and say "ID and version" (kind of major thing
> first).
>
> > +
> > Open discussion
> > ===============
> > FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> >
>
>
On Thu, 20 Oct 2022, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add generic support for MSI-X interrupts for DFL devices.
>
> 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]>
> ---
> v4: s/MSIX/MSI_X
> move kernel doc to implementation
> use structure assignment
> fix decode of absolute address
> clean up comment in parse_feature_irqs
> remove use of csr_res
>
> v3: remove unneeded blank line
> use clearer variable name
> pass finfo into parse_feature_irqs()
> refactor code for better indentation
> use switch statement for irq parsing
> squash in code parsing register location
>
> v2: fix kernel doc
> clarify use of DFH_VERSION field
> ---
> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> + int size = 0;
> + u64 v, next;
> +
> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> + readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> + return 0;
> +
> + while (size + DFHv1_PARAM_HDR < max) {
> + v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
In general, try to not use inverse logic for defining masks. However here,
just change DFHv1_PARAM_HDR_NEXT_OFFSET to not include any extra bits
(no rsvd nor eop) and you no longer need this extra masking.
> + return -EINVAL;
> +
> + size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
...Then you can drop this anding too.
> +
> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
Your docs say EOP, but here you use EOL.
Change DFHv1_PARAM_HDR_NEXT_EOL such that this is extracted directly from
v.
--
i.
On Thu, 20 Oct 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]>
> ---
> v4: Remove marketing speak and separate v0 and v1 descriptions.
> Fix errors reported by "make htmldocs".
>
> v3: no change
>
> v2: s/GUILD/GUID/
> add picture
> ---
> Documentation/fpga/dfl.rst | 96 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 96 insertions(+)
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 15b670926084..12365be435a8 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -561,6 +561,102 @@ new DFL feature via UIO direct access, its feature id should be added to the
> driver's id_table.
>
>
> +Device Feature Header - Version 0
> +===========================================
> +The format of Version 0 of a Device Feature Header (DFH) is shown below::
> +
> + +-----------------------------------------------------------------------+
> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> + +-----------------------------------------------------------------------+
> + |63 GUID_L 0| 0x08
> + +-----------------------------------------------------------------------+
> + |63 GUID_H 0| 0x10
> + +-----------------------------------------------------------------------+
> +
> +Offset 0x00
> +Type - The type of DFH (e.g. FME, AFU, or private feature).
> +DFH VER - The version of the DFH.
> +Rsvd - Currently unused.
> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +Next - The offset of the next DFH in the DFL from the start of the DFH.
> +If EOL is set, Next refers to size of mmio for last feature in the list.
> +ID - If Type field is 'private feature', then ID of the private feature.
> +
> +Offset 0x08
> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +Offset 0x10
> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +
> +Device Feature Header - Version 1
> +===========================================
While this is structurally better than the previous one. I'd still include
at least one paragraph about the purpose. Something along these lines
(picked from v3 + edited the marketing speak/v0 compare away from it):
Version 1 of the Device Feature Header (DFHv1) provides flexibility and
extensibility to hardware designs using Device Feature Lists. It is a
standardized mechanism for features to describe parameters/capabilities to
software.
With DFHv1:
* GUID is mandatory for all types
* The register space of the feature is decoupled from the location of the DFH
* A list of parameter values associates to a particular feature.
After that, the header itself makes much more sense already.
> +The format of Version 1 of a Device Feature Header (DFH) is shown below::
> +
> + +-----------------------------------------------------------------------+
> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> + +-----------------------------------------------------------------------+
> + |63 GUID_L 0| 0x08
> + +-----------------------------------------------------------------------+
> + |63 GUID_H 0| 0x10
> + +-----------------------------------------------------------------------+
> + |63 Address/Offset 1| Rel 0| 0x18
Should this mention it's addr/offs of registers? As is it's bit hard to
figure out from the diagram w/o the extra description. I think you have
plenty of space for adding that extra bit of info.
> + +-----------------------------------------------------------------------+
> + |63 Reg Size 32|Params 31|30 Group 16|15 Instance 0| 0x20
> + +-----------------------------------------------------------------------+
> + |63 Next 34|RSV33|EOP32|31 Param Version 16|15 Param ID 0| 0x28
> + +-----------------------------------------------------------------------+
> + |63 Parameter Data 0| 0x30
> + +-----------------------------------------------------------------------+
> +
> + ...
> +
> + +-----------------------------------------------------------------------+
> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
> + +-----------------------------------------------------------------------+
> + |63 Parameter Data 0|
> + +-----------------------------------------------------------------------+
> +
> +Offset 0x00
> +Type - The type of DFH (e.g. FME, AFU, or private feature).
> +DFH VER - The version of the DFH.
> +Rsvd - Currently unused.
> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +Next - The offset of the next DFH in the DFL from the start of the DFH.
> +If EOL is set, Next refers to size of mmio for last feature in the list.
> +ID - If Type field is 'private feature', then ID of the private feature.
> +
> +Offset 0x08
> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
> +
> +Offset 0x10
> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
A copy-paste error?
> +
> +Offset 0x18
> +Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
> +absolute address for the location of the feature's registers.
> +If Rel bit is clear, then the feature's registers start at the
> +offset from the start of the DFH.
> +
> +Offset 0x20
> +Reg Size - Size of feature's register set.
> +Params - Set if DFH has one or more parameter blocks.
> +Group - Id of group if feature is part of a group.
> +Instance - Id of instance of feature within a group.
> +
> +Offset 0x28 if feature has parameters
> +Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
> +If EOP set, size of last parameter.
> +Param Version - Version of Param ID.
> +Param ID - ID of parameter.
> +
> +Offset 0x30
> +Parameter Data - Parameter data whose size and format is defined by version
> +and ID of the parameter.
I'd reverse the order and say "ID and version" (kind of major thing
first).
> +
> Open discussion
> ===============
> FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
>
--
i.
On Thu, 20 Oct 2022, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add generic support for MSI-X interrupts for DFL devices.
>
> 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]>
> ---
> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
> return 0;
> }
>
> +static void *find_param(void *base, resource_size_t max, int param)
> +{
> + int off = 0;
> + u64 v, next;
> +
> + while (off < max) {
> + v = *(u64 *)(base + off);
> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> + return base + off + DFHv1_PARAM_DATA;
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> + break;
> +
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find data for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to parameter data on success, NULL otherwise.
> + */
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
> +{
> + return find_param(dfl_dev->params, dfl_dev->param_size, param);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);
Do you expect this split between dfh_find_param() and find_param() to
be useful in the future? If no other callers are expected, I'd just pull
find_param() into dfh_find_param() and create local variables for base and
max.
--
i.
On Thu, 20 Oct 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]>
> ---
> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> + if (!p)
> + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
> +
> + v = *p;
> + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
Just use *p directly for the FIELD_GETs. I suppose you can drop v after
changing it.
Other than that and what Andy noted, this patch looks good.
--
i.
On Fri, 21 Oct 2022, Bagas Sanjaya wrote:
> On Thu, Oct 20, 2022 at 02:26:07PM -0700, [email protected] wrote:
>> +Device Feature Header - Version 0
>> +===========================================
>> +The format of Version 0 of a Device Feature Header (DFH) is shown below::
>> +
>> + +-----------------------------------------------------------------------+
>> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
>> + +-----------------------------------------------------------------------+
>> + |63 GUID_L 0| 0x08
>> + +-----------------------------------------------------------------------+
>> + |63 GUID_H 0| 0x10
>> + +-----------------------------------------------------------------------+
>> +
>> +Offset 0x00
>> +Type - The type of DFH (e.g. FME, AFU, or private feature).
>> +DFH VER - The version of the DFH.
>> +Rsvd - Currently unused.
>> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
>> +Next - The offset of the next DFH in the DFL from the start of the DFH.
>> +If EOL is set, Next refers to size of mmio for last feature in the list.
>> +ID - If Type field is 'private feature', then ID of the private feature.
>> +
>> +Offset 0x08
>> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
>> +if Type is FME or AFU.
>> +
>> +Offset 0x10
>> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
>> +if Type is FME or AFU.
>> +
>> +
>> +Device Feature Header - Version 1
>> +===========================================
>> +The format of Version 1 of a Device Feature Header (DFH) is shown below::
>> +
>> + +-----------------------------------------------------------------------+
>> + |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
>> + +-----------------------------------------------------------------------+
>> + |63 GUID_L 0| 0x08
>> + +-----------------------------------------------------------------------+
>> + |63 GUID_H 0| 0x10
>> + +-----------------------------------------------------------------------+
>> + |63 Address/Offset 1| Rel 0| 0x18
>> + +-----------------------------------------------------------------------+
>> + |63 Reg Size 32|Params 31|30 Group 16|15 Instance 0| 0x20
>> + +-----------------------------------------------------------------------+
>> + |63 Next 34|RSV33|EOP32|31 Param Version 16|15 Param ID 0| 0x28
>> + +-----------------------------------------------------------------------+
>> + |63 Parameter Data 0| 0x30
>> + +-----------------------------------------------------------------------+
>> +
>> + ...
>> +
>> + +-----------------------------------------------------------------------+
>> + |63 Next parameter offset 32|31 Param Version 16|15 Param ID 0|
>> + +-----------------------------------------------------------------------+
>> + |63 Parameter Data 0|
>> + +-----------------------------------------------------------------------+
>> +
>> +Offset 0x00
>> +Type - The type of DFH (e.g. FME, AFU, or private feature).
>> +DFH VER - The version of the DFH.
>> +Rsvd - Currently unused.
>> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
>> +Next - The offset of the next DFH in the DFL from the start of the DFH.
>> +If EOL is set, Next refers to size of mmio for last feature in the list.
>> +ID - If Type field is 'private feature', then ID of the private feature.
>> +
>> +Offset 0x08
>> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
>> +
>> +Offset 0x10
>> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
>> +if Type is FME or AFU.
>> +
>> +Offset 0x18
>> +Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
>> +absolute address for the location of the feature's registers.
>> +If Rel bit is clear, then the feature's registers start at the
>> +offset from the start of the DFH.
>> +
>> +Offset 0x20
>> +Reg Size - Size of feature's register set.
>> +Params - Set if DFH has one or more parameter blocks.
>> +Group - Id of group if feature is part of a group.
>> +Instance - Id of instance of feature within a group.
>> +
>> +Offset 0x28 if feature has parameters
>> +Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
>> +If EOP set, size of last parameter.
>> +Param Version - Version of Param ID.
>> +Param ID - ID of parameter.
>> +
>> +Offset 0x30
>> +Parameter Data - Parameter data whose size and format is defined by version
>> +and ID of the parameter.
>> +
>
> The offset fields list should be formatted with nested list (with
> prose improv):
>
> ---- >8 ----
Thanks for the good suggestion.
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 12365be435a812..9c19ee62d4ac44 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -573,22 +573,27 @@ The format of Version 0 of a Device Feature Header (DFH) is shown below::
> |63 GUID_H 0| 0x10
> +-----------------------------------------------------------------------+
>
> -Offset 0x00
> -Type - The type of DFH (e.g. FME, AFU, or private feature).
> -DFH VER - The version of the DFH.
> -Rsvd - Currently unused.
> -EOL - Set if this DFH is the end of the Device Feature List (DFL).
> -Next - The offset of the next DFH in the DFL from the start of the DFH.
> -If EOL is set, Next refers to size of mmio for last feature in the list.
> -ID - If Type field is 'private feature', then ID of the private feature.
> +The fields are:
>
> -Offset 0x08
> -GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
> -if Type is FME or AFU.
> + * Offset 0x00
>
> -Offset 0x10
> -GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> -if Type is FME or AFU.
> + * Type - The type of DFH (e.g. FME, AFU, or private feature).
> + * DFH VER - The version of the DFH.
> + * Rsvd - Currently unused.
> + * EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +
> + * Next - The offset of the next DFH in the DFL from the start of the DFH.
> + If EOL is set, Next refers to size of mmio for last feature in the list.
> +
> + * ID - Private feature ID if Type is private feature.
> +
> + * Offset 0x08
> +
> + * GUID_L - Least significant half of a 128-bit GUID if Type is FME or AFU.
> +
> + * Offset 0x10
> +
> + * GUID_H - Most significant half of a 128-bit GUID if Type if FME or AFU.
>
>
> Device Feature Header - Version 1
> @@ -619,43 +624,53 @@ The format of Version 1 of a Device Feature Header (DFH) is shown below::
> |63 Parameter Data 0|
> +-----------------------------------------------------------------------+
>
> -Offset 0x00
> -Type - The type of DFH (e.g. FME, AFU, or private feature).
> -DFH VER - The version of the DFH.
> -Rsvd - Currently unused.
> -EOL - Set if this DFH is the end of the Device Feature List (DFL).
> -Next - The offset of the next DFH in the DFL from the start of the DFH.
> -If EOL is set, Next refers to size of mmio for last feature in the list.
> -ID - If Type field is 'private feature', then ID of the private feature.
> +The fields are:
>
> -Offset 0x08
> -GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
> + * Offset 0x00
>
> -Offset 0x10
> -GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> -if Type is FME or AFU.
> + * Type - The type of DFH (e.g. FME, AFU, or private feature).
> + * DFH VER - The version of the DFH.
> + * Rsvd - Currently unused.
> + * EOL - Set if this DFH is the end of the Device Feature List (DFL).
>
> -Offset 0x18
> -Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
> -absolute address for the location of the feature's registers.
> -If Rel bit is clear, then the feature's registers start at the
> -offset from the start of the DFH.
> + * Next - The offset of the next DFH in the DFL from the start of the DFH.
> + If EOL is set, Next refers to size of mmio for last feature in the list.
>
> -Offset 0x20
> -Reg Size - Size of feature's register set.
> -Params - Set if DFH has one or more parameter blocks.
> -Group - Id of group if feature is part of a group.
> -Instance - Id of instance of feature within a group.
> + * ID - Private feature ID if Type is private feature.
>
> -Offset 0x28 if feature has parameters
> -Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
> -If EOP set, size of last parameter.
> -Param Version - Version of Param ID.
> -Param ID - ID of parameter.
> + * Offset 0x08
>
> -Offset 0x30
> -Parameter Data - Parameter data whose size and format is defined by version
> -and ID of the parameter.
> + * GUID_L - Least significant half of a 128-bit GUID if Type is FME or APU.
> +
> + * Offset 0x10
> +
> + * GUID_H - Most significant half of a 128-bit GUID if Type is FME or AFU.
> +
> + * Offset 0x18
> +
> + * Address/Offset - If Rel bit is set, upper 63 bits of a 16-bit aligned
> + absolute address for the location of feature registers; otherwise
> + registers of the feature start at the offset from the start of the DFH.
> +
> + * Offset 0x20
> +
> + * Reg Size - Size of register set of the feature.
> + * Params - Set if DFH has one or more parameter blocks.
> + * Group - ID of group if the feature is part of a group.
> + * Instance - ID of instance of the feature within a group.
> +
> + * Offset 0x28 (if the feature has parameters)
> +
> + * Next - Upper 30 bits of a 32-bit aligned offset to the next parameter
> + block. If EOP is set, size of last parameter.
> +
> + * Param Version - Version of Param ID.
> + * Param ID - ID of parameter.
> +
> + * Offset 0x30 (if the feature has parameters)
> +
> + * Parameter Data - Parameter data whose size and format is defined by
> + version and ID of the parameter.
>
> Open discussion
> ===============
>
> Thanks.
>
> --
> An old man doll... just what I always wanted! - Clara
>
On Fri, 21 Oct 2022, Andy Shevchenko wrote:
> On Thu, Oct 20, 2022 at 02:26:09PM -0700, [email protected] wrote:
>> From: Matthew Gerlach <[email protected]>
>>
>> Add generic support for MSI-X interrupts for DFL devices.
>>
>> 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.
>
> ...
>
>> +static void *find_param(void *base, resource_size_t max, int param)
>
> Why base can't be u64 * to begin with?
It can be u64, and I will consider it for the next iteration.
>
>> +{
>> + int off = 0;
>> + u64 v, next;
>> +
>> + while (off < max) {
>
> Maybe you need a comment somewhere to tell that the caller guarantees that max
> won't provoke OOB accesses.
>
>> + v = *(u64 *)(base + off);
>
> Okay, if offset is not multiple of at least 4, how do you guarantee no
> exception on the architectures with disallowed misaligned accesses?
>
> Making base to be u64 * solves this, but you need to take care to provide
> offset in terms of u64 words.
The masking of next below ensures that the offset it at least 4 byte
aligned, but it might make sense to define the next field in terms of 8
byte words.
>
>> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> + return base + off + DFHv1_PARAM_DATA;
>> +
>> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> + off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> + break;
>> +
>> + }
>> +
>> + return NULL;
>> +}
>
> ...
>
>> + /*
>> + * DFHv0 only provides mmio resource information for each feature
>
> MMIO
I'll change mmio to MMIO here and a place in the documentation that I
noticed.
>
>> + * in the DFL header. There is no generic interrupt information.
>> + * Instead, features with interrupt functionality provide
>> + * the information in feature specific registers.
>> + */
>
> ...
>
>> + if (!finfo->param_size)
>> break;
>
> This is redundant as it's implied by find_param().
I will remove the redundant code.
>
>> + p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
>> + if (!p)
>> break;
>
> ...
>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> + int size = 0;
>> + u64 v, next;
>> +
>> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> + readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> + return 0;
>> +
>> + while (size + DFHv1_PARAM_HDR < max) {
>> + v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> + if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>> + return -EINVAL;
>> +
>> + size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +
>> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> + return size;
>
> These 3 looks like they deserve different fields and hence separate FIELD_GET()
> will return exactly what we need without additional masking, right?
I agree separate FIELD_GET() calls will be cleaner.
>
>> + }
>> +
>> + return -ENOENT;
>> +}
>
> ...
>
>> + if (dfh_psize > 0) {
>
> Isn't this implied by memcpy_fromio()? I mean if it's 0, nothing bad will
> happen if you call the above directly.
>
>> + memcpy_fromio(finfo->params,
>> + binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
>> + finfo->param_size = dfh_psize;
>> + }
>
> ...
>
>> finfo->mmio_res.flags = IORESOURCE_MEM;
>> + if (dfh_ver == 1) {
>> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> + if (v & DFHv1_CSR_ADDR_REL)
>> + finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>> + else
>> + finfo->mmio_res.start = binfo->start + ofst +
>> + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> + finfo->mmio_res.end = finfo->mmio_res.start +
>> + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>> + } else {
>> + finfo->mmio_res.start = binfo->start + ofst;
>> + finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> + }
>
> You may define
>
> resource_size_t start, end;
>
> locally and simplify above quite a bit.
That is a good suggestion that should clean up the code quite a bit.
>
> ...
>
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);
>
> + Blank line.
>
>> #endif /* __LINUX_DFL_H */
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
On Fri, 21 Oct 2022, Ilpo J?rvinen wrote:
> On Thu, 20 Oct 2022, [email protected] wrote:
>
>> From: Matthew Gerlach <[email protected]>
>>
>> Add generic support for MSI-X interrupts for DFL devices.
>>
>> 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]>
>> ---
>> v4: s/MSIX/MSI_X
>> move kernel doc to implementation
>> use structure assignment
>> fix decode of absolute address
>> clean up comment in parse_feature_irqs
>> remove use of csr_res
>>
>> v3: remove unneeded blank line
>> use clearer variable name
>> pass finfo into parse_feature_irqs()
>> refactor code for better indentation
>> use switch statement for irq parsing
>> squash in code parsing register location
>>
>> v2: fix kernel doc
>> clarify use of DFH_VERSION field
>> ---
>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> + int size = 0;
>> + u64 v, next;
>> +
>> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> + readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> + return 0;
>> +
>> + while (size + DFHv1_PARAM_HDR < max) {
>> + v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> + if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>
> In general, try to not use inverse logic for defining masks. However here,
> just change DFHv1_PARAM_HDR_NEXT_OFFSET to not include any extra bits
> (no rsvd nor eop) and you no longer need this extra masking.
I agree that defining the fields better and using FIELD_GET would make
this code cleaner.
>
>> + return -EINVAL;
>> +
>> + size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>
> ...Then you can drop this anding too.
>
>> +
>> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>
> Your docs say EOP, but here you use EOL.
Thanks for catching the inconsistency.
>
> Change DFHv1_PARAM_HDR_NEXT_EOL such that this is extracted directly from
> v.
>
> --
> i.
>
On Fri, 21 Oct 2022, Ilpo J?rvinen wrote:
> On Thu, 20 Oct 2022, [email protected] wrote:
>
>> 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]>
>> ---
>> v4: s/MSIX/MSI_X/g
>> move kerneldoc to implementation
>> don't change copyright date
>>
>> v3:
>> keep DFHv1 definitions "hidden" in drivers/fpga/dfl.h
>>
>> v2: clean up whitespace and one line comments
>> remove extra space in commit
>> use uniform number of digits in constants
>> don't change copyright date because of removed content
>>
>> dfl.h s/MSIX/MSI_X/g move kerneldoc
>> ---
>> drivers/fpga/dfl.h | 33 +++++++++++++++++++++++++++++++++
>> include/linux/dfl.h | 11 +++++++++++
>> 2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 06cfcd5e84bb..45e6e1359a67 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -74,11 +74,44 @@
>> #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 */
>> #define DFH_TYPE_AFU 1
>> #define DFH_TYPE_PRIVATE 3
>> #define DFH_TYPE_FIU 4
>>
>> +/*
>> + * 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 */
>> +
>> +/*
>> + * 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 */
>
> SIZE -> SZ would remove two letters w/o loss of info (remember to
> rename the offset too if you make this change).
>
>> +/* 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 */
>> +#define DFHv1_PARAM_HDR_NEXT_EOL BIT_ULL(0)
>> +#define DFHv1_PARAM_HDR_NEXT_MASK GENMASK_ULL(1, 0)
>> +#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */
>> +
>> /* Next AFU Register Bitfield */
>> #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
>>
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 431636a0dc78..fea9e16d35b6 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -11,6 +11,17 @@
>> #include <linux/device.h>
>> #include <linux/mod_devicetable.h>
>>
>> +#define DFHv1_PARAM_ID_MSI_X 0x1
>> +#define DFHv1_PARAM_MSI_X_STARTV 0x0
>> +#define DFHv1_PARAM_MSI_X_NUMV 0x4
>> +
>> +#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)
>
> Any particular reason why MSI_X parameters are given as offsets and
> these REG_LAYOUT ones as bitfields (both are 32-bit)?
I agree that it would be much better to be consistent.
>
> The naming here would indicate that DFHv1_PARAM_ID_REG_WIDTH is one of the
> parameters but it's part of param data instead. I suppose you'd want
> DFHv1_PARAM_REG_LAYOUT_WIDTH instead.
Thanks for the naming suggestions.
>
> --
> i.
>
>
On 2022-10-20 at 14:26:09 -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add generic support for MSI-X interrupts for DFL devices.
The first paragraph of the commit message should be the summary of the
whole change. But this seems one of the changes. Please add the proper
summary at the beginning.
>
> 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]>
> ---
> v4: s/MSIX/MSI_X
> move kernel doc to implementation
> use structure assignment
> fix decode of absolute address
> clean up comment in parse_feature_irqs
> remove use of csr_res
>
> v3: remove unneeded blank line
> use clearer variable name
> pass finfo into parse_feature_irqs()
> refactor code for better indentation
> use switch statement for irq parsing
> squash in code parsing register location
>
> v2: fix kernel doc
> clarify use of DFH_VERSION field
> ---
> drivers/fpga/dfl.c | 234 ++++++++++++++++++++++++++++++++++----------
> drivers/fpga/dfl.h | 5 +
> include/linux/dfl.h | 4 +
> 3 files changed, 194 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b9aae85ba930..37f995e66436 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
> if (ddev->mmio_res.parent)
> release_resource(&ddev->mmio_res);
>
> + kfree(ddev->params);
> +
> ida_free(&dfl_device_ida, ddev->id);
> kfree(ddev->irqs);
> kfree(ddev);
> @@ -380,7 +382,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
> ddev->type = feature_dev_id_type(pdev);
> ddev->feature_id = feature->id;
> ddev->revision = feature->revision;
> + ddev->dfh_version = feature->dfh_version;
> ddev->cdev = pdata->dfl_cdev;
> + if (feature->param_size) {
> + ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
> + if (!ddev->params) {
> + ret = -ENOMEM;
> + goto put_dev;
> + }
> + ddev->param_size = feature->param_size;
> + }
>
> /* add mmio resource */
> parent_res = &pdev->resource[feature->resource_index];
> @@ -708,20 +719,27 @@ 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.
> * @irq_base: start of irq index in this sub feature.
> * @nr_irqs: number of irqs of this sub feature.
> + * @param_size: size DFH parameters.
> + * @params: DFH parameter data.
> */
> struct dfl_feature_info {
> u16 fid;
> u8 revision;
> + u8 dfh_version;
> struct resource mmio_res;
> void __iomem *ioaddr;
> struct list_head node;
> unsigned int irq_base;
> unsigned int nr_irqs;
> + unsigned int param_size;
> + u64 params[];
> };
>
> static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
> @@ -797,7 +815,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> feature->dev = fdev;
> feature->id = finfo->fid;
> feature->revision = finfo->revision;
> + feature->dfh_version = finfo->dfh_version;
>
> + if (finfo->param_size) {
> + feature->params = devm_kmemdup(binfo->dev,
> + finfo->params, finfo->param_size,
> + GFP_KERNEL);
> + if (!feature->params)
> + return -ENOMEM;
> +
> + feature->param_size = finfo->param_size;
> + }
> /*
> * the FIU header feature has some fundamental functions (sriov
> * set, port enable/disable) needed for the dfl bus device and
> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
> return 0;
> }
>
> +static void *find_param(void *base, resource_size_t max, int param)
> +{
> + int off = 0;
> + u64 v, next;
> +
> + while (off < max) {
> + v = *(u64 *)(base + off);
> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> + return base + off + DFHv1_PARAM_DATA;
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> + break;
> +
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find data for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to parameter data on success, NULL otherwise.
> + */
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
> +{
> + return find_param(dfl_dev->params, dfl_dev->param_size, param);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);
> +
> static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> - resource_size_t ofst, u16 fid,
> - unsigned int *irq_base, unsigned int *nr_irqs)
> + resource_size_t ofst, struct dfl_feature_info *finfo)
> {
> void __iomem *base = binfo->ioaddr + ofst;
> unsigned int i, ibase, inr = 0;
> + void *params = finfo->params;
> enum dfl_id_type type;
> + u16 fid = finfo->fid;
> int virq;
> + u32 *p;
> u64 v;
>
> - type = feature_dev_id_type(binfo->feature_dev);
> + switch (finfo->dfh_version) {
> + case 0:
> + /*
> + * DFHv0 only provides mmio resource information for each feature
> + * in the DFL header. There is no generic interrupt information.
> + * Instead, features with interrupt functionality provide
> + * the information in feature specific registers.
> + */
> + type = feature_dev_id_type(binfo->feature_dev);
> + if (type == PORT_ID) {
> + switch (fid) {
> + case PORT_FEATURE_ID_UINT:
> + v = readq(base + PORT_UINT_CAP);
> + ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> + inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> + break;
> + case PORT_FEATURE_ID_ERROR:
> + v = readq(base + PORT_ERROR_CAP);
> + ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> + inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> + break;
> + }
> + } else if (type == FME_ID) {
> + switch (fid) {
> + case FME_FEATURE_ID_GLOBAL_ERR:
> + v = readq(base + FME_ERROR_CAP);
> + ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> + inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> + break;
> + }
> + }
> + break;
>
> - /*
> - * Ideally DFL framework should only read info from DFL header, but
> - * current version DFL 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
> - * DFL, the driver may still fall back to these quirks.
> - */
> - if (type == PORT_ID) {
> - switch (fid) {
> - case PORT_FEATURE_ID_UINT:
> - v = readq(base + PORT_UINT_CAP);
> - ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> - inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> + case 1:
> + /*
> + * DFHv1 provides interrupt resource information in DFHv1
> + * parameter blocks.
> + */
> + if (!finfo->param_size)
> break;
> - case PORT_FEATURE_ID_ERROR:
> - v = readq(base + PORT_ERROR_CAP);
> - ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> - inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +
> + p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
> + if (!p)
> break;
> - }
> - } else if (type == FME_ID) {
> - if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> - v = readq(base + FME_ERROR_CAP);
> - ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> - inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> - }
> +
> + ibase = *p++;
> + inr = *p;
> + break;
> +
> + default:
> + dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
> + break;
> }
>
> if (!inr) {
> - *irq_base = 0;
> - *nr_irqs = 0;
> + finfo->irq_base = 0;
> + finfo->nr_irqs = 0;
> return 0;
> }
>
> @@ -1006,12 +1086,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> }
> }
>
> - *irq_base = ibase;
> - *nr_irqs = inr;
> + finfo->irq_base = ibase;
> + finfo->nr_irqs = inr;
>
> return 0;
> }
>
> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> + int size = 0;
> + u64 v, next;
> +
> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> + readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> + return 0;
> +
> + while (size + DFHv1_PARAM_HDR < max) {
> + v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
> + return -EINVAL;
> +
> + size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +
> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> + return size;
> + }
> +
> + return -ENOENT;
> +}
> +
> /*
> * when create sub feature instances, for private features, it doesn't need
> * to provide resource size and feature id as they could be read from DFH
> @@ -1023,39 +1128,70 @@ static int
> create_feature_instance(struct build_feature_devs_info *binfo,
> resource_size_t ofst, resource_size_t size, u16 fid)
> {
> - unsigned int irq_base, nr_irqs;
> struct dfl_feature_info *finfo;
> + int dfh_psize = 0;
> u8 revision = 0;
> + u8 dfh_ver = 0;
> int ret;
> u64 v;
>
> if (fid != FEATURE_ID_AFU) {
> v = readq(binfo->ioaddr + ofst);
> revision = FIELD_GET(DFH_REVISION, v);
> -
> + dfh_ver = 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);
> + if (dfh_ver == 1) {
> + dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
> + if (dfh_psize < 0) {
> + dev_err(binfo->dev,
> + "failed to read size of DFHv1 parameters %d\n",
> + dfh_psize);
> + return dfh_psize;
> + }
> + dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
> + }
> }
>
> if (binfo->len - ofst < size)
> return -EINVAL;
>
> - ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> - if (ret)
> - return ret;
> -
> - finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
> + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
The u64 flexible array in the structure, but seems dfh_get_psize could
not garantee 64bit aligned size.
What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
u32[].
Thanks,
Yilun
> if (!finfo)
> return -ENOMEM;
>
> + if (dfh_psize > 0) {
> + memcpy_fromio(finfo->params,
> + binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> + finfo->param_size = dfh_psize;
> + }
> +
> finfo->fid = fid;
> finfo->revision = revision;
> - finfo->mmio_res.start = binfo->start + ofst;
> - finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> + finfo->dfh_version = dfh_ver;
> finfo->mmio_res.flags = IORESOURCE_MEM;
> - finfo->irq_base = irq_base;
> - finfo->nr_irqs = nr_irqs;
> + if (dfh_ver == 1) {
> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> + if (v & DFHv1_CSR_ADDR_REL)
> + finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> + else
> + finfo->mmio_res.start = binfo->start + ofst +
> + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> + finfo->mmio_res.end = finfo->mmio_res.start +
> + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
So for dflv1, no feature header resource for dfl_device, is it a problem
for dfl_uio? Does userspace driver need the raw feature header?
> + } else {
> + finfo->mmio_res.start = binfo->start + ofst;
> + finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> + }
> +
> + ret = parse_feature_irqs(binfo, ofst, finfo);
> + if (ret) {
> + kfree(finfo);
> + return ret;
> + }
>
> list_add_tail(&finfo->node, &binfo->sub_features);
> binfo->feature_num++;
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 45e6e1359a67..1ea7f40c1af6 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -273,11 +273,14 @@ struct dfl_feature_irq_ctx {
> * @ops: ops of this sub feature.
> * @ddev: ptr to the dfl device of this sub feature.
> * @priv: priv data of this feature.
> + * @param_size: size of dfh parameters
> + * @params: point to memory copy of dfh parameters
> */
> struct dfl_feature {
> struct platform_device *dev;
> u16 id;
> u8 revision;
> + u8 dfh_version;
> int resource_index;
> void __iomem *ioaddr;
> struct dfl_feature_irq_ctx *irq_ctx;
> @@ -285,6 +288,8 @@ struct dfl_feature {
> const struct dfl_feature_ops *ops;
> struct dfl_device *ddev;
> void *priv;
> + unsigned int param_size;
> + void *params;
> };
>
> #define FEATURE_DEV_ID_UNUSED (-1)
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index fea9e16d35b6..d00787e870b7 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -50,9 +50,12 @@ struct dfl_device {
> u16 type;
> u16 feature_id;
> u8 revision;
> + u8 dfh_version;
> struct resource mmio_res;
> int *irqs;
> unsigned int num_irqs;
> + unsigned int param_size;
> + void *params;
> struct dfl_fpga_cdev *cdev;
> const struct dfl_device_id *id_entry;
> };
> @@ -95,4 +98,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> module_driver(__dfl_driver, dfl_driver_register, \
> dfl_driver_unregister)
>
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);
int param_id is better?
Thanks,
Yilun
> #endif /* __LINUX_DFL_H */
> --
> 2.25.1
>
On Sat, 29 Oct 2022, Xu Yilun wrote:
> On 2022-10-20 at 14:26:09 -0700, [email protected] wrote:
>> From: Matthew Gerlach <[email protected]>
>>
>> Add generic support for MSI-X interrupts for DFL devices.
>
> The first paragraph of the commit message should be the summary of the
> whole change. But this seems one of the changes. Please add the proper
> summary at the beginning.
This is a very good suggestion. It will be addressed in the next
revision.
>
>>
>> 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]>
>> ---
>> v4: s/MSIX/MSI_X
>> move kernel doc to implementation
>> use structure assignment
>> fix decode of absolute address
>> clean up comment in parse_feature_irqs
>> remove use of csr_res
>>
>> v3: remove unneeded blank line
>> use clearer variable name
>> pass finfo into parse_feature_irqs()
>> refactor code for better indentation
>> use switch statement for irq parsing
>> squash in code parsing register location
>>
>> v2: fix kernel doc
>> clarify use of DFH_VERSION field
>> ---
>> drivers/fpga/dfl.c | 234 ++++++++++++++++++++++++++++++++++----------
>> drivers/fpga/dfl.h | 5 +
>> include/linux/dfl.h | 4 +
>> 3 files changed, 194 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index b9aae85ba930..37f995e66436 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
>> if (ddev->mmio_res.parent)
>> release_resource(&ddev->mmio_res);
>>
>> + kfree(ddev->params);
>> +
>> ida_free(&dfl_device_ida, ddev->id);
>> kfree(ddev->irqs);
>> kfree(ddev);
>> @@ -380,7 +382,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>> ddev->type = feature_dev_id_type(pdev);
>> ddev->feature_id = feature->id;
>> ddev->revision = feature->revision;
>> + ddev->dfh_version = feature->dfh_version;
>> ddev->cdev = pdata->dfl_cdev;
>> + if (feature->param_size) {
>> + ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
>> + if (!ddev->params) {
>> + ret = -ENOMEM;
>> + goto put_dev;
>> + }
>> + ddev->param_size = feature->param_size;
>> + }
>>
>> /* add mmio resource */
>> parent_res = &pdev->resource[feature->resource_index];
>> @@ -708,20 +719,27 @@ 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.
>> * @irq_base: start of irq index in this sub feature.
>> * @nr_irqs: number of irqs of this sub feature.
>> + * @param_size: size DFH parameters.
>> + * @params: DFH parameter data.
>> */
>> struct dfl_feature_info {
>> u16 fid;
>> u8 revision;
>> + u8 dfh_version;
>> struct resource mmio_res;
>> void __iomem *ioaddr;
>> struct list_head node;
>> unsigned int irq_base;
>> unsigned int nr_irqs;
>> + unsigned int param_size;
>> + u64 params[];
>> };
>>
>> static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
>> @@ -797,7 +815,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>> feature->dev = fdev;
>> feature->id = finfo->fid;
>> feature->revision = finfo->revision;
>> + feature->dfh_version = finfo->dfh_version;
>>
>> + if (finfo->param_size) {
>> + feature->params = devm_kmemdup(binfo->dev,
>> + finfo->params, finfo->param_size,
>> + GFP_KERNEL);
>> + if (!feature->params)
>> + return -ENOMEM;
>> +
>> + feature->param_size = finfo->param_size;
>> + }
>> /*
>> * the FIU header feature has some fundamental functions (sriov
>> * set, port enable/disable) needed for the dfl bus device and
>> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
>> return 0;
>> }
>>
>> +static void *find_param(void *base, resource_size_t max, int param)
>> +{
>> + int off = 0;
>> + u64 v, next;
>> +
>> + while (off < max) {
>> + v = *(u64 *)(base + off);
>> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> + return base + off + DFHv1_PARAM_DATA;
>> +
>> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> + off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> + break;
>> +
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +/**
>> + * dfh_find_param() - find data for the given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + *
>> + * Return: pointer to parameter data on success, NULL otherwise.
>> + */
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
>> +{
>> + return find_param(dfl_dev->params, dfl_dev->param_size, param);
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>> +
>> static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> - resource_size_t ofst, u16 fid,
>> - unsigned int *irq_base, unsigned int *nr_irqs)
>> + resource_size_t ofst, struct dfl_feature_info *finfo)
>> {
>> void __iomem *base = binfo->ioaddr + ofst;
>> unsigned int i, ibase, inr = 0;
>> + void *params = finfo->params;
>> enum dfl_id_type type;
>> + u16 fid = finfo->fid;
>> int virq;
>> + u32 *p;
>> u64 v;
>>
>> - type = feature_dev_id_type(binfo->feature_dev);
>> + switch (finfo->dfh_version) {
>> + case 0:
>> + /*
>> + * DFHv0 only provides mmio resource information for each feature
>> + * in the DFL header. There is no generic interrupt information.
>> + * Instead, features with interrupt functionality provide
>> + * the information in feature specific registers.
>> + */
>> + type = feature_dev_id_type(binfo->feature_dev);
>> + if (type == PORT_ID) {
>> + switch (fid) {
>> + case PORT_FEATURE_ID_UINT:
>> + v = readq(base + PORT_UINT_CAP);
>> + ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> + inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> + break;
>> + case PORT_FEATURE_ID_ERROR:
>> + v = readq(base + PORT_ERROR_CAP);
>> + ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> + inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> + break;
>> + }
>> + } else if (type == FME_ID) {
>> + switch (fid) {
>> + case FME_FEATURE_ID_GLOBAL_ERR:
>> + v = readq(base + FME_ERROR_CAP);
>> + ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> + inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> + break;
>> + }
>> + }
>> + break;
>>
>> - /*
>> - * Ideally DFL framework should only read info from DFL header, but
>> - * current version DFL 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
>> - * DFL, the driver may still fall back to these quirks.
>> - */
>> - if (type == PORT_ID) {
>> - switch (fid) {
>> - case PORT_FEATURE_ID_UINT:
>> - v = readq(base + PORT_UINT_CAP);
>> - ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> - inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> + case 1:
>> + /*
>> + * DFHv1 provides interrupt resource information in DFHv1
>> + * parameter blocks.
>> + */
>> + if (!finfo->param_size)
>> break;
>> - case PORT_FEATURE_ID_ERROR:
>> - v = readq(base + PORT_ERROR_CAP);
>> - ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> - inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +
>> + p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
>> + if (!p)
>> break;
>> - }
>> - } else if (type == FME_ID) {
>> - if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> - v = readq(base + FME_ERROR_CAP);
>> - ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> - inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> - }
>> +
>> + ibase = *p++;
>> + inr = *p;
>> + break;
>> +
>> + default:
>> + dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
>> + break;
>> }
>>
>> if (!inr) {
>> - *irq_base = 0;
>> - *nr_irqs = 0;
>> + finfo->irq_base = 0;
>> + finfo->nr_irqs = 0;
>> return 0;
>> }
>>
>> @@ -1006,12 +1086,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> }
>> }
>>
>> - *irq_base = ibase;
>> - *nr_irqs = inr;
>> + finfo->irq_base = ibase;
>> + finfo->nr_irqs = inr;
>>
>> return 0;
>> }
>>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> + int size = 0;
>> + u64 v, next;
>> +
>> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> + readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> + return 0;
>> +
>> + while (size + DFHv1_PARAM_HDR < max) {
>> + v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> + if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>> + return -EINVAL;
>> +
>> + size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +
>> + if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> + return size;
>> + }
>> +
>> + return -ENOENT;
>> +}
>> +
>> /*
>> * when create sub feature instances, for private features, it doesn't need
>> * to provide resource size and feature id as they could be read from DFH
>> @@ -1023,39 +1128,70 @@ static int
>> create_feature_instance(struct build_feature_devs_info *binfo,
>> resource_size_t ofst, resource_size_t size, u16 fid)
>> {
>> - unsigned int irq_base, nr_irqs;
>> struct dfl_feature_info *finfo;
>> + int dfh_psize = 0;
>> u8 revision = 0;
>> + u8 dfh_ver = 0;
>> int ret;
>> u64 v;
>>
>> if (fid != FEATURE_ID_AFU) {
>> v = readq(binfo->ioaddr + ofst);
>> revision = FIELD_GET(DFH_REVISION, v);
>> -
>> + dfh_ver = 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);
>> + if (dfh_ver == 1) {
>> + dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
>> + if (dfh_psize < 0) {
>> + dev_err(binfo->dev,
>> + "failed to read size of DFHv1 parameters %d\n",
>> + dfh_psize);
>> + return dfh_psize;
>> + }
>> + dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
>> + }
>> }
>>
>> if (binfo->len - ofst < size)
>> return -EINVAL;
>>
>> - ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
>> - if (ret)
>> - return ret;
>> -
>> - finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>> + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>
> The u64 flexible array in the structure, but seems dfh_get_psize could
> not garantee 64bit aligned size.
>
> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> u32[].
>
> Thanks,
> Yilun
The mandatory alignment alignment is 64. The documentation and field
definitions will be updated accordingly in the next revision.
>
>> if (!finfo)
>> return -ENOMEM;
>>
>> + if (dfh_psize > 0) {
>> + memcpy_fromio(finfo->params,
>> + binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
>> + finfo->param_size = dfh_psize;
>> + }
>> +
>> finfo->fid = fid;
>> finfo->revision = revision;
>> - finfo->mmio_res.start = binfo->start + ofst;
>> - finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> + finfo->dfh_version = dfh_ver;
>> finfo->mmio_res.flags = IORESOURCE_MEM;
>> - finfo->irq_base = irq_base;
>> - finfo->nr_irqs = nr_irqs;
>> + if (dfh_ver == 1) {
>> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> + if (v & DFHv1_CSR_ADDR_REL)
>> + finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>> + else
>> + finfo->mmio_res.start = binfo->start + ofst +
>> + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> + finfo->mmio_res.end = finfo->mmio_res.start +
>> + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>
> So for dflv1, no feature header resource for dfl_device, is it a problem
> for dfl_uio? Does userspace driver need the raw feature header?
These are two very good questions. The dfl_uio driver question is
particularly relevent because user space is looking at the GUIDs.
>
>> + } else {
>> + finfo->mmio_res.start = binfo->start + ofst;
>> + finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> + }
>> +
>> + ret = parse_feature_irqs(binfo, ofst, finfo);
>> + if (ret) {
>> + kfree(finfo);
>> + return ret;
>> + }
>>
>> list_add_tail(&finfo->node, &binfo->sub_features);
>> binfo->feature_num++;
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 45e6e1359a67..1ea7f40c1af6 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -273,11 +273,14 @@ struct dfl_feature_irq_ctx {
>> * @ops: ops of this sub feature.
>> * @ddev: ptr to the dfl device of this sub feature.
>> * @priv: priv data of this feature.
>> + * @param_size: size of dfh parameters
>> + * @params: point to memory copy of dfh parameters
>> */
>> struct dfl_feature {
>> struct platform_device *dev;
>> u16 id;
>> u8 revision;
>> + u8 dfh_version;
>> int resource_index;
>> void __iomem *ioaddr;
>> struct dfl_feature_irq_ctx *irq_ctx;
>> @@ -285,6 +288,8 @@ struct dfl_feature {
>> const struct dfl_feature_ops *ops;
>> struct dfl_device *ddev;
>> void *priv;
>> + unsigned int param_size;
>> + void *params;
>> };
>>
>> #define FEATURE_DEV_ID_UNUSED (-1)
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index fea9e16d35b6..d00787e870b7 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -50,9 +50,12 @@ struct dfl_device {
>> u16 type;
>> u16 feature_id;
>> u8 revision;
>> + u8 dfh_version;
>> struct resource mmio_res;
>> int *irqs;
>> unsigned int num_irqs;
>> + unsigned int param_size;
>> + void *params;
>> struct dfl_fpga_cdev *cdev;
>> const struct dfl_device_id *id_entry;
>> };
>> @@ -95,4 +98,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>> module_driver(__dfl_driver, dfl_driver_register, \
>> dfl_driver_unregister)
>>
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);
>
> int param_id is better?
param_id would be better.
>
> Thanks,
> Yilun
>
>> #endif /* __LINUX_DFL_H */
>> --
>> 2.25.1
>>
>
On 2022-10-20 at 14:26:10 -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]>
> ---
> v4: use dev_err_probe() everywhere that is appropriate
> clean up noise
> change error messages to use the word, unsupported
> tried again to sort Makefile and KConfig better
> reorder probe function for easier error handling
> use new dfh_find_param API
>
> v3: use passed in location of registers
> use cleaned up functions for parsing parameters
>
> 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 | 149 +++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 12 +++
> drivers/tty/serial/8250/Makefile | 1 +
> 3 files changed, 162 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..f02f0ba2a565
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,149 @@
> +// 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 {
> + int line;
> +};
> +
> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
> +{
> + struct device *dev = &dfl_dev->dev;
> + u64 v, fifo_len, reg_width;
> + u64 *p;
> +
> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> + if (!p)
> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
> +
> + uart->port.uartclk = *p;
> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> +
> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> + if (!p)
> + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
> +
> + fifo_len = *p;
> + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
> +
> + switch (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:
> + return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
> + }
> +
> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> + if (!p)
> + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
> +
> + v = *p;
> + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
I have concern that the raw layout inside the parameter block is
still exposed to drivers and need to be parsed by each driver.
How about we define HW agnostic IDs for parameter specific fields like:
PARAM_ID FIELD_ID
================================
MSIX STARTV
NUMV
--------------------------------
CLK FREQ
--------------------------------
FIFO LEN
--------------------------------
REG_LAYOUT WIDTH
SHIFT
And define like u64 dfl_find_param(struct dfl_device *, int param_id, int field_id)
Think further, if we have to define HW agnostic property - value pairs,
why don't we just use "Software nodes for the firmware node", see
drivers/base/swnode.c. I think this may be a better choice.
Thanks,
Yilun
On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> On 2022-10-20 at 14:26:09 -0700, [email protected] wrote:
> > struct dfl_feature_info {
> > u16 fid;
> > u8 revision;
> > + u8 dfh_version;
> > struct resource mmio_res;
> > void __iomem *ioaddr;
> > struct list_head node;
> > unsigned int irq_base;
> > unsigned int nr_irqs;
> > + unsigned int param_size;
> > + u64 params[];
> > };
...
> > + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
This probably may use something from overflow.h.
> The u64 flexible array in the structure, but seems dfh_get_psize could
> not garantee 64bit aligned size.
>
> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> u32[].
Isn't it guaranteed by the C standard / architecture ABI?
--
With Best Regards,
Andy Shevchenko
On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > On 2022-10-20 at 14:26:09 -0700, [email protected] wrote:
>
> > > struct dfl_feature_info {
> > > u16 fid;
> > > u8 revision;
> > > + u8 dfh_version;
> > > struct resource mmio_res;
> > > void __iomem *ioaddr;
> > > struct list_head node;
> > > unsigned int irq_base;
> > > unsigned int nr_irqs;
> > > + unsigned int param_size;
> > > + u64 params[];
> > > };
>
> ...
>
> > > + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>
>
> This probably may use something from overflow.h.
>
> > The u64 flexible array in the structure, but seems dfh_get_psize could
> > not garantee 64bit aligned size.
> >
> > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > u32[].
>
> Isn't it guaranteed by the C standard / architecture ABI?
I'm referring to the malloc size of the structure. It reserved dfh_psize
bytes for this u64 array, but there is no garantee dfh_psize should be a
multiple of 8. So there may be memory leak when accessing the last
array element?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
> On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> > On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > > On 2022-10-20 at 14:26:09 -0700, [email protected] wrote:
> >
> > > > struct dfl_feature_info {
> > > > u16 fid;
> > > > u8 revision;
> > > > + u8 dfh_version;
> > > > struct resource mmio_res;
> > > > void __iomem *ioaddr;
> > > > struct list_head node;
> > > > unsigned int irq_base;
> > > > unsigned int nr_irqs;
> > > > + unsigned int param_size;
> > > > + u64 params[];
> > > > };
> >
> > ...
> >
> > > > + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> >
> >
> > This probably may use something from overflow.h.
> >
> > > The u64 flexible array in the structure, but seems dfh_get_psize could
> > > not garantee 64bit aligned size.
> > >
> > > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > > u32[].
> >
> > Isn't it guaranteed by the C standard / architecture ABI?
>
> I'm referring to the malloc size of the structure. It reserved dfh_psize
> bytes for this u64 array, but there is no garantee dfh_psize should be a
> multiple of 8. So there may be memory leak when accessing the last
> array element?
Have you looked at macros in the overflow.h? Would the use of it solve your
concern?
--
With Best Regards,
Andy Shevchenko
On Mon, 31 Oct 2022, Andy Shevchenko wrote:
> On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
>> On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
>>> On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
>>>> On 2022-10-20 at 14:26:09 -0700, [email protected] wrote:
>>>
>>>>> struct dfl_feature_info {
>>>>> u16 fid;
>>>>> u8 revision;
>>>>> + u8 dfh_version;
>>>>> struct resource mmio_res;
>>>>> void __iomem *ioaddr;
>>>>> struct list_head node;
>>>>> unsigned int irq_base;
>>>>> unsigned int nr_irqs;
>>>>> + unsigned int param_size;
>>>>> + u64 params[];
>>>>> };
>>>
>>> ...
>>>
>>>>> + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>>>
>>>
>>> This probably may use something from overflow.h.
>>>
>>>> The u64 flexible array in the structure, but seems dfh_get_psize could
>>>> not garantee 64bit aligned size.
>>>>
>>>> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
>>>> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
>>>> u32[].
>>>
>>> Isn't it guaranteed by the C standard / architecture ABI?
>>
>> I'm referring to the malloc size of the structure. It reserved dfh_psize
>> bytes for this u64 array, but there is no garantee dfh_psize should be a
>> multiple of 8. So there may be memory leak when accessing the last
>> array element?
>
> Have you looked at macros in the overflow.h? Would the use of it solve your
> concern?
By clarifying the definition of the next field in the parameter header
as the number of 8-byte words, dfh_get_psize is guaranteed to be a multiple of 8.
This is fixed in the next revision of patches.
Matthew Gerlach
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
On Sat, 29 Oct 2022, Xu Yilun wrote:
> On 2022-10-20 at 14:26:10 -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]>
>> ---
>> v4: use dev_err_probe() everywhere that is appropriate
>> clean up noise
>> change error messages to use the word, unsupported
>> tried again to sort Makefile and KConfig better
>> reorder probe function for easier error handling
>> use new dfh_find_param API
>>
>> v3: use passed in location of registers
>> use cleaned up functions for parsing parameters
>>
>> 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 | 149 +++++++++++++++++++++++++++++
>> drivers/tty/serial/8250/Kconfig | 12 +++
>> drivers/tty/serial/8250/Makefile | 1 +
>> 3 files changed, 162 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..f02f0ba2a565
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>> @@ -0,0 +1,149 @@
>> +// 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 {
>> + int line;
>> +};
>> +
>> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
>> +{
>> + struct device *dev = &dfl_dev->dev;
>> + u64 v, fifo_len, reg_width;
>> + u64 *p;
>> +
>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>> + if (!p)
>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>> +
>> + uart->port.uartclk = *p;
>> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>> +
>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
>> + if (!p)
>> + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
>> +
>> + fifo_len = *p;
>> + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
>> +
>> + switch (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:
>> + return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
>> + }
>> +
>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
>> + if (!p)
>> + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
>> +
>> + v = *p;
>> + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>> + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>
> I have concern that the raw layout inside the parameter block is
> still exposed to drivers and need to be parsed by each driver.
Raw parameter block will always have to be passed to the driver because HW
specific properties can be defined that will need to be parsed by the
specific driver.
>
> How about we define HW agnostic IDs for parameter specific fields like:
>
> PARAM_ID FIELD_ID
> ================================
> MSIX STARTV
> NUMV
> --------------------------------
> CLK FREQ
> --------------------------------
> FIFO LEN
> --------------------------------
> REG_LAYOUT WIDTH
> SHIFT
>
> And define like u64 dfl_find_param(struct dfl_device *, int param_id, int field_id)
I don't think dfl_find_param as defined above adds much value.
>
> Think further, if we have to define HW agnostic property - value pairs,
> why don't we just use "Software nodes for the firmware node", see
> drivers/base/swnode.c. I think this may be a better choice.
I am looking into "Software nodes for the firmware node", and it can be
used for HW agnostic properties. Each dfl driver will still have to
make a function call to fetch each HW agnostice property value as well as
a function call to find the HW specific parameters and then parse those
parameters.
>
> Thanks,
> Yilun
>
On 2022-10-31 at 17:34:39 -0700, [email protected] wrote:
>
>
> On Sat, 29 Oct 2022, Xu Yilun wrote:
>
> > On 2022-10-20 at 14:26:10 -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]>
> > > ---
> > > v4: use dev_err_probe() everywhere that is appropriate
> > > clean up noise
> > > change error messages to use the word, unsupported
> > > tried again to sort Makefile and KConfig better
> > > reorder probe function for easier error handling
> > > use new dfh_find_param API
> > >
> > > v3: use passed in location of registers
> > > use cleaned up functions for parsing parameters
> > >
> > > 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 | 149 +++++++++++++++++++++++++++++
> > > drivers/tty/serial/8250/Kconfig | 12 +++
> > > drivers/tty/serial/8250/Makefile | 1 +
> > > 3 files changed, 162 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..f02f0ba2a565
> > > --- /dev/null
> > > +++ b/drivers/tty/serial/8250/8250_dfl.c
> > > @@ -0,0 +1,149 @@
> > > +// 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 {
> > > + int line;
> > > +};
> > > +
> > > +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
> > > +{
> > > + struct device *dev = &dfl_dev->dev;
> > > + u64 v, fifo_len, reg_width;
> > > + u64 *p;
> > > +
> > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> > > + if (!p)
> > > + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
> > > +
> > > + uart->port.uartclk = *p;
> > > + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> > > +
> > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> > > + if (!p)
> > > + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
> > > +
> > > + fifo_len = *p;
> > > + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
> > > +
> > > + switch (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:
> > > + return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
> > > + }
> > > +
> > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> > > + if (!p)
> > > + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
> > > +
> > > + v = *p;
> > > + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> > > + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> >
> > I have concern that the raw layout inside the parameter block is
> > still exposed to drivers and need to be parsed by each driver.
>
> Raw parameter block will always have to be passed to the driver because HW
> specific properties can be defined that will need to be parsed by the
> specific driver.
So there is a question about the scope of the definitions of these parameter
blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
seems specific to uart?
If a parameter block is widely used in dfl drivers, duplicate the parsing
from HW layout in each driver may not be a good idea. While for device
specific parameter block, it's OK.
Another concern is the indexing of the parameter IDs. If some parameter
blocks should be device specific, then no need to have globally indexed
parameter IDs. Index them locally in device is OK. So put the definitions
of ID values, HW layout and their parsing operation in each driver.
Thanks,
Yilun
>
> >
> > How about we define HW agnostic IDs for parameter specific fields like:
> >
> > PARAM_ID FIELD_ID
> > ================================
> > MSIX STARTV
> > NUMV
> > --------------------------------
> > CLK FREQ
> > --------------------------------
> > FIFO LEN
> > --------------------------------
> > REG_LAYOUT WIDTH
> > SHIFT
> >
> > And define like u64 dfl_find_param(struct dfl_device *, int param_id, int field_id)
>
> I don't think dfl_find_param as defined above adds much value.
>
> >
> > Think further, if we have to define HW agnostic property - value pairs,
> > why don't we just use "Software nodes for the firmware node", see
> > drivers/base/swnode.c. I think this may be a better choice.
>
> I am looking into "Software nodes for the firmware node", and it can be used
> for HW agnostic properties. Each dfl driver will still have to make a
> function call to fetch each HW agnostice property value as well as a
> function call to find the HW specific parameters and then parse those
> parameters.
>
> >
> > Thanks,
> > Yilun
> >
On 2022-10-31 at 17:34:20 +0200, Andy Shevchenko wrote:
> On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
> > On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> > > On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > > > On 2022-10-20 at 14:26:09 -0700, [email protected] wrote:
> > >
> > > > > struct dfl_feature_info {
> > > > > u16 fid;
> > > > > u8 revision;
> > > > > + u8 dfh_version;
> > > > > struct resource mmio_res;
> > > > > void __iomem *ioaddr;
> > > > > struct list_head node;
> > > > > unsigned int irq_base;
> > > > > unsigned int nr_irqs;
> > > > > + unsigned int param_size;
> > > > > + u64 params[];
> > > > > };
> > >
> > > ...
> > >
> > > > > + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> > >
> > >
> > > This probably may use something from overflow.h.
> > >
> > > > The u64 flexible array in the structure, but seems dfh_get_psize could
> > > > not garantee 64bit aligned size.
> > > >
> > > > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > > > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > > > u32[].
> > >
> > > Isn't it guaranteed by the C standard / architecture ABI?
> >
> > I'm referring to the malloc size of the structure. It reserved dfh_psize
> > bytes for this u64 array, but there is no garantee dfh_psize should be a
> > multiple of 8. So there may be memory leak when accessing the last
> > array element?
>
> Have you looked at macros in the overflow.h? Would the use of it solve your
> concern?
Yes, struct_size() or array_size() specifies the element size & count,
which solve the concern at the root.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Tue, 1 Nov 2022, [email protected] wrote:
>
>
> On Tue, 1 Nov 2022, Xu Yilun wrote:
>
> > On 2022-10-31 at 17:34:39 -0700, [email protected] wrote:
> > >
> > >
> > > On Sat, 29 Oct 2022, Xu Yilun wrote:
> > >
> > > > On 2022-10-20 at 14:26:10 -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]>
> > > > > ---
> > > > > v4: use dev_err_probe() everywhere that is appropriate
> > > > > clean up noise
> > > > > change error messages to use the word, unsupported
> > > > > tried again to sort Makefile and KConfig better
> > > > > reorder probe function for easier error handling
> > > > > use new dfh_find_param API
> > > > >
> > > > > v3: use passed in location of registers
> > > > > use cleaned up functions for parsing parameters
> > > > >
> > > > > 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 | 149
> > > > > +++++++++++++++++++++++++++++
> > > > > drivers/tty/serial/8250/Kconfig | 12 +++
> > > > > drivers/tty/serial/8250/Makefile | 1 +
> > > > > 3 files changed, 162 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..f02f0ba2a565
> > > > > --- /dev/null
> > > > > +++ b/drivers/tty/serial/8250/8250_dfl.c
> > > > > @@ -0,0 +1,149 @@
> > > > > +// 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 {
> > > > > + int line;
> > > > > +};
> > > > > +
> > > > > +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct
> > > > > uart_8250_port *uart)
> > > > > +{
> > > > > + struct device *dev = &dfl_dev->dev;
> > > > > + u64 v, fifo_len, reg_width;
> > > > > + u64 *p;
> > > > > +
> > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> > > > > + if (!p)
> > > > > + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
> > > > > param\n");
> > > > > +
> > > > > + uart->port.uartclk = *p;
> > > > > + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> > > > > +
> > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> > > > > + if (!p)
> > > > > + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
> > > > > param\n");
> > > > > +
> > > > > + fifo_len = *p;
> > > > > + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
> > > > > +
> > > > > + switch (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:
> > > > > + return dev_err_probe(dev, -EINVAL, "unsupported
> > > > > fifo_len %llu\n", fifo_len);
> > > > > + }
> > > > > +
> > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> > > > > + if (!p)
> > > > > + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
> > > > > param\n");
> > > > > +
> > > > > + v = *p;
> > > > > + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> > > > > + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> > > >
> > > > I have concern that the raw layout inside the parameter block is
> > > > still exposed to drivers and need to be parsed by each driver.
> > >
> > > Raw parameter block will always have to be passed to the driver because HW
> > > specific properties can be defined that will need to be parsed by the
> > > specific driver.
> >
> > So there is a question about the scope of the definitions of these parameter
> > blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> > seems specific to uart?
>
> There are definitely two classes of parameter blocks. One class is HW
> agnostic parameters where the parameters are relevant to many different kinds
> of HW components. MSI-X, and input clock-frequency are certainly HW agnostic,
> and it turns out that REG_LAYOUT is not specific to uart. You can see
> reg_bits and reg_stride in struct regmap_config. There are also device tree
> bindings for reg-shift and reg-io-width. The second class of parameters would
> be specific to HW component. In the case of this uart driver, all parameters
> would be considered HW agnostic parameters.
>
> >
> > If a parameter block is widely used in dfl drivers, duplicate the parsing
> > from HW layout in each driver may not be a good idea. While for device
> > specific parameter block, it's OK.
>
> It sounds like we are in agreement.
>
> >
> > Another concern is the indexing of the parameter IDs. If some parameter
> > blocks should be device specific, then no need to have globally indexed
> > parameter IDs. Index them locally in device is OK. So put the definitions
> > of ID values, HW layout and their parsing operation in each driver.
>
> It may be confusing for two drivers to use the same parameter id that have
> different meanings and data layout. Since all the parameters for this driver
> would be considered HW agnostic, we'd don't need to address this issue with
> this patchset.
>
> > > > How about we define HW agnostic IDs for parameter specific fields like:
> > > >
> > > > PARAM_ID FIELD_ID
> > > > ================================
> > > > MSIX STARTV
> > > > NUMV
> > > > --------------------------------
> > > > CLK FREQ
> > > > --------------------------------
> > > > FIFO LEN
> > > > --------------------------------
> > > > REG_LAYOUT WIDTH
> > > > SHIFT
> > > >
> > > > And define like u64 dfl_find_param(struct dfl_device *, int param_id,
> > > > int field_id)
> > >
> > > I don't think dfl_find_param as defined above adds much value.
> > >
> > > >
> > > > Think further, if we have to define HW agnostic property - value pairs,
> > > > why don't we just use "Software nodes for the firmware node", see
> > > > drivers/base/swnode.c. I think this may be a better choice.
> > >
> > > I am looking into "Software nodes for the firmware node", and it can be
> > > used
> > > for HW agnostic properties. Each dfl driver will still have to make a
> > > function call to fetch each HW agnostice property value as well as a
> > > function call to find the HW specific parameters and then parse those
> > > parameters.
Btw, another aspect this discussion has completely overlooked is the
presence of parameter version and how it impacts data layout. Is v1
always going be a subset of v2 or can a later version remove something
v1 had?
--
i.
On Tue, 1 Nov 2022, Xu Yilun wrote:
> On 2022-10-31 at 17:34:39 -0700, [email protected] wrote:
>>
>>
>> On Sat, 29 Oct 2022, Xu Yilun wrote:
>>
>>> On 2022-10-20 at 14:26:10 -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]>
>>>> ---
>>>> v4: use dev_err_probe() everywhere that is appropriate
>>>> clean up noise
>>>> change error messages to use the word, unsupported
>>>> tried again to sort Makefile and KConfig better
>>>> reorder probe function for easier error handling
>>>> use new dfh_find_param API
>>>>
>>>> v3: use passed in location of registers
>>>> use cleaned up functions for parsing parameters
>>>>
>>>> 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 | 149 +++++++++++++++++++++++++++++
>>>> drivers/tty/serial/8250/Kconfig | 12 +++
>>>> drivers/tty/serial/8250/Makefile | 1 +
>>>> 3 files changed, 162 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..f02f0ba2a565
>>>> --- /dev/null
>>>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>>>> @@ -0,0 +1,149 @@
>>>> +// 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 {
>>>> + int line;
>>>> +};
>>>> +
>>>> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
>>>> +{
>>>> + struct device *dev = &dfl_dev->dev;
>>>> + u64 v, fifo_len, reg_width;
>>>> + u64 *p;
>>>> +
>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>>>> + if (!p)
>>>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>>>> +
>>>> + uart->port.uartclk = *p;
>>>> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>>>> +
>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
>>>> + if (!p)
>>>> + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
>>>> +
>>>> + fifo_len = *p;
>>>> + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
>>>> +
>>>> + switch (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:
>>>> + return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
>>>> + }
>>>> +
>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
>>>> + if (!p)
>>>> + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
>>>> +
>>>> + v = *p;
>>>> + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>>>> + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>>>
>>> I have concern that the raw layout inside the parameter block is
>>> still exposed to drivers and need to be parsed by each driver.
>>
>> Raw parameter block will always have to be passed to the driver because HW
>> specific properties can be defined that will need to be parsed by the
>> specific driver.
>
> So there is a question about the scope of the definitions of these parameter
> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> seems specific to uart?
There are definitely two classes of parameter blocks. One class is HW
agnostic parameters where the parameters are relevant to many different
kinds of HW components. MSI-X, and input clock-frequency are certainly HW
agnostic, and it turns out that REG_LAYOUT is not specific to uart. You
can see reg_bits and reg_stride in struct regmap_config. There are also
device tree bindings for reg-shift and reg-io-width. The second class of
parameters would be specific to HW component. In the case of this uart
driver, all parameters would be considered HW agnostic parameters.
>
> If a parameter block is widely used in dfl drivers, duplicate the parsing
> from HW layout in each driver may not be a good idea. While for device
> specific parameter block, it's OK.
It sounds like we are in agreement.
>
> Another concern is the indexing of the parameter IDs. If some parameter
> blocks should be device specific, then no need to have globally indexed
> parameter IDs. Index them locally in device is OK. So put the definitions
> of ID values, HW layout and their parsing operation in each driver.
It may be confusing for two drivers to use the same parameter id that have
different meanings and data layout. Since all the parameters for this
driver would be considered HW agnostic, we'd don't need to address this
issue with this patchset.
>
> Thanks,
> Yilun
>
>>
>>>
>>> How about we define HW agnostic IDs for parameter specific fields like:
>>>
>>> PARAM_ID FIELD_ID
>>> ================================
>>> MSIX STARTV
>>> NUMV
>>> --------------------------------
>>> CLK FREQ
>>> --------------------------------
>>> FIFO LEN
>>> --------------------------------
>>> REG_LAYOUT WIDTH
>>> SHIFT
>>>
>>> And define like u64 dfl_find_param(struct dfl_device *, int param_id, int field_id)
>>
>> I don't think dfl_find_param as defined above adds much value.
>>
>>>
>>> Think further, if we have to define HW agnostic property - value pairs,
>>> why don't we just use "Software nodes for the firmware node", see
>>> drivers/base/swnode.c. I think this may be a better choice.
>>
>> I am looking into "Software nodes for the firmware node", and it can be used
>> for HW agnostic properties. Each dfl driver will still have to make a
>> function call to fetch each HW agnostice property value as well as a
>> function call to find the HW specific parameters and then parse those
>> parameters.
>>
>>>
>>> Thanks,
>>> Yilun
>>>
>
On Tue, 1 Nov 2022, Ilpo J?rvinen wrote:
> On Tue, 1 Nov 2022, [email protected] wrote:
>
>>
>>
>> On Tue, 1 Nov 2022, Xu Yilun wrote:
>>
>>> On 2022-10-31 at 17:34:39 -0700, [email protected] wrote:
>>>>
>>>>
>>>> On Sat, 29 Oct 2022, Xu Yilun wrote:
>>>>
>>>>> On 2022-10-20 at 14:26:10 -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]>
>>>>>> ---
>>>>>> v4: use dev_err_probe() everywhere that is appropriate
>>>>>> clean up noise
>>>>>> change error messages to use the word, unsupported
>>>>>> tried again to sort Makefile and KConfig better
>>>>>> reorder probe function for easier error handling
>>>>>> use new dfh_find_param API
>>>>>>
>>>>>> v3: use passed in location of registers
>>>>>> use cleaned up functions for parsing parameters
>>>>>>
>>>>>> 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 | 149
>>>>>> +++++++++++++++++++++++++++++
>>>>>> drivers/tty/serial/8250/Kconfig | 12 +++
>>>>>> drivers/tty/serial/8250/Makefile | 1 +
>>>>>> 3 files changed, 162 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..f02f0ba2a565
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>>>>>> @@ -0,0 +1,149 @@
>>>>>> +// 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 {
>>>>>> + int line;
>>>>>> +};
>>>>>> +
>>>>>> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct
>>>>>> uart_8250_port *uart)
>>>>>> +{
>>>>>> + struct device *dev = &dfl_dev->dev;
>>>>>> + u64 v, fifo_len, reg_width;
>>>>>> + u64 *p;
>>>>>> +
>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>>>>>> + if (!p)
>>>>>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
>>>>>> param\n");
>>>>>> +
>>>>>> + uart->port.uartclk = *p;
>>>>>> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>>>>>> +
>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
>>>>>> + if (!p)
>>>>>> + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
>>>>>> param\n");
>>>>>> +
>>>>>> + fifo_len = *p;
>>>>>> + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
>>>>>> +
>>>>>> + switch (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:
>>>>>> + return dev_err_probe(dev, -EINVAL, "unsupported
>>>>>> fifo_len %llu\n", fifo_len);
>>>>>> + }
>>>>>> +
>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
>>>>>> + if (!p)
>>>>>> + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
>>>>>> param\n");
>>>>>> +
>>>>>> + v = *p;
>>>>>> + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>>>>>> + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>>>>>
>>>>> I have concern that the raw layout inside the parameter block is
>>>>> still exposed to drivers and need to be parsed by each driver.
>>>>
>>>> Raw parameter block will always have to be passed to the driver because HW
>>>> specific properties can be defined that will need to be parsed by the
>>>> specific driver.
>>>
>>> So there is a question about the scope of the definitions of these parameter
>>> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
>>> seems specific to uart?
>>
>> There are definitely two classes of parameter blocks. One class is HW
>> agnostic parameters where the parameters are relevant to many different kinds
>> of HW components. MSI-X, and input clock-frequency are certainly HW agnostic,
>> and it turns out that REG_LAYOUT is not specific to uart. You can see
>> reg_bits and reg_stride in struct regmap_config. There are also device tree
>> bindings for reg-shift and reg-io-width. The second class of parameters would
>> be specific to HW component. In the case of this uart driver, all parameters
>> would be considered HW agnostic parameters.
>>
>>>
>>> If a parameter block is widely used in dfl drivers, duplicate the parsing
>>> from HW layout in each driver may not be a good idea. While for device
>>> specific parameter block, it's OK.
>>
>> It sounds like we are in agreement.
>>
>>>
>>> Another concern is the indexing of the parameter IDs. If some parameter
>>> blocks should be device specific, then no need to have globally indexed
>>> parameter IDs. Index them locally in device is OK. So put the definitions
>>> of ID values, HW layout and their parsing operation in each driver.
>>
>> It may be confusing for two drivers to use the same parameter id that have
>> different meanings and data layout. Since all the parameters for this driver
>> would be considered HW agnostic, we'd don't need to address this issue with
>> this patchset.
>>
>>>>> How about we define HW agnostic IDs for parameter specific fields like:
>>>>>
>>>>> PARAM_ID FIELD_ID
>>>>> ================================
>>>>> MSIX STARTV
>>>>> NUMV
>>>>> --------------------------------
>>>>> CLK FREQ
>>>>> --------------------------------
>>>>> FIFO LEN
>>>>> --------------------------------
>>>>> REG_LAYOUT WIDTH
>>>>> SHIFT
>>>>>
>>>>> And define like u64 dfl_find_param(struct dfl_device *, int param_id,
>>>>> int field_id)
>>>>
>>>> I don't think dfl_find_param as defined above adds much value.
>>>>
>>>>>
>>>>> Think further, if we have to define HW agnostic property - value pairs,
>>>>> why don't we just use "Software nodes for the firmware node", see
>>>>> drivers/base/swnode.c. I think this may be a better choice.
>>>>
>>>> I am looking into "Software nodes for the firmware node", and it can be
>>>> used
>>>> for HW agnostic properties. Each dfl driver will still have to make a
>>>> function call to fetch each HW agnostice property value as well as a
>>>> function call to find the HW specific parameters and then parse those
>>>> parameters.
>
> Btw, another aspect this discussion has completely overlooked is the
> presence of parameter version and how it impacts data layout. Is v1
> always going be a subset of v2 or can a later version remove something
> v1 had?
In general it would be preferable for v1 to be a subset of v2. This
allows for v1 SW to work on v2 HW.
>
> --
> i.
>
On Sat, 29 Oct 2022, [email protected] wrote:
>
>>
>>> if (!finfo)
>>> return -ENOMEM;
>>>
>>> + if (dfh_psize > 0) {
>>> + memcpy_fromio(finfo->params,
>>> + binfo->ioaddr + ofst + DFHv1_PARAM_HDR,
>>> dfh_psize);
>>> + finfo->param_size = dfh_psize;
>>> + }
>>> +
>>> finfo->fid = fid;
>>> finfo->revision = revision;
>>> - finfo->mmio_res.start = binfo->start + ofst;
>>> - finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>> + finfo->dfh_version = dfh_ver;
>>> finfo->mmio_res.flags = IORESOURCE_MEM;
>>> - finfo->irq_base = irq_base;
>>> - finfo->nr_irqs = nr_irqs;
>>> + if (dfh_ver == 1) {
>>> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>>> + if (v & DFHv1_CSR_ADDR_REL)
>>> + finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>>> + else
>>> + finfo->mmio_res.start = binfo->start + ofst +
>>> + FIELD_GET(DFHv1_CSR_ADDR_MASK,
>>> v);
>>> +
>>> + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>>> + finfo->mmio_res.end = finfo->mmio_res.start +
>>> + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) -
>>> 1;
>>
>> So for dflv1, no feature header resource for dfl_device, is it a problem
>> for dfl_uio? Does userspace driver need the raw feature header?
> These are two very good questions. The dfl_uio driver question is
> particularly relevent because user space is looking at the GUIDs.
>
In the case of dfl_uio driver, user space will definitely want to look at
the feature header for the GUID and the parameters. Since DFHv1 can have
the DFH header and the feature registers in non-contiguous memory
locations, a resource for the dfl_device will be required. In earlier
revisions of this patch set, a second resource was added called csr_res
pointing to the feature's register while mmio_res pointed at the header.
Do we just need better names or do we need an array of named resources?
>>
>>> + } else {
>>> + finfo->mmio_res.start = binfo->start + ofst;
>>> + finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>> + }
>>> +
>>> + ret = parse_feature_irqs(binfo, ofst, finfo);
>>> + if (ret) {
>>> + kfree(finfo);
>>> + return ret;
>>> + }
>>>
On Tue, 1 Nov 2022, [email protected] wrote:
>
>
> On Tue, 1 Nov 2022, Ilpo J?rvinen wrote:
>
> > On Tue, 1 Nov 2022, [email protected] wrote:
> >
> > >
> > >
> > > On Tue, 1 Nov 2022, Xu Yilun wrote:
> > >
> > > > On 2022-10-31 at 17:34:39 -0700, [email protected] wrote:
> > > > >
> > > > >
> > > > > On Sat, 29 Oct 2022, Xu Yilun wrote:
> > > > >
> > > > > > On 2022-10-20 at 14:26:10 -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]>
> > > > > > > ---
> > > > > > > v4: use dev_err_probe() everywhere that is appropriate
> > > > > > > clean up noise
> > > > > > > change error messages to use the word, unsupported
> > > > > > > tried again to sort Makefile and KConfig better
> > > > > > > reorder probe function for easier error handling
> > > > > > > use new dfh_find_param API
> > > > > > >
> > > > > > > v3: use passed in location of registers
> > > > > > > use cleaned up functions for parsing parameters
> > > > > > >
> > > > > > > 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 | 149
> > > > > > > +++++++++++++++++++++++++++++
> > > > > > > drivers/tty/serial/8250/Kconfig | 12 +++
> > > > > > > drivers/tty/serial/8250/Makefile | 1 +
> > > > > > > 3 files changed, 162 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..f02f0ba2a565
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/tty/serial/8250/8250_dfl.c
> > > > > > > @@ -0,0 +1,149 @@
> > > > > > > +// 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 {
> > > > > > > + int line;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct
> > > > > > > uart_8250_port *uart)
> > > > > > > +{
> > > > > > > + struct device *dev = &dfl_dev->dev;
> > > > > > > + u64 v, fifo_len, reg_width;
> > > > > > > + u64 *p;
> > > > > > > +
> > > > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> > > > > > > + if (!p)
> > > > > > > + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
> > > > > > > param\n");
> > > > > > > +
> > > > > > > + uart->port.uartclk = *p;
> > > > > > > + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> > > > > > > +
> > > > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> > > > > > > + if (!p)
> > > > > > > + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
> > > > > > > param\n");
> > > > > > > +
> > > > > > > + fifo_len = *p;
> > > > > > > + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
> > > > > > > +
> > > > > > > + switch (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:
> > > > > > > + return dev_err_probe(dev, -EINVAL, "unsupported
> > > > > > > fifo_len %llu\n", fifo_len);
> > > > > > > + }
> > > > > > > +
> > > > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> > > > > > > + if (!p)
> > > > > > > + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
> > > > > > > param\n");
> > > > > > > +
> > > > > > > + v = *p;
> > > > > > > + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> > > > > > > + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> > > > > >
> > > > > > I have concern that the raw layout inside the parameter block is
> > > > > > still exposed to drivers and need to be parsed by each driver.
> > > > >
> > > > > Raw parameter block will always have to be passed to the driver
> > > > > because HW
> > > > > specific properties can be defined that will need to be parsed by the
> > > > > specific driver.
> > > >
> > > > So there is a question about the scope of the definitions of these
> > > > parameter
> > > > blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> > > > seems specific to uart?
> > >
> > > There are definitely two classes of parameter blocks. One class is HW
> > > agnostic parameters where the parameters are relevant to many different
> > > kinds
> > > of HW components. MSI-X, and input clock-frequency are certainly HW
> > > agnostic,
> > > and it turns out that REG_LAYOUT is not specific to uart. You can see
> > > reg_bits and reg_stride in struct regmap_config. There are also device
> > > tree
> > > bindings for reg-shift and reg-io-width. The second class of parameters
> > > would
> > > be specific to HW component. In the case of this uart driver, all
> > > parameters
> > > would be considered HW agnostic parameters.
> > >
> > > >
> > > > If a parameter block is widely used in dfl drivers, duplicate the
> > > > parsing
> > > > from HW layout in each driver may not be a good idea. While for device
> > > > specific parameter block, it's OK.
> > >
> > > It sounds like we are in agreement.
> > >
> > > >
> > > > Another concern is the indexing of the parameter IDs. If some parameter
> > > > blocks should be device specific, then no need to have globally indexed
> > > > parameter IDs. Index them locally in device is OK. So put the
> > > > definitions
> > > > of ID values, HW layout and their parsing operation in each driver.
> > >
> > > It may be confusing for two drivers to use the same parameter id that have
> > > different meanings and data layout. Since all the parameters for this
> > > driver
> > > would be considered HW agnostic, we'd don't need to address this issue
> > > with
> > > this patchset.
> > >
> > > > > > How about we define HW agnostic IDs for parameter specific fields
> > > > > > like:
> > > > > >
> > > > > > PARAM_ID FIELD_ID
> > > > > > ================================
> > > > > > MSIX STARTV
> > > > > > NUMV
> > > > > > --------------------------------
> > > > > > CLK FREQ
> > > > > > --------------------------------
> > > > > > FIFO LEN
> > > > > > --------------------------------
> > > > > > REG_LAYOUT WIDTH
> > > > > > SHIFT
> > > > > >
> > > > > > And define like u64 dfl_find_param(struct dfl_device *, int
> > > > > > param_id,
> > > > > > int field_id)
> > > > >
> > > > > I don't think dfl_find_param as defined above adds much value.
> > > > >
> > > > > >
> > > > > > Think further, if we have to define HW agnostic property - value
> > > > > > pairs,
> > > > > > why don't we just use "Software nodes for the firmware node", see
> > > > > > drivers/base/swnode.c. I think this may be a better choice.
> > > > >
> > > > > I am looking into "Software nodes for the firmware node", and it can
> > > > > be
> > > > > used
> > > > > for HW agnostic properties. Each dfl driver will still have to make a
> > > > > function call to fetch each HW agnostice property value as well as a
> > > > > function call to find the HW specific parameters and then parse those
> > > > > parameters.
> >
> > Btw, another aspect this discussion has completely overlooked is the
> > presence of parameter version and how it impacts data layout. Is v1
> > always going be a subset of v2 or can a later version remove something
> > v1 had?
>
> In general it would be preferable for v1 to be a subset of v2. This allows
> for v1 SW to work on v2 HW.
In that case, shouldn't the minimum acceptable version be part of
dfh_find_param() parameters?
Currently there's no way for the caller to even look what version the
parameter is from dfh_find_param()'s return value (except with some
negative offset hack to access parameter header).
--
i.
On 2022-11-01 at 15:37:19 -0700, [email protected] wrote:
>
>
> On Sat, 29 Oct 2022, [email protected] wrote:
>
> >
> > >
> > > > if (!finfo)
> > > > return -ENOMEM;
> > > >
> > > > + if (dfh_psize > 0) {
> > > > + memcpy_fromio(finfo->params,
> > > > + binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> > > > + finfo->param_size = dfh_psize;
> > > > + }
> > > > +
> > > > finfo->fid = fid;
> > > > finfo->revision = revision;
> > > > - finfo->mmio_res.start = binfo->start + ofst;
> > > > - finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > > > + finfo->dfh_version = dfh_ver;
> > > > finfo->mmio_res.flags = IORESOURCE_MEM;
> > > > - finfo->irq_base = irq_base;
> > > > - finfo->nr_irqs = nr_irqs;
> > > > + if (dfh_ver == 1) {
> > > > + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> > > > + if (v & DFHv1_CSR_ADDR_REL)
> > > > + finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> > > > + else
> > > > + finfo->mmio_res.start = binfo->start + ofst +
> > > > + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> > > > +
> > > > + v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> > > > + finfo->mmio_res.end = finfo->mmio_res.start +
> > > > + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> > >
> > > So for dflv1, no feature header resource for dfl_device, is it a problem
> > > for dfl_uio? Does userspace driver need the raw feature header?
> > These are two very good questions. The dfl_uio driver question is
> > particularly relevent because user space is looking at the GUIDs.
> >
>
> In the case of dfl_uio driver, user space will definitely want to look at
> the feature header for the GUID and the parameters. Since DFHv1 can have
> the DFH header and the feature registers in non-contiguous memory locations,
> a resource for the dfl_device will be required. In earlier
> revisions of this patch set, a second resource was added called csr_res
> pointing to the feature's register while mmio_res pointed at the header.
> Do we just need better names or do we need an array of named resources?
Either is OK, you could also name a resource element in an array by
struct resource:name. But my concern is still no overlapping.
Thanks,
Yilun
On 2022-11-02 10:57, Ilpo Järvinen wrote:
> On Tue, 1 Nov 2022, [email protected] wrote:
>
>>
>>
>> On Tue, 1 Nov 2022, Ilpo Järvinen wrote:
>>
>>> On Tue, 1 Nov 2022, [email protected] wrote:
>>>
>>>>
>>>>
>>>> On Tue, 1 Nov 2022, Xu Yilun wrote:
>>>>
>>>>> On 2022-10-31 at 17:34:39 -0700, [email protected] wrote:
>>>>>>
>>>>>>
>>>>>> On Sat, 29 Oct 2022, Xu Yilun wrote:
>>>>>>
>>>>>>> On 2022-10-20 at 14:26:10 -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]>
>>>>>>>> ---
>>>>>>>> v4: use dev_err_probe() everywhere that is appropriate
>>>>>>>> clean up noise
>>>>>>>> change error messages to use the word, unsupported
>>>>>>>> tried again to sort Makefile and KConfig better
>>>>>>>> reorder probe function for easier error handling
>>>>>>>> use new dfh_find_param API
>>>>>>>>
>>>>>>>> v3: use passed in location of registers
>>>>>>>> use cleaned up functions for parsing parameters
>>>>>>>>
>>>>>>>> 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 | 149
>>>>>>>> +++++++++++++++++++++++++++++
>>>>>>>> drivers/tty/serial/8250/Kconfig | 12 +++
>>>>>>>> drivers/tty/serial/8250/Makefile | 1 +
>>>>>>>> 3 files changed, 162 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..f02f0ba2a565
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>>>>>>>> @@ -0,0 +1,149 @@
>>>>>>>> +// 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 {
>>>>>>>> + int line;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct
>>>>>>>> uart_8250_port *uart)
>>>>>>>> +{
>>>>>>>> + struct device *dev = &dfl_dev->dev;
>>>>>>>> + u64 v, fifo_len, reg_width;
>>>>>>>> + u64 *p;
>>>>>>>> +
>>>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>>>>>>>> + if (!p)
>>>>>>>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
>>>>>>>> param\n");
>>>>>>>> +
>>>>>>>> + uart->port.uartclk = *p;
>>>>>>>> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>>>>>>>> +
>>>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
>>>>>>>> + if (!p)
>>>>>>>> + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
>>>>>>>> param\n");
>>>>>>>> +
>>>>>>>> + fifo_len = *p;
>>>>>>>> + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
>>>>>>>> +
>>>>>>>> + switch (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:
>>>>>>>> + return dev_err_probe(dev, -EINVAL, "unsupported
>>>>>>>> fifo_len %llu\n", fifo_len);
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
>>>>>>>> + if (!p)
>>>>>>>> + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
>>>>>>>> param\n");
>>>>>>>> +
>>>>>>>> + v = *p;
>>>>>>>> + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>>>>>>>> + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>>>>>>>
>>>>>>> I have concern that the raw layout inside the parameter block is
>>>>>>> still exposed to drivers and need to be parsed by each driver.
>>>>>>
>>>>>> Raw parameter block will always have to be passed to the driver
>>>>>> because HW
>>>>>> specific properties can be defined that will need to be parsed by the
>>>>>> specific driver.
>>>>>
>>>>> So there is a question about the scope of the definitions of these
>>>>> parameter
>>>>> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
>>>>> seems specific to uart?
>>>>
>>>> There are definitely two classes of parameter blocks. One class is HW
>>>> agnostic parameters where the parameters are relevant to many different
>>>> kinds
>>>> of HW components. MSI-X, and input clock-frequency are certainly HW
>>>> agnostic,
>>>> and it turns out that REG_LAYOUT is not specific to uart. You can see
>>>> reg_bits and reg_stride in struct regmap_config. There are also device
>>>> tree
>>>> bindings for reg-shift and reg-io-width. The second class of parameters
>>>> would
>>>> be specific to HW component. In the case of this uart driver, all
>>>> parameters
>>>> would be considered HW agnostic parameters.
>>>>
>>>>>
>>>>> If a parameter block is widely used in dfl drivers, duplicate the
>>>>> parsing
>>>>> from HW layout in each driver may not be a good idea. While for device
>>>>> specific parameter block, it's OK.
>>>>
>>>> It sounds like we are in agreement.
>>>>
>>>>>
>>>>> Another concern is the indexing of the parameter IDs. If some parameter
>>>>> blocks should be device specific, then no need to have globally indexed
>>>>> parameter IDs. Index them locally in device is OK. So put the
>>>>> definitions
>>>>> of ID values, HW layout and their parsing operation in each driver.
>>>>
>>>> It may be confusing for two drivers to use the same parameter id that have
>>>> different meanings and data layout. Since all the parameters for this
>>>> driver
>>>> would be considered HW agnostic, we'd don't need to address this issue
>>>> with
>>>> this patchset.
>>>>
>>>>>>> How about we define HW agnostic IDs for parameter specific fields
>>>>>>> like:
>>>>>>>
>>>>>>> PARAM_ID FIELD_ID
>>>>>>> ================================
>>>>>>> MSIX STARTV
>>>>>>> NUMV
>>>>>>> --------------------------------
>>>>>>> CLK FREQ
>>>>>>> --------------------------------
>>>>>>> FIFO LEN
>>>>>>> --------------------------------
>>>>>>> REG_LAYOUT WIDTH
>>>>>>> SHIFT
>>>>>>>
>>>>>>> And define like u64 dfl_find_param(struct dfl_device *, int
>>>>>>> param_id,
>>>>>>> int field_id)
>>>>>>
>>>>>> I don't think dfl_find_param as defined above adds much value.
>>>>>>
>>>>>>>
>>>>>>> Think further, if we have to define HW agnostic property - value
>>>>>>> pairs,
>>>>>>> why don't we just use "Software nodes for the firmware node", see
>>>>>>> drivers/base/swnode.c. I think this may be a better choice.
>>>>>>
>>>>>> I am looking into "Software nodes for the firmware node", and it can
>>>>>> be
>>>>>> used
>>>>>> for HW agnostic properties. Each dfl driver will still have to make a
>>>>>> function call to fetch each HW agnostice property value as well as a
>>>>>> function call to find the HW specific parameters and then parse those
>>>>>> parameters.
>>>
>>> Btw, another aspect this discussion has completely overlooked is the
>>> presence of parameter version and how it impacts data layout. Is v1
>>> always going be a subset of v2 or can a later version remove something
>>> v1 had?
>>
>> In general it would be preferable for v1 to be a subset of v2. This allows
>> for v1 SW to work on v2 HW.
>
> In that case, shouldn't the minimum acceptable version be part of
> dfh_find_param() parameters?
>
> Currently there's no way for the caller to even look what version the
> parameter is from dfh_find_param()'s return value (except with some
> negative offset hack to access parameter header).
>
>
Why not just checking dfl_dev->dfh_version in dfl_uart_probe() before
calling dfh_find_param()? In general, any dfl_driver could potentially
do this check in its *_probe() function before reading the header to avoid
compatibility issues.
Cheers,
Marco
On Tue, 8 Nov 2022, Marco Pagani wrote:
>
> On 2022-11-02 10:57, Ilpo Järvinen wrote:
> > On Tue, 1 Nov 2022, [email protected] wrote:
> >
> >>
> >>
> >> On Tue, 1 Nov 2022, Ilpo Järvinen wrote:
> >>
> >>> On Tue, 1 Nov 2022, [email protected] wrote:
> >>>
> >>>>
> >>>>
> >>>> On Tue, 1 Nov 2022, Xu Yilun wrote:
> >>>>
> >>>>> On 2022-10-31 at 17:34:39 -0700, [email protected] wrote:
> >>>>>>
> >>>>>>
> >>>>>> On Sat, 29 Oct 2022, Xu Yilun wrote:
> >>>>>>
> >>>>>>> On 2022-10-20 at 14:26:10 -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]>
> >>>>>>>> ---
> >>>>>>>> v4: use dev_err_probe() everywhere that is appropriate
> >>>>>>>> clean up noise
> >>>>>>>> change error messages to use the word, unsupported
> >>>>>>>> tried again to sort Makefile and KConfig better
> >>>>>>>> reorder probe function for easier error handling
> >>>>>>>> use new dfh_find_param API
> >>>>>>>>
> >>>>>>>> v3: use passed in location of registers
> >>>>>>>> use cleaned up functions for parsing parameters
> >>>>>>>>
> >>>>>>>> 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 | 149
> >>>>>>>> +++++++++++++++++++++++++++++
> >>>>>>>> drivers/tty/serial/8250/Kconfig | 12 +++
> >>>>>>>> drivers/tty/serial/8250/Makefile | 1 +
> >>>>>>>> 3 files changed, 162 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..f02f0ba2a565
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/drivers/tty/serial/8250/8250_dfl.c
> >>>>>>>> @@ -0,0 +1,149 @@
> >>>>>>>> +// 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 {
> >>>>>>>> + int line;
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct
> >>>>>>>> uart_8250_port *uart)
> >>>>>>>> +{
> >>>>>>>> + struct device *dev = &dfl_dev->dev;
> >>>>>>>> + u64 v, fifo_len, reg_width;
> >>>>>>>> + u64 *p;
> >>>>>>>> +
> >>>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> >>>>>>>> + if (!p)
> >>>>>>>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
> >>>>>>>> param\n");
> >>>>>>>> +
> >>>>>>>> + uart->port.uartclk = *p;
> >>>>>>>> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> >>>>>>>> +
> >>>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> >>>>>>>> + if (!p)
> >>>>>>>> + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
> >>>>>>>> param\n");
> >>>>>>>> +
> >>>>>>>> + fifo_len = *p;
> >>>>>>>> + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
> >>>>>>>> +
> >>>>>>>> + switch (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:
> >>>>>>>> + return dev_err_probe(dev, -EINVAL, "unsupported
> >>>>>>>> fifo_len %llu\n", fifo_len);
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> >>>>>>>> + if (!p)
> >>>>>>>> + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
> >>>>>>>> param\n");
> >>>>>>>> +
> >>>>>>>> + v = *p;
> >>>>>>>> + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> >>>>>>>> + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> >>>>>>>
> >>>>>>> I have concern that the raw layout inside the parameter block is
> >>>>>>> still exposed to drivers and need to be parsed by each driver.
> >>>>>>
> >>>>>> Raw parameter block will always have to be passed to the driver
> >>>>>> because HW
> >>>>>> specific properties can be defined that will need to be parsed by the
> >>>>>> specific driver.
> >>>>>
> >>>>> So there is a question about the scope of the definitions of these
> >>>>> parameter
> >>>>> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> >>>>> seems specific to uart?
> >>>>
> >>>> There are definitely two classes of parameter blocks. One class is HW
> >>>> agnostic parameters where the parameters are relevant to many different
> >>>> kinds
> >>>> of HW components. MSI-X, and input clock-frequency are certainly HW
> >>>> agnostic,
> >>>> and it turns out that REG_LAYOUT is not specific to uart. You can see
> >>>> reg_bits and reg_stride in struct regmap_config. There are also device
> >>>> tree
> >>>> bindings for reg-shift and reg-io-width. The second class of parameters
> >>>> would
> >>>> be specific to HW component. In the case of this uart driver, all
> >>>> parameters
> >>>> would be considered HW agnostic parameters.
> >>>>
> >>>>>
> >>>>> If a parameter block is widely used in dfl drivers, duplicate the
> >>>>> parsing
> >>>>> from HW layout in each driver may not be a good idea. While for device
> >>>>> specific parameter block, it's OK.
> >>>>
> >>>> It sounds like we are in agreement.
> >>>>
> >>>>>
> >>>>> Another concern is the indexing of the parameter IDs. If some parameter
> >>>>> blocks should be device specific, then no need to have globally indexed
> >>>>> parameter IDs. Index them locally in device is OK. So put the
> >>>>> definitions
> >>>>> of ID values, HW layout and their parsing operation in each driver.
> >>>>
> >>>> It may be confusing for two drivers to use the same parameter id that have
> >>>> different meanings and data layout. Since all the parameters for this
> >>>> driver
> >>>> would be considered HW agnostic, we'd don't need to address this issue
> >>>> with
> >>>> this patchset.
> >>>>
> >>>>>>> How about we define HW agnostic IDs for parameter specific fields
> >>>>>>> like:
> >>>>>>>
> >>>>>>> PARAM_ID FIELD_ID
> >>>>>>> ================================
> >>>>>>> MSIX STARTV
> >>>>>>> NUMV
> >>>>>>> --------------------------------
> >>>>>>> CLK FREQ
> >>>>>>> --------------------------------
> >>>>>>> FIFO LEN
> >>>>>>> --------------------------------
> >>>>>>> REG_LAYOUT WIDTH
> >>>>>>> SHIFT
> >>>>>>>
> >>>>>>> And define like u64 dfl_find_param(struct dfl_device *, int
> >>>>>>> param_id,
> >>>>>>> int field_id)
> >>>>>>
> >>>>>> I don't think dfl_find_param as defined above adds much value.
> >>>>>>
> >>>>>>>
> >>>>>>> Think further, if we have to define HW agnostic property - value
> >>>>>>> pairs,
> >>>>>>> why don't we just use "Software nodes for the firmware node", see
> >>>>>>> drivers/base/swnode.c. I think this may be a better choice.
> >>>>>>
> >>>>>> I am looking into "Software nodes for the firmware node", and it can
> >>>>>> be
> >>>>>> used
> >>>>>> for HW agnostic properties. Each dfl driver will still have to make a
> >>>>>> function call to fetch each HW agnostice property value as well as a
> >>>>>> function call to find the HW specific parameters and then parse those
> >>>>>> parameters.
> >>>
> >>> Btw, another aspect this discussion has completely overlooked is the
> >>> presence of parameter version and how it impacts data layout. Is v1
> >>> always going be a subset of v2 or can a later version remove something
> >>> v1 had?
> >>
> >> In general it would be preferable for v1 to be a subset of v2. This allows
> >> for v1 SW to work on v2 HW.
> >
> > In that case, shouldn't the minimum acceptable version be part of
> > dfh_find_param() parameters?
> >
> > Currently there's no way for the caller to even look what version the
> > parameter is from dfh_find_param()'s return value (except with some
> > negative offset hack to access parameter header).
> >
> >
>
> Why not just checking dfl_dev->dfh_version in dfl_uart_probe() before
> calling dfh_find_param()? In general, any dfl_driver could potentially
> do this check in its *_probe() function before reading the header to avoid
> compatibility issues.
It's about a different version. DFH has it's own version and every
parameter header has a separate version specific to that parameter.
--
i.