[U-Boot] [PATCH 11/11] ventana: switch to SPL

[Stefano Babic <sbabic@denx.de>]

Hi Tim,

On 24/04/2014 10:06, Tim Harvey wrote:
>>> +
>>> +/* configure mx6 mmdc io registers */
>>> +struct mx6_mmdc_ioregs mmdc_ioregs = {
>>> +     /* DDR3 */
>>> +     .mmdc_grp_ddr_type = 0x000c0000,
>>> +     /* disable DDR pullups */
>>> +     .mmdc_grp_ddrpke = 0x00000000,
>>> +     /* SDCLK[0:1], CAS, RAS, Reset: Differential input, 40ohm */
>>> +     .mmdc_dram_sdclk_0 = 0x00020030,
>>> +     .mmdc_dram_sdclk_1 = 0x00020030,
>>> +     .mmdc_dram_cas = 0x00020030,
>>> +     .mmdc_dram_ras = 0x00020030,
>>> +     .mmdc_dram_reset = 0x00020030,
>>> +     /* ADDR[00:16], SDBA[0:1]: 40 ohm */
>>> +     .mmdc_grp_addds = 0x00000030,
>>> +     /* SDCKE[0:1]: 100k pull-up */
>>> +     .mmdc_dram_sdcke0 = 0x00003000,
>>> +     .mmdc_dram_sdcke1 = 0x00003000,
>>> +     /* SDBA2: pull-up disabled */
>>> +     .mmdc_dram_sdba2 = 0x00000000,
>>> +     /* SDODT[0:1]: 100k pull-up, 40 ohm */
>>> +     .mmdc_dram_sdodt0 = 0x00003030,
>>> +     .mmdc_dram_sdodt1 = 0x00003030,
>>> +     /* CS0/CS1/SDBA2/CKE0/CKE1/SDWE: 40 ohm */
>>> +     .mmdc_grp_ctlds = 0x00000030,
>>> +     /* SDQS[0:7]: Differential input, 40 ohm */
>>> +     .mmdc_ddrmode_ctl = 0x00020000,
>>> +     .mmdc_dram_sdqs0 = 0x00000030,
>>> +     .mmdc_dram_sdqs1 = 0x00000030,
>>> +     .mmdc_dram_sdqs2 = 0x00000030,
>>> +     .mmdc_dram_sdqs3 = 0x00000030,
>>> +     .mmdc_dram_sdqs4 = 0x00000030,
>>> +     .mmdc_dram_sdqs5 = 0x00000030,
>>> +     .mmdc_dram_sdqs6 = 0x00000030,
>>> +     .mmdc_dram_sdqs7 = 0x00000030,
>>> +
>>> +     /* DATA[00:63]: Differential input, 40 ohm */
>>> +     .mmdc_grp_ddrmode = 0x00020000,
>>> +     .mmdc_grp_b0ds = 0x00000030,
>>> +     .mmdc_grp_b1ds = 0x00000030,
>>> +     .mmdc_grp_b2ds = 0x00000030,
>>> +     .mmdc_grp_b3ds = 0x00000030,
>>> +     .mmdc_grp_b4ds = 0x00000030,
>>> +     .mmdc_grp_b5ds = 0x00000030,
>>> +     .mmdc_grp_b6ds = 0x00000030,
>>> +     .mmdc_grp_b7ds = 0x00000030,
>>> +
>>> +     /* DQM[0:7]: Differential input, 40 ohm */
>>> +     .mmdc_dram_dqm0 = 0x00020030,
>>> +     .mmdc_dram_dqm1 = 0x00020030,
>>> +     .mmdc_dram_dqm2 = 0x00020030,
>>> +     .mmdc_dram_dqm3 = 0x00020030,
>>> +     .mmdc_dram_dqm4 = 0x00020030,
>>> +     .mmdc_dram_dqm5 = 0x00020030,
>>> +     .mmdc_dram_dqm6 = 0x00020030,
>>> +     .mmdc_dram_dqm7 = 0x00020030,
>>> +};
>>
>> I will suggest you move these structure in a separate file. It is then
>> easier for a board developer to understand what is very board specific.
> 
> hmmm... they are all very board specific but ok.

Well, I am noy saying to move it outside board/gateworks/gw_ventana. My
proposal is more as to signalize how to add SPL support for a new board.
Anyway, I agree it is a personal taste.

