Dear Fred Fan,
In message 12537172421605-git-send-email-fanyefeng@gmail.com you wrote:
This patch just supports boot into u-boot from mmc or spi-nor flash. It just implements console, iomux and clock. There are no ethernet, nor-flash, mmc or other peripheral drivers.
Signed-off-by: Fred.Fan fanyefeng@gmail.com Signed-off-by: Fred.Fan r01011@freescale.com
Please use a shorter subject (it gets tructaed), something like:
mx51_babbage: add support for Freescale iMX51 board
diff --git a/MAKEALL b/MAKEALL index edebaea..852baf1 100755 --- a/MAKEALL +++ b/MAKEALL @@ -581,6 +581,7 @@ LIST_ARM_CORTEX_A8=" \ omap3_pandora \ omap3_zoom1 \ omap3_zoom2 \
- mx51_babbage \
Please keep lists sorted.
diff --git a/board/freescale/mx51_babbage/board-mx51_babbage.h b/board/freescale/mx51_babbage/board-mx51_babbage.h new file mode 100644 index 0000000..31e3875 --- /dev/null +++ b/board/freescale/mx51_babbage/board-mx51_babbage.h
Please drop the "board-" part of the file name.
+/*
- The code contained herein is licensed under the GNU General Public
- License. You may obtain a copy of the GNU General Public License
- Version 2 or later at the following locations:
Please use correct licnese terms. Specify which version of the license applies, i. e. use something like:
This code is licensed under the GPL, version 2 (or any later version).
+/*!
- @defgroup BRDCFG_MX51_BABBAGE Board Configuration Options
- @ingroup MSL_MX51_BABBAGE
- */
Please get rid of this "!", "@defgroup", "@ingroup" etc. stuff.
+/*!
- @file mx51_babbage/board-mx51_babbage.h
Drop this line. It carries no information.
- @brief This file contains all the board level configuration options.
Drop the blank line.
- It currently hold the options defined for MX51 babbage Platform.
- @ingroup BRDCFG_MX51_BABBAGE
Drop this line, Please apply similar to the rest of the code.
+/* CPLD offsets */ +#define PBC_LED_CTRL (0x20000) +#define PBC_SB_STAT (0x20008) +#define PBC_ID_AAAA (0x20040) +#define PBC_ID_5555 (0x20048) +#define PBC_VERSION (0x20050) +#define PBC_ID_CAFE (0x20058) +#define PBC_INT_STAT (0x20010) +#define PBC_INT_MASK (0x20038) +#define PBC_INT_REST (0x20020) +#define PBC_SW_RESET (0x20060)
Don't use base address + offsets, but declare proper C structures to describe the register layout. Then use I/O accessors to access registers.
diff --git a/board/freescale/mx51_babbage/config.mk b/board/freescale/mx51_babbage/config.mk new file mode 100644 index 0000000..d8b0f10 --- /dev/null +++ b/board/freescale/mx51_babbage/config.mk @@ -0,0 +1,2 @@ +LDSCRIPT = board/$(VENDOR)/$(BOARD)/u-boot.lds +TEXT_BASE = 0x97800000
Even though the file is short, please add a (C) and license header.
diff --git a/board/freescale/mx51_babbage/flash_header.S b/board/freescale/mx51_babbage/flash_header.S new file mode 100644 index 0000000..bbac2c1 --- /dev/null +++ b/board/freescale/mx51_babbage/flash_header.S
...
+/*!
- @file mx51_babbage/flash_head.S
- @brief Thie file contains the information as the image header for boot.
S/Thie/This/ ?
Please explain what "image header" means. U-Boot has an image geader, too, but you probably refer to something different.
- The boot rom code will parse this structure , load image and initial
- hardware as the description of hardware setting.
- */
I read: "The ROM code will ... load ... initial hardware" ?
Something seems to be missing, or do you mean s/initial/initialize/ ?
Please clean up.
+#include <config.h> +#include <asm/arch/mx51.h> +#include "board-mx51_babbage.h"
+#ifdef CONFIG_FLASH_HEADER +#ifndef CONFIG_FLASH_HEADER_OFFSET
It seems this is a iMX51 specific thing, right? Then please make this clear in the name oif the file and the macro names. "FLASH_HEADER" or "flash_head" is way too generic.
diff --git a/board/freescale/mx51_babbage/lowlevel_init.S b/board/freescale/mx51_babbage/lowlevel_init.S new file mode 100644 index 0000000..bf536b1 --- /dev/null +++ b/board/freescale/mx51_babbage/lowlevel_init.S
How much of this is really board specific code, or how much of it should be moved to the CPU directory instead?
+int dram_init(void) +{
- gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
- gd->bd->bi_dram[0].size = PHYS_SDRAM_1_SIZE;
Please use get_ram_size() for autosizing and (simple) RAM test.
+}
+static void setup_uart(void) +{
- unsigned int pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE |
PAD_CTL_PUE_PULL | PAD_CTL_DRV_HIGH;- mxc_request_iomux(MX51_PIN_UART1_RXD, IOMUX_CONFIG_ALT0);
- mxc_iomux_set_pad(MX51_PIN_UART1_RXD, pad | PAD_CTL_SRE_FAST);
- mxc_request_iomux(MX51_PIN_UART1_TXD, IOMUX_CONFIG_ALT0);
- mxc_iomux_set_pad(MX51_PIN_UART1_TXD, pad | PAD_CTL_SRE_FAST);
- mxc_request_iomux(MX51_PIN_UART1_RTS, IOMUX_CONFIG_ALT0);
- mxc_iomux_set_pad(MX51_PIN_UART1_RTS, pad);
- mxc_request_iomux(MX51_PIN_UART1_CTS, IOMUX_CONFIG_ALT0);
- mxc_iomux_set_pad(MX51_PIN_UART1_CTS, pad);
Hmm... the comment in the code for mxc_request_iomux() reads:
* Request ownership for an IO pin. This function has to be the first one * being called before that pin is used. The caller has to check the * return value to make sure it returns 0.
I do not see any such checks in your code?
Either they are missing here, or the whole mxc_request_iomux() is not needed at all.
+static void setup_expio(void) +{
...
/* WAL=0, WBED=1, WWSC=50, WADVA=2, WADVN=6, WEA=0,* WEN=0, WCSA=0, WCSN=0*/
Incorrect multiline comment. Please fix globally.
- /* Reset interrupt status reg */
- writew(0x1F, mx51_io_base_addr + PBC_INT_REST);
- writew(0x00, mx51_io_base_addr + PBC_INT_REST);
- writew(0xFFFF, mx51_io_base_addr + PBC_INT_MASK);
As mentioned above: do not use base address plus offset, but use a proper C structure to define the register map.
+#ifdef BOARD_LATE_INIT +int board_late_init(void) +{
- return 0;
+} +#endif
If the function is not needed, then don't implement it. Don;t add dead code.
+int checkboard(void) +{
- printf("Board: MX51 BABBAGE ");
- if (system_rev & CHIP_REV_2_5)
printf("2.5 [");- else if (system_rev & CHIP_REV_2_0)
printf("2.0 [");- else if (system_rev & CHIP_REV_1_1)
printf("1.1 [");- else
printf("1.0 [");
Please use puts() for constant strings that don;t need any formatting.
diff --git a/board/freescale/mx51_babbage/u-boot.lds b/board/freescale/mx51_babbage/u-boot.lds new file mode 100644 index 0000000..f608a6d --- /dev/null +++ b/board/freescale/mx51_babbage/u-boot.lds
Does this board really need a specific linker script? Can we not use a generic one for all (or most of the) i.MX51 boards?
- . = ALIGN(4);
- .text :
- {
/* WARNING - the following is hand-optimized to fit within *//* the sector layout of our flash chips! XXX FIXME XXX */
Why is this needed / useful? It doesn't seem to match the environment definitions in your board config file.
...
diff --git a/cpu/arm_cortexa8/mx51/clock.c b/cpu/arm_cortexa8/mx51/clock.c new file mode 100644 index 0000000..3abcdc8 --- /dev/null +++ b/cpu/arm_cortexa8/mx51/clock.c
...
+static u32 __decode_pll(enum pll_clocks pll, u32 infreq)
...
+u32 __get_mcu_main_clk(void)
...
+static u32 __get_periph_clk(void)
Is there any special reason for the "__" pat in all these names? You are aware that this has a special meaning in standard conforming C code, aren't you?
Please also add comments what all these functions are doing.
Hey, and add comments what the code is doing. It's not exactly obvious to me.
- u32 reg;
- reg = __raw_readl(MXC_CCM_CBCDR);
- if (reg & MXC_CCM_CBCDR_PERIPH_CLK_SEL) {
reg = __raw_readl(MXC_CCM_CBCMR);switch ((reg & MXC_CCM_CBCMR_PERIPH_CLK_SEL_MASK) >>MXC_CCM_CBCMR_PERIPH_CLK_SEL_OFFSET) {case 0:return __decode_pll(PLL1_CLK, CONFIG_MX51_HCLK_FREQ);case 1:return __decode_pll(PLL3_CLK, CONFIG_MX51_HCLK_FREQ);default:return 0;}- }
- return __decode_pll(PLL2_CLK, CONFIG_MX51_HCLK_FREQ);
In such cases it's better to turn around the logig, i. e. write:
if ((reg & MXC_CCM_CBCDR_PERIPH_CLK_SEL) == 0) return __decode_pll(PLL2_CLK, CONFIG_MX51_HCLK_FREQ); reg = __raw_readl(MXC_CCM_CBCMR); switch ((reg & MXC_CCM_CBCMR_PERIPH_CLK_SEL_MASK) >> MXC_CCM_CBCMR_PERIPH_CLK_SEL_OFFSET) { case 0: return __decode_pll(PLL1_CLK, CONFIG_MX51_HCLK_FREQ); case 1: return __decode_pll(PLL3_CLK, CONFIG_MX51_HCLK_FREQ); default: return 0; } /* NOTREACHED */
You see? Less indentation = easier to read.
...
+void mxc_show_clocks(void) +{
- u32 freq;
- freq = __decode_pll(PLL1_CLK, CONFIG_MX51_HCLK_FREQ);
- printf("mx51 pll1: %dMHz\n", freq / 1000000);
- freq = __decode_pll(PLL2_CLK, CONFIG_MX51_HCLK_FREQ);
- printf("mx51 pll2: %dMHz\n", freq / 1000000);
- freq = __decode_pll(PLL3_CLK, CONFIG_MX51_HCLK_FREQ);
- printf("mx51 pll3: %dMHz\n", freq / 1000000);
- printf("ipg clock : %dHz\n", mxc_get_clock(MXC_IPG_CLK));
- printf("ipg per clock : %dHz\n", mxc_get_clock(MXC_IPG_PERCLK));
- printf("uart clock : %dHz\n", mxc_get_clock(MXC_UART_CLK));
- printf("cspi clock : %dHz\n", mxc_get_clock(MXC_CSPI_CLK));
Please don't print too much during regular boot. Either make this debug() code, or provide a separate command so the user can print this if he is interested in this information. Standard boot output shall be terse, as it affects boot ime.
diff --git a/cpu/arm_cortexa8/mx51/crm_regs.h b/cpu/arm_cortexa8/mx51/crm_regs.h new file mode 100644 index 0000000..6436bed --- /dev/null +++ b/cpu/arm_cortexa8/mx51/crm_regs.h
Should this file go to include/... ?
+/*
- Copyright 2009 Freescale Semiconductor, Inc. All Rights Reserved.
- */
+/*
- The code contained herein is licensed under the GNU General Public
- License. You may obtain a copy of the GNU General Public License
- Version 2 or later at the following locations:
See license header comments above.
+#ifndef __ARCH_ARM_MACH_MX51_CRM_REGS_H__ +#define __ARCH_ARM_MACH_MX51_CRM_REGS_H__
+#define MXC_CCM_BASE CCM_BASE_ADDR +#define MXC_DPLL1_BASE PLL1_BASE_ADDR +#define MXC_DPLL2_BASE PLL2_BASE_ADDR +#define MXC_DPLL3_BASE PLL3_BASE_ADDR
+/* PLL Register Offsets */ +#define MXC_PLL_DP_CTL 0x00 +#define MXC_PLL_DP_CONFIG 0x04 +#define MXC_PLL_DP_OP 0x08 +#define MXC_PLL_DP_MFD 0x0C +#define MXC_PLL_DP_MFN 0x10 +#define MXC_PLL_DP_MFNMINUS 0x14 +#define MXC_PLL_DP_MFNPLUS 0x18 +#define MXC_PLL_DP_HFS_OP 0x1C +#define MXC_PLL_DP_HFS_MFD 0x20 +#define MXC_PLL_DP_HFS_MFN 0x24 +#define MXC_PLL_DP_MFN_TOGC 0x28 +#define MXC_PLL_DP_DESTAT 0x2c
See comments above - use C structs instead of offset definitions.
diff --git a/cpu/arm_cortexa8/mx51/iomux.c b/cpu/arm_cortexa8/mx51/iomux.c new file mode 100644 index 0000000..80082aa --- /dev/null +++ b/cpu/arm_cortexa8/mx51/iomux.c
...
+static inline u32 _get_mux_reg(iomux_pin_name_t pin) +{
- u32 mux_reg = PIN_TO_IOMUX_MUX(pin);
- if (is_soc_rev(CHIP_REV_2_0) < 0) {
if ((pin == MX51_PIN_NANDF_RB5) ||(pin == MX51_PIN_NANDF_RB6) ||(pin == MX51_PIN_NANDF_RB7)); /* Do nothing */else if (mux_reg >= 0x2FC)mux_reg += 8;else if (mux_reg >= 0x130)mux_reg += 0xC;- }
- mux_reg += IOMUXSW_MUX_CTL;
- return mux_reg;
+}
+static inline u32 _get_pad_reg(iomux_pin_name_t pin) +{
Please add comments what all this stuff is supposed to do.
- if (is_soc_rev(CHIP_REV_2_0) < 0) {
if ((pin == MX51_PIN_NANDF_RB5) ||(pin == MX51_PIN_NANDF_RB6) ||(pin == MX51_PIN_NANDF_RB7)); /* Do nothing */else if (pad_reg == 0x4D0 - PAD_I_START)pad_reg += 0x4C;else if (pad_reg == 0x860 - PAD_I_START)pad_reg += 0x9C;else if (pad_reg >= 0x804 - PAD_I_START)pad_reg += 0xB0;else if (pad_reg >= 0x7FC - PAD_I_START)pad_reg += 0xB4;else if (pad_reg >= 0x4E4 - PAD_I_START)pad_reg += 0xCC;elsepad_reg += 8;
You may also want to explain suchblack magic like the code here.
And please add parens around the (0x??? - PAD_I_START) parts.
...
+/*!
- This function is used to configure a pin through the IOMUX module.
- FIXED ME: for backward compatible. Will be static function!
I don't understand what this means. Please explain.
- @param pin a pin number as defined in \b #iomux_pin_name_t
- @param cfg an output function as defined in \b #iomux_pin_cfg_t
- @return 0 if successful; Non-zero otherwise
- */
+static int iomux_config_mux(iomux_pin_name_t pin, iomux_pin_cfg_t cfg)
...
+#if defined(CONFIG_DISPLAY_CPUINFO) +int print_cpuinfo(void) +{
- printf("CPU: Freescale i.MX51 family %d.%dV at %d MHz\n",
(get_board_rev() & 0xFF) >> 4,(get_board_rev() & 0xF),__get_mcu_main_clk() / 1000000);- mxc_show_clocks();
Please see above - please be terse.
diff --git a/cpu/arm_cortexa8/mx51/timer.c b/cpu/arm_cortexa8/mx51/timer.c new file mode 100644 index 0000000..5201768 --- /dev/null +++ b/cpu/arm_cortexa8/mx51/timer.c
...
+/* General purpose timers registers */ +#define GPTCR GPT1_BASE_ADDR /* Control register */ +#define GPTPR (GPT1_BASE_ADDR + 0x4) /* Prescaler register */ +#define GPTSR (GPT1_BASE_ADDR + 0x8) /* Status register */ +#define GPTCNT (GPT1_BASE_ADDR + 0x24) /* Counter register */
+/* General purpose timers bitfields */ +#define GPTCR_SWR (1<<15) /* Software reset */ +#define GPTCR_FRR (1<<9) /* Freerun / restart */ +#define GPTCR_CLKSOURCE_32 (4<<6) /* Clock source */ +#define GPTCR_TEN (1) /* Timer enable */
Move to header file. Use C struct instead of base addr + offsets.
...
+/*!
- defines for SPBA modules
- */
+#define SPBA_SDHC1 0x04 +#define SPBA_SDHC2 0x08 +#define SPBA_UART3 0x0C +#define SPBA_CSPI1 0x10 +#define SPBA_SSI2 0x14 +#define SPBA_SDHC3 0x20 +#define SPBA_SDHC4 0x24 +#define SPBA_SPDIF 0x28 +#define SPBA_ATA 0x30 +#define SPBA_SLIM 0x34 +#define SPBA_HSI2C 0x38 +#define SPBA_CTRL 0x3C
Use C struct ?
+/*
- Interrupt numbers
- */
+#define MXC_INT_BASE 0 +#define MXC_INT_RESV0 0 +#define MXC_INT_MMC_SDHC1 1 +#define MXC_INT_MMC_SDHC2 2 +#define MXC_INT_MMC_SDHC3 3 +#define MXC_INT_MMC_SDHC4 4 +#define MXC_INT_RESV5 5 +#define MXC_INT_SDMA 6 +#define MXC_INT_IOMUX 7 +#define MXC_INT_NFC 8 +#define MXC_INT_VPU 9 +#define MXC_INT_IPU_ERR 10 +#define MXC_INT_IPU_SYN 11 +#define MXC_INT_GPU 12 +#define MXC_INT_RESV13 13 +#define MXC_INT_USB_H1 14 +#define MXC_INT_EMI 15 +#define MXC_INT_USB_H2 16 +#define MXC_INT_USB_H3 17 +#define MXC_INT_USB_OTG 18 +#define MXC_INT_SAHARA_H0 19 +#define MXC_INT_SAHARA_H1 20 +#define MXC_INT_SCC_SMN 21 +#define MXC_INT_SCC_STZ 22 +#define MXC_INT_SCC_SCM 23 +#define MXC_INT_SRTC_NTZ 24 +#define MXC_INT_SRTC_TZ 25 +#define MXC_INT_RTIC 26 +#define MXC_INT_CSU 27 +#define MXC_INT_SLIM_B 28 +#define MXC_INT_SSI1 29 +#define MXC_INT_SSI2 30 +#define MXC_INT_UART1 31 +#define MXC_INT_UART2 32 +#define MXC_INT_UART3 33 +#define MXC_INT_RESV34 34 +#define MXC_INT_RESV35 35 +#define MXC_INT_CSPI1 36 +#define MXC_INT_CSPI2 37 +#define MXC_INT_CSPI 38 +#define MXC_INT_GPT 39 +#define MXC_INT_EPIT1 40 +#define MXC_INT_EPIT2 41 +#define MXC_INT_GPIO1_INT7 42 +#define MXC_INT_GPIO1_INT6 43 +#define MXC_INT_GPIO1_INT5 44 +#define MXC_INT_GPIO1_INT4 45 +#define MXC_INT_GPIO1_INT3 46 +#define MXC_INT_GPIO1_INT2 47 +#define MXC_INT_GPIO1_INT1 48 +#define MXC_INT_GPIO1_INT0 49 +#define MXC_INT_GPIO1_LOW 50 +#define MXC_INT_GPIO1_HIGH 51 +#define MXC_INT_GPIO2_LOW 52 +#define MXC_INT_GPIO2_HIGH 53 +#define MXC_INT_GPIO3_LOW 54 +#define MXC_INT_GPIO3_HIGH 55 +#define MXC_INT_GPIO4_LOW 56 +#define MXC_INT_GPIO4_HIGH 57 +#define MXC_INT_WDOG1 58 +#define MXC_INT_WDOG2 59 +#define MXC_INT_KPP 60 +#define MXC_INT_PWM1 61 +#define MXC_INT_I2C1 62 +#define MXC_INT_I2C2 63 +#define MXC_INT_HS_I2C 64 +#define MXC_INT_RESV65 65 +#define MXC_INT_RESV66 66 +#define MXC_INT_SIM_IPB 67 +#define MXC_INT_SIM_DAT 68 +#define MXC_INT_IIM 69 +#define MXC_INT_ATA 70 +#define MXC_INT_CCM1 71 +#define MXC_INT_CCM2 72 +#define MXC_INT_GPC1 73 +#define MXC_INT_GPC2 74 +#define MXC_INT_SRC 75 +#define MXC_INT_NM 76 +#define MXC_INT_PMU 77 +#define MXC_INT_CTI_IRQ 78 +#define MXC_INT_CTI1_TG0 79 +#define MXC_INT_CTI1_TG1 80 +#define MXC_INT_MCG_ERR 81 +#define MXC_INT_MCG_TMR 82 +#define MXC_INT_MCG_FUNC 83 +#define MXC_INT_RESV84 84 +#define MXC_INT_RESV85 85 +#define MXC_INT_RESV86 86 +#define MXC_INT_FEC 87 +#define MXC_INT_OWIRE 88 +#define MXC_INT_CTI1_TG2 89 +#define MXC_INT_SJC 90 +#define MXC_INT_SPDIF 91 +#define MXC_INT_TVE 92 +#define MXC_INT_FIRI 93 +#define MXC_INT_PWM2 94 +#define MXC_INT_SLIM_EXP 95 +#define MXC_INT_SSI3 96 +#define MXC_INT_RESV97 97 +#define MXC_INT_CTI1_TG3 98 +#define MXC_INT_SMC_RX 99 +#define MXC_INT_VPU_IDLE 100 +#define MXC_INT_RESV101 101 +#define MXC_INT_GPU_IDLE 102
+#define MXC_MAX_INT_LINES 128
Use C struct ?
And so on and on ...
diff --git a/include/configs/mx51_babbage.h b/include/configs/mx51_babbage.h new file mode 100644 index 0000000..a24b39b --- /dev/null +++ b/include/configs/mx51_babbage.h
...
+#define BOARD_LATE_INIT
Why do you #define this, when you don't need it at all?
+#define CONFIG_SYS_MEMTEST_START 0 /* memtest works on */ +#define CONFIG_SYS_MEMTEST_END 0x10000
That's not exactly a thorough testing, then. And is low memory really unused? Please check!
+#undef CONFIG_SYS_CLKS_IN_HZ /* everything, incl board info, in Hz */
Don't undef what doesn't exist.
+#define CONFIG_SYS_LOAD_ADDR CONFIG_LOADADDR
+#define CONFIG_SYS_HZ CONFIG_MX51_CLK32/* use 32kHz clock as source */
CONFIG_SYS_HZ must be 1000.
Best regards,
Wolfgang Denk