[PATCH v1 0/5] Enhance definition of DFH and use enhancements for uart driver

[PATCH v1 0/5] Enhance definition of DFH and use enhancements for uart driver

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@intel.com>

This patchset enhances the definition of the Device Feature Header (DFH) used by
the Device Feature List (DFL) bus and then uses the new enhancements in a uart
driver.

Patch 1 updates the DFL documentation to provide the motivation behind the 
enhancements to the definition of the DFH.

Patch 2 moves some of the DFH definitions to include/linux/dfl.h so that
they can be accessed by drivers outside of drivers/fpga.

Patch 3 adds the definitions for DFHv1.

Patch 4 defines and uses a DFHv1 parameter to provide a generic mechanism for
describing MSIX interrupts used by a particular feature instance.

Patch 5 adds a DFL uart driver that makes use of the new features of DFHv1.

Basheer Ahmed Muddebihal (2):
  fpga: dfl: Move the DFH definitions
  fpga: dfl: Add DFHv1 Register Definitions

Matthew Gerlach (3):
  Documentation: fpga: dfl: Add documentation for DFHv1
  fpga: dfl: add generic support for MSIX interrupts
  tty: serial: 8250: add DFL bus driver for Altera 16550.

 Documentation/fpga/dfl.rst         |  24 ++++
 drivers/fpga/dfl.c                 |  59 ++++++---
 drivers/fpga/dfl.h                 |  22 +---
 drivers/tty/serial/8250/8250_dfl.c | 188 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |   9 ++
 drivers/tty/serial/8250/Makefile   |   1 +
 include/linux/dfl.h                |  80 +++++++++++-
 7 files changed, 347 insertions(+), 36 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

-- 
2.25.1

[PATCH v1 1/5] Documentation: fpga: dfl: Add documentation for DFHv1

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Add documentation describing the extentions provided by Version
1 of the Device Feature Header (DFHv1).

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 15b670926084..31699b89781e 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -561,6 +561,30 @@ new DFL feature via UIO direct access, its feature id should be added to the
 driver's id_table.
 
 
+Extending the Device Feature Header - DFHv1
+===========================================
+The current 8 bytes of the Device Feature Header, hereafter referred to as
+to DFHv0, provide very little opportunity for the hardware to describe itself
+to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
+to provide increased flexibility and extensibility to hardware designs using
+Device Feature Lists.  The list below describes some of the goals behind the
+changes in DFHv1:
+
+* Provide a standardized mechanism for features to describe
+  parameters/capabilities to software.
+* Standardize the use of a GUID for all DFHv1 types.
+* Decouple the location of the DFH from the register space of the feature itself.
+
+Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
+a list of parameter values to a particular feature.
+
+With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUILD standard
+across all types.
+
+With DFHv0, the register map of a given feature is located immediately following
+the DFHv0 in the memory space.  With DFHv1, the location of the feature register
+map can be specified as an offset to the DFHv1 or as an absolute address.
+
 Open discussion
 ===============
 FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
-- 
2.25.1

[PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[matthew.gerlach]

From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>

Moving the DFH register offset and register definitions from
drivers/fpga/dfl.h to include/linux/dfl.h.  These definitions
need to be accessed by dfl drivers that are outside of
drivers/fpga.

Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/fpga/dfl.h  | 22 ++--------------------
 include/linux/dfl.h | 23 ++++++++++++++++++++++-
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 06cfcd5e84bb..d4dfc03a0b61 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -2,7 +2,7 @@
 /*
  * Driver Header File for FPGA Device Feature List (DFL) Support
  *
- * Copyright (C) 2017-2018 Intel Corporation, Inc.
+ * Copyright (C) 2017-2022 Intel Corporation, Inc.
  *
  * Authors:
  *   Kang Luwei <luwei.kang@intel.com>
@@ -17,6 +17,7 @@
 #include <linux/bitfield.h>
 #include <linux/cdev.h>
 #include <linux/delay.h>
+#include <linux/dfl.h>
 #include <linux/eventfd.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
@@ -53,28 +54,9 @@
 #define PORT_FEATURE_ID_UINT		0x12
 #define PORT_FEATURE_ID_STP		0x13
 
-/*
- * Device Feature Header Register Set
- *
- * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
- * For AFUs, they have DFH + GUID as common header registers.
- * For private features, they only have DFH register as common header.
- */
-#define DFH			0x0
-#define GUID_L			0x8
-#define GUID_H			0x10
-#define NEXT_AFU		0x18
-
-#define DFH_SIZE		0x8
-
 /* Device Feature Header Register Bitfield */
-#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
 #define DFH_ID_FIU_FME		0
 #define DFH_ID_FIU_PORT		1
-#define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
-#define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
-#define DFH_EOL			BIT_ULL(40)		/* End of list */
-#define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
 #define DFH_TYPE_AFU		1
 #define DFH_TYPE_PRIVATE	3
 #define DFH_TYPE_FIU		4
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 431636a0dc78..b5accdcfa368 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -2,7 +2,7 @@
 /*
  * Header file for DFL driver and device API
  *
- * Copyright (C) 2020 Intel Corporation, Inc.
+ * Copyright (C) 2020-2022 Intel Corporation, Inc.
  */
 
 #ifndef __LINUX_DFL_H
@@ -11,6 +11,27 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 
+/*
+ * Device Feature Header Register Set
+ *
+ * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
+ * For AFUs, they have DFH + GUID as common header registers.
+ * For private features, they only have DFH register as common header.
+ */
+#define DFH			0x0
+#define GUID_L			0x8
+#define GUID_H			0x10
+#define NEXT_AFU		0x18
+
+#define DFH_SIZE		0x8
+
+/* Device Feature Header Register Bitfield */
+#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
+#define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
+#define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
+#define DFH_EOL			BIT_ULL(40)		/* End of list */
+#define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
+
 /**
  * enum dfl_id_type - define the DFL FIU types
  */
-- 
2.25.1

[PATCH v1 3/5] fpga: dfl: Add DFHv1 Register Definitions

[matthew.gerlach]

From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>

This patch adds the definitions for DFHv1 header and related register
bitfields.

Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 include/linux/dfl.h | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index b5accdcfa368..61bcf20c1bc8 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -23,6 +23,16 @@
 #define GUID_H			0x10
 #define NEXT_AFU		0x18
 
+/*
+ * DFHv1 Register Offset definitons
+ * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
+ * as common header registers
+ */
+#define DFHv1_CSR_ADDR		0x18  /* CSR Register start address */
+#define DFHv1_CSR_SIZE_GRP	0x20  /* Size of Reg Block and Group/tag */
+#define DFHv1_PARAM_HDR		0x28  /* Optional First Param header */
+#define DFHv1_PARAM_DATA	0x8   /* Offset of Param data from Param header */
+
 #define DFH_SIZE		0x8
 
 /* Device Feature Header Register Bitfield */
@@ -30,8 +40,35 @@
 #define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
 #define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
 #define DFH_EOL			BIT_ULL(40)		/* End of list */
+#define DFH_VERSION		GENMASK_ULL(59, 52)	/* DFH version */
 #define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
 
+/*
+ *  CSR Rel Bit, 1'b0 = relative (offset from feature DFH start),
+ * 1'b1 = absolute (ARM or other non-PCIe use)
+ */
+#define DFHv1_CSR_ADDR_REL	BIT_ULL(0)
+
+/*
+ * CSR Header Register Bit Definitions
+ */
+#define DFHv1_CSR_ADDR_MASK       GENMASK_ULL(63, 1)  /* 63:1 of CSR address */
+
+/*
+ * CSR SIZE Goup Register Bit Definitions
+ */
+#define DFHv1_CSR_SIZE_GRP_INSTANCE_ID	GENMASK_ULL(15, 0)	/* Enumeration instantiated IP */
+#define DFHv1_CSR_SIZE_GRP_GROUPING_ID	GENMASK_ULL(30, 16)	/* Group Features/interfaces */
+#define DFHv1_CSR_SIZE_GRP_HAS_PARAMS	BIT_ULL(31)		/* Presence of Parameters */
+#define DFHv1_CSR_SIZE_GRP_SIZE		GENMASK_ULL(63, 32)	/* Size of CSR Block in bytes */
+
+/*
+ * PARAM Header Register Bit Definitions
+ */
+#define DFHv1_PARAM_HDR_ID		GENMASK_ULL(15, 0) /* Id of this Param  */
+#define DFHv1_PARAM_HDR_VERSION		GENMASK_ULL(31, 16) /* Version Param */
+#define DFHv1_PARAM_HDR_NEXT_OFFSET	GENMASK_ULL(63, 32) /* Offset of next Param */
+
 /**
  * enum dfl_id_type - define the DFL FIU types
  */
-- 
2.25.1

[PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Define and use a DFHv1 parameter to add generic support for MSIX
interrupts for DFL devices.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/fpga/dfl.c  | 59 +++++++++++++++++++++++++++++++++------------
 include/linux/dfl.h | 13 ++++++++++
 2 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..17f704dc8483 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -941,25 +941,11 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
 	void __iomem *base = binfo->ioaddr + ofst;
 	unsigned int i, ibase, inr = 0;
 	enum dfl_id_type type;
-	int virq;
+	int virq, off;
 	u64 v;
 
 	type = feature_dev_id_type(binfo->feature_dev);
 
-	/*
-	 * Ideally DFL framework should only read info from DFL header, but
-	 * current version DFL only provides mmio resources information for
-	 * 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:
@@ -981,6 +967,28 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
 		}
 	}
 
+	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
+	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
+		v = readq(base);
+		v = FIELD_GET(DFH_VERSION, v);
+
+		if (v == 1) {
+			v =  readq(base + DFHv1_CSR_SIZE_GRP);
+			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
+				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
+						     DFHv1_PARAM_ID_MSIX);
+				if (off >= 0) {
+					ibase = readl(base + DFHv1_PARAM_HDR +
+						      off + DFHv1_PARAM_MSIX_STARTV);
+					inr = readl(base + DFHv1_PARAM_HDR +
+						    off + DFHv1_PARAM_MSIX_NUMV);
+					dev_dbg(binfo->dev, "%s start %d num %d fid 0x%x\n",
+						__func__, ibase, inr, fid);
+				}
+			}
+		}
+	}
+
 	if (!inr) {
 		*irq_base = 0;
 		*nr_irqs = 0;
@@ -1879,6 +1887,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
 
+int dfl_find_param(void __iomem *base, resource_size_t max, int param)
+{
+	int off = 0;
+	u64 v, next;
+
+	while (off < max) {
+		v = readq(base + off);
+		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+			return off;
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		if (!next)
+			break;
+
+		off += next;
+	}
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(dfl_find_param);
+
 static void __exit dfl_fpga_exit(void)
 {
 	dfl_chardev_uinit();
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 61bcf20c1bc8..5652879ab48e 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -69,6 +69,10 @@
 #define DFHv1_PARAM_HDR_VERSION		GENMASK_ULL(31, 16) /* Version Param */
 #define DFHv1_PARAM_HDR_NEXT_OFFSET	GENMASK_ULL(63, 32) /* Offset of next Param */
 
+#define DFHv1_PARAM_ID_MSIX	0x1
+#define DFHv1_PARAM_MSIX_STARTV	0x8
+#define DFHv1_PARAM_MSIX_NUMV	0xc
+
 /**
  * enum dfl_id_type - define the DFL FIU types
  */
@@ -142,4 +146,13 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
 	module_driver(__dfl_driver, dfl_driver_register, \
 		      dfl_driver_unregister)
 
+/*
+ * dfl_find_param() - find the offset of the given parameter
+ * @base: base pointer to start of dfl parameters in DFH
+ * @max: maximum offset to search
+ * @param: id of dfl parameter
+ *
+ * Return: positive offset on success, negative error code otherwise.
+ */
+int dfl_find_param(void __iomem *base, resource_size_t max, int param);
 #endif /* __LINUX_DFL_H */
-- 
2.25.1

[PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Add a Device Feature List (DFL) bus driver for the Altera
16550 implementation of UART.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dfl.c | 188 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |   9 ++
 drivers/tty/serial/8250/Makefile   |   1 +
 include/linux/dfl.h                |   7 ++
 4 files changed, 205 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..dcf6638a298c
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA UART
+ *
+ * Copyright (C) 2022 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Ananda Ravuri <ananda.ravuri@intel.com>
+ *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
+ */
+
+#include <linux/dfl.h>
+#include <linux/version.h>
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/bitfield.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+
+struct dfl_uart {
+	void __iomem   *csr_base;
+	u64             csr_addr;
+	unsigned int    csr_size;
+	struct device  *dev;
+	u64             uart_clk;
+	u64             fifo_len;
+	unsigned int    fifo_size;
+	unsigned int    reg_shift;
+	unsigned int    line;
+};
+
+int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
+{
+	void __iomem *param_base;
+	int off;
+	u64 v;
+
+	v = readq(dfluart->csr_base + DFHv1_CSR_ADDR);
+	dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+
+	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
+	dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
+
+	if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) {
+		dev_err(dfluart->dev, "FIXME bad dfh address and size\n");
+		return -EINVAL;
+	}
+
+	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
+		dev_err(dfluart->dev, "missing required parameters\n");
+		return -EINVAL;
+	}
+
+	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
+
+	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
+	if (off < 0) {
+		dev_err(dfluart->dev, "missing CLK_FRQ param\n");
+		return -EINVAL;
+	}
+
+	dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
+	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
+
+	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
+	if (off < 0) {
+		dev_err(dfluart->dev, "missing FIFO_LEN param\n");
+		return -EINVAL;
+	}
+
+	dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
+	dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
+
+	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
+	if (off < 0) {
+		dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
+		return -EINVAL;
+	}
+
+	v = readq(param_base + off + DFHv1_PARAM_DATA);
+	dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
+	dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
+	dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
+		dfluart->fifo_size, dfluart->reg_shift);
+
+	return 0;
+}
+
+static int dfl_uart_probe(struct dfl_device *dfl_dev)
+{
+	struct device *dev = &dfl_dev->dev;
+	struct uart_8250_port uart;
+	struct dfl_uart *dfluart;
+	int ret;
+
+	memset(&uart, 0, sizeof(uart));
+
+	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+	if (!dfluart)
+		return -ENOMEM;
+
+	dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
+	if (IS_ERR(dfluart->csr_base)) {
+		dev_err(dev, "failed to get mem resource!\n");
+		return PTR_ERR(dfluart->csr_base);
+	}
+
+	dfluart->dev = dev;
+
+	ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res));
+	if (ret < 0) {
+		dev_err(dev, "failed to uart feature walk %d\n", ret);
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
+
+	if (dfl_dev->num_irqs == 1)
+		uart.port.irq = dfl_dev->irqs[0];
+
+	switch (dfluart->fifo_len) {
+	case 32:
+		uart.port.type = PORT_ALTR_16550_F32;
+		break;
+
+	case 64:
+		uart.port.type = PORT_ALTR_16550_F64;
+		break;
+
+	case 128:
+		uart.port.type = PORT_ALTR_16550_F128;
+		break;
+
+	default:
+		dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len);
+		return -EINVAL;
+	}
+
+	uart.port.iotype = UPIO_MEM32;
+	uart.port.membase = dfluart->csr_base + dfluart->csr_addr;
+	uart.port.mapsize = dfluart->csr_size;
+	uart.port.regshift = dfluart->reg_shift;
+	uart.port.uartclk = dfluart->uart_clk;
+
+	/* register the port */
+	ret = serial8250_register_8250_port(&uart);
+	if (ret < 0) {
+		dev_err(dev, "unable to register 8250 port %d.\n", ret);
+		return -EINVAL;
+	}
+	dev_info(dev, "serial8250_register_8250_port %d\n", ret);
+	dfluart->line = ret;
+	dev_set_drvdata(dev, dfluart);
+
+	return 0;
+}
+
+static void dfl_uart_remove(struct dfl_device *dfl_dev)
+{
+	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
+
+	if (dfluart->line > 0)
+		serial8250_unregister_port(dfluart->line);
+}
+
+#define FME_FEATURE_ID_UART 0x24
+
+static const struct dfl_device_id dfl_uart_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_UART },
+	{ }
+};
+
+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_DEVICE_TABLE(dfl, dfl_uart_ids);
+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..fbb59216ce7f 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -546,3 +546,12 @@ config SERIAL_OF_PLATFORM
 	  are probed through devicetree, including Open Firmware based
 	  PowerPC systems and embedded systems on architectures using the
 	  flattened device tree format.
