diff mbox series

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

Message ID 20221220163652.499831-5-matthew.gerlach@linux.intel.com (mailing list archive)
State New
Headers show
Series Enhance definition of DFH and use enhancements for UART driver | expand

Commit Message

Matthew Gerlach Dec. 20, 2022, 4:36 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v7: no change

v6: move driver specific parameter definitions to limit scope

v5: removed unneeded blank line
    removed unneeded includes
    included device.h and types.h
    removed unneeded local variable
    remove calls to dev_dbg
    memset -> { }
    remove space after period
    explicitly include used headers
    remove redundant Inc from Copyright
    fix format specifier

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 | 154 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |  12 +++
 drivers/tty/serial/8250/Makefile   |   1 +
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

Comments

Andy Shevchenko Dec. 20, 2022, 5:09 p.m. UTC | #1 
On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.

In general the code here looks good to me, but one thing to discuss due to
comment to the previous patch(es).

...

> +	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");
> +
> +	p++;
> +	uart->port.uartclk = *p;

So, here and the below is using always the second u64 from the returned data.
Does it mean:
- we always skip the first u64 from the returned buffer and hence... (see below)
- we may actually return the second u64 as a plain number (not a pointer) from
  (an additional?) API? In such case we would not need to take care about this
  p++; lines here and there.
- we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
Marco Pagani Dec. 21, 2022, 5:26 p.m. UTC | #2 
On 2022-12-20 18:09, Andy Shevchenko wrote:
> On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
> 
> In general the code here looks good to me, but one thing to discuss due to
> comment to the previous patch(es).
> 
> ...
> 
>> +	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");
>> +
>> +	p++;
>> +	uart->port.uartclk = *p;
> 
> So, here and the below is using always the second u64 from the returned data.
> Does it mean:
> - we always skip the first u64 from the returned buffer and hence... (see below)
> - we may actually return the second u64 as a plain number (not a pointer) from
>   (an additional?) API? In such case we would not need to take care about this
>   p++; lines here and there.
> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
> 

I also had the impression that this method of getting and incrementing a pointer
to the beginning of the parameter block is a bit more error-prone than necessary.
Since parameter blocks are now standardized, wouldn't be easier and safer to wrap
the access logic into a helper function like:

u16 dfh_get_param_data(struct dfl_device *dfl_dev, u16 param_id, u64 *data)

that directly provides a copy of the parameter's data into a pointer provided by
the caller and returns the parameter version or an error if not found?

Thanks,
Marco
Marco Pagani Dec. 21, 2022, 7:52 p.m. UTC | #3 
On 2022-12-21 18:26, Marco Pagani wrote:
> 
> 
> On 2022-12-20 18:09, Andy Shevchenko wrote:
>> On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
>>
>> In general the code here looks good to me, but one thing to discuss due to
>> comment to the previous patch(es).
>>
>> ...
>>
>>> +	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");
>>> +
>>> +	p++;
>>> +	uart->port.uartclk = *p;
>>
>> So, here and the below is using always the second u64 from the returned data.
>> Does it mean:
>> - we always skip the first u64 from the returned buffer and hence... (see below)
>> - we may actually return the second u64 as a plain number (not a pointer) from
>>   (an additional?) API? In such case we would not need to take care about this
>>   p++; lines here and there.
>> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
>>
> 
> I also had the impression that this method of getting and incrementing a pointer
> to the beginning of the parameter block is a bit more error-prone than necessary.
> Since parameter blocks are now standardized, wouldn't be easier and safer to wrap
> the access logic into a helper function like:
> 
> u16 dfh_get_param_data(struct dfl_device *dfl_dev, u16 param_id, u64 *data)
> 
> that directly provides a copy of the parameter's data into a pointer provided by
> the caller and returns the parameter version or an error if not found?

Please ignore the last part of my reply. The diagram in the documentation made
me think that parameter data are always 64-bit wide. Since the parameter data
"payload" size depends on the version and ID, an eventual helper function could
then return a pointer to the data payload and the version number to the caller.

Thanks,
Marco
Matthew Gerlach Dec. 21, 2022, 10:16 p.m. UTC | #4 
On Tue, 20 Dec 2022, Andy Shevchenko wrote:

