[U-Boot] [PATCH V2 2/6] i.mx: add the initial support for freescale i.MX6Q processor
Stefano Babic
sbabic at denx.de
Mon Nov 21 15:44:56 CET 2011
On 18/11/2011 08:11, Jason Liu wrote:
> i.MX6Q is freescale quad core processors with ARM cortex_a9 complex.
> This patch is to add the initial support for this processor.
>
> Signed-off-by: Jason Liu <jason.hui at linaro.org>
> Cc:Stefano Babic <sbabic at denx.de>
> ---
> v2:put aips init to c code as Marek suggest
> remove the parenthesis such as (2)
> remork get_usdhcx_clk() to get_usdhc_clk(port)
> remove the iomux base init and assign it staticly
> correct the chip rev print-message(rev1.0)
> ---
Hi Jason,
please understand I cannot go deep into the review due to not yet
published user manual - my comments are more related to how your code
can be integrated in current IMX code.
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/errno.h>
> +#include <asm/arch/imx-regs.h>
> +#include <asm/arch/ccm_regs.h>
> +#include <asm/arch/clock.h>
> +
> +enum pll_clocks {
> + PLL_SYS, /* System PLL */
> + PLL_BUS, /* System Bus PLL*/
> + PLL_USBOTG, /* OTG USB PLL */
> + PLL_ENET, /* ENET PLL */
> +};
> +
> +struct imx_ccm_reg *imx_ccm = (struct imx_ccm_reg *)CCM_BASE_ADDR;
Is it ok to initialize it here ? How does it work before and after
relocation ?
> +
> + return PLL3_80M / (uart_podf + 1);
> +}
> +
> +
One newline too much
> + if (cbcdr & MXC_CCM_CBCDR_AXI_SEL) {
> + if (cbcdr & MXC_CCM_CBCDR_AXI_ALT_SEL)
> + root_freq = PLL2_PFD2_FREQ;
> + else
> + root_freq = PLL3_PFD1_FREQ;;
^---one is enough
Drop the redundant semicolon.
> +static u32 get_mmdc_ch0_clk(void)
> +{
> + u32 cbcdr = __raw_readl(&imx_ccm->cbcdr);
> + u32 mmdc_ch0_podf = (cbcdr & MXC_CCM_CBCDR_MMDC_CH0_PODF_MASK) >>
> + MXC_CCM_CBCDR_MMDC_CH0_PODF_OFFSET;
> +
> + return get_periph_clk() / (mmdc_ch0_podf + 1);
> +}
add a newline here
> +u32 imx_get_uartclk(void)
> +{
> + return get_uart_clk();
> +}
This is ok - but I think we should switch to mxc_get_clock(MXC_UART_CLK)
in serial_mxc.c. However, this should be done in another patch.
> +
> + printf("\n");
> + printf("IPG %8d kHz\n", mxc_get_clock(MXC_IPG_CLK) / 1000);
> + printf("UART %8d kHz\n", mxc_get_clock(MXC_UART_CLK) / 1000);
> + printf("CSPI %8d kHz\n", mxc_get_clock(MXC_CSPI_CLK) / 1000);
> + printf("AHB %8d kHz\n", mxc_get_clock(MXC_AHB_CLK) / 1000);
> + printf("AXI %8d kHz\n", mxc_get_clock(MXC_AXI_CLK) / 1000);
> + printf("DDR %8d kHz\n", mxc_get_clock(MXC_DDR_CLK) / 1000);
> + printf("USDHC1 %8d kHz\n", mxc_get_clock(MXC_ESDHC_CLK) / 1000);
> + printf("USDHC2 %8d kHz\n", mxc_get_clock(MXC_ESDHC2_CLK) / 1000);
> + printf("USDHC3 %8d kHz\n", mxc_get_clock(MXC_ESDHC3_CLK) / 1000);
> + printf("USDHC4 %8d kHz\n", mxc_get_clock(MXC_ESDHC4_CLK) / 1000);
> + printf("EMI SLOW %8d kHz\n", mxc_get_clock(MXC_EMI_SLOW_CLK) / 1000);
> + printf("IPG PERCLK %8d kHz\n", mxc_get_clock(MXC_IPG_PERCLK) / 1000);
Not yet checked, but it seems to me that some lines are too long, are'nt
they ?
> diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c
> new file mode 100644
> index 0000000..dbf8b93
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/mx6/soc.c
> @@ -0,0 +1,93 @@
> +/*
> + * (C) Copyright 2007
> + * Sascha Hauer, Pengutronix
> + *
> + * (C) Copyright 2009 Freescale Semiconductor, Inc.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * 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
> + */
> +
> +#include <common.h>
> +#include <asm/errno.h>
> +#include <asm/io.h>
> +#include <asm/arch/imx-regs.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch/sys_proto.h>
> +
> +u32 get_cpu_rev(void)
> +{
> + int system_rev = 0x61000 | CHIP_REV_1_0;
> +
> + return system_rev;
> +}
> +#ifdef CONFIG_ARCH_CPU_INIT
> +void init_aips(void)
> +{
> + u32 reg = AIPS1_ON_BASE_ADDR;
I know that in manual (MX51, at least) this range is called
AIPS1_ON_PLATFORM, but we used the name AIPS1_BASE_ADDR for MX51 and
MX53. Could we stick with the same name ? It is easier for everybody
starting with IMX if the registers with the same meaning.
> +
> + /*
> + * Set all MPROTx to be non-bufferable, trusted for R/W,
> + * not forced to user-mode.
> + */
> + writel(0x77777777, reg + 0x00);
> + writel(0x77777777, reg + 0x04);
> +
> + writel(0x00000000, reg + 0x40);
> + writel(0x00000000, reg + 0x44);
> + writel(0x00000000, reg + 0x48);
> + writel(0x00000000, reg + 0x4c);
> + writel(0x00000000, reg + 0x50);
I would prefer to have a structure (as described in AIPSTZ Register
Memory Map) for this instead of a raw translating of the assembly routine.
> diff --git a/arch/arm/include/asm/arch-mx6/ccm_regs.h b/arch/arm/include/asm/arch-mx6/ccm_regs.h
> new file mode 100644
> index 0000000..b55cb49
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx6/ccm_regs.h
> @@ -0,0 +1,894 @@
> +/*
> + * Freescale ANADIG Register Definitions
^---
ANADIG: this is new. What is ?
> diff --git a/arch/arm/include/asm/arch-mx6/gpio.h b/arch/arm/include/asm/arch-mx6/gpio.h
> new file mode 100644
> index 0000000..20c4e57
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx6/gpio.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2011
> + * Stefano Babic, DENX Software Engineering, <sbabic at denx.de>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * 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 __ASM_ARCH_MX6_GPIO_H
> +#define __ASM_ARCH_MX6_GPIO_H
> +
> +/* GPIO registers */
> +struct gpio_regs {
> + u32 gpio_dr;
> + u32 gpio_dir;
> + u32 gpio_psr;
> +};
> +
> +#endif /* __ASM_ARCH_MX6_GPIO_H */
What about the proposal to have a common include path ?
> +++ b/arch/arm/include/asm/arch-mx6/mx6x_pins.h
> @@ -0,0 +1,1683 @@
> +/*
> + * Copyright (C) 2011 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.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Auto Generate file, please don't edit it
> + *
> + */
> +
> +#ifndef __ASM_ARCH_MX6_MX6X_PINS_H__
> +#define __ASM_ARCH_MX6_MX6X_PINS_H__
> +
> +#include <asm/arch/iomux-v3.h>
> +
> +/* Use to set PAD control */
> +#define PAD_CTL_HYS (1 << 16)
> +#define PAD_CTL_PUS_100K_DOWN (0 << 14)
> +#define PAD_CTL_PUS_47K_UP (1 << 14)
> +#define PAD_CTL_PUS_100K_UP (2 << 14)
> +#define PAD_CTL_PUS_22K_UP (3 << 14)
> +
> +#define PAD_CTL_PUE (1 << 13)
> +#define PAD_CTL_PKE (1 << 12)
> +#define PAD_CTL_ODE (1 << 11)
> +#define PAD_CTL_SPEED_LOW (1 << 6)
> +#define PAD_CTL_SPEED_MED (2 << 6)
> +#define PAD_CTL_SPEED_HIGH (3 << 6)
> +#define PAD_CTL_DSE_DISABLE (0 << 3)
> +#define PAD_CTL_DSE_240ohm (1 << 3)
> +#define PAD_CTL_DSE_120ohm (2 << 3)
> +#define PAD_CTL_DSE_80ohm (3 << 3)
> +#define PAD_CTL_DSE_60ohm (4 << 3)
> +#define PAD_CTL_DSE_48ohm (5 << 3)
> +#define PAD_CTL_DSE_40ohm (6 << 3)
> +#define PAD_CTL_DSE_34ohm (7 << 3)
> +#define PAD_CTL_SRE_FAST (1 << 0)
> +#define PAD_CTL_SRE_SLOW (0 << 0)
> +
> +#define NO_MUX_I 0x3FF
> +#define NO_PAD_I 0x7FF
> +
> +enum {
> + MX6Q_PAD_SD2_DAT1__USDHC2_DAT1 = IOMUX_PAD(0x0360, 0x004C, 0, 0x0000, 0, 0),
> + MX6Q_PAD_SD2_DAT1__ECSPI5_SS0 = IOMUX_PAD(0x0360, 0x004C, 1, 0x0834, 0, 0),
Only to remark. These lines will be marked with WARNING by checkpatch,
but it is surely more readable as it is in the patch as if each line
will be split. I have already merged the MX28 iomux, and even in that
case the single lines were longer as 80 bytes. If nobody complains, I
will merge the pin definition as it is.
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