On Tue, 1 Nov 2022, Ilpo Järvinen wrote: > On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote: > >> >> >> On Tue, 1 Nov 2022, Xu Yilun wrote: >> >>> On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote: >>>> >>>> >>>> On Sat, 29 Oct 2022, Xu Yilun wrote: >>>> >>>>> On 2022-10-20 at 14:26:10 -0700, matthew.gerlach@linux.intel.com wrote: >>>>>> From: Matthew Gerlach >>>>>> >>>>>> Add a Device Feature List (DFL) bus driver for the Altera >>>>>> 16550 implementation of UART. >>>>>> >>>>>> Signed-off-by: Matthew Gerlach >>>>>> --- >>>>>> 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 >>>>>> + * Matthew Gerlach >>>>>> + */ >>>>>> + >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> + >>>>>> +struct dfl_uart { >>>>>> + int line; >>>>>> +}; >>>>>> + >>>>>> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct >>>>>> uart_8250_port *uart) >>>>>> +{ >>>>>> + struct device *dev = &dfl_dev->dev; >>>>>> + u64 v, fifo_len, reg_width; >>>>>> + u64 *p; >>>>>> + >>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ); >>>>>> + if (!p) >>>>>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ >>>>>> param\n"); >>>>>> + >>>>>> + uart->port.uartclk = *p; >>>>>> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk); >>>>>> + >>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN); >>>>>> + if (!p) >>>>>> + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN >>>>>> param\n"); >>>>>> + >>>>>> + fifo_len = *p; >>>>>> + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len); >>>>>> + >>>>>> + switch (fifo_len) { >>>>>> + case 32: >>>>>> + uart->port.type = PORT_ALTR_16550_F32; >>>>>> + break; >>>>>> + >>>>>> + case 64: >>>>>> + uart->port.type = PORT_ALTR_16550_F64; >>>>>> + break; >>>>>> + >>>>>> + case 128: >>>>>> + uart->port.type = PORT_ALTR_16550_F128; >>>>>> + break; >>>>>> + >>>>>> + default: >>>>>> + return dev_err_probe(dev, -EINVAL, "unsupported >>>>>> fifo_len %llu\n", fifo_len); >>>>>> + } >>>>>> + >>>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT); >>>>>> + if (!p) >>>>>> + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT >>>>>> param\n"); >>>>>> + >>>>>> + v = *p; >>>>>> + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v); >>>>>> + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v); >>>>> >>>>> I have concern that the raw layout inside the parameter block is >>>>> still exposed to drivers and need to be parsed by each driver. >>>> >>>> Raw parameter block will always have to be passed to the driver because HW >>>> specific properties can be defined that will need to be parsed by the >>>> specific driver. >>> >>> So there is a question about the scope of the definitions of these parameter >>> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT >>> seems specific to uart? >> >> There are definitely two classes of parameter blocks. One class is HW >> agnostic parameters where the parameters are relevant to many different kinds >> of HW components. MSI-X, and input clock-frequency are certainly HW agnostic, >> and it turns out that REG_LAYOUT is not specific to uart. You can see >> reg_bits and reg_stride in struct regmap_config. There are also device tree >> bindings for reg-shift and reg-io-width. The second class of parameters would >> be specific to HW component. In the case of this uart driver, all parameters >> would be considered HW agnostic parameters. >> >>> >>> If a parameter block is widely used in dfl drivers, duplicate the parsing >>> from HW layout in each driver may not be a good idea. While for device >>> specific parameter block, it's OK. >> >> It sounds like we are in agreement. >> >>> >>> Another concern is the indexing of the parameter IDs. If some parameter >>> blocks should be device specific, then no need to have globally indexed >>> parameter IDs. Index them locally in device is OK. So put the definitions >>> of ID values, HW layout and their parsing operation in each driver. >> >> It may be confusing for two drivers to use the same parameter id that have >> different meanings and data layout. Since all the parameters for this driver >> would be considered HW agnostic, we'd don't need to address this issue with >> this patchset. >> >>>>> How about we define HW agnostic IDs for parameter specific fields like: >>>>> >>>>> PARAM_ID FIELD_ID >>>>> ================================ >>>>> MSIX STARTV >>>>> NUMV >>>>> -------------------------------- >>>>> CLK FREQ >>>>> -------------------------------- >>>>> FIFO LEN >>>>> -------------------------------- >>>>> REG_LAYOUT WIDTH >>>>> SHIFT >>>>> >>>>> And define like u64 dfl_find_param(struct dfl_device *, int param_id, >>>>> int field_id) >>>> >>>> I don't think dfl_find_param as defined above adds much value. >>>> >>>>> >>>>> Think further, if we have to define HW agnostic property - value pairs, >>>>> why don't we just use "Software nodes for the firmware node", see >>>>> drivers/base/swnode.c. I think this may be a better choice. >>>> >>>> I am looking into "Software nodes for the firmware node", and it can be >>>> used >>>> for HW agnostic properties. Each dfl driver will still have to make a >>>> function call to fetch each HW agnostice property value as well as a >>>> function call to find the HW specific parameters and then parse those >>>> parameters. > > Btw, another aspect this discussion has completely overlooked is the > presence of parameter version and how it impacts data layout. Is v1 > always going be a subset of v2 or can a later version remove something > v1 had? In general it would be preferable for v1 to be a subset of v2. This allows for v1 SW to work on v2 HW. > > -- > i. >