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.elkhl...@arm.com>
> > Signed-off-by: Rui Miguel Silva <rui.si...@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
