[U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU

[Marek Vasut]

On Friday, September 09, 2011 10:23:09 AM Stefano Babic wrote:
> On 09/08/2011 10:42 PM, Marek Vasut wrote:
> > This patch supports:
> > - Timers
> > - Debug UART
> > - Clock
> 
> Hi Marek,
> 
> a general remark. It seems to me that your patchset comes directly from
> your development, as it looks like what I normally have when I develop a
> new board ;-)
> 
> However, to put into mainline and to make review easier, because this is
> a porting to a new SOC, I am expecting that all patches related to a new
> file are squashed together. It makes no sense to have several patches
> regarding, for example, m28evk.h, because this is a new file.

Done, I'll retest and send V2 of the series

> 
> Another general remark here: we tend to have the same structure and the
> same files for all IMX SOC. This means that all IMX SOC have a
> imx-regs.h that contain the required register definitions (really a
> subset what we have in kernel). Is it really necessary to split the
> definitions in several small files ?

Just like Heiko said ... it'd produce terribly big and messy file. I'd prefer to 
avoid that.

> 
> > Signed-off-by: Marek Vasut <marek.vasut at gmail.com>
> > Cc: Stefano Babic <sbabic at denx.de>
> > Cc: Wolfgang Denk <wd at denx.de>
> > Cc: Detlev Zundel <dzu at denx.de>
> > ---
> > 
> >  arch/arm/cpu/arm926ejs/mx28/Makefile          |   46 +++
> >  arch/arm/cpu/arm926ejs/mx28/clock.c           |  359
> >  ++++++++++++++++++++++ arch/arm/cpu/arm926ejs/mx28/mx28.c            | 
> >  131 ++++++++
> >  arch/arm/cpu/arm926ejs/mx28/timer.c           |  143 +++++++++
> >  arch/arm/include/asm/arch-mx28/clock.h        |   48 +++
> >  arch/arm/include/asm/arch-mx28/imx-regs.h     |   33 ++
> >  arch/arm/include/asm/arch-mx28/mx28.h         |   30 ++
> >  arch/arm/include/asm/arch-mx28/regs-base.h    |   88 ++++++
> >  arch/arm/include/asm/arch-mx28/regs-clkctrl.h |  308 +++++++++++++++++++
> >  arch/arm/include/asm/arch-mx28/regs-common.h  |   66 ++++
> >  arch/arm/include/asm/arch-mx28/regs-power.h   |  409
> >  +++++++++++++++++++++++++ arch/arm/include/asm/arch-mx28/regs-ssp.h    
> >  |  345 +++++++++++++++++++++
> >  arch/arm/include/asm/arch-mx28/regs-timrot.h  |  167 ++++++++++
> >  arch/arm/include/asm/arch-mx28/regs-uartdbg.h |  182 +++++++++++ 14
> >  files changed, 2355 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/cpu/arm926ejs/mx28/Makefile
> >  create mode 100644 arch/arm/cpu/arm926ejs/mx28/clock.c
> >  create mode 100644 arch/arm/cpu/arm926ejs/mx28/mx28.c
> >  create mode 100644 arch/arm/cpu/arm926ejs/mx28/timer.c
> >  create mode 100644 arch/arm/include/asm/arch-mx28/clock.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/imx-regs.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/mx28.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/regs-base.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/regs-clkctrl.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/regs-common.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/regs-power.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/regs-ssp.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/regs-timrot.h
> >  create mode 100644 arch/arm/include/asm/arch-mx28/regs-uartdbg.h
> 
> regs-uartdbg.h is used only by the driver, right ? The driver should be
> in the driver directory, and also its header. See drivers/serial/

Honestly, I need to check if the file is used at all. We replaced the original 
driver with pl011 driver.

