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]>
Reported-by: kernel test robot <[email protected]>
---
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 | 177 +++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 9 ++
drivers/tty/serial/8250/Makefile | 1 +
3 files changed, 187 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..110ad3a73459
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,177 @@
+// 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/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 device *dev, void __iomem *dfh_base, resource_size_t max,
+ struct uart_8250_port *uart)
+{
+ u64 v, fifo_len, reg_width;
+ int off;
+
+ if (!dfhv1_has_params(dfh_base)) {
+ dev_err(dev, "missing required DFH parameters\n");
+ return -EINVAL;
+ }
+
+ off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
+ if (off < 0) {
+ dev_err(dev, "missing CLK_FRQ param\n");
+ return -EINVAL;
+ }
+
+ uart->port.uartclk = readq(dfh_base + off);
+ dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
+
+ off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
+ if (off < 0) {
+ dev_err(dev, "missing FIFO_LEN param\n");
+ return -EINVAL;
+ }
+
+ fifo_len = readq(dfh_base + off);
+ 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:
+ dev_err(dev, "bad fifo_len %llu\n", fifo_len);
+ return -EINVAL;
+ }
+
+ off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
+ if (off < 0) {
+ dev_err(dev, "missing REG_LAYOUT param\n");
+ return -EINVAL;
+ }
+
+ v = readq(dfh_base + off);
+ 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",
+ FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
+
+ switch (reg_width) {
+ case 4:
+ uart->port.iotype = UPIO_MEM32;
+ break;
+
+ case 2:
+ uart->port.iotype = UPIO_MEM16;
+ break;
+
+ default:
+ dev_err(dev, "invalid reg_width %lld\n", reg_width);
+ return -EINVAL;
+ }
+
+ 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;
+ resource_size_t res_size;
+ void __iomem *dfh_base;
+ int ret;
+
+ memset(&uart, 0, sizeof(uart));
+ uart.port.flags = UPF_IOREMAP;
+ uart.port.mapbase = dfl_dev->csr_res.start;
+ uart.port.mapsize = resource_size(&dfl_dev->csr_res);
+
+ dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+ if (!dfluart)
+ return -ENOMEM;
+
+ dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
+ if (IS_ERR(dfh_base))
+ return PTR_ERR(dfh_base);
+
+ res_size = resource_size(&dfl_dev->mmio_res);
+
+ ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
+
+ devm_iounmap(dev, dfh_base);
+ devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
+
+ 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];
+
+ /* register the port */
+ 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_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
+ 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 },
+ { }
+};
+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..5c6497ce5c12 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -361,6 +361,15 @@ config SERIAL_8250_BCM2835AUX
If unsure, say N.
+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.
+
config SERIAL_8250_FSL
bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
depends on SERIAL_8250_CONSOLE
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..32006e0982d1 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE) += 8250_early.o
obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
+obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
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
--
2.25.1
On Tue, Oct 04, 2022 at 07:37:18AM -0700, [email protected] wrote:
> From: Matthew Gerlach <[email protected]>
>
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
...
> Reported-by: kernel test robot <[email protected]>
https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
"The Reported-by tag gives credit to people who find bugs and report them and it
hopefully inspires them to help us again in the future. Please note that if the
bug was reported in private, then ask for permission first before using the
Reported-by tag. The tag is intended for bugs; please do not use it to credit
feature requests."
...
> + if (!dfhv1_has_params(dfh_base)) {
> + dev_err(dev, "missing required DFH parameters\n");
> + return -EINVAL;
> + }
Why not use dev_err_probe() everywhere since this is called only at ->probe()
stage?
...
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> + if (off < 0) {
> + dev_err(dev, "missing CLK_FRQ param\n");
> + return -EINVAL;
Why error code is being shadowed?
> + }
...
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> + if (off < 0) {
> + dev_err(dev, "missing FIFO_LEN param\n");
> + return -EINVAL;
Ditto.
> + }
...
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> + if (off < 0) {
> + dev_err(dev, "missing REG_LAYOUT param\n");
> + return -EINVAL;
> + }
Ditto.
...
> + dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
> + FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
Casting in printf() in kernel in 99% shows the wrong specifier in use. Try to
select the best suitable one.
...
> + dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> + if (IS_ERR(dfh_base))
> + return PTR_ERR(dfh_base);
> +
> + res_size = resource_size(&dfl_dev->mmio_res);
> +
> + ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
> + devm_iounmap(dev, dfh_base);
> + devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
to begin with? The devm_* release type of functions in 99% of the cases
indicate of the abusing devm_.
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed uart feature walk\n");
...
> + dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
Why do we need this noise?
...
> + if (dfluart->line >= 0)
When this can be false?
> + serial8250_unregister_port(dfluart->line);
...
> +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.
When m, what be the module name?
...
> obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
> obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
> obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
> +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
entries are not properly placed there and in Makefile.)
> 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
--
With Best Regards,
Andy Shevchenko
On Tue, 4 Oct 2022, [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]>
> Reported-by: kernel test robot <[email protected]>
> ---
> 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 | 177 +++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 9 ++
> drivers/tty/serial/8250/Makefile | 1 +
> 3 files changed, 187 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..110ad3a73459
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,177 @@
> +static int dfl_uart_get_params(struct device *dev, void __iomem *dfh_base, resource_size_t max,
> + struct uart_8250_port *uart)
> +{
> + u64 v, fifo_len, reg_width;
> + int off;
> +
> + if (!dfhv1_has_params(dfh_base)) {
> + dev_err(dev, "missing required DFH parameters\n");
> + return -EINVAL;
> + }
> +
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> + if (off < 0) {
> + dev_err(dev, "missing CLK_FRQ param\n");
> + return -EINVAL;
> + }
> +
> + uart->port.uartclk = readq(dfh_base + off);
> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> +
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> + if (off < 0) {
> + dev_err(dev, "missing FIFO_LEN param\n");
> + return -EINVAL;
> + }
> +
> + fifo_len = readq(dfh_base + off);
> + 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:
> + dev_err(dev, "bad fifo_len %llu\n", fifo_len);
I'd tell user "unsupported" rather than "bad".
> + return -EINVAL;
> + }
> +
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> + if (off < 0) {
> + dev_err(dev, "missing REG_LAYOUT param\n");
> + return -EINVAL;
> + }
> +
> + v = readq(dfh_base + off);
> + 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",
> + FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
Why not use reg_width directly?
> + switch (reg_width) {
> + case 4:
> + uart->port.iotype = UPIO_MEM32;
> + break;
> +
> + case 2:
> + uart->port.iotype = UPIO_MEM16;
> + break;
> +
> + default:
> + dev_err(dev, "invalid reg_width %lld\n", reg_width);
unsupported ?
> + return -EINVAL;
> + }
> +
> + 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;
> + resource_size_t res_size;
> + void __iomem *dfh_base;
> + int ret;
> +
> + memset(&uart, 0, sizeof(uart));
> + uart.port.flags = UPF_IOREMAP;
> + uart.port.mapbase = dfl_dev->csr_res.start;
> + uart.port.mapsize = resource_size(&dfl_dev->csr_res);
> +
> + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> + if (!dfluart)
> + return -ENOMEM;
> +
> + dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> + if (IS_ERR(dfh_base))
> + return PTR_ERR(dfh_base);
> +
> + res_size = resource_size(&dfl_dev->mmio_res);
> +
> + ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
> +
> + devm_iounmap(dev, dfh_base);
> + devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
> +
> + 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];
> +
> + /* register the port */
This comment is pretty useless. Just drop it.
> + 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_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
This you want to drop too. It seems a debug thing rather than info level
stuff.
--
i.
On Tue, 4 Oct 2022, Andy Shevchenko wrote:
> On Tue, Oct 04, 2022 at 07:37:18AM -0700, [email protected] wrote:
>> From: Matthew Gerlach <[email protected]>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>
> ...
>
>> Reported-by: kernel test robot <[email protected]>
>
> https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>
> "The Reported-by tag gives credit to people who find bugs and report them and it
> hopefully inspires them to help us again in the future. Please note that if the
> bug was reported in private, then ask for permission first before using the
> Reported-by tag. The tag is intended for bugs; please do not use it to credit
> feature requests."
>
The kernel test robot did find a bug in my v1 submission. I was missing
the static keyword for a function declaration. Should I remove the tag?
>
> ...
>
>> + if (!dfhv1_has_params(dfh_base)) {
>> + dev_err(dev, "missing required DFH parameters\n");
>> + return -EINVAL;
>> + }
>
> Why not use dev_err_probe() everywhere since this is called only at ->probe()
> stage?
I wasn't sure if using dev_err_probe() was correct, since the usage is
technically in a different function. Since the code is only called from
->probe(), and it is much cleaner, I'll switch to dev_err_probe()
everywhere
> > ...
>
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
>> + if (off < 0) {
>> + dev_err(dev, "missing CLK_FRQ param\n");
>
>> + return -EINVAL;
>
> Why error code is being shadowed?
Definitely a mistake.
>
>> + }
>
> ...
>
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
>> + if (off < 0) {
>> + dev_err(dev, "missing FIFO_LEN param\n");
>> + return -EINVAL;
>
> Ditto.
>
>> + }
>
> ...
>
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
>> + if (off < 0) {
>> + dev_err(dev, "missing REG_LAYOUT param\n");
>> + return -EINVAL;
>> + }
>
> Ditto.
>
> ...
>
>> + dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
>> + FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
>
> Casting in printf() in kernel in 99% shows the wrong specifier in use. Try to
> select the best suitable one.
I will remove the casting and find the correct format specifier.
>
> ...
>
>> + dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> + if (IS_ERR(dfh_base))
>> + return PTR_ERR(dfh_base);
>> +
>> + res_size = resource_size(&dfl_dev->mmio_res);
>> +
>> + ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
>
>> + devm_iounmap(dev, dfh_base);
>> + devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
>
> If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
> to begin with? The devm_* release type of functions in 99% of the cases
> indicate of the abusing devm_.
I will change the code to call ioremap() and request_mem_region() directly
instead of the devm_ versions.
>
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "failed uart feature walk\n");
>
> ...
>
>> + dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
>
> Why do we need this noise?
No, we do not need this noise.
>
> ...
>
>> + if (dfluart->line >= 0)
>
> When this can be false?
This can never be false. I will remove it.
>
>> + serial8250_unregister_port(dfluart->line);
>
> ...
>
>> +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.
>
> When m, what be the module name?
I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed
into /lib/modules/... I also see "alias dfl:t0000f0024* 8250_dfl" in
modules.alias
>
> ...
>
>> obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
>> obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
>> obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
>> +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
>
> This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
> it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
> entries are not properly placed there and in Makefile.)
Since 8250_dfl results in its own module, and my kernel config doesn't
have FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
>
>> 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
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks for the feedback.
>
>
>
On Thu, Oct 06, 2022 at 10:00:43AM -0700, [email protected] wrote:
> On Tue, 4 Oct 2022, Andy Shevchenko wrote:
> > On Tue, Oct 04, 2022 at 07:37:18AM -0700, [email protected] wrote:
...
> > > Reported-by: kernel test robot <[email protected]>
> >
> > https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> >
> > "The Reported-by tag gives credit to people who find bugs and report them and it
> > hopefully inspires them to help us again in the future. Please note that if the
> > bug was reported in private, then ask for permission first before using the
> > Reported-by tag. The tag is intended for bugs; please do not use it to credit
> > feature requests."
>
> The kernel test robot did find a bug in my v1 submission. I was missing the
> static keyword for a function declaration. Should I remove the tag?
What's yours take from the above documentation?
...
> > > + dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > + if (IS_ERR(dfh_base))
> > > + return PTR_ERR(dfh_base);
> > > +
> > > + res_size = resource_size(&dfl_dev->mmio_res);
> > > +
> > > + ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
> >
> > > + devm_iounmap(dev, dfh_base);
> > > + devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
> >
> > If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
> > to begin with? The devm_* release type of functions in 99% of the cases
> > indicate of the abusing devm_.
>
> I will change the code to call ioremap() and request_mem_region() directly
> instead of the devm_ versions.
But why will you need request_mem_region() in that case?
> > > + if (ret < 0)
> > > + return dev_err_probe(dev, ret, "failed uart feature walk\n");
...
> > > +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.
> >
> > When m, what be the module name?
>
> I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed into
> /lib/modules/... I also see "alias dfl:t0000f0024* 8250_dfl" in
> modules.alias
My point is that user who will run `make menuconfig` will read this and have
no clue after the kernel build if the module was built or not. Look into other
(recent) sections of the Kconfig for drivers in the kernel for how they inform
user about the module name (this more or less standard pattern you just need
to copy'n'paste'n'edit carefully).
...
> > > obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
> > > obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
> > > obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
> > > +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
> >
> > This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
> > it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
> > entries are not properly placed there and in Makefile.)
>
> Since 8250_dfl results in its own module, and my kernel config doesn't have
> FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
The Makefile is a bit chaotic, but try to find the sorted (more or less)
group of drivers that are not 4 ports and squeeze your entry there
(I expect somewhere between the LPSS/MID lines).
It will help to sort out that mess in the future.
> > > 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
--
With Best Regards,
Andy Shevchenko
On Wed, 5 Oct 2022, Ilpo J?rvinen wrote:
> On Tue, 4 Oct 2022, [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]>
>> Reported-by: kernel test robot <[email protected]>
>> ---
>> 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 | 177 +++++++++++++++++++++++++++++
>> drivers/tty/serial/8250/Kconfig | 9 ++
>> drivers/tty/serial/8250/Makefile | 1 +
>> 3 files changed, 187 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..110ad3a73459
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>> @@ -0,0 +1,177 @@
>
>> +static int dfl_uart_get_params(struct device *dev, void __iomem *dfh_base, resource_size_t max,
>> + struct uart_8250_port *uart)
>> +{
>> + u64 v, fifo_len, reg_width;
>> + int off;
>> +
>> + if (!dfhv1_has_params(dfh_base)) {
>> + dev_err(dev, "missing required DFH parameters\n");
>> + return -EINVAL;
>> + }
>> +
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
>> + if (off < 0) {
>> + dev_err(dev, "missing CLK_FRQ param\n");
>> + return -EINVAL;
>> + }
>> +
>> + uart->port.uartclk = readq(dfh_base + off);
>> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>> +
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
>> + if (off < 0) {
>> + dev_err(dev, "missing FIFO_LEN param\n");
>> + return -EINVAL;
>> + }
>> +
>> + fifo_len = readq(dfh_base + off);
>> + 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:
>> + dev_err(dev, "bad fifo_len %llu\n", fifo_len);
>
> I'd tell user "unsupported" rather than "bad".
The word, unsupported, sounds better. I will change it in both places you
suggested.
>
>> + return -EINVAL;
>> + }
>> +
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
>> + if (off < 0) {
>> + dev_err(dev, "missing REG_LAYOUT param\n");
>> + return -EINVAL;
>> + }
>> +
>> + v = readq(dfh_base + off);
>> + 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",
>> + FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
>
> Why not use reg_width directly?
Good catch.
>
>> + switch (reg_width) {
>> + case 4:
>> + uart->port.iotype = UPIO_MEM32;
>> + break;
>> +
>> + case 2:
>> + uart->port.iotype = UPIO_MEM16;
>> + break;
>> +
>> + default:
>> + dev_err(dev, "invalid reg_width %lld\n", reg_width);
>
> unsupported ?
>
>> + return -EINVAL;
>> + }
>> +
>> + 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;
>> + resource_size_t res_size;
>> + void __iomem *dfh_base;
>> + int ret;
>> +
>> + memset(&uart, 0, sizeof(uart));
>> + uart.port.flags = UPF_IOREMAP;
>> + uart.port.mapbase = dfl_dev->csr_res.start;
>> + uart.port.mapsize = resource_size(&dfl_dev->csr_res);
>> +
>> + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
>> + if (!dfluart)
>> + return -ENOMEM;
>> +
>> + dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> + if (IS_ERR(dfh_base))
>> + return PTR_ERR(dfh_base);
>> +
>> + res_size = resource_size(&dfl_dev->mmio_res);
>> +
>> + ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
>> +
>> + devm_iounmap(dev, dfh_base);
>> + devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
>> +
>> + 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];
>> +
>> + /* register the port */
>
> This comment is pretty useless. Just drop it.
Will drop this useless comment.
>
>> + 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_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
>
> This you want to drop too. It seems a debug thing rather than info level
> stuff.
It is actually redundant output because serial8250_register_8250_port()
produces useful output. I will drop the line.
>
>
> --
> i.
>
>
On Thu, 6 Oct 2022, Andy Shevchenko wrote:
> On Thu, Oct 06, 2022 at 10:00:43AM -0700, [email protected] wrote:
>> On Tue, 4 Oct 2022, Andy Shevchenko wrote:
>>> On Tue, Oct 04, 2022 at 07:37:18AM -0700, [email protected] wrote:
>
> ...
>
>>>> Reported-by: kernel test robot <[email protected]>
>>>
>>> https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>>>
>>> "The Reported-by tag gives credit to people who find bugs and report them and it
>>> hopefully inspires them to help us again in the future. Please note that if the
>>> bug was reported in private, then ask for permission first before using the
>>> Reported-by tag. The tag is intended for bugs; please do not use it to credit
>>> feature requests."
>>
>> The kernel test robot did find a bug in my v1 submission. I was missing the
>> static keyword for a function declaration. Should I remove the tag?
>
> What's yours take from the above documentation?
>
Since the kernel test robot did find a bug. The tag should stay.
> ...
>
>>>> + dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>>>> + if (IS_ERR(dfh_base))
>>>> + return PTR_ERR(dfh_base);
>>>> +
>>>> + res_size = resource_size(&dfl_dev->mmio_res);
>>>> +
>>>> + ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
>>>
>>>> + devm_iounmap(dev, dfh_base);
>>>> + devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
>>>
>>> If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
>>> to begin with? The devm_* release type of functions in 99% of the cases
>>> indicate of the abusing devm_.
>>
>> I will change the code to call ioremap() and request_mem_region() directly
>> instead of the devm_ versions.
>
> But why will you need request_mem_region() in that case?
It doesn't seem that I need to call request_mem_regsion; so I will skip
it.
>
>>>> + if (ret < 0)
>>>> + return dev_err_probe(dev, ret, "failed uart feature walk\n");
>
> ...
>
>>>> +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.
>>>
>>> When m, what be the module name?
>>
>> I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed into
>> /lib/modules/... I also see "alias dfl:t0000f0024* 8250_dfl" in
>> modules.alias
>
> My point is that user who will run `make menuconfig` will read this and have
> no clue after the kernel build if the module was built or not. Look into other
> (recent) sections of the Kconfig for drivers in the kernel for how they inform
> user about the module name (this more or less standard pattern you just need
> to copy'n'paste'n'edit carefully).
>
> ...
I think this should be added:
To compile this driver as a module, chose M here: the
module will be called 8250_dfl.
>
>>>> obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
>>>> obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
>>>> obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
>>>> +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
>>>
>>> This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
>>> it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
>>> entries are not properly placed there and in Makefile.)
>>
>> Since 8250_dfl results in its own module, and my kernel config doesn't have
>> FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
>
> The Makefile is a bit chaotic, but try to find the sorted (more or less)
> group of drivers that are not 4 ports and squeeze your entry there
> (I expect somewhere between the LPSS/MID lines).
>
> It will help to sort out that mess in the future.
I will move 8250_dfl between LPSS and MID lines in the Makefile. Should I
move the definition in Kconfig to be between LPSS and MID to be consistent?
>
>>>> 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
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
On Thu, Oct 06, 2022 at 03:24:16PM -0700, [email protected] wrote:
> On Thu, 6 Oct 2022, Andy Shevchenko wrote:
> > On Thu, Oct 06, 2022 at 10:00:43AM -0700, [email protected] wrote:
> > > On Tue, 4 Oct 2022, Andy Shevchenko wrote:
> > > > On Tue, Oct 04, 2022 at 07:37:18AM -0700, [email protected] wrote:
...
> > > > > Reported-by: kernel test robot <[email protected]>
> > > >
> > > > https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> > > >
> > > > "The Reported-by tag gives credit to people who find bugs and report them and it
> > > > hopefully inspires them to help us again in the future. Please note that if the
> > > > bug was reported in private, then ask for permission first before using the
> > > > Reported-by tag. The tag is intended for bugs; please do not use it to credit
> > > > feature requests."
> > >
> > > The kernel test robot did find a bug in my v1 submission. I was missing the
> > > static keyword for a function declaration. Should I remove the tag?
> >
> > What's yours take from the above documentation?
>
> Since the kernel test robot did find a bug. The tag should stay.
I suggest otherwise because of the last sentence in the cited excerpt: "please
do not use it to credit feature requests". To distinguish "feature request" you
can ask yourself "Am I fixing _existing_ code or adding a new one?" And the
answer here is crystal clear (at least to me).
...
> > > > > +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.
> > > >
> > > > When m, what be the module name?
> > >
> > > I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed into
> > > /lib/modules/... I also see "alias dfl:t0000f0024* 8250_dfl" in
> > > modules.alias
> >
> > My point is that user who will run `make menuconfig` will read this and have
> > no clue after the kernel build if the module was built or not. Look into other
> > (recent) sections of the Kconfig for drivers in the kernel for how they inform
> > user about the module name (this more or less standard pattern you just need
> > to copy'n'paste'n'edit carefully).
>
> I think this should be added:
> To compile this driver as a module, chose M here: the
> module will be called 8250_dfl.
Looks good to me!
> > > > > obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
> > > > > obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
> > > > > obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
> > > > > +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
> > > >
> > > > This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
> > > > it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
> > > > entries are not properly placed there and in Makefile.)
> > >
> > > Since 8250_dfl results in its own module, and my kernel config doesn't have
> > > FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
> >
> > The Makefile is a bit chaotic, but try to find the sorted (more or less)
> > group of drivers that are not 4 ports and squeeze your entry there
> > (I expect somewhere between the LPSS/MID lines).
> >
> > It will help to sort out that mess in the future.
>
> I will move 8250_dfl between LPSS and MID lines in the Makefile. Should I
> move the definition in Kconfig to be between LPSS and MID to be consistent?
D is not ordered if put between L and M, I meant not to literally put it there
but think about it a bit.
Kconfig is another story because it has different approach in ordering (seems
so), try to find the best compromise there.
> > > > > 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
--
With Best Regards,
Andy Shevchenko
On Fri, 7 Oct 2022, Andy Shevchenko wrote:
> On Thu, Oct 06, 2022 at 03:24:16PM -0700, [email protected] wrote:
>> On Thu, 6 Oct 2022, Andy Shevchenko wrote:
>>> On Thu, Oct 06, 2022 at 10:00:43AM -0700, [email protected] wrote:
>>>> On Tue, 4 Oct 2022, Andy Shevchenko wrote:
>>>>> On Tue, Oct 04, 2022 at 07:37:18AM -0700, [email protected] wrote:
>
> ...
>
>>>>>> Reported-by: kernel test robot <[email protected]>
>>>>>
>>>>> https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>>>>>
>>>>> "The Reported-by tag gives credit to people who find bugs and report them and it
>>>>> hopefully inspires them to help us again in the future. Please note that if the
>>>>> bug was reported in private, then ask for permission first before using the
>>>>> Reported-by tag. The tag is intended for bugs; please do not use it to credit
>>>>> feature requests."
>>>>
>>>> The kernel test robot did find a bug in my v1 submission. I was missing the
>>>> static keyword for a function declaration. Should I remove the tag?
>>>
>>> What's yours take from the above documentation?
>>
>> Since the kernel test robot did find a bug. The tag should stay.
>
> I suggest otherwise because of the last sentence in the cited excerpt: "please
> do not use it to credit feature requests". To distinguish "feature request" you
> can ask yourself "Am I fixing _existing_ code or adding a new one?" And the
> answer here is crystal clear (at least to me).
>
> ...
>
>>>>>> +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.
>>>>>
>>>>> When m, what be the module name?
>>>>
>>>> I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed into
>>>> /lib/modules/... I also see "alias dfl:t0000f0024* 8250_dfl" in
>>>> modules.alias
>>>
>>> My point is that user who will run `make menuconfig` will read this and have
>>> no clue after the kernel build if the module was built or not. Look into other
>>> (recent) sections of the Kconfig for drivers in the kernel for how they inform
>>> user about the module name (this more or less standard pattern you just need
>>> to copy'n'paste'n'edit carefully).
>>
>> I think this should be added:
>> To compile this driver as a module, chose M here: the
>> module will be called 8250_dfl.
>
> Looks good to me!
>
>
>>>>>> obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
>>>>>> obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
>>>>>> obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
>>>>>> +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
>>>>>
>>>>> This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
>>>>> it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
>>>>> entries are not properly placed there and in Makefile.)
>>>>
>>>> Since 8250_dfl results in its own module, and my kernel config doesn't have
>>>> FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
>>>
>>> The Makefile is a bit chaotic, but try to find the sorted (more or less)
>>> group of drivers that are not 4 ports and squeeze your entry there
>>> (I expect somewhere between the LPSS/MID lines).
>>>
>>> It will help to sort out that mess in the future.
>>
>> I will move 8250_dfl between LPSS and MID lines in the Makefile. Should I
>> move the definition in Kconfig to be between LPSS and MID to be consistent?
>
> D is not ordered if put between L and M, I meant not to literally put it there
> but think about it a bit.
>
> Kconfig is another story because it has different approach in ordering (seems
> so), try to find the best compromise there.
In the Kconfig, I think the driver fits into the section, Misc.
options/drivers. Within this section, I think SERIAL_8250_DFL should go
before SERIAL_8250_DW to approximate alphabetical ordering. Similarly, I
think 8250_dfl.o should go above 8250_dw.o in the Makefile.
>
>>>>>> 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
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
On Fri, Oct 07, 2022 at 08:10:34AM -0700, [email protected] wrote:
> On Fri, 7 Oct 2022, Andy Shevchenko wrote:
...
> In the Kconfig, I think the driver fits into the section, Misc.
> options/drivers. Within this section, I think SERIAL_8250_DFL should go
> before SERIAL_8250_DW to approximate alphabetical ordering. Similarly, I
> think 8250_dfl.o should go above 8250_dw.o in the Makefile.
Sounds good to me!
--
With Best Regards,
Andy Shevchenko
On 2022-10-04 16:37, [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]>
> Reported-by: kernel test robot <[email protected]>
> ---
> 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 | 177 +++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 9 ++
> drivers/tty/serial/8250/Makefile | 1 +
> 3 files changed, 187 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..110ad3a73459
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,177 @@
> +// 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/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 device *dev, void __iomem *dfh_base, resource_size_t max,
> + struct uart_8250_port *uart)
> +{
> + u64 v, fifo_len, reg_width;
> + int off;
> +
> + if (!dfhv1_has_params(dfh_base)) {
> + dev_err(dev, "missing required DFH parameters\n");
> + return -EINVAL;
> + }
> +
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> + if (off < 0) {
> + dev_err(dev, "missing CLK_FRQ param\n");
> + return -EINVAL;
> + }
> +
> + uart->port.uartclk = readq(dfh_base + off);
> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> +
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> + if (off < 0) {
> + dev_err(dev, "missing FIFO_LEN param\n");
> + return -EINVAL;
> + }
> +
> + fifo_len = readq(dfh_base + off);
> + 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:
> + dev_err(dev, "bad fifo_len %llu\n", fifo_len);
> + return -EINVAL;
> + }
> +
> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> + if (off < 0) {
> + dev_err(dev, "missing REG_LAYOUT param\n");
> + return -EINVAL;
> + }
> +
> + v = readq(dfh_base + off);
> + 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",
> + FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
> +
> + switch (reg_width) {
> + case 4:
> + uart->port.iotype = UPIO_MEM32;
> + break;
> +
> + case 2:
> + uart->port.iotype = UPIO_MEM16;
> + break;
> +
> + default:
> + dev_err(dev, "invalid reg_width %lld\n", reg_width);
> + return -EINVAL;
> + }
> +
> + 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;
> + resource_size_t res_size;
> + void __iomem *dfh_base;
> + int ret;
> +
> + memset(&uart, 0, sizeof(uart));
> + uart.port.flags = UPF_IOREMAP;
> + uart.port.mapbase = dfl_dev->csr_res.start;
> + uart.port.mapsize = resource_size(&dfl_dev->csr_res);
> +
> + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> + if (!dfluart)
> + return -ENOMEM;
> +
> + dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> + if (IS_ERR(dfh_base))
> + return PTR_ERR(dfh_base);
> +
> + res_size = resource_size(&dfl_dev->mmio_res);
> +
> + ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
It seems to me that the dfl_uart driver supports only DFHv1 headers.
So why not checking dfl_dev->dfh_version in dfl_uart_probe() before
allocating, mapping, and then checking with dfl_uart_get_params()?
> +
> + devm_iounmap(dev, dfh_base);
> + devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
> +
> + 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];
> +
> + /* register the port */
> + 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_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
> + 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 },
> + { }
> +};
> +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..5c6497ce5c12 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -361,6 +361,15 @@ config SERIAL_8250_BCM2835AUX
>
> If unsure, say N.
>
> +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.
> +
> config SERIAL_8250_FSL
> bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
> depends on SERIAL_8250_CONSOLE
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index bee908f99ea0..32006e0982d1 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE) += 8250_early.o
> obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
> obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
> obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
> +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
> 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
Thanks,
Marco
On Mon, 10 Oct 2022, Marco Pagani wrote:
>
> On 2022-10-04 16:37, [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]>
>> Reported-by: kernel test robot <[email protected]>
>> ---
>> 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 | 177 +++++++++++++++++++++++++++++
>> drivers/tty/serial/8250/Kconfig | 9 ++
>> drivers/tty/serial/8250/Makefile | 1 +
>> 3 files changed, 187 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..110ad3a73459
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>> @@ -0,0 +1,177 @@
>> +// 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/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 device *dev, void __iomem *dfh_base, resource_size_t max,
>> + struct uart_8250_port *uart)
>> +{
>> + u64 v, fifo_len, reg_width;
>> + int off;
>> +
>> + if (!dfhv1_has_params(dfh_base)) {
>> + dev_err(dev, "missing required DFH parameters\n");
>> + return -EINVAL;
>> + }
>> +
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
>> + if (off < 0) {
>> + dev_err(dev, "missing CLK_FRQ param\n");
>> + return -EINVAL;
>> + }
>> +
>> + uart->port.uartclk = readq(dfh_base + off);
>> + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>> +
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
>> + if (off < 0) {
>> + dev_err(dev, "missing FIFO_LEN param\n");
>> + return -EINVAL;
>> + }
>> +
>> + fifo_len = readq(dfh_base + off);
>> + 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:
>> + dev_err(dev, "bad fifo_len %llu\n", fifo_len);
>> + return -EINVAL;
>> + }
>> +
>> + off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
>> + if (off < 0) {
>> + dev_err(dev, "missing REG_LAYOUT param\n");
>> + return -EINVAL;
>> + }
>> +
>> + v = readq(dfh_base + off);
>> + 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",
>> + FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
>> +
>> + switch (reg_width) {
>> + case 4:
>> + uart->port.iotype = UPIO_MEM32;
>> + break;
>> +
>> + case 2:
>> + uart->port.iotype = UPIO_MEM16;
>> + break;
>> +
>> + default:
>> + dev_err(dev, "invalid reg_width %lld\n", reg_width);
>> + return -EINVAL;
>> + }
>> +
>> + 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;
>> + resource_size_t res_size;
>> + void __iomem *dfh_base;
>> + int ret;
>> +
>> + memset(&uart, 0, sizeof(uart));
>> + uart.port.flags = UPF_IOREMAP;
>> + uart.port.mapbase = dfl_dev->csr_res.start;
>> + uart.port.mapsize = resource_size(&dfl_dev->csr_res);
>> +
>> + dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
>> + if (!dfluart)
>> + return -ENOMEM;
>> +
>> + dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> + if (IS_ERR(dfh_base))
>> + return PTR_ERR(dfh_base);
>> +
>> + res_size = resource_size(&dfl_dev->mmio_res);
>> +
>> + ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
>
>
> It seems to me that the dfl_uart driver supports only DFHv1 headers.
> So why not checking dfl_dev->dfh_version in dfl_uart_probe() before
> allocating, mapping, and then checking with dfl_uart_get_params()?
Checking dfl_dev->dfh_version at the top of dfl_uart_probe() is a good
suggestion. I can also move the call to devm_kzalloc until after the call
the dfl_uart_get_params.
>
>
>> +
>> + devm_iounmap(dev, dfh_base);
>> + devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
>> +
>> + 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];
>> +
>> + /* register the port */
>> + 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_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
>> + 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 },
>> + { }
>> +};
>> +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..5c6497ce5c12 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -361,6 +361,15 @@ config SERIAL_8250_BCM2835AUX
>>
>> If unsure, say N.
>>
>> +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.
>> +
>> config SERIAL_8250_FSL
>> bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
>> depends on SERIAL_8250_CONSOLE
>> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
>> index bee908f99ea0..32006e0982d1 100644
>> --- a/drivers/tty/serial/8250/Makefile
>> +++ b/drivers/tty/serial/8250/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE) += 8250_early.o
>> obj-$(CONFIG_SERIAL_8250_FOURPORT) += 8250_fourport.o
>> obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o
>> obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
>> +obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
>> 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
>
>
> Thanks,
> Marco
>
>