From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Thu, 21 Apr 2016 12:28:21 -0400 Subject: [U-Boot] [PATCH 2/2] armv8: s32v234: Introduce basic support for s32v234evb In-Reply-To: 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: <20160421162821.GQ3732@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, Apr 21, 2016 at 06:03:26PM +0200, Eddy Petri?or wrote: > 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 at 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. Good to know, thanks. > > 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? Yes. some SoC header (since this is unlikely to be board specific). [snip] > > > +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. It's also possible to just drop debug things like that :) [snip] > > > +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? I'm OK with a message as a last resort. [snip] > > [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. So, if we can (and I suspect we can), we would move the armv7 code out of arch/arm/cpu/armv7/ and into drivers/dd/imx/ and that would make access easy for both :) It will require some research on your end but I strongly suspect the IP block was updated and re-used so there should be a great deal of overlap. [snip] > > > 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. Right, that's what I was trying to convey. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: