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

[Marco Pagani]

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