[PATCH 0/5] VExpress64 board family improvements

[PATCH 0/5] VExpress64 board family improvements

[Peter Hoyes]

From: Peter Hoyes <Peter.Hoyes@arm.com>

These patches add the follow improvements to the VExpress64 board family
(BASE_FVP and Juno):

 * Add documentation
 * Allow use of OF_BOARD for BASE_FVP (off by default)
 * Allow use of the virtio-net driver (off by default)
 * Refactor header file to make it easier to add new FVPs with similar
 * memory layouts
 * Fix issue where fdt is called with invalid arguments during BASE_FVP
   boot
 * Update BASE_FVP env vars to recommended names

This is towards future work to add support for the
FVP_BaseR_AEMv8R.

Peter Hoyes (5):
  doc: Add documentation for the Arm VExpress64 board configs
  vexpress64: Refactor header file to make it easier to add new FVPs
  vexpress64: Clean up BASE_FVP boot configuration
  vexpress64: Enable OF_CONTROL and OF_BOARD for VExpress64
  vexpress64: Enable VIRTIO_NET network driver

 board/armltd/vexpress64/Kconfig               |   2 +-
 board/armltd/vexpress64/Makefile              |   5 +
 board/armltd/vexpress64/lowlevel_init.S       |  12 +++
 board/armltd/vexpress64/vexpress64.c          |  31 ++++++
 doc/README.semihosting                        |   2 +-
 doc/board/armltd/index.rst                    |   9 ++
 doc/board/armltd/vexpress64.rst               |  49 +++++++++
 doc/board/index.rst                           |   1 +
 .../{vexpress_aemv8a.h => vexpress_aemv8.h}   | 102 ++++++++++--------
 9 files changed, 167 insertions(+), 46 deletions(-)
 create mode 100644 board/armltd/vexpress64/lowlevel_init.S
 create mode 100644 doc/board/armltd/index.rst
 create mode 100644 doc/board/armltd/vexpress64.rst
 rename include/configs/{vexpress_aemv8a.h => vexpress_aemv8.h} (72%)

-- 
2.25.1

[PATCH 1/5] doc: Add documentation for the Arm VExpress64 board configs

[Peter Hoyes]

From: Peter Hoyes <Peter.Hoyes@arm.com>

Create a new documentation section for Arm Ltd boards with a sub-page
for the VExpress64 boards (FVP-A and Juno).

Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
---
 doc/board/armltd/index.rst      |  9 ++++++
 doc/board/armltd/vexpress64.rst | 49 +++++++++++++++++++++++++++++++++
 doc/board/index.rst             |  1 +
 3 files changed, 59 insertions(+)
 create mode 100644 doc/board/armltd/index.rst
 create mode 100644 doc/board/armltd/vexpress64.rst

diff --git a/doc/board/armltd/index.rst b/doc/board/armltd/index.rst
new file mode 100644
index 0000000000..c20d8a0a26
--- /dev/null
+++ b/doc/board/armltd/index.rst
@@ -0,0 +1,9 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Arm Ltd
+=============
+
+.. toctree::
+   :maxdepth: 2
+
+   vexpress64.rst
diff --git a/doc/board/armltd/vexpress64.rst b/doc/board/armltd/vexpress64.rst
new file mode 100644
index 0000000000..9e97b0e9cd
--- /dev/null
+++ b/doc/board/armltd/vexpress64.rst
@@ -0,0 +1,49 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Arm Versatile Express
+=====================
+
+The vexpress_* board configuration supports the following platforms:
+
+ * FVP_Base_RevC-2xAEMvA
+ * Juno development board
+
+Fixed Virtual Platforms
+-----------------------
+
+The Fixed Virtual Platforms (FVP) are complete simulations of an Arm system,
+including processor, memory and peripherals. They are set out in a "programmer's
+view", which gives a comprehensive model on which to build and test software.
+
+The supported FVPs are available free of charge and can be downloaded from the
+Arm developer site [1]_ (user registration might be required).
+
+Supported features:
+
+ * GICv3
+ * Generic timer
+ * PL011 UART
+
+The default configuration assumes that U-Boot is bootstrapped using a suitable
+bootloader.
+
+The FVPs can be debugged using Arm Development Studio [2]_.
+
+Juno
+----
+
+Juno is an Arm development board with the following features:
+
+ * Arm Cortex-A72/A57 and Arm Cortex-A53 in a "big.LITTLE" configuration
+ * A PCIe Gen2.0 bus with 4 lanes
+ * 8GB of DRAM
+ * GICv2
+
+More details can be found in the board documentation [3]_.
+
+References
+----------
+
+.. [1] https://developer.arm.com/tools-and-software/simulation-models/fixed-virtual-platforms
+.. [2] https://developer.arm.com/tools-and-software/embedded/arm-development-studio
+.. [3] https://developer.arm.com/tools-and-software/development-boards/juno-development-board
\ No newline at end of file
diff --git a/doc/board/index.rst b/doc/board/index.rst
index 74ea33e081..78b486538b 100644
--- a/doc/board/index.rst
+++ b/doc/board/index.rst
@@ -11,6 +11,7 @@ Board-specific doc
    AndesTech/index
    amlogic/index
    apple/index
+   armltd/index
    atmel/index
    congatec/index
    coreboot/index
-- 
2.25.1

[PATCH 2/5] vexpress64: Refactor header file to make it easier to add new FVPs

[Peter Hoyes]

From: Peter Hoyes <Peter.Hoyes@arm.com>

Rename from vexpress_aemv8a.h -> vepxress_aemv8.h as new FVPs may not be
v8-A. No change in behavior.

This is towards future work to enable support for the FVP_BaseR.

Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
---
 board/armltd/vexpress64/Kconfig               |  2 +-
 doc/README.semihosting                        |  2 +-
 .../{vexpress_aemv8a.h => vexpress_aemv8.h}   | 48 ++++++++++---------
 3 files changed, 27 insertions(+), 25 deletions(-)
 rename include/configs/{vexpress_aemv8a.h => vexpress_aemv8.h} (88%)

diff --git a/board/armltd/vexpress64/Kconfig b/board/armltd/vexpress64/Kconfig
index 1d13f542e6..4aab3f092e 100644
--- a/board/armltd/vexpress64/Kconfig
+++ b/board/armltd/vexpress64/Kconfig
@@ -7,7 +7,7 @@ config SYS_VENDOR
 	default "armltd"
 
 config SYS_CONFIG_NAME
-	default "vexpress_aemv8a"
+	default "vexpress_aemv8"
 
 config JUNO_DTB_PART
 	string "NOR flash partition holding DTB"