> 
> > +
> > +#include <common.h>
> > +#include <asm/errno.h>
> > +#include <asm/io.h>
> > +#include <asm/arch/clock.h>
> > +#include <asm/arch/regs-common.h>
> > +#include <asm/arch/regs-base.h>
> > +#include <asm/arch/regs-clkctrl.h>
> > +#include <asm/arch/regs-ssp.h>
> 
> Again, regarding file splitting for register. If you prefer to have
> several files, I think it is better that imx-regs.h include them. Then
> we have still a common header imx-regs.h that contains all needed
> register definitions. This is a better interface for a board maintainer.

Won't it make the compiler slower at compile time if it has to go through all of 
the included files instead of a subset ?

> 
> > +
> > +/* The PLL frequency is always 480MHz, see section 10.2 in iMX28
> > datasheet. */ +#define	PLL_FREQ_KHZ	480000
> > +#define	PLL_FREQ_COEF	18
> > +/* The XTAL frequency is always 24MHz, see section 10.2 in iMX28
> > datasheet. */ +#define	XTAL_FREQ_KHZ	24000
> > +
> > +#define	PLL_FREQ_MHZ	(PLL_FREQ_KHZ / 1000)
> > +#define	XTAL_FREQ_MHZ	(XTAL_FREQ_KHZ / 1000)
> > +
> > +uint32_t mx28_get_pclk(void)
> 
> This function must be static.
> 

[...]
Fixed all the stuff

> > +/* Lowlevel init isn't used on i.MX28, so just have a dummy here */
> > +inline void lowlevel_init(void) {}
> 
> Ok.
> 
> > +
> > +void reset_cpu(ulong ignored) __attribute__((noreturn));
> > +
> > +void reset_cpu(ulong ignored)
> > +{
> > +	struct mx28_clkctrl_regs *clkctrl_regs =
> > +		(struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
> > +	struct mx28_power_regs *power_regs =
> > +		(struct mx28_power_regs *)MXS_POWER_BASE;
> > +
> > +	writel(0, &power_regs->hw_power_charge);
> > +	writel(0, &power_regs->hw_power_minpwr);
> > +	writel(CLKCTRL_RESET_DIG, &clkctrl_regs->hw_clkctrl_reset);
> > +
> > +	/* Endless loop, reset will exit from here */
> > +	for (;;)
> > +		;
> 
> To understand: does a reset mean on the i.MX28 a power off ? Do you turn
> off the power ? If this is the case, some features are not possible (as
> PRAM, for example) on this SOC.

This should just reset the chip. No poweroff.

[...]

> 
> > +ulong get_timer(ulong base)
> > +{
> > +	struct mx28_timrot_regs *timrot_regs =
> > +		(struct mx28_timrot_regs *)MXS_TIMROT_BASE;
> > +
> > +	/* Current tick value */
> > +	uint32_t now = readl(&timrot_regs->hw_timrot_running_count0);
> > +
> > +	if (lastdec >= now) {
> > +		/*
> > +		 * normal mode (non roll)
> > +		 * move stamp forward with absolut diff ticks
> > +		 */
> > +		timestamp += (lastdec - now);
> > +	} else {
> > +		/* we have rollover of decrementer */
> > +		timestamp += (TIMER_LOAD_VAL - now) + lastdec;
> > +
> > +	}
> > +	lastdec = now;
> > +
> > +	return tick_to_time(timestamp) - base;
> > +}
> 
> Not sure, but it seems to me this implementation does not follow the
> last changes to this kind of interface. We should implement
> get_timer_masked(), but then get_timer is simply:
> 
> ulong get_timer(ulong base)
> {
>         return get_timer_masked() - base;
> }
> 
> as I see in most SOC. However, Heiko (in CC) knows more deeply this
> stuff as me, and  he can better explain us if this implementation is
> correct.
> 
> I know that other i.MX are look like your implementation, but they are
> quite old...

I'll probably wait for someone to clearly say how this should be to avoid 
reworking it for the fourth time.

[...]

> Best regards,
> Stefano Babic

Cheers