On 2022-11-02 10:57, Ilpo Järvinen wrote:

On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:

quoted

On Tue, 1 Nov 2022, Ilpo Järvinen wrote:

quoted

On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:

quoted

On Tue, 1 Nov 2022, Xu Yilun wrote:

quoted

On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote:

quoted

On Sat, 29 Oct 2022, Xu Yilun wrote:

quoted

On 2022-10-20 at 14:26:10 -0700, matthew.gerlach@linux.intel.com
wrote:

quoted

From: Matthew Gerlach <redacted>

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

Signed-off-by: Matthew Gerlach <redacted>
---
v4: use dev_err_probe() everywhere that is appropriate
    clean up noise
    change error messages to use the word, unsupported
    tried again to sort Makefile and KConfig better
    reorder probe function for easier error handling
    use new dfh_find_param API

v3: use passed in location of registers
    use cleaned up functions for parsing parameters

v2: clean up error messages
    alphabetize header files
    fix 'missing prototype' error by making function static
    tried to sort Makefile and Kconfig better
---
 drivers/tty/serial/8250/8250_dfl.c | 149
+++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |  12 +++
 drivers/tty/serial/8250/Makefile   |   1 +
 3 files changed, 162 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

diff --git a/drivers/tty/serial/8250/8250_dfl.c

b/drivers/tty/serial/8250/8250_dfl.c
new file mode 100644
index 000000000000..f02f0ba2a565

--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c

@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA UART
+ *
+ * Copyright (C) 2022 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Ananda Ravuri <ananda.ravuri@intel.com>
+ *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/dfl.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+
+struct dfl_uart {
+	int line;
+};
+
+static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct

uart_8250_port *uart)
+{
+	struct device *dev = &dfl_dev->dev;
+	u64 v, fifo_len, reg_width;
+	u64 *p;
+
+	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
+	if (!p)
+		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
param\n");
+
+	uart->port.uartclk = *p;
+	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
+
+	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
+	if (!p)
+		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
param\n");
+
+	fifo_len = *p;
+	dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
+
+	switch (fifo_len) {
+	case 32:
+		uart->port.type = PORT_ALTR_16550_F32;
+		break;
+
+	case 64:
+		uart->port.type = PORT_ALTR_16550_F64;
+		break;
+
+	case 128:
+		uart->port.type = PORT_ALTR_16550_F128;
+		break;
+
+	default:
+		return dev_err_probe(dev, -EINVAL, "unsupported
fifo_len %llu\n", fifo_len);
+	}
+
+	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
+	if (!p)
+		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
param\n");
+
+	v = *p;
+	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
+	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);

I have concern that the raw layout inside the parameter block is
still exposed to drivers and need to be parsed by each driver.

Raw parameter block will always have to be passed to the driver
because HW
specific properties can be defined that will need to be parsed by the
specific driver.

So there is a question about the scope of the definitions of these
parameter
blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
seems specific to uart?

There are definitely two classes of parameter blocks.  One class is HW
agnostic parameters where the parameters are relevant to many different
kinds
of HW components.  MSI-X, and input clock-frequency are certainly HW
agnostic,
and it turns out that REG_LAYOUT is not specific to uart.  You can see
reg_bits and reg_stride in struct regmap_config.  There are also device
tree
bindings for reg-shift and reg-io-width.  The second class of parameters
would
be specific to HW component.  In the case of this uart driver, all
parameters
would be considered HW agnostic parameters.

quoted

If a parameter block is widely used in dfl drivers, duplicate the
parsing
from HW layout in each driver may not be a good idea. While for device
specific parameter block, it's OK.

It sounds like we are in agreement.

quoted

Another concern is the indexing of the parameter IDs. If some parameter
blocks should be device specific, then no need to have globally indexed
parameter IDs. Index them locally in device is OK. So put the
definitions
of ID values, HW layout and their parsing operation in each driver.

It may be confusing for two drivers to use the same parameter id that have
different meanings and data layout.  Since all the parameters for this
driver
would be considered HW agnostic, we'd don't need to address this issue
with
this patchset.

quoted

quoted

quoted

How about we define HW agnostic IDs for parameter specific fields
like:

PARAM_ID		FIELD_ID
================================
MSIX			STARTV
			NUMV
--------------------------------
CLK			FREQ
--------------------------------
FIFO			LEN
--------------------------------
REG_LAYOUT		WIDTH
			SHIFT

And define like u64 dfl_find_param(struct dfl_device *, int
param_id,
int field_id)

I don't think dfl_find_param as defined above adds much value.

quoted

Think further, if we have to define HW agnostic property - value
pairs,
why don't we just use "Software nodes for the firmware node", see
drivers/base/swnode.c. I think this may be a better choice.

I am looking into "Software nodes for the firmware node", and it can
be
used
for HW agnostic properties.  Each dfl driver will still have to make a
function call to fetch each HW agnostice property value as well as a
function call to find the HW specific parameters and then parse those
parameters.

Btw, another aspect this discussion has completely overlooked is the
presence of parameter version and how it impacts data layout. Is v1
always going be a subset of v2 or can a later version remove something
v1 had?

In general it would be preferable for v1 to be a subset of v2.  This allows
for v1 SW to work on v2 HW.

In that case, shouldn't the minimum acceptable version be part of 
dfh_find_param() parameters?

Currently there's no way for the caller to even look what version the 
parameter is from dfh_find_param()'s return value (except with some 
negative offset hack to access parameter header).

Why not just checking dfl_dev->dfh_version in dfl_uart_probe() before
calling dfh_find_param()? In general, any dfl_driver could potentially
do this check in its *_probe() function before reading the header to avoid
compatibility issues.

Cheers,
Marco