[U-Boot] [PATCH 1/1] arm: add uart dcc support

[Dirk Behme <dirk.behme@googlemail.com>]

Dear Jean-Christophe,

Jean-Christophe PLAGNIOL-VILLARD wrote:

Do you like to add some words going to git describing this patch here? 
E.g. what DCC is and what it might be used for?

> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  common/devices.c         |    3 +
>  drivers/serial/Makefile  |    1 +
>  drivers/serial/arm_dcc.c |  270 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/devices.h        |    3 +
>  lib_arm/board.c          |    3 +
>  5 files changed, 280 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/serial/arm_dcc.c
> 
> diff --git a/common/devices.c b/common/devices.c
> index 38f1bbc..ba53c9b 100644
> --- a/common/devices.c
> +++ b/common/devices.c
> @@ -215,6 +215,9 @@ int devices_init (void)
>  	/* Initialize the list */
>  	INIT_LIST_HEAD(&(devs.list));
>  
> +#ifdef CONFIG_ARM_DCC_MULTI
> +	drv_arm_dcc_init ();
> +#endif
>  #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
>  	i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
>  #endif
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index b6fd0d7..af121a2 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -25,6 +25,7 @@ include $(TOPDIR)/config.mk
>  
>  LIB	:= $(obj)libserial.a
>  
> +COBJS-$(CONFIG_ARM_DCC) += arm_dcc.o

Above you use CONFIG_ARM_DCC_MULTI, here you use CONFIG_ARM_DCC.

Why do you introduce two (new?) macros? Wouldn't one be sufficient?

For which board do want to enable this? There is no enable of these 
two macros in this patch?

And you will know better than I, but do new basic config options like 
these need some documentation somewhere?

>  COBJS-$(CONFIG_ATMEL_USART) += atmel_usart.o
>  COBJS-$(CONFIG_MCFUART) += mcfuart.o
>  COBJS-$(CONFIG_NS9750_UART) += ns9750_serial.o
> diff --git a/drivers/serial/arm_dcc.c b/drivers/serial/arm_dcc.c
> new file mode 100644
> index 0000000..8be4d4c
> --- /dev/null
> +++ b/drivers/serial/arm_dcc.c
> @@ -0,0 +1,270 @@
> +/*
> + * Copyright (C) 2004-2007 ARM Limited.
> + * Copyright (C) 2008 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

2009?

> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + * As a special exception, if other files instantiate templates or use macros
> + * or inline functions from this file, or you compile this file and link it
> + * with other works to produce a work based on this file, this file does not
> + * by itself cause the resulting work to be covered by the GNU General Public
> + * License. However the source code for this file must still be made available
> + * in accordance with section (3) of the GNU General Public License.
> +
> + * This exception does not invalidate any other reasons why a work based on
> + * this file might be covered by the GNU General Public License.
> + */
> +
> +#include <common.h>
> +#include <devices.h>
> +
> +#define DCC_ARM9_RBIT	(1 << 0)
> +#define DCC_ARM9_WBIT	(1 << 1)
> +#define DCC_ARM10_RBIT	(1 << 7)
> +#define DCC_ARM10_WBIT	(1 << 6)
> +#define DCC_ARM11_RBIT	(1 << 30)
> +#define DCC_ARM11_WBIT	(1 << 29)

Should something like this go to a header file?

> +#define read_core_id(x)	do {						\
> +		__asm__ ("mrc p15, 0, %0, c0, c0, 0\n" : "=r" (x));	\
> +		x = (x >> 4) & 0xFFF;					\

What's about macros instead of hard coded values?

