Hi Tom, On Thu, 3 Oct 2024 at 08:43, Tom Rini <tr...@konsulko.com> wrote: > > On Thu, Oct 03, 2024 at 07:50:45AM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 2 Oct 2024 at 19:35, Tom Rini <tr...@konsulko.com> wrote: > > > > > > On Wed, Oct 02, 2024 at 04:55:34PM -0600, Simon Glass wrote: > > > > Hi Patrick, > > > > > > > > On Wed, 2 Oct 2024 at 03:52, Patrick Rudolph > > > > <patrick.rudo...@9elements.com> wrote: > > > > > > > > > > Add support for Arm sbsa [1] v0.3+ that is supported by QEMU [2]. > > > > > > > > > > Unlike other Arm based platforms the machine only provides a minimal > > > > > FDT that contains number of CPUs, ammount of memory and > > > > > machine-version. > > > > > The boot firmware has to provide ACPI tables to the OS. > > > > > Due to this design a full DTB is added here as well that allows > > > > > U-Boot's > > > > > driver to properly function. The DTB is appended at the end of the > > > > > U-Boot > > > > > image and will be merged with the QEMU provided DTB. > > > > > > > > > > In addition provide documentation how to use, enable binman to > > > > > fabricate both > > > > > ROMs that are required to boot and add ACPI tables to make it full > > > > > compatible > > > > > to the EDK2 reference implementation. > > > > > > > > > > The board was tested using Fedora 40 Aarch64 Workstation. It's able > > > > > to boot from USB and AHCI or network. > > > > > > > > > > Tested and found working: > > > > > - serial > > > > > - PCI > > > > > - xHCI > > > > > - Bochs display > > > > > - AHCI > > > > > - network using e1000e > > > > > - CPU init > > > > > - Booting Fedora 40 > > > > > > > > > > 1: Server Base System Architecture (SBSA) > > > > > 2: https://www.qemu.org/docs/master/system/arm/sbsa.html > > > > > > > > > > Signed-off-by: Patrick Rudolph <patrick.rudo...@9elements.com> > > > > > Cc: Peter Robinson <pbrobin...@gmail.com> > > > > > Cc: Simon Glass <s...@chromium.org> > > > > > Cc: Tom Rini <tr...@konsulko.com> > > > > > --- > > > > > Changelog v3: > > > > > - Add GIC and GIC-ITS to devicetree > > > > > - Select GICv3 driver > > > > > - Drop acpi_fill_madt and use driver model instead > > > > > Changelog v4: > > > > > - Drop CPU platform code > > > > > - Enhance the DT to allow MADT generation from DT > > > > > Changelog v5: > > > > > - Add full DT and place it at the end of U-Boot > > > > > - Merge DT with QEMU's DT > > > > > - Drop DT generation code > > > > > - Fix flash region length > > > > > - Drop enable_caches() > > > > > - Support platforms that do not pass FDT in x0 > > > > > Changelog v6: > > > > > - Update header order > > > > > - Drop pad-byte from DT > > > > > - select BINMAN_FDT > > > > > - select E1000_NO_NVM > > > > > - drop config.h include > > > > > - drop a few CFG_ defines that were used for SPL > > > > > > > > > > --- > > > > > arch/arm/Kconfig | 3 +- > > > > > arch/arm/dts/qemu-sbsa.dts | 136 ++++++ > > > > > arch/arm/include/asm/arch-qemu-sbsa/boot0.h | 34 ++ > > > > > arch/arm/mach-qemu/Kconfig | 36 +- > > > > > board/emulation/qemu-arm/MAINTAINERS | 2 + > > > > > board/emulation/qemu-sbsa/Kconfig | 57 +++ > > > > > board/emulation/qemu-sbsa/Makefile | 8 + > > > > > board/emulation/qemu-sbsa/acpi.c | 192 ++++++++ > > > > > board/emulation/qemu-sbsa/dsdt.asl | 483 > > > > > ++++++++++++++++++++ > > > > > board/emulation/qemu-sbsa/lowlevel_init.S | 22 + > > > > > board/emulation/qemu-sbsa/qemu-sbsa.c | 311 +++++++++++++ > > > > > board/emulation/qemu-sbsa/qemu-sbsa.env | 14 + > > > > > board/emulation/qemu-sbsa/qemu-sbsa.h | 38 ++ > > > > > board/emulation/qemu-sbsa/smc.c | 71 +++ > > > > > configs/qemu-arm-sbsa_defconfig | 10 + > > > > > doc/board/emulation/index.rst | 1 + > > > > > doc/board/emulation/qemu-sbsa.rst | 98 ++++ > > > > > doc/develop/driver-model/virtio.rst | 1 + > > > > > include/configs/qemu-sbsa.h | 95 ++++ > > > > > 19 files changed, 1605 insertions(+), 7 deletions(-) > > > > > create mode 100644 arch/arm/dts/qemu-sbsa.dts > > > > > create mode 100644 arch/arm/include/asm/arch-qemu-sbsa/boot0.h > > > > > create mode 100644 board/emulation/qemu-sbsa/Kconfig > > > > > create mode 100644 board/emulation/qemu-sbsa/Makefile > > > > > create mode 100644 board/emulation/qemu-sbsa/acpi.c > > > > > create mode 100644 board/emulation/qemu-sbsa/dsdt.asl > > > > > create mode 100644 board/emulation/qemu-sbsa/lowlevel_init.S > > > > > create mode 100644 board/emulation/qemu-sbsa/qemu-sbsa.c > > > > > create mode 100644 board/emulation/qemu-sbsa/qemu-sbsa.env > > > > > create mode 100644 board/emulation/qemu-sbsa/qemu-sbsa.h > > > > > create mode 100644 board/emulation/qemu-sbsa/smc.c > > > > > create mode 100644 configs/qemu-arm-sbsa_defconfig > > > > > create mode 100644 doc/board/emulation/qemu-sbsa.rst > > > > > create mode 100644 include/configs/qemu-sbsa.h > > > > > > > > With the tweaks below: > > > > > > > > Reviewed-by: Simon Glass <s...@chromium.org> > > > > > > > > [..] > > > > > > > > > diff --git a/arch/arm/dts/qemu-sbsa.dts b/arch/arm/dts/qemu-sbsa.dts > > > > > new file mode 100644 > > > > > index 0000000000..f743ea3863 > > > > > --- /dev/null > > > > > +++ b/arch/arm/dts/qemu-sbsa.dts > > > > > @@ -0,0 +1,136 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0+ OR MIT > > > > > +/* > > > > > + * Devicetree with onboard devices for qemu_sbsa-ref for internal > > > > > use only! > > > > > + * DO NOT PASS TO THE OS! > > > > > + * > > > > > + * As QEMU provides only a minimal devicetree this one is merged with > > > > > + * it and then fixed at runtime. > > > > > + * > > > > > + * Copyright 2024 9elements GmbH > > > > > + */ > > > > > +#include "configs/qemu-sbsa.h" > > > > > + > > > > > +/dts-v1/; > > > > > + > > > > > +#define MAKE_PROP(x) (x>>32) (x&0xffffffff) > > > > > + > > > > > +/ { > > > > > + #address-cells = <2>; > > > > > + #size-cells = <2>; > > > > > + interrupt-parent = <&intc>; > > > > > + compatible = "linux,sbsa-ref"; > > > > > + > > > > > + binman: binman { > > > > > + multiple-images; > > > > > + }; > > > > > + > > > > > + cpus { > > > > > + /* Filled by fdtdec_board_setup() */ > > > > > + }; > > > > > + > > > > > + memory { > > > > > + /* Filled by fdtdec_board_setup() */ > > > > > + }; > > > > > + > > > > > + soc { > > > > > + compatible = "simple-bus"; > > > > > + #address-cells = <2>; > > > > > + #size-cells = <2>; > > > > > + ranges; > > > > > + > > > > > + uart0 { > > > > > + compatible = "arm,pl011"; > > > > > + status = "okay"; > > > > > + reg = <MAKE_PROP(SBSA_UART_BASE_ADDR) > > > > > > > > Can you use > > > > > > > > reg = /bits 64/ <0x60000000> > > > > > > > > instead? > > > > > > > > Same for those below. > > > > > > Isn't that less readable ? > > > > Which bit of it is less readable? > > Having hex spelled out rather than a macro that's clear what's being > generated.
To my eye it seems odd to define a base address in a macro only to have it used once in a devicetree. We don't do that elsewhere. Breaking down the SBSA_UART_BASE_ADDR: - this is in the uart0 node so obviously relates to the UART - it is a reg property, so BASE_ADDR is obvious - SBSA is the board name, which is in the filename, so don't really add anything Normally where the devicetree needs a value defined, we put it in the dt-bindings directory. But even then, we don't typically put addresses there. It is just for cases where we need the code and the devicetree to be in sync about a property value. With addresses, the code reads them from the devicetree, so doesn't need to have a #define to access them...in fact that would defeat the purpose. If we did this sort of thing with other boards, we would have a heap of binding files containing addresses that are only used in one place. I just don't see the point. However for SBSA_PCIE_MMIO_BASE_ADDR I see that it is used in the .asl and the memory map, so it seems fair enough to define that, at least unless/until we can refer to devicetree addresses in the .asl file. Even with this one though, I would expect something like: ranges = <0x02000000>, /bits/ 64 <SBSA_PCIE_MMIO_BASE_ADDR SBSA_PCIE_MMIO_BASE_ADDR>; But in this case I can see how a macro might be more convenient and I'm not sure I have a strong opinion here. > > > But I very much doubt upstream would accept it with a macro. Other > > 64-bit boards seem to use '/bits/ 64'. > > What upstream are you referring to here? I'm not sure what other project > will want a device tree for the "no device tree only acpi" virtual > reference platform. It depends if they want to run U-Boot or not. U-Boot always needs a devicetree, as you know. It cannot use ACPI tables to configure itself and I would rather it doesn't start. I don't actually see any reason why this devicetree could not be upstreamed, except that QEMU actually creates a Linux devicetree right now (on the ARM side) and takes no notice of what U-Boot needs... I don't see any U-Boot patches from the maintainers, for example, although I may have missed some. It seems very focussed on UEFI. I would love that to change, as QEMU is widely used in U-Boot on ARM... Regards, Simon
