From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tapani Utriainen Date: Fri, 30 Aug 2013 13:07:54 +0800 Subject: [U-Boot] [PATCH] Add support for TechNexion edm1-cf-imx6 SoM In-Reply-To: <521E2085.6070800@denx.de> References: <20130828192333.21f7ca6b@triceratops> <521E2085.6070800@denx.de> Message-ID: <20130830130754.5ab2d107@triceratops> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Stefano, Thank you for reviewing this patch, and for the constructive comments. Most of your comments are taken on board, and we will re-submit updated patches in the near future. On some things we probably need some clarification, see inlined responses to some of your questions. > > --- > > MAINTAINERS | 4 + > > arch/arm/include/asm/arch-mx6/spl.h | 19 + > > board/technexion/edm_cf_imx6/Makefile | 26 + > > board/technexion/edm_cf_imx6/README | 30 + > > board/technexion/edm_cf_imx6/clocks.cfg | 44 ++ > > board/technexion/edm_cf_imx6/edm_cf_imx6.c | 801 ++++++++++++++++++++++++ > > board/technexion/edm_cf_imx6/edm_cf_imx6_pins.h | 44 ++ > > board/technexion/edm_cf_imx6/imximage.cfg | 17 + > > boards.cfg | 1 + > > include/configs/edm_cf_imx6.h | 140 +++++ > > 10 files changed, 1126 insertions(+) > > create mode 100644 arch/arm/include/asm/arch-mx6/spl.h > > create mode 100644 board/technexion/edm_cf_imx6/Makefile > > create mode 100644 board/technexion/edm_cf_imx6/README > > create mode 100644 board/technexion/edm_cf_imx6/clocks.cfg > > create mode 100644 board/technexion/edm_cf_imx6/edm_cf_imx6.c > > create mode 100644 board/technexion/edm_cf_imx6/edm_cf_imx6_pins.h > > create mode 100644 board/technexion/edm_cf_imx6/imximage.cfg > > create mode 100644 include/configs/edm_cf_imx6.h > > > > The patch should be split into separate patches, and each of them fixes > a specific issue. From our previous discussion, we agree about: > > - we need to clean up conflicts in pad definitions - see Eric's answer. > - we need a way to simply defines pins for the different SOC variations. > Eric and Troy have already proposed a schema adding tables *only* into > the board file, and the generation of the table is quite automatic. > Let's say, if I am a board maintainer and I want to add support for a > board having all iMX6 variations, I would like to define only once which > pins I need, without replicating for each SOC variant. > - the same should be done with DDR and clocks, if necessary. > > After these preparation patches, there should be a patch preparing i.MX6 > for SPL - changes in i.MX6 common code should go here. > > Last, there will be a patch on top of the rest adding support to your board. > Understand. However, as far as I can tell Troy's suggestion is still creating the pad setup compile time for one cpu. Is there something obvious we are missing? > > Please change all license text according to the new rule with > SPDX-License-Identifier. > Will do. > > Maybe you should add some comments explaining that this is your > decision, not due to the SOC. Only the first dd is mandatory by iMX > (offset is 0x400 in flash). For u-boot, you have decided to put it into > raw data exactly after SPL and not into a filesystem. > > > + > > +Only raw mmc boot has been verified to work. > > The phrase is misleading, and let me think the other ways do not work. I > assume that you tested only raw mmc, so please rephrase to explain this. > Not using a filesystem is by choice. We'll clarify on that (or maybe better, enable FAT) ... > > + > > +static inline void setup_boot_device(void) > > +{ > > + uint soc_sbmr = readl(SRC_BASE_ADDR + 0x4); > > + uint bt_mem_ctl = (soc_sbmr & 0x000000FF) >> 4 ; > > + uint bt_mem_type = (soc_sbmr & 0x00000008) >> 3; > > + uint bt_mem_mmc = (soc_sbmr & 0x00001000) >> 12; > > + > > + switch (bt_mem_ctl) { > > + case 0x0: > > + if (bt_mem_type) > > + boot_dev = ONE_NAND_BOOT; > > + else > > + boot_dev = WEIM_NOR_BOOT; > > + break; > > + case 0x2: > > + boot_dev = SATA_BOOT; > > + break; > > + case 0x3: > > + if (bt_mem_type) > > + boot_dev = I2C_BOOT; > > + else > > + boot_dev = SPI_NOR_BOOT; > > + break; > > + case 0x4: > > + case 0x5: > > + if (bt_mem_mmc) > > + boot_dev = SD0_BOOT; > > + else > > + boot_dev = SD1_BOOT; > > + break; > > + case 0x6: > > + case 0x7: > > + boot_dev = MMC_BOOT; > > + break; > > + case 0x8 ... 0xf: > > + boot_dev = NAND_BOOT; > > + break; > > + default: > > + boot_dev = UNKNOWN_BOOT; > > + break; > > + } > > +} > > > > Let's say: boot device is not board dependent, and the required SPL > function should be moved into general code (imx_common or > arch/arm/cpu/armv7/mx6). > Will do. In the first patch attempt we deliberately tried not to touch common imx6 code. > + > > +int dram_init(void) > > +{ > > + uint cpurev, imxtype; > > + u32 sdram_size; > > + > > + cpurev = get_cpu_rev(); > > + imxtype = (cpurev & 0xFF000) >> 12; > > I am expecting to have a global function getting the cputype, with > macros of the type is_cpu_mx6q() (I see you use it later) to help us the > usage. Not here in board code. > Added. > > +static iomux_v3_cfg_t const edmdl_uart1_pads[] = { > > + MX6DL_PAD_CSI0_DAT10__UART1_TXD | MUX_PAD_CTRL(UART_PAD_CTRL), > > + MX6DL_PAD_CSI0_DAT11__UART1_RXD | MUX_PAD_CTRL(UART_PAD_CTRL), > > +}; > > + > > +static iomux_v3_cfg_t const edmq_uart1_pads[] = { > > + MX6Q_PAD_CSI0_DAT10__UART1_TXD | MUX_PAD_CTRL(UART_PAD_CTRL), > > + MX6Q_PAD_CSI0_DAT11__UART1_RXD | MUX_PAD_CTRL(UART_PAD_CTRL), > > +}; > > + > > +static iomux_v3_cfg_t const edmdl_usdhc1_pads[] = { > > + MX6DL_PAD_SD1_CLK__USDHC1_CLK | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6DL_PAD_SD1_CMD__USDHC1_CMD | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6DL_PAD_SD1_DAT0__USDHC1_DAT0 | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6DL_PAD_SD1_DAT1__USDHC1_DAT1 | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6DL_PAD_SD1_DAT2__USDHC1_DAT2 | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6DL_PAD_SD1_DAT3__USDHC1_DAT3 | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + /* Card detect */ > > + MX6DL_PAD_GPIO_2__GPIO_1_2 | MUX_PAD_CTRL(NO_PAD_CTRL), > > +}; > > + > > +static iomux_v3_cfg_t const edmq_usdhc1_pads[] = { > > + MX6Q_PAD_SD1_CLK__USDHC1_CLK | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6Q_PAD_SD1_CMD__USDHC1_CMD | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6Q_PAD_SD1_DAT0__USDHC1_DAT0 | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6Q_PAD_SD1_DAT1__USDHC1_DAT1 | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6Q_PAD_SD1_DAT2__USDHC1_DAT2 | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + MX6Q_PAD_SD1_DAT3__USDHC1_DAT3 | MUX_PAD_CTRL(USDHC_PAD_CTRL), > > + /* Card detect */ > > + MX6Q_PAD_GPIO_2__GPIO_1_2 | MUX_PAD_CTRL(NO_PAD_CTRL), > > +}; > > I do not like this solution: this is a bare duplication of the pads, and > it is prone to errors. The definitions of the table must be in some way > automatic. I want to define a pin: if SD1_CLK is used for MMC, this does > not depend from the SOC variant. > TBH, I hate this solution as well. But guess we heard Eric's cries for mercy, and did it the way Boundary (and several kernel board files) do it -- rather than the way we did it in the Wandboard kernel. > What about Troy's solution ? > Did not apply? And still had a mess of arrays in the board file. Or what we have *is* the multi-cpu variant of Troy's approach...? (With both CPU type macros expanded, manually) > > Let's say : this does not scale well. For each peripheral we have > exactly the same code. The if..then..else should be hidden in some way, > using macros to select the right table. > ... > > As you see, we can have a lot of some code. > ... > > Again here. > Beating a dead horse. I happily killed that mess in the WB kernel once already. > > + > > +int board_early_init_f(void) > > +{ > > + setup_iomux_uart(); > > + return 0; > > +} > > + > > +/* > > + * Do not overwrite the console > > + * Use always serial for U-Boot console > > + */ > > +int overwrite_console(void) > > +{ > > + return 1; > > +} > > I have not seen CONFIG_VIDEO in your configuration file. Is this dead code ? > Yes, we cut away the splash screen support from the submitted patch. (Our original patch was against another u-boot version, and we tried to cut out everything non-essential or thoroughly tested). Will cut even more :-) > > +#if defined(CONFIG_SPL_BUILD) > > +void board_init_f(ulong dummy) > > +{ > > + /* Set the stack pointer. */ > > + asm volatile("mov sp, %0\n" : : "r"(CONFIG_SPL_STACK)); > > + > > + /* Clear the BSS. */ > > + memset(__bss_start, 0, __bss_end - __bss_start); > > + > > + /* Set global data pointer. */ > > + gd = &gdata; > > + > > + arch_cpu_init(); > > + board_early_init_f(); > > + timer_init(); > > + preloader_console_init(); > > + > > + board_init_r(NULL, 0); > > +} > > None of this stuff is board specific - we need to factorize this, making > available for all i.MX6 boards. I will say, for i.MX5, too. > Will try. > > + > > +static void spl_dram_init_mx6solo_512mb(void) > > +{ > > + /* DDR3 initialization based on the MX6Solo Auto Reference Design (ARD) */ > > + /* DDR IO TYPE */ > > + writel(0x000c0000, IOMUXC_BASE_ADDR + 0x774); > > + writel(0x00000000, IOMUXC_BASE_ADDR + 0x754); > > + /* Clock */ > > + writel(0x00000030, IOMUXC_BASE_ADDR + 0x4ac); > > + writel(0x00000030, IOMUXC_BASE_ADDR + 0x4b0); > > + /* Address */ [ .... long list of writels ... ] > > + writel(0x04008032, MMDC_P0_BASE_ADDR + 0x01c); > > + writel(0x00008033, MMDC_P0_BASE_ADDR + 0x01c); > > + writel(0x00048031, MMDC_P0_BASE_ADDR + 0x01c); > > + writel(0x07208030, MMDC_P0_BASE_ADDR + 0x01c); > > + writel(0x04008040, MMDC_P0_BASE_ADDR + 0x01c); > > + writel(0x00005800, MMDC_P0_BASE_ADDR + 0x020); > > + writel(0x00011117, MMDC_P0_BASE_ADDR + 0x818); > > + writel(0x00011117, MMDC_P1_BASE_ADDR + 0x818); > > + writel(0x0002556d, MMDC_P0_BASE_ADDR + 0x004); > > + writel(0x00011006, MMDC_P1_BASE_ADDR + 0x004); > > + writel(0x00000000, MMDC_P0_BASE_ADDR + 0x01c); > > +} > > Really I do not like this solution. First, you should accessor to set > the iomux, without using base address + offset. And of course, access to > the ram controller should be done in the same way using a C structure, > not offsets. > > Then the problem is similar to the pads. I will propose we have a > general function, and the values of board specific parameters (32 > against 64 bit size, calibration registers, and so on), and start the > ddr procedure. The functions here do the same on different registers. > We agree that the does does not look pretty. But there needs to be some clarification. However, using this has some benefits: * It is easier to convert from (and compare to) DCD tables * Easier to look things up in the TRM (base + offset are easier to find in a long list of registers sorted by offset, than a name) * It is a lot of effort to do struct accessors for huge tables. Both of IOMUX and MMDC are large (offsets of 0x800+). Having struct accessors would take quite long to enter manually (two tables of 500+ entries each, and different between cpu types). This would be hours, if not a day of braindead work without any tangible benefit. I could make those tables of { offset, value } and do the writels using a for-loop, but that would just mariginally improve on the ugliness. > > +u32 spl_boot_device(void) > > +{ > > + puts("Boot Device: "); > > + switch (get_boot_device()) { > > + case SD0_BOOT: > > + printf("SD0\n"); > > + return BOOT_DEVICE_MMC1; > > + case SD1_BOOT: > > + printf("SD1\n"); > > + return BOOT_DEVICE_MMC2; > > + case MMC_BOOT: > > + printf("MMC\n"); > > + return BOOT_DEVICE_MMC2; > > + case NAND_BOOT: > > + printf("NAND\n"); > > + return BOOT_DEVICE_NAND; > > + case UNKNOWN_BOOT: > > + default: > > + printf("UNKNOWN\n"); > > + return BOOT_DEVICE_NONE; > > + } > > +} > > This is also common code. > Yes. > > + > > +u32 spl_boot_mode(void) > > +{ > > + switch (spl_boot_device()) { > > + case BOOT_DEVICE_MMC1: > > + case BOOT_DEVICE_MMC2: > > + case BOOT_DEVICE_MMC2_2: > > +#ifdef CONFIG_SPL_FAT_SUPPORT > > + return MMCSD_MODE_FAT; > > +#else > > + return MMCSD_MODE_RAW; > > +#endif > > + break; > > + case BOOT_DEVICE_NAND: > > + default: > > + puts("spl: ERROR: unsupported device\n"); > > + hang(); > > + } > > +} > > + > > +void reset_cpu(ulong addr) > > +{ > > + > > +} > > reset_cpu for imx should activate the watchdog, see > drivers/watchdog/imx_watchdog.c > Ok. > > diff --git a/boards.cfg b/boards.cfg > > index 79d6cd8..b7d66ff 100644 > > --- a/boards.cfg > > +++ b/boards.cfg > > @@ -288,6 +288,7 @@ nitrogen6s1g arm armv7 nitrogen6x boundar > > wandboard_dl arm armv7 wandboard - mx6 wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL,DDR_MB=1024 > > wandboard_quad arm armv7 wandboard - mx6 wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q,DDR_MB=2048 > > wandboard_solo arm armv7 wandboard - mx6 wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6s.cfg,MX6S,DDR_MB=512 > > +edm_cf_imx6 arm armv7 edm_cf_imx6 technexion mx6 edm_cf_imx6:IMX_CONFIG=board/technexion/edm_cf_imx6/imximage.cfg,SPL > > Why CONFIG_SPL is not set into the configuration file ? Is there a > version without SPL ? > There used to be... yes, we'll fix that. > Please maintain the list sorted. > No problem. ... > > + > > +#define MACH_TYPE_EDM_CF_IMX6 4257 > > +#define CONFIG_MACH_TYPE MACH_TYPE_EDM_CF_IMX6 > > if MACH_TYPE_EDM_CF_IMX6 is used only here, better: > > #define CONFIG_MACH_TYPE 4257 > Yes, guess we tried to mimic wandboard. > > + > > +#define CONFIG_CMDLINE_TAG > > +#define CONFIG_SETUP_MEMORY_TAGS > > +#define CONFIG_REVISION_TAG > > + > > > +#ifdef CONFIG_SPL > > +#define CONFIG_SPL_FRAMEWORK > > +#define CONFIG_SPL_TEXT_BASE 0x00908400 > > Ok - SPL goes into IRAM, that is good. Can you explain me the value of > the 0x8400 offset ? > The available IRAM starts at 907000 and we need space for the IVT tables. > > +#define CONFIG_SPL_PAD_TO 0x400 > > +#define CONFIG_SPL_START_S_PATH "arch/arm/cpu/armv7" > > +#define CONFIG_SPL_STACK 0x0091FFB8 > > Maybe better set it with some size - where is coming this value ? > >From page 393 in the imx6solo TRM. That is Freescale's recommended initial stack top. regards, //Tapani