| Message ID | 20221020212610.697729-5-matthew.gerlach@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance definition of DFH and use enhancements for uart driver | expand |
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");
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
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.
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
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