diff --git a/doc/README.semihosting b/doc/README.semihosting
index c019999bed..f382d0131e 100644
--- a/doc/README.semihosting
+++ b/doc/README.semihosting
@@ -25,7 +25,7 @@ or turning on CONFIG_BASE_FVP for the more full featured model.
 Rather than create a new armv8 board similar to armltd/vexpress64, add
 semihosting calls to the existing one, enabled with CONFIG_SEMIHOSTING
 and CONFIG_BASE_FVP both set. Also reuse the existing board config file
-vexpress_aemv8a.h but differentiate the two models by the presence or
+vexpress_aemv8.h but differentiate the two models by the presence or
 absence of CONFIG_BASE_FVP. This change is tested and works on both the
 Foundation and Base fastmodel simulators.
 
diff --git a/include/configs/vexpress_aemv8a.h b/include/configs/vexpress_aemv8.h
similarity index 88%
rename from include/configs/vexpress_aemv8a.h
rename to include/configs/vexpress_aemv8.h
index df22584d9a..49517a60b0 100644
--- a/include/configs/vexpress_aemv8a.h
+++ b/include/configs/vexpress_aemv8.h
@@ -4,36 +4,37 @@
  *   configurations.
  */
 
-#ifndef __VEXPRESS_AEMV8A_H
-#define __VEXPRESS_AEMV8A_H
+#ifndef __VEXPRESS_AEMV8_H
+#define __VEXPRESS_AEMV8_H
 
 #define CONFIG_REMAKE_ELF
 
 /* Link Definitions */
-#ifdef CONFIG_TARGET_VEXPRESS64_BASE_FVP
+#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
+#define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_SDRAM_BASE + 0x7fff0)
+#else
 /* ATF loads u-boot here for BASE_FVP model */
 #define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_SDRAM_BASE + 0x03f00000)
-#elif CONFIG_TARGET_VEXPRESS64_JUNO
-#define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_SDRAM_BASE + 0x7fff0)
 #endif
 
 #define CONFIG_SYS_BOOTM_LEN (64 << 20)      /* Increase max gunzip size */
 
 /* CS register bases for the original memory map. */
-#define V2M_PA_CS0			0x00000000
-#define V2M_PA_CS1			0x14000000
-#define V2M_PA_CS2			0x18000000
-#define V2M_PA_CS3			0x1c000000
-#define V2M_PA_CS4			0x0c000000
-#define V2M_PA_CS5			0x10000000
+#define V2M_BASE			0x80000000
+#define V2M_PA_BASE			0x00000000
+
+#define V2M_PA_CS0			(V2M_PA_BASE + 0x00000000)
+#define V2M_PA_CS1			(V2M_PA_BASE + 0x14000000)
+#define V2M_PA_CS2			(V2M_PA_BASE + 0x18000000)
+#define V2M_PA_CS3			(V2M_PA_BASE + 0x1c000000)
+#define V2M_PA_CS4			(V2M_PA_BASE + 0x0c000000)
+#define V2M_PA_CS5			(V2M_PA_BASE + 0x10000000)
 
 #define V2M_PERIPH_OFFSET(x)		(x << 16)
 #define V2M_SYSREGS			(V2M_PA_CS3 + V2M_PERIPH_OFFSET(1))
 #define V2M_SYSCTL			(V2M_PA_CS3 + V2M_PERIPH_OFFSET(2))
 #define V2M_SERIAL_BUS_PCI		(V2M_PA_CS3 + V2M_PERIPH_OFFSET(3))
 
-#define V2M_BASE			0x80000000
-
 /* Common peripherals relative to CS7. */
 #define V2M_AACI			(V2M_PA_CS3 + V2M_PERIPH_OFFSET(4))
 #define V2M_MMCI			(V2M_PA_CS3 + V2M_PERIPH_OFFSET(5))
@@ -72,23 +73,23 @@
 
 /* Generic Interrupt Controller Definitions */
 #ifdef CONFIG_GICV3
-#define GICD_BASE			(0x2f000000)
-#define GICR_BASE			(0x2f100000)
+#define GICD_BASE			(V2M_PA_BASE + 0x2f000000)
+#define GICR_BASE			(V2M_PA_BASE + 0x2f100000)
 #else
 
-#ifdef CONFIG_TARGET_VEXPRESS64_BASE_FVP
-#define GICD_BASE			(0x2f000000)
-#define GICC_BASE			(0x2c000000)
-#elif CONFIG_TARGET_VEXPRESS64_JUNO
+#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
 #define GICD_BASE			(0x2C010000)
 #define GICC_BASE			(0x2C02f000)
+#else
+#define GICD_BASE			(V2M_PA_BASE + 0x2f000000)
+#define GICC_BASE			(V2M_PA_BASE + 0x2c000000)
 #endif
 #endif /* !CONFIG_GICV3 */
 
 #ifndef CONFIG_TARGET_VEXPRESS64_JUNO
 /* The Vexpress64 simulators use SMSC91C111 */
 #define CONFIG_SMC91111			1
-#define CONFIG_SMC91111_BASE		(0x01A000000)
+#define CONFIG_SMC91111_BASE		(V2M_PA_BASE + 0x01A000000)
 #endif
 
 /* PL011 Serial Configuration */
@@ -113,7 +114,7 @@
 #ifdef CONFIG_TARGET_VEXPRESS64_JUNO
 #define PHYS_SDRAM_2			(0x880000000)
 #define PHYS_SDRAM_2_SIZE		0x180000000
-#elif CONFIG_TARGET_VEXPRESS64_BASE_FVP && CONFIG_NR_DRAM_BANKS == 2
+#elif CONFIG_NR_DRAM_BANKS == 2
 #define PHYS_SDRAM_2			(0x880000000)
 #define PHYS_SDRAM_2_SIZE		0x80000000
 #endif
@@ -200,6 +201,7 @@
 				"  booti $kernel_addr - $fdt_addr; " \
 				"fi"
 #endif
+
 #endif
 
 /* Monitor Command Prompt */
@@ -213,7 +215,7 @@
 /* Store environment at top of flash in the same location as blank.img */
 /* in the Juno firmware. */
 #else
-#define CONFIG_SYS_FLASH_BASE		0x0C000000
+#define CONFIG_SYS_FLASH_BASE		(V2M_PA_BASE + 0x0C000000)
 /* 256 x 256KiB sectors */
 #define CONFIG_SYS_MAX_FLASH_SECT	256
 /* Store environment at top of flash */
@@ -230,4 +232,4 @@
 #define CONFIG_SYS_FLASH_EMPTY_INFO	/* flinfo indicates empty blocks */
 #define FLASH_MAX_SECTOR_SIZE		0x00040000
 
-#endif /* __VEXPRESS_AEMV8A_H */
+#endif /* __VEXPRESS_AEMV8_H */
-- 
2.25.1

[PATCH 3/5] vexpress64: Clean up BASE_FVP boot configuration

[Peter Hoyes]

From: Peter Hoyes <Peter.Hoyes@arm.com>

