On Mon, Jan 11, 2021 at 02:45 AM, Jeremy Linton wrote: > > 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: >> >> >>> 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 == 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". >> >> The Linux device tree has the following comment: >> >> /* >> * 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. >> * >> >> And the dma-ranges property it references is: >> >> dma-ranges = <0x0 0xc0000000  0x0 0x00000000  0x40000000>; >> >> 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. >> >> >>> 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 fixing >>> 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. >> >> One question I have is whether _DMA methods should be applied recursively >> or >> not. If you have a bus that provides a translating _DMA method that is >> behind a >> bus that also has a _DMA method, should the translation specified in that >> 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. > > > >> If I read the Linux code in drivers/acpi/scan.c correctly, Linux would not >> apply >> the translation in this case. Since the _DMA method you add does not >> specify any >> translation, I think that means the required address translation isn't >> happening >> 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 >> since the right >> memory isn't addressed. >> >> So I think the best thing to do would be to have a separate ACPI0004 >> container >> 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 > container, which includes the GpuDevs, of which Sdhc.asl is child. That > container has a _DMA method which specifies the 1G translation+limit > which matches what you would expect based on the DT. > > So this further _DMA should be layering on it and re-enforcing only the > DMA limit, not a new translation. As I wrote upthread, I suspect the Linux kernel isn't handling this properly. The code walks up the ACPI device tree until it finds a _DMA method and uses the information from that instance to calculate the translation offset and the mask for the DMA limit. That would result in a translation offset of 0 for the emmc2 device which wouldn't work. Given that this 2nd _DMA method isn't really necessary, I think it would be better to drop it.