+
+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.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..8e987b04820a 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -43,5 +43,6 @@ obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
 obj-$(CONFIG_SERIAL_8250_TEGRA)		+= 8250_tegra.o
 obj-$(CONFIG_SERIAL_8250_BCM7271)	+= 8250_bcm7271.o
 obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
+obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
 
 CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 5652879ab48e..d37636090fed 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -73,6 +73,13 @@
 #define DFHv1_PARAM_MSIX_STARTV	0x8
 #define DFHv1_PARAM_MSIX_NUMV	0xc
 
+#define DFHv1_PARAM_ID_CLK_FRQ    0x2
+#define DFHv1_PARAM_ID_FIFO_LEN   0x3
+
+#define DFHv1_PARAM_ID_REG_LAYOUT 0x4
+#define DFHv1_PARAM_ID_REG_WIDTH  GENMASK_ULL(63, 32)
+#define DFHv1_PARAM_ID_REG_SHIFT  GENMASK_ULL(31, 0)
+
 /**
  * enum dfl_id_type - define the DFL FIU types
  */
-- 
2.25.1

Re: [PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 12:04:23PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> 
> Moving the DFH register offset and register definitions from
> drivers/fpga/dfl.h to include/linux/dfl.h.  These definitions

Single space?

> need to be accessed by dfl drivers that are outside of
> drivers/fpga.

...

> +#define DFH			0x0
> +#define GUID_L			0x8
> +#define GUID_H			0x10
> +#define NEXT_AFU		0x18

While at it, you may make them same width, like "0x08".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] Documentation: fpga: dfl: Add documentation for DFHv1

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 12:04:22PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add documentation describing the extentions provided by Version
> 1 of the Device Feature Header (DFHv1).

...

> +With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUILD standard
> +across all types.

GUI_L_D?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 12:04:25PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.

...

> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +		v = readq(base);
> +		v = FIELD_GET(DFH_VERSION, v);
> +
> +		if (v == 1) {
> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);

I am already lost what v keeps...

Perhaps

		v = readq(base);
		switch (FIELD_GET(DFH_VERSION, v)) {
		case 1:
			...
			break;
		}

> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {

				void __iomem *v1hdr = base + DFHv1_PARAM_HDR;

> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> +						     DFHv1_PARAM_ID_MSIX);

				off = dfl_find_param(v1hdr, ofst, DFHv1_PARAM_ID_MSIX);

> +				if (off >= 0) {
> +					ibase = readl(base + DFHv1_PARAM_HDR +
> +						      off + DFHv1_PARAM_MSIX_STARTV);
> +					inr = readl(base + DFHv1_PARAM_HDR +
> +						    off + DFHv1_PARAM_MSIX_NUMV);

					ibase = readl(v1hdr + off + DFHv1_PARAM_MSIX_STARTV);
					inr = readl(v1hdr + off + DFHv1_PARAM_MSIX_NUMV);

> +					dev_dbg(binfo->dev, "%s start %d num %d fid 0x%x\n",
> +						__func__, ibase, inr, fid);

No __func__ for dev_dbg(). Dynamic debug has this feature at runtime!

> +				}
> +			}
> +		}
> +	}

...

> +/*

If it's a kernel doc, make it to be parsable as a such.

> + * dfl_find_param() - find the offset of the given parameter
> + * @base: base pointer to start of dfl parameters in DFH
> + * @max: maximum offset to search
> + * @param: id of dfl parameter
> + *
> + * Return: positive offset on success, negative error code otherwise.
> + */
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);

+ blank line.

>  #endif /* __LINUX_DFL_H */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 12:04:26PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.

...

> +#include <linux/dfl.h>

> +#include <linux/version.h>

Hmm... Do we need this?

> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/bitfield.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>

Can this block be sorted alphabetically?

...

> +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
> +{
> +	void __iomem *param_base;
> +	int off;
> +	u64 v;
> +
> +	v = readq(dfluart->csr_base + DFHv1_CSR_ADDR);
> +	dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
> +	dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
> +
> +	if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) {
> +		dev_err(dfluart->dev, "FIXME bad dfh address and size\n");

DFH ?

> +		return -EINVAL;
> +	}
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> +		dev_err(dfluart->dev, "missing required parameters\n");

Not sure I understood what parameters are here. FPGA VHDL? Configuration? RTL?

> +		return -EINVAL;
> +	}
> +
> +	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
> +
> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing CLK_FRQ param\n");
> +		return -EINVAL;
> +	}
> +
> +	dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);

> +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);

