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

Vitaly Andrianov
2014-02-11T01:44:01Z

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