[U-Boot] [PATCH 1/2] mx6: Add initial support for the Hummingboard solo

[Fabio Estevam]

Hi Stefano,

On Fri, Jan 3, 2014 at 8:04 AM, Stefano Babic <sbabic at denx.de> wrote:

> Indeed - I will push only fixes for 2014.01. However, new board could
> flow before release into my -next branch.

Thanks for your review.  Ok, understood.

>> +
>> +This file contains information for the port of U-Boot to the Hummingboard.
>> +
>> +For more details about Hummingboard, please refer to:
>> +http://cubox-i.com
>
> I read the pages, but it is misleading. cubox-i.com reports information
> only about the cubox-i, and this seems another hardware. I know this is
> not a problem of the patch, but maybe it is better to refer only to the
> wiki page
> http://imx.solid-run.com/wiki/index.php?title=Carrier-One_Hardware (I
> assume that Hummingbird = Carrier-One).

Yes, hummingboard is the new name for the carrier-one board.

Maybe Rabeeh could update the website.

>> +Building U-boot for Hummingboard
>> +--------------------------------
>> +
>> +To build U-Boot for the Hummingboard Solo version:
>> +
>> +$ make hummingboard_solo_config
>> +$ make
>> +
>> +Flashing U-boot into the SD card
>> +--------------------------------
>> +
>> +- After the 'make' command completes, the generated 'u-boot.imx' binary must be
>> +flashed into the SD card:
>> +
>> +$ sudo dd if=u-boot.imx of=/dev/mmcblk0 bs=1k seek=1; sync
>
> This is the generic description valid for most iMXes. We have a better
> and detailed description in doc/README.imximage. I think we should add a
> reference to doc/README.imximage.

Will add a reference to doc/README.imximage, thanks.

>
>> +
>> +(Note - the SD card node may vary, so adjust this as needed).
>> +
>> +- Insert the micro SD card into the slot located in the bottom of the board
>> +
>> +- Connect a 3.3V USB to serial converter cable to the host PC. The MX6 UART
>> +signals are available in the 26 pin connector as shown at:
>> +http://imx.solid-run.com/wiki/index.php?title=Carrier-One_Hardware
>> +(Check for "26 pin header layout").
>
> ok, this is clear. Maybe it will help if we add a warning to use a 3.3V
> converter to access the UART's pins. It is your choice: I think that the
> HW description on the site should be improved most as the documentation
> here, but some good hints cannot disturb ;-)

Got it. Rabeeh on Cc could probably help updating the website.

>> +int dram_init(void)
>> +{
>> +     gd->ram_size = ((phys_size_t)CONFIG_DDR_MB * 1024 * 1024);
>
> On the documentation, I read that the Carrier-One HW has 2GB RAM.
> http://imx.solid-run.com/wiki/index.php?title=Carrier-One_Hardware#Detailed_hardware_specification
>
> Why is only 512MB supported ? Or the solo has only 512MB and in the
> DUAl/QUAD 2GB ?

The solo variant of humming board has only 512MB.

>> +static void setup_iomux_enet(void)
>> +{
>> +     imx_iomux_v3_setup_multiple_pads(enet_pads, ARRAY_SIZE(enet_pads));
>> +
>> +     /*
>> +      * Reset AR8035 PHY. Since it runs 25MHz reference clock, it
>> +      * requires two resets.
>
> Only for my knowledge: why do we require twice in case of 25 Mhz source
> ? Where is it described ?

Jon or Rabeeh, could you please comment?

> We have enable_fec_anatop_clock() (same function with same name !) in
> arch/arm/cpu/armv7/mx6/clock.c. The function is *identical* with the
> exception that the first two bits of pll_enet are cleared
>
>         reg &= 0xfffffffc; /* Set PLL to generate 25MHz */
>
> We can sure avoid duplication, adding a function parameter to the
> original function. It is used by now only by mx6slevk board.

Agreed.

Will Otavio's patch (http://patchwork.ozlabs.org/patch/301916/) be
applied into your master branch or only for next?

Then I could extend the enable_fec_anatop_clock() function as you
suggested and avoid the code duplication.

>> +
>> +int board_eth_init(bd_t *bis)
>> +{
>> +     int ret;
>> +     struct iomuxc_base_regs *const iomuxc_regs =
>> +                     (struct iomuxc_base_regs *)IOMUXC_BASE_ADDR;
>> +     struct anatop_regs __iomem *anatop =
>> +                     (struct anatop_regs __iomem *)ANATOP_BASE_ADDR;
>> +     s32 timeout = 100000;
>> +
>> +     enable_fec_anatop_clock();
>> +     /* set gpr1[21] */
>> +     clrsetbits_le32(&iomuxc_regs->gpr[1], 0, (1 << 21));
>
> Change these two lines using defines (defines for GPR1 are missing in
> imx-regs.h, please add them).

Will do.

>
>> +
>> +     while (timeout--) {
>
> I think board_eth_init() is called after relocation, and then tiemrs are
> available. Sure that udelay does not work here ?

Will try udelay.

>
>> +             if (readl(&anatop->pll_enet) & BM_ANADIG_PLL_ENET_LOCK)
>> +                     break;
>> +     }
>
> It is not clear to me why we need this. The setup of the clock is done
> by enable_fec_anatop_clock(), why do we need to check again here ?

Good point. Will check it.

>> +Active  arm         armv7          mx6         solidrun        hummingboard        hummingboard_solo                           hummingboard:IMX_CONFIG=board/solidrun/hummingboard/hummingboard_solo.cfg,MX6S,DDR_MB=512
>
> We need also the Maintainer's address, you or Jon.

Ok, will add Jon's email.

>> +#define MACH_TYPE_HUMMINGBOARD               4773
>> +#define CONFIG_MACH_TYPE             MACH_TYPE_HUMMINGBOARD
>
> You can drop CONFIG_MACH_TYPE if it is used only here.

Ok, will do it.

>> +#define CONFIG_FEC_XCV_TYPE          RGMII
>> +#define CONFIG_ETHPRIME                      "FEC"
>
> Really required ? I do not see a second ethernet.

Ok, will remove CONFIG_ETHPRIME.

Regards,

Fabio Estevam