diff mbox series

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

Message ID 20221020212610.697729-5-matthew.gerlach@linux.intel.com
State New
Headers
Series Enhance definition of DFH and use enhancements for uart driver |

Commit Message

Matthew Gerlach Oct. 20, 2022, 9:26 p.m. UTC 
  From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

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

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
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

Comments

Andy Shevchenko Oct. 20, 2022, 10:13 p.m. UTC | #1 
On Thu, Oct 20, 2022 at 02:26:10PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.

...

> +#include <linux/bitfield.h>

Where is device.h?

> +#include <linux/dfl.h>

> +#include <linux/io-64-nonatomic-lo-hi.h>

User?

> +#include <linux/kernel.h>

Try to use what is really needed, yet this one may be still needed for
something like ARRAY_SIZE().

> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>

Missed types.h.

...

> +	ret = dfl_uart_get_params(dfl_dev, &uart);

> +

Redundant blank line.

> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
Greg KH Oct. 21, 2022, 4:33 a.m. UTC | #2 
On Thu, Oct 20, 2022 at 02:26:10PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>

As per the rules of how Intel developers are supposed to send out
patches, I can't take this.  Please work with the Intel kernel developer
team to get this properly reviewed first, and the correct reviews,
before sending it out again.

thanks,

greg k-h
Ilpo Järvinen Oct. 21, 2022, 9:24 a.m. UTC | #3 
On Thu, 20 Oct 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---

> +	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);

Just use *p directly for the FIELD_GETs. I suppose you can drop v after 
changing it.

Other than that and what Andy noted, this patch looks good.
Xu Yilun Oct. 29, 2022, 3:24 p.m. UTC | #4 
On 2022-10-20 at 14:26:10 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> 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.

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)

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.

Thanks,
Yilun
Matthew Gerlach Nov. 1, 2022, 12:34 a.m. UTC | #5 
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 <matthew.gerlach@linux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> 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.

>
> 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.

>
> Thanks,
> Yilun
>
Xu Yilun Nov. 1, 2022, 1:46 a.m. UTC | #6 
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 <matthew.gerlach@linux.intel.com>
> > > 
> > > Add a Device Feature List (DFL) bus driver for the Altera
> > > 16550 implementation of UART.
> > > 
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > > 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?

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.

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.

Thanks,
Yilun

> 
> > 
> > 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.
> 
> > 
> > Thanks,
> > Yilun
> >
Matthew Gerlach Nov. 1, 2022, 4:04 p.m. UTC | #7 
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 <matthew.gerlach@linux.intel.com>
>>>>
>>>> Add a Device Feature List (DFL) bus driver for the Altera
>>>> 16550 implementation of UART.
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> ---
>>>> 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.

>
> 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.

>
> Thanks,
> Yilun
>
>>
>>>
>>> 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.
>>
>>>
>>> Thanks,
>>> Yilun
>>>
>
Ilpo Järvinen Nov. 1, 2022, 4:30 p.m. UTC | #8 
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 <matthew.gerlach@linux.intel.com>
> > > > > 
> > > > > Add a Device Feature List (DFL) bus driver for the Altera
> > > > > 16550 implementation of UART.
> > > > > 
> > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > ---
> > > > > 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.
> 
> > 
> > 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?
Matthew Gerlach Nov. 1, 2022, 5:39 p.m. UTC | #9 
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 <matthew.gerlach@linux.intel.com>
>>>>>>
>>>>>> Add a Device Feature List (DFL) bus driver for the Altera
>>>>>> 16550 implementation of UART.
>>>>>>
>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>> ---
>>>>>> 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.
>>
>>>
>>> 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.
>
Ilpo Järvinen Nov. 2, 2022, 9:57 a.m. UTC | #10 
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 <matthew.gerlach@linux.intel.com>
> > > > > > > 
> > > > > > > Add a Device Feature List (DFL) bus driver for the Altera
> > > > > > > 16550 implementation of UART.
> > > > > > > 
> > > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > ---
> > > > > > > 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.
> > > 
> > > > 
> > > > 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).
Marco Pagani Nov. 8, 2022, 12:48 p.m. UTC | #11 
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 <matthew.gerlach@linux.intel.com>
>>>>>>>>
>>>>>>>> Add a Device Feature List (DFL) bus driver for the Altera
>>>>>>>> 16550 implementation of UART.
>>>>>>>>
>>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>> ---
>>>>>>>> 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.
>>>>
>>>>>
>>>>> 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
Ilpo Järvinen Nov. 8, 2022, 12:51 p.m. UTC | #12 
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 <matthew.gerlach@linux.intel.com>
> >>>>>>>>
> >>>>>>>> Add a Device Feature List (DFL) bus driver for the Altera
> >>>>>>>> 16550 implementation of UART.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>>>>>>> ---
> >>>>>>>> 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.
> >>>>
> >>>>>
> >>>>> 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.
diff mbox series

