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

[Tom Rini]

On Thu, Apr 21, 2016 at 06:03:26PM +0200, Eddy Petrișor wrote:
> Pe 19 apr. 2016 6:53 p.m., "Tom Rini" <trini at 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 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.
> 
> 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: <http://lists.denx.de/pipermail/u-boot/attachments/20160421/3ab6c387/attachment.sig>