Re: [PATCH 1/1] hw/arm/sbsa-ref: add PCIe node into DT

[Leif Lindholm]

On 2023-06-27 14:27, Peter Maydell wrote:
> On Tue, 27 Jun 2023 at 13:52, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> > On 2023-06-27 13:12, Peter Maydell wrote:
> > > On Mon, 26 Jun 2023 at 08:52, Marcin Juszkiewicz
> > > <marcin.juszkiewicz@linaro.org> wrote:
> > > > Add PCI Express information into DeviceTree as part of SBSA-REF
> > > > versioning.
> > > >
> > > > Trusted Firmware will read it and provide to next firmware level.
> > > >
> > > > Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> > > > ---
> > > >    hw/arm/sbsa-ref.c | 20 ++++++++++++++++++++
> > > >    1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> > > > index 0639f97dd5..b87d2ee3b2 100644
> > > > --- a/hw/arm/sbsa-ref.c
> > > > +++ b/hw/arm/sbsa-ref.c
> > > > @@ -171,6 +171,25 @@ static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState 
> > > > *sms, int idx)
> > > >        return arm_cpu_mp_affinity(idx, clustersz);
> > > >    }
> > > >
> > > > +static void sbsa_fdt_add_pcie_node(SBSAMachineState *sms)
> > > > +{
> > > > +    char *nodename;
> > > > +
> > > > +    nodename = g_strdup_printf("/pcie");
> > > > +    qemu_fdt_add_subnode(sms->fdt, nodename);
> > > > +    qemu_fdt_setprop_sized_cells(sms->fdt, nodename, "reg",
> > > > +                                 2, sbsa_ref_memmap[SBSA_PCIE_ECAM].base,
> > > > +                                 2, sbsa_ref_memmap[SBSA_PCIE_ECAM].size,
> > > > +                                 2, sbsa_ref_memmap[SBSA_PCIE_PIO].base,
> > > > +                                 2, sbsa_ref_memmap[SBSA_PCIE_PIO].size,
> > > > +                                 2, sbsa_ref_memmap[SBSA_PCIE_MMIO].base,
> > > > +                                 2, sbsa_ref_memmap[SBSA_PCIE_MMIO].size,
> > > > +                                 2, sbsa_ref_memmap[SBSA_PCIE_MMIO_HIGH].base,
> > > > +                                 2, sbsa_ref_memmap[SBSA_PCIE_MMIO_HIGH].size);
> > > > +
> > > > +    g_free(nodename);
> > >
> > >
> > > Why do we need to do this? The firmware should just
> > > know exactly where the PCIE windows are, the same way
> > > it knows where the flash, the UART, the RTC etc etc
> > > all are in the memory map.
> >
> > That is not automatically true (it was not for at least one SoC I have
> > worked on). In a real system which had these dynamically decided, some
> > coprocessor would program the CMN to route these address ranges to
> > certain offsets within certain components, and that same coprocessor
> > could then provide a mechanism for how TF-A could discover these and
> > provide it to later-stage firmware via SiP SMC calls.
> >
> > Sticking the information in the DT lets us emulate this without actually
> > emulating the CMN and the coprocessor, and prototype (and verify) the
> > same firmware interfaces for developing i.e. edk2 support.
>
> OK. This is the kind of rationale that needs to be described
> in the commit message and comments, though, so it's clear
> why the PCI controller is special in this way.

I mean, ultimately it's not. We've kept static mappings for the items 
that it wouldn't really provide any additional benefit anywhere to be 
able to shuffle around. (Which failed with EHCI.)
Having the UART static has minor debug advantages. Everything else is 
static because it's poor return on investment (it doesn't let us test 
anything interesting in the firmware stacks) to make them dynamic.

> What I'm trying to avoid here is that we start off saying
> "the dtb we pass to the firmware is just a convenient format
> for passing a tiny amount of information to it" and then
> gradually add more and more stuff to it until we've backed
> ourselves into "actually it's almost a complete dtb except
> it's not compliant with the spec". That means there needs
> to be a clear rationale for what is in the dtb versus
> what is "the firmware knows what hardware it runs on".

Yet again I find myself wishing I'd invented a custom format instead of 
using a DT to pass the information across. And I'm not even being sarky 
- it keeps confusing people, and across multiple projects we're being 
asked to repeatedly justify why we're not conforming to bindings that 
are not designed for what we want to do, when we only opted to use an 
existing storage format in order to maximise code reuse.

The purpose of the DT in this platform is to pass information about the 
machine configuration to the highest-level firmware that in real 
hardware it would have means of determining programmatically *handwavy* 
"in other ways", so that that higher-level firmware can abstract the 
information away behind SMC calls that lower levels of firmware can 
access throug into reusable libraries that will also be useful for 
actual hardware platforms.
*and* let us wiggle things around to try to shake out bugs in those (and 
other) firmware components.

Serious question: would it be preferable if we moved to a custom DT node 
where we stick everything in as KEY=VALUE pairs to reduce this confusion?

The end goal at some point in the future is to add an (emulated or 
co-emulated) SCP that can provide us with this information, at which 
point the (not a linux bindings)DT can simply go away forever.

/
    Leif