From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f47.google.com (mail-wm1-f47.google.com [209.85.128.47]) by mx.groups.io with SMTP id smtpd.web08.131.1612804121103217834 for ; Mon, 08 Feb 2021 09:08:41 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=sAeYdUhe; spf=pass (domain: akeo.ie, ip: 209.85.128.47, mailfrom: pete@akeo.ie) Received: by mail-wm1-f47.google.com with SMTP id m1so13893560wml.2 for ; Mon, 08 Feb 2021 09:08:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=eXtW0xMtOV5pkYlu0dzxYDQ6mALvpRIXiejQWSXCF4k=; b=sAeYdUhe/MBmmZaHDjK5+eHTmwocbgFtzShSr4wcxF/WWYnxVX0l7M4yQvK5ykVY6A UKvDGn+k68JeAAY6b5Nd25oiqu0NkCcAM12cLttl8YhJrdKQYZO79qgyZ8yacE1ocgye Gtne3kUn7/MSl8JI6lnt12WdekEPb97FJ/6PJltjsJjxEFRgyxTvPrwolqj3SF/Ur0n6 fTdndgYPeUgQr9w35mVnv1BuiBkg2byz1MJp22c599y3nHeDohdanHhSziCHnr/gw5xI f1g+ZyKzyzhrAxRO7vLLAVCduAJ7tE+3MZdm+l1JFySsZCuvW6y//qbCTUu5ul0HNRM6 ay6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=eXtW0xMtOV5pkYlu0dzxYDQ6mALvpRIXiejQWSXCF4k=; b=A2koc7GPbrvkNqETbbgyDZJ9ZqdsP0e/O40ApBdBWWz77NSXHDHGxu6QoRjYOimWK1 U8I8pjfUZy02bE2dJ5y+B1Sk/jnQ66smVqzc6x5aiPVoqyuExBGufpnlJnNMp+vAmFAk YUUhUg411ABxF5ueKfKCcBld9UsIi2E3WqUP6gTN0t0ezc1shqrpg5eUJ8GBgv4mhGXr 6sQMIrk/lfmWvxrhb9476gDlj5cNhxtxcoNpPeaoWqbHIfuHLLEtPOc2WmXHojb6ryj7 J3kszvomUiQwIDY8J9N3lFfDvRMiXgihOcfXISsiWCXioiH0gbe1mR4rSsP6893Twz3q i+fA== X-Gm-Message-State: AOAM532tr4NGNSJXzTzADqihREKbJ0BCov1KcbhMhYS4yMIRzZUzLYm/ Mxe8zMKKwgZqd5GDevrxmdgCQA== X-Google-Smtp-Source: ABdhPJyIFR8tNatJ1qFG8rP0/rb4yGL7MCqlXdTHM8OovTnF3DZkIk7dUnWDR0FIz6AXL60FECTJ9Q== X-Received: by 2002:a1c:39d5:: with SMTP id g204mr15045719wma.127.1612804118129; Mon, 08 Feb 2021 09:08:38 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.77.31]) by smtp.googlemail.com with ESMTPSA id d10sm29402757wrn.88.2021.02.08.09.08.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 08 Feb 2021 09:08:37 -0800 (PST) Subject: Re: [PATCH] Platform/RaspberryPi/Acpitables: Add eMMC2 device and tweak Arasan To: Jeremy Linton , devel@edk2.groups.io Cc: awarkentin@vmware.com, samer.el-haj-mahmoud@arm.com, leif@nuviainc.com, ardb+tianocore@kernel.org References: <20210201225343.2001835-1-jeremy.linton@arm.com> <20210201225343.2001835-3-jeremy.linton@arm.com> From: "Pete Batard" Message-ID: Date: Mon, 8 Feb 2021 17:08:36 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <20210201225343.2001835-3-jeremy.linton@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit On 2021.02.01 22:53, Jeremy Linton wrote: > The primarly problem with the rpi Arasan controller working Small typo: primarly -> primary > with a default SDHCI driver is the lack of a meaningful > capabilities register. As such if we add a _DSD entry > to provide that information, we can then bind it to > the linux sdhci_iproc driver which already > hardcodes the remaining controller bugs. > > Further we have gotten BRCME88C approved as the HID > for the newer eMMC2 controller. So lets define an > ACPI object to describe it. > > Of course both devices are sharing an interrupt so > we should also indicate that in the table as well. > > Signed-off-by: Jeremy Linton > --- > Platform/RaspberryPi/AcpiTables/AcpiTables.inf | 1 + > Platform/RaspberryPi/AcpiTables/Emmc.asl | 131 +++++++++++++++++++++ > Platform/RaspberryPi/AcpiTables/Sdhc.asl | 18 ++- > Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 6 + > 4 files changed, 153 insertions(+), 3 deletions(-) > create mode 100644 Platform/RaspberryPi/AcpiTables/Emmc.asl > > diff --git a/Platform/RaspberryPi/AcpiTables/AcpiTables.inf b/Platform/RaspberryPi/AcpiTables/AcpiTables.inf > index d2cce074e5..743261afcf 100644 > --- a/Platform/RaspberryPi/AcpiTables/AcpiTables.inf > +++ b/Platform/RaspberryPi/AcpiTables/AcpiTables.inf > @@ -35,6 +35,7 @@ > Spcr.aslc > > Pptt.aslc > > SsdtThermal.asl > > + Emmc.asl > > > > [Packages] > > ArmPkg/ArmPkg.dec > > diff --git a/Platform/RaspberryPi/AcpiTables/Emmc.asl b/Platform/RaspberryPi/AcpiTables/Emmc.asl > new file mode 100644 > index 0000000000..f089068556 > --- /dev/null > +++ b/Platform/RaspberryPi/AcpiTables/Emmc.asl > @@ -0,0 +1,131 @@ > +/** @file > + * > + * Copyright (c) 2021 Arm. All rights reserved. > + * > + * SPDX-License-Identifier: BSD-2-Clause-Patent > + * > + **/ > + > +#include > +#include > + > +#include "AcpiTables.h" > + > +DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4EMMC", 2) > +{ > + Scope (\_SB_) > + { > +#if (RPI_MODEL == 4) > + Device (GDV1) { > + Name (_HID, "ACPI0004") > + Name (_UID, 0x0) > + Name (_CCA, 0x0) > + > + Name (RBUF, ResourceTemplate () > + { > + MEMORY32FIXED (ReadWrite, 0, MMCHS2_LENGTH, RMEM) > + }) > + Method (_CRS, 0x0, Serialized) > + { > + MEMORY32SETBASE (RBUF, RMEM, RBAS, MMCHS2_OFFSET) > + Return (^RBUF) > + } > + > + Name (_DMA, ResourceTemplate() { > + /* > + * XHC0 is limited to DMA to first 3GB. Note this > + * only applies to PCIe, not GENET or other devices > + * next to the A72. > + */ I don't think this comment, that appears to have been lifted from a copy/paste of https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/AcpiTables/Xhci.asl#L62-L82, applies here, since you are no longer describing a [0x00000000, 0xbfffffff] range here (or dealing with the XHC0 ACPI device) but applying a translation. I would either remove the comment or make it explain why the translation is needed. > + QWordMemory (ResourceConsumer, > + , > + MinFixed, > + MaxFixed, > + NonCacheable, > + ReadWrite, > + 0x0, > + 0x00000000C0000000, // MIN > + 0x00000000FFFFFFFF, // MAX > + 0xFFFFFFFF40000000, // TRA > + 0x0000000040000000, // LEN > + , > + , > + ) > + }) > + > + // emmc2 Host Controller. (brcm,bcm2711-emmc2) > + Device (SDC3) > + { > + Name (_HID, "BRCME88C") > + Name (_UID, 0x1) vvvvvvvvvvvv > + Name (_CCA, 0x0) > + Name (_S1D, 0x1) > + Name (_S2D, 0x1) > + Name (_S3D, 0x1) > + Name (_S4D, 0x1) > + Name (SDMA, 0x2) > + Method (_STA) > + { > + Return(0xf) > + } > + Name (RBUF, ResourceTemplate () > + { > + MEMORY32FIXED (ReadWrite, 0, MMCHS2_LENGTH, RMEM) > + Interrupt (ResourceConsumer, Level, ActiveHigh, Shared) { BCM2836_MMCHS1_INTERRUPT } > + }) > + Method (_CRS, 0x0, Serialized) > + { > + MEMORY32SETBASE (RBUF, RMEM, RBAS, MMCHS2_OFFSET) > + Return (^RBUF) > + } ^^^^^^^^^^^^^ Section above uses tabs instead of spaces. I believe edk2/BaseTools/Scripts/PatchCheck.py, when run, should warn you about this. > + > + // Unfortunatly this controller doesn't honor the Small typo: Unfortunatly -> Unfortunately > + // standard sdhci voltage control registers > + // (or at least linux's standard code can't > + // lower the voltage) So, UHS mode is disabled with caps > + Name (DSD1, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package () { "sdhci-caps-mask", 0x0000000500080000 }, > + } > + }) vvvvvvvvvvvv > + // We also disable both SDMA and ADMA2 until the linux > + // _DMA() mask/translate works properly ^^^^^^^^^^^^^ Another tabbed section Also, the "also" above seems to imply that DSD1 applies on top of DSD2, whereas, per the code further down, we only apply one of DSD1 or DSD2 according to SDMA. Maybe, for clarity, this should be mentioned more explicitly in the comments above? > + Name (DSD2, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package () { "sdhci-caps-mask", 0x0000000504480000 }, > + } ^^^^^^^^^^^^^ Another tabbed line > + }) > + Method (_DSD, 0x0, Serialized) ^^^^^^^^^^^^^ Another tabbed line > + { > + if (SDMA == 0) > + { > + return (^DSD2) > + } > + else > + { > + return (^DSD1) > + } > + } > + > + // > + // A child device that represents the > + // sd card, which is marked as non-removable. > + // > + Device (SDMM) > + { > + Method (_ADR) > + { > + Return (0) > + } > + Method (_RMV) // Is removable > + { > + Return (0) // 0 - fixed > + } > + } > + } //SDC3 > + } //GDV1 > +#endif > + } //\SB > +} > diff --git a/Platform/RaspberryPi/AcpiTables/Sdhc.asl b/Platform/RaspberryPi/AcpiTables/Sdhc.asl > index 0ab1ba27f2..0430ab7d2d 100644 > --- a/Platform/RaspberryPi/AcpiTables/Sdhc.asl > +++ b/Platform/RaspberryPi/AcpiTables/Sdhc.asl > @@ -19,7 +19,7 @@ > // Note: UEFI can use either SDHost or Arasan. We expose both to the OS. > > // > > > > -// ArasanSD 3.0 SD Host Controller. > > +// ArasanSD 3.0 SD Host Controller. (brcm,bcm2835-sdhci) > > Device (SDC1) > > { > > Name (_HID, "BCM2847") > > @@ -37,7 +37,7 @@ Device (SDC1) > Name (RBUF, ResourceTemplate () > > { > > MEMORY32FIXED (ReadWrite, 0, MMCHS1_LENGTH, RMEM) > > - Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { BCM2836_MMCHS1_INTERRUPT } > > + Interrupt (ResourceConsumer, Level, ActiveHigh, Shared) { BCM2836_MMCHS1_INTERRUPT } > > }) > > Method (_CRS, 0x0, Serialized) > > { > > @@ -45,6 +45,17 @@ Device (SDC1) > Return (^RBUF) > > } > > > > + // The standard CAPs registers on this controller > > + // appear to be 0, lets set some minimal defaults > > + // Since this cap doesn't indicate DMA capability > > + // we don't need a _DMA() > > + Name (_DSD, Package () { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package () { > > + Package () { "sdhci-caps", 0x0100fa81 }, > > + } > > + }) > > + > > // > > // A child device that represents the > > // sd card, which is marked as non-removable. > > @@ -62,7 +73,7 @@ Device (SDC1) > } > > } > > > > - > > +#if (RPI_MODEL < 4) > > // Broadcom SDHost 2.0 SD Host Controller > > Device (SDC2) > > { > > @@ -105,3 +116,4 @@ Device (SDC2) > } > > } > > } > > +#endif // !RPI4 > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > index ca7533cbee..7f26f7b4be 100644 > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > @@ -738,6 +738,12 @@ STATIC CONST NAMESPACE_TABLES SdtTables[] = { > SsdtNameOpReplace > > }, > > { > > + SIGNATURE_64 ('R', 'P', 'I', '4', 'E', 'M', 'M', 'C'), > > + 0, > > + PcdToken(PcdSdIsArasan), > > + NULL > > + }, > > + { > > SIGNATURE_64 ('R', 'P', 'I', 0, 0, 0, 0, 0), > > 0, > > 0, > With all of the above being formatting/comment/typos issues: Reviewed-by: Pete Batard