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