[v3] serial: 8250_uniphier: add UniPhier serial driver

Commit Message

Masahiro Yamada May 19, 2015, 1:11 a.m. UTC

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 <[e-mail]>
---

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

Comments

Matthias Brugger May 19, 2015, 8:22 a.m. UTC | #1

2015-05-19 3:11 GMT+02:00 Masahiro Yamada <[e-mail]>:
> 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 <[e-mail]>
> ---
>
> 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 <[e-mail]>
> + *
> + * 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 <[e-mail]>

Thanks,
Matthias

Masahiro Yamada May 19, 2015, 8:54 a.m. UTC | #2

Hi Matthias,

2015-05-19 17:22 GMT+09:00 Matthias Brugger <[e-mail]>:
> 2015-05-19 3:11 GMT+02:00 Masahiro Yamada <[e-mail]>:
>> 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 <[e-mail]>
>> ---
>>
>> 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 <[e-mail]>
>> + *
>> + * 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.

Jiri Slaby May 19, 2015, 8:57 a.m. UTC | #3

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?

Masahiro Yamada May 19, 2015, 9:10 a.m. UTC | #4

2015-05-19 17:57 GMT+09:00 Jiri Slaby <[e-mail]>:
> 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...

Matthias Brugger May 19, 2015, 10:13 a.m. UTC | #5

2015-05-19 10:54 GMT+02:00 Masahiro Yamada <[e-mail]>:
> Hi Matthias,
>
> 2015-05-19 17:22 GMT+09:00 Matthias Brugger <[e-mail]>:
>> 2015-05-19 3:11 GMT+02:00 Masahiro Yamada <[e-mail]>:
>>> 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 <[e-mail]>
>>> ---
>>>
>>> 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 <[e-mail]>
>>> + *
>>> + * 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 <[e-mail]>

Andy Shevchenko May 20, 2015, 12:56 p.m. UTC | #6

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 <[e-mail]>
> ---
> 
> 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 <[e-mail]>
> + *
> + * 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 <[e-mail]>");
> +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

Masahiro Yamada May 23, 2015, 12:55 p.m. UTC | #7

Hi Andy,

Thanks for reviewing my patch.


2015-05-20 21:56 GMT+09:00 Andy Shevchenko <[e-mail]>:
> 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().

Patch

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 <[e-mail]>
+ *
+ * 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 <[e-mail]>");
+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