On Tue, 8 Nov 2022, Marco Pagani wrote: > > On 2022-11-02 10:57, Ilpo Järvinen wrote: > > On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote: > > > >> > >> > >> 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. > > > > In that case, shouldn't the minimum acceptable version be part of > > dfh_find_param() parameters? > > > > Currently there's no way for the caller to even look what version the > > parameter is from dfh_find_param()'s return value (except with some > > negative offset hack to access parameter header). > > > > > > Why not just checking dfl_dev->dfh_version in dfl_uart_probe() before > calling dfh_find_param()? In general, any dfl_driver could potentially > do this check in its *_probe() function before reading the header to avoid > compatibility issues. It's about a different version. DFH has it's own version and every parameter header has a separate version specific to that parameter. -- i.