[edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64

Chao Li posted 30 patches 2 years, 3 months ago
There is a newer version of this series
[edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64
Posted by Chao Li 2 years, 3 months ago
Added LoongArch64 architecture CPU IO width.

https://bugzilla.tianocore.org/show_bug.cgi?id=4584

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@amd.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Signed-off-by: Chao Li <lichao@loongson.cn>
---
 EmbeddedPkg/EmbeddedPkg.dec | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 341ef5e6a6..241d4f3acc 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -165,6 +165,9 @@
 [PcdsFixedAtBuild.X64]
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x00000011
 
+[PcdsFixedAtBuild.LOONGARCH64]
+  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x00000011
+
 [PcdsFixedAtBuild.common, PcdsDynamic.common]
   #
   # Value to add to a host address to obtain a device address, using
-- 
2.27.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110719): https://edk2.groups.io/g/devel/message/110719
Mute This Topic: https://groups.io/mt/102413876/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64
Posted by Leif Lindholm 2 years, 2 months ago
On 2023-11-06 03:29, Chao Li wrote:
> Added LoongArch64 architecture CPU IO width.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=4584
> 
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Daniel Schaefer <git@danielschaefer.me>
> Signed-off-by: Chao Li <lichao@loongson.cn>

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

I note that as a result of this we are now definining this token 
individually for 5 different architectures, in order to provide two 
different default values. We should probably look at consolidating 
those, but that responsibility doesn't have to land on this set.

/
     Leif

> ---
>   EmbeddedPkg/EmbeddedPkg.dec | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 341ef5e6a6..241d4f3acc 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -165,6 +165,9 @@
>   [PcdsFixedAtBuild.X64]
>     gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x00000011
>   
> +[PcdsFixedAtBuild.LOONGARCH64]
> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x00000011
> +
>   [PcdsFixedAtBuild.common, PcdsDynamic.common]
>     #
>     # Value to add to a host address to obtain a device address, using



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111289): https://edk2.groups.io/g/devel/message/111289
Mute This Topic: https://groups.io/mt/102413876/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64
Posted by Pedro Falcato 2 years, 2 months ago
On Wed, Nov 15, 2023 at 6:55 PM Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On 2023-11-06 03:29, Chao Li wrote:
> > Added LoongArch64 architecture CPU IO width.
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=4584
> >
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Abner Chang <abner.chang@amd.com>
> > Cc: Daniel Schaefer <git@danielschaefer.me>
> > Signed-off-by: Chao Li <lichao@loongson.cn>
>
> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
>
> I note that as a result of this we are now definining this token
> individually for 5 different architectures, in order to provide two
> different default values. We should probably look at consolidating
> those, but that responsibility doesn't have to land on this set.
>
> /
>      Leif
>
> > ---
> >   EmbeddedPkg/EmbeddedPkg.dec | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> > index 341ef5e6a6..241d4f3acc 100644
> > --- a/EmbeddedPkg/EmbeddedPkg.dec
> > +++ b/EmbeddedPkg/EmbeddedPkg.dec
> > @@ -165,6 +165,9 @@
> >   [PcdsFixedAtBuild.X64]
> >     gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x00000011
> >
> > +[PcdsFixedAtBuild.LOONGARCH64]
> > +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x00000011
> > +

Leif,

Can you clarify the meaning of PcdPrePiCpuIoSize? I was thinking it's
supposed to be the size of the port-mapped IO for the
architecture/platform (as hinted by in X64 = IA32 = 16, and ARM=0),
but from a quick git grep I can tell that
1) most platforms define it to something else (ArmVirt defines it to
16, real platforms have a plethora of other values)
2) gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize has no internal consumer
in EmbeddedPkg, but only in ArmPlatformPkg and LoongArchQemuPkg (and
BeagleBoardPkg's PrePi)
3) Platform/Loongson/LoongArchQemuPkg/Loongson.dec:
gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|0|UINT8|0x00010001 <-- ??

and FWIW, LoongArch does not seem to have port-mapped IO at all.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111303): https://edk2.groups.io/g/devel/message/111303
Mute This Topic: https://groups.io/mt/102413876/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64
Posted by Leif Lindholm 2 years, 2 months ago
On 2023-11-16 08:15, Pedro Falcato wrote:
> Leif,
> 
> Can you clarify the meaning of PcdPrePiCpuIoSize?

