From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vladimir Zapolskiy Date: Fri, 10 Feb 2012 22:14:47 +0200 Subject: [U-Boot] [PATCH 1/3 v8] arm926ejs: add NXP LPC32x0 cpu series support In-Reply-To: <4F355B51.8010806@aribaud.net> References: <1326415341-7351-1-git-send-email-vz@mleia.com> <1326415341-7351-2-git-send-email-vz@mleia.com> <4F355B51.8010806@aribaud.net> Message-ID: <4F357AB7.9060108@mleia.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Albert, On 10.02.2012 20:00, Albert ARIBAUD wrote: > Hi Vladimir, > > Le 13/01/2012 01:42, Vladimir Zapolskiy a ?crit : > >> diff --git a/arch/arm/cpu/arm926ejs/lpc32xx/cpu.c >> b/arch/arm/cpu/arm926ejs/lpc32xx/cpu.c >> new file mode 100644 >> index 0000000..e29e130 >> --- /dev/null >> +++ b/arch/arm/cpu/arm926ejs/lpc32xx/cpu.c >> @@ -0,0 +1,70 @@ >> +/* >> + * Copyright (C) 2011 by Vladimir Zapolskiy >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * as published by the Free Software Foundation; either version 2 >> + * of the License, or (at your option) any later version. >> + * >> + * 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., 51 Franklin Street, Fifth Floor, Boston, >> + * MA 02110-1301, USA. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +static struct clk_pm_regs *clk = (struct clk_pm_regs *)CLK_PM_BASE; >> +static struct wdt_regs *wdt = (struct wdt_regs *)WDT_BASE; >> + >> +void reset_cpu(ulong addr) >> +{ >> + /* Enable watchdog clock */ >> + setbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG); >> + >> + /* Reset pulse length is 13005 peripheral clock frames */ >> + writel(13000,&wdt->pulse); > > I can't help but ask, if the comment says 13005, then why does the code > say 13000? > the provided comment is done according to the descriptions of WDTIM_PULSE register from User Manual, for register value X it gives the RESET pulse length of (X + 5) PERIPH_CLKs. >> + /* Force WDOG_RESET2 and RESOUT_N signal active */ >> + writel(WDTIM_MCTRL_RESFRC2 | WDTIM_MCTRL_RESFRC1 | WDTIM_MCTRL_M_RES2, >> + &wdt->mctrl); >> + >> + while (1) >> + /* NOP */; >> +} >> + >> +#if defined(CONFIG_ARCH_CPU_INIT) >> +int arch_cpu_init(void) >> +{ >> + /* >> + * It might be necessary to flush data cache, if U-boot is loaded >> + * from kickstart bootloader, e.g. from S1L loader >> + */ >> + flush_dcache_all(); >> + >> + return 0; >> +} >> +#else >> +#error "You have to select CONFIG_ARCH_CPU_INIT" >> +#endif > > No other code ever has this kind of #error on CONFIG_ARCH_CPU_INIT not > being defined. What's the point of it? People won't suddenly decide to > undefine CONFIG_ARCH_CPU_INIT for fun, I think. > Generally for all future boards it's a good idea to clean up caches after a bootstrap/kickstart bootloader's work, which is expected to be present mostly on all LPC32XX boards. The macro CONFIG_ARCH_CPU_INIT is defined by default in header include/asm/arch0lpc32xx/config.h, which is included in board configuration files for shared definitions, but of course technically for some of future boards it can be undefined after inclusion of the header. Originally I didn't have such an #else case, however Marek convinced me to add the clause as a clear enough reminder about its importance for board developers. If you think this #else case is redundant, I won't argue and it could be just removed. >> diff --git a/arch/arm/include/asm/arch-lpc32xx/sys_proto.h >> b/arch/arm/include/asm/arch-lpc32xx/sys_proto.h >> new file mode 100644 >> index 0000000..b2cbfcd >> --- /dev/null >> +++ b/arch/arm/include/asm/arch-lpc32xx/sys_proto.h >> @@ -0,0 +1,25 @@ >> +/* >> + * Copyright (C) 2011 Vladimir Zapolskiy >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * as published by the Free Software Foundation; either version 2 >> + * of the License, or (at your option) any later version. >> + * >> + * 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., 51 Franklin Street, Fifth Floor, Boston, >> + * MA 02110-1301, USA. >> + */ >> + >> +#ifndef _LPC32XX_SYS_PROTO_H >> +#define _LPC32XX_SYS_PROTO_H >> + >> +void lpc32xx_uart_init(unsigned int uart_id); >> + >> +#endif /* _LPC32XX_SYS_PROTO_H */ > > Do we need this .h for a single function when we already have uart.h? > I think we need it, I found that's quite a common practice to have such a header with SoC specific declarations for boards, and these headers have the same name. I expect that later on this header will contain SoC EMAC specific declarations and may be something else depending on U-boot support of other LPC32XX peripherals. Thank you for review. -- With best wishes, Vladimir