Isn't this available via normal interfaces to user?

> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing FIFO_LEN param\n");
> +		return -EINVAL;
> +	}
> +
> +	dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
> +	dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
> +
> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
> +		return -EINVAL;
> +	}
> +
> +	v = readq(param_base + off + DFHv1_PARAM_DATA);
> +	dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> +	dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> +	dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
> +		dfluart->fifo_size, dfluart->reg_shift);
> +
> +	return 0;
> +}
> +
> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	struct uart_8250_port uart;
> +	struct dfl_uart *dfluart;
> +	int ret;
> +
> +	memset(&uart, 0, sizeof(uart));
> +
> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> +	if (!dfluart)
> +		return -ENOMEM;
> +
> +	dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> +	if (IS_ERR(dfluart->csr_base)) {

> +		dev_err(dev, "failed to get mem resource!\n");

The above call have a few different messages depending on error code.
No need to repeat this.

> +		return PTR_ERR(dfluart->csr_base);
> +	}
> +
> +	dfluart->dev = dev;
> +
> +	ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res));
> +	if (ret < 0) {
> +		dev_err(dev, "failed to uart feature walk %d\n", ret);

> +		return -EINVAL;

Why shadowing error code?
What about

	return dev_err_probe(dev, ret, ...);
?

> +	}
> +
> +	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
> +
> +	if (dfl_dev->num_irqs == 1)
> +		uart.port.irq = dfl_dev->irqs[0];
> +
> +	switch (dfluart->fifo_len) {
> +	case 32:
> +		uart.port.type = PORT_ALTR_16550_F32;
> +		break;
> +
> +	case 64:
> +		uart.port.type = PORT_ALTR_16550_F64;
> +		break;
> +
> +	case 128:
> +		uart.port.type = PORT_ALTR_16550_F128;
> +		break;
> +
> +	default:
> +		dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len);
> +		return -EINVAL;
> +	}
> +
> +	uart.port.iotype = UPIO_MEM32;
> +	uart.port.membase = dfluart->csr_base + dfluart->csr_addr;
> +	uart.port.mapsize = dfluart->csr_size;
> +	uart.port.regshift = dfluart->reg_shift;
> +	uart.port.uartclk = dfluart->uart_clk;
> +
> +	/* register the port */
> +	ret = serial8250_register_8250_port(&uart);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to register 8250 port %d.\n", ret);
> +		return -EINVAL;
> +	}
> +	dev_info(dev, "serial8250_register_8250_port %d\n", ret);
> +	dfluart->line = ret;
> +	dev_set_drvdata(dev, dfluart);
> +
> +	return 0;
> +}
> +
> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
> +{
> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
> +
> +	if (dfluart->line > 0)
> +		serial8250_unregister_port(dfluart->line);
> +}

...

> +#define FME_FEATURE_ID_UART 0x24

Purpose of this definition? For me with or without is still an ID.

> +static const struct dfl_device_id dfl_uart_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_UART },
> +	{ }
> +};

...

> +static struct dfl_driver dfl_uart_driver = {
> +	.drv = {
> +		.name = "dfl-uart",
> +	},
> +	.id_table = dfl_uart_ids,
> +	.probe = dfl_uart_probe,
> +	.remove = dfl_uart_remove,
> +};

> +

No need to have this blank line.

> +module_dfl_driver(dfl_uart_driver);

...

> +MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);

Move this closer to the definition. That's how other drivers do in the kernel.

...

> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig

> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile

I know that the records in those files are not sorted, but can you try hard
to find the best place for them in those files from sorting point of view?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[Greg KH]

On Tue, Sep 06, 2022 at 12:04:23PM -0700, matthew.gerlach@linux.intel.com wrote:
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -2,7 +2,7 @@
>  /*
>   * Driver Header File for FPGA Device Feature List (DFL) Support
>   *
> - * Copyright (C) 2017-2018 Intel Corporation, Inc.
> + * Copyright (C) 2017-2022 Intel Corporation, Inc.

I'm all for updated proper copyright dates, but in a patch that
_removes_ text from a file does not seem like the proper place for that,
right?  Please discuss with your corporate lawyers as to how to do this
properly and when to do it.

thanks,

greg k-h

Re: [PATCH v1 1/5] Documentation: fpga: dfl: Add documentation for DFHv1

[matthew.gerlach]

On Tue, 6 Sep 2022, Andy Shevchenko wrote:

> On Tue, Sep 06, 2022 at 12:04:22PM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add documentation describing the extentions provided by Version
>> 1 of the Device Feature Header (DFHv1).
>
> ...
>
>> +With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUILD standard
>> +across all types.
>
> GUI_L_D?
>

Thanks for the review and pointing out the typo.

Matthew Gerlach
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[matthew.gerlach]

On Tue, 6 Sep 2022, Andy Shevchenko wrote:

> On Tue, Sep 06, 2022 at 12:04:23PM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>>
>> Moving the DFH register offset and register definitions from
>> drivers/fpga/dfl.h to include/linux/dfl.h.  These definitions
>
> Single space?

Sorry for the old habit. I will make it one space.

>
>> need to be accessed by dfl drivers that are outside of
>> drivers/fpga.
>
> ...
>
>> +#define DFH			0x0
>> +#define GUID_L			0x8
>> +#define GUID_H			0x10
>> +#define NEXT_AFU		0x18
>
> While at it, you may make them same width, like "0x08".

The same width does look better.  Thanks for the suggestion.

Matthew Gerlach
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

On Tue, 6 Sep 2022, Andy Shevchenko wrote:

> On Tue, Sep 06, 2022 at 12:04:25PM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>
> ...
>
>> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>> +		v = readq(base);
>> +		v = FIELD_GET(DFH_VERSION, v);
>> +
>> +		if (v == 1) {
>> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);
>
> I am already lost what v keeps...
>
> Perhaps
>
> 		v = readq(base);
> 		switch (FIELD_GET(DFH_VERSION, v)) {
> 		case 1:
> 			...
> 			break;
> 		}

How about?
 		if (FIELD_GET(DFH_VERSION, readq(base)) == 1) {
 			...
 		}
>
>> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>
> 				void __iomem *v1hdr = base + DFHv1_PARAM_HDR;
>
>> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> +						     DFHv1_PARAM_ID_MSIX);
>
> 				off = dfl_find_param(v1hdr, ofst, DFHv1_PARAM_ID_MSIX);
>
>> +				if (off >= 0) {
>> +					ibase = readl(base + DFHv1_PARAM_HDR +
>> +						      off + DFHv1_PARAM_MSIX_STARTV);
>> +					inr = readl(base + DFHv1_PARAM_HDR +
>> +						    off + DFHv1_PARAM_MSIX_NUMV);
>
> 					ibase = readl(v1hdr + off + DFHv1_PARAM_MSIX_STARTV);
> 					inr = readl(v1hdr + off + DFHv1_PARAM_MSIX_NUMV);
>
>> +					dev_dbg(binfo->dev, "%s start %d num %d fid 0x%x\n",
>> +						__func__, ibase, inr, fid);
>
> No __func__ for dev_dbg(). Dynamic debug has this feature at runtime!
>
>> +				}
>> +			}
>> +		}
>> +	}
>
> ...
>
>> +/*
>
> If it's a kernel doc, make it to be parsable as a such.

Yes, the intention is kernel doc. Thanks for the feedback. I
will fix this and add the newline as suggested below.

Matthew Gerlach

>
>> + * dfl_find_param() - find the offset of the given parameter
>> + * @base: base pointer to start of dfl parameters in DFH
>> + * @max: maximum offset to search
>> + * @param: id of dfl parameter
>> + *
>> + * Return: positive offset on success, negative error code otherwise.
>> + */
>> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);
>
> + blank line.
>
>>  #endif /* __LINUX_DFL_H */
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[Andy Shevchenko]

On Wed, Sep 07, 2022 at 02:37:32PM -0700, matthew.gerlach@linux.intel.com wrote:
> On Tue, 6 Sep 2022, Andy Shevchenko wrote:
> > On Tue, Sep 06, 2022 at 12:04:25PM -0700, matthew.gerlach@linux.intel.com wrote:

...