Not out of memory, so let's go exploring.

 > I was thinking it's
 > supposed to be the size of the port-mapped IO for the
 > architecture/platform (as hinted by in X64 = IA32 = 16, and ARM=0),
 > but from a quick git grep I can tell that
 > 1) most platforms define it to something else (ArmVirt defines it to
 > 16, real platforms have a plethora of other values)
 > 2) gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize has no internal consumer
 > in EmbeddedPkg, but only in ArmPlatformPkg and LoongArchQemuPkg (and
 > BeagleBoardPkg's PrePi)

ArmVirtPkg reads this Pcd in
https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/PrePi/PrePi.c#L87
which is in a call to BuildCpuHob, in
https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Library/PrePiHobLib/Hob.c#L644
which uses the argument to initialize the SizeOfIoSpace in EFI_HOB_CPU
(defined in PI spec).

This eventually gets dug up in
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Gcd/Gcd.c#L2559
where it is used to set up the GCD I/O Space Map.
This is also the only way to find out it describes a power of 2. Neither 
the spec nor any of the comment headers in the codebase describe what 
the size means. Tsk, tsk.

Ultimately, this ends up being used in
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c#L175
during InitializePciHostBridge ().

So I'm guessing, and I think it rings a bell, that this is needed to 
enable PCIe I/O resources to be assigned, which *can* be supported on 
Arm and probably Loongson systems through magic memory regions trapping 
accesses in the PCIe IP.

> 3) Platform/Loongson/LoongArchQemuPkg/Loongson.dec:
> gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|0|UINT8|0x00010001 <-- ??

Yeah, it's not great that we don't seem to have a way to trap when Pcd's 
get redefined outside of their own namespace. That happens way too 
frequently. That's certainly a bug.

> and FWIW, LoongArch does not seem to have port-mapped IO at all.

So I guess the setting comes down to whether that is considered to be 
the expected behaviour on Loongson platforms or not. About which I have 
no idea and am happy to trust the author on :)

/
     Leif



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111307): https://edk2.groups.io/g/devel/message/111307
Mute This Topic: https://groups.io/mt/102413876/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64
Posted by Chao Li 2 years, 2 months ago
Hi Pedro,

I think this size is the CPU IO or PCI IO space width, like you saied, 
LoongArch doesn't mapped the IO ports, but it has IO area.

For example, it can map the LPC IO or PCI IO ports onto the physical 
addres space and register the CPU IO or PCI IO protocols to access them.

The *Loongson.dsc* of edk2-platforms sets this value to 32. I think this 
value is too wide since most IO device only use 16-bit width, so I 
changed this value from 32 to 16 at this point.


Thanks,
Chao
On 2023/11/16 16:15, Pedro Falcato wrote:
> On Wed, Nov 15, 2023 at 6:55 PM Leif Lindholm<quic_llindhol@quicinc.com>  wrote:
>> On 2023-11-06 03:29, Chao Li wrote:
>>> Added LoongArch64 architecture CPU IO width.
>>>
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=4584
>>>
>>> Cc: Leif Lindholm<quic_llindhol@quicinc.com>
>>> Cc: Ard Biesheuvel<ardb+tianocore@kernel.org>
>>> Cc: Abner Chang<abner.chang@amd.com>
>>> Cc: Daniel Schaefer<git@danielschaefer.me>
>>> Signed-off-by: Chao Li<lichao@loongson.cn>
>> Reviewed-by: Leif Lindholm<quic_llindhol@quicinc.com>
>>
>> I note that as a result of this we are now definining this token
>> individually for 5 different architectures, in order to provide two
>> different default values. We should probably look at consolidating
>> those, but that responsibility doesn't have to land on this set.
>>
>> /
>>       Leif
>>
>>> ---
>>>    EmbeddedPkg/EmbeddedPkg.dec | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
>>> index 341ef5e6a6..241d4f3acc 100644
>>> --- a/EmbeddedPkg/EmbeddedPkg.dec
>>> +++ b/EmbeddedPkg/EmbeddedPkg.dec
>>> @@ -165,6 +165,9 @@
>>>    [PcdsFixedAtBuild.X64]
>>>      gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x00000011
>>>
>>> +[PcdsFixedAtBuild.LOONGARCH64]
>>> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x00000011
>>> +
> Leif,
>
> Can you clarify the meaning of PcdPrePiCpuIoSize? I was thinking it's
> supposed to be the size of the port-mapped IO for the
> architecture/platform (as hinted by in X64 = IA32 = 16, and ARM=0),
> but from a quick git grep I can tell that
> 1) most platforms define it to something else (ArmVirt defines it to
> 16, real platforms have a plethora of other values)
> 2) gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize has no internal consumer
> in EmbeddedPkg, but only in ArmPlatformPkg and LoongArchQemuPkg (and
> BeagleBoardPkg's PrePi)
> 3) Platform/Loongson/LoongArchQemuPkg/Loongson.dec:
> gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|0|UINT8|0x00010001 <-- ??
>
> and FWIW, LoongArch does not seem to have port-mapped IO at all.
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111306): https://edk2.groups.io/g/devel/message/111306
Mute This Topic: https://groups.io/mt/102413876/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-