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

["Eddy Petrișor" <eddy.petrisor@gmail.com>]

Pe 19 apr. 2016 6:53 p.m., "Tom Rini" <trini@konsulko.com> 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 <Mihaela.Martinas@freescale.com
>
> > Original-signed-off-by: Eddy Petri?or <eddy.petrisor@gmail.com>
> >
> > Signed-off-by: Eddy Petri?or <eddy.petrisor@gmail.com>
>
> 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