[U-Boot] [PATCH] OMAP5: Add support for the SOM5_EVB board (OMAP5430-based)

[Lubomir Popov]

Hi Tom,

I'm currently busy with other work; on the other hand, careful
rebasing shall require some time, especially the Palmas stuff.
What would be the deadline for a V2 submission?

Meanwhile could you please have a look at the (already old)
http://patchwork.ozlabs.org/patch/232743/? A simple patch,
shall be needed if we enable USB (for the uEVM along with
our board). In general, what are your plans regarding USB
(.../patch/232742/)?

And again on I2C (.../patch/233823/): what is you final
opinion? I'm confident that this patch is a major improvement
for OMAP4/5 at least.

Please see some more comments inline below.

On 13/05/13 22:37, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/26/2013 11:59 AM, Lubomir Popov wrote:
>> Hi Tom,
>>
>> On 25/04/13 22:01, Tom Rini wrote:
>>> On Mon, Apr 01, 2013 at 05:06:16PM +0300, Lubomir Popov wrote:
>>>
>>>> Signed-off-by: Lubomir Popov <lpopov at mm-sol.com>
>>>
>>> Thought I had reviewed this already, sorry.
>> Thanks for your review. During the past month U-Boot has changed; I
>> have tried to follow as well (although I'm engaged with other
>> stuff) and some of your remarks have been already fixed. Please see
>> my comments inline below.
>>
>> Anyway, I guess that a V2 patch shall have to be referenced against
>> the current master. Or against u-boot-ti/next?
> 
> OK, sorry for the late reply again.  Now that u-boot-arm has resynced
> with master, I've also resynced and gotten a pull done.  You can use
> either u-boot-arm/master or u-boot-ti/master.
> 
>>
>> Please note that for this board to work with the defined
>> configuration, the following patches are also required
>> (unfortunately some are already quite old and might cause 
>> conflicts):
>>
>> - Power: http://patchwork.ozlabs.org/patch/232732/. Potential
>> conflict with Nishanth Menon's series of March 26, applied to
>> u-boot-ti/next. - For I2C support: * The patch series of Apr. 8
>> that enables I2C4 and I2C5 (applied to u-boot-ti/next; affects all
>> OMAP5 boards). * The modified i2c driver:
>> http://patchwork.ozlabs.org/patch/233823/ (useful for all OMAP3/4/5
>> boards). - For USB support: *
>> http://patchwork.ozlabs.org/patch/235684/ (affects all OMAP5
>> boards) * http://patchwork.ozlabs.org/patch/232742/ (affects all
>> OMAP5 boards)
> 
> OK.  Please make sure these still apply and if not update and re-send.
>  I think I've already grabbed a few of these.
> 
> [snip]
>>>> +#define USB_HOST_HS_CLKCTRL_MASK	0x0100D7C0	/*
>>>> CM_L3INIT_USB_HOST_HS_CLKCTRL */ +#define
>>>> USB_TLL_HS_CLKCTRL_MASK		0x00000700	/*
>>>> CM_L3INIT_USB_TLL_HS_CLKCTRL */
>>>
>>> Some header please.
>> Currently moved to board header. I wondered if a common OMAP header
>> wouldn't be more suitable, but having in mind that the utilized USB
>> ports (and thus the clocks that should be enabled) vary from board
>> to board, perhaps this (i.e. board header) is the best place.
> 
> The masks won't change tho, yes?  Common header somewhere.
These are in fact not masks, but enable bits (sort of confusing
terminology found throughout TI headers). Now renamed to
..._CLKCTRL_EN; staying in board header.
> 
>>>
>>>> +	 * TODO: Replace this ugly hardcoding with proper defines +
>>>> */ +	writel(0x0100, 0x4ae0a310);
>>>
>>> Again, do please.
>> This should be (*scrm)->auxclk0. The problem is that the
>> omap5_scrm_regs struct (holding the auxclk0 member) has to be
>> defined somewhere in the common OMAP5 headers. Sricharan? Or should
>> I hack around?
> 
> Add it to the most likely struct in the headers.
The entire struct (I call it omap5_scrm_regs in theory, similar to the
corresponding omap4_scrm_regs for OMAP4) is not defined anywhere. Of
course I could define only the member that I need, but I guess it is
a (responsible) TI job to define hardware descriptors. Or I'm wrong?
Please advise. If I have time, I could do it myself - it's some 27
registers, almost identical to the OMAP4, and should go into 
arch/arm/include/asm/arch-omap5/clocks.h.
> 
> [snip]
>>> Ah, since you do have this part already, just update the rest as
>>> I had said.
>> /* Machine type for Linux */ #ifndef MACH_TYPE_SOM5_EVB #define
>> MACH_TYPE_SOM5_EVB		4545 #endif #define CONFIG_MACH_TYPE
>> MACH_TYPE_SOM5_EVB
>>
>> Is this OK?
> 
> I think we'd decided in general to not do ifndef and just always
> define it.
> 
>>>> +/* Enable all clocks: */ +/*#define
>>>> CONFIG_SYS_CLOCKS_ENABLE_ALL*/ + +#define
>>>> CONFIG_SYS_ENABLE_PADS_ALL	/* Enable all PADS for now */
>>>
>>> Not allowed.
>> Shall see to it. What if one needs to test pins and connections
>> during board bring-up, e.g. via gpio commands?
> 
> Then code in what you need at the time, drop later.
> 
> [snip]
>>> This is a little un-clear. If MMC will be in mmc like the uEVM,
>>> just do so, and if no storage of env, leave it as NOWHERE.
>> Cerrently looks like this: /* MMC ENV related defines */ #undef
>> CONFIG_ENV_IS_IN_MMC #undef CONFIG_SYS_MMC_ENV_DEV #undef
>> CONFIG_ENV_OFFSET #define CONFIG_ENV_IS_NOWHERE
> 
> Shouldn't need that now, omap5_common is common and the env bits got
> moved to omap5_uevm.h
> 
> [snip]
>>>> +/* + * memtest setup + */ +#define CONFIG_SYS_MEMTEST_START
>>>> 0xb8000000 +#define CONFIG_SYS_MEMTEST_END
>>>> (CONFIG_SYS_MEMTEST_START + (256 << 20)) +/* Undef following
>>>> two for simple mtest */ +#define CONFIG_SYS_ALT_MEMTEST 
>>>> +#define CONFIG_SYS_MEMTEST_SCRATCH	0x81000000
>>>
>>> Please see doc/README.memory-test and update as mtest is no
>>> longer a default command.
>> Now the config tail looks like this:
>>
>> /* Undef/remove after bring-up: */ #define CONFIG_CMD_MEMTEST
>>
>> /* Disabled commands */ #undef CONFIG_CMD_SAVEENV
>>
>> /* Prompt */ #define CONFIG_SYS_PROMPT		"SOM5_EVB # "
>>
>> #ifdef CONFIG_CMD_MEMTEST /* Undef following two for simple mtest
>> */ #define CONFIG_SYS_ALT_MEMTEST #define
>> CONFIG_SYS_MEMTEST_SCRATCH	0xb7fffff0 #ifdef
>> CONFIG_SYS_ALT_MEMTEST #undef CONFIG_SYS_MEMTEST_START #define
>> CONFIG_SYS_MEMTEST_START	0xb8000000 #undef CONFIG_SYS_MEMTEST_END 
>> #define CONFIG_SYS_MEMTEST_END		(CONFIG_SYS_MEMTEST_START + (256 <<
>> 20)) #endif #endif /* CONFIG_CMD_MEMTEST */
> 
> OK.
> 
> - -- 
> Tom
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQIcBAEBAgAGBQJRkUEQAAoJENk4IS6UOR1WtscP/0RApAXEgttNem+eho1kFJMZ
> FSi3UgiO0+XqGbETTyYuG/r6RTz9grDcbtSs9np6/9H3wrc2FjCtlqXygJmQgbmr
> 2idPh4fyHerFvkZTZexJ3Lr+GC/1cJJyJliPvhj281OujbL/iDICkI/SLsnS3rtA
> hDC8MmrWWoDITtLzVNCbzkUk6gZVjFG/49dgjNMrRL9E8ctYp3NXtPK8VA4MpFnz
> WWM7qXQAHdBOuPRmixU4XuxHgG7/PAAzXH8Ac5dqFjnas+H8PEHqO90LPJm8tsFS
> 8dgb2ieQKlLl7yRLyRoQNvLy/B5EKUu+LKp6Yr3UI0oLm1iUnLdplEnKBbxpNHLS
> T+gQ7ScVCd/fFrx9oiF2tNurd6dhKTvAm8v7R9cfBAM/PjqZmoZGMAla8nvdw4XY
> g41Q1inBVn5w5/QbIzJCDZsWl9CHfoLMUHvGOKXV11NFbhjhY/9eDXwBzQmsKUXr
> 3dQYnzlCi3MxaZfsnDV9uKNSJ5sn84uBK4t9TanqGMsshen3CN5UU+bKPZKC3yty
> OQoxVTTAFnDlyJ+cQL77TmA/zkqDrL61qCrPBwZStX1f9lze1Ht7jcHwIthK3UzD
> wORqMKYFs/1DV5N7x7Un298qPGuCq9nPObl9JcCP5QWuhX6RTBn+g8ULYolQTSY0
> xWeTauEupQWcZNDUldwK
> =D5d3
> -----END PGP SIGNATURE-----
> 

Best regards,
Lubo