> On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
>
> In general the code here looks good to me, but one thing to discuss due to
> comment to the previous patch(es).
>
> ...
>
>> +	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");
>> +
>> +	p++;
>> +	uart->port.uartclk = *p;
>
> So, here and the below is using always the second u64 from the returned data.
> Does it mean:
> - we always skip the first u64 from the returned buffer and hence... (see below)

The first u64 of the parameter block, the parameter header, contains a 
version field and a next/size field that a parameter consumer might use.
The version field determines the exact layout of the data, and the 
next/size field could/should be used to prevent out of bounds accesses.

> - we may actually return the second u64 as a plain number (not a pointer) from
>  (an additional?) API? In such case we would not need to take care about this
>  p++; lines here and there.

I think an additional API that can be used to fetch an array of u64's 
while also checking boundary conditions would be helpful.

> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?

The length and layout of the parameter data is determined by the parameter 
id and version. So the data portion of a parameter is not fixed length.

Thanks for the feedback,
Matthew Gerlach

>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>
Matthew Gerlach Dec. 21, 2022, 10:37 p.m. UTC | #5 
On Wed, 21 Dec 2022, Marco Pagani wrote:

>
> On 2022-12-21 18:26, Marco Pagani wrote:
>>
>>
>> On 2022-12-20 18:09, Andy Shevchenko wrote:
>>> On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
>>>
>>> In general the code here looks good to me, but one thing to discuss due to
>>> comment to the previous patch(es).
>>>
>>> ...
>>>
>>>> +	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");
>>>> +
>>>> +	p++;
>>>> +	uart->port.uartclk = *p;
>>>
>>> So, here and the below is using always the second u64 from the returned data.
>>> Does it mean:
>>> - we always skip the first u64 from the returned buffer and hence... (see below)
>>> - we may actually return the second u64 as a plain number (not a pointer) from
>>>   (an additional?) API? In such case we would not need to take care about this
>>>   p++; lines here and there.
>>> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
>>>
>>
>> I also had the impression that this method of getting and incrementing a pointer
>> to the beginning of the parameter block is a bit more error-prone than necessary.
>> Since parameter blocks are now standardized, wouldn't be easier and safer to wrap
>> the access logic into a helper function like:
>>
>> u16 dfh_get_param_data(struct dfl_device *dfl_dev, u16 param_id, u64 *data)
>>
>> that directly provides a copy of the parameter's data into a pointer provided by
>> the caller and returns the parameter version or an error if not found?
>
> Please ignore the last part of my reply. The diagram in the documentation made
> me think that parameter data are always 64-bit wide. Since the parameter data
> "payload" size depends on the version and ID, an eventual helper function could
> then return a pointer to the data payload and the version number to the caller.

Certainly helper functions should be created to make accessing the actual 
parameter easier and safer.

Thanks for the feedback,
Matthew Gerlach

>
> Thanks,
> Marco
>
>
>
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..606279d420e0
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,154 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA UART
+ *
+ * Copyright (C) 2022 Intel Corporation.
+ *
+ * Authors:
+ *   Ananda Ravuri <ananda.ravuri@intel.com>
+ *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/dfl.h>
+#include <linux/errno.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/types.h>
+
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+
+#define DFHv1_PARAM_ID_CLK_FRQ    0x2
+#define DFHv1_PARAM_ID_FIFO_LEN   0x3
+
+#define DFHv1_PARAM_ID_REG_LAYOUT	0x4
+#define DFHv1_PARAM_REG_LAYOUT_WIDTH	GENMASK_ULL(63, 32)
+#define DFHv1_PARAM_REG_LAYOUT_SHIFT	GENMASK_ULL(31, 0)
+
+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;
+	u32 reg_width;
+	u64 fifo_len;
+	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");
+
+	p++;
+	uart->port.uartclk = *p;
+
+	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
+	if (!p)
+		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
+
+	p++;
+	fifo_len = *p;
+	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");
+
+	p++;
+	uart->port.regshift = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_SHIFT, *p);
+	reg_width = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_WIDTH, *p);
+	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 %u\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;
+
+	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");
+
+	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 b0f62345bc84..08af2acd4645 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 1615bfdde2a0..4e1a32812683 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