[U-Boot] [PATCH 2/2] armv8: s32v234: Introduce basic support for s32v234evb

[Tom Rini]

On Sun, Apr 03, 2016 at 03:10:06AM +0300, Eddy Petrișor wrote:

> Add initial support for NXP's S32V234 SoC and S32V234EVB board.
> 
> The S32V230 family is designed to support computation-intensive applications
> for image processing. The S32V234, as part of the S32V230 family, is a
> high-performance automotive processor designed to support safe
> computation-intensive applications in the area of vision and sensor fusion.
> 
> Code originally writen by:
> Original-signed-off-by: Stoica Cosmin-Stefan <cosminstefan.stoica at freescale.com>
> Original-signed-off-by: Mihaela Martinas <Mihaela.Martinas at freescale.com>
> Original-signed-off-by: Eddy Petrișor <eddy.petrisor at gmail.com>
> 
> Signed-off-by: Eddy Petrișor <eddy.petrisor at gmail.com>

Interesting, thanks for the contribution.  Some comments:

[snip]
> +#ifndef CONFIG_SYS_DCACHE_OFF
> +
> +#define CONFIG_SYS_FSL_IRAM_BASE        0x3e800000UL
> +#define CONFIG_SYS_FSL_IRAM_SIZE        0x800000UL
> +#define CONFIG_SYS_FSL_DRAM_BASE1       0x80000000UL
> +#define CONFIG_SYS_FSL_DRAM_SIZE1       0x40000000UL
> +#define CONFIG_SYS_FSL_DRAM_BASE2       0xC0000000UL
> +#define CONFIG_SYS_FSL_DRAM_SIZE2       0x20000000UL
> +#define CONFIG_SYS_FSL_PERIPH_BASE      0x40000000UL
> +#define CONFIG_SYS_FSL_PERIPH_SIZE      0x40000000UL

We shouldn't use CONFIG_SYS here as it's not a config option.

[snip]
> +++ b/arch/arm/cpu/armv8/s32v234/generic.c
[snip]
> +#ifdef CONFIG_FSL_ESDHC
> +DECLARE_GLOBAL_DATA_PTR;
> +#endif

No need to guard this.

[snip]
> +/* Dump some core clocks */
> +int do_s32v234_showclocks(cmd_tbl_t * cmdtp, int flag, int argc,
> +			  char *const argv[])
> +{
> +#if 0				/* Disable until the clock code will updated for S32V234 */

We should probably remove this then..

> +U_BOOT_CMD(clocks, CONFIG_SYS_MAXARGS, 1, do_s32v234_showclocks,
> +	   "display clocks", "");

And we're trying to not have commands in places other than cmd/

> +#ifdef CONFIG_FEC_MXC
> +void imx_get_mac_from_fuse(int dev_id, unsigned char *mac)
> +{
> +#if 0				/* b46902 */
> +	struct ocotp_regs *ocotp = (struct ocotp_regs *)OCOTP_BASE_ADDR;
> +	struct fuse_bank *bank = &ocotp->bank[4];
> +	struct fuse_bank4_regs *fuse =
> +	    (struct fuse_bank4_regs *)bank->fuse_regs;
> +
> +	u32 value = readl(&fuse->mac_addr0);
> +	mac[0] = (value >> 8);
> +	mac[1] = value;
> +
> +	value = readl(&fuse->mac_addr1);
> +	mac[2] = value >> 24;
> +	mac[3] = value >> 16;
> +	mac[4] = value >> 8;
> +	mac[5] = value;
> +#endif
> +}
> +#endif

If the FEC stuff doesn't work yet, lets just leave it out.

> +void reset_cpu(ulong addr)
> +{
> +#if 0				/* b46902 */
> +	struct src *src_regs = (struct src *)SRC_BASE_ADDR;
> +
> +	/* Generate a SW reset from SRC SCR register */
> +	writel(SRC_SCR_SW_RST, &src_regs->scr);
> +
> +	/* If we get there, we are not in good shape */
> +	mdelay(1000);
> +	printf("FATAL: Reset Failed!\n");
> +	hang();
> +#endif
> +};

Here and elsewhere, we should drop if 0'd code.  Can we not really do a
reset?

> diff --git a/arch/arm/include/asm/arch-s32v234/mc_me_regs.h b/arch/arm/include/asm/arch-s32v234/mc_me_regs.h
> new file mode 100644
> index 0000000..4313140
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-s32v234/mc_me_regs.h
> @@ -0,0 +1,212 @@
> +/*
> + * Copyright 2015 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.

Here and anywhere else that was missed, please use SPDX tags.

[snip]
> diff --git a/board/freescale/s32v234evb/lpddr2.c b/board/freescale/s32v234evb/lpddr2.c
> new file mode 100644
> index 0000000..0bd5183
> --- /dev/null
> +++ b/board/freescale/s32v234evb/lpddr2.c

This file, and a few other things too possibly, feel more like SoC
specific things rather than board specific things.  Further, especially
for the DDR parts, can we leverage arch/arm/cpu/armv7/mx6/ddr.c perhaps
here? And move it into drivers/ddr/imx/ ?

> +static void setup_iomux_enet(void)
> +{
> +	/* TODO: Implement enet iomux when it is activated. */
> +}
> +
> +static void setup_iomux_i2c(void)
> +{
> +	/* TODO: Implement i2c iomux when it is activated. */
> +}
> +
> +#ifdef CONFIG_SYS_USE_NAND
> +void setup_iomux_nfc(void)
> +{
> +	/*TODO: Implement nfc iomux when it is activated. */
> +}
> +#endif

We should leave out these TODO bits for now.

> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index ea5f4bf..d564022 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -182,7 +182,7 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
>  	int timeout;
>  	struct fsl_esdhc_cfg *cfg = mmc->priv;
>  	struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;
> -#ifdef CONFIG_FSL_LAYERSCAPE
> +#if defined(CONFIG_FSL_LAYERSCAPE) || defined(CONFIG_S32V234)
>  	dma_addr_t addr;
>  #endif
>  	uint wml_value;

Maybe we need to come up with a flag that says 64bit CPU and use that
here?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160419/6ebbbed3/attachment.sig>