Move env var address values to #defines so they can be reused elsewhere.

Rename env var names to those recommended in the README.

Fix issue where fdt is called with invalid arguments when booting
without a ramdisk.

Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
---
 include/configs/vexpress_aemv8.h | 50 ++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/include/configs/vexpress_aemv8.h b/include/configs/vexpress_aemv8.h
index 49517a60b0..48c21082a6 100644
--- a/include/configs/vexpress_aemv8.h
+++ b/include/configs/vexpress_aemv8.h
@@ -7,6 +7,8 @@
 #ifndef __VEXPRESS_AEMV8_H
 #define __VEXPRESS_AEMV8_H
 
+#include <linux/stringify.h>
+
 #define CONFIG_REMAKE_ELF
 
 /* Link Definitions */
@@ -172,33 +174,43 @@
 				BOOTENV
 
 #elif CONFIG_TARGET_VEXPRESS64_BASE_FVP
+
+#define VEXPRESS_FDT_ADDR	0x83000000
+#define VEXPRESS_RAMDISK_ADDR	0x88000000
+#define VEXPRESS_KERNEL_ADDR	0x80080000
+#define VEXPRESS_BOOT_ADDR	0x8007f800
+
 #define CONFIG_EXTRA_ENV_SETTINGS	\
 				"kernel_name=Image\0"		\
-				"kernel_addr=0x80080000\0"	\
-				"initrd_name=ramdisk.img\0"	\
-				"initrd_addr=0x88000000\0"	\
-				"fdtfile=devtree.dtb\0"		\
-				"fdt_addr=0x83000000\0"		\
-				"boot_name=boot.img\0"		\
-				"boot_addr=0x8007f800\0"
+				"kernel_addr_r=" __stringify(VEXPRESS_KERNEL_ADDR) "\0"	\
+				"ramdisk_name=ramdisk.img\0"	\
+				"ramdisk_addr_r=" __stringify(VEXPRESS_RAMDISK_ADDR) "\0" \
+				"fdtfile=devtree.dtb\0"	\
+				"fdt_addr_r=" __stringify(VEXPRESS_FDT_ADDR) "\0"	\
+				"boot_name=boot.img\0" \
+				"boot_addr_r=" __stringify(VEXPRESS_BOOT_ADDR) "\0"
 
 #ifndef CONFIG_BOOTCOMMAND
-#define CONFIG_BOOTCOMMAND	"if smhload ${boot_name} ${boot_addr}; then " \
+#define CONFIG_BOOTCOMMAND	"if smhload ${boot_name} ${boot_addr_r}; then " \
 				"  set bootargs; " \
-				"  abootimg addr ${boot_addr}; " \
-				"  abootimg get dtb --index=0 fdt_addr; " \
-				"  bootm ${boot_addr} ${boot_addr} " \
-				"  ${fdt_addr}; " \
+				"  abootimg addr ${boot_addr_r}; " \
+				"  abootimg get dtb --index=0 fdt_addr_r; " \
+				"  bootm ${boot_addr_r} ${boot_addr_r} " \
+				"  ${fdt_addr_r}; " \
 				"else; " \
 				"  set fdt_high 0xffffffffffffffff; " \
 				"  set initrd_high 0xffffffffffffffff; " \
-				"  smhload ${kernel_name} ${kernel_addr}; " \
-				"  smhload ${fdtfile} ${fdt_addr}; " \
-				"  smhload ${initrd_name} ${initrd_addr} "\
-				"  initrd_end; " \
-				"  fdt addr ${fdt_addr}; fdt resize; " \
-				"  fdt chosen ${initrd_addr} ${initrd_end}; " \
-				"  booti $kernel_addr - $fdt_addr; " \
+				"  smhload ${kernel_name} ${kernel_addr_r}; " \
+				"  smhload ${fdtfile} ${fdt_addr_r}; " \
+				"  smhload ${ramdisk_name} ${ramdisk_addr_r} "\
+				"  ramdisk_end; " \
+				"  fdt addr ${fdt_addr_r}; fdt resize; " \
+				"  if test -n ${ramdisk_end}; then "\
+				"    fdt chosen ${ramdisk_addr_r} ${ramdisk_end}; " \
+				"  else; " \
+				"    fdt chosen; " \
+				"  fi; " \
+				"  booti $kernel_addr_r - $fdt_addr_r; " \
 				"fi"
 #endif
 
-- 
2.25.1

[PATCH 4/5] vexpress64: Enable OF_CONTROL and OF_BOARD for VExpress64

[Peter Hoyes]

From: Peter Hoyes <Peter.Hoyes@arm.com>

Capture x0 in lowlevel_init.S as potential fdt address. Modify
board_fdt_blob_setup to use fdt address from either vexpress_aemv8.h
or lowlevel_init.S.

Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
---
 board/armltd/vexpress64/Makefile        |  5 +++++
 board/armltd/vexpress64/lowlevel_init.S | 12 ++++++++++++
 board/armltd/vexpress64/vexpress64.c    | 24 ++++++++++++++++++++++++
 3 files changed, 41 insertions(+)
 create mode 100644 board/armltd/vexpress64/lowlevel_init.S

diff --git a/board/armltd/vexpress64/Makefile b/board/armltd/vexpress64/Makefile
index 868dc4f629..5703e75967 100644
--- a/board/armltd/vexpress64/Makefile
+++ b/board/armltd/vexpress64/Makefile
@@ -5,3 +5,8 @@
 
 obj-y	:= vexpress64.o
 obj-$(CONFIG_TARGET_VEXPRESS64_JUNO)	+= pcie.o
+ifdef CONFIG_OF_BOARD
+ifndef CONFIG_TARGET_VEXPRESS64_JUNO
+obj-y += lowlevel_init.o
+endif
+endif
diff --git a/board/armltd/vexpress64/lowlevel_init.S b/board/armltd/vexpress64/lowlevel_init.S
new file mode 100644
index 0000000000..3dcfb85d0e
--- /dev/null
+++ b/board/armltd/vexpress64/lowlevel_init.S
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * (C) Copyright 2021 Arm Limited
+ */
+
+.global save_boot_params
+save_boot_params:
+
+	adr	x8, prior_stage_fdt_address
+	str	x0, [x8]
+
+	b	save_boot_params_ret
diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c
index d2f307cca5..bde6ef1ba3 100644
--- a/board/armltd/vexpress64/vexpress64.c
+++ b/board/armltd/vexpress64/vexpress64.c
@@ -86,6 +86,8 @@ int dram_init_banksize(void)
 }
 
 #ifdef CONFIG_OF_BOARD
+
+#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
 #define JUNO_FLASH_SEC_SIZE	(256 * 1024)
 static phys_addr_t find_dtb_in_nor_flash(const char *partname)
 {
@@ -130,9 +132,19 @@ static phys_addr_t find_dtb_in_nor_flash(const char *partname)
 
 	return ~0;
 }
