From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [PATCH 2/3] Platform/RaspberryPi/Acpitables: Add eMMC2 device and tweak Arasan To: Mark Kettenis ,devel@edk2.groups.io From: "Mark Kettenis" X-Originating-Location: Kampen, Provincie Overijssel, NL (83.163.83.176) X-Originating-Platform: Linux Chrome 87 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Sun, 10 Jan 2021 03:53:41 -0800 References: <7393.1610201355597676892@groups.io> In-Reply-To: <7393.1610201355597676892@groups.io> Message-ID: <15768.1610279621085605539@groups.io> Content-Type: multipart/alternative; boundary="V6F6w1MMUeBZ2M7s6trF" --V6F6w1MMUeBZ2M7s6trF Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Sat, Jan 9, 2021 at 03:09 PM, Mark Kettenis wrote: >=20 > On Fri, Jan 8, 2021 at 05:27 PM, Jeremy Linton wrote: >=20 >> Hi, >>=20 >> On 1/8/21 2:28 AM, Ard Biesheuvel wrote: >>=20 >>> On 1/8/21 7:16 AM, Jeremy Linton wrote: >>>=20 >>>> 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. >>>>=20 >>>> Further we have gotten BRCME88C approved as the HID >>>> for the newer eMMC2 controller. So lets define an >>>> ACPI object to describe it. >>>>=20 >>>> Of course both devices are sharing an interrupt so >>>> we should also indicate that in the table as well. >>>>=20 >>>> Signed-off-by: Jeremy Linton >>>> --- >>>> Platform/RaspberryPi/AcpiTables/Dsdt.asl | 2 + >>>> Platform/RaspberryPi/AcpiTables/Sdhc.asl | 86 >>>> +++++++++++++++++++++++++++++++- >>>> 2 files changed, 86 insertions(+), 2 deletions(-) >>>>=20 >>>> 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) >>>> }) >>>>=20 >>>> // 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) >>>>=20 >>>> 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. >>>> // >>>>=20 >>>> -// 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) >>>> } >>>>=20 >>>> + // 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 >>>> + , >>>> + , >>>> + ) >>>> + }) >>>> + >>>=20 >>> Shouldn't the _DMA method be on a container object instead of on the >>> device object itself? >>=20 >> 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 This is what I get on my Pi4: dma-ranges: 00000000.c0000000.00000000.00000000.3c000000 So we don't have get the full 1G to play with on some revisions of the SoC= . >=20 >=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 >=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 > behind 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... >=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 > 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. >=20 > 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. >=20 >=20 >>=20 >>>=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) >>>> } >>>> } >>>>=20 >>>> +#else // !RPI4 >>>>=20 >>>> // Broadcom SDHost 2.0 SD Host Controller >>>> Device (SDC2) >>>> @@ -105,3 +186,4 @@ Device (SDC2) >>>> } >>>> } >>>> } >>>> +#endif // !RPI4 >>>=20 >>>=20 >>=20 >>=20 >=20 > --V6F6w1MMUeBZ2M7s6trF Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Sat, Jan 9, 2021 at 03:09 PM, Mark Kettenis 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 workingwith a default SDHCI driver is the lack of a meaningful
capabilitie= s 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 alreadyhardcodes the remaining controller bugs.

Further we have gott= en BRCME88C approved as the HID
for the newer eMMC2 controller. So let= s define an
ACPI object to describe it.

Of course both devi= ces are sharing an interrupt so
we should also indicate that in the ta= ble as well.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.= com>
---
Platform/RaspberryPi/AcpiTables/Dsdt.asl | 2 +
P= latform/RaspberryPi/AcpiTables/Sdhc.asl | 86 ++++++++++++++++++++++++++++++= +-
2 files changed, 86 insertions(+), 2 deletions(-)

