[U-Boot] [PATCH 2/2] OMAP3: Add SPL support to Beagleboard

[Igor Grinberg]

On 10/27/2011 11:29 PM, Tom Rini wrote:
> On 10/27/2011 02:18 PM, Igor Grinberg wrote:
>> On 10/26/2011 11:13 PM, Tom Rini wrote:
>>> This introduces 200MHz Micron parts timing information based on x-loader
>>> and re-organizes the file slightly for grouping.  The memory init logic
>>> is also based on what x-loader does in these cases.  Note that while
>>> previously u-boot would be flashed in with SW ECC in this case it now
>>> must be flashed with HW ECC.
>> You have two spaces between the sentences, why is that?
> Old habit.
>
>>> Beagleboard rev C5, xM rev A:
>>> Tested-by: Tom Rini <trini at ti.com>
>>> Beagleboard xM rev C:
>>> Tested-by: Matt Ranostay <mranostay at gmail.com>
>>> Beagleboard rev B7, C2, xM rev B:
>>> Tested-by: Matt Porter <mporter at ti.com>
>>> Signed-off-by: Tom Rini <trini at ti.com>
>>> ---
>>>  arch/arm/include/asm/arch-omap3/mem.h |   24 +++++
>>>  board/ti/beagle/beagle.c              |  160 ++++++++++++++++++++++++++++++++-
>>>  board/ti/beagle/config.mk             |   33 -------
>>>  include/configs/omap3_beagle.h        |   60 ++++++++++++-
>>>  4 files changed, 242 insertions(+), 35 deletions(-)
>>>  delete mode 100644 board/ti/beagle/config.mk
>> config.mk removal does not belong to that patch...
>> It should be a separate one, say cleanup patch.
> I'll see if I can restructure things and kill that off first then.
>
>>> diff --git a/arch/arm/include/asm/arch-omap3/mem.h b/arch/arm/include/asm/arch-omap3/mem.h
>>> index af3504c..a784813 100644
>>> --- a/arch/arm/include/asm/arch-omap3/mem.h
>>> +++ b/arch/arm/include/asm/arch-omap3/mem.h
>>> @@ -171,6 +171,30 @@ enum {
>>>  #define MICRON_V_MR ((MICRON_WBST << 9) | (MICRON_CASL << 4) | \
>>>  	(MICRON_SIL << 3) | (MICRON_BL))
>>>  
>>> +
>>> +/* Micron part (200MHz optimized) 5 ns
>>> +  */
>>> +#define MICRON_TDAL_200   6
>>> +#define MICRON_TDPL_200   3
>>> +#define MICRON_TRRD_200   2
>>> +#define MICRON_TRCD_200   3
>>> +#define MICRON_TRP_200    3
>>> +#define MICRON_TRAS_200   8
>>> +#define MICRON_TRC_200   11
>>> +#define MICRON_TRFC_200  15
>>> +#define MICRON_V_ACTIMA_200 ((MICRON_TRFC_200 << 27) | (MICRON_TRC_200 << 22) | (MICRON_TRAS_200 << 18) \
>>> +		| (MICRON_TRP_200 << 15) | (MICRON_TRCD_200 << 12) |(MICRON_TRRD_200 << 9) | \
>>> +		(MICRON_TDPL_200 << 6) | (MICRON_TDAL_200))
>> MICRON_TDAL_200 does not need parenthesis.
> As I said to Sanjeev this is all yanked right from X-loader so yes,
> there's spacing and so on problems (and I swore git send-email --subject
> modified subject for all patches, but I guess not).
>
> [snip]
>>> +#define WRITE_NAND_COMMAND(d, adr) \
>>> +	do {*(volatile u16 *)GPMC_NAND_COMMAND_0 = d;} while(0)
>>> +#define WRITE_NAND_ADDRESS(d, adr) \
>>> +	do {*(volatile u16 *)GPMC_NAND_ADDRESS_0 = d;} while(0)
>>> +#define READ_NAND(adr)          (*(volatile u16 *)GPMC_NAND_DATA_0)
>> This is definitely needs a cleanup...
>> Consider Sanjeev's proposal. If it will not work for some reason,
>> you need at least to use writel() readl() io accessors.
> At first pass you can't call gpmc_init() as-is this early, but I'll
> switch over to readl/writel as a start.
>
>>> +
>>> +/* nand_command: Send a flash command to the flash chip */
>>> +static void nand_command(unsigned char command)
>>> +{
>>> + 	WRITE_NAND_COMMAND(command, NAND_ADDR);
>>> +
>>> +  	if (command == NAND_CMD_RESET) {
>>> +		unsigned char ret_val;
>>> +		nand_command(NAND_CMD_STATUS);
>>> +		do {
>>> +			ret_val = READ_NAND(NAND_ADDR);/* wait till ready */
>>> +  		} while ((ret_val & 0x40) != 0x40);
>> You should be using some kind of timeout, so you will not stuck in here
>> without being noticed.
> OK.  I've been wondering if we shouldn't somehow make a
> not-tied-to-full-mtd nand_command more available since I suspect a few
> other boards will be in a similar situation, for probing early on.

That would be much better solution.

> [snip]
>>> +	*mr = MICRON_V_MR;
>>> +	switch (get_board_revision()) {
>>> +	case REVISION_C4:
>>> +		if (identify_xm_ddr() == NUMONYX_MCP) {
>>> +			*cs_cfg = 0x4;
>>> +			*mcfg = 0x04590099;
>>> +			*ctrla = NUMONYX_V_ACTIMA_165;
>>> +			*ctrlb = NUMONYX_V_ACTIMB_165;
>>> +			*rfr_ctrl = SDP_3430_SDRC_RFR_CTRL_165MHz;
>> aligning the assignments above (and below) will make it much more readable
> I'm really not a fan of spacing out assignments but I'll see what
> CodingSytle says.

Yeah, me too, but it looks like these, can benefit from it...
It is not a blocker - up to you ;)

> [snip]
>>> -#ifdef CONFIG_GENERIC_MMC
>>> +#if defined(CONFIG_GENERIC_MMC) && !defined(CONFIG_SPL_BUILD)
>> This should be another patch.
> Really?  I'm adding SPL support here and that's what introduces the need
> to not build this on just CONFIG_GENERIC_MMC.

No, not really ;) I've been disrupted and did a mistake - sorry for that.

> [snip]
>>> +#define CONFIG_SPL_BSS_START_ADDR	0x80000000
>>> +#define CONFIG_SPL_BSS_MAX_SIZE		0x80000		/* 512 KB */
>> alignment
> I'll double check but all of them but this one is fine once applied.
>
> Thanks.
>

Regards,
Igor