+#else
+
+/* Assigned in lowlevel_init.S
+ * Push the variable into the .data section so that it
+ * does not get cleared later.
+ */
+unsigned long __section(".data") prior_stage_fdt_address;
+
+#endif
 
 void *board_fdt_blob_setup(int *err)
 {
+#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
 	phys_addr_t fdt_rom_addr = find_dtb_in_nor_flash(CONFIG_JUNO_DTB_PART);
 
 	*err = 0;
@@ -142,6 +154,18 @@ void *board_fdt_blob_setup(int *err)
 	}
 
 	return (void *)fdt_rom_addr;
+#else
+	if (fdt_magic(VEXPRESS_FDT_ADDR) == FDT_MAGIC) {
+		*err = 0;
+		return (void *)VEXPRESS_FDT_ADDR;
+	} else if (fdt_magic(prior_stage_fdt_address) == FDT_MAGIC) {
+		*err = 0;
+		return (void *)prior_stage_fdt_address;
+	} else {
+		*err = -ENXIO;
+		return NULL;
+	}
+#endif
 }
 #endif
 
-- 
2.25.1

[PATCH 5/5] vexpress64: Enable VIRTIO_NET network driver

[Peter Hoyes]

From: Peter Hoyes <Peter.Hoyes@arm.com>

The SMSC driver is using the old driver model.

Init the virtio system in vexpress64.c so that the network device is
discovered.

Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
---
 board/armltd/vexpress64/vexpress64.c | 7 +++++++
 include/configs/vexpress_aemv8.h     | 4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c
index bde6ef1ba3..d2e2404849 100644
--- a/board/armltd/vexpress64/vexpress64.c
+++ b/board/armltd/vexpress64/vexpress64.c
@@ -18,6 +18,10 @@
 #include <dm/platform_data/serial_pl01x.h>
 #include "pcie.h"
 #include <asm/armv8/mmu.h>
+#ifdef CONFIG_VIRTIO_NET
+#include <virtio_types.h>
+#include <virtio.h>
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -64,6 +68,9 @@ __weak void vexpress64_pcie_init(void)
 int board_init(void)
 {
 	vexpress64_pcie_init();
+#ifdef CONFIG_VIRTIO_NET
+	virtio_init();
+#endif
 	return 0;
 }
 
diff --git a/include/configs/vexpress_aemv8.h b/include/configs/vexpress_aemv8.h
index 48c21082a6..af160a9a66 100644
--- a/include/configs/vexpress_aemv8.h
+++ b/include/configs/vexpress_aemv8.h
@@ -88,8 +88,8 @@
 #endif
 #endif /* !CONFIG_GICV3 */
 
-#ifndef CONFIG_TARGET_VEXPRESS64_JUNO
-/* The Vexpress64 simulators use SMSC91C111 */
+#if defined(CONFIG_TARGET_VEXPRESS64_BASE_FVP) && !defined(CONFIG_DM_ETH)
+/* The Vexpress64 BASE_FVP simulator uses SMSC91C111 */
 #define CONFIG_SMC91111			1
 #define CONFIG_SMC91111_BASE		(V2M_PA_BASE + 0x01A000000)
 #endif
-- 
2.25.1

Re: [PATCH 1/5] doc: Add documentation for the Arm VExpress64 board configs

[Andre Przywara]

On Thu,  4 Nov 2021 16:56:15 +0000
Peter Hoyes <peter.hoyes@arm.com> wrote:

> From: Peter Hoyes <Peter.Hoyes@arm.com>
> 
> Create a new documentation section for Arm Ltd boards with a sub-page
> for the VExpress64 boards (FVP-A and Juno).

Many thanks for providing this!

> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> ---
>  doc/board/armltd/index.rst      |  9 ++++++
>  doc/board/armltd/vexpress64.rst | 49 +++++++++++++++++++++++++++++++++
>  doc/board/index.rst             |  1 +
>  3 files changed, 59 insertions(+)
>  create mode 100644 doc/board/armltd/index.rst
>  create mode 100644 doc/board/armltd/vexpress64.rst
> 
> diff --git a/doc/board/armltd/index.rst b/doc/board/armltd/index.rst
> new file mode 100644
> index 0000000000..c20d8a0a26
> --- /dev/null
> +++ b/doc/board/armltd/index.rst
> @@ -0,0 +1,9 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Arm Ltd
> +=============
> +
> +.. toctree::
> +   :maxdepth: 2
> +
> +   vexpress64.rst
> diff --git a/doc/board/armltd/vexpress64.rst b/doc/board/armltd/vexpress64.rst
> new file mode 100644
> index 0000000000..9e97b0e9cd
> --- /dev/null
> +++ b/doc/board/armltd/vexpress64.rst
> @@ -0,0 +1,49 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Arm Versatile Express
> +=====================
> +
> +The vexpress_* board configuration supports the following platforms:
> +
> + * FVP_Base_RevC-2xAEMvA
> + * Juno development board
> +
> +Fixed Virtual Platforms
> +-----------------------
> +
> +The Fixed Virtual Platforms (FVP) are complete simulations of an Arm system,
> +including processor, memory and peripherals. They are set out in a "programmer's
> +view", which gives a comprehensive model on which to build and test software.
> +
> +The supported FVPs are available free of charge and can be downloaded from the
> +Arm developer site [1]_ (user registration might be required).
> +
> +Supported features:
> +
> + * GICv3
> + * Generic timer
> + * PL011 UART
> +
> +The default configuration assumes that U-Boot is bootstrapped using a suitable
> +bootloader.

Can you give an example here, to give people some direction? For instance
that it can be included as BL33 in a FIT image created by a TF-A build.
Ideally with some example build command line?

Cheers,
Andre

> +
> +The FVPs can be debugged using Arm Development Studio [2]_.
> +
> +Juno
> +----
> +
> +Juno is an Arm development board with the following features:
> +
> + * Arm Cortex-A72/A57 and Arm Cortex-A53 in a "big.LITTLE" configuration
> + * A PCIe Gen2.0 bus with 4 lanes
> + * 8GB of DRAM
> + * GICv2
> +
> +More details can be found in the board documentation [3]_.
> +
> +References
> +----------
> +
> +.. [1] https://developer.arm.com/tools-and-software/simulation-models/fixed-virtual-platforms
> +.. [2] https://developer.arm.com/tools-and-software/embedded/arm-development-studio
> +.. [3] https://developer.arm.com/tools-and-software/development-boards/juno-development-board
> \ No newline at end of file
> diff --git a/doc/board/index.rst b/doc/board/index.rst
> index 74ea33e081..78b486538b 100644
> --- a/doc/board/index.rst
> +++ b/doc/board/index.rst
> @@ -11,6 +11,7 @@ Board-specific doc
>     AndesTech/index
>     amlogic/index
>     apple/index
> +   armltd/index
>     atmel/index
>     congatec/index
>     coreboot/index

