From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vitaly Andrianov Date: Mon, 10 Feb 2014 20:44:01 -0500 Subject: [U-Boot] [U-Boot:RESEND][[PATCH 6/7] k2hk: add support for k2hk SOC and EVM In-Reply-To: <20140210212539.GA7049@bill-the-cat> References: <1391815393-19536-1-git-send-email-m-karicheri2@ti.com> <1391815393-19536-7-git-send-email-m-karicheri2@ti.com> <20140210212539.GA7049@bill-the-cat> Message-ID: <52F98061.5090902@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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, >> +# >> +# 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, >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include >> +#include >> +#include >> + >> +#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, >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +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", >> + "
\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", >> + "\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, >> +# 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