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 Zapolskiyvz@mleia.com
- 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<common.h> +#include<asm/arch/cpu.h> +#include<asm/arch/clk.h> +#include<asm/arch/wdt.h> +#include<asm/io.h>
+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 Zapolskiyvz@mleia.com
- 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,