From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Eddy_Petri=C8=99or?= Date: Thu, 21 Apr 2016 18:03:26 +0200 Subject: [U-Boot] [PATCH 2/2] armv8: s32v234: Introduce basic support for s32v234evb In-Reply-To: <20160419165352.GD13577@bill-the-cat> References: <1459642206-20101-1-git-send-email-eddy.petrisor@gmail.com> <1459642206-20101-2-git-send-email-eddy.petrisor@gmail.com> <20160419165352.GD13577@bill-the-cat> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Pe 19 apr. 2016 6:53 p.m., "Tom Rini" a scris: > > 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@freescale.com> > > Original-signed-off-by: Mihaela Martinas > > Original-signed-off-by: Eddy Petri?or > > > > Signed-off-by: Eddy Petri?or > > Interesting, thanks for the contribution. I am trying to make our vendor branch less divergent from mainline, so most of the todo-s and '#if 0'-s are due to squashing of existing old commits from our vendor repository, sorry for the unclean code due to this. > 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. What other way of defining these should we use? Should we simply put this kind of defines in a board or SoC specific header? > > [snip] > > +++ b/arch/arm/cpu/armv8/s32v234/generic.c > [snip] > > +#ifdef CONFIG_FSL_ESDHC > > +DECLARE_GLOBAL_DATA_PTR; > > +#endif > > No need to guard this. OK, will remove the guard, maybe it was a temporary workaround until the SD support was ready and I failed to pick the clean up, too. > > [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.. Will do. > > +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/ OK, I'll look for a matching command it remove it for now. It was only for debug purposes added at this stage of the support. > > > +#ifdef CONFIG_FEC_MXC > > +void imx_get_mac_from_fuse(int dev_id, unsigned char *mac) > > +{ > > +#if 0 /* b46902 */ > > + struct ocotp_regs *ocotp = (struct [...] > > + mac[5] = value; > > +#endif > > +} > > +#endif > > If the FEC stuff doesn't work yet, lets just leave it out. OK. > > > +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? The code was added at a later point, so for rebasing and synchronisation reasons I prefer to insert it in later patches, too. I'll try to see if I can easily cherry pick the code, but I doubt it, since there was some major refactoring of the code in our vendor branch when support for other boards was added. If I can't, I'll put a message there so it says the feature is not yet supported. Is that OK, or should I simply leave it empty? > > > 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. Sorry, I missed this, I'll make a script to detect all remaining non-SPDX copyright headers. > > [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/ ? Actually, depending on the board we might use ddr2 or ddr3. I don't know how I should refer the armv7 files from this armv8 config. Are there any examples in the code where I could see am example? Also I'll have to check if i.mx6 code matches what we have on s32v234. > > +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. Will remove them. > > 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! I am thinking we need to represent (also) the fact we have 64 bit core, but 32 bit peripherals. > -- > Tom Eddy