LinuxLists.cc - [PATCH v1 0/5] Enhance definition of DFH and use enhancements for uart driver

On Sun, 11 Sep 2022, Xu Yilun wrote:

> On 2022-09-06 at 12:04:26 -0700, [email protected] wrote:
>> From: Matthew Gerlach <[email protected]>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>>
>> Signed-off-by: Matthew Gerlach <[email protected]>
>> ---
>> 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 <[email protected]>
>> + * Matthew Gerlach <[email protected]>
>> + */
>> +
>> +#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
>>
>

2022-09-13 03:42:36

by Xu Yilun

[permalink] [raw]

Subject: Re: [PATCH v1 5/5] tty: serial: 8250: add DFL bus driver for Altera 16550.

On 2022-09-12 at 08:29:47 -0700, [email protected] wrote:
>
>
> On Sun, 11 Sep 2022, Xu Yilun wrote:
>
> > On 2022-09-06 at 12:04:26 -0700, [email protected] wrote:
> > > From: Matthew Gerlach <[email protected]>
> > >
> > > Add a Device Feature List (DFL) bus driver for the Altera
> > > 16550 implementation of UART.
> > >
> > > Signed-off-by: Matthew Gerlach <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > > + * Matthew Gerlach <[email protected]>
> > > + */
> > > +
> > > +#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