Re: [PATCH 2/5] vexpress64: Refactor header file to make it easier to add new FVPs

[Andre Przywara]

On Thu,  4 Nov 2021 16:56:16 +0000
Peter Hoyes <peter.hoyes@arm.com> wrote:

> From: Peter Hoyes <Peter.Hoyes@arm.com>
> 
> Rename from vexpress_aemv8a.h -> vepxress_aemv8.h as new FVPs may not be
> v8-A. No change in behavior.

That looks like a reasonable cleanup to me, mostly just changing "is it
the FVP, or Juno?" into "is it the Juno, or something else?".
And even though this only really makes sense with the (upcoming) v8-R
support, I don't see any issues with having this patch already.

> This is towards future work to enable support for the FVP_BaseR.
> 
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  board/armltd/vexpress64/Kconfig               |  2 +-
>  doc/README.semihosting                        |  2 +-
>  .../{vexpress_aemv8a.h => vexpress_aemv8.h}   | 48 ++++++++++---------
>  3 files changed, 27 insertions(+), 25 deletions(-)
>  rename include/configs/{vexpress_aemv8a.h => vexpress_aemv8.h} (88%)
> 
> diff --git a/board/armltd/vexpress64/Kconfig
> b/board/armltd/vexpress64/Kconfig index 1d13f542e6..4aab3f092e 100644
> --- a/board/armltd/vexpress64/Kconfig
> +++ b/board/armltd/vexpress64/Kconfig
> @@ -7,7 +7,7 @@ config SYS_VENDOR
>  	default "armltd"
>  
>  config SYS_CONFIG_NAME
> -	default "vexpress_aemv8a"
> +	default "vexpress_aemv8"
>  
>  config JUNO_DTB_PART
>  	string "NOR flash partition holding DTB"
> diff --git a/doc/README.semihosting b/doc/README.semihosting
> index c019999bed..f382d0131e 100644
> --- a/doc/README.semihosting
> +++ b/doc/README.semihosting
> @@ -25,7 +25,7 @@ or turning on CONFIG_BASE_FVP for the more full
> featured model. Rather than create a new armv8 board similar to
> armltd/vexpress64, add semihosting calls to the existing one, enabled
> with CONFIG_SEMIHOSTING and CONFIG_BASE_FVP both set. Also reuse the
> existing board config file -vexpress_aemv8a.h but differentiate the two
> models by the presence or +vexpress_aemv8.h but differentiate the two
> models by the presence or absence of CONFIG_BASE_FVP. This change is
> tested and works on both the Foundation and Base fastmodel simulators.
>  
> diff --git a/include/configs/vexpress_aemv8a.h
> b/include/configs/vexpress_aemv8.h similarity index 88%
> rename from include/configs/vexpress_aemv8a.h
> rename to include/configs/vexpress_aemv8.h
> index df22584d9a..49517a60b0 100644
> --- a/include/configs/vexpress_aemv8a.h
> +++ b/include/configs/vexpress_aemv8.h
> @@ -4,36 +4,37 @@
>   *   configurations.
>   */
>  
> -#ifndef __VEXPRESS_AEMV8A_H
> -#define __VEXPRESS_AEMV8A_H
> +#ifndef __VEXPRESS_AEMV8_H
> +#define __VEXPRESS_AEMV8_H
>  
>  #define CONFIG_REMAKE_ELF
>  
>  /* Link Definitions */
> -#ifdef CONFIG_TARGET_VEXPRESS64_BASE_FVP
> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
> +#define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_SDRAM_BASE +
> 0x7fff0) +#else
>  /* ATF loads u-boot here for BASE_FVP model */
>  #define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_SDRAM_BASE +
> 0x03f00000) -#elif CONFIG_TARGET_VEXPRESS64_JUNO
> -#define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_SDRAM_BASE +
> 0x7fff0) #endif
>  
>  #define CONFIG_SYS_BOOTM_LEN (64 << 20)      /* Increase max gunzip
> size */ 
>  /* CS register bases for the original memory map. */
> -#define V2M_PA_CS0			0x00000000
> -#define V2M_PA_CS1			0x14000000
> -#define V2M_PA_CS2			0x18000000
> -#define V2M_PA_CS3			0x1c000000
> -#define V2M_PA_CS4			0x0c000000
> -#define V2M_PA_CS5			0x10000000
> +#define V2M_BASE			0x80000000
> +#define V2M_PA_BASE			0x00000000
> +
> +#define V2M_PA_CS0			(V2M_PA_BASE + 0x00000000)
> +#define V2M_PA_CS1			(V2M_PA_BASE + 0x14000000)
> +#define V2M_PA_CS2			(V2M_PA_BASE + 0x18000000)
> +#define V2M_PA_CS3			(V2M_PA_BASE + 0x1c000000)
> +#define V2M_PA_CS4			(V2M_PA_BASE + 0x0c000000)
> +#define V2M_PA_CS5			(V2M_PA_BASE + 0x10000000)
>  
>  #define V2M_PERIPH_OFFSET(x)		(x << 16)
>  #define V2M_SYSREGS			(V2M_PA_CS3 +
> V2M_PERIPH_OFFSET(1)) #define V2M_SYSCTL
> (V2M_PA_CS3 + V2M_PERIPH_OFFSET(2)) #define V2M_SERIAL_BUS_PCI
> 	(V2M_PA_CS3 + V2M_PERIPH_OFFSET(3)) 
> -#define V2M_BASE			0x80000000
> -
>  /* Common peripherals relative to CS7. */
>  #define V2M_AACI			(V2M_PA_CS3 +
> V2M_PERIPH_OFFSET(4)) #define V2M_MMCI
> (V2M_PA_CS3 + V2M_PERIPH_OFFSET(5)) @@ -72,23 +73,23 @@
>  
>  /* Generic Interrupt Controller Definitions */
>  #ifdef CONFIG_GICV3
> -#define GICD_BASE			(0x2f000000)
> -#define GICR_BASE			(0x2f100000)
> +#define GICD_BASE			(V2M_PA_BASE + 0x2f000000)
> +#define GICR_BASE			(V2M_PA_BASE + 0x2f100000)
>  #else
>  
> -#ifdef CONFIG_TARGET_VEXPRESS64_BASE_FVP
> -#define GICD_BASE			(0x2f000000)
> -#define GICC_BASE			(0x2c000000)
> -#elif CONFIG_TARGET_VEXPRESS64_JUNO
> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
>  #define GICD_BASE			(0x2C010000)
>  #define GICC_BASE			(0x2C02f000)
> +#else
> +#define GICD_BASE			(V2M_PA_BASE + 0x2f000000)
> +#define GICC_BASE			(V2M_PA_BASE + 0x2c000000)
>  #endif
>  #endif /* !CONFIG_GICV3 */
>  
>  #ifndef CONFIG_TARGET_VEXPRESS64_JUNO
>  /* The Vexpress64 simulators use SMSC91C111 */
>  #define CONFIG_SMC91111			1
> -#define CONFIG_SMC91111_BASE		(0x01A000000)
> +#define CONFIG_SMC91111_BASE		(V2M_PA_BASE + 0x01A000000)
>  #endif
>  
>  /* PL011 Serial Configuration */
> @@ -113,7 +114,7 @@
>  #ifdef CONFIG_TARGET_VEXPRESS64_JUNO
>  #define PHYS_SDRAM_2			(0x880000000)
>  #define PHYS_SDRAM_2_SIZE		0x180000000
> -#elif CONFIG_TARGET_VEXPRESS64_BASE_FVP && CONFIG_NR_DRAM_BANKS == 2
> +#elif CONFIG_NR_DRAM_BANKS == 2
>  #define PHYS_SDRAM_2			(0x880000000)
>  #define PHYS_SDRAM_2_SIZE		0x80000000
>  #endif
> @@ -200,6 +201,7 @@
>  				"  booti $kernel_addr - $fdt_addr; " \
>  				"fi"
>  #endif
> +
>  #endif
>  
>  /* Monitor Command Prompt */
> @@ -213,7 +215,7 @@
>  /* Store environment at top of flash in the same location as blank.img
> */ /* in the Juno firmware. */
>  #else
> -#define CONFIG_SYS_FLASH_BASE		0x0C000000
> +#define CONFIG_SYS_FLASH_BASE		(V2M_PA_BASE + 0x0C000000)
>  /* 256 x 256KiB sectors */
>  #define CONFIG_SYS_MAX_FLASH_SECT	256
>  /* Store environment at top of flash */
> @@ -230,4 +232,4 @@
>  #define CONFIG_SYS_FLASH_EMPTY_INFO	/* flinfo indicates empty
> blocks */ #define FLASH_MAX_SECTOR_SIZE		0x00040000
>  
> -#endif /* __VEXPRESS_AEMV8A_H */
> +#endif /* __VEXPRESS_AEMV8_H */

