Hi,
On Fri, Dec 06, 2013 at 01:09:31PM +0400, Sergei Ianovich wrote:
> pxa2xx-uart was a separate uart platform driver. It was declaring
> the same device names and numbers as 8250 driver. As a result,
> it was impossible to use 8250 driver on PXA SoCs.
>
> Upon closer examination pxa2xx-uart turned out to be a clone of
> 8250_core driver.
>
> Workaround for Erratum #19 according to Marvel(R) PXA270M Processor
> Specification Update (April 19, 2010) is dropped. 8250_core reads
> from FIFO immediately after checking DR bit in LSR.
>
> Signed-off-by: Sergei Ianovich <ynvich@xxxxxxxxx>
> CC: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> CC: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> CC: James Cameron <quozl@xxxxxxxxxx>
> ---
> changes v1..v2
> * actually implement workaround for E74 in dl_write as spooted
> by James Cameron
> * added comment about E19 in commit message
Nice job Sergei! I have a few minor nitpicks below, but if they are
the only comments, don't bother with v3. I'm fine with this. For what
it's worth:
Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> arch/arm/configs/am200epdkit_defconfig | 3 +-
> arch/arm/configs/cm_x2xx_defconfig | 3 +-
> arch/arm/configs/cm_x300_defconfig | 3 +-
> arch/arm/configs/colibri_pxa270_defconfig | 3 +-
> arch/arm/configs/colibri_pxa300_defconfig | 3 +-
> arch/arm/configs/corgi_defconfig | 4 +-
> arch/arm/configs/em_x270_defconfig | 3 +-
> arch/arm/configs/ezx_defconfig | 3 +-
> arch/arm/configs/h5000_defconfig | 3 +-
> arch/arm/configs/imote2_defconfig | 3 +-
> arch/arm/configs/lpd270_defconfig | 3 +-
> arch/arm/configs/lubbock_defconfig | 3 +-
> arch/arm/configs/mainstone_defconfig | 3 +-
> arch/arm/configs/mmp2_defconfig | 3 +-
> arch/arm/configs/pcm027_defconfig | 3 +-
> arch/arm/configs/pxa168_defconfig | 3 +-
> arch/arm/configs/pxa255-idp_defconfig | 3 +-
> arch/arm/configs/pxa3xx_defconfig | 3 +-
> arch/arm/configs/pxa910_defconfig | 3 +-
> arch/arm/configs/raumfeld_defconfig | 3 +-
> arch/arm/configs/spitz_defconfig | 4 +-
> arch/arm/configs/trizeps4_defconfig | 3 +-
> arch/arm/configs/viper_defconfig | 4 +-
> arch/arm/configs/xcep_defconfig | 3 +-
> drivers/tty/serial/8250/8250_pxa.c | 189 ++++++
> drivers/tty/serial/8250/Kconfig | 9 +
> drivers/tty/serial/8250/Makefile | 1 +
> drivers/tty/serial/Kconfig | 23 -
> drivers/tty/serial/Makefile | 1 -
> drivers/tty/serial/pxa.c | 971 ------------------------------
> 30 files changed, 247 insertions(+), 1022 deletions(-)
> create mode 100644 drivers/tty/serial/8250/8250_pxa.c
> delete mode 100644 drivers/tty/serial/pxa.c
<snip>
> diff --git a/drivers/tty/serial/8250/8250_pxa.c b/drivers/tty/serial/8250/8250_pxa.c
> new file mode 100644
> index 0000000..88ae73c
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pxa.c
> @@ -0,0 +1,189 @@
> +/*
> + * drivers/tty/serial/8250/8250_pxa.c -- driver for PXA on-board UARTS
> + * Copyright: (C) 2013 Sergei Ianovich <ynvich@xxxxxxxxx>
> + *
> + * replaces drivers/serial/pxa.c by Nicolas Pitre
> + * Created: Feb 20, 2003
> + * Copyright: (C) 2003 Monta Vista Software, Inc.
> + *
> + * Based on drivers/serial/8250.c by Russell King.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/serial_8250.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "8250.h"
> +
> +struct pxa8250_data {
> + int line;
> + struct clk *clk;
> +};
> +
> +#ifdef CONFIG_PM
> +static int serial_pxa_suspend(struct device *dev)
> +{
> + struct pxa8250_data *data = dev_get_drvdata(dev);
> +
> + serial8250_suspend_port(data->line);
> +
> + return 0;
> +}
> +
> +static int serial_pxa_resume(struct device *dev)
> +{
> + struct pxa8250_data *data = dev_get_drvdata(dev);
> +
> + serial8250_resume_port(data->line);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops serial_pxa_pm_ops = {
> + .suspend = serial_pxa_suspend,
> + .resume = serial_pxa_resume,
> +};
If you leave this structure outside the ifdef CONFIG_PM and use
SET_SYSTEM_SLEEP_PM_OPS() macro, you don't need the ifdef when you set
the driver pm ops below.
> +#endif
> +
> +static struct of_device_id serial_pxa_dt_ids[] = {
> + { .compatible = "mrvl,pxa-uart", },
> + { .compatible = "mrvl,mmp-uart", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, serial_pxa_dt_ids);
> +
> +/* Uart divisor latch write */
> +static void serial_pxa_dl_write(struct uart_8250_port *up, int value)
> +{
> + unsigned int dll;
> +
> + serial_out(up, UART_DLL, value & 0xff);
> + /*
> + * work around Erratum #74 according to Marvel(R) PXA270M Processor
> + * Specification Update (April 19, 2010)
> + */
> + dll = serial_in(up, UART_DLL);
> + WARN_ON(dll != (value & 0xff));
> +
> + serial_out(up, UART_DLM, value >> 8 & 0xff);
> +}
> +
> +
> +static void serial_pxa_pm(struct uart_port *port, unsigned int state,
> + unsigned int oldstate)
> +{
> + struct pxa8250_data *data = port->private_data;
> +
> + if (!state)
> + clk_prepare_enable(data->clk);
> + else
> + clk_disable_unprepare(data->clk);
> +}
> +
> +static int serial_pxa_probe(struct platform_device *pdev)
> +{
> + struct uart_8250_port uart = {};
> + struct pxa8250_data *data;
> + struct resource *mmres, *irqres;
> + int ret;
> +
> + mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + irqres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!mmres || !irqres)
> + return -ENODEV;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(data->clk)) {
> + ret = PTR_ERR(data->clk);
> + goto err_free;
You can just return here.
> + }
> +
> + ret = clk_prepare(data->clk);
> + if (ret)
> + goto err_free_clk;
ditto
> + uart.port.type = PORT_XSCALE;
> + uart.port.iotype = UPIO_MEM32;
> + uart.port.mapbase = mmres->start;
> + uart.port.regshift = 2;
> + uart.port.irq = irqres->start;
> + uart.port.fifosize = 64;
> + uart.port.flags = UPF_IOREMAP | UPF_SKIP_TEST;
> + uart.port.dev = &pdev->dev;
> + uart.port.uartclk = clk_get_rate(data->clk);
> + uart.port.pm = serial_pxa_pm;
> + uart.port.private_data = data;
> + uart.dl_write = serial_pxa_dl_write;
> +
> + ret = serial8250_register_8250_port(&uart);
> + if (ret < 0)
> + goto err_clk;
> +
> + data->line = ret;
> +
> + platform_set_drvdata(pdev, data);
> +
> + return 0;
> +
> + err_clk:
> + clk_unprepare(data->clk);
> + err_free_clk:
> + devm_clk_put(&pdev->dev, data->clk);
> + err_free:
> + devm_kfree(&pdev->dev, data);
And there labels could be dropped.
> + return ret;
> +}
> +
> +static int serial_pxa_remove(struct platform_device *pdev)
> +{
> + struct pxa8250_data *data = platform_get_drvdata(pdev);
> +
> + serial8250_unregister_port(data->line);
> +
> + clk_unprepare(data->clk);
> + devm_clk_put(&pdev->dev, data->clk);
> + devm_kfree(&pdev->dev, data);
You also don't need to call these resource freeing functions here.
> + return 0;
> +}
> +
> +static struct platform_driver serial_pxa_driver = {
> + .probe = serial_pxa_probe,
> + .remove = serial_pxa_remove,
> +
> + .driver = {
> + .name = "pxa2xx-uart",
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_PM
> + .pm = &serial_pxa_pm_ops,
> +#endif
> + .of_match_table = serial_pxa_dt_ids,
> + },
> +};
> +
> +module_platform_driver(serial_pxa_driver);
> +
> +MODULE_AUTHOR("Sergei Ianovich");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pxa2xx-uart");
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