> > > +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> > > +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> > > +		v = readq(base);
> > > +		v = FIELD_GET(DFH_VERSION, v);
> > > +
> > > +		if (v == 1) {
> > > +			v =  readq(base + DFHv1_CSR_SIZE_GRP);
> > 
> > I am already lost what v keeps...
> > 
> > Perhaps
> > 
> > 		v = readq(base);
> > 		switch (FIELD_GET(DFH_VERSION, v)) {
> > 		case 1:
> > 			...
> > 			break;
> > 		}
> 
> How about?
> 		if (FIELD_GET(DFH_VERSION, readq(base)) == 1) {
> 			...
> 		}

This one tends to be expanded in the future, so I would keep it switch case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

On Thu, 8 Sep 2022, Andy Shevchenko wrote:

> On Wed, Sep 07, 2022 at 02:37:32PM -0700, matthew.gerlach@linux.intel.com wrote:
>> On Tue, 6 Sep 2022, Andy Shevchenko wrote:
>>> On Tue, Sep 06, 2022 at 12:04:25PM -0700, matthew.gerlach@linux.intel.com wrote:
>
> ...
>
>>>> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>>>> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>>>> +		v = readq(base);
>>>> +		v = FIELD_GET(DFH_VERSION, v);
>>>> +
>>>> +		if (v == 1) {
>>>> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);
>>>
>>> I am already lost what v keeps...
>>>
>>> Perhaps
>>>
>>> 		v = readq(base);
>>> 		switch (FIELD_GET(DFH_VERSION, v)) {
>>> 		case 1:
>>> 			...
>>> 			break;
>>> 		}
>>
>> How about?
>> 		if (FIELD_GET(DFH_VERSION, readq(base)) == 1) {
>> 			...
>> 		}
>
> This one tends to be expanded in the future, so I would keep it switch case.
>

I'm okay with using the switch statement, but how about the following?

 		switch (FIELD_GET(DFH_VERSION, readq(base))) {
                 case 1:
 			...
 			break;
 		}
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[Andy Shevchenko]

On Thu, Sep 08, 2022 at 10:34:07AM -0700, matthew.gerlach@linux.intel.com wrote:
> On Thu, 8 Sep 2022, Andy Shevchenko wrote:
> > On Wed, Sep 07, 2022 at 02:37:32PM -0700, matthew.gerlach@linux.intel.com wrote:

...

> > This one tends to be expanded in the future, so I would keep it switch case.
> 
> I'm okay with using the switch statement, but how about the following?
> 
> 		switch (FIELD_GET(DFH_VERSION, readq(base))) {
>                 case 1:
> 			...
> 			break;
> 		}

Fine to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

On Tue, 6 Sep 2022, Andy Shevchenko wrote:

> On Tue, Sep 06, 2022 at 12:04:26PM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>
> ...
>
>> +#include <linux/dfl.h>
>
>> +#include <linux/version.h>
>
> Hmm... Do we need this?

We do not need this.

>
>> +#include <linux/serial.h>
>> +#include <linux/serial_8250.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/io-64-nonatomic-lo-hi.h>
>
> Can this block be sorted alphabetically?

Yes, they can and should be sorted alphabetically.

>
> ...
>
>> +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
>> +{
>> +	void __iomem *param_base;
>> +	int off;
>> +	u64 v;
>> +
>> +	v = readq(dfluart->csr_base + DFHv1_CSR_ADDR);
>> +	dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> +	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
>> +	dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
>> +
>> +	if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) {
>> +		dev_err(dfluart->dev, "FIXME bad dfh address and size\n");
>
> DFH ?

Yes,"DFH" is better than "dfh" in the message.

>
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> +		dev_err(dfluart->dev, "missing required parameters\n");
>
> Not sure I understood what parameters are here. FPGA VHDL? Configuration? RTL?

How about "missing required DFH parameters\n"?

>
>> +		return -EINVAL;
>> +	}
>> +
>> +	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
>> +
>> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
>> +	if (off < 0) {
>> +		dev_err(dfluart->dev, "missing CLK_FRQ param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
>
>> +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
>
> Isn't this available via normal interfaces to user?

I am not sure what "normal interfaces to user" you are referring to.  The 
code is just trying to read the frequency of the input clock to the uart 
from a DFH paramter.

>
>> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
>> +	if (off < 0) {
>> +		dev_err(dfluart->dev, "missing FIFO_LEN param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
>> +	dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
>> +
>> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
>> +	if (off < 0) {
>> +		dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	v = readq(param_base + off + DFHv1_PARAM_DATA);
>> +	dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>> +	dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>> +	dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
>> +		dfluart->fifo_size, dfluart->reg_shift);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
>> +{
>> +	struct device *dev = &dfl_dev->dev;
>> +	struct uart_8250_port uart;
>> +	struct dfl_uart *dfluart;
>> +	int ret;
>> +
>> +	memset(&uart, 0, sizeof(uart));
>> +
>> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
>> +	if (!dfluart)
>> +		return -ENOMEM;
>> +
>> +	dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> +	if (IS_ERR(dfluart->csr_base)) {
>
>> +		dev_err(dev, "failed to get mem resource!\n");
>
> The above call have a few different messages depending on error code.
> No need to repeat this.

I will remove the call to dev_err().
>
>> +		return PTR_ERR(dfluart->csr_base);
>> +	}
>> +
>> +	dfluart->dev = dev;
>> +
>> +	ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res));
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to uart feature walk %d\n", ret);
>
>> +		return -EINVAL;
>
> Why shadowing error code?
> What about
>
> 	return dev_err_probe(dev, ret, ...);
> ?
>

Using dev_err_probe seems like the way to go.

>> +	}
>> +
>> +	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
>> +
>> +	if (dfl_dev->num_irqs == 1)
>> +		uart.port.irq = dfl_dev->irqs[0];
>> +
>> +	switch (dfluart->fifo_len) {
>> +	case 32:
>> +		uart.port.type = PORT_ALTR_16550_F32;
>> +		break;
>> +
>> +	case 64:
>> +		uart.port.type = PORT_ALTR_16550_F64;
>> +		break;
>> +
>> +	case 128:
>> +		uart.port.type = PORT_ALTR_16550_F128;
>> +		break;
>> +
>> +	default:
>> +		dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len);
>> +		return -EINVAL;
>> +	}
>> +
>> +	uart.port.iotype = UPIO_MEM32;
>> +	uart.port.membase = dfluart->csr_base + dfluart->csr_addr;
>> +	uart.port.mapsize = dfluart->csr_size;
>> +	uart.port.regshift = dfluart->reg_shift;
>> +	uart.port.uartclk = dfluart->uart_clk;
>> +
>> +	/* register the port */
>> +	ret = serial8250_register_8250_port(&uart);
>> +	if (ret < 0) {
>> +		dev_err(dev, "unable to register 8250 port %d.\n", ret);
>> +		return -EINVAL;
>> +	}
>> +	dev_info(dev, "serial8250_register_8250_port %d\n", ret);
>> +	dfluart->line = ret;
>> +	dev_set_drvdata(dev, dfluart);
>> +
>> +	return 0;
>> +}
>> +
>> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
>> +{
>> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
>> +
>> +	if (dfluart->line > 0)
>> +		serial8250_unregister_port(dfluart->line);
>> +}
>
> ...
>
>> +#define FME_FEATURE_ID_UART 0x24
>
> Purpose of this definition? For me with or without is still an ID.

I don't think I understand the question. Is the name of the macro unclear, 
or do you think it is not necessary?

>
>> +static const struct dfl_device_id dfl_uart_ids[] = {
>> +	{ FME_ID, FME_FEATURE_ID_UART },
>> +	{ }
>> +};
>
> ...
>
>> +static struct dfl_driver dfl_uart_driver = {
>> +	.drv = {
>> +		.name = "dfl-uart",
>> +	},
>> +	.id_table = dfl_uart_ids,
>> +	.probe = dfl_uart_probe,
>> +	.remove = dfl_uart_remove,
>> +};
>
>> +
>
> No need to have this blank line.

I will remove the blank line.

>
>> +module_dfl_driver(dfl_uart_driver);
>
> ...
>
>> +MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
>
> Move this closer to the definition. That's how other drivers do in the kernel.

Thanks for the suggestion.

>
> ...
>
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>
>> --- a/drivers/tty/serial/8250/Makefile
>> +++ b/drivers/tty/serial/8250/Makefile
>
> I know that the records in those files are not sorted, but can you try hard
> to find the best place for them in those files from sorting point of view?
>

I will try to find a better place from sorting porint of view.

> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[Geert Uytterhoeven]

Hi Matthew,

On Thu, Sep 8, 2022 at 7:34 PM <matthew.gerlach@linux.intel.com> wrote:
> On Thu, 8 Sep 2022, Andy Shevchenko wrote:
> > On Wed, Sep 07, 2022 at 02:37:32PM -0700, matthew.gerlach@linux.intel.com wrote:
> >> On Tue, 6 Sep 2022, Andy Shevchenko wrote:
> >>> On Tue, Sep 06, 2022 at 12:04:25PM -0700, matthew.gerlach@linux.intel.com wrote:
> >
> > ...
> >
> >>>> +  if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> >>>> +      fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> >>>> +          v = readq(base);
> >>>> +          v = FIELD_GET(DFH_VERSION, v);
> >>>> +
> >>>> +          if (v == 1) {
> >>>> +                  v =  readq(base + DFHv1_CSR_SIZE_GRP);
> >>>
> >>> I am already lost what v keeps...
> >>>
> >>> Perhaps
> >>>
> >>>             v = readq(base);
> >>>             switch (FIELD_GET(DFH_VERSION, v)) {
> >>>             case 1:
> >>>                     ...
> >>>                     break;
> >>>             }
> >>
> >> How about?
> >>              if (FIELD_GET(DFH_VERSION, readq(base)) == 1) {
> >>                      ...
> >>              }
> >
> > This one tends to be expanded in the future, so I would keep it switch case.
> >
>
> I'm okay with using the switch statement, but how about the following?
>
>                 switch (FIELD_GET(DFH_VERSION, readq(base))) {
>                  case 1:
>                         ...
>                         break;
>                 }

Would it make sense to print an error if a newer version than 1 is detected?
BTW, what is the expected value when DFHv1 is not detected? Zero
or an arbitrary number?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

On Thu, 8 Sep 2022, Geert Uytterhoeven wrote:

> Hi Matthew,
>
> On Thu, Sep 8, 2022 at 7:34 PM <matthew.gerlach@linux.intel.com> wrote:
>> On Thu, 8 Sep 2022, Andy Shevchenko wrote:
>>> On Wed, Sep 07, 2022 at 02:37:32PM -0700, matthew.gerlach@linux.intel.com wrote:
>>>> On Tue, 6 Sep 2022, Andy Shevchenko wrote:
>>>>> On Tue, Sep 06, 2022 at 12:04:25PM -0700, matthew.gerlach@linux.intel.com wrote:
>>>
>>> ...
>>>
>>>>>> +  if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>>>>>> +      fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>>>>>> +          v = readq(base);
>>>>>> +          v = FIELD_GET(DFH_VERSION, v);
>>>>>> +
>>>>>> +          if (v == 1) {
>>>>>> +                  v =  readq(base + DFHv1_CSR_SIZE_GRP);
>>>>>
>>>>> I am already lost what v keeps...
>>>>>
>>>>> Perhaps
>>>>>
>>>>>             v = readq(base);
>>>>>             switch (FIELD_GET(DFH_VERSION, v)) {
>>>>>             case 1:
>>>>>                     ...
>>>>>                     break;
>>>>>             }
>>>>
>>>> How about?
>>>>              if (FIELD_GET(DFH_VERSION, readq(base)) == 1) {
>>>>                      ...
>>>>              }
>>>
>>> This one tends to be expanded in the future, so I would keep it switch case.
>>>
>>
>> I'm okay with using the switch statement, but how about the following?
>>
>>                 switch (FIELD_GET(DFH_VERSION, readq(base))) {
>>                  case 1:
>>                         ...
>>                         break;
>>                 }
>
> Would it make sense to print an error if a newer version than 1 is detected?
> BTW, what is the expected value when DFHv1 is not detected? Zero
> or an arbitrary number?
>
> Gr{oetje,eeting}s,
>
>                        Geert
>

Hi Geert,

Currently, DFHs that are not version 1 should be version 0. I will fill in 
the switch statement to do nothing for version 0, and the default case 
will print a warning of an unexpected version.

Thanks for the feedback.

Matthew Gerlach
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds
>

Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Thu, Sep 08, 2022 at 11:27:03AM -0700, matthew.gerlach@linux.intel.com wrote:
> On Tue, 6 Sep 2022, Andy Shevchenko wrote:
> > On Tue, Sep 06, 2022 at 12:04:26PM -0700, matthew.gerlach@linux.intel.com wrote:

...

> > > +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
> > 
> > Isn't this available via normal interfaces to user?
> 
> I am not sure what "normal interfaces to user" you are referring to.  The
> code is just trying to read the frequency of the input clock to the uart
> from a DFH paramter.

I mean dev_dbg() call. The user can get uart_clk via one of the UART/serial
ABIs (don't remember which one, though).


...

> > > +#define FME_FEATURE_ID_UART 0x24
> > 
> > Purpose of this definition? For me with or without is still an ID.
> 
> I don't think I understand the question. Is the name of the macro unclear,
> or do you think it is not necessary?

I mean how the definition is useful / useless. I.o.w. I think it's not
necessary.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[Xu Yilun]

On 2022-09-06 at 12:04:23 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> 
> Moving the DFH register offset and register definitions from
> drivers/fpga/dfl.h to include/linux/dfl.h.  These definitions
> need to be accessed by dfl drivers that are outside of
> drivers/fpga.
> 
> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/fpga/dfl.h  | 22 ++--------------------
>  include/linux/dfl.h | 23 ++++++++++++++++++++++-
>  2 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 06cfcd5e84bb..d4dfc03a0b61 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -2,7 +2,7 @@
>  /*
>   * Driver Header File for FPGA Device Feature List (DFL) Support
>   *
> - * Copyright (C) 2017-2018 Intel Corporation, Inc.
> + * Copyright (C) 2017-2022 Intel Corporation, Inc.
>   *
>   * Authors:
>   *   Kang Luwei <luwei.kang@intel.com>
> @@ -17,6 +17,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/cdev.h>
>  #include <linux/delay.h>
> +#include <linux/dfl.h>
>  #include <linux/eventfd.h>
>  #include <linux/fs.h>
>  #include <linux/interrupt.h>
> @@ -53,28 +54,9 @@
>  #define PORT_FEATURE_ID_UINT		0x12
>  #define PORT_FEATURE_ID_STP		0x13
>  
> -/*
> - * Device Feature Header Register Set
> - *
> - * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> - * For AFUs, they have DFH + GUID as common header registers.
> - * For private features, they only have DFH register as common header.
> - */
> -#define DFH			0x0
> -#define GUID_L			0x8
> -#define GUID_H			0x10
> -#define NEXT_AFU		0x18
> -
> -#define DFH_SIZE		0x8
> -
>  /* Device Feature Header Register Bitfield */
> -#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
>  #define DFH_ID_FIU_FME		0
>  #define DFH_ID_FIU_PORT		1
> -#define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
> -#define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
> -#define DFH_EOL			BIT_ULL(40)		/* End of list */
> -#define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>  #define DFH_TYPE_AFU		1
>  #define DFH_TYPE_PRIVATE	3
>  #define DFH_TYPE_FIU		4
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 431636a0dc78..b5accdcfa368 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -2,7 +2,7 @@
>  /*
>   * Header file for DFL driver and device API
>   *
> - * Copyright (C) 2020 Intel Corporation, Inc.
> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
>   */
>  
>  #ifndef __LINUX_DFL_H
> @@ -11,6 +11,27 @@
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
>  
> +/*
> + * Device Feature Header Register Set
> + *
> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> + * For AFUs, they have DFH + GUID as common header registers.
> + * For private features, they only have DFH register as common header.
> + */
> +#define DFH			0x0
> +#define GUID_L			0x8
> +#define GUID_H			0x10
> +#define NEXT_AFU		0x18

Now these macros are accessible in global kernel, should we add the
DFL_ or DFH_ prefix for them?

Thanks,
Yilun

> +
> +#define DFH_SIZE		0x8
> +
> +/* Device Feature Header Register Bitfield */
> +#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
> +#define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
> +#define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
> +#define DFH_EOL			BIT_ULL(40)		/* End of list */
> +#define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
> +
>  /**
>   * enum dfl_id_type - define the DFL FIU types
>   */
> -- 
> 2.25.1
> 

Re: [PATCH v1 3/5] fpga: dfl: Add DFHv1 Register Definitions

[Xu Yilun]

On 2022-09-06 at 12:04:24 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> 
> This patch adds the definitions for DFHv1 header and related register
> bitfields.
> 
> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  include/linux/dfl.h | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index b5accdcfa368..61bcf20c1bc8 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -23,6 +23,16 @@
>  #define GUID_H			0x10
>  #define NEXT_AFU		0x18
>  
> +/*
> + * DFHv1 Register Offset definitons
> + * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
> + * as common header registers
> + */
> +#define DFHv1_CSR_ADDR		0x18  /* CSR Register start address */
> +#define DFHv1_CSR_SIZE_GRP	0x20  /* Size of Reg Block and Group/tag */
> +#define DFHv1_PARAM_HDR		0x28  /* Optional First Param header */
> +#define DFHv1_PARAM_DATA	0x8   /* Offset of Param data from Param header */
> +
>  #define DFH_SIZE		0x8
>  
>  /* Device Feature Header Register Bitfield */
> @@ -30,8 +40,35 @@
>  #define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
>  #define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
>  #define DFH_EOL			BIT_ULL(40)		/* End of list */
> +#define DFH_VERSION		GENMASK_ULL(59, 52)	/* DFH version */
>  #define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>  
> +/*
> + *  CSR Rel Bit, 1'b0 = relative (offset from feature DFH start),

Reduce one whitespace indent.

> + * 1'b1 = absolute (ARM or other non-PCIe use)
> + */
> +#define DFHv1_CSR_ADDR_REL	BIT_ULL(0)
> +
> +/*
> + * CSR Header Register Bit Definitions
> + */

Use oneline style comment should be OK?

> +#define DFHv1_CSR_ADDR_MASK       GENMASK_ULL(63, 1)  /* 63:1 of CSR address */
> +
> +/*
> + * CSR SIZE Goup Register Bit Definitions
> + */

Same concern

> +#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
> + */

Same

> +#define DFHv1_PARAM_HDR_ID		GENMASK_ULL(15, 0) /* Id of this Param  */
> +#define DFHv1_PARAM_HDR_VERSION		GENMASK_ULL(31, 16) /* Version Param */
> +#define DFHv1_PARAM_HDR_NEXT_OFFSET	GENMASK_ULL(63, 32) /* Offset of next Param */
> +
>  /**
>   * enum dfl_id_type - define the DFL FIU types
>   */
> -- 
> 2.25.1
> 

Re: [PATCH v1 4/5] fpga: dfl: add generic support for MSIX interrupts

[Xu Yilun]

On 2022-09-06 at 12:04:25 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/fpga/dfl.c  | 59 +++++++++++++++++++++++++++++++++------------
>  include/linux/dfl.h | 13 ++++++++++
>  2 files changed, 57 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b9aae85ba930..17f704dc8483 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -941,25 +941,11 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>  	void __iomem *base = binfo->ioaddr + ofst;
>  	unsigned int i, ibase, inr = 0;
>  	enum dfl_id_type type;
> -	int virq;
> +	int virq, off;
>  	u64 v;
>  
>  	type = feature_dev_id_type(binfo->feature_dev);
>  
> -	/*
> -	 * Ideally DFL framework should only read info from DFL header, but
> -	 * current version DFL only provides mmio resources information for
> -	 * 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.
> -	 */

I don't think we should just remove the entire comments here, we still
need these quirks for DFHv0.

>  	if (type == PORT_ID) {
>  		switch (fid) {
>  		case PORT_FEATURE_ID_UINT:
> @@ -981,6 +967,28 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>  		}
>  	}
>  
> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {

Is it possible we don't list all quirks again?

> +		v = readq(base);
> +		v = FIELD_GET(DFH_VERSION, v);
> +
> +		if (v == 1) {
> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);
> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> +						     DFHv1_PARAM_ID_MSIX);
> +				if (off >= 0) {
> +					ibase = readl(base + DFHv1_PARAM_HDR +
> +						      off + DFHv1_PARAM_MSIX_STARTV);
> +					inr = readl(base + DFHv1_PARAM_HDR +
> +						    off + DFHv1_PARAM_MSIX_NUMV);
> +					dev_dbg(binfo->dev, "%s start %d num %d fid 0x%x\n",
> +						__func__, ibase, inr, fid);
> +				}
> +			}
> +		}
> +	}

Please help describe how the new irq params works with existing irq
capable features. Some possible cases?

If version = v1, has irq param, no feature quirk.
If version = v1, no irq param, has feature quirk.
If version = v1, has irq param, has feature quirk.

Thanks,
Yilun

> +
>  	if (!inr) {
>  		*irq_base = 0;
>  		*nr_irqs = 0;
> @@ -1879,6 +1887,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>  
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param)
> +{
> +	int off = 0;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = readq(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return off;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!next)
> +			break;
> +
> +		off += next;
> +	}
> +
> +	return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(dfl_find_param);
> +
>  static void __exit dfl_fpga_exit(void)
>  {
>  	dfl_chardev_uinit();
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 61bcf20c1bc8..5652879ab48e 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -69,6 +69,10 @@
>  #define DFHv1_PARAM_HDR_VERSION		GENMASK_ULL(31, 16) /* Version Param */
>  #define DFHv1_PARAM_HDR_NEXT_OFFSET	GENMASK_ULL(63, 32) /* Offset of next Param */
>  
> +#define DFHv1_PARAM_ID_MSIX	0x1
> +#define DFHv1_PARAM_MSIX_STARTV	0x8
> +#define DFHv1_PARAM_MSIX_NUMV	0xc
> +
>  /**
>   * enum dfl_id_type - define the DFL FIU types
>   */
> @@ -142,4 +146,13 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>  	module_driver(__dfl_driver, dfl_driver_register, \
>  		      dfl_driver_unregister)
>  
> +/*
> + * dfl_find_param() - find the offset of the given parameter
> + * @base: base pointer to start of dfl parameters in DFH
> + * @max: maximum offset to search
> + * @param: id of dfl parameter
> + *
> + * Return: positive offset on success, negative error code otherwise.
> + */
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);
>  #endif /* __LINUX_DFL_H */
> -- 
> 2.25.1
> 

Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Xu Yilun]