Re: [PATCH 3/5] vexpress64: Clean up BASE_FVP boot configuration

[Andre Przywara]

On Thu,  4 Nov 2021 16:56:17 +0000
Peter Hoyes <peter.hoyes@arm.com> wrote:

> From: Peter Hoyes <Peter.Hoyes@arm.com>
> 
> Move env var address values to #defines so they can be reused elsewhere.
> 
> Rename env var names to those recommended in the README.

Many thanks for cleaning this up and fixing the wrong variable names.
I think we should use opportunity to relax the load addresses, see below.

> Fix issue where fdt is called with invalid arguments when booting
> without a ramdisk.
> 
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> ---
>  include/configs/vexpress_aemv8.h | 50 ++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/include/configs/vexpress_aemv8.h b/include/configs/vexpress_aemv8.h
> index 49517a60b0..48c21082a6 100644
> --- a/include/configs/vexpress_aemv8.h
> +++ b/include/configs/vexpress_aemv8.h
> @@ -7,6 +7,8 @@
>  #ifndef __VEXPRESS_AEMV8_H
>  #define __VEXPRESS_AEMV8_H
>  
> +#include <linux/stringify.h>
> +
>  #define CONFIG_REMAKE_ELF
>  
>  /* Link Definitions */
> @@ -172,33 +174,43 @@
>  				BOOTENV
>  
>  #elif CONFIG_TARGET_VEXPRESS64_BASE_FVP
> +
> +#define VEXPRESS_FDT_ADDR	0x83000000

So this layout is somewhat miserly. My debug kernel of the day is 42MB
already, so the 47.5 MB we have reserved for the kernel now sound a bit tight.
In arm64 we don't have real constraints, and the model has surely enough
RAM, so can we become more generous?
- Kernel at 512KB (for compatibility with older Linux versions)
- ramdisk just below (or at?) 256MB
- everything else (script, DTB) just below the ramdisk

Cheers,
Andre

> +#define VEXPRESS_RAMDISK_ADDR	0x88000000
> +#define VEXPRESS_KERNEL_ADDR	0x80080000
> +#define VEXPRESS_BOOT_ADDR	0x8007f800
> +
>  #define CONFIG_EXTRA_ENV_SETTINGS	\
>  				"kernel_name=Image\0"		\
> -				"kernel_addr=0x80080000\0"	\
> -				"initrd_name=ramdisk.img\0"	\
> -				"initrd_addr=0x88000000\0"	\
> -				"fdtfile=devtree.dtb\0"		\
> -				"fdt_addr=0x83000000\0"		\
> -				"boot_name=boot.img\0"		\
> -				"boot_addr=0x8007f800\0"
> +				"kernel_addr_r=" __stringify(VEXPRESS_KERNEL_ADDR) "\0"	\
> +				"ramdisk_name=ramdisk.img\0"	\
> +				"ramdisk_addr_r=" __stringify(VEXPRESS_RAMDISK_ADDR) "\0" \
> +				"fdtfile=devtree.dtb\0"	\
> +				"fdt_addr_r=" __stringify(VEXPRESS_FDT_ADDR) "\0"	\
> +				"boot_name=boot.img\0" \
> +				"boot_addr_r=" __stringify(VEXPRESS_BOOT_ADDR) "\0"
>  
>  #ifndef CONFIG_BOOTCOMMAND
> -#define CONFIG_BOOTCOMMAND	"if smhload ${boot_name} ${boot_addr}; then " \
> +#define CONFIG_BOOTCOMMAND	"if smhload ${boot_name} ${boot_addr_r}; then " \
>  				"  set bootargs; " \
> -				"  abootimg addr ${boot_addr}; " \
> -				"  abootimg get dtb --index=0 fdt_addr; " \
> -				"  bootm ${boot_addr} ${boot_addr} " \
> -				"  ${fdt_addr}; " \
> +				"  abootimg addr ${boot_addr_r}; " \
> +				"  abootimg get dtb --index=0 fdt_addr_r; " \
> +				"  bootm ${boot_addr_r} ${boot_addr_r} " \
> +				"  ${fdt_addr_r}; " \
>  				"else; " \
>  				"  set fdt_high 0xffffffffffffffff; " \
>  				"  set initrd_high 0xffffffffffffffff; " \
> -				"  smhload ${kernel_name} ${kernel_addr}; " \
> -				"  smhload ${fdtfile} ${fdt_addr}; " \
> -				"  smhload ${initrd_name} ${initrd_addr} "\
> -				"  initrd_end; " \
> -				"  fdt addr ${fdt_addr}; fdt resize; " \
> -				"  fdt chosen ${initrd_addr} ${initrd_end}; " \
> -				"  booti $kernel_addr - $fdt_addr; " \
> +				"  smhload ${kernel_name} ${kernel_addr_r}; " \
> +				"  smhload ${fdtfile} ${fdt_addr_r}; " \
> +				"  smhload ${ramdisk_name} ${ramdisk_addr_r} "\
> +				"  ramdisk_end; " \
> +				"  fdt addr ${fdt_addr_r}; fdt resize; " \
> +				"  if test -n ${ramdisk_end}; then "\
> +				"    fdt chosen ${ramdisk_addr_r} ${ramdisk_end}; " \
> +				"  else; " \
> +				"    fdt chosen; " \
> +				"  fi; " \
> +				"  booti $kernel_addr_r - $fdt_addr_r; " \
>  				"fi"
>  #endif
>  

