On Mon, Nov 21, 2011 at 10:44 PM, Stefano Babic <sba...@denx.de> wrote:
> 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....@linaro.org>
>> Cc:Stefano Babic <sba...@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.
yes, understood. Thanks for the review.
>
>> +#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 ?
IMHO, This should be OK.
>
>> +
>> + return PLL3_80M / (uart_podf + 1);
>> +}
>> +
>> +
>
> One newline too much
yes, will remove it.
>
>> + 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.
yes, good eyesight.
>
>> +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
OK,
>
>> +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.
yes, will do it in another patch to do the clean up.
>
>> +
>> + 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 ?
Yes, too long, will fix it.
>
>> 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.
>
OK, agree.
>
>> +
>> + /*
>> + * 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.
I double check with IC team, that we don't need do it here.
So I will remove it.
>
>> 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 ?
hmm, It's the CCM register, I will change it to CCM.
>
>
>> 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, <sba...@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 ?
I remember that there is not must have feature. And I consider to add
another patch to do clean up for the whole i.mx5 and i.mx6. Agree?
>
>> +++ 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.
Yes, it's better to keep as it for more readable, linux kernel also has a lot of
such situation.
>
> 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: off...@denx.de
> =====================================================================
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