On 2022-09-06 at 12:04:26 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dfl.c | 188 +++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig    |   9 ++
>  drivers/tty/serial/8250/Makefile   |   1 +
>  include/linux/dfl.h                |   7 ++
>  4 files changed, 205 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..dcf6638a298c
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA UART
> + *
> + * Copyright (C) 2022 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Ananda Ravuri <ananda.ravuri@intel.com>
> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> + */
> +
> +#include <linux/dfl.h>
> +#include <linux/version.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/bitfield.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +
> +struct dfl_uart {
> +	void __iomem   *csr_base;
> +	u64             csr_addr;
> +	unsigned int    csr_size;
> +	struct device  *dev;
> +	u64             uart_clk;
> +	u64             fifo_len;
> +	unsigned int    fifo_size;
> +	unsigned int    reg_shift;
> +	unsigned int    line;
> +};
> +
> +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
> +{
> +	void __iomem *param_base;
> +	int off;
> +	u64 v;
> +
> +	v = readq(dfluart->csr_base + DFHv1_CSR_ADDR);
> +	dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
> +	dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);

These are generic for DFHv1, so maybe we parse them in DFL generic code.

> +
> +	if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) {
> +		dev_err(dfluart->dev, "FIXME bad dfh address and size\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> +		dev_err(dfluart->dev, "missing required parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;

The same concern.

> +
> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing CLK_FRQ param\n");
> +		return -EINVAL;
> +	}
> +
> +	dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
> +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);

