[U-Boot] [PATCH v2] ARMV7: Add support For Logic OMAP35x/DM37x modules

[Peter Barada <peter.barada@logicpd.com>]

On 12/15/2011 01:30 PM, Tom Rini wrote:
> On Thu, Dec 15, 2011 at 10:15 AM, Peter Barada <peter.barada@logicpd.com> wrote:
>> This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo
>> reference boards. It assumes U-boot is loaded to SDRAM with the
>> help of another small bootloader (x-load) running from SRAM.
> #if 0'd code isn't allowed for merging so I assume this is an RFC :)
My bad - sent the wrong patch.  I'll update and send again.
>> +               /* Let it soak for a bit */
>> +               for (i = 0; i < 0x100; ++i)
>> +                       asm("nop");
> Can we just use a sdelay(...) here?  And in the whole function you
> haven't addressed Igor's comment (unless it's incoming and I just
> haven't gotten the email yet) about this just being checkboard().
Done - "sdelay(0x100)" instead of the delay loop.

As for the function, in arch/arm/cpu/armv7/omap3/board.c there's already
a checkboard() function that is called early (before relocation) that
prints out the banner.  I tried to make that a weak alias and override
it in my board file, but when its called, gd->bd is not setup so that
code aborts when it tries to set gd->bd->bi_arch_number.  I then tried
storing the value in a global and then set gd->bd->bi_arch_number in
board_init(), but between the call to checkboard() and board_init() the
BSS section is zeroed.  If I stored/load the computed bi_arch_number
into a non-zero static value it works, but I feel that is really fragile.

Remember that identify_board() not only prints a banner but also
determines one of four machine IDs passed to the linux kernel to
identify which of the variants its running on.

>> +/* GPMC CS1 settings for Logic SOM LV/Torpedo LAN92xx Ethernet chip */
>> +#define LOGIC_NET_GPMC_CONFIG1  0x00001000
>> +#define LOGIC_NET_GPMC_CONFIG2  0x00080801
>> +#define LOGIC_NET_GPMC_CONFIG3  0x00000000
>> +#define LOGIC_NET_GPMC_CONFIG4  0x08010801
>> +#define LOGIC_NET_GPMC_CONFIG5  0x00080a0a
>> +#define LOGIC_NET_GPMC_CONFIG6  0x03000280
>> +#define LOGIC_NET_GPMC_CONFIG7  0x00000848
>> +
>> +/*
>> + * Routine: setup_net_chip
>> + * Description: Setting up the configuration GPMC registers specific to the
>> + *             Ethernet hardware.
>> + */
>> +static void setup_net_chip(void)
>> +{
>> +       struct ctrl *ctrl_base = (struct ctrl *)OMAP34XX_CTRL_BASE;
>> +
>> +       /* Configure GPMC registers */
>> +       writel(LOGIC_NET_GPMC_CONFIG1, &gpmc_cfg->cs[1].config1);
>> +       writel(LOGIC_NET_GPMC_CONFIG2, &gpmc_cfg->cs[1].config2);
>> +       writel(LOGIC_NET_GPMC_CONFIG3, &gpmc_cfg->cs[1].config3);
>> +       writel(LOGIC_NET_GPMC_CONFIG4, &gpmc_cfg->cs[1].config4);
>> +       writel(LOGIC_NET_GPMC_CONFIG5, &gpmc_cfg->cs[1].config5);
>> +       writel(LOGIC_NET_GPMC_CONFIG6, &gpmc_cfg->cs[1].config6);
>> +       writel(LOGIC_NET_GPMC_CONFIG7, &gpmc_cfg->cs[1].config7);
>> +
>> +       /* Enable off mode for NWE in PADCONF_GPMC_NWE register */
>> +       writew(readw(&ctrl_base->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe);
>> +       /* Enable off mode for NOE in PADCONF_GPMC_NADV_ALE register */
>> +       writew(readw(&ctrl_base->gpmc_noe) | 0x0E00, &ctrl_base->gpmc_noe);
>> +       /* Enable off mode for ALE in PADCONF_GPMC_NADV_ALE register */
>> +       writew(readw(&ctrl_base->gpmc_nadv_ale) | 0x0E00,
>> +               &ctrl_base->gpmc_nadv_ale);
>> +
>> +}
> Or this just being an enable_gpmc_cs_config call.
Done.
>> +int misc_init_r(void)
>> +{
>> +
>> +       dieid_num_r();
>> +
>> +       return 0;
>> +}
> Extra whitespace.  I think checkpatch.pl will catch this so please run
> your patch through checkpatch.pl, roughly like this:
> $ git format-patch -1
> $ ./tools/checkpatch.pl 0001-whatever-its-called.patch
I rand checkpatch.pl over the result of "git diff --cached" and it
didn't complain about the emty line after the open brace.  I've removed it.
> [snip]
>> +#define CONFIG_L2_OFF                  /* Keep L2 Cache Disabled */
> Why do we want to do this?
This is a holdover from a previous verison ofu-boot (2010.06) where I
added LCD/HDMI support for up to a 720p 32bpp display and when I did
that the kernel wouldn't start w/o disabling L2 in u-boot (possibly
because the reserved memory for the display in u-boot alone is larger
than the L2 cache in a OMAP35x/DM37x), but I never figured out why it
failed.  I've removed it and will readdress when I add in the LCD/HDMI
support.

> [snip]
>> +#define CONFIG_SYS_MAXARGS             64      /* max number of command args */
> This is very very large and probably doesn't need to be.  This is the
> number of arguments to the u-boot commands, not to the linux kernel.
You're right, yet another holdover from an older version of u-boot. 
I've reduced it 16 to match up with most of the other boards.

So the big question before I submit V3 of this patch is whether to
include overriding OMAP3's checkboard() function or to use
identify_board() as is to compute bi_arch_number...  I can do it either
way, but don't want to waste anyone's time (with the wrong method).

Thanks in advance!

-- 
Peter Barada
peter.barada at logicpd.com