> +		} while (0);
> +
> +/*
> + * ARM9
> + */
> +#define write_arm9_dcc(x)	\
> +		__asm__ volatile ("mcr p14, 0, %0, c1, c0, 0\n" : : "r" (x))
> +
> +#define read_arm9_dcc(x)	\
> +		__asm__ volatile ("mrc p14, 0, %0, c1, c0, 0\n" : "=r" (x))
> +
> +#define status_arm9_dcc(x)	\
> +		__asm__ volatile ("mrc p14, 0, %0, c0, c0, 0\n" : "=r" (x))
> +
> +#define can_read_arm9_dcc(x)	do {	\
> +		status_arm9_dcc(x);	\
> +		x &= DCC_ARM9_RBIT;	\
> +		} while (0);
> +
> +#define can_write_arm9_dcc(x)	do {	\
> +		status_arm9_dcc(x);	\
> +		x &= DCC_ARM9_WBIT;	\
> +		x = (x == 0);		\
> +		} while (0);
> +
> +/*
> + * ARM10
> + */
> +#define write_arm10_dcc(x)	\
> +		__asm__ volatile ("mcr p14, 0, %0, c0, c5, 0\n" : : "r" (x))
> +
> +#define read_arm10_dcc(x)	\
> +		__asm__ volatile ("mrc p14, 0, %0, c0, c5, 0\n" : "=r" (x))
> +
> +#define status_arm10_dcc(x)	\
> +		__asm__ volatile ("mrc p14, 0, %0, c0, c1, 0\n" : "=r" (x))
> +
> +#define can_read_arm10_dcc(x)	do {	\
> +		status_arm10_dcc(x);	\
> +		x &= DCC_ARM10_RBIT;	\
> +		} while (0);
> +
> +#define can_write_arm10_dcc(x)	do {	\
> +		status_arm10_dcc(x);	\
> +		x &= DCC_ARM10_WBIT;	\
> +		x = (x == 0);		\
> +		} while (0);
> +
> +/*
> + * ARM11
> + */
> +#define write_arm11_dcc(x)	\
> +		__asm__ volatile ("mcr p14, 0, %0, c0, c5, 0\n" : : "r" (x))
> +
> +#define read_arm11_dcc(x)	\
> +		__asm__ volatile ("mrc p14, 0, %0, c0, c5, 0\n" : "=r" (x))
> +
> +#define status_arm11_dcc(x)	\
> +		__asm__ volatile ("mrc p14, 0, %0, c0, c1, 0\n" : "=r" (x))
> +
> +#define can_read_arm11_dcc(x)	do {	\
> +		status_arm11_dcc(x);	\
> +		x &= DCC_ARM11_RBIT;	\
> +		} while (0);
> +
> +#define can_write_arm11_dcc(x)	do {	\
> +		status_arm11_dcc(x);	\
> +		x &= DCC_ARM11_WBIT;	\
> +		x = (x == 0);		\
> +		} while (0);
> +
> +#define TIMEOUT_COUNT 0x4000000
> +
> +static enum {arm9_and_earlier, arm10, arm11_and_later} arm_type = arm9_and_earlier;
> +
> +#ifndef CONFIG_ARM_DCC_MULTI
> +#define arm_dcc_init serial_init
> +void serial_setbrg(void) {}
> +#define arm_dcc_getc serial_getc
> +#define arm_dcc_putc serial_putc
> +#define arm_dcc_puts serial_puts
> +#define arm_dcc_tstc serial_tstc
> +#endif

Why are all these #defines above in a .c file and not in a header file?

> +int arm_dcc_init(void)
> +{
> +	register unsigned int id;
> +
> +	read_core_id(id);
> +
> +	if ((id & 0xF00) == 0xA00)

What's about macros instead of hard coded values?

> +		arm_type = arm10;
> +	else if (id >= 0xb00)

ditto

> +		arm_type = arm11_and_later;
> +	else
> +		arm_type = arm9_and_earlier;
> +
> +	return 0;

Can this function return something else than 0 ? If not, why then have 
a return value?

> +	if (rc == 0) {
> +		arm_dcc_init();

Here you don't check the return value of arm_dcc_init(), so it makes 
no sense to have arm_dcc_init() returning anything?

> +		return 1;
> +	}
...

I'm not sure if this does matter, and if I have a recent version of 
Linux checkpatch script, but my version of checkpatch reports for this 
patch:

-- cut --
WARNING: space prohibited between function name and open parenthesis '('
#29: FILE: common/devices.c:219:
+       drv_arm_dcc_init ();

WARNING: line over 80 characters
#165: FILE: drivers/serial/arm_dcc.c:115:
+static enum {arm9_and_earlier, arm10, arm11_and_later} arm_type = 
arm9_and_earlier;

WARNING: space prohibited between function name and open parenthesis '('
#301: FILE: drivers/serial/arm_dcc.c:251:
+       memset (&arm_dcc_dev, 0, sizeof (arm_dcc_dev));

WARNING: space prohibited between function name and open parenthesis '('
#301: FILE: drivers/serial/arm_dcc.c:251:
+       memset (&arm_dcc_dev, 0, sizeof (arm_dcc_dev));

WARNING: space prohibited between function name and open parenthesis '('
#303: FILE: drivers/serial/arm_dcc.c:253:
+       strcpy (arm_dcc_dev.name, "dcc");

WARNING: space prohibited between function name and open parenthesis '('
#311: FILE: drivers/serial/arm_dcc.c:261:
+       rc = device_register (&arm_dcc_dev);
-- cut --

Best regards

Dirk