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

Tom Rini trini at ti.com
Mon Feb 10 22:25:39 CET 2014


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
 */

> +++ 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.

> +++ 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?

> +#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.

> +#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_...

> +++ 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.

> +	return ret;
> +}
> +
> +
> +
> +
> +unsigned long clk_get_rate(unsigned int clk)

Extra empty lines.

> +++ 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.

> +++ b/arch/arm/cpu/armv7/keystone/cmd_clock.c

We just added a generic

> @@ -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).

> +++ 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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140210/a45b7e91/attachment.pgp>


More information about the U-Boot mailing list