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 <jeremy.linton@arm.com>
>>> ---
>>> 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...

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.

> 
> 
>> 
>>> + 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
>> 
>> 
> 
>