[edk2-devel] [PATCH 2/3] Platform/RaspberryPi/Acpitables: Add eMMC2 device and tweak Arasan

Tue Jan 12 18:48:27 UTC 2021

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 <jeremy.linton at 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...
> 
> 
> 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.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70164): https://edk2.groups.io/g/devel/message/70164
Mute This Topic: https://groups.io/mt/79518731/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210112/5d418412/attachment.htm>