Re: [PATCH 2/2] arm: add support to corstone1000 platform

[Rui Miguel Silva <rui.silva@linaro.org>]

Hi Tom,
Many thanks for the review.

On Wed, Mar 23, 2022 at 09:21:42AM -0400, Tom Rini wrote:
> On Tue, Mar 22, 2022 at 10:41:18AM +0000, Rui Miguel Silva wrote:
> 
> > Corstone1000 is a platform from arm, which includes pre
> > verified Corstone SSE710 sub-system that combines Cortex-A and
> > Cortex-M processors [0].
> > 
> > This code adds the support for the Cortex-A35 implementation
> > at host side, it contains also the necessary bits to support
> > the Corstone 1000 FVP (Fixed Virtual Platform) [1] and also the
> > FPGA MPS3 board implementation of this platform. [2]
> > 
> > 0: https://documentation-service.arm.com/static/619e02b1f45f0b1fbf3a8f16
> > 1: https://developer.arm.com/tools-and-software/open-source-software/arm-platforms-software/arm-ecosystem-fvps
> > 2: https://documentation-service.arm.com/static/61f3f4d7fa8173727a1b71bf
> > 
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> > ---
> >  arch/arm/Kconfig                         |   8 ++
> >  arch/arm/dts/Makefile                    |   3 +
> >  arch/arm/dts/corstone1000-fvp.dts        |  33 +++++
> >  arch/arm/dts/corstone1000-mps3.dts       |  41 ++++++
> >  arch/arm/dts/corstone1000.dtsi           | 167 +++++++++++++++++++++++
> 
> What is the status of these dts files with upstream Linux?  They need to
> be in linux-next at least before we take them.

Hrr, did not know that one. I will do that and spin a v2 when these
device trees get in linux-next. Also will address all yours other
comments in this series.

Cheers,
     Rui

> 
> [snip]
> > @@ -2230,6 +2236,8 @@ source "arch/arm/mach-nexell/Kconfig"
> >  
> >  source "board/armltd/total_compute/Kconfig"
> >  
> > +source "board/armltd/corstone1000/Kconfig"
> > +
> >  source "board/bosch/shc/Kconfig"
> >  source "board/bosch/guardian/Kconfig"
> >  source "board/Marvell/octeontx/Kconfig"
> 
> There shouldn't be a space today where there is, so this entry can just
> fill in that gap.
> 
> [snip]
> > +/*
> > + * Board specific ethernet initialization routine.
> > + */
> > +int board_eth_init(struct bd_info *bis)
> > +{
> > +	int rc = 0;
> > +
> > +#ifndef CONFIG_DM_ETH
> > +#ifdef CONFIG_SMC91111
> > +	rc = smc91111_initialize(0, CONFIG_SMC91111_BASE);
> > +#endif
> > +#ifdef CONFIG_SMC911X
> > +	rc = smc911x_initialize(0, CONFIG_SMC911X_BASE);
> > +#endif
> > +#endif
> > +
> > +	return rc;
> > +}
> 
> DM_ETH should always be set, so please clean this up.
> 
> [snip]
> > +#define V2M_SRAM0		0x02000000
> > +#define V2M_QSPI		0x08000000
> > +
> > +#define V2M_DEBUG		0x10000000
> > +#define V2M_BASE_PERIPH		0x1A000000
> > +
> > +#define V2M_BASE		0x80000000
> > +
> > +#define V2M_PERIPH_OFFSET(x)	((x) << 16)
> > +
> > +#define V2M_SYSID		(V2M_BASE_PERIPH)
> > +#define V2M_SYSCTL		(V2M_BASE_PERIPH + V2M_PERIPH_OFFSET(1))
> > +
> > +#define V2M_COUNTER_CTL		(V2M_BASE_PERIPH + V2M_PERIPH_OFFSET(32))
> > +#define V2M_COUNTER_READ	(V2M_BASE_PERIPH + V2M_PERIPH_OFFSET(33))
> > +
> > +#define V2M_TIMER_CTL		(V2M_BASE_PERIPH + V2M_PERIPH_OFFSET(34))
> > +#define V2M_TIMER_BASE0		(V2M_BASE_PERIPH + V2M_PERIPH_OFFSET(35))
> > +
> > +#define V2M_UART0		(V2M_BASE_PERIPH + V2M_PERIPH_OFFSET(81))
> > +#define V2M_UART1		(V2M_BASE_PERIPH + V2M_PERIPH_OFFSET(82))
> 
> Please find someplace better than the board config.h file for these
> values.  Either some SoC header file, or pulled out of the dt.  And
> you're probably not the first board to do this, but it does need
> cleaning up.
> 
> > +/*
> > + * config_distro_bootcmd define the boot command to distro_bootcmd, but we here
> > + * want to first try to load a kernel if exists, override that config then
> > + */
> > +#undef CONFIG_BOOTCOMMAND
> > +
> > +#define CONFIG_BOOTCOMMAND								\
> > +				"run retrieve_kernel_load_addr;"			\
> > +				"echo Loading kernel from $kernel_addr to memory ... ;"	\
> > +				"loadm $kernel_addr $kernel_addr_r 0xc00000;"		\
> > +				"usb start; usb reset;"					\
> > +				"run distro_bootcmd;"					\
> > +				"bootefi $kernel_addr_r $fdtcontroladdr;"
> 
> CONFIG_BOOTCOMMMAND is part of Kconfig, so this needs to be set in the
> defconfig.  I didn't see any other options that need to be migrated,
> but, CI will fail (at least, -next has the test fixed) over migrated
> options being in the board config.h file.
> 
> -- 
> Tom