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: Jeremy Linton ,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: Sat, 09 Jan 2021 06:09:15 -0800 References: <11f288a0-aab8-bdca-de7e-b3a4bb9f1469@arm.com> In-Reply-To: <11f288a0-aab8-bdca-de7e-b3a4bb9f1469@arm.com> Message-ID: <7393.1610201355597676892@groups.io> Content-Type: multipart/alternative; boundary="PnxlUQgeT7GAdLAK48qD" --PnxlUQgeT7GAdLAK48qD Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 O= S. >>> // >>>=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". 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 =3D <0x0 0xc0000000=C2=A0 0x0 0x00000000=C2=A0 0x40000000>; Not sure how the firmware will adjust this, but it does imply that there i= s actual translation going on here and it doesn't match the _DMA property you added. >=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 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. >=20 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 beh= ind 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 speci= fy any translation, I think that means the required address translation isn't hap= pening which means that DMA transactions would fail. I suspect the block device b= ehind the controller would still be detected, but any actual I/O would fail sinc= e the right memory isn't addressed. So I think the best thing to do would be to have a separate ACPI0004 conta= iner with a translating _DMA method and but the emmc2 device below that. >=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 > --PnxlUQgeT7GAdLAK48qD Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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.

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