[U-Boot] [U-Boot:RESEND][[PATCH 6/7] k2hk: add support for k2hk SOC and EVM

[Vitaly Andrianov]

On 02/10/2014 04:25 PM, Tom Rini wrote:
> On Fri, Feb 07, 2014 at 06:23:13PM -0500, Murali Karicheri wrote:
>
>> k2hk EVM is based on Texas Instruments Keystone2 Hawking/Kepler
>> SoC. Keystone2 SoC has ARM v7 Cortex-A15 MPCore processor. Please
>> refer the ti/k2hk_evm/README for details on the board, build and other
>> information.
>>
>> This patch add support for keystone architecture and k2hk evm.
> Globally, only use
> /*
>   * Multiline comments like this
>   */
I'll fix this.
>> +++ b/arch/arm/cpu/armv7/keystone/Makefile
>> @@ -0,0 +1,18 @@
>> +#
>> +# (C) Copyright 2012-2014
>> +#     Texas Instruments Incorporated, <www.ti.com>
>> +#
>> +# SPDX-License-Identifier:     GPL-2.0+
>> +#
>> +
>> +obj-y	+= aemif.o
>> +obj-y   += init.o
>> +obj-y   += psc.o
>> +obj-y   += clock.o
>> +obj-y   += cmd_clock.o
>> +obj-y   += cmd_mon.o
>> +obj-y   += msmc.o
>> +obj-y   += lowlevel_init.o
>> +obj-$(CONFIG_SPL_BUILD) += spl.o
>> +obj-y	+= ddr3.o
> Mix of space and tab, just tab it all out to line up please.
Will fix.

>> +++ b/arch/arm/cpu/armv7/keystone/aemif.c
>> @@ -0,0 +1,79 @@
>> +/*
>> + * Keystone2: Asynchronous EMIF Configuration
>> + *
>> + * (C) Copyright 2012-2014
>> + *     Texas Instruments Incorporated, <www.ti.com>
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <asm/io.h>
>> +#include <asm/arch/clock.h>
>> +
>> +#ifdef CONFIG_SOC_K2HK
>> +#define ASYNC_EMIF_BASE			K2HK_ASYNC_EMIF_CNTRL_BASE
>> +#endif
> So, does Keystone 1 have this in a different location?
That is not for Keystone1. K2HK is for Hawking and Kepler SoC.
The ASYNC_EMIF_BASE may be different for other SOC of the Keystone2 family.
>> +#define ASYNC_EMIF_CONFIG(cs)		(ASYNC_EMIF_BASE+0x10+(cs)*4)
>> +#define ASYNC_EMIF_ONENAND_CONTROL	(ASYNC_EMIF_BASE+0x5c)
>> +#define ASYNC_EMIF_NAND_CONTROL		(ASYNC_EMIF_BASE+0x60)
>> +#define ASYNC_EMIF_WAITCYCLE_CONFIG	(ASYNC_EMIF_BASE+0x4)
> Register access is done over structs not define of base + offset, please
> rework.
This style was taken form Davinci code. Shall we rework it?
>> +#define CONFIG_SELECT_STROBE(v)		((v) ? 1 << 31 : 0)
>> +#define CONFIG_EXTEND_WAIT(v)		((v) ? 1 << 30 : 0)
>> +#define CONFIG_WR_SETUP(v)		(((v) & 0x0f) << 26)
>> +#define CONFIG_WR_STROBE(v)		(((v) & 0x3f) << 20)
>> +#define CONFIG_WR_HOLD(v)		(((v) & 0x07) << 17)
>> +#define CONFIG_RD_SETUP(v)		(((v) & 0x0f) << 13)
>> +#define CONFIG_RD_STROBE(v)		(((v) & 0x3f) << 7)
>> +#define CONFIG_RD_HOLD(v)		(((v) & 0x07) << 4)
>> +#define CONFIG_TURN_AROUND(v)		(((v) & 0x03) << 2)
>> +#define CONFIG_WIDTH(v)			(((v) & 0x03) << 0)
> To avoid confusion please do AEMIF_CONFIG_... or AEMIF_CFG_... or maybe
> just CFG_...
Agree and will change.

>> +++ b/arch/arm/cpu/armv7/keystone/clock-k2hk.c
> [snip]
>> +			prediv = (tmp & 0x3f) + 1;
>> +			mult = (((tmp & 0x7f000) >> 6) | (pllctl_reg_read(pll,
>> +							mult) & 0x3f)) + 1;
>> +			output_div = ((pllctl_reg_read(pll, secctl) >> 19) &
>> +				      0xf) + 1;
> Please define magic numbers, check globally.
OK
>
>> +	return ret;
>> +}
>> +
>> +
>> +
>> +
>> +unsigned long clk_get_rate(unsigned int clk)
> Extra empty lines.
Will remove.

>> +++ b/arch/arm/cpu/armv7/keystone/clock.c
> [snip]
>> +static void pll_delay(unsigned int loop_count)
>> +{
>> +	while (loop_count--)
>> +		asm("   NOP");
>> +}
> If we cannot use udelay yet use sdelay.
>
>> +#include "clock-k2hk.c"
> Please use function prototypes, header files and then build the right
> SoC clock file itself.
The idea was to have common clock.h for all Keystone2 family and include 
in to
it a specific for each SoC include file. Are you asking to remove that 
specific include
from the clock.h?

>> +++ b/arch/arm/cpu/armv7/keystone/cmd_clock.c
> We just added a generic
I'll check it out.

>> @@ -0,0 +1,139 @@
>> +/*
>> + * keystone2: commands for clocks
>> + *
>> + * (C) Copyright 2012-2014
>> + *     Texas Instruments Incorporated, <www.ti.com>
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <command.h>
>> +#include <asm/arch/hardware.h>
>> +#include <asm/arch/clock.h>
>> +#include <asm/arch/psc_defs.h>
>> +
>> +static u32 atoui(char *pstr)
>> +{
>> +	u32 res = 0;
>> +
>> +	for (; *pstr != 0; pstr++) {
>> +		if (*pstr < '0' || *pstr > '9')
>> +			break;
>> +
>> +		res = (res * 10) + (*pstr - '0');
>> +	}
>> +
>> +	return res;
>> +}
>> +
>> +struct pll_init_data cmd_pll_data = {
>> +	.pll			= MAIN_PLL,
>> +	.pll_m			= 16,
>> +	.pll_d			= 1,
>> +	.pll_od			= 2,
>> +};
>> +
>> +int do_pll_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +	if (argc != 5)
>> +		goto pll_cmd_usage;
>> +
>> +	if (strncmp(argv[1], "pa", 2) == 0)
>> +		cmd_pll_data.pll = PASS_PLL;
>> +	else if (strncmp(argv[1], "arm", 3) == 0)
>> +		cmd_pll_data.pll = TETRIS_PLL;
>> +	else if (strncmp(argv[1], "ddr3a", 5) == 0)
>> +		cmd_pll_data.pll = DDR3A_PLL;
>> +	else if (strncmp(argv[1], "ddr3b", 5) == 0)
>> +		cmd_pll_data.pll = DDR3B_PLL;
>> +	else
>> +		goto pll_cmd_usage;
>> +
>> +	cmd_pll_data.pll_m   = atoui(argv[2]);
>> +	cmd_pll_data.pll_d   = atoui(argv[3]);
>> +	cmd_pll_data.pll_od  = atoui(argv[4]);
> Why not simple_strtoul(argv[n], NULL, 10) ?
>
>> +
>> +	printf("Trying to set pll %d; mult %d; div %d; OD %d\n",
>> +	       cmd_pll_data.pll, cmd_pll_data.pll_m,
>> +	       cmd_pll_data.pll_d, cmd_pll_data.pll_od);
>> +	init_pll(&cmd_pll_data);
>> +
>> +	return 0;
>> +
>> +pll_cmd_usage:
>> +	return cmd_usage(cmdtp);
>> +}
>> +
>> +U_BOOT_CMD(
>> +	pllset,	5,	0,	do_pll_cmd,
>> +	"set pll multiplier and pre divider",
>> +	"<pa|arm|ddr3a|ddr3b> <mult> <div> <OD>\n"
>> +);
> Wolfgang, if we add another command that takes decimal not hexidecimal
> inputs, you're going to hit me with a ruler, aren't you?  Even for a
> clock command?
>
>> +U_BOOT_CMD(
>> +	getclk,	2,	0,	do_getclk_cmd,
>> +	"get clock rate",
>> +	"<clk index>\n"
>> +	"See the 'enum clk_e' in the k2hk clock.h for clk indexes\n"
>> +);
> This would fit with the generic clk command and 'dump' which we just
> added.
>
>> +++ b/arch/arm/cpu/armv7/keystone/config.mk
>> @@ -0,0 +1,14 @@
>> +# (C) Copyright 2012-2014
>> +#     Texas Instruments Incorporated, <www.ti.com>
>> +# SPDX-License-Identifier:     GPL-2.0+
>> +#
>> +
>> +PLATFORM_RELFLAGS += -fno-common -ffixed-r8 -msoft-float
>> +
>> +# =========================================================================
>> +#
>> +# Supply options according to compiler version
>> +#
>> +# =========================================================================
>> +PLATFORM_RELFLAGS +=$(call cc-option,-mshort-load-bytes,\
>> +		    $(call cc-option,-malignment-traps,))
> You need to do this as:
> PF_CPPFLAGS_SLB := ...
> PLATFORM_RELFALGS += $(PF_CPPFLAGS_SLB)
>
> The way you have it causes an evaluation per file and the above is a one
> time evaluation.
>
>> diff --git a/arch/arm/cpu/armv7/keystone/ddr3.c b/arch/arm/cpu/armv7/keystone/ddr3.c
>> new file mode 100644
>> index 0000000..4875db7
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/keystone/ddr3.c
> Part of me wonders just how close/far this is from the omap-common
> EMIF/DDR code.  Some parts look pretty familiar (and makes me wonder if
> you don't have some corner-case bugs around not setting shadow registers
> and initially setting some of the values to max, then setting them
> optimally due to which bits cause a refresh cycle).
This code is based on AVV test code and is very specific to SOC DDR3 
controller,
which as I know is different from OMAP.

>> +++ b/arch/arm/cpu/armv7/keystone/lowlevel_init.S
> Is there a reason you don't use arch/arm/cpu/armv7/lowlevel_init.S and
> the s_init calling sequence?
>
>> +++ b/board/ti/k2hk_evm/board.c
> [snip]
>> +/* Byte swap the 32-bit data if the device is BE */
>> +int cpu_to_bus(u32 *ptr, u32 length)
>> +{
>> +	u32 i;
>> +
>> +	if (device_big_endian)
>> +		for (i = 0; i < length; i++, ptr++)
>> +			*ptr = __swab32(*ptr);
>> +
>> +	return 0;
>> +}
> cpu_to_be32() ?
>
>> +int board_init(void)
>> +{
>> +	gd->bd->bi_arch_number = -1;
> Just don't set it?
>
>> +void enable_caches(void)
>> +{
>> +#ifndef CONFIG_SYS_DCACHE_OFF
>> +	/* Enable D-cache. I-cache is already enabled in start.S */
>> +	dcache_enable();
>> +#endif
>> +}
> This belongs in one of the arch/arm/cpu/armv7/keystone/ files
>
>> +++ b/drivers/i2c/keystone_i2c.c
>> @@ -0,0 +1,372 @@
>> +/*
>> + * TI Keystone i2c driver
>> + * Based on TI DaVinci (TMS320DM644x) I2C driver.
> And how different is it really?  Can we not reuse the davinci driver?
>
It is reworked a little bit and also supports multiple ports, while 
davinci driver
supports only one.

I'll check out other comment as well.

Thanks,
Vitaly