On Mon, Jan 11, 2021 at 02:45 AM, Jeremy Linton wrote:<br />
<blockquote>Hi,<br /><br />On 1/9/21 8:09 AM, Mark Kettenis via groups.io wrote:<br />
<blockquote>On Fri, Jan 8, 2021 at 05:27 PM, Jeremy Linton wrote:<br /><br />
<blockquote>Hi,<br /><br />On 1/8/21 2:28 AM, Ard Biesheuvel wrote:<br /><br />
<blockquote>On 1/8/21 7:16 AM, Jeremy Linton wrote:<br /><br />
<blockquote>The primarly problem with the rpi Arasan controller working<br />with a default SDHCI driver is the lack of a meaningful<br />capabilities register. As such if we add a _DSD entry<br />to provide that information, we can then bind it to<br />the linux sdhci_iproc driver which already<br />hardcodes the remaining controller bugs.<br /><br />Further we have gotten BRCME88C approved as the HID<br />for the newer eMMC2 controller. So lets define an<br />ACPI object to describe it.<br /><br />Of course both devices are sharing an interrupt so<br />we should also indicate that in the table as well.<br /><br />Signed-off-by: Jeremy Linton &lt;jeremy.linton@arm.com&gt;<br />---<br />Platform/RaspberryPi/AcpiTables/Dsdt.asl | 2 +<br />Platform/RaspberryPi/AcpiTables/Sdhc.asl | 86<br />+++++++++++++++++++++++++++++++-<br />2 files changed, 86 insertions(+), 2 deletions(-)<br /><br />diff --git a/Platform/RaspberryPi/AcpiTables/Dsdt.asl<br />b/Platform/RaspberryPi/AcpiTables/Dsdt.asl<br />index d116f965e1..cca08c0539 100644<br />--- a/Platform/RaspberryPi/AcpiTables/Dsdt.asl<br />+++ b/Platform/RaspberryPi/AcpiTables/Dsdt.asl<br />@@ -150,6 +150,7 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "RPIFDN",<br />"RPI", 2)<br />QWORDMEMORYBUF(23)<br />QWORDMEMORYBUF(24)<br />QWORDMEMORYBUF(25)<br />+ QWORDMEMORYBUF(26)<br />})<br /><br />// USB<br />@@ -196,6 +197,7 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "RPIFDN",<br />"RPI", 2)<br />// SDC<br />QWORDMEMORYSET(24, MMCHS1_OFFSET, MMCHS1_LENGTH)<br />QWORDMEMORYSET(25, SDHOST_OFFSET, SDHOST_LENGTH)<br />+ QWORDMEMORYSET(26, MMCHS2_OFFSET, MMCHS2_LENGTH)<br /><br />Return (RBUF)<br />}<br />diff --git a/Platform/RaspberryPi/AcpiTables/Sdhc.asl<br />b/Platform/RaspberryPi/AcpiTables/Sdhc.asl<br />index 0ab1ba27f2..a7ac831066 100644<br />--- a/Platform/RaspberryPi/AcpiTables/Sdhc.asl<br />+++ b/Platform/RaspberryPi/AcpiTables/Sdhc.asl<br />@@ -19,7 +19,7 @@<br />// Note: UEFI can use either SDHost or Arasan. We expose both to the OS.<br />//<br /><br />-// ArasanSD 3.0 SD Host Controller.<br />+// ArasanSD 3.0 SD Host Controller. (brcm,bcm2835-sdhci)<br />Device (SDC1)<br />{<br />Name (_HID, "BCM2847")<br />@@ -37,7 +37,7 @@ Device (SDC1)<br />Name (RBUF, ResourceTemplate ()<br />{<br />MEMORY32FIXED (ReadWrite, 0, MMCHS1_LENGTH, RMEM)<br />- Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {<br />BCM2836_MMCHS1_INTERRUPT }<br />+ Interrupt (ResourceConsumer, Level, ActiveHigh, Shared) {<br />BCM2836_MMCHS1_INTERRUPT }<br />})<br />Method (_CRS, 0x0, Serialized)<br />{<br />@@ -45,6 +45,86 @@ Device (SDC1)<br />Return (^RBUF)<br />}<br /><br />+ // The standard CAPs registers on this controller<br />+ // appear to be 0, lets set some minimal defaults<br />+ // Since this cap doesn't indicate DMA capability<br />+ // we don't need a _DMA()<br />+ Name (_DSD, Package () {<br />+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),<br />+ Package () {<br />+ Package () { "sdhci-caps", 0x0100fa81 },<br />+ }<br />+ })<br />+<br />+<br />+ //<br />+ // A child device that represents the<br />+ // sd card, which is marked as non-removable.<br />+ //<br />+ Device (SDMM)<br />+ {<br />+ Method (_ADR)<br />+ {<br />+ Return (0)<br />+ }<br />+ Method (_RMV) // Is removable<br />+ {<br />+ Return (0) // 0 - fixed<br />+ }<br />+ }<br />+}<br />+<br />+#if (RPI_MODEL == 4)<br />+// emmc2 Host Controller. (brcm,bcm2711-emmc2)<br />+Device (SDC3)<br />+{<br />+ Name (_HID, "BRCME88C")<br />+ Name (_UID, 0x1)<br />+ Name (_CCA, 0x0)<br />+ Name (_S1D, 0x1)<br />+ Name (_S2D, 0x1)<br />+ Name (_S3D, 0x1)<br />+ Name (_S4D, 0x1)<br />+ Method (_STA)<br />+ {<br />+ Return(0xf)<br />+ }<br />+ Name (RBUF, ResourceTemplate ()<br />+ {<br />+ MEMORY32FIXED (ReadWrite, 0, MMCHS2_LENGTH, RMEM)<br />+ Interrupt (ResourceConsumer, Level, ActiveHigh, Shared) {<br />BCM2836_MMCHS1_INTERRUPT }<br />+ })<br />+ Method (_CRS, 0x0, Serialized)<br />+ {<br />+ MEMORY32SETBASE (RBUF, RMEM, RBAS, MMCHS2_OFFSET)<br />+ Return (^RBUF)<br />+ }<br />+<br />+ // forceably limit to 1G<br />+ Name (_DMA, ResourceTemplate() {<br />+ QWordMemory (ResourceConsumer,<br />+ ,<br />+ MinFixed,<br />+ MaxFixed,<br />+ NonCacheable,<br />+ ReadWrite,<br />+ 0x0,<br />+ 0x0, // MIN<br />+ 0x3fffffff, // MAX<br />+ 0x0, // TRA<br />+ 0x40000000, // LEN<br />+ ,<br />+ ,<br />+ )<br />+ })<br />+</blockquote>
Shouldn't the _DMA method be on a container object instead of on the<br />device object itself?</blockquote>
Pained cough... There is a _DMA on the gpu devs container of which this<br />device belongs. That doesn't make the DMA work. The spec says that _DMA<br />is to exist on the bus device, and I think what is happening here is<br />that the "device" is the SDMM (which isn't directly visible because of<br />the way diff broke this patch up) contained in the "SDC3 mmc bus".</blockquote>
The Linux device tree has the following comment:<br /><br />/*<br />* emmc2 has different DMA constraints based on SoC revisions. It was<br />* moved into its own bus, so as for RPi4's firmware to update them.<br />* The firmware will find whether the emmc2bus alias is defined, and if<br />* so, it'll edit the dma-ranges property below accordingly.<br />*<br /><br />And the dma-ranges property it references is:<br /><br />dma-ranges = &lt;0x0 0xc0000000&nbsp; 0x0 0x00000000&nbsp; 0x40000000&gt;;<br /><br />Not sure how the firmware will adjust this, but it does imply that there is actual translation<br />going on here and it doesn't match the _DMA property you added.<br /><br />
<blockquote>Frankly, I could be wrong, but what I do know is that without that bit<br />of ugliness DMA doesn't work in linux on this controller. I just<br />stripped it out again to verify the failure with 4.11. It does the same<br />thing, the controller itself seems to be working but the block device<br />associated with it can't be read. There are few other possibilities<br />(like maybe this controller shouldn't be a gpudevs child) and i'm fixing<br />a bug I created..I had kernel instrumentation in 5.9 tracking the dma<br />mask propagation, and I was fairly confident in this a couple months<br />ago, less so now.</blockquote>
One question I have is whether _DMA methods should be applied recursively or<br />not. If you have a bus that provides a translating _DMA method that is behind a<br />bus that also has a _DMA method, should the translation specified in that first _DMA<br />method be applied or not? The text in the ACPI spec is far for clear her...</blockquote>
<br />First, thanks for looking at this!<br /><br />The way I understand the spec, one does have to honor nested translations.<br /><br />
<blockquote>If I read the Linux code in drivers/acpi/scan.c correctly, Linux would not apply<br />the translation in this case. Since the _DMA method you add does not specify any<br />translation, I think that means the required address translation isn't happening<br />which means that DMA transactions would fail. I suspect the block device behind<br />the controller would still be detected, but any actual I/O would fail since the right<br />memory isn't addressed.<br /><br />So I think the best thing to do would be to have a separate ACPI0004 container<br />with a translating _DMA method and but the emmc2 device below that.</blockquote>
I think if you look at the rpi's Dsdt, you will see an ACPI0004 <br />container, which includes the GpuDevs, of which Sdhc.asl is child. That <br />container has a _DMA method which specifies the 1G translation+limit <br />which matches what you would expect based on the DT.<br /><br />So this further _DMA should be layering on it and re-enforcing only the <br />DMA limit, not a new translation.</blockquote>
As I wrote upthread, I suspect the Linux kernel isn't handling this properly.<br />The code walks up the ACPI device tree until it finds a _DMA method and uses<br />the information from that instance to calculate the translation offset and the<br />mask for the DMA limit. That would result in a translation offset of 0 for the emmc2<br />device which wouldn't work.<br /><br />Given that this 2nd _DMA method isn't really necessary, I think it would be<br />better to drop it.