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 <[email protected]> >> --- >> 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 >> <[email protected]> > > 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. _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
