* [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
@ 2015-05-19 1:11 Masahiro Yamada
2015-05-19 8:22 ` Matthias Brugger
2015-05-20 12:56 ` Andy Shevchenko
0 siblings, 2 replies; 11+ messages in thread
From: Masahiro Yamada @ 2015-05-19 1:11 UTC (permalink / raw)
To: linux-serial
Cc: Joachim Eastwood, Matthias Brugger, Masahiro Yamada,
Sebastian Andrzej Siewior, Jiri Slaby, linux-arm-kernel,
Peter Hurley, Alan Cox, linux-kernel, Andy Shevchenko,
John Crispin, Ricardo Ribalda Delgado, Greg Kroah-Hartman,
Tony Lindgren
Add the driver for on-chip UART used on UniPhier SoCs.
This hardware is similar to 8250 with a slightly different register
mapping, so it should go into drivers/tty/serial/8250 directory.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v3:
- Just add *_SHIFT macro for the special case
Changes in v2:
- Drop unnecessary #include <linux/init.h>
- Sort includes in alphabetical order
- Use devm_clk_get() rather than of_clk_get()
- Delete unneeded clk_put() from uniphier_uart_remove callback
- Delete unneeded IS_ERR_OR_NULL check from uniphier_uart_remove callback
- Use UNIPHIER_UART_*_SHIFT instead of hard-coded shift values
- Change the first argument type of uniphier_of_serial_setup()
from (struct platform_device *) to (struct device *) for code-cleanup.
drivers/tty/serial/8250/8250_uniphier.c | 237 ++++++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 7 +
drivers/tty/serial/8250/Makefile | 1 +
3 files changed, 245 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_uniphier.c
diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
new file mode 100644
index 0000000..50349bf
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_uniphier.c
@@ -0,0 +1,237 @@
+/*
+ * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/serial_8250.h>
+#include <linux/serial_core.h>
+#include <linux/serial_reg.h>
+#include "8250.h"
+
+/* Most (but not all) of UniPhier UART devices have 64-depth FIFO. */
+#define UNIPHIER_UART_DEFAULT_FIFO_SIZE 64
+
+#define UNIPHIER_UART_CHAR_FCR 3 /* Character / FIFO Control Register */
+#define UNIPHIER_UART_LCR_MCR 4 /* Line/Modem Control Register */
+#define UNIPHIER_UART_LCR_SHIFT 8
+#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
+
+/*
+ * The register map is slightly different from that of 8250.
+ * IO callbacks must be overridden for correct access to FCR, LCR, and MCR.
+ */
+static unsigned int uniphier_serial_in(struct uart_port *p, int offset)
+{
+ int valshift = 0;
+
+ switch (offset) {
+ case UART_LCR:
+ valshift = UNIPHIER_UART_LCR_SHIFT;
+ case UART_MCR:
+ offset = UNIPHIER_UART_LCR_MCR;
+ break;
+ default:
+ break;
+ }
+
+ offset <<= p->regshift;
+
+ /*
+ * The return value must be masked with 0xff because LCR and MCR reside
+ * in the same register that must be accessed by 32-bit write/read.
+ * 8 or 16 bit access to this hardware result in unexpected behavior.
+ */
+ return (readl(p->membase + offset) >> valshift) & 0xff;
+}
+
+static void uniphier_serial_out(struct uart_port *p, int offset, int value)
+{
+ int valshift = 0;
+ bool normal = false;
+
+ switch (offset) {
+ case UART_FCR:
+ offset = UNIPHIER_UART_CHAR_FCR;
+ break;
+ case UART_LCR:
+ valshift = UNIPHIER_UART_LCR_SHIFT;
+ /* Divisor latch access bit does not exist. */
+ value &= ~(UART_LCR_DLAB << valshift);
+ case UART_MCR:
+ offset = UNIPHIER_UART_LCR_MCR;
+ break;
+ default:
+ normal = true;
+ break;
+ }
+
+ offset <<= p->regshift;
+
+ if (normal) {
+ writel(value, p->membase + offset);
+ } else {
+ /* special case: two registers share the same address. */
+ u32 tmp = readl(p->membase + offset);
+
+ tmp &= ~(0xff << valshift);
+ tmp |= value << valshift;
+ writel(tmp, p->membase + offset);
+ }
+}
+
+/*
+ * This hardware does not have the divisor latch access bit.
+ * The divisor latch register exists at different address.
+ * Override dl_read/write callbacks.
+ */
+static int uniphier_serial_dl_read(struct uart_8250_port *up)
+{
+ return readl(up->port.membase + UNIPHIER_UART_DLR);
+}
+
+static void uniphier_serial_dl_write(struct uart_8250_port *up, int value)
+{
+ writel(value, up->port.membase + UNIPHIER_UART_DLR);
+}
+
+struct uniphier8250_priv {
+ int line;
+ struct clk *clk;
+};
+
+static int uniphier_of_serial_setup(struct device *dev, struct uart_port *port,
+ struct uniphier8250_priv *priv)
+{
+ int ret;
+ u32 prop;
+ struct device_node *np = dev->of_node;
+
+ ret = of_alias_get_id(np, "serial");
+ if (ret < 0) {
+ dev_err(dev, "failed to get alias id\n");
+ return ret;
+ }
+ port->line = priv->line = ret;
+
+ /* Get clk rate through clk driver */
+ priv->clk = devm_clk_get(dev, 0);
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev, "failed to get clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret < 0)
+ return ret;
+
+ port->uartclk = clk_get_rate(priv->clk);
+
+ /* Check for fifo size */
+ if (of_property_read_u32(np, "fifo-size", &prop) == 0)
+ port->fifosize = prop;
+ else
+ port->fifosize = UNIPHIER_UART_DEFAULT_FIFO_SIZE;
+
+ return 0;
+}
+
+static int uniphier_uart_probe(struct platform_device *pdev)
+{
+ struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ struct device *dev = &pdev->dev;
+ struct uniphier8250_priv *priv;
+ struct uart_8250_port up;
+ int ret;
+ void __iomem *membase;
+
+ if (!regs || !irq) {
+ dev_err(dev, "missing registers or irq\n");
+ return -EINVAL;
+ }
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ membase = devm_ioremap(dev, regs->start, resource_size(regs));
+ if (!membase)
+ return -ENOMEM;
+
+ memset(&up, 0, sizeof(up));
+
+ ret = uniphier_of_serial_setup(dev, &up.port, priv);
+ if (ret < 0)
+ return ret;
+
+ up.port.dev = dev;
+ up.port.private_data = priv;
+ up.port.mapbase = regs->start;
+ up.port.membase = membase;
+ up.port.irq = irq->start;
+
+ up.port.type = PORT_16550A;
+ up.port.iotype = UPIO_MEM32;
+ up.port.regshift = 2;
+ up.port.flags = UPF_FIXED_PORT | UPF_FIXED_TYPE;
+ up.capabilities = UART_CAP_FIFO;
+
+ up.port.serial_in = uniphier_serial_in;
+ up.port.serial_out = uniphier_serial_out;
+ up.dl_read = uniphier_serial_dl_read;
+ up.dl_write = uniphier_serial_dl_write;
+
+ ret = serial8250_register_8250_port(&up);
+ if (ret < 0) {
+ dev_err(dev, "failed to register 8250 port\n");
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static int uniphier_uart_remove(struct platform_device *pdev)
+{
+ struct uniphier8250_priv *priv = platform_get_drvdata(pdev);
+
+ serial8250_unregister_port(priv->line);
+ clk_disable_unprepare(priv->clk);
+
+ return 0;
+}
+
+static const struct of_device_id uniphier_uart_match[] = {
+ { .compatible = "socionext,uniphier-uart" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, uniphier_uart_match);
+
+static struct platform_driver uniphier_uart_platform_driver = {
+ .probe = uniphier_uart_probe,
+ .remove = uniphier_uart_remove,
+ .driver = {
+ .name = "uniphier-uart",
+ .of_match_table = uniphier_uart_match,
+ },
+};
+module_platform_driver(uniphier_uart_platform_driver);
+
+MODULE_AUTHOR("Masahiro Yamada <yamada.masahiro@socionext.com>");
+MODULE_DESCRIPTION("UniPhier UART driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index c350703..3e1ae446 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -342,3 +342,10 @@ config SERIAL_8250_MT6577
help
If you have a Mediatek based board and want to use the
serial port, say Y to this option. If unsure, say N.
+
+config SERIAL_8250_UNIPHIER
+ tristate "Support for UniPhier on-chip UART"
+ depends on SERIAL_8250 && ARCH_UNIPHIER
+ help
+ If you have a UniPhier based board and want to use the on-chip
+ serial ports, say Y to this option. If unsure, say N.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 31e7cdc..f7ca8c3 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_SERIAL_8250_EM) += 8250_em.o
obj-$(CONFIG_SERIAL_8250_OMAP) += 8250_omap.o
obj-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
obj-$(CONFIG_SERIAL_8250_MT6577) += 8250_mtk.o
+obj-$(CONFIG_SERIAL_8250_UNIPHIER) += 8250_uniphier.o
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 1:11 [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver Masahiro Yamada
@ 2015-05-19 8:22 ` Matthias Brugger
2015-05-19 8:54 ` Masahiro Yamada
2015-05-20 12:56 ` Andy Shevchenko
1 sibling, 1 reply; 11+ messages in thread
From: Matthias Brugger @ 2015-05-19 8:22 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-serial, Joachim Eastwood, Sebastian Andrzej Siewior,
Jiri Slaby, linux-arm-kernel, Peter Hurley, Alan Cox,
linux-kernel, Andy Shevchenko, John Crispin,
Ricardo Ribalda Delgado, Greg Kroah-Hartman, Tony Lindgren
2015-05-19 3:11 GMT+02:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Add the driver for on-chip UART used on UniPhier SoCs.
>
> This hardware is similar to 8250 with a slightly different register
> mapping, so it should go into drivers/tty/serial/8250 directory.
The patch is for a driver in this folder, so no need to but it in the
commit message.
Instead just write a short description about the difference to a
standard 8250 device.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v3:
> - Just add *_SHIFT macro for the special case
>
> Changes in v2:
> - Drop unnecessary #include <linux/init.h>
> - Sort includes in alphabetical order
> - Use devm_clk_get() rather than of_clk_get()
> - Delete unneeded clk_put() from uniphier_uart_remove callback
> - Delete unneeded IS_ERR_OR_NULL check from uniphier_uart_remove callback
> - Use UNIPHIER_UART_*_SHIFT instead of hard-coded shift values
> - Change the first argument type of uniphier_of_serial_setup()
> from (struct platform_device *) to (struct device *) for code-cleanup.
>
> drivers/tty/serial/8250/8250_uniphier.c | 237 ++++++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 7 +
> drivers/tty/serial/8250/Makefile | 1 +
> 3 files changed, 245 insertions(+)
> create mode 100644 drivers/tty/serial/8250/8250_uniphier.c
>
> diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
> new file mode 100644
> index 0000000..50349bf
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_uniphier.c
> @@ -0,0 +1,237 @@
> +/*
> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/serial_8250.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>
> +#include "8250.h"
> +
> +/* Most (but not all) of UniPhier UART devices have 64-depth FIFO. */
> +#define UNIPHIER_UART_DEFAULT_FIFO_SIZE 64
> +
> +#define UNIPHIER_UART_CHAR_FCR 3 /* Character / FIFO Control Register */
> +#define UNIPHIER_UART_LCR_MCR 4 /* Line/Modem Control Register */
> +#define UNIPHIER_UART_LCR_SHIFT 8
> +#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
Nit pick: Please fix identation. After "define" just put one blank.
I personally prefer that the define values are aligned by tabs, but
that's up tu you.
With these changes you can add:
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Thanks,
Matthias
--
motzblog.wordpress.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 8:22 ` Matthias Brugger
@ 2015-05-19 8:54 ` Masahiro Yamada
2015-05-19 8:57 ` Jiri Slaby
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Masahiro Yamada @ 2015-05-19 8:54 UTC (permalink / raw)
To: Matthias Brugger
Cc: Peter Hurley, John Crispin, Tony Lindgren, Greg Kroah-Hartman,
Joachim Eastwood, linux-kernel, linux-serial,
Ricardo Ribalda Delgado, Andy Shevchenko, Jiri Slaby,
Sebastian Andrzej Siewior, linux-arm-kernel, Alan Cox
Hi Matthias,
2015-05-19 17:22 GMT+09:00 Matthias Brugger <matthias.bgg@gmail.com>:
> 2015-05-19 3:11 GMT+02:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
>> Add the driver for on-chip UART used on UniPhier SoCs.
>>
>> This hardware is similar to 8250 with a slightly different register
>> mapping, so it should go into drivers/tty/serial/8250 directory.
>
> The patch is for a driver in this folder, so no need to but it in the
> commit message.
> Instead just write a short description about the difference to a
> standard 8250 device.
>
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Changes in v3:
>> - Just add *_SHIFT macro for the special case
>>
>> Changes in v2:
>> - Drop unnecessary #include <linux/init.h>
>> - Sort includes in alphabetical order
>> - Use devm_clk_get() rather than of_clk_get()
>> - Delete unneeded clk_put() from uniphier_uart_remove callback
>> - Delete unneeded IS_ERR_OR_NULL check from uniphier_uart_remove callback
>> - Use UNIPHIER_UART_*_SHIFT instead of hard-coded shift values
>> - Change the first argument type of uniphier_of_serial_setup()
>> from (struct platform_device *) to (struct device *) for code-cleanup.
>>
>> drivers/tty/serial/8250/8250_uniphier.c | 237 ++++++++++++++++++++++++++++++++
>> drivers/tty/serial/8250/Kconfig | 7 +
>> drivers/tty/serial/8250/Makefile | 1 +
>> 3 files changed, 245 insertions(+)
>> create mode 100644 drivers/tty/serial/8250/8250_uniphier.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
>> new file mode 100644
>> index 0000000..50349bf
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_uniphier.c
>> @@ -0,0 +1,237 @@
>> +/*
>> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/serial_8250.h>
>> +#include <linux/serial_core.h>
>> +#include <linux/serial_reg.h>
>> +#include "8250.h"
>> +
>> +/* Most (but not all) of UniPhier UART devices have 64-depth FIFO. */
>> +#define UNIPHIER_UART_DEFAULT_FIFO_SIZE 64
>> +
>> +#define UNIPHIER_UART_CHAR_FCR 3 /* Character / FIFO Control Register */
>> +#define UNIPHIER_UART_LCR_MCR 4 /* Line/Modem Control Register */
>> +#define UNIPHIER_UART_LCR_SHIFT 8
>> +#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
>
> Nit pick: Please fix identation. After "define" just put one blank.
> I personally prefer that the define values are aligned by tabs, but
> that's up tu you.
>
I intentionally use deeper indentation for *_SHIFT
because I want to clearly show UNIPHIER_UART_LCR_SHIFT
belongs to UNIPHIER_UART_LCR_MCR register.
I also want to describe them within 80 columns including comments,
so I cannot insert extra TABs to align the values.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 8:54 ` Masahiro Yamada
@ 2015-05-19 8:57 ` Jiri Slaby
2015-05-19 9:10 ` Masahiro Yamada
2015-05-19 9:48 ` One Thousand Gnomes
2015-05-19 10:13 ` Matthias Brugger
2 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2015-05-19 8:57 UTC (permalink / raw)
To: Masahiro Yamada, Matthias Brugger
Cc: Peter Hurley, John Crispin, Tony Lindgren, Greg Kroah-Hartman,
Joachim Eastwood, linux-kernel, linux-serial,
Ricardo Ribalda Delgado, Andy Shevchenko,
Sebastian Andrzej Siewior, linux-arm-kernel, Alan Cox
On 05/19/2015, 10:54 AM, Masahiro Yamada wrote:
>>> +#define UNIPHIER_UART_CHAR_FCR 3 /* Character / FIFO Control Register */
>>> +#define UNIPHIER_UART_LCR_MCR 4 /* Line/Modem Control Register */
>>> +#define UNIPHIER_UART_LCR_SHIFT 8
>>> +#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
>>
>> Nit pick: Please fix identation. After "define" just put one blank.
>> I personally prefer that the define values are aligned by tabs, but
>> that's up tu you.
>>
>
> I intentionally use deeper indentation for *_SHIFT
> because I want to clearly show UNIPHIER_UART_LCR_SHIFT
> belongs to UNIPHIER_UART_LCR_MCR register.
>
> I also want to describe them within 80 columns including comments,
> so I cannot insert extra TABs to align the values.
Hi, what about one space between # and define which is used sometimes?
--
js
suse labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 8:57 ` Jiri Slaby
@ 2015-05-19 9:10 ` Masahiro Yamada
0 siblings, 0 replies; 11+ messages in thread
From: Masahiro Yamada @ 2015-05-19 9:10 UTC (permalink / raw)
To: Jiri Slaby
Cc: Matthias Brugger, Peter Hurley, Alan Cox, Tony Lindgren,
Greg Kroah-Hartman, Joachim Eastwood, linux-kernel, linux-serial,
Ricardo Ribalda Delgado, Andy Shevchenko,
Sebastian Andrzej Siewior, linux-arm-kernel, John Crispin
2015-05-19 17:57 GMT+09:00 Jiri Slaby <jslaby@suse.cz>:
> On 05/19/2015, 10:54 AM, Masahiro Yamada wrote:
>>>> +#define UNIPHIER_UART_CHAR_FCR 3 /* Character / FIFO Control Register */
>>>> +#define UNIPHIER_UART_LCR_MCR 4 /* Line/Modem Control Register */
>>>> +#define UNIPHIER_UART_LCR_SHIFT 8
>>>> +#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
>>>
>>> Nit pick: Please fix identation. After "define" just put one blank.
>>> I personally prefer that the define values are aligned by tabs, but
>>> that's up tu you.
>>>
>>
>> I intentionally use deeper indentation for *_SHIFT
>> because I want to clearly show UNIPHIER_UART_LCR_SHIFT
>> belongs to UNIPHIER_UART_LCR_MCR register.
>>
>> I also want to describe them within 80 columns including comments,
>> so I cannot insert extra TABs to align the values.
>
> Hi, what about one space between # and define which is used sometimes?
With more one space after #, '8' goes to the next stop,
so it makes no difference.
Is this a problem? It almost seems a bike shed...
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 8:54 ` Masahiro Yamada
2015-05-19 8:57 ` Jiri Slaby
@ 2015-05-19 9:48 ` One Thousand Gnomes
2015-05-19 11:18 ` Masahiro Yamada
2015-05-19 10:13 ` Matthias Brugger
2 siblings, 1 reply; 11+ messages in thread
From: One Thousand Gnomes @ 2015-05-19 9:48 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Matthias Brugger, Peter Hurley, John Crispin, Tony Lindgren,
Greg Kroah-Hartman, Joachim Eastwood, linux-kernel, linux-serial,
Ricardo Ribalda Delgado, Andy Shevchenko, Jiri Slaby,
Sebastian Andrzej Siewior,
linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
> I intentionally use deeper indentation for *_SHIFT
> because I want to clearly show UNIPHIER_UART_LCR_SHIFT
> belongs to UNIPHIER_UART_LCR_MCR register.
Seems sensible to do it that way to me and a lot of other bits of the
kernel do.
The only other question I have is about the unipher_serial_out. If I am
writing a "special" register then the sequence becomes
- read 32bits
- modify
- write 32bits
That means that it's no longer atomic safe as the kernel expects.
Checking the users FCR seems safe, LCR is probably safe and MCR likewise
so I don't see a problem but I think it's worth noting in case anyone
else does.
Finally can you add a comment in serial_in and serial_out where one
switch case drops through into the next so its obvious to anyone looking
at Coverity and other analyser output that this drop through was
intentional ?
Alan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 8:54 ` Masahiro Yamada
2015-05-19 8:57 ` Jiri Slaby
2015-05-19 9:48 ` One Thousand Gnomes
@ 2015-05-19 10:13 ` Matthias Brugger
2 siblings, 0 replies; 11+ messages in thread
From: Matthias Brugger @ 2015-05-19 10:13 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Peter Hurley, John Crispin, Tony Lindgren, Greg Kroah-Hartman,
Joachim Eastwood, linux-kernel, linux-serial,
Ricardo Ribalda Delgado, Andy Shevchenko, Jiri Slaby,
Sebastian Andrzej Siewior, linux-arm-kernel, Alan Cox
2015-05-19 10:54 GMT+02:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Matthias,
>
> 2015-05-19 17:22 GMT+09:00 Matthias Brugger <matthias.bgg@gmail.com>:
>> 2015-05-19 3:11 GMT+02:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
>>> Add the driver for on-chip UART used on UniPhier SoCs.
>>>
>>> This hardware is similar to 8250 with a slightly different register
>>> mapping, so it should go into drivers/tty/serial/8250 directory.
>>
>> The patch is for a driver in this folder, so no need to but it in the
>> commit message.
>> Instead just write a short description about the difference to a
>> standard 8250 device.
>>
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>> Changes in v3:
>>> - Just add *_SHIFT macro for the special case
>>>
>>> Changes in v2:
>>> - Drop unnecessary #include <linux/init.h>
>>> - Sort includes in alphabetical order
>>> - Use devm_clk_get() rather than of_clk_get()
>>> - Delete unneeded clk_put() from uniphier_uart_remove callback
>>> - Delete unneeded IS_ERR_OR_NULL check from uniphier_uart_remove callback
>>> - Use UNIPHIER_UART_*_SHIFT instead of hard-coded shift values
>>> - Change the first argument type of uniphier_of_serial_setup()
>>> from (struct platform_device *) to (struct device *) for code-cleanup.
>>>
>>> drivers/tty/serial/8250/8250_uniphier.c | 237 ++++++++++++++++++++++++++++++++
>>> drivers/tty/serial/8250/Kconfig | 7 +
>>> drivers/tty/serial/8250/Makefile | 1 +
>>> 3 files changed, 245 insertions(+)
>>> create mode 100644 drivers/tty/serial/8250/8250_uniphier.c
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
>>> new file mode 100644
>>> index 0000000..50349bf
>>> --- /dev/null
>>> +++ b/drivers/tty/serial/8250/8250_uniphier.c
>>> @@ -0,0 +1,237 @@
>>> +/*
>>> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/serial_8250.h>
>>> +#include <linux/serial_core.h>
>>> +#include <linux/serial_reg.h>
>>> +#include "8250.h"
>>> +
>>> +/* Most (but not all) of UniPhier UART devices have 64-depth FIFO. */
>>> +#define UNIPHIER_UART_DEFAULT_FIFO_SIZE 64
>>> +
>>> +#define UNIPHIER_UART_CHAR_FCR 3 /* Character / FIFO Control Register */
>>> +#define UNIPHIER_UART_LCR_MCR 4 /* Line/Modem Control Register */
>>> +#define UNIPHIER_UART_LCR_SHIFT 8
>>> +#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
>>
>> Nit pick: Please fix identation. After "define" just put one blank.
>> I personally prefer that the define values are aligned by tabs, but
>> that's up tu you.
>>
>
> I intentionally use deeper indentation for *_SHIFT
> because I want to clearly show UNIPHIER_UART_LCR_SHIFT
> belongs to UNIPHIER_UART_LCR_MCR register.
>
> I also want to describe them within 80 columns including comments,
> so I cannot insert extra TABs to align the values.
Ok, than from my side that's fine as it is.
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
--
motzblog.wordpress.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 9:48 ` One Thousand Gnomes
@ 2015-05-19 11:18 ` Masahiro Yamada
2015-05-19 11:31 ` Matthias Brugger
0 siblings, 1 reply; 11+ messages in thread
From: Masahiro Yamada @ 2015-05-19 11:18 UTC (permalink / raw)
To: One Thousand Gnomes
Cc: Matthias Brugger, Peter Hurley, John Crispin, Tony Lindgren,
Greg Kroah-Hartman, Joachim Eastwood, linux-kernel, linux-serial,
Ricardo Ribalda Delgado, Andy Shevchenko, Jiri Slaby,
Sebastian Andrzej Siewior,
linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Hi Alan,
2015-05-19 18:48 GMT+09:00 One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>:
>> I intentionally use deeper indentation for *_SHIFT
>> because I want to clearly show UNIPHIER_UART_LCR_SHIFT
>> belongs to UNIPHIER_UART_LCR_MCR register.
>
> Seems sensible to do it that way to me and a lot of other bits of the
> kernel do.
>
> The only other question I have is about the unipher_serial_out. If I am
> writing a "special" register then the sequence becomes
>
> - read 32bits
> - modify
> - write 32bits
>
> That means that it's no longer atomic safe as the kernel expects.
> Checking the users FCR seems safe, LCR is probably safe and MCR likewise
> so I don't see a problem but I think it's worth noting in case anyone
> else does.
Uh, I missed this.
I am a bit afraid what if LCR and MCR are updated at the same time.
Is it better to add mutex for writing special case registers?
if (normal) {
writel(value, p->membase + offset);
} else {
/* special case: two registers share the same address. */
u32 tmp = readl(p->membase + offset);
struct uniphier8250_priv *priv = p->private_data;
mutex_lock(&priv->atomic_write_lock);
tmp &= ~(0xff << valshift);
tmp |= value << valshift;
writel(tmp, p->membase + offset);
mutex_unlock(&priv->atomic_write_lock);
}
If it is OK, I can fix it in v4.
> Finally can you add a comment in serial_in and serial_out where one
> switch case drops through into the next so its obvious to anyone looking
> at Coverity and other analyser output that this drop through was
> intentional ?
I thought about it, too.
My previous version was as follows:
+#define UNIPHIER_UART_CHAR_FCR 3
+#define UNIPHIER_UART_CHAR_SHIFT 8 /* Character Register */
+#define UNIPHIER_UART_FCR_SHIFT 0 /* FIFO Control Register */
+#define UNIPHIER_UART_LCR_MCR 4
+#define UNIPHIER_UART_LCR_SHIFT 8 /* Line Control Register */
+#define UNIPHIER_UART_MCR_SHIFT 0 /* Modem Control Register */
+#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
[snip]
+static void uniphier_serial_out(struct uart_port *p, int offset, int value)
+{
+ int valshift = 0;
+ bool normal = false;
+
+ switch (offset) {
+ case UART_FCR:
+ offset = UNIPHIER_UART_CHAR_FCR;
+ valshift = UNIPHIER_UART_FCR_SHIFT;
+ break;
+ case UART_LCR:
+ offset = UNIPHIER_UART_LCR_MCR;
+ valshift = UNIPHIER_UART_LCR_SHIFT;
+ /* Divisor latch access bit does not exist. */
+ value &= ~(UART_LCR_DLAB << valshift);
+ break;
+ case UART_MCR:
+ offset = UNIPHIER_UART_LCR_MCR;
+ valshift = UNIPHIER_UART_MCR_SHIFT;
+ break;
+ default:
+ normal = true;
+ break;
+ }
I thought it was clear to anyone although it was a bit redundant and
Matthias was opposed to it.
I personally prefer clear code to tricky code that requires comments.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 11:18 ` Masahiro Yamada
@ 2015-05-19 11:31 ` Matthias Brugger
0 siblings, 0 replies; 11+ messages in thread
From: Matthias Brugger @ 2015-05-19 11:31 UTC (permalink / raw)
To: Masahiro Yamada
Cc: One Thousand Gnomes, Peter Hurley, John Crispin, Tony Lindgren,
Greg Kroah-Hartman, Joachim Eastwood, linux-kernel, linux-serial,
Ricardo Ribalda Delgado, Andy Shevchenko, Jiri Slaby,
Sebastian Andrzej Siewior,
linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
2015-05-19 13:18 GMT+02:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Alan,
>
>
> 2015-05-19 18:48 GMT+09:00 One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>:
>>> I intentionally use deeper indentation for *_SHIFT
>>> because I want to clearly show UNIPHIER_UART_LCR_SHIFT
>>> belongs to UNIPHIER_UART_LCR_MCR register.
>>
>> Seems sensible to do it that way to me and a lot of other bits of the
>> kernel do.
>>
>> The only other question I have is about the unipher_serial_out. If I am
>> writing a "special" register then the sequence becomes
>>
>> - read 32bits
>> - modify
>> - write 32bits
>>
>> That means that it's no longer atomic safe as the kernel expects.
>> Checking the users FCR seems safe, LCR is probably safe and MCR likewise
>> so I don't see a problem but I think it's worth noting in case anyone
>> else does.
>
> Uh, I missed this.
> I am a bit afraid what if LCR and MCR are updated at the same time.
>
> Is it better to add mutex for writing special case registers?
>
> if (normal) {
> writel(value, p->membase + offset);
> } else {
> /* special case: two registers share the same address. */
> u32 tmp = readl(p->membase + offset);
> struct uniphier8250_priv *priv = p->private_data;
>
> mutex_lock(&priv->atomic_write_lock);
> tmp &= ~(0xff << valshift);
> tmp |= value << valshift;
> writel(tmp, p->membase + offset);
> mutex_unlock(&priv->atomic_write_lock);
> }
>
>
> If it is OK, I can fix it in v4.
>
>
>
>
>> Finally can you add a comment in serial_in and serial_out where one
>> switch case drops through into the next so its obvious to anyone looking
>> at Coverity and other analyser output that this drop through was
>> intentional ?
>
> I thought about it, too.
>
> My previous version was as follows:
>
>
> +#define UNIPHIER_UART_CHAR_FCR 3
> +#define UNIPHIER_UART_CHAR_SHIFT 8 /* Character Register */
> +#define UNIPHIER_UART_FCR_SHIFT 0 /* FIFO Control Register */
> +#define UNIPHIER_UART_LCR_MCR 4
> +#define UNIPHIER_UART_LCR_SHIFT 8 /* Line Control Register */
> +#define UNIPHIER_UART_MCR_SHIFT 0 /* Modem Control Register */
> +#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
>
> [snip]
>
> +static void uniphier_serial_out(struct uart_port *p, int offset, int value)
> +{
> + int valshift = 0;
> + bool normal = false;
> +
> + switch (offset) {
> + case UART_FCR:
> + offset = UNIPHIER_UART_CHAR_FCR;
> + valshift = UNIPHIER_UART_FCR_SHIFT;
> + break;
> + case UART_LCR:
> + offset = UNIPHIER_UART_LCR_MCR;
> + valshift = UNIPHIER_UART_LCR_SHIFT;
> + /* Divisor latch access bit does not exist. */
> + value &= ~(UART_LCR_DLAB << valshift);
> + break;
> + case UART_MCR:
> + offset = UNIPHIER_UART_LCR_MCR;
> + valshift = UNIPHIER_UART_MCR_SHIFT;
> + break;
> + default:
> + normal = true;
> + break;
> + }
>
>
>
> I thought it was clear to anyone although it was a bit redundant and
> Matthias was opposed to it.
>
> I personally prefer clear code to tricky code that requires comments.
Me too :)
What I wanted to say was, that in the case statement you don't need to
set valshift to zero as this is the default value when entering
uniphier_serial_out. This way you can get rid of
UNIPHIER_UART_MCR_SHIFT and UNIPHIER_UART_FCR_SHIFT.
Sorry, I didn't explained myself.
Cheers,
Matthias
--
motzblog.wordpress.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-19 1:11 [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver Masahiro Yamada
2015-05-19 8:22 ` Matthias Brugger
@ 2015-05-20 12:56 ` Andy Shevchenko
2015-05-23 12:55 ` Masahiro Yamada
1 sibling, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2015-05-20 12:56 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-serial, Joachim Eastwood, Matthias Brugger,
Sebastian Andrzej Siewior, Jiri Slaby, linux-arm-kernel,
Peter Hurley, Alan Cox, linux-kernel, John Crispin,
Ricardo Ribalda Delgado, Greg Kroah-Hartman, Tony Lindgren
On Tue, 2015-05-19 at 10:11 +0900, Masahiro Yamada wrote:
> Add the driver for on-chip UART used on UniPhier SoCs.
>
> This hardware is similar to 8250 with a slightly different register
> mapping, so it should go into drivers/tty/serial/8250 directory.
Few comments below.
First of all what is the differences with the original 8250? It worth to
list them here in the changelog.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v3:
> - Just add *_SHIFT macro for the special case
>
> Changes in v2:
> - Drop unnecessary #include <linux/init.h>
> - Sort includes in alphabetical order
> - Use devm_clk_get() rather than of_clk_get()
> - Delete unneeded clk_put() from uniphier_uart_remove callback
> - Delete unneeded IS_ERR_OR_NULL check from uniphier_uart_remove callback
> - Use UNIPHIER_UART_*_SHIFT instead of hard-coded shift values
> - Change the first argument type of uniphier_of_serial_setup()
> from (struct platform_device *) to (struct device *) for code-cleanup.
>
> drivers/tty/serial/8250/8250_uniphier.c | 237 ++++++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 7 +
> drivers/tty/serial/8250/Makefile | 1 +
> 3 files changed, 245 insertions(+)
> create mode 100644 drivers/tty/serial/8250/8250_uniphier.c
>
> diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
> new file mode 100644
> index 0000000..50349bf
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_uniphier.c
> @@ -0,0 +1,237 @@
> +/*
> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/serial_8250.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>
Shouldn't be enough to have only serial_8250.h here?
+ empty line between #include <> and "".
> +#include "8250.h"
> +
> +/* Most (but not all) of UniPhier UART devices have 64-depth FIFO. */
> +#define UNIPHIER_UART_DEFAULT_FIFO_SIZE 64
> +
> +#define UNIPHIER_UART_CHAR_FCR 3 /* Character / FIFO Control Register */
> +#define UNIPHIER_UART_LCR_MCR 4 /* Line/Modem Control Register */
> +#define UNIPHIER_UART_LCR_SHIFT 8
Use space after #define.
> +#define UNIPHIER_UART_DLR 9 /* Divisor Latch Register */
> +
> +/*
> + * The register map is slightly different from that of 8250.
> + * IO callbacks must be overridden for correct access to FCR, LCR, and MCR.
> + */
> +static unsigned int uniphier_serial_in(struct uart_port *p, int offset)
> +{
> + int valshift = 0;
> +
> + switch (offset) {
> + case UART_LCR:
> + valshift = UNIPHIER_UART_LCR_SHIFT;
> + case UART_MCR:
> + offset = UNIPHIER_UART_LCR_MCR;
> + break;
> + default:
> + break;
> + }
> +
> + offset <<= p->regshift;
> +
> + /*
> + * The return value must be masked with 0xff because LCR and MCR reside
> + * in the same register that must be accessed by 32-bit write/read.
> + * 8 or 16 bit access to this hardware result in unexpected behavior.
> + */
> + return (readl(p->membase + offset) >> valshift) & 0xff;
> +}
> +
> +static void uniphier_serial_out(struct uart_port *p, int offset, int value)
> +{
> + int valshift = 0;
> + bool normal = false;
> +
> + switch (offset) {
> + case UART_FCR:
> + offset = UNIPHIER_UART_CHAR_FCR;
> + break;
> + case UART_LCR:
> + valshift = UNIPHIER_UART_LCR_SHIFT;
> + /* Divisor latch access bit does not exist. */
> + value &= ~(UART_LCR_DLAB << valshift);
> + case UART_MCR:
> + offset = UNIPHIER_UART_LCR_MCR;
> + break;
> + default:
> + normal = true;
> + break;
> + }
> +
> + offset <<= p->regshift;
> +
> + if (normal) {
> + writel(value, p->membase + offset);
> + } else {
> + /* special case: two registers share the same address. */
> + u32 tmp = readl(p->membase + offset);
> +
> + tmp &= ~(0xff << valshift);
> + tmp |= value << valshift;
> + writel(tmp, p->membase + offset);
> + }
> +}
> +
> +/*
> + * This hardware does not have the divisor latch access bit.
> + * The divisor latch register exists at different address.
> + * Override dl_read/write callbacks.
> + */
> +static int uniphier_serial_dl_read(struct uart_8250_port *up)
> +{
> + return readl(up->port.membase + UNIPHIER_UART_DLR);
> +}
> +
> +static void uniphier_serial_dl_write(struct uart_8250_port *up, int value)
> +{
> + writel(value, up->port.membase + UNIPHIER_UART_DLR);
> +}
> +
> +struct uniphier8250_priv {
> + int line;
> + struct clk *clk;
> +};
> +
> +static int uniphier_of_serial_setup(struct device *dev, struct uart_port *port,
> + struct uniphier8250_priv *priv)
> +{
> + int ret;
> + u32 prop;
> + struct device_node *np = dev->of_node;
> +
> + ret = of_alias_get_id(np, "serial");
> + if (ret < 0) {
> + dev_err(dev, "failed to get alias id\n");
> + return ret;
> + }
> + port->line = priv->line = ret;
> +
> + /* Get clk rate through clk driver */
> + priv->clk = devm_clk_get(dev, 0);
> + if (IS_ERR(priv->clk)) {
> + dev_err(dev, "failed to get clock\n");
> + return PTR_ERR(priv->clk);
> + }
> +
> + ret = clk_prepare_enable(priv->clk);
> + if (ret < 0)
> + return ret;
> +
> + port->uartclk = clk_get_rate(priv->clk);
> +
> + /* Check for fifo size */
> + if (of_property_read_u32(np, "fifo-size", &prop) == 0)
> + port->fifosize = prop;
> + else
> + port->fifosize = UNIPHIER_UART_DEFAULT_FIFO_SIZE;
> +
> + return 0;
> +}
> +
> +static int uniphier_uart_probe(struct platform_device *pdev)
> +{
> + struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
Please use platform_get_irq().
> + struct device *dev = &pdev->dev;
> + struct uniphier8250_priv *priv;
> + struct uart_8250_port up;
> + int ret;
> + void __iomem *membase;
> +
> + if (!regs || !irq) {
> + dev_err(dev, "missing registers or irq\n");
> + return -EINVAL;
> + }
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + membase = devm_ioremap(dev, regs->start, resource_size(regs));
Shouldn't be devm_ioremap_resource() ?
> + if (!membase)
> + return -ENOMEM;
> +
> + memset(&up, 0, sizeof(up));
> +
> + ret = uniphier_of_serial_setup(dev, &up.port, priv);
> + if (ret < 0)
> + return ret;
> +
> + up.port.dev = dev;
> + up.port.private_data = priv;
> + up.port.mapbase = regs->start;
> + up.port.membase = membase;
> + up.port.irq = irq->start;
> +
> + up.port.type = PORT_16550A;
> + up.port.iotype = UPIO_MEM32;
> + up.port.regshift = 2;
> + up.port.flags = UPF_FIXED_PORT | UPF_FIXED_TYPE;
> + up.capabilities = UART_CAP_FIFO;
> +
> + up.port.serial_in = uniphier_serial_in;
> + up.port.serial_out = uniphier_serial_out;
> + up.dl_read = uniphier_serial_dl_read;
> + up.dl_write = uniphier_serial_dl_write;
> +
> + ret = serial8250_register_8250_port(&up);
> + if (ret < 0) {
> + dev_err(dev, "failed to register 8250 port\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static int uniphier_uart_remove(struct platform_device *pdev)
> +{
> + struct uniphier8250_priv *priv = platform_get_drvdata(pdev);
> +
> + serial8250_unregister_port(priv->line);
> + clk_disable_unprepare(priv->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id uniphier_uart_match[] = {
> + { .compatible = "socionext,uniphier-uart" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, uniphier_uart_match);
> +
> +static struct platform_driver uniphier_uart_platform_driver = {
> + .probe = uniphier_uart_probe,
> + .remove = uniphier_uart_remove,
> + .driver = {
> + .name = "uniphier-uart",
> + .of_match_table = uniphier_uart_match,
> + },
> +};
> +module_platform_driver(uniphier_uart_platform_driver);
> +
> +MODULE_AUTHOR("Masahiro Yamada <yamada.masahiro@socionext.com>");
> +MODULE_DESCRIPTION("UniPhier UART driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index c350703..3e1ae446 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -342,3 +342,10 @@ config SERIAL_8250_MT6577
> help
> If you have a Mediatek based board and want to use the
> serial port, say Y to this option. If unsure, say N.
> +
> +config SERIAL_8250_UNIPHIER
> + tristate "Support for UniPhier on-chip UART"
> + depends on SERIAL_8250 && ARCH_UNIPHIER
> + help
> + If you have a UniPhier based board and want to use the on-chip
> + serial ports, say Y to this option. If unsure, say N.
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index 31e7cdc..f7ca8c3 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -23,3 +23,4 @@ obj-$(CONFIG_SERIAL_8250_EM) += 8250_em.o
> obj-$(CONFIG_SERIAL_8250_OMAP) += 8250_omap.o
> obj-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
> obj-$(CONFIG_SERIAL_8250_MT6577) += 8250_mtk.o
> +obj-$(CONFIG_SERIAL_8250_UNIPHIER) += 8250_uniphier.o
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver
2015-05-20 12:56 ` Andy Shevchenko
@ 2015-05-23 12:55 ` Masahiro Yamada
0 siblings, 0 replies; 11+ messages in thread
From: Masahiro Yamada @ 2015-05-23 12:55 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Peter Hurley, Tony Lindgren, Greg Kroah-Hartman,
Joachim Eastwood, Linux Kernel Mailing List, linux-serial,
Matthias Brugger, Ricardo Ribalda Delgado, John Crispin,
Jiri Slaby, Sebastian Andrzej Siewior, linux-arm-kernel,
Alan Cox
Hi Andy,
Thanks for reviewing my patch.
2015-05-20 21:56 GMT+09:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Tue, 2015-05-19 at 10:11 +0900, Masahiro Yamada wrote:
>> Add the driver for on-chip UART used on UniPhier SoCs.
>>
>> This hardware is similar to 8250 with a slightly different register
>> mapping, so it should go into drivers/tty/serial/8250 directory.
>
> Few comments below.
>
> First of all what is the differences with the original 8250? It worth to
> list them here in the changelog.
>
I noted this in the git-log
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/serial_8250.h>
>> +#include <linux/serial_core.h>
>> +#include <linux/serial_reg.h>
>
> Shouldn't be enough to have only serial_8250.h here?
>
> + empty line between #include <> and "".
I did so in v4.
>> +static int uniphier_uart_probe(struct platform_device *pdev)
>> +{
>> + struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> Please use platform_get_irq().
OK. Fixed.
>> + struct device *dev = &pdev->dev;
>> + struct uniphier8250_priv *priv;
>> + struct uart_8250_port up;
>> + int ret;
>> + void __iomem *membase;
>> +
>> + if (!regs || !irq) {
>> + dev_err(dev, "missing registers or irq\n");
>> + return -EINVAL;
>> + }
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + membase = devm_ioremap(dev, regs->start, resource_size(regs));
>
> Shouldn't be devm_ioremap_resource() ?
I tried this, but it broke my driver.
It looks like the cause of error is in the devm_request_mem_region()
called from that function,
8250_core.c also calls request_mem_region().
I think it fails if the memory region is already reserved
before calling serial8250_register_8250_port().
So, I am sticking to devm_ioremap().
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-05-23 12:55 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-19 1:11 [PATCH v3] serial: 8250_uniphier: add UniPhier serial driver Masahiro Yamada
2015-05-19 8:22 ` Matthias Brugger
2015-05-19 8:54 ` Masahiro Yamada
2015-05-19 8:57 ` Jiri Slaby
2015-05-19 9:10 ` Masahiro Yamada
2015-05-19 9:48 ` One Thousand Gnomes
2015-05-19 11:18 ` Masahiro Yamada
2015-05-19 11:31 ` Matthias Brugger
2015-05-19 10:13 ` Matthias Brugger
2015-05-20 12:56 ` Andy Shevchenko
2015-05-23 12:55 ` Masahiro Yamada
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).