Hi Patrick,

On Fri, 20 Sept 2024 at 12:29, Patrick Rudolph
<patrick.rudo...@9elements.com> wrote:
>
> On Fri, Sep 20, 2024 at 11:37 AM Simon Glass <s...@chromium.org> wrote:
> >
> > +Peter Maydell for comment on the below
> >
> > Hi Patrick,
> >
> > On Fri, 20 Sept 2024 at 09:54, Patrick Rudolph
> > <patrick.rudo...@9elements.com> wrote:
> > >
> > > On Thu, Sep 19, 2024 at 4:10 PM Simon Glass <s...@chromium.org> wrote:
> > > >
> > > > Hi Patrick,
> > > >
> > > > On Wed, 18 Sept 2024 at 08:29, Patrick Rudolph
> > > > <patrick.rudo...@9elements.com> wrote:
> > > > >
> > > > > On Mon, Sep 16, 2024 at 5:41 PM Simon Glass <s...@chromium.org> wrote:
> > > > > >
> > > > > > Hi Patrick,
> > > > > >
> > > > > > On Thu, 12 Sept 2024 at 00:23, Patrick Rudolph
> > > > > > <patrick.rudo...@9elements.com> wrote:
> > > > > > >
> > > > > > > On Thu, Sep 12, 2024 at 2:58 AM Simon Glass <s...@chromium.org> 
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi Patrick,
> > > > > > > >
> > > > > > > > On Wed, 11 Sept 2024 at 00:26, 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.
> > > > > > > > >
> > > > > > > > > Add a minimal amount of devicetree nodes to make the board 
> > > > > > > > > useable within
> > > > > > > > > U-Boot, 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
> > > > > > > > >
> > > > > > > > > ---
> > > > > > > > >  arch/arm/Kconfig                            |   1 +
> > > > > > > > >  arch/arm/dts/qemu-sbsa.dts                  |  49 ++
> > > > > > > > >  arch/arm/include/asm/arch-qemu-sbsa/boot0.h |  33 ++
> > > > > > > > >  arch/arm/mach-qemu/Kconfig                  |  36 +-
> > > > > > > > >  board/emulation/qemu-arm/MAINTAINERS        |   2 +
> > > > > > > > >  board/emulation/qemu-sbsa/Kconfig           |  58 +++
> > > > > > > > >  board/emulation/qemu-sbsa/Makefile          |   8 +
> > > > > > > > >  board/emulation/qemu-sbsa/acpi.c            | 235 ++++++++++
> > > > > > > > >  board/emulation/qemu-sbsa/dsdt.asl          | 483 
> > > > > > > > > ++++++++++++++++++++
> > > > > > > > >  board/emulation/qemu-sbsa/lowlevel_init.S   |  22 +
> > > > > > > > >  board/emulation/qemu-sbsa/qemu-sbsa.c       | 481 
> > > > > > > > > +++++++++++++++++++
> > > > > > > > >  board/emulation/qemu-sbsa/qemu-sbsa.env     |  14 +
> > > > > > > > >  board/emulation/qemu-sbsa/qemu-sbsa.h       |  38 ++
> > > > > > > > >  board/emulation/qemu-sbsa/smc.c             |  72 +++
> > > > > > > > >  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                 |  98 ++++
> > > > > > > > >  19 files changed, 1734 insertions(+), 6 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
> > > > > > > >
> > > > > > > > Wow there is a lot in this patch!
> > > > > > > >
> > > > > > > > I don't think this should be in a separate subdir, just using
> > > > > > > > board/emulation/qemu-arm should be OK?
> > > > > > > Why would it share the code base with qemu-arm, as it's a 
> > > > > > > completely
> > > > > > > different board?
> > > > > >
> > > > > > Perhaps I misunderstand this...what kind of board is it? How do you
> > > > > > run qemu for this board?
> > > > > >
> > > > > For QEMU SBSA you use -machine sbsa-ref, but for qemu-arm you use 
> > > > > -machine virt.
> > > > >
> > > > > Both can use a cortex-a57 as CPU, but that's all those boards share. 
> > > > > They have a
> > > > > different memory layout, virt provides a complete FDT, while sbsa-ref 
> > > > > doesn't.
> > > > > sbsa-ref needs an Arm-TF and nonsecure boot firmware, while virt works
> > > > > without a firmware.
> > > >
> > > > OK I see
> > > >
> > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > I see you are setting plat data in the board file...is there 
> > > > > > > > not a CPU
> > > > > > > > driver which can read it from the devicetree or set it based on 
> > > > > > > > the
> > > > > > > > compatible string?
> > > > > > > The CPU driver could read the data from devicetree, but that 
> > > > > > > would require a new
> > > > > > > DT binding. Is that the preferred method over using plat data?
> > > > > >
> > > > > > It is fine to use plat data...but it should be set up in the driver
> > > > > > where possible. In this case if you don't have a binding you can 
> > > > > > check
> > > > > > the compatible string of the device (or even the root
> > > > > > compatible-string) and add your plat data if it matches.
> > > > >
> > > > > I was able to parse everything from the DT, except for PMU IRQs. Those
> > > > > aren't needed to
> > > > > boot the platform and can be added later on.
> > > > > Will send out a new revision soon.
> > > >
> > > > OK
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > For FDT fixup, can you please use the event and the ofnode 
> > > > > > > > interface?
> > > > > > > > See EVT_FT_FIXUP and bootmeth_vbe_ft_fixup() as an example.
> > > > >
> > > > > It looks like EVT_FT_FIXUP is run rather late in the bootflow, used to
> > > > > updated the FDT for OS usage,
> > > > > but it must be run early to update the minimal FDT passed by QEMU and
> > > > > add the missing nodes
> > > > > for U-Boot drivers.
> > > >
> > > > If the QEMU FDT is not suitable, shouldn't we use the U-Boot/Linux
> > > > one? Do we have to use the one passed in by QEMU?
> > > >
> > > There's no U-Boot/Linux FDT, since this is an "ACPI only" platform.
> > > In EDK2 firmware there's everything hard-coded as well to generate
> > > proper ACPI code, but since
> > > U-Boot relies on FDT, I decided to "fix" it and add the required
> > > missing nodes. This allows to make
> > > use of U-Boot existing infrastructure around FDT.
> > >
> >
> > So wouldn't it be better to pass in a u-boot.bin with the DT attached?
> > Adding the thing in code seems pretty ugly.
> >
> Would that make use of CONFIG_OF_EMBED?
> I decided against this option as it "is not recommended for production 
> devices."
> However I agree that it's possible and would be much cleaner and less code.
> Will look into this.

Well, indeed, it would be better not to use that. Does QEMU support a
flat binary or does it require an ELF? Given the choice between
OF_EMBED and your DT-building code I would go with the latter.

>
> > > The QEMU generated FDT contains a bare minimum of nodes and isn't
> > > suitable to boot an OS.
> >
> > Yes, I am not very pleased about that, particularly as my patch to
> > allow passing a proper FDT to QEMU[1] has still not been accepted.
> >
> The SBSA ref machine was designed that way, to make sure that only ACPI is 
> used
> to boot the OS.

Er, but doesn't firmware run before the OS? It seems like a strange
design decision.


Regards,
Simon

> >
> > [1] 
> > https://patchwork.kernel.org/project/qemu-devel/patch/20210926183410.256484-1-...@chromium.org/#24481799

Reply via email to