[U-Boot] [PATCH 1/1] arm: add uart dcc support
Jean-Christophe PLAGNIOL-VILLARD
plagnioj at jcrosoft.com
Sat Feb 7 15:38:22 CET 2009
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 at 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 at 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.
More information about the U-Boot
mailing list