Hi Tapani,
On 08/29/2013 10:07 PM, Tapani Utriainen wrote:
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.
<snip>
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?
Troy's patch-set defines some macros, then includes "pads.h" either once (for single architecture) or twice (for multi-arch images):
#ifdef CONFIG_MX6Q #include "pads.h" #endif #if defined(CONFIG_MX6DL) || defined(CONFIG_MX6S) #define FOR_DL_SOLO #include "pads.h" #endif
This allows pads.h to define the set of pads once, and build either one or two sets of tables.
This allows you to define the pads in one place, easing maintenance but requires the use of formalized naming of tables using MX6NAME() macro when defining and referencing per-architecture data.
<snip>
+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)
Right.
There is no way around having multiple tables. The "Troy" approach just makes it easier to create and maintain.
+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.
It seems to me that there should be a way to define the memory configuration data once and use it both in the imximage file to produce a DCD for a single-architecture binary or to produce a table for the multi-arch approach.
If you look at the structure of boundary/nitrogen6q.cfg and variants, you'll see that we pulled out the common parts (ddr-setup.cfg and clocks.cfg) and that the memory timings for each configuration and processor variant are separated into the files
1066mhz_4x128mx16.cfg ... 800mhz_2x256mx16.cfg
With some slight additional macro-fu, we should be able to do both with the same structure.
Regards,
Eric