From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?UTF-8?B?QW5kcmVhcyBCaWXDn21hbm4=?= <andreas.devel@googlemail.com>
Date: Thu, 23 Oct 2014 22:04:15 +0200
Subject: [U-Boot] [PATCH 6/7] dm: at91: Add driver model support for the
 serial driver
In-Reply-To: <1412619262-23903-7-git-send-email-sjg@chromium.org>
References: <1412619262-23903-1-git-send-email-sjg@chromium.org>
	<1412619262-23903-7-git-send-email-sjg@chromium.org>
Message-ID: <54495F3F.7040403@googlemail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Simon,

On 06.10.14 20:14, Simon Glass wrote:
> Add driver model support while retaining the existing legacy code. This
> allows the driver to support boards that have converted to driver model
> as well as those that have not.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  arch/arm/include/asm/arch-at91/at91_serial.h | 15 +++++
>  drivers/serial/atmel_usart.c                 | 85 ++++++++++++++++++++++++++++
>  2 files changed, 100 insertions(+)
>  create mode 100644 arch/arm/include/asm/arch-at91/at91_serial.h
> 
> diff --git a/arch/arm/include/asm/arch-at91/at91_serial.h b/arch/arm/include/asm/arch-at91/at91_serial.h
> new file mode 100644
> index 0000000..9f78ed4
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-at91/at91_serial.h
> @@ -0,0 +1,15 @@
> +/*
> + * Copyright (c) 2014 Google, Inc
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#ifndef _AT91_SERIAL_H
> +#define _AT91_SERIAL_H
> +
> +/* Information about a serial port */
> +struct at91_serial_platdata {
> +	uint32_t base_addr;
> +};
> +
> +#endif
> diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
> index ce36e99..d3e2ade 100644
> --- a/drivers/serial/atmel_usart.c
> +++ b/drivers/serial/atmel_usart.c
> @@ -7,11 +7,17 @@
>   * SPDX-License-Identifier:	GPL-2.0+
>   */
>  #include <common.h>
> +#include <dm.h>
> +#include <errno.h>
>  #include <watchdog.h>
>  #include <serial.h>
>  #include <linux/compiler.h>
>  
>  #include <asm/io.h>
> +#ifdef CONFIG_DM_SERIAL
> +/* Should this be made common with avr32? */

yes, we should do so. Therefore I vote for atmel_serial.h.

> +#include <asm/arch/at91_serial.h>
> +#endif
>  #include <asm/arch/clk.h>
>  #include <asm/arch/hardware.h>
>  
> @@ -60,6 +66,7 @@ static void atmel_serial_activate(atmel_usart3_t *usart)
>  	__udelay(100);
>  }
>  
> +#ifndef CONFIG_DM_SERIAL
>  static void atmel_serial_setbrg(void)
>  {
>  	atmel_serial_setbrg_internal((atmel_usart3_t *)CONFIG_USART_BASE,
> @@ -123,3 +130,81 @@ __weak struct serial_device *default_serial_console(void)
>  {
>  	return &atmel_serial_drv;
>  }
> +#endif
> +
> +#ifdef CONFIG_DM_SERIAL
> +
> +struct at91_serial_priv {
> +	atmel_usart3_t *usart;
> +};
> +
> +int at91_serial_setbrg(struct udevice *dev, int baudrate)
> +{
> +	struct at91_serial_priv *priv = dev_get_priv(dev);
> +
> +	atmel_serial_setbrg_internal(priv->usart, 0 /* ignored */, baudrate);
> +	atmel_serial_activate(priv->usart);
> +
> +	return 0;
> +}
> +
> +static int at91_serial_getc(struct udevice *dev)
> +{
> +	struct at91_serial_priv *priv = dev_get_priv(dev);
> +
> +	if (!(readl(&priv->usart->csr) & USART3_BIT(RXRDY)))
> +		return -EAGAIN;
> +
> +	return readl(&priv->usart->rhr);
> +}
> +
> +static int at91_serial_putc(struct udevice *dev, const char ch)
> +{
> +	struct at91_serial_priv *priv = dev_get_priv(dev);
> +
> +	if (!(readl(&priv->usart->csr) & USART3_BIT(TXRDY)))
> +		return -EAGAIN;
> +
> +	writel(ch, &priv->usart->thr);
> +
> +	return 0;
> +}
> +
> +static int at91_serial_pending(struct udevice *dev, bool input)
> +{
> +	struct at91_serial_priv *priv = dev_get_priv(dev);
> +	uint32_t csr = readl(&priv->usart->csr);
> +
> +	if (input)
> +		return csr & USART3_BIT(RXRDY) ? 1 : 0;
> +	else
> +		return csr & USART3_BIT(TXEMPTY) ? 0 : 1;
> +}
> +
> +static const struct dm_serial_ops at91_serial_ops = {
> +	.putc = at91_serial_putc,
> +	.pending = at91_serial_pending,
> +	.getc = at91_serial_getc,
> +	.setbrg = at91_serial_setbrg,
> +};
> +
> +static int at91_serial_probe(struct udevice *dev)
> +{
> +	struct at91_serial_platdata *plat = dev->platdata;
> +	struct at91_serial_priv *priv = dev_get_priv(dev);
> +
> +	priv->usart = (atmel_usart3_t *)plat->base_addr;
> +	atmel_serial_init_internal(priv->usart);
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(serial_at91) = {

Please name it 'atmel' rather than 'at91' at91 is just one (legacy)
product family, there is also avr32 (ok, outdated) but new and growing
'sama5'.

> +	.name	= "serial_at91",
> +	.id	= UCLASS_SERIAL,
> +	.probe = at91_serial_probe,
> +	.ops	= &at91_serial_ops,
> +	.flags = DM_FLAG_PRE_RELOC,
> +	.priv_auto_alloc_size	= sizeof(struct at91_serial_priv),
> +};
> +#endif
> 

The patch looks good to me, but I wonder if changing the baudrate is
working as expected. Did you test this?

Best regards

Andreas Bie?mann