[U-Boot] [PATCH 1/3 v8] arm926ejs: add NXP LPC32x0 cpu series support

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

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<vz@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 Zapolskiy<vz@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,
-- 
Albert.