| Message ID | 20220906190426.3139760-6-matthew.gerlach@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance definition of DFH and use enhancements for uart driver | expand |
On Tue, Sep 06, 2022 at 12:04:26PM -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/dfl.h> > +#include <linux/version.h> Hmm... Do we need this? > +#include <linux/serial.h> > +#include <linux/serial_8250.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/bitfield.h> > +#include <linux/io-64-nonatomic-lo-hi.h> Can this block be sorted alphabetically? ... > +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max) > +{ > + void __iomem *param_base; > + int off; > + u64 v; > + > + v = readq(dfluart->csr_base + DFHv1_CSR_ADDR); > + dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v); > + > + v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP); > + dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v); > + > + if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) { > + dev_err(dfluart->dev, "FIXME bad dfh address and size\n"); DFH ? > + return -EINVAL; > + } > + > + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) { > + dev_err(dfluart->dev, "missing required parameters\n"); Not sure I understood what parameters are here. FPGA VHDL? Configuration? RTL? > + return -EINVAL; > + } > + > + param_base = dfluart->csr_base + DFHv1_PARAM_HDR; > + > + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ); > + if (off < 0) { > + dev_err(dfluart->dev, "missing CLK_FRQ param\n"); > + return -EINVAL; > + } > + > + dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA); > + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); Isn't this available via normal interfaces to user? > + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN); > + if (off < 0) { > + dev_err(dfluart->dev, "missing FIFO_LEN param\n"); > + return -EINVAL; > + } > + > + dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA); > + dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len); > + > + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT); > + if (off < 0) { > + dev_err(dfluart->dev, "missing REG_LAYOUT param\n"); > + return -EINVAL; > + } > + > + v = readq(param_base + off + DFHv1_PARAM_DATA); > + dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v); > + dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v); > + dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n", > + dfluart->fifo_size, dfluart->reg_shift); > + > + 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)); > + > + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL); > + if (!dfluart) > + return -ENOMEM; > + > + dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res); > + if (IS_ERR(dfluart->csr_base)) { > + dev_err(dev, "failed to get mem resource!\n"); The above call have a few different messages depending on error code. No need to repeat this. > + return PTR_ERR(dfluart->csr_base); > + } > + > + dfluart->dev = dev; > + > + ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res)); > + if (ret < 0) { > + dev_err(dev, "failed to uart feature walk %d\n", ret); > + return -EINVAL; Why shadowing error code? What about return dev_err_probe(dev, ret, ...); ? > + } > + > + 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]; > + > + switch (dfluart->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: > + dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len); > + return -EINVAL; > + } > + > + uart.port.iotype = UPIO_MEM32; > + uart.port.membase = dfluart->csr_base + dfluart->csr_addr; > + uart.port.mapsize = dfluart->csr_size; > + uart.port.regshift = dfluart->reg_shift; > + uart.port.uartclk = dfluart->uart_clk; > + > + /* register the port */ > + ret = serial8250_register_8250_port(&uart); > + if (ret < 0) { > + dev_err(dev, "unable to register 8250 port %d.\n", ret); > + return -EINVAL; > + } > + dev_info(dev, "serial8250_register_8250_port %d\n", ret); > + dfluart->line = ret; > + 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); > + > + if (dfluart->line > 0) > + serial8250_unregister_port(dfluart->line); > +} ... > +#define FME_FEATURE_ID_UART 0x24 Purpose of this definition? For me with or without is still an ID. > +static const struct dfl_device_id dfl_uart_ids[] = { > + { FME_ID, FME_FEATURE_ID_UART }, > + { } > +}; ... > +static struct dfl_driver dfl_uart_driver = { > + .drv = { > + .name = "dfl-uart", > + }, > + .id_table = dfl_uart_ids, > + .probe = dfl_uart_probe, > + .remove = dfl_uart_remove, > +}; > + No need to have this blank line. > +module_dfl_driver(dfl_uart_driver); ... > +MODULE_DEVICE_TABLE(dfl, dfl_uart_ids); Move this closer to the definition. That's how other drivers do in the kernel. ... > --- a/drivers/tty/serial/8250/Kconfig > +++ b/drivers/tty/serial/8250/Kconfig > --- a/drivers/tty/serial/8250/Makefile > +++ b/drivers/tty/serial/8250/Makefile I know that the records in those files are not sorted, but can you try hard to find the best place for them in those files from sorting point of view?
On Tue, 6 Sep 2022, Andy Shevchenko wrote: > On Tue, Sep 06, 2022 at 12:04:26PM -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/dfl.h> > >> +#include <linux/version.h> > > Hmm... Do we need this? We do not need this. > >> +#include <linux/serial.h> >> +#include <linux/serial_8250.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/bitfield.h> >> +#include <linux/io-64-nonatomic-lo-hi.h> > > Can this block be sorted alphabetically? Yes, they can and should be sorted alphabetically. > > ... > >> +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max) >> +{ >> + void __iomem *param_base; >> + int off; >> + u64 v; >> + >> + v = readq(dfluart->csr_base + DFHv1_CSR_ADDR); >> + dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v); >> + >> + v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP); >> + dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v); >> + >> + if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) { >> + dev_err(dfluart->dev, "FIXME bad dfh address and size\n"); > > DFH ? Yes,"DFH" is better than "dfh" in the message. > >> + return -EINVAL; >> + } >> + >> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) { >> + dev_err(dfluart->dev, "missing required parameters\n"); > > Not sure I understood what parameters are here. FPGA VHDL? Configuration? RTL? How about "missing required DFH parameters\n"? > >> + return -EINVAL; >> + } >> + >> + param_base = dfluart->csr_base + DFHv1_PARAM_HDR; >> + >> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ); >> + if (off < 0) { >> + dev_err(dfluart->dev, "missing CLK_FRQ param\n"); >> + return -EINVAL; >> + } >> + >> + dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA); > >> + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); > > Isn't this available via normal interfaces to user? I am not sure what "normal interfaces to user" you are referring to. The code is just trying to read the frequency of the input clock to the uart from a DFH paramter. > >> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN); >> + if (off < 0) { >> + dev_err(dfluart->dev, "missing FIFO_LEN param\n"); >> + return -EINVAL; >> + } >> + >> + dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA); >> + dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len); >> + >> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT); >> + if (off < 0) { >> + dev_err(dfluart->dev, "missing REG_LAYOUT param\n"); >> + return -EINVAL; >> + } >> + >> + v = readq(param_base + off + DFHv1_PARAM_DATA); >> + dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v); >> + dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v); >> + dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n", >> + dfluart->fifo_size, dfluart->reg_shift); >> + >> + 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)); >> + >> + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL); >> + if (!dfluart) >> + return -ENOMEM; >> + >> + dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res); >> + if (IS_ERR(dfluart->csr_base)) { > >> + dev_err(dev, "failed to get mem resource!\n"); > > The above call have a few different messages depending on error code. > No need to repeat this. I will remove the call to dev_err(). > >> + return PTR_ERR(dfluart->csr_base); >> + } >> + >> + dfluart->dev = dev; >> + >> + ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res)); >> + if (ret < 0) { >> + dev_err(dev, "failed to uart feature walk %d\n", ret); > >> + return -EINVAL; > > Why shadowing error code? > What about > > return dev_err_probe(dev, ret, ...); > ? > Using dev_err_probe seems like the way to go. >> + } >> + >> + 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]; >> + >> + switch (dfluart->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: >> + dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len); >> + return -EINVAL; >> + } >> + >> + uart.port.iotype = UPIO_MEM32; >> + uart.port.membase = dfluart->csr_base + dfluart->csr_addr; >> + uart.port.mapsize = dfluart->csr_size; >> + uart.port.regshift = dfluart->reg_shift; >> + uart.port.uartclk = dfluart->uart_clk; >> + >> + /* register the port */ >> + ret = serial8250_register_8250_port(&uart); >> + if (ret < 0) { >> + dev_err(dev, "unable to register 8250 port %d.\n", ret); >> + return -EINVAL; >> + } >> + dev_info(dev, "serial8250_register_8250_port %d\n", ret); >> + dfluart->line = ret; >> + 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); >> + >> + if (dfluart->line > 0) >> + serial8250_unregister_port(dfluart->line); >> +} > > ... > >> +#define FME_FEATURE_ID_UART 0x24 > > Purpose of this definition? For me with or without is still an ID. I don't think I understand the question. Is the name of the macro unclear, or do you think it is not necessary? > >> +static const struct dfl_device_id dfl_uart_ids[] = { >> + { FME_ID, FME_FEATURE_ID_UART }, >> + { } >> +}; > > ... > >> +static struct dfl_driver dfl_uart_driver = { >> + .drv = { >> + .name = "dfl-uart", >> + }, >> + .id_table = dfl_uart_ids, >> + .probe = dfl_uart_probe, >> + .remove = dfl_uart_remove, >> +}; > >> + > > No need to have this blank line. I will remove the blank line. > >> +module_dfl_driver(dfl_uart_driver); > > ... > >> +MODULE_DEVICE_TABLE(dfl, dfl_uart_ids); > > Move this closer to the definition. That's how other drivers do in the kernel. Thanks for the suggestion. > > ... > >> --- a/drivers/tty/serial/8250/Kconfig >> +++ b/drivers/tty/serial/8250/Kconfig > >> --- a/drivers/tty/serial/8250/Makefile >> +++ b/drivers/tty/serial/8250/Makefile > > I know that the records in those files are not sorted, but can you try hard > to find the best place for them in those files from sorting point of view? > I will try to find a better place from sorting porint of view. > -- > With Best Regards, > Andy Shevchenko > > >
On Thu, Sep 08, 2022 at 11:27:03AM -0700, matthew.gerlach@linux.intel.com wrote: > On Tue, 6 Sep 2022, Andy Shevchenko wrote: > > On Tue, Sep 06, 2022 at 12:04:26PM -0700, matthew.gerlach@linux.intel.com wrote: ... > > > + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); > > > > Isn't this available via normal interfaces to user? > > I am not sure what "normal interfaces to user" you are referring to. The > code is just trying to read the frequency of the input clock to the uart > from a DFH paramter. I mean dev_dbg() call. The user can get uart_clk via one of the UART/serial ABIs (don't remember which one, though). ... > > > +#define FME_FEATURE_ID_UART 0x24 > > > > Purpose of this definition? For me with or without is still an ID. > > I don't think I understand the question. Is the name of the macro unclear, > or do you think it is not necessary? I mean how the definition is useful / useless. I.o.w. I think it's not necessary.
On 2022-09-06 at 12:04:26 -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> > --- > drivers/tty/serial/8250/8250_dfl.c | 188 +++++++++++++++++++++++++++++ > drivers/tty/serial/8250/Kconfig | 9 ++ > drivers/tty/serial/8250/Makefile | 1 + > include/linux/dfl.h | 7 ++ > 4 files changed, 205 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..dcf6638a298c > --- /dev/null > +++ b/drivers/tty/serial/8250/8250_dfl.c > @@ -0,0 +1,188 @@ > +// 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/dfl.h> > +#include <linux/version.h> > +#include <linux/serial.h> > +#include <linux/serial_8250.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/bitfield.h> > +#include <linux/io-64-nonatomic-lo-hi.h> > + > +struct dfl_uart { > + void __iomem *csr_base; > + u64 csr_addr; > + unsigned int csr_size; > + struct device *dev; > + u64 uart_clk; > + u64 fifo_len; > + unsigned int fifo_size; > + unsigned int reg_shift; > + unsigned int line; > +}; > + > +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max) > +{ > + void __iomem *param_base; > + int off; > + u64 v; > + > + v = readq(dfluart->csr_base + DFHv1_CSR_ADDR); > + dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v); > + > + v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP); > + dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v); These are generic for DFHv1, so maybe we parse them in DFL generic code. > + > + if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) { > + dev_err(dfluart->dev, "FIXME bad dfh address and size\n"); > + return -EINVAL; > + } > + > + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) { > + dev_err(dfluart->dev, "missing required parameters\n"); > + return -EINVAL; > + } > + > + param_base = dfluart->csr_base + DFHv1_PARAM_HDR; The same concern. > + > + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ); > + if (off < 0) { > + dev_err(dfluart->dev, "missing CLK_FRQ param\n"); > + return -EINVAL; > + } > + > + dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA); > + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); I see the DFHv1_PARAM_ID_CLK_FRQ defined in generic dfl.h, is this param definition global to all features, or specific to uart? Do we have clear definition of generic parameters vs feature specific parameters? The concern here is to avoid duplicated parameter parsing for each driver. Thanks, Yilun > + > + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN); > + if (off < 0) { > + dev_err(dfluart->dev, "missing FIFO_LEN param\n"); > + return -EINVAL; > + } > + > + dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA); > + dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len); > + > + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT); > + if (off < 0) { > + dev_err(dfluart->dev, "missing REG_LAYOUT param\n"); > + return -EINVAL; > + } > + > + v = readq(param_base + off + DFHv1_PARAM_DATA); > + dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v); > + dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v); > + dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n", > + dfluart->fifo_size, dfluart->reg_shift); > + > + 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)); > + > + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL); > + if (!dfluart) > + return -ENOMEM; > + > + dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res); > + if (IS_ERR(dfluart->csr_base)) { > + dev_err(dev, "failed to get mem resource!\n"); > + return PTR_ERR(dfluart->csr_base); > + } > + > + dfluart->dev = dev; > + > + ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res)); > + if (ret < 0) { > + dev_err(dev, "failed to uart feature walk %d\n", ret); > + return -EINVAL; > + } > + > + 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]; > + > + switch (dfluart->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: > + dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len); > + return -EINVAL; > + } > + > + uart.port.iotype = UPIO_MEM32; > + uart.port.membase = dfluart->csr_base + dfluart->csr_addr; > + uart.port.mapsize = dfluart->csr_size; > + uart.port.regshift = dfluart->reg_shift; > + uart.port.uartclk = dfluart->uart_clk; > + > + /* register the port */ > + ret = serial8250_register_8250_port(&uart); > + if (ret < 0) { > + dev_err(dev, "unable to register 8250 port %d.\n", ret); > + return -EINVAL; > + } > + dev_info(dev, "serial8250_register_8250_port %d\n", ret); > + dfluart->line = ret; > + 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); > + > + if (dfluart->line > 0) > + 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 }, > + { } > +}; > + > +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_DEVICE_TABLE(dfl, dfl_uart_ids); > +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..fbb59216ce7f 100644 > --- a/drivers/tty/serial/8250/Kconfig > +++ b/drivers/tty/serial/8250/Kconfig > @@ -546,3 +546,12 @@ config SERIAL_OF_PLATFORM > are probed through devicetree, including Open Firmware based > PowerPC systems and embedded systems on architectures using the > flattened device tree format. > + > +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. > diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile > index bee908f99ea0..8e987b04820a 100644 > --- a/drivers/tty/serial/8250/Makefile > +++ b/drivers/tty/serial/8250/Makefile > @@ -43,5 +43,6 @@ obj-$(CONFIG_SERIAL_8250_PXA) += 8250_pxa.o > obj-$(CONFIG_SERIAL_8250_TEGRA) += 8250_tegra.o > obj-$(CONFIG_SERIAL_8250_BCM7271) += 8250_bcm7271.o > obj-$(CONFIG_SERIAL_OF_PLATFORM) += 8250_of.o > +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o > > CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt > diff --git a/include/linux/dfl.h b/include/linux/dfl.h > index 5652879ab48e..d37636090fed 100644 > --- a/include/linux/dfl.h > +++ b/include/linux/dfl.h > @@ -73,6 +73,13 @@ > #define DFHv1_PARAM_MSIX_STARTV 0x8 > #define DFHv1_PARAM_MSIX_NUMV 0xc > > +#define DFHv1_PARAM_ID_CLK_FRQ 0x2 > +#define DFHv1_PARAM_ID_FIFO_LEN 0x3 > + > +#define DFHv1_PARAM_ID_REG_LAYOUT 0x4 > +#define DFHv1_PARAM_ID_REG_WIDTH GENMASK_ULL(63, 32) > +#define DFHv1_PARAM_ID_REG_SHIFT GENMASK_ULL(31, 0) > + > /** > * enum dfl_id_type - define the DFL FIU types > */ > -- > 2.25.1 >
On Fri, 9 Sep 2022, Andy Shevchenko wrote: > On Thu, Sep 08, 2022 at 11:27:03AM -0700, matthew.gerlach@linux.intel.com wrote: >> On Tue, 6 Sep 2022, Andy Shevchenko wrote: >>> On Tue, Sep 06, 2022 at 12:04:26PM -0700, matthew.gerlach@linux.intel.com wrote: > > ... > >>>> + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); >>> >>> Isn't this available via normal interfaces to user? >> >> I am not sure what "normal interfaces to user" you are referring to. The >> code is just trying to read the frequency of the input clock to the uart >> from a DFH paramter. > > I mean dev_dbg() call. The user can get uart_clk via one of the UART/serial > ABIs (don't remember which one, though). I don't think UART/serial ABIs to get the input clock frequency would be available until after the call to serial8250_register_8250_port() which needs the clock frequency as an input. > > > ... > >>>> +#define FME_FEATURE_ID_UART 0x24 >>> >>> Purpose of this definition? For me with or without is still an ID. >> >> I don't think I understand the question. Is the name of the macro unclear, >> or do you think it is not necessary? > > I mean how the definition is useful / useless. I.o.w. I think it's not > necessary. The macro may not be necessary, but its usage is consistent with other dfl bus drivers. Thanks for the feedback, Matthew Gerlach > > -- > With Best Regards, > Andy Shevchenko > > >
On Sun, Sep 11, 2022 at 08:56:41AM -0700, matthew.gerlach@linux.intel.com wrote: > On Fri, 9 Sep 2022, Andy Shevchenko wrote: > > On Thu, Sep 08, 2022 at 11:27:03AM -0700, matthew.gerlach@linux.intel.com wrote: > > > On Tue, 6 Sep 2022, Andy Shevchenko wrote: > > > > On Tue, Sep 06, 2022 at 12:04:26PM -0700, matthew.gerlach@linux.intel.com wrote: ... > > > > > + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); > > > > > > > > Isn't this available via normal interfaces to user? > > > > > > I am not sure what "normal interfaces to user" you are referring to. The > > > code is just trying to read the frequency of the input clock to the uart > > > from a DFH paramter. > > > > I mean dev_dbg() call. The user can get uart_clk via one of the UART/serial > > ABIs (don't remember which one, though). > > I don't think UART/serial ABIs to get the input clock frequency would be > available until after the call to serial8250_register_8250_port() Is it a problem? > which needs the clock frequency as an input.
On Sun, 11 Sep 2022, Xu Yilun wrote: > On 2022-09-06 at 12:04:26 -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> >> --- >> drivers/tty/serial/8250/8250_dfl.c | 188 +++++++++++++++++++++++++++++ >> drivers/tty/serial/8250/Kconfig | 9 ++ >> drivers/tty/serial/8250/Makefile | 1 + >> include/linux/dfl.h | 7 ++ >> 4 files changed, 205 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..dcf6638a298c >> --- /dev/null >> +++ b/drivers/tty/serial/8250/8250_dfl.c >> @@ -0,0 +1,188 @@ >> +// 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/dfl.h> >> +#include <linux/version.h> >> +#include <linux/serial.h> >> +#include <linux/serial_8250.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/bitfield.h> >> +#include <linux/io-64-nonatomic-lo-hi.h> >> + >> +struct dfl_uart { >> + void __iomem *csr_base; >> + u64 csr_addr; >> + unsigned int csr_size; >> + struct device *dev; >> + u64 uart_clk; >> + u64 fifo_len; >> + unsigned int fifo_size; >> + unsigned int reg_shift; >> + unsigned int line; >> +}; >> + >> +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max) >> +{ >> + void __iomem *param_base; >> + int off; >> + u64 v; >> + >> + v = readq(dfluart->csr_base + DFHv1_CSR_ADDR); >> + dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v); >> + >> + v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP); >> + dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v); > > These are generic for DFHv1, so maybe we parse them in DFL generic code. I will look into moving this to the DFL generic code. > >> + >> + if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) { >> + dev_err(dfluart->dev, "FIXME bad dfh address and size\n"); >> + return -EINVAL; >> + } >> + >> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) { >> + dev_err(dfluart->dev, "missing required parameters\n"); >> + return -EINVAL; >> + } >> + >> + param_base = dfluart->csr_base + DFHv1_PARAM_HDR; > > The same concern. > >> + >> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ); >> + if (off < 0) { >> + dev_err(dfluart->dev, "missing CLK_FRQ param\n"); >> + return -EINVAL; >> + } >> + >> + dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA); >> + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); > > I see the DFHv1_PARAM_ID_CLK_FRQ defined in generic dfl.h, is this > param definition global to all features, or specific to uart? Certainly uart drivers need to know the input clock frequency in order to properly calculate baud rate dividers, but drivers for other features/IP blocks may need to know the input clock frequency as well. On the other hand not all drivers need to know the input clock frequency to the feature/IP block. > > Do we have clear definition of generic parameters vs feature specific > parameters? I don't think there is a clear definition of generic versus feature specific, but a clock frequency and interrupt information it fairly generic. > > The concern here is to avoid duplicated parameter parsing for each driver. I understand the concern about avoiding duplicated parameter parsing. > > Thanks, > Yilun > >> + >> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN); >> + if (off < 0) { >> + dev_err(dfluart->dev, "missing FIFO_LEN param\n"); >> + return -EINVAL; >> + } >> + >> + dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA); >> + dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len); >> + >> + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT); >> + if (off < 0) { >> + dev_err(dfluart->dev, "missing REG_LAYOUT param\n"); >> + return -EINVAL; >> + } >> + >> + v = readq(param_base + off + DFHv1_PARAM_DATA); >> + dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v); >> + dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v); >> + dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n", >> + dfluart->fifo_size, dfluart->reg_shift); >> + >> + 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)); >> + >> + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL); >> + if (!dfluart) >> + return -ENOMEM; >> + >> + dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res); >> + if (IS_ERR(dfluart->csr_base)) { >> + dev_err(dev, "failed to get mem resource!\n"); >> + return PTR_ERR(dfluart->csr_base); >> + } >> + >> + dfluart->dev = dev; >> + >> + ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res)); >> + if (ret < 0) { >> + dev_err(dev, "failed to uart feature walk %d\n", ret); >> + return -EINVAL; >> + } >> + >> + 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]; >> + >> + switch (dfluart->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: >> + dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len); >> + return -EINVAL; >> + } >> + >> + uart.port.iotype = UPIO_MEM32; >> + uart.port.membase = dfluart->csr_base + dfluart->csr_addr; >> + uart.port.mapsize = dfluart->csr_size; >> + uart.port.regshift = dfluart->reg_shift; >> + uart.port.uartclk = dfluart->uart_clk; >> + >> + /* register the port */ >> + ret = serial8250_register_8250_port(&uart); >> + if (ret < 0) { >> + dev_err(dev, "unable to register 8250 port %d.\n", ret); >> + return -EINVAL; >> + } >> + dev_info(dev, "serial8250_register_8250_port %d\n", ret); >> + dfluart->line = ret; >> + 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); >> + >> + if (dfluart->line > 0) >> + 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 }, >> + { } >> +}; >> + >> +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_DEVICE_TABLE(dfl, dfl_uart_ids); >> +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..fbb59216ce7f 100644 >> --- a/drivers/tty/serial/8250/Kconfig >> +++ b/drivers/tty/serial/8250/Kconfig >> @@ -546,3 +546,12 @@ config SERIAL_OF_PLATFORM >> are probed through devicetree, including Open Firmware based >> PowerPC systems and embedded systems on architectures using the >> flattened device tree format. >> + >> +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. >> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile >> index bee908f99ea0..8e987b04820a 100644 >> --- a/drivers/tty/serial/8250/Makefile >> +++ b/drivers/tty/serial/8250/Makefile >> @@ -43,5 +43,6 @@ obj-$(CONFIG_SERIAL_8250_PXA) += 8250_pxa.o >> obj-$(CONFIG_SERIAL_8250_TEGRA) += 8250_tegra.o >> obj-$(CONFIG_SERIAL_8250_BCM7271) += 8250_bcm7271.o >> obj-$(CONFIG_SERIAL_OF_PLATFORM) += 8250_of.o >> +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o >> >> CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt >> diff --git a/include/linux/dfl.h b/include/linux/dfl.h >> index 5652879ab48e..d37636090fed 100644 >> --- a/include/linux/dfl.h >> +++ b/include/linux/dfl.h >> @@ -73,6 +73,13 @@ >> #define DFHv1_PARAM_MSIX_STARTV 0x8 >> #define DFHv1_PARAM_MSIX_NUMV 0xc >> >> +#define DFHv1_PARAM_ID_CLK_FRQ 0x2 >> +#define DFHv1_PARAM_ID_FIFO_LEN 0x3 >> + >> +#define DFHv1_PARAM_ID_REG_LAYOUT 0x4 >> +#define DFHv1_PARAM_ID_REG_WIDTH GENMASK_ULL(63, 32) >> +#define DFHv1_PARAM_ID_REG_SHIFT GENMASK_ULL(31, 0) >> + >> /** >> * enum dfl_id_type - define the DFL FIU types >> */ >> -- >> 2.25.1 >> >
On 2022-09-12 at 08:29:47 -0700, matthew.gerlach@linux.intel.com wrote: > > > On Sun, 11 Sep 2022, Xu Yilun wrote: > > > On 2022-09-06 at 12:04:26 -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> > > > --- > > > drivers/tty/serial/8250/8250_dfl.c | 188 +++++++++++++++++++++++++++++ > > > drivers/tty/serial/8250/Kconfig | 9 ++ > > > drivers/tty/serial/8250/Makefile | 1 + > > > include/linux/dfl.h | 7 ++ > > > 4 files changed, 205 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..dcf6638a298c > > > --- /dev/null > > > +++ b/drivers/tty/serial/8250/8250_dfl.c > > > @@ -0,0 +1,188 @@ > > > +// 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/dfl.h> > > > +#include <linux/version.h> > > > +#include <linux/serial.h> > > > +#include <linux/serial_8250.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/bitfield.h> > > > +#include <linux/io-64-nonatomic-lo-hi.h> > > > + > > > +struct dfl_uart { > > > + void __iomem *csr_base; > > > + u64 csr_addr; > > > + unsigned int csr_size; > > > + struct device *dev; > > > + u64 uart_clk; > > > + u64 fifo_len; > > > + unsigned int fifo_size; > > > + unsigned int reg_shift; > > > + unsigned int line; > > > +}; > > > + > > > +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max) > > > +{ > > > + void __iomem *param_base; > > > + int off; > > > + u64 v; > > > + > > > + v = readq(dfluart->csr_base + DFHv1_CSR_ADDR); > > > + dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v); > > > + > > > + v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP); > > > + dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v); > > > > These are generic for DFHv1, so maybe we parse them in DFL generic code. > > I will look into moving this to the DFL generic code. > > > > > > + > > > + if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) { > > > + dev_err(dfluart->dev, "FIXME bad dfh address and size\n"); > > > + return -EINVAL; > > > + } > > > + > > > + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) { > > > + dev_err(dfluart->dev, "missing required parameters\n"); > > > + return -EINVAL; > > > + } > > > + > > > + param_base = dfluart->csr_base + DFHv1_PARAM_HDR; > > > > The same concern. > > > > > + > > > + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ); > > > + if (off < 0) { > > > + dev_err(dfluart->dev, "missing CLK_FRQ param\n"); > > > + return -EINVAL; > > > + } > > > + > > > + dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA); > > > + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); > > > > I see the DFHv1_PARAM_ID_CLK_FRQ defined in generic dfl.h, is this > > param definition global to all features, or specific to uart? > > Certainly uart drivers need to know the input clock frequency in order to > properly calculate baud rate dividers, but drivers for other features/IP > blocks may need to know the input clock frequency as well. On the other > hand not all drivers need to know the input clock frequency to the > feature/IP block. > > > > > Do we have clear definition of generic parameters vs feature specific > > parameters? > > I don't think there is a clear definition of generic versus feature > specific, but a clock frequency and interrupt information it fairly generic. > > > > > The concern here is to avoid duplicated parameter parsing for each driver. > > I understand the concern about avoiding duplicated parameter parsing. Yeah. Another concern is, reviewers from other domains have to look into every detail of the DFH param layout to know what happened, which I think is not that friendly. 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..dcf6638a298c --- /dev/null +++ b/drivers/tty/serial/8250/8250_dfl.c @@ -0,0 +1,188 @@ +// 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/dfl.h> +#include <linux/version.h> +#include <linux/serial.h> +#include <linux/serial_8250.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/bitfield.h> +#include <linux/io-64-nonatomic-lo-hi.h> + +struct dfl_uart { + void __iomem *csr_base; + u64 csr_addr; + unsigned int csr_size; + struct device *dev; + u64 uart_clk; + u64 fifo_len; + unsigned int fifo_size; + unsigned int reg_shift; + unsigned int line; +}; + +int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max) +{ + void __iomem *param_base; + int off; + u64 v; + + v = readq(dfluart->csr_base + DFHv1_CSR_ADDR); + dfluart->csr_addr = FIELD_GET(DFHv1_CSR_ADDR_MASK, v); + + v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP); + dfluart->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v); + + if (dfluart->csr_addr == 0 || dfluart->csr_size == 0) { + dev_err(dfluart->dev, "FIXME bad dfh address and size\n"); + return -EINVAL; + } + + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) { + dev_err(dfluart->dev, "missing required parameters\n"); + return -EINVAL; + } + + param_base = dfluart->csr_base + DFHv1_PARAM_HDR; + + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ); + if (off < 0) { + dev_err(dfluart->dev, "missing CLK_FRQ param\n"); + return -EINVAL; + } + + dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA); + dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk); + + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN); + if (off < 0) { + dev_err(dfluart->dev, "missing FIFO_LEN param\n"); + return -EINVAL; + } + + dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA); + dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len); + + off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT); + if (off < 0) { + dev_err(dfluart->dev, "missing REG_LAYOUT param\n"); + return -EINVAL; + } + + v = readq(param_base + off + DFHv1_PARAM_DATA); + dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v); + dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v); + dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n", + dfluart->fifo_size, dfluart->reg_shift); + + 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)); + + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL); + if (!dfluart) + return -ENOMEM; + + dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res); + if (IS_ERR(dfluart->csr_base)) { + dev_err(dev, "failed to get mem resource!\n"); + return PTR_ERR(dfluart->csr_base); + } + + dfluart->dev = dev; + + ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res)); + if (ret < 0) { + dev_err(dev, "failed to uart feature walk %d\n", ret); + return -EINVAL; + } + + 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]; + + switch (dfluart->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: + dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len); + return -EINVAL; + } + + uart.port.iotype = UPIO_MEM32; + uart.port.membase = dfluart->csr_base + dfluart->csr_addr; + uart.port.mapsize = dfluart->csr_size; + uart.port.regshift = dfluart->reg_shift; + uart.port.uartclk = dfluart->uart_clk; + + /* register the port */ + ret = serial8250_register_8250_port(&uart); + if (ret < 0) { + dev_err(dev, "unable to register 8250 port %d.\n", ret); + return -EINVAL; + } + dev_info(dev, "serial8250_register_8250_port %d\n", ret); + dfluart->line = ret; + 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); + + if (dfluart->line > 0) + 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 }, + { } +}; + +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_DEVICE_TABLE(dfl, dfl_uart_ids); +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..fbb59216ce7f 100644 --- a/drivers/tty/serial/8250/Kconfig +++ b/drivers/tty/serial/8250/Kconfig @@ -546,3 +546,12 @@ config SERIAL_OF_PLATFORM are probed through devicetree, including Open Firmware based PowerPC systems and embedded systems on architectures using the flattened device tree format. + +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. diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile index bee908f99ea0..8e987b04820a 100644 --- a/drivers/tty/serial/8250/Makefile +++ b/drivers/tty/serial/8250/Makefile @@ -43,5 +43,6 @@ obj-$(CONFIG_SERIAL_8250_PXA) += 8250_pxa.o obj-$(CONFIG_SERIAL_8250_TEGRA) += 8250_tegra.o obj-$(CONFIG_SERIAL_8250_BCM7271) += 8250_bcm7271.o obj-$(CONFIG_SERIAL_OF_PLATFORM) += 8250_of.o +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt diff --git a/include/linux/dfl.h b/include/linux/dfl.h index 5652879ab48e..d37636090fed 100644 --- a/include/linux/dfl.h +++ b/include/linux/dfl.h @@ -73,6 +73,13 @@ #define DFHv1_PARAM_MSIX_STARTV 0x8 #define DFHv1_PARAM_MSIX_NUMV 0xc +#define DFHv1_PARAM_ID_CLK_FRQ 0x2 +#define DFHv1_PARAM_ID_FIFO_LEN 0x3 + +#define DFHv1_PARAM_ID_REG_LAYOUT 0x4 +#define DFHv1_PARAM_ID_REG_WIDTH GENMASK_ULL(63, 32) +#define DFHv1_PARAM_ID_REG_SHIFT GENMASK_ULL(31, 0) + /** * enum dfl_id_type - define the DFL FIU types */