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
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