Re: [PATCH 4/5] vexpress64: Enable OF_CONTROL and OF_BOARD for VExpress64

[Andre Przywara]

On Thu,  4 Nov 2021 16:56:18 +0000
Peter Hoyes <peter.hoyes@arm.com> wrote:

Hi,

> From: Peter Hoyes <Peter.Hoyes@arm.com>
> 
> Capture x0 in lowlevel_init.S as potential fdt address. Modify
> board_fdt_blob_setup to use fdt address from either vexpress_aemv8.h
> or lowlevel_init.S.
> 
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> ---
>  board/armltd/vexpress64/Makefile        |  5 +++++
>  board/armltd/vexpress64/lowlevel_init.S | 12 ++++++++++++
>  board/armltd/vexpress64/vexpress64.c    | 24 ++++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
>  create mode 100644 board/armltd/vexpress64/lowlevel_init.S
> 
> diff --git a/board/armltd/vexpress64/Makefile b/board/armltd/vexpress64/Makefile
> index 868dc4f629..5703e75967 100644
> --- a/board/armltd/vexpress64/Makefile
> +++ b/board/armltd/vexpress64/Makefile
> @@ -5,3 +5,8 @@
>  
>  obj-y	:= vexpress64.o
>  obj-$(CONFIG_TARGET_VEXPRESS64_JUNO)	+= pcie.o
> +ifdef CONFIG_OF_BOARD
> +ifndef CONFIG_TARGET_VEXPRESS64_JUNO
> +obj-y += lowlevel_init.o

I wonder if it hurts to avoid all these confusing conditionals and just
always include this, even for Juno? Maybe we can use x0 even on the Juno
at some day?

> +endif
> +endif
> diff --git a/board/armltd/vexpress64/lowlevel_init.S b/board/armltd/vexpress64/lowlevel_init.S
> new file mode 100644
> index 0000000000..3dcfb85d0e
> --- /dev/null
> +++ b/board/armltd/vexpress64/lowlevel_init.S
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * (C) Copyright 2021 Arm Limited
> + */
> +
> +.global save_boot_params
> +save_boot_params:
> +
> +	adr	x8, prior_stage_fdt_address
> +	str	x0, [x8]
> +
> +	b	save_boot_params_ret
> diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c
> index d2f307cca5..bde6ef1ba3 100644
> --- a/board/armltd/vexpress64/vexpress64.c
> +++ b/board/armltd/vexpress64/vexpress64.c
> @@ -86,6 +86,8 @@ int dram_init_banksize(void)
>  }
>  
>  #ifdef CONFIG_OF_BOARD

Do we still need this? Every vexpress64 platform should now rely on OF_BOARD?

> +
> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
>  #define JUNO_FLASH_SEC_SIZE	(256 * 1024)
>  static phys_addr_t find_dtb_in_nor_flash(const char *partname)
>  {
> @@ -130,9 +132,19 @@ static phys_addr_t find_dtb_in_nor_flash(const char *partname)
>  
>  	return ~0;
>  }
> +#else

As suggested above, we probably should always include this variable below, so turn the #else into an #endif?

> +
> +/* Assigned in lowlevel_init.S
> + * Push the variable into the .data section so that it
> + * does not get cleared later.
> + */
> +unsigned long __section(".data") prior_stage_fdt_address;
> +
> +#endif
>  
>  void *board_fdt_blob_setup(int *err)
>  {
> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
>  	phys_addr_t fdt_rom_addr = find_dtb_in_nor_flash(CONFIG_JUNO_DTB_PART);
>  
>  	*err = 0;
> @@ -142,6 +154,18 @@ void *board_fdt_blob_setup(int *err)
>  	}
>  
>  	return (void *)fdt_rom_addr;
> +#else

Can you turn this into an #endif?

> +	if (fdt_magic(VEXPRESS_FDT_ADDR) == FDT_MAGIC) {
> +		*err = 0;
> +		return (void *)VEXPRESS_FDT_ADDR;

"else" after an unconditional return is somewhat frowned upon, since it
gives a wrong impression about the code flow.

What about:
#ifdef VEXPRESS_FDT_ADDR
	if (fdt_magic(VEXPRESS_FDT_ADDR) ... {
		...
		return ...;
	}
#endif

> +	} else if (fdt_magic(prior_stage_fdt_address) == FDT_MAGIC) {
> +		*err = 0;
> +		return (void *)prior_stage_fdt_address;
> +	} else {

If we always include prior_stage_fdt_address (even though it may be
unused), you can just always include this piece. And lose the else here,
since we return inside the if branch.

Cheers,
Andre

> +		*err = -ENXIO;
> +		return NULL;
> +	}
> +#endif
>  }
>  #endif
>  

Re: [PATCH 5/5] vexpress64: Enable VIRTIO_NET network driver

[Andre Przywara]

On Thu,  4 Nov 2021 16:56:19 +0000
Peter Hoyes <peter.hoyes@arm.com> wrote:

> From: Peter Hoyes <Peter.Hoyes@arm.com>
> 
> The SMSC driver is using the old driver model.
> 
> Init the virtio system in vexpress64.c so that the network device is
> discovered.

Mmmh, it looks a bit weird to explicitly call virtio_init(), when this
should be discovered through the DT, but that's probably how it's done at
the moment.

> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  board/armltd/vexpress64/vexpress64.c | 7 +++++++
>  include/configs/vexpress_aemv8.h     | 4 ++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c
> index bde6ef1ba3..d2e2404849 100644
> --- a/board/armltd/vexpress64/vexpress64.c
> +++ b/board/armltd/vexpress64/vexpress64.c
> @@ -18,6 +18,10 @@
>  #include <dm/platform_data/serial_pl01x.h>
>  #include "pcie.h"
>  #include <asm/armv8/mmu.h>
> +#ifdef CONFIG_VIRTIO_NET
> +#include <virtio_types.h>
> +#include <virtio.h>
> +#endif
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -64,6 +68,9 @@ __weak void vexpress64_pcie_init(void)
>  int board_init(void)
>  {
>  	vexpress64_pcie_init();
> +#ifdef CONFIG_VIRTIO_NET
> +	virtio_init();
> +#endif
>  	return 0;
>  }
>  
> diff --git a/include/configs/vexpress_aemv8.h b/include/configs/vexpress_aemv8.h
> index 48c21082a6..af160a9a66 100644
> --- a/include/configs/vexpress_aemv8.h
> +++ b/include/configs/vexpress_aemv8.h
> @@ -88,8 +88,8 @@
>  #endif
>  #endif /* !CONFIG_GICV3 */
>  
> -#ifndef CONFIG_TARGET_VEXPRESS64_JUNO
> -/* The Vexpress64 simulators use SMSC91C111 */
> +#if defined(CONFIG_TARGET_VEXPRESS64_BASE_FVP) && !defined(CONFIG_DM_ETH)
> +/* The Vexpress64 BASE_FVP simulator uses SMSC91C111 */
>  #define CONFIG_SMC91111			1
>  #define CONFIG_SMC91111_BASE		(V2M_PA_BASE + 0x01A000000)
>  #endif

Re: [PATCH 4/5] vexpress64: Enable OF_CONTROL and OF_BOARD for VExpress64

[Peter Hoyes]

Hi,

On 09/11/2021 15:06, Andre Przywara wrote:
> On Thu,  4 Nov 2021 16:56:18 +0000
> Peter Hoyes <peter.hoyes@arm.com> wrote:
>
> Hi,
>
>> From: Peter Hoyes <Peter.Hoyes@arm.com>
>>
>> Capture x0 in lowlevel_init.S as potential fdt address. Modify
>> board_fdt_blob_setup to use fdt address from either vexpress_aemv8.h
>> or lowlevel_init.S.
>>
>> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
>> ---
>>   board/armltd/vexpress64/Makefile        |  5 +++++
>>   board/armltd/vexpress64/lowlevel_init.S | 12 ++++++++++++
>>   board/armltd/vexpress64/vexpress64.c    | 24 ++++++++++++++++++++++++
>>   3 files changed, 41 insertions(+)
>>   create mode 100644 board/armltd/vexpress64/lowlevel_init.S
>>
>> diff --git a/board/armltd/vexpress64/Makefile b/board/armltd/vexpress64/Makefile
>> index 868dc4f629..5703e75967 100644
>> --- a/board/armltd/vexpress64/Makefile
>> +++ b/board/armltd/vexpress64/Makefile
>> @@ -5,3 +5,8 @@
>>   
>>   obj-y	:= vexpress64.o
>>   obj-$(CONFIG_TARGET_VEXPRESS64_JUNO)	+= pcie.o
>> +ifdef CONFIG_OF_BOARD
>> +ifndef CONFIG_TARGET_VEXPRESS64_JUNO
>> +obj-y += lowlevel_init.o
> I wonder if it hurts to avoid all these confusing conditionals and just
> always include this, even for Juno? Maybe we can use x0 even on the Juno
> at some day?
>
>> +endif
>> +endif
>> diff --git a/board/armltd/vexpress64/lowlevel_init.S b/board/armltd/vexpress64/lowlevel_init.S
>> new file mode 100644
>> index 0000000000..3dcfb85d0e
>> --- /dev/null
>> +++ b/board/armltd/vexpress64/lowlevel_init.S
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * (C) Copyright 2021 Arm Limited
>> + */
>> +
>> +.global save_boot_params
>> +save_boot_params:
>> +
>> +	adr	x8, prior_stage_fdt_address
>> +	str	x0, [x8]
>> +
>> +	b	save_boot_params_ret
>> diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c
>> index d2f307cca5..bde6ef1ba3 100644
>> --- a/board/armltd/vexpress64/vexpress64.c
>> +++ b/board/armltd/vexpress64/vexpress64.c
>> @@ -86,6 +86,8 @@ int dram_init_banksize(void)
>>   }
>>   
>>   #ifdef CONFIG_OF_BOARD
> Do we still need this? Every vexpress64 platform should now rely on OF_BOARD?

BASE_FVP does not have OF_BOARD defined in its defconfig for now.

I have done some basic testing with OF_BOARD enabled on the BASE_FVP and 
it seems OK at first glance, but I'm concerned about changes in behavior 
for existing users (e.g. if extra drivers are being automatically 
instantiated by nodes in the fdt). The other changes in this patch 
series (e.g. changing the env vars) are easier to reason about.

>
>> +
>> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
>>   #define JUNO_FLASH_SEC_SIZE	(256 * 1024)
>>   static phys_addr_t find_dtb_in_nor_flash(const char *partname)
>>   {
>> @@ -130,9 +132,19 @@ static phys_addr_t find_dtb_in_nor_flash(const char *partname)
>>   
>>   	return ~0;
>>   }
>> +#else
> As suggested above, we probably should always include this variable below, so turn the #else into an #endif?
>
>> +
>> +/* Assigned in lowlevel_init.S
>> + * Push the variable into the .data section so that it
>> + * does not get cleared later.
>> + */
>> +unsigned long __section(".data") prior_stage_fdt_address;
>> +
>> +#endif
>>   
>>   void *board_fdt_blob_setup(int *err)
>>   {
>> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
>>   	phys_addr_t fdt_rom_addr = find_dtb_in_nor_flash(CONFIG_JUNO_DTB_PART);
>>   
>>   	*err = 0;
>> @@ -142,6 +154,18 @@ void *board_fdt_blob_setup(int *err)
>>   	}
>>   
>>   	return (void *)fdt_rom_addr;
>> +#else
> Can you turn this into an #endif?
>
>> +	if (fdt_magic(VEXPRESS_FDT_ADDR) == FDT_MAGIC) {
>> +		*err = 0;
>> +		return (void *)VEXPRESS_FDT_ADDR;
> "else" after an unconditional return is somewhat frowned upon, since it
> gives a wrong impression about the code flow.
>
> What about:
> #ifdef VEXPRESS_FDT_ADDR
> 	if (fdt_magic(VEXPRESS_FDT_ADDR) ... {
> 		...
> 		return ...;
> 	}
> #endif
>
>> +	} else if (fdt_magic(prior_stage_fdt_address) == FDT_MAGIC) {
>> +		*err = 0;
>> +		return (void *)prior_stage_fdt_address;
>> +	} else {
> If we always include prior_stage_fdt_address (even though it may be
> unused), you can just always include this piece. And lose the else here,
> since we return inside the if branch.
>
> Cheers,
> Andre
>
>> +		*err = -ENXIO;
>> +		return NULL;
>> +	}
>> +#endif
>>   }
>>   #endif
>>   

Your other suggestions make sense to me.

Peter