I see the DFHv1_PARAM_ID_CLK_FRQ defined in generic dfl.h, is this
param definition global to all features, or specific to uart?

Do we have clear definition of generic parameters vs feature specific
parameters?

The concern here is to avoid duplicated parameter parsing for each driver.

Thanks,
Yilun

> +
> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing FIFO_LEN param\n");
> +		return -EINVAL;
> +	}
> +
> +	dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
> +	dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
> +
> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
> +		return -EINVAL;
> +	}
> +
> +	v = readq(param_base + off + DFHv1_PARAM_DATA);
> +	dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> +	dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> +	dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
> +		dfluart->fifo_size, dfluart->reg_shift);
> +
> +	return 0;
> +}
> +
> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	struct uart_8250_port uart;
> +	struct dfl_uart *dfluart;
> +	int ret;
> +
> +	memset(&uart, 0, sizeof(uart));
> +
> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> +	if (!dfluart)
> +		return -ENOMEM;
> +
> +	dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> +	if (IS_ERR(dfluart->csr_base)) {
> +		dev_err(dev, "failed to get mem resource!\n");
> +		return PTR_ERR(dfluart->csr_base);
> +	}
> +
> +	dfluart->dev = dev;
> +
> +	ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res));
> +	if (ret < 0) {
> +		dev_err(dev, "failed to uart feature walk %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
> +
> +	if (dfl_dev->num_irqs == 1)
> +		uart.port.irq = dfl_dev->irqs[0];
> +
> +	switch (dfluart->fifo_len) {
> +	case 32:
> +		uart.port.type = PORT_ALTR_16550_F32;
> +		break;
> +
> +	case 64:
> +		uart.port.type = PORT_ALTR_16550_F64;
> +		break;
> +
> +	case 128:
> +		uart.port.type = PORT_ALTR_16550_F128;
> +		break;
> +
> +	default:
> +		dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len);
> +		return -EINVAL;
> +	}
> +
> +	uart.port.iotype = UPIO_MEM32;
> +	uart.port.membase = dfluart->csr_base + dfluart->csr_addr;
> +	uart.port.mapsize = dfluart->csr_size;
> +	uart.port.regshift = dfluart->reg_shift;
> +	uart.port.uartclk = dfluart->uart_clk;
> +
> +	/* register the port */
> +	ret = serial8250_register_8250_port(&uart);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to register 8250 port %d.\n", ret);
> +		return -EINVAL;
> +	}
> +	dev_info(dev, "serial8250_register_8250_port %d\n", ret);
> +	dfluart->line = ret;
> +	dev_set_drvdata(dev, dfluart);
> +
> +	return 0;
> +}
> +
> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
> +{
> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
> +
> +	if (dfluart->line > 0)
> +		serial8250_unregister_port(dfluart->line);
> +}
> +
> +#define FME_FEATURE_ID_UART 0x24
> +
> +static const struct dfl_device_id dfl_uart_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_UART },
> +	{ }
> +};
> +
> +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_DEVICE_TABLE(dfl, dfl_uart_ids);
> +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..fbb59216ce7f 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -546,3 +546,12 @@ config SERIAL_OF_PLATFORM
>  	  are probed through devicetree, including Open Firmware based
>  	  PowerPC systems and embedded systems on architectures using the
>  	  flattened device tree format.
> +
> +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.
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index bee908f99ea0..8e987b04820a 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -43,5 +43,6 @@ obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
>  obj-$(CONFIG_SERIAL_8250_TEGRA)		+= 8250_tegra.o
>  obj-$(CONFIG_SERIAL_8250_BCM7271)	+= 8250_bcm7271.o
>  obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>  
>  CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 5652879ab48e..d37636090fed 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -73,6 +73,13 @@
>  #define DFHv1_PARAM_MSIX_STARTV	0x8
>  #define DFHv1_PARAM_MSIX_NUMV	0xc
>  
> +#define DFHv1_PARAM_ID_CLK_FRQ    0x2
> +#define DFHv1_PARAM_ID_FIFO_LEN   0x3
> +
> +#define DFHv1_PARAM_ID_REG_LAYOUT 0x4
> +#define DFHv1_PARAM_ID_REG_WIDTH  GENMASK_ULL(63, 32)
> +#define DFHv1_PARAM_ID_REG_SHIFT  GENMASK_ULL(31, 0)
> +
>  /**
>   * enum dfl_id_type - define the DFL FIU types
>   */
> -- 
> 2.25.1
> 

Re: [PATCH v1 1/5] Documentation: fpga: dfl: Add documentation for DFHv1

[Xu Yilun]

On 2022-09-06 at 12:04:22 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add documentation describing the extentions provided by Version
> 1 of the Device Feature Header (DFHv1).
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 15b670926084..31699b89781e 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -561,6 +561,30 @@ new DFL feature via UIO direct access, its feature id should be added to the
>  driver's id_table.
>  
>  
> +Extending the Device Feature Header - DFHv1
> +===========================================
> +The current 8 bytes of the Device Feature Header, hereafter referred to as
> +to DFHv0, provide very little opportunity for the hardware to describe itself
> +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
> +to provide increased flexibility and extensibility to hardware designs using
> +Device Feature Lists.  The list below describes some of the goals behind the
> +changes in DFHv1:
> +
> +* Provide a standardized mechanism for features to describe
> +  parameters/capabilities to software.
> +* Standardize the use of a GUID for all DFHv1 types.
> +* Decouple the location of the DFH from the register space of the feature itself.
> +
> +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
> +a list of parameter values to a particular feature.
> +
> +With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUILD standard
> +across all types.
> +
> +With DFHv0, the register map of a given feature is located immediately following
> +the DFHv0 in the memory space.  With DFHv1, the location of the feature register
> +map can be specified as an offset to the DFHv1 or as an absolute address.

Could you make a table or diagram to describe the data structure layout of DFHv1
extention.

Thanks,
Yilun

> +
>  Open discussion
>  ===============
>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> -- 
> 2.25.1
> 

