Re: [PATCH 3/3] ARM: Add CLPS711X architecture

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

On 19:14 Thu 04 Oct     , Alexander Shiyan wrote:
> On Thu, 4 Oct 2012 14:52:29 +0200
> Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> 
> > On 16:37 Thu 04 Oct     , Alexander Shiyan wrote:
> > > This patch adds new architecture (CLPS711X) into barebox.
> > > The core-logic functionality of the device is built around an ARM720T
> > > processor running at clock speeds up to 90 MHz and 74 MHz.
> > > Patch also adds a generic board support (CLEP7212, Linux ARM ID=91) and
> > > serial driver for this CPU.
> ...
> > > +CONFIG_BAUDRATE=57600
> > why?? usally 115200
> It is impossible rate when CPU is clocked from an external source.
> Possible rates is very limited for this platform  and historically
> (for this target) default baudrate is 57600. In any case it can be changed
> via config option.
so put a commet about it
> 
> ...
> > > +static int clocks_init(void)
> > please split clocksource and clk
> I do not know how best to do it because I need access to calculated values of
> the frequency in initialization clkdev & clocksource.
see my patch series I've send about clocksource

the clocksource driver  request it's clock and get it's rate
the clock init is done speratly
> 
> > > +void _setup_memcfg(int bank, u32 addr, u32 val)
> > > +{
> > > +	u32 tmp = readl(addr);
> > > +
> > > +	switch (bank) {
> > > +	case 0:
> > > +		tmp &= ~(0xff << 0);
> > > +		tmp |= val << 0;
> > > +		break;
> > > +	case 1:
> > > +		tmp &= ~(0xff << 8);
> > > +		tmp |= val << 8;
> > > +		break;
> > > +	case 2:
> > > +		tmp &= ~(0xff << 16);
> > > +		tmp |= val << 16;
> > > +		break;
> > > +	case 3:
> > > +		tmp &= ~(0xff << 24);
> > > +		tmp |= val << 24;
> > > +		break;
> > no default?
> "Default" is not needed, because call is used internally in this unit and it
> protected by "switch/case" in the caller.
> 
> > > +void __noreturn reset_cpu(unsigned long addr)
> > > +{
> > > +	for (;;)
> > no cpu reset?
> This CPU don't have reset register.
al lot of CPU does not ahve one they have instead a watchdog that can reset
the SoC I never see a SoC that can not been reset by soft

or can not been reset by a fault

Best Regards,
J.
> 
> -- 
> Alexander Shiyan <shc_work@mail.ru>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox