[U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU
Stefano Babic
sbabic at denx.de
Fri Sep 9 10:23:09 CEST 2011
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.
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 ?
>
> 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/
> +
> +#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.
> +
> +/* 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.
> +
> +uint32_t mx28_get_hclk(void)
Ditto. All *_get_some_clk are exported with mxc_get_clk(index_number)
> +
> +uint32_t mx28_get_emiclk(void)
Ditto
> +static inline void enable_gpmi_clk(void)
> +{
Is there a reason to have inline ? And this is called only once. Do we
have a enable_ without a disable_ function ? Your mx28_get_gpmiclk()
enables without the caller knows the gpmi clock. Strange for a pure
"get" function.
> +
> +uint32_t mx28_get_gpmiclk(void)
Also static.
> +/*
> + * Set IO clock frequency, in kHz
> + */
> +void mx28_set_ioclk(unsigned int io, uint32_t freq)
> +{
Please use an enum for io - it cannot assume any integer value,
something with GATE0 and GATE1 to be the call easier to understand.
> +
> +/*
> + * Get IO clock, returns IO clock in kHz
> + */
> +uint32_t mx28_get_ioclk(unsigned int io)
Static function. Do you think to export this function ? In this case,
you need to add enums for mxc_get_clk().
> +
> +/*
> + * Configure SSP clock frequency, in kHz
> + */
> +void mx28_set_sspclk(unsigned int ssp, uint32_t freq, int xtal)
> +{
> + struct mx28_clkctrl_regs *clkctrl_regs =
> + (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
> + uint32_t clk, clkreg;
> +
> + if (ssp > 3)
> + return;
Maybe a define in imx-regs.h setting the max number of serial ports ?
> +
> + clkreg = (uint32_t)(&clkctrl_regs->hw_clkctrl_ssp0) + (ssp * 0x10);
You have a pointer to a mx28_reg structure. Replace fixed offset with a
sizeof.
> +uint32_t mx28_get_sspclk(unsigned int ssp)
> +{
Static as other get functions.
> +uint32_t mxc_get_clock(enum mxc_clock clk)
> +{
> + switch (clk) {
> + case MXC_ARM_CLK:
> + return mx28_get_pclk() * 1000000;
> + case MXC_GPMI_CLK:
> + return mx28_get_gpmiclk() * 1000000;
> + case MXC_AHB_CLK:
> + case MXC_IPG_CLK:
> + return mx28_get_hclk() * 1000000;
Probably you need to add some new enums if you want to export all your
get functions, such as MXC_GET_UARTCLK for your mx28_get_sspclk (or new
enums if you think there new features).
> +
> +#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-power.h>
> +#include <asm/arch/mx28.h>
Same comment for register definitions splitting as before.
> +/* 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.
> +int mx28_wait_mask_set(struct mx28_register *reg, uint32_t mask, int timeout)
> +{
> + while (--timeout) {
> + if ((readl(®->reg) & mask) == mask)
> + break;
> + udelay(1);
> + }
> +
> + return !timeout;
> +}
> +
> +int mx28_wait_mask_clr(struct mx28_register *reg, uint32_t mask, int timeout)
> +{
> + while (--timeout) {
> + if ((readl(®->reg) & mask) == 0)
> + break;
> + udelay(1);
> + }
> +
> + return !timeout;
> +}
> +
> +int mx28_reset_block(struct mx28_register *reg)
> +{
> + /* Clear SFTRST */
> + writel(MX28_BLOCK_SFTRST, ®->reg_clr);
> +
> + if (mx28_wait_mask_clr(reg, MX28_BLOCK_SFTRST, RESET_MAX_TIMEOUT))
> + return 1;
> +
> + /* Clear CLKGATE */
> + writel(MX28_BLOCK_CLKGATE, ®->reg_clr);
> +
> + /* Set SFTRST */
> + writel(MX28_BLOCK_SFTRST, ®->reg_set);
> +
> + /* Wait for CLKGATE being set */
> + if (mx28_wait_mask_set(reg, MX28_BLOCK_CLKGATE, RESET_MAX_TIMEOUT))
> + return 1;
> +
> + /* Clear SFTRST */
> + writel(MX28_BLOCK_SFTRST, ®->reg_clr);
> +
> + if (mx28_wait_mask_clr(reg, MX28_BLOCK_SFTRST, RESET_MAX_TIMEOUT))
> + return 1;
> +
> + /* Clear CLKGATE */
> + writel(MX28_BLOCK_CLKGATE, ®->reg_clr);
> +
> + if (mx28_wait_mask_clr(reg, MX28_BLOCK_CLKGATE, RESET_MAX_TIMEOUT))
> + return 1;
> +
> + return 0;
> +}
> +
> +#if defined(CONFIG_DISPLAY_CPUINFO)
> +int print_cpuinfo(void)
> +{
> + printf("Freescale i.MX28 family\n");
> + printf("CPU: %3d MHz\n", mx28_get_pclk());
> + printf("BUS: %3d MHz\n", mx28_get_hclk());
> + printf("EMI: %3d MHz\n", mx28_get_emiclk());
> + printf("GPMI: %3d MHz\n", mx28_get_gpmiclk());
> + return 0;
> +}
The function has the name "cpuinfo", but you print nothing about the
cpu. Instead of that, the clock's value are printed.
After some discussion we have this common statement for the i.MX SOCs:
- the print_cpuinfo gives the revision number and the reset cause
See all other i.MX implementations
- clocks are printed with a command, whose name *must* be (because we
use for PowerPC also) simply "clocks". This avoid too much printing at
the startup. See for example do_mx35_showclocks(), or the same for mx5.
Please follow this rule.
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/arch/regs-common.h>
> +#include <asm/arch/regs-base.h>
> +#include <asm/arch/regs-timrot.h>
> +#include <asm/arch/mx28.h>
Remark to check the register definitions. My idea is to have a simple
easy to understand interface to get the register definitions for a SOC,
and I would replace this stuff only with imx-regs.h. What do you think ?
> +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...
> +
> +/* We use the HW_DIGCTL_MICROSECONDS register for sub-millisecond timer. */
> +#define MX28_HW_DIGCTL_MICROSECONDS 0x8001c0c0
> +
> +void __udelay(unsigned long usec)
> +{
> + uint32_t old, new, incr;
> + uint32_t counter = 0;
> +
> + old = readl(MX28_HW_DIGCTL_MICROSECONDS);
> +
> + while (counter < usec) {
> + new = readl(MX28_HW_DIGCTL_MICROSECONDS);
> +
> + /* Check if the timer wrapped. */
> + if (new < old) {
> + incr = 0xffffffff - old;
> + incr += new;
> + } else {
> + incr = new - old;
> + }
The same for __udelay. I think there is a common agreement to it, and I
am expecting that the code to manage overflow is similar.
Should we export also a get_ticks() ?
> diff --git a/arch/arm/include/asm/arch-mx28/clock.h b/arch/arm/include/asm/arch-mx28/clock.h
> new file mode 100644
> index 0000000..5e13a51
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx28/clock.h
> @@ -0,0 +1,48 @@
> +/*
> + * Freescale i.MX28 Clock
> + *
> + * Copyright (C) 2011 Marek Vasut <marek.vasut at gmail.com>
> + * on behalf of DENX Software Engineering GmbH
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#ifndef __CLOCK_H__
> +#define __CLOCK_H__
> +
> +enum mxc_clock {
> + MXC_ARM_CLK = 0,
> + MXC_AHB_CLK,
> + MXC_IPG_CLK,
> + MXC_GPMI_CLK,
> +};
> +
> +uint32_t mxc_get_clock(enum mxc_clock clk);
> +uint32_t mx28_get_pclk(void);
> +uint32_t mx28_get_hclk(void);
> +uint32_t mx28_get_emiclk(void);
> +uint32_t mx28_get_gpmiclk(void);
wrong. You should export only mxc_get_clock.
> +
> +void mx28_set_ioclk(unsigned int io, uint32_t freq);
> +uint32_t mx28_get_ioclk(unsigned int io);
Ditto.
> diff --git a/arch/arm/include/asm/arch-mx28/imx-regs.h b/arch/arm/include/asm/arch-mx28/imx-regs.h
> new file mode 100644
> index 0000000..a082f1d
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx28/imx-regs.h
> @@ -0,0 +1,33 @@
> +/*
> + * Freescale i.MX28 Registers
> + *
> + * Copyright (C) 2011 Marek Vasut <marek.vasut at gmail.com>
> + * on behalf of DENX Software Engineering GmbH
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#ifndef __IMX_REGS_H__
> +#define __IMX_REGS_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>
> +#include <asm/arch/regs-timrot.h>
> +#include <asm/arch/regs-uartdbg.h>
> +
Ok, I see. Then I do not understand why you include explicitely each
single file in the code instead of imx-regs.h only.
> diff --git a/arch/arm/include/asm/arch-mx28/mx28.h b/arch/arm/include/asm/arch-mx28/mx28.h
> new file mode 100644
> index 0000000..a262c05
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx28/mx28.h
> @@ -0,0 +1,30 @@
> +/*
> + * Freescale i.MX28 MX28 specific functions
> + *
> + * Copyright (C) 2011 Marek Vasut <marek.vasut at gmail.com>
> + * on behalf of DENX Software Engineering GmbH
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#ifndef __MX28_H__
> +#define __MX28_H__
> +
> +int mx28_reset_block(struct mx28_register *reg);
> +int mx28_wait_mask_set(struct mx28_register *reg, uint32_t mask, int timeout);
> +int mx28_wait_mask_clr(struct mx28_register *reg, uint32_t mask, int timeout);
> +
> +#endif /* __MX28_H__ */
Should be these function available also outside the SOC directory ? If
the answer is yes, the common name (copied from omap) for the header
file to add such prototypes is sys_proto.h
> diff --git a/arch/arm/include/asm/arch-mx28/regs-clkctrl.h b/arch/arm/include/asm/arch-mx28/regs-clkctrl.h
> new file mode 100644
> index 0000000..66d492f
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx28/regs-clkctrl.h
> @@ -0,0 +1,308 @@
> +/*
> + * Freescale i.MX28 CLKCTRL Register Definitions
> + *
> + * Copyright (C) 2011 Marek Vasut <marek.vasut at gmail.com>
> + * on behalf of DENX Software Engineering GmbH
> + *
> + * Based on code from LTIB:
> + * Copyright 2008-2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#ifndef __REGS_CLKCTRL_H__
> +#define __REGS_CLKCTRL_H__
> +
> +struct mx28_clkctrl_regs {
I think you should protect with defined(__ASSEMBLY__). Register
definitions could be included by assembly file also - even if it is not
the case now with M28EVK, it could be in future.
> + mx28_reg(hw_clkctrl_pll0ctrl0) /* 0x00 */
This adds an implicit order to include your file. mx28_reg is not
defined here. regs-common.h must be included before this file. This is
pretty bad.
Best regards,
Stefano Babic
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================
More information about the U-Boot
mailing list