Re: [PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[matthew.gerlach]

On Wed, 7 Sep 2022, Greg KH wrote:

> On Tue, Sep 06, 2022 at 12:04:23PM -0700, matthew.gerlach@linux.intel.com wrote:
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -2,7 +2,7 @@
>>  /*
>>   * Driver Header File for FPGA Device Feature List (DFL) Support
>>   *
>> - * Copyright (C) 2017-2018 Intel Corporation, Inc.
>> + * Copyright (C) 2017-2022 Intel Corporation, Inc.
>
> I'm all for updated proper copyright dates, but in a patch that
> _removes_ text from a file does not seem like the proper place for that,
> right?  Please discuss with your corporate lawyers as to how to do this
> properly and when to do it.
>
> thanks,
>
> greg k-h
>

Hi Greg,
I discussed how and when to do this properly with my corporate lawyers and 
confirmed this submission is consistent with their guidelines.

You do raise an interesting point, though.  If you think this approach is 
improper, we should probably discuss it, including whether this 
restriction is already a condition for contributions or whether it should 
be.  It wouldn't be the first difference of opinion on the finer points of 
copyright law.

Best Regards,

Matthew Gerlach

Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

On Fri, 9 Sep 2022, Andy Shevchenko wrote:

> On Thu, Sep 08, 2022 at 11:27:03AM -0700, matthew.gerlach@linux.intel.com wrote:
>> On Tue, 6 Sep 2022, Andy Shevchenko wrote:
>>> On Tue, Sep 06, 2022 at 12:04:26PM -0700, matthew.gerlach@linux.intel.com wrote:
>
> ...
>
>>>> +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
>>>
>>> Isn't this available via normal interfaces to user?
>>
>> I am not sure what "normal interfaces to user" you are referring to.  The
>> code is just trying to read the frequency of the input clock to the uart
>> from a DFH paramter.
>
> I mean dev_dbg() call. The user can get uart_clk via one of the UART/serial
> ABIs (don't remember which one, though).

I don't think UART/serial ABIs to get the input clock frequency would be 
available until after the call to serial8250_register_8250_port() which 
needs the clock frequency as an input.

>
>
> ...
>
>>>> +#define FME_FEATURE_ID_UART 0x24
>>>
>>> Purpose of this definition? For me with or without is still an ID.
>>
>> I don't think I understand the question. Is the name of the macro unclear,
>> or do you think it is not necessary?
>
> I mean how the definition is useful / useless. I.o.w. I think it's not
> necessary.

The macro may not be necessary, but its usage is consistent with other dfl 
bus drivers.

Thanks for the feedback,
Matthew Gerlach

>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v1 1/5] Documentation: fpga: dfl: Add documentation for DFHv1

[matthew.gerlach]

On Sun, 11 Sep 2022, Xu Yilun wrote:

> On 2022-09-06 at 12:04:22 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add documentation describing the extentions provided by Version
>> 1 of the Device Feature Header (DFHv1).
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 15b670926084..31699b89781e 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -561,6 +561,30 @@ new DFL feature via UIO direct access, its feature id should be added to the
>>  driver's id_table.
>>
>>
>> +Extending the Device Feature Header - DFHv1
>> +===========================================
>> +The current 8 bytes of the Device Feature Header, hereafter referred to as
>> +to DFHv0, provide very little opportunity for the hardware to describe itself
>> +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
>> +to provide increased flexibility and extensibility to hardware designs using
>> +Device Feature Lists.  The list below describes some of the goals behind the
>> +changes in DFHv1:
>> +
>> +* Provide a standardized mechanism for features to describe
>> +  parameters/capabilities to software.
>> +* Standardize the use of a GUID for all DFHv1 types.
>> +* Decouple the location of the DFH from the register space of the feature itself.
>> +
>> +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
>> +a list of parameter values to a particular feature.
>> +
>> +With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUILD standard
>> +across all types.
>> +
>> +With DFHv0, the register map of a given feature is located immediately following
>> +the DFHv0 in the memory space.  With DFHv1, the location of the feature register
>> +map can be specified as an offset to the DFHv1 or as an absolute address.
>
> Could you make a table or diagram to describe the data structure layout of DFHv1
> extention.
>
> Thanks,
> Yilun

I was hoping that the actual macro definitions would be sufficient, but I 
will look into making a table.

Matthew Gerlach

>
>> +
>>  Open discussion
>>  ===============
>>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
>> --
>> 2.25.1
>>
>

Re: [PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[matthew.gerlach]

On Sun, 11 Sep 2022, Xu Yilun wrote:

> On 2022-09-06 at 12:04:23 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>>
>> Moving the DFH register offset and register definitions from
>> drivers/fpga/dfl.h to include/linux/dfl.h.  These definitions
>> need to be accessed by dfl drivers that are outside of
>> drivers/fpga.
>>
>> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/fpga/dfl.h  | 22 ++--------------------
>>  include/linux/dfl.h | 23 ++++++++++++++++++++++-
>>  2 files changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 06cfcd5e84bb..d4dfc03a0b61 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -2,7 +2,7 @@
>>  /*
>>   * Driver Header File for FPGA Device Feature List (DFL) Support
>>   *
>> - * Copyright (C) 2017-2018 Intel Corporation, Inc.
>> + * Copyright (C) 2017-2022 Intel Corporation, Inc.
>>   *
>>   * Authors:
>>   *   Kang Luwei <luwei.kang@intel.com>
>> @@ -17,6 +17,7 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/cdev.h>
>>  #include <linux/delay.h>
>> +#include <linux/dfl.h>
>>  #include <linux/eventfd.h>
>>  #include <linux/fs.h>
>>  #include <linux/interrupt.h>
>> @@ -53,28 +54,9 @@
>>  #define PORT_FEATURE_ID_UINT		0x12
>>  #define PORT_FEATURE_ID_STP		0x13
>>
>> -/*
>> - * Device Feature Header Register Set
>> - *
>> - * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
>> - * For AFUs, they have DFH + GUID as common header registers.
>> - * For private features, they only have DFH register as common header.
>> - */
>> -#define DFH			0x0
>> -#define GUID_L			0x8
>> -#define GUID_H			0x10
>> -#define NEXT_AFU		0x18
>> -
>> -#define DFH_SIZE		0x8
>> -
>>  /* Device Feature Header Register Bitfield */
>> -#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
>>  #define DFH_ID_FIU_FME		0
>>  #define DFH_ID_FIU_PORT		1
>> -#define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
>> -#define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
>> -#define DFH_EOL			BIT_ULL(40)		/* End of list */
>> -#define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>>  #define DFH_TYPE_AFU		1
>>  #define DFH_TYPE_PRIVATE	3
>>  #define DFH_TYPE_FIU		4
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 431636a0dc78..b5accdcfa368 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -2,7 +2,7 @@
>>  /*
>>   * Header file for DFL driver and device API
>>   *
>> - * Copyright (C) 2020 Intel Corporation, Inc.
>> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
>>   */
>>
>>  #ifndef __LINUX_DFL_H
>> @@ -11,6 +11,27 @@
>>  #include <linux/device.h>
>>  #include <linux/mod_devicetable.h>
>>
>> +/*
>> + * Device Feature Header Register Set
>> + *
>> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
>> + * For AFUs, they have DFH + GUID as common header registers.
>> + * For private features, they only have DFH register as common header.
>> + */
>> +#define DFH			0x0
>> +#define GUID_L			0x8
>> +#define GUID_H			0x10
>> +#define NEXT_AFU		0x18
>
> Now these macros are accessible in global kernel, should we add the
> DFL_ or DFH_ prefix for them?
>
> Thanks,
> Yilun

It does make sense to a DFL_ or DFH_ to these globabl macros, but I'll 
look again to see if the ones above really need to be global, where as the 
macros below definitely need to be global.  I also think a marco like 
DFL_DFH might be a little strange.

Thanks,
Matthew Gerlach

8>
>> +
>> +#define DFH_SIZE		0x8
>> +
>> +/* Device Feature Header Register Bitfield */
>> +#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
>> +#define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
>> +#define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
>> +#define DFH_EOL			BIT_ULL(40)		/* End of list */
>> +#define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>> +
>>  /**
>>   * enum dfl_id_type - define the DFL FIU types
>>   */
>> --
>> 2.25.1
>>
>

Re: [PATCH v1 3/5] fpga: dfl: Add DFHv1 Register Definitions

[matthew.gerlach]

On Sun, 11 Sep 2022, Xu Yilun wrote:

> On 2022-09-06 at 12:04:24 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>>
>> This patch adds the definitions for DFHv1 header and related register
>> bitfields.
>>
>> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  include/linux/dfl.h | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index b5accdcfa368..61bcf20c1bc8 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -23,6 +23,16 @@
>>  #define GUID_H			0x10
>>  #define NEXT_AFU		0x18
>>
>> +/*
>> + * DFHv1 Register Offset definitons
>> + * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
>> + * as common header registers
>> + */
>> +#define DFHv1_CSR_ADDR		0x18  /* CSR Register start address */
>> +#define DFHv1_CSR_SIZE_GRP	0x20  /* Size of Reg Block and Group/tag */
>> +#define DFHv1_PARAM_HDR		0x28  /* Optional First Param header */
>> +#define DFHv1_PARAM_DATA	0x8   /* Offset of Param data from Param header */
>> +
>>  #define DFH_SIZE		0x8
>>
>>  /* Device Feature Header Register Bitfield */
>> @@ -30,8 +40,35 @@
>>  #define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
>>  #define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
>>  #define DFH_EOL			BIT_ULL(40)		/* End of list */
>> +#define DFH_VERSION		GENMASK_ULL(59, 52)	/* DFH version */
>>  #define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>>
>> +/*
>> + *  CSR Rel Bit, 1'b0 = relative (offset from feature DFH start),
>
> Reduce one whitespace indent.
>
>> + * 1'b1 = absolute (ARM or other non-PCIe use)
>> + */
>> +#define DFHv1_CSR_ADDR_REL	BIT_ULL(0)
>> +
>> +/*
>> + * CSR Header Register Bit Definitions
>> + */
>
> Use oneline style comment should be OK?
>
>> +#define DFHv1_CSR_ADDR_MASK       GENMASK_ULL(63, 1)  /* 63:1 of CSR address */
>> +
>> +/*
>> + * CSR SIZE Goup Register Bit Definitions
>> + */
>
> Same concern
>
>> +#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
>> + */
>
> Same
>
>> +#define DFHv1_PARAM_HDR_ID		GENMASK_ULL(15, 0) /* Id of this Param  */
>> +#define DFHv1_PARAM_HDR_VERSION		GENMASK_ULL(31, 16) /* Version Param */
>> +#define DFHv1_PARAM_HDR_NEXT_OFFSET	GENMASK_ULL(63, 32) /* Offset of next Param */
>> +
>>  /**
>>   * enum dfl_id_type - define the DFL FIU types
>>   */
>> --
>> 2.25.1
>>
>

Hi Yilun,

I will fix the extra whitespace and change the comments to a single line 
where appropropriate.

Thanks,
Matthew Gerlach

Re: [PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[Geert Uytterhoeven]

Hi Matthew,

On Sun, Sep 11, 2022 at 5:40 PM <matthew.gerlach@linux.intel.com> wrote:
> On Wed, 7 Sep 2022, Greg KH wrote:
> > On Tue, Sep 06, 2022 at 12:04:23PM -0700, matthew.gerlach@linux.intel.com wrote:
> >> --- a/drivers/fpga/dfl.h
> >> +++ b/drivers/fpga/dfl.h
> >> @@ -2,7 +2,7 @@
> >>  /*
> >>   * Driver Header File for FPGA Device Feature List (DFL) Support
> >>   *
> >> - * Copyright (C) 2017-2018 Intel Corporation, Inc.
> >> + * Copyright (C) 2017-2022 Intel Corporation, Inc.
> >
> > I'm all for updated proper copyright dates, but in a patch that
> > _removes_ text from a file does not seem like the proper place for that,
> > right?  Please discuss with your corporate lawyers as to how to do this
> > properly and when to do it.

> I discussed how and when to do this properly with my corporate lawyers and
> confirmed this submission is consistent with their guidelines.
>
> You do raise an interesting point, though.  If you think this approach is
> improper, we should probably discuss it, including whether this
> restriction is already a condition for contributions or whether it should
> be.  It wouldn't be the first difference of opinion on the finer points of
> copyright law.

So each time code is removed from a file, its copyright year should
be updated? Eventually, we may end up with an empty file which
is copyrighted <this_year>? Do you think that makes sense?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Sun, Sep 11, 2022 at 08:56:41AM -0700, matthew.gerlach@linux.intel.com wrote:
> On Fri, 9 Sep 2022, Andy Shevchenko wrote:
> > On Thu, Sep 08, 2022 at 11:27:03AM -0700, matthew.gerlach@linux.intel.com wrote:
> > > On Tue, 6 Sep 2022, Andy Shevchenko wrote:
> > > > On Tue, Sep 06, 2022 at 12:04:26PM -0700, matthew.gerlach@linux.intel.com wrote:

...

> > > > > +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
> > > > 
> > > > Isn't this available via normal interfaces to user?
> > > 
> > > I am not sure what "normal interfaces to user" you are referring to.  The
> > > code is just trying to read the frequency of the input clock to the uart
> > > from a DFH paramter.
> > 
> > I mean dev_dbg() call. The user can get uart_clk via one of the UART/serial
> > ABIs (don't remember which one, though).
> 
> I don't think UART/serial ABIs to get the input clock frequency would be
> available until after the call to serial8250_register_8250_port()

Is it a problem?

> which needs the clock frequency as an input.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

On Sun, 11 Sep 2022, Xu Yilun wrote:

> On 2022-09-06 at 12:04:26 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/tty/serial/8250/8250_dfl.c | 188 +++++++++++++++++++++++++++++
>>  drivers/tty/serial/8250/Kconfig    |   9 ++
>>  drivers/tty/serial/8250/Makefile   |   1 +
>>  include/linux/dfl.h                |   7 ++
>>  4 files changed, 205 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..dcf6638a298c
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for FPGA UART
>> + *
>> + * Copyright (C) 2022 Intel Corporation, Inc.
>> + *
>> + * Authors:
>> + *   Ananda Ravuri <ananda.ravuri@intel.com>
>> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> + */
>> +
>> +#include <linux/dfl.h>
>> +#include <linux/version.h>
>> +#include <linux/serial.h>
>> +#include <linux/serial_8250.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/io-64-nonatomic-lo-hi.h>
>> +
>> +struct dfl_uart {
>> +	void __iomem   *csr_base;
>> +	u64             csr_addr;
>> +	unsigned int    csr_size;
>> +	struct device  *dev;
>> +	u64             uart_clk;
>> +	u64             fifo_len;
>> +	unsigned int    fifo_size;
>> +	unsigned int    reg_shift;
>> +	unsigned int    line;
>> +};
>> +
>> +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
>> +{
>> +	void __iomem *param_base;
>> +	int off;
>> +	u64 v;
>> +
>> +	v = readq(dfluart->csr_base + DFHv1_CSR_ADDR);
>> +	dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> +	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
>> +	dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
>
> These are generic for DFHv1, so maybe we parse them in DFL generic code.

I will look into moving this to the DFL generic code.

>
>> +
>> +	if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) {
>> +		dev_err(dfluart->dev, "FIXME bad dfh address and size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> +		dev_err(dfluart->dev, "missing required parameters\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
>
> The same concern.
>
>> +
>> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
>> +	if (off < 0) {
>> +		dev_err(dfluart->dev, "missing CLK_FRQ param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
>> +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
>
> I see the DFHv1_PARAM_ID_CLK_FRQ defined in generic dfl.h, is this
> param definition global to all features, or specific to uart?

Certainly uart drivers need to know the input clock frequency in order to 
properly calculate baud rate dividers, but drivers for other features/IP 
blocks may need to know the input clock frequency as well.  On the other 
hand not all drivers need to know the input clock frequency to the 
feature/IP block.

>
> Do we have clear definition of generic parameters vs feature specific
> parameters?

I don't think there is a clear definition of generic versus feature 
specific, but a clock frequency and interrupt information it fairly 
generic.

>
> The concern here is to avoid duplicated parameter parsing for each driver.

I understand the concern about avoiding duplicated parameter parsing.


>
> Thanks,
> Yilun
>
>> +
>> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
>> +	if (off < 0) {
>> +		dev_err(dfluart->dev, "missing FIFO_LEN param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
>> +	dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
>> +
>> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
>> +	if (off < 0) {
>> +		dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	v = readq(param_base + off + DFHv1_PARAM_DATA);
>> +	dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>> +	dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>> +	dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
>> +		dfluart->fifo_size, dfluart->reg_shift);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
>> +{
>> +	struct device *dev = &dfl_dev->dev;
>> +	struct uart_8250_port uart;
>> +	struct dfl_uart *dfluart;
>> +	int ret;
>> +
>> +	memset(&uart, 0, sizeof(uart));
>> +
>> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
>> +	if (!dfluart)
>> +		return -ENOMEM;
>> +
>> +	dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> +	if (IS_ERR(dfluart->csr_base)) {
>> +		dev_err(dev, "failed to get mem resource!\n");
>> +		return PTR_ERR(dfluart->csr_base);
>> +	}
>> +
>> +	dfluart->dev = dev;
>> +
>> +	ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res));
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to uart feature walk %d\n", ret);
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
>> +
>> +	if (dfl_dev->num_irqs == 1)
>> +		uart.port.irq = dfl_dev->irqs[0];
>> +
>> +	switch (dfluart->fifo_len) {
>> +	case 32:
>> +		uart.port.type = PORT_ALTR_16550_F32;
>> +		break;
>> +
>> +	case 64:
>> +		uart.port.type = PORT_ALTR_16550_F64;
>> +		break;
>> +
>> +	case 128:
>> +		uart.port.type = PORT_ALTR_16550_F128;
>> +		break;
>> +
>> +	default:
>> +		dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len);
>> +		return -EINVAL;
>> +	}
>> +
>> +	uart.port.iotype = UPIO_MEM32;
>> +	uart.port.membase = dfluart->csr_base + dfluart->csr_addr;
>> +	uart.port.mapsize = dfluart->csr_size;
>> +	uart.port.regshift = dfluart->reg_shift;
>> +	uart.port.uartclk = dfluart->uart_clk;
>> +
>> +	/* register the port */
>> +	ret = serial8250_register_8250_port(&uart);
>> +	if (ret < 0) {
>> +		dev_err(dev, "unable to register 8250 port %d.\n", ret);
>> +		return -EINVAL;
>> +	}
>> +	dev_info(dev, "serial8250_register_8250_port %d\n", ret);
>> +	dfluart->line = ret;
>> +	dev_set_drvdata(dev, dfluart);
>> +
>> +	return 0;
>> +}
>> +
>> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
>> +{
>> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
>> +
>> +	if (dfluart->line > 0)
>> +		serial8250_unregister_port(dfluart->line);
>> +}
>> +
>> +#define FME_FEATURE_ID_UART 0x24
>> +
>> +static const struct dfl_device_id dfl_uart_ids[] = {
>> +	{ FME_ID, FME_FEATURE_ID_UART },
>> +	{ }
>> +};
>> +
>> +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_DEVICE_TABLE(dfl, dfl_uart_ids);
>> +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..fbb59216ce7f 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -546,3 +546,12 @@ config SERIAL_OF_PLATFORM
>>  	  are probed through devicetree, including Open Firmware based
>>  	  PowerPC systems and embedded systems on architectures using the
>>  	  flattened device tree format.
>> +
>> +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.
>> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
>> index bee908f99ea0..8e987b04820a 100644
>> --- a/drivers/tty/serial/8250/Makefile
>> +++ b/drivers/tty/serial/8250/Makefile
>> @@ -43,5 +43,6 @@ obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
>>  obj-$(CONFIG_SERIAL_8250_TEGRA)		+= 8250_tegra.o
>>  obj-$(CONFIG_SERIAL_8250_BCM7271)	+= 8250_bcm7271.o
>>  obj-$(CONFIG_SERIAL_OF_PLATFORM)	+= 8250_of.o
>> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>>
>>  CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 5652879ab48e..d37636090fed 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -73,6 +73,13 @@
>>  #define DFHv1_PARAM_MSIX_STARTV	0x8
>>  #define DFHv1_PARAM_MSIX_NUMV	0xc
>>
>> +#define DFHv1_PARAM_ID_CLK_FRQ    0x2
>> +#define DFHv1_PARAM_ID_FIFO_LEN   0x3
>> +
>> +#define DFHv1_PARAM_ID_REG_LAYOUT 0x4
>> +#define DFHv1_PARAM_ID_REG_WIDTH  GENMASK_ULL(63, 32)
>> +#define DFHv1_PARAM_ID_REG_SHIFT  GENMASK_ULL(31, 0)
>> +
>>  /**
>>   * enum dfl_id_type - define the DFL FIU types
>>   */
>> --
>> 2.25.1
>>
>

Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Xu Yilun]

On 2022-09-12 at 08:29:47 -0700, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Sun, 11 Sep 2022, Xu Yilun wrote:
> 
> > On 2022-09-06 at 12:04:26 -0700, matthew.gerlach@linux.intel.com wrote:
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > 
> > > Add a Device Feature List (DFL) bus driver for the Altera
> > > 16550 implementation of UART.
> > > 
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_dfl.c | 188 +++++++++++++++++++++++++++++
> > >  drivers/tty/serial/8250/Kconfig    |   9 ++
> > >  drivers/tty/serial/8250/Makefile   |   1 +
> > >  include/linux/dfl.h                |   7 ++
> > >  4 files changed, 205 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..dcf6638a298c
> > > --- /dev/null
> > > +++ b/drivers/tty/serial/8250/8250_dfl.c
> > > @@ -0,0 +1,188 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for FPGA UART
> > > + *
> > > + * Copyright (C) 2022 Intel Corporation, Inc.
> > > + *
> > > + * Authors:
> > > + *   Ananda Ravuri <ananda.ravuri@intel.com>
> > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > + */
> > > +
> > > +#include <linux/dfl.h>
> > > +#include <linux/version.h>
> > > +#include <linux/serial.h>
> > > +#include <linux/serial_8250.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/bitfield.h>
> > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > +
> > > +struct dfl_uart {
> > > +	void __iomem   *csr_base;
> > > +	u64             csr_addr;
> > > +	unsigned int    csr_size;
> > > +	struct device  *dev;
> > > +	u64             uart_clk;
> > > +	u64             fifo_len;
> > > +	unsigned int    fifo_size;
> > > +	unsigned int    reg_shift;
> > > +	unsigned int    line;
> > > +};
> > > +
> > > +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
> > > +{
> > > +	void __iomem *param_base;
> > > +	int off;
> > > +	u64 v;
> > > +
> > > +	v = readq(dfluart->csr_base + DFHv1_CSR_ADDR);
> > > +	dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> > > +
> > > +	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
> > > +	dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
> > 
> > These are generic for DFHv1, so maybe we parse them in DFL generic code.
> 
> I will look into moving this to the DFL generic code.
> 
> > 
> > > +
> > > +	if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) {
> > > +		dev_err(dfluart->dev, "FIXME bad dfh address and size\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> > > +		dev_err(dfluart->dev, "missing required parameters\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
> > 
> > The same concern.
> > 
> > > +
> > > +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> > > +	if (off < 0) {
> > > +		dev_err(dfluart->dev, "missing CLK_FRQ param\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
> > > +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
> > 
> > I see the DFHv1_PARAM_ID_CLK_FRQ defined in generic dfl.h, is this
> > param definition global to all features, or specific to uart?
> 
> Certainly uart drivers need to know the input clock frequency in order to
> properly calculate baud rate dividers, but drivers for other features/IP
> blocks may need to know the input clock frequency as well.  On the other
> hand not all drivers need to know the input clock frequency to the
> feature/IP block.
> 
> > 
> > Do we have clear definition of generic parameters vs feature specific
> > parameters?
> 
> I don't think there is a clear definition of generic versus feature
> specific, but a clock frequency and interrupt information it fairly generic.
> 
> > 
> > The concern here is to avoid duplicated parameter parsing for each driver.
> 
> I understand the concern about avoiding duplicated parameter parsing.

Yeah. Another concern is, reviewers from other domains have to look into
every detail of the DFH param layout to know what happened, which I
think is not that friendly.

Thanks,
Yilun