From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web08.27675.1610329541915259420 for ; Sun, 10 Jan 2021 17:45:42 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: jeremy.linton@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 84960101E; Sun, 10 Jan 2021 17:45:40 -0800 (PST) Received: from [192.168.122.166] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 67A843F66E; Sun, 10 Jan 2021 17:45:40 -0800 (PST) Subject: Re: [edk2-devel] [PATCH 2/3] Platform/RaspberryPi/Acpitables: Add eMMC2 device and tweak Arasan To: devel@edk2.groups.io, mark.kettenis@xs4all.nl References: <11f288a0-aab8-bdca-de7e-b3a4bb9f1469@arm.com> <7393.1610201355597676892@groups.io> From: "Jeremy Linton" Message-ID: <40b30cb8-aa00-735b-f535-a0f37142d1b0@arm.com> Date: Sun, 10 Jan 2021 19:45:32 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <7393.1610201355597676892@groups.io> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi, On 1/9/21 8:09 AM, Mark Kettenis via groups.io wrote: > On Fri, Jan 8, 2021 at 05:27 PM, Jeremy Linton wrote: >=20 >> >> Hi, >> >> On 1/8/21 2:28 AM, Ard Biesheuvel wrote: >> >>> On 1/8/21 7:16 AM, Jeremy Linton wrote: >>> >>>> The primarly problem with the rpi Arasan controller working >>>> 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/Dsdt.asl | 2 + >>>> Platform/RaspberryPi/AcpiTables/Sdhc.asl | 86 >>>> +++++++++++++++++++++++++++++++- >>>> 2 files changed, 86 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Platform/RaspberryPi/AcpiTables/Dsdt.asl >>>> b/Platform/RaspberryPi/AcpiTables/Dsdt.asl >>>> index d116f965e1..cca08c0539 100644 >>>> --- a/Platform/RaspberryPi/AcpiTables/Dsdt.asl >>>> +++ b/Platform/RaspberryPi/AcpiTables/Dsdt.asl >>>> @@ -150,6 +150,7 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "RPIFDN", >>>> "RPI", 2) >>>> QWORDMEMORYBUF(23) >>>> QWORDMEMORYBUF(24) >>>> QWORDMEMORYBUF(25) >>>> + QWORDMEMORYBUF(26) >>>> }) >>>> >>>> // USB >>>> @@ -196,6 +197,7 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "RPIFDN", >>>> "RPI", 2) >>>> // SDC >>>> QWORDMEMORYSET(24, MMCHS1_OFFSET, MMCHS1_LENGTH) >>>> QWORDMEMORYSET(25, SDHOST_OFFSET, SDHOST_LENGTH) >>>> + QWORDMEMORYSET(26, MMCHS2_OFFSET, MMCHS2_LENGTH) >>>> >>>> Return (RBUF) >>>> } >>>> diff --git a/Platform/RaspberryPi/AcpiTables/Sdhc.asl >>>> b/Platform/RaspberryPi/AcpiTables/Sdhc.asl >>>> index 0ab1ba27f2..a7ac831066 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,86 @@ 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. >>>> + // >>>> + Device (SDMM) >>>> + { >>>> + Method (_ADR) >>>> + { >>>> + Return (0) >>>> + } >>>> + Method (_RMV) // Is removable >>>> + { >>>> + Return (0) // 0 - fixed >>>> + } >>>> + } >>>> +} >>>> + >>>> +#if (RPI_MODEL =3D=3D 4) >>>> +// emmc2 Host Controller. (brcm,bcm2711-emmc2) >>>> +Device (SDC3) >>>> +{ >>>> + Name (_HID, "BRCME88C") >>>> + Name (_UID, 0x1) >>>> + Name (_CCA, 0x0) >>>> + Name (_S1D, 0x1) >>>> + Name (_S2D, 0x1) >>>> + Name (_S3D, 0x1) >>>> + Name (_S4D, 0x1) >>>> + 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) >>>> + } >>>> + >>>> + // forceably limit to 1G >>>> + Name (_DMA, ResourceTemplate() { >>>> + QWordMemory (ResourceConsumer, >>>> + , >>>> + MinFixed, >>>> + MaxFixed, >>>> + NonCacheable, >>>> + ReadWrite, >>>> + 0x0, >>>> + 0x0, // MIN >>>> + 0x3fffffff, // MAX >>>> + 0x0, // TRA >>>> + 0x40000000, // LEN >>>> + , >>>> + , >>>> + ) >>>> + }) >>>> + >>> >>> Shouldn't the _DMA method be on a container object instead of on the >>> device object itself? >> >> Pained cough... There is a _DMA on the gpu devs container of which this >> device belongs. That doesn't make the DMA work. The spec says that _DMA >> is to exist on the bus device, and I think what is happening here is >> that the "device" is the SDMM (which isn't directly visible because of >> the way diff broke this patch up) contained in the "SDC3 mmc bus". >=20 > The Linux device tree has the following comment: >=20 > /* > * emmc2 has different DMA constraints based on SoC revisions. It was > * moved into its own bus, so as for RPi4's firmware to update them. > * The firmware will find whether the emmc2bus alias is defined, and if > * so, it'll edit the dma-ranges property below accordingly. > * >=20 > And the dma-ranges property it references is: >=20 > dma-ranges =3D <0x0 0xc0000000=C2=A0 0x0 0x00000000=C2=A0 0x40000000>; >=20 > Not sure how the firmware will adjust this, but it does imply that there= is actual translation > going on here and it doesn't match the _DMA property you added. >=20 >> >> Frankly, I could be wrong, but what I do know is that without that bit >> of ugliness DMA doesn't work in linux on this controller. I just >> stripped it out again to verify the failure with 4.11. It does the same >> thing, the controller itself seems to be working but the block device >> associated with it can't be read. There are few other possibilities >> (like maybe this controller shouldn't be a gpudevs child) and i'm fixin= g >> a bug I created..I had kernel instrumentation in 5.9 tracking the dma >> mask propagation, and I was fairly confident in this a couple months >> ago, less so now. >> >=20 > One question I have is whether _DMA methods should be applied recursivel= y or > not. If you have a bus that provides a translating _DMA method that is b= ehind a > bus that also has a _DMA method, should the translation specified in tha= t first _DMA > method be applied or not? The text in the ACPI spec is far for clear her= ... First, thanks for looking at this! The way I understand the spec, one does have to honor nested translations. >=20 > If I read the Linux code in drivers/acpi/scan.c correctly, Linux would n= ot apply > the translation in this case. Since the _DMA method you add does not spe= cify any > translation, I think that means the required address translation isn't h= appening > which means that DMA transactions would fail. I suspect the block device= behind > the controller would still be detected, but any actual I/O would fail si= nce the right > memory isn't addressed. >=20 > So I think the best thing to do would be to have a separate ACPI0004 con= tainer > with a translating _DMA method and but the emmc2 device below that. I think if you look at the rpi's Dsdt, you will see an ACPI0004=20 container, which includes the GpuDevs, of which Sdhc.asl is child. That=20 container has a _DMA method which specifies the 1G translation+limit=20 which matches what you would expect based on the DT. So this further _DMA should be layering on it and re-enforcing only the=20 DMA limit, not a new translation. >=20 >> >> >>> >>>> + Name (_DSD, Package () { >>>> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >>>> + Package () { >>>> + Package () { "sdhci-caps-mask", 0x0000000000080000 }, >>>> + } >>>> + }) >>>> + >>>> // >>>> // A child device that represents the >>>> // sd card, which is marked as non-removable. >>>> @@ -62,6 +142,7 @@ Device (SDC1) >>>> } >>>> } >>>> >>>> +#else // !RPI4 >>>> >>>> // Broadcom SDHost 2.0 SD Host Controller >>>> Device (SDC2) >>>> @@ -105,3 +186,4 @@ Device (SDC2) >>>> } >>>> } >>>> } >>>> +#endif // !RPI4 >>> >>> >> >> >=20 >=20 >=20 >=20 >=20 >=20