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

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

On 14:03 Sat 07 Feb     , Dirk Behme wrote:
> 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?
You maybe need to read arm documentation
>
>> 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.
maybe you need to re-read the code
it's an option of the driver
>
> Why do you introduce two (new?) macros? Wouldn't one be sufficient?
no you may also need to read u-boot implementation about multiple serial and
std serial

which both are implemented here
>
> For which board do want to enable this? There is no enable of these two 
> macros in this patch?
at91 which will come after or nearly any arm cpu
> --- /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?
no I've wrote it in 2008
>
>> + *
>> + * 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?
no way it's a mask
>
>> +		} 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?
maybe because it's dual api
>
>> +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?
why? it will not fail
>
>> +	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?
no it will never fail
>
>> +		return 1;
>> +	}
Best Regards,
J.