> 
> not needed - I used it mainly for debugging and will remove it.
> 
>>
>>> +
>>> +     board_init_r(NULL, 0);
>>> +}
>>
>> Mmhhh...apart the access to the eeprom to get the ram size, this
>> function should be common.
> 
> maybe, but I think we should wait to see what other boards come up
> with SPL support to see what actually ends up being common. An SPL
> that supports SPI or some of the other boot devices may need to do
> some additional things.

Agree, this makes sense. We can do it later.

> 
> yes... considering we currently have 4 baseboards (a fifth on the
> way), supporting both IMX6DL or IMX6Q on each, and have 2 memory
> densities (a third on the way) SPL is a must for my sanity to
> eliminate what would be something like 5*2*3 various build-time
> configurations.
> 
> The dynamic ddr configuration functionality I'm proposing helps out
> tremendously as well because the DDR3 calibration and testing I've
> done tells me that each board design (varied layout between SoC and
> DDR3) needs its own calibration values due to propagation delays and
> IMX6Q/D vs IMX6DL/SOLO need different calibration values on each board
> as well. This can all be handled with minimal tables and easily
> configured at runtime via baseboard detection, cpu detection, and
> memory density information.

Very nice.

>>> +#define CONFIG_SYS_NAND_U_BOOT_OFFS     (14 * 1024 * 1024)
>>
>> This is ok - it is your decision where to put it.
>>
>> May I ask why do you need 14 MB at the beginning ? It seems you lose a
>> lot of place. NAND is cheap nowadays, but...
> 
> it was perhaps a poor decision I made early on when I was following
> too much of the reference design without understanding all the
> details. They used a 16MB area in NAND for the bootstreams so that is
> what I used as well. I worked through the calculation once and the
> 16MB wasn't all that crazy considering at the time I was wanting
> enough room to support a 1MB u-boot. When you add the blocks and
> redundant blocks for FCB/DBBT (default is to have 2 copies of these),
> then double the bootloader size (because IMX BOOT ROM supports 2
> firmware images for redundancy), and factor in 20% headroom to allow
> for bad blocks, the 16MB wasn't all that crazy. Now that I'm talking
> about using 14MB for a <64KB SPL image, its overkill for sure.
> 
> I don't want to impose a partition layout change on our existing users
> that I want to update to the SPL bootloader (with all the improved
> DDR3 calibration values) so I figure we'll put u-boot.img in the last
> 2MB of the 16MB partition for the 'bootloader' and leave the first
> 14MB for SPL to be flashed according to the IMX6 BOOT ROM.

ok, thanks for explanation

> 
>>
>>> +
>>> +#include "imx6_spl.h"                  /* common IMX6 SPL configuration */
>>>  #include "mx6_common.h"
>>>  #define CONFIG_MX6
>>>  #define CONFIG_DISPLAY_CPUINFO         /* display cpu info */
>>> @@ -242,7 +253,7 @@
>>>       "mtdparts=nor:512k(uboot),64k(env),2m(kernel),-(rootfs)"
>>>  #else
>>>  #define MTDIDS_DEFAULT    "nand0=nand"
>>> -#define MTDPARTS_DEFAULT  "mtdparts=nand:16m(uboot),1m(env),-(rootfs)"
>>> +#define MTDPARTS_DEFAULT  "mtdparts=nand:14m(spl),2m(uboot),1m(env),-(rootfs)"
> 
> and I'll be reverting that last change as well and leaving the
> original 16M partition for the 'bootloader' meaning the 14MB area
> flashed by kobs-ng according to the IMX6 BOOT ROM requirements for
> NAND boot as well as the 2MB area I'm reserving for u-boot.img.
> 
> If I were to split the partitions like the above change, it will cause
> some grief for existing users that are using the 3.0.35 (non
> device-tree) vendor kernel that has the mtd partitions hard coded in
> the board support. Instead, I decided to patch the kobs-ng application
> used to flash the SPL so that it can be passed a max size to use
> within /dev/mtd0 and users that need to upgrade to the SPL bootloader
> will need to use that patched kobs-ng to flash the SPL to the first
> 14MB, then use std mtd utils to flash u-boot.img in the area between
> 14M and 16M.
> 
> Don't forget, at some point soon I hope to add some functionality to
> u-boot to flash the SPL portion to nand (the way kobs-ng does) so that
> you don't need to boot to linux and use kobs-ng or use our JTAG tool.

This will be a great improvement ! Thanks for working on this issue.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================