diff -= -git a/Platform/RaspberryPi/AcpiTables/Dsdt.asl b/Platform/RaspberryPi/Acpi= Tables/Dsdt.asl
index d116f965e1..cca08c0539 100644
--- a/Platfor= m/RaspberryPi/AcpiTables/Dsdt.asl
+++ b/Platform/RaspberryPi/AcpiTable= s/Dsdt.asl
@@ -150,6 +150,7 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5,= "RPIFDN", "RPI", 2)
QWORDMEMORYBUF(23)
QWORDMEMORYBUF(24)
Q= WORDMEMORYBUF(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/AcpiT= ables/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 o= r Arasan. We expose both to the OS.
//

-// ArasanSD 3.0 SD = Host Controller.
+// ArasanSD 3.0 SD Host Controller. (brcm,bcm2835-sd= hci)
Device (SDC1)
{
Name (_HID, "BCM2847")
@@ -37,7 +3= 7,7 @@ Device (SDC1)
Name (RBUF, ResourceTemplate ()
{
MEMOR= Y32FIXED (ReadWrite, 0, MMCHS1_LENGTH, RMEM)
- Interrupt (ResourceCons= umer, Level, ActiveHigh, Exclusive) { BCM2836_MMCHS1_INTERRUPT }
+ Int= errupt (ResourceConsumer, Level, ActiveHigh, Shared) { BCM2836_MMCHS1_INTER= RUPT }
})
Method (_CRS, 0x0, Serialized)
{
@@ -45,6 +45= ,86 @@ Device (SDC1)
Return (^RBUF)
}

+ // The standar= d CAPs registers on this controller
+ // appear to be 0, lets set some= minimal defaults
+ // Since this cap doesn't indicate DMA capability<= br />+ // we don't need a _DMA()
+ Name (_DSD, Package () {
+ ToU= UID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Pac= kage () { "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, "BRCME88= C")
+ 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_LEN= GTH, RMEM)
+ Interrupt (ResourceConsumer, Level, ActiveHigh, Shared) {= BCM2836_MMCHS1_INTERRUPT }
+ })
+ Method (_CRS, 0x0, Serialized)=
+ {
+ MEMORY32SETBASE (RBUF, RMEM, RBAS, MMCHS2_OFFSET)
+ R= eturn (^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 constrain= ts 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 th= e emmc2bus alias is defined, and if
         * so, it'll edit the dma-ranges pro= perty below accordingly.
         *

And the dma-ranges pro= perty it references is:

    dma-ranges =3D <0x0 0xc0000000  0x0 0x000= 00000  0x40000000>;

Not sure how the firmware will adjus= t this, but it does imply that there is actual translation
going on he= re and it doesn't match the _DMA property you added.
This is what I get on my Pi4:

    dma-ranges: 00000000.c0000000.00000000.00000000.3c00000= 0

So we don't have get the full 1G to play with on some revision= s of the SoC.

Frankly, I could be wrong, but what I do know is that without that bi= t
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 devic= e
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 fi= xing
a bug I created..I had kernel instrumentation in 5.9 tracking th= e dma
mask propagation, and I was fairly confident in this a couple m= onths
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 s= pecified 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 dr= ivers/acpi/scan.c correctly, Linux would not apply
the translation in = this case. Since the _DMA method you add does not specify any
translat= ion, I think that means the required address translation isn't happeningwhich means that DMA transactions would fail. I suspect the block device= behind
the controller would still be detected, but any actual I/O wou= ld fail since the right
memory isn't addressed.

So I think = the best thing to do would be to have a separate ACPI0004 container
wi= th a translating _DMA method and but the emmc2 device below that.

+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8= a91-bc9bbf4aa301"),
+ Package () {
+ Package () { "sdhci-caps-mas= k", 0x0000000000080000 },
+ }
+ })
+
//
// A child= device that represents the
// sd card, which is marked as non-removab= le.
@@ -62,6 +142,7 @@ Device (SDC1)
}
}

+#else /= / !RPI4

// Broadcom SDHost 2.0 SD Host Controller
Device (S= DC2)
@@ -105,3 +186,4 @@ Device (SDC2)
}
}
}
+#end= if // !RPI4
--V6F6w1MMUeBZ2M7s6trF--