From mboxrd@z Thu Jan 1 00:00:00 1970 From: Albert ARIBAUD Date: Fri, 10 Feb 2012 21:19:55 +0100 Subject: [U-Boot] [PATCH 1/3 v8] arm926ejs: add NXP LPC32x0 cpu series support In-Reply-To: <4F357AB7.9060108@mleia.com> References: <1326415341-7351-1-git-send-email-vz@mleia.com> <1326415341-7351-2-git-send-email-vz@mleia.com> <4F355B51.8010806@aribaud.net> <4F357AB7.9060108@mleia.com> Message-ID: <4F357BEB.7060609@aribaud.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Vladimir, Le 10/02/2012 21:14, Vladimir Zapolskiy a ?crit : > 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. Fair enough. > Thank you for review. > > -- > With best wishes, > Vladimir Amicalement, -- Albert.