Patch

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);
+
+	dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n", reg_width, uart->port.regshift);
+
+	switch (reg_width) {
+	case 4:
+		uart->port.iotype = UPIO_MEM32;
+		break;
+
+	case 2:
+		uart->port.iotype = UPIO_MEM16;
+		break;
+
+	default:
+		return dev_err_probe(dev, -EINVAL, "unsupported reg_width %lld\n", reg_width);
+	}
+
+	return 0;
+}
+
+static int dfl_uart_probe(struct dfl_device *dfl_dev)
+{
+	struct device *dev = &dfl_dev->dev;
+	struct uart_8250_port uart;
+	struct dfl_uart *dfluart;
+	int ret;
+
+	memset(&uart, 0, sizeof(uart));
+	uart.port.flags = UPF_IOREMAP;
+	uart.port.mapbase = dfl_dev->mmio_res.start;
+	uart.port.mapsize = resource_size(&dfl_dev->mmio_res);
+
+	ret = dfl_uart_get_params(dfl_dev, &uart);
+
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed uart feature walk\n");
+
+	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
+
+	if (dfl_dev->num_irqs == 1)
+		uart.port.irq = dfl_dev->irqs[0];
+
+	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+	if (!dfluart)
+		return -ENOMEM;
+
+	dfluart->line = serial8250_register_8250_port(&uart);
+	if (dfluart->line < 0)
+		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
+
+	dev_set_drvdata(dev, dfluart);
+
+	return 0;
+}
+
+static void dfl_uart_remove(struct dfl_device *dfl_dev)
+{
+	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
+
+	serial8250_unregister_port(dfluart->line);
+}
+
+#define FME_FEATURE_ID_UART 0x24
+
+static const struct dfl_device_id dfl_uart_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_UART },
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
+
+static struct dfl_driver dfl_uart_driver = {
+	.drv = {
+		.name = "dfl-uart",
+	},
+	.id_table = dfl_uart_ids,
+	.probe = dfl_uart_probe,
+	.remove = dfl_uart_remove,
+};
+module_dfl_driver(dfl_uart_driver);
+
+MODULE_DESCRIPTION("DFL Intel UART driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index d0b49e15fbf5..4efc8ee51c18 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -370,6 +370,18 @@  config SERIAL_8250_FSL
 	  erratum for Freescale 16550 UARTs in the 8250 driver. It also
 	  enables support for ACPI enumeration.
 
+config SERIAL_8250_DFL
+	tristate "DFL bus driver for Altera 16550 UART"
+	depends on SERIAL_8250 && FPGA_DFL
+	help
+	  This option enables support for a Device Feature List (DFL) bus
+	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
+	  can be instantiated in a FPGA and then be discovered during
+	  enumeration of the DFL bus.
+
+	  To compile this driver as a module, chose M here: the
+	  module will be called 8250_dfl.
+
 config SERIAL_8250_DW
 	tristate "Support for Synopsys DesignWare 8250 quirks"
 	depends on SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..65bc6ad4dd01 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -28,6 +28,7 @@  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_MEN_MCB)	+= 8250_men_mcb.o
+obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
 obj-$(CONFIG_SERIAL_8250_IOC3)		+= 8250_ioc3.o