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;elseboot_dev = WEIM_NOR_BOOT;break;- case 0x2:
boot_dev = SATA_BOOT;break;- case 0x3:
if (bt_mem_type)boot_dev = I2C_BOOT;elseboot_dev = SPI_NOR_BOOT;break;- case 0x4:
- case 0x5:
if (bt_mem_mmc)boot_dev = SD0_BOOT;elseboot_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