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@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.
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.