From: "Leif Lindholm" <quic_llindhol@quicinc.com>
To: <devel@edk2.groups.io>, <wangyuquan1236@phytium.com.cn>
Cc: <ardb+tianocore@kernel.org>, <chenbaozi@phytium.com.cn>
Subject: Re: [edk2-devel] [PATCH edk2-platforms v3 1/1] Platform/SbsaQemu: add XHCI support and replace EHCI
Date: Wed, 27 Sep 2023 11:30:23 +0100 [thread overview]
Message-ID: <ZRQEP5xNdde2lnrw@qc-i7.hemma.eciton.net> (raw)
In-Reply-To: <20230922134115.326726-2-wangyuquan1236@phytium.com.cn>
Hi Yuquan,
I am mostly happy with the below, but there are a couple of things I'd
like to change (comments inline).
On Fri, Sep 22, 2023 at 21:41:15 +0800, Yuquan Wang wrote:
> As sbsa-ref does not have DRAM below 4G, it cannot utilize EHCI
> that only has 32-bit DMA capablity and sbsa-ref board uses xhci
> to replace ehci.
>
> This updates DSDT to match the platform xhci controller with two
> usb ports changed from type A to type C.
>
> This also registers the non-discoverable XHCI for sbsa-ref.
Ah, I failed to notice this on previous review. Since we didn't map
EHCI at all (probably because it was non-functioning), this
registering becomes a separate change, which should be in a separate
patch.
This is just mechanics, so I'll rework it myself.
> Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> ---
> Silicon/Qemu/SbsaQemu/SbsaQemu.dec | 4 +-
> Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 6 +--
> .../Qemu/SbsaQemu/AcpiTables/AcpiTables.inf | 4 +-
> .../SbsaQemuPlatformDxe.inf | 2 +
> .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c | 41 +++++++++++++++----
> Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl | 15 +++----
> 6 files changed, 49 insertions(+), 23 deletions(-)
>
> diff --git a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> index aab2894e6455..913d1d75ef29 100644
> --- a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> +++ b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> @@ -32,8 +32,8 @@ [PcdsFixedAtBuild.common]
> # Non discoverable devices Pcds
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciBase|0|UINT64|0x00000001
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciSize|0x10000|UINT32|0x00000002
> - gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformEhciBase|0|UINT64|0x00000003
> - gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformEhciSize|0x10000|UINT32|0x00000004
> + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformXhciBase|0|UINT64|0x00000003
> + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformXhciSize|0x10000|UINT32|0x00000004
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress|0x10000000000|UINT64|0x00000005
>
> # PCDs complementing PCIe layout pulled into ACPI tables
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> index be406144c242..948e42326c33 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> @@ -429,11 +429,11 @@ [PcdsFixedAtBuild.common]
> # Initial Device Tree Location
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress|0x10000000000
>
> - # Non discoverable devices (AHCI,EHCI)
> + # Non discoverable devices (AHCI,XHCI)
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciBase|0x60100000
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciSize|0x00010000
> - gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformEhciBase|0x60110000
> - gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformEhciSize|0x00010000
> + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformXhciBase|0x60110000
> + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformXhciSize|0x00010000
>
> # PL011 - Serial Terminal
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0x60000000
> diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf b/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
> index 97021f7971c7..abd10b6c3098 100644
> --- a/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
> +++ b/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
> @@ -69,8 +69,8 @@ [FixedPcd]
>
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciBase
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciSize
> - gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformEhciBase
> - gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformEhciSize
> + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformXhciBase
> + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformXhciSize
>
> [Pcd]
> gArmTokenSpaceGuid.PcdGicDistributorBase
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
> index 0e3b11d60426..19534b7a274a 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
> @@ -37,6 +37,8 @@ [LibraryClasses]
> [Pcd]
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciBase
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformAhciSize
> + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformXhciBase
> + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformXhciSize
>
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor
> gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
> index ddcca2b7243c..4a04dbeb07ae 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
> @@ -26,8 +26,9 @@ InitializeSbsaQemuPlatformDxe (
> )
> {
> EFI_STATUS Status;
> - UINTN Size;
> - VOID* Base;
> + UINTN AhciSize, XhciSize;
> + VOID* AhciBase;
> + VOID* XhciBase;
There is no need for AhciSize/Base and XhciSize/Base to be live
simultaneously in the code. And the patch becomes much cleaner if we
cimply reuse the existing Base/Size variables.
Again, that is purely mechanical, so I have reworked it myself.
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
Pushed as cbe0e979e45a..d03a60523a60.
Thanks!
/
Leif
> UINTN Arg0;
> UINTN Arg1;
> UINTN SmcResult;
> @@ -35,13 +36,13 @@ InitializeSbsaQemuPlatformDxe (
>
> DEBUG ((DEBUG_INFO, "%a: InitializeSbsaQemuPlatformDxe called\n", __FUNCTION__));
>
> - Base = (VOID*)(UINTN)PcdGet64 (PcdPlatformAhciBase);
> - ASSERT (Base != NULL);
> - Size = (UINTN)PcdGet32 (PcdPlatformAhciSize);
> - ASSERT (Size != 0);
> + AhciBase = (VOID*)(UINTN)PcdGet64 (PcdPlatformAhciBase);
> + ASSERT (AhciBase != NULL);
> + AhciSize = (UINTN)PcdGet32 (PcdPlatformAhciSize);
> + ASSERT (AhciSize != 0);
>
> DEBUG ((DEBUG_INFO, "%a: Got platform AHCI %llx %u\n",
> - __FUNCTION__, Base, Size));
> + __FUNCTION__, AhciBase, AhciSize));
>
> Status = RegisterNonDiscoverableMmioDevice (
> NonDiscoverableDeviceTypeAhci,
> @@ -49,11 +50,33 @@ InitializeSbsaQemuPlatformDxe (
> NULL,
> NULL,
> 1,
> - Base, Size);
> + AhciBase, AhciSize);
>
> if (EFI_ERROR(Status)) {
> DEBUG ((DEBUG_ERROR, "%a: NonDiscoverable: Cannot install AHCI device @%p (Staus == %r)\n",
> - __FUNCTION__, Base, Status));
> + __FUNCTION__, AhciBase, Status));
> + return Status;
> + }
> +
> + XhciBase = (VOID*)(UINTN)PcdGet64 (PcdPlatformXhciBase);
> + ASSERT (XhciBase != NULL);
> + XhciSize = (UINTN)PcdGet32 (PcdPlatformXhciSize);
> + ASSERT (XhciSize != 0);
> +
> + DEBUG ((DEBUG_INFO, "%a: Got platform XHCI %llx %u\n",
> + __FUNCTION__, XhciBase, XhciSize));
> +
> + Status = RegisterNonDiscoverableMmioDevice (
> + NonDiscoverableDeviceTypeXhci,
> + NonDiscoverableDeviceDmaTypeCoherent,
> + NULL,
> + NULL,
> + 1,
> + XhciBase, XhciSize);
> +
> + if (EFI_ERROR(Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: NonDiscoverable: Cannot install XHCI device @%p (Staus == %r)\n",
> + __FUNCTION__, XhciBase, Status));
> return Status;
> }
>
> diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl b/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
> index e50772fcf76d..543b5782580a 100644
> --- a/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
> +++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
> @@ -68,18 +68,19 @@ DefinitionBlock ("DsdtTable.aml", "DSDT",
> }
> }
>
> - // USB EHCI Host Controller
> + // USB XHCI Host Controller
> Device (USB0) {
> - Name (_HID, "LNRO0D20")
> - Name (_CID, "PNP0D20")
> + Name (_HID, "PNP0D10") // _HID: Hardware ID
> + Name (_UID, 0x00) // _UID: Unique ID
> + Name (_CCA, 0x01) // _CCA: Cache Coherency Attribute
> Method (_STA) {
> Return (0xF)
> }
> Method (_CRS, 0x0, Serialized) {
> Name (RBUF, ResourceTemplate() {
> Memory32Fixed (ReadWrite,
> - FixedPcdGet32 (PcdPlatformEhciBase),
> - FixedPcdGet32 (PcdPlatformEhciSize))
> + FixedPcdGet32 (PcdPlatformXhciBase),
> + FixedPcdGet32 (PcdPlatformXhciSize))
> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 43 }
> })
> Return (RBUF)
> @@ -146,7 +147,7 @@ DefinitionBlock ("DsdtTable.aml", "DSDT",
> Name (_ADR, 0x00000003)
> Name (_UPC, Package() {
> 0xFF, // Port is connectable
> - 0x00, // Port connector is A
> + 0x09, // Type C connector - USB2 and SS with Switch
> 0x00000000,
> 0x00000000
> })
> @@ -165,7 +166,7 @@ DefinitionBlock ("DsdtTable.aml", "DSDT",
> Name (_ADR, 0x00000004)
> Name (_UPC, Package() {
> 0xFF, // Port is connectable
> - 0x00, // Port connector is A
> + 0x09, // Type C connector - USB2 and SS with Switch
> 0x00000000,
> 0x00000000
> })
> --
> 2.34.1
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109118): https://edk2.groups.io/g/devel/message/109118
Mute This Topic: https://groups.io/mt/101521163/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
prev parent reply other threads:[~2023-09-27 10:30 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-22 13:41 [edk2-devel] [PATCH edk2-platforms v3 0/1] Platform/SbsaQemu: use XHCI to replace EHCI Yuquan Wang
2023-09-22 13:41 ` [edk2-devel] [PATCH edk2-platforms v3 1/1] Platform/SbsaQemu: add XHCI support and " Yuquan Wang
2023-09-27 9:19 ` Marcin Juszkiewicz
2023-09-27 10:30 ` Leif Lindholm [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZRQEP5xNdde2lnrw@qc-i7.hemma.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox