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

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

[Marcin Juszkiewicz]

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);
+}
+
 static void sbsa_fdt_add_gic_node(SBSAMachineState *sms)
 {
     char *nodename;
@@ -286,6 +305,7 @@ static void create_fdt(SBSAMachineState *sms)
     }
 
     sbsa_fdt_add_gic_node(sms);
+    sbsa_fdt_add_pcie_node(sms);
 }
 
 #define SBSA_FLASH_SECTOR_SIZE (256 * KiB)
-- 
2.41.0

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Leif Lindholm]

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.

/
     Leif

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

[Peter Maydell]

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.

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".

thanks
-- PMM

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

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

[Peter Maydell]

On Tue, 27 Jun 2023 at 15:09, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> On 2023-06-27 14:27, Peter Maydell wrote:
> > 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?

I don't really mind, I just want it to be clear what is going on here
so that when I'm reviewing patches I have a design I can keep in mind.
The way this was presented to me originally, at least as I recall
it, was "this board will work the way that real hardware does, ie
the firmware knows what hardware it was built for". In that
setup QEMU doesn't need to tell the firmware anything, except
a very limited set of things which it's more convenient to have
flexible and specifiable on the QEMU command line, like number of
CPUs and size of RAM. And that's what the comments in the source say
at the moment:

/*
 * Firmware on this machine only uses ACPI table to load OS, these limited
 * device tree nodes are just to let firmware know the info which varies from
 * command line parameters, so it is not necessary to be fully compatible
 * with the kernel CPU and NUMA binding rules.
 */

So that's the design I've been implicitly reviewing these changes
against.

It is pretty surprising to me to hear that in real-world systems
the firmware is not built to know exactly where its UART, USB
controller, etc are and that it is instead asking some board
management controller chip for all this information and being
fully-flexible in the firmware that runs on the application CPU,
but I have zero experience in that area so that's just my
lack of knowledge speaking.

If there's a standard/common protocol for how the BMC communicates
that info to the application-CPU firmware then it might be
less confusing to use it, I guess. But I'm not inherently
opposed to putting this stuff in a dtb-format blob.

(Side note: is the commit message line "Trusted Firmware will
read it and provide to next firmware level." intended to
mean "TF will take this dtb node and pass it on", or merely
"TF will take the information in this dtb node and use
it to construct or modify the ACPI tables it passes to the
next level"?)

thanks
-- PMM

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

[Leif Lindholm]

On 2023-06-27 15:27, Peter Maydell wrote:
>> 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?
> 
> I don't really mind, I just want it to be clear what is going on here
> so that when I'm reviewing patches I have a design I can keep in mind.

No, I totally understand that. I'm just feeling that the call we made to 
use DT to pass this information tends to put everyone's mind into the 
wrong state when reviewing.

> The way this was presented to me originally, at least as I recall
> it, was "this board will work the way that real hardware does, ie
> the firmware knows what hardware it was built for".
 >
> In that
> setup QEMU doesn't need to tell the firmware anything, except
> a very limited set of things which it's more convenient to have
> flexible and specifiable on the QEMU command line, like number of
> CPUs and size of RAM. And that's what the comments in the source say
> at the moment:
> 
> /*
>   * Firmware on this machine only uses ACPI table to load OS, these limited
>   * device tree nodes are just to let firmware know the info which varies from
>   * command line parameters, so it is not necessary to be fully compatible
>   * with the kernel CPU and NUMA binding rules.
>   */
> 
> So that's the design I've been implicitly reviewing these changes
> against.

I still agree fully with the above. But possibly I meant less by what I 
said than you heard. Human language, eh?

The only things that *need* hardcoding really are:
- What (family of) platform(s) is this? (sbsa-ref)
- Start of Secure memory.
- Start of Non-secure memory.
- Mechanism by which to receive platform configuration data that might
   differ at runtime.

Admittedly, using QEMU gives us more flexibility than would be likely in 
a real platform - like specifying any supported CPU that can access the 
whole address map. A real platform would be very unlikely to have 
runtime detection for everything from Cortex-A57 to Neoverse-N2 - but 
it's genuinely useful for our (non-platform-specific) firmware 
development to be able to do that.

> It is pretty surprising to me to hear that in real-world systems
> the firmware is not built to know exactly where its UART, USB
> controller, etc are and that it is instead asking some board
> management controller chip for all this information and being
> fully-flexible in the firmware that runs on the application CPU,
> but I have zero experience in that area so that's just my
> lack of knowledge speaking.

As an example, in said previous design we were prototyping having the 
ability to hold PCIe space inside 48 bits (to work with non-LPA2-aware
operating systems utilising 4k pages) and having a software-configurable 
option to expand into 52 bits (enabling more space for both DRAM and 
PCIe), where an LPA2-aware (or 64k-paged) OS was used.

The addresses (from the application processor's perspective) of certain 
other i/o blocks were also ultimately decided by a microcontroller 
programming the CMN. So even if we "decided" on locations for them and 
stuck to those for simplicity, they weren't actually hard-wired.

> If there's a standard/common protocol for how the BMC communicates
> that info to the application-CPU firmware then it might be
> less confusing to use it, I guess. But I'm not inherently
> opposed to putting this stuff in a dtb-format blob.

Not really. If anything I'm hoping to inspire standardisation along 
those axes by having libraries available to just plug in.

> (Side note: is the commit message line "Trusted Firmware will
> read it and provide to next firmware level." intended to
> mean "TF will take this dtb node and pass it on", or merely
> "TF will take the information in this dtb node and use
> it to construct or modify the ACPI tables it passes to the
> next level"?)

The latter.
We're kind of working our way backwards towards the design we ultimately 
want, so the DT was originally placed in NS DRAM since that let us reuse 
more of the mach-virt code in edk2 rather than needing to rewrite 
everything to get anything working.

Now we're getting versioning in place, we will eventually deprecate that 
and move the DT to Secure DRAM, dropping access to it for Non-secure 
firmware. But we never exposed the DT to the OS as a configuration 
table, it was always used to generate the bits of ACPI that weren't just 
hardcoded.

/
     Leif



/
     Leif

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

[Peter Maydell]

On Tue, 27 Jun 2023 at 14:27, Peter Maydell <peter.maydell@linaro.org> 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.

Just to be clear about the status of this patch, I don't
have a problem with the code changes, but it does definitely
need a much clearer commit message to explain why we're changing
the way we handle the PCI controller. So I'm dropping this from
my to-review list on the assumption there will be a v2.

We could also do with expanding the commentary in the source
file to clarify the design approach we're using w.r.t. what
we do and don't want to put into the "dt" blob, but that would
probably be best in a different patch.

thanks
-- PMM

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

[Marcin Juszkiewicz]

W dniu 4.07.2023 o 15:21, Peter Maydell pisze:

> Just to be clear about the status of this patch, I don't
> have a problem with the code changes, but it does definitely
> need a much clearer commit message to explain why we're changing
> the way we handle the PCI controller. So I'm dropping this from
> my to-review list on the assumption there will be a v2.

Sorry for delay. There will be v2 version of it. I am busy with sorting 
out EDK2 side of GIC ITS enablement.

> We could also do with expanding the commentary in the source
> file to clarify the design approach we're using w.r.t. what
> we do and don't want to put into the "dt" blob, but that would
> probably be best in a different patch.

Already on todo list.