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: Tue, 12 Jan 2021 10:48:27 -0800 References: <40b30cb8-aa00-735b-f535-a0f37142d1b0@arm.com> In-Reply-To: <40b30cb8-aa00-735b-f535-a0f37142d1b0@arm.com> Message-ID: <22138.1610477307386675880@groups.io> Content-Type: multipart/alternative; boundary="oa8KpB5gE9PnQn1txRkB" --oa8KpB5gE9PnQn1txRkB Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Mon, Jan 11, 2021 at 02:45 AM, Jeremy Linton wrote: >=20 > Hi, >=20 > On 1/9/21 8:09 AM, Mark Kettenis via groups.io wrote: >=20 >> On Fri, Jan 8, 2021 at 05:27 PM, Jeremy Linton wrote: >>=20 >>=20 >>> Hi, >>>=20 >>> On 1/8/21 2:28 AM, Ard Biesheuvel wrote: >>>=20 >>>=20 >>>> On 1/8/21 7:16 AM, Jeremy Linton wrote: >>>>=20 >>>>=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 thi= s >>> device belongs. That doesn't make the DMA work. The spec says that _DM= A >>> 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 ther= e >> is actual translation >> going on here and it doesn't match the _DMA property you added. >>=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 sam= e >>> 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 fixi= ng >>> 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 recursive= ly >> 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 th= at >> first _DMA >> method be applied or not? The text in the ACPI spec is far for clear >> her... >=20 >=20 > First, thanks for looking at this! >=20 > The way I understand the spec, one does have to honor nested translation= s. >=20 >=20 >=20 >> 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 devic= e >> 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 > 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. >=20 > 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 proper= ly. The code walks up the ACPI device tree until it finds a _DMA method and us= es 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 b= e better to drop it. --oa8KpB5gE9PnQn1txRkB Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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:
<= br />
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(-)

d= iff --git a/Platform/RaspberryPi/AcpiTables/Dsdt.asl
b/Platform/Raspbe= rryPi/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)
QWORDMEMOR= YBUF(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/P= latform/RaspberryPi/AcpiTables/Sdhc.asl
index 0ab1ba27f2..a7ac831066 1= 00644
--- a/Platform/RaspberryPi/AcpiTables/Sdhc.asl
+++ b/Platfo= rm/RaspberryPi/AcpiTables/Sdhc.asl
@@ -19,7 +19,7 @@
// Note: UEF= I can use either SDHost or Arasan. We expose both to the OS.
//
<= br />-// ArasanSD 3.0 SD Host Controller.
+// ArasanSD 3.0 SD Host Con= troller. (brcm,bcm2835-sdhci)
Device (SDC1)
{
Name (_HID, "B= CM2847")
@@ -37,7 +37,7 @@ Device (SDC1)
Name (RBUF, ResourceTemp= late ()
{
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, S= erialized)
{
@@ -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 c= ap doesn't indicate DMA capability
+ // we don't need a _DMA()
+ = Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa30= 1"),
+ Package () {
+ Package () { "sdhci-caps", 0x0100fa81 },+ }
+ })
+
+
+ //
+ // A child device that repr= esents 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)<= br />+{
+ Name (_HID, "BRCME88C")
+ Name (_UID, 0x1)
+ Name = (_CCA, 0x0)
+ Name (_S1D, 0x1)
+ Name (_S2D, 0x1)
+ Name (_S= 3D, 0x1)
+ Name (_S4D, 0x1)
+ Method (_STA)
+ {
+ Retur= n(0xf)
+ }
+ Name (RBUF, ResourceTemplate ()
+ {
+ MEMO= RY32FIXED (ReadWrite, 0, MMCHS2_LENGTH, RMEM)
+ Interrupt (ResourceCon= sumer, Level, ActiveHigh, Shared) {
BCM2836_MMCHS1_INTERRUPT }
+ = })
+ Method (_CRS, 0x0, Serialized)
+ {
+ MEMORY32SETBASE (R= BUF, RMEM, RBAS, MMCHS2_OFFSET)
+ Return (^RBUF)
+ }
+
= + // forceably limit to 1G
+ Name (_DMA, ResourceTemplate() {
+ Q= WordMemory (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 thisdevice belongs. That doesn't make the DMA work. The spec says that _DMA<= br />is to exist on the bus device, and I think what is happening here isthat the "device" is the SDMM (which isn't directly visible because of<= br />the way diff broke this patch up) contained in the "SDC3 mmc bus". The Linux device tree has the following comment:

/*
* emmc= 2 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 fir= mware will find whether the emmc2bus alias is defined, and if
* so, it= 'll edit the dma-ranges property below accordingly.
*

And t= he dma-ranges property it references is:

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

Not sure h= ow the firmware will adjust this, but it does imply that there is actual tr= anslation
going on here and it doesn't match the _DMA property you add= ed.

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 j= ust
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 d= evice
associated with it can't be read. There are few other possibilit= ies
(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 t= he 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...

First, thanks for looking at this!

The way I understand t= he spec, one does have to honor nested translations.

If I read the Linux code in drivers/acpi/scan.c correctly, Lin= ux would not apply
the translation in this case. Since the _DMA method= you add does not specify any
translation, I think that means the requ= ired address translation isn't happening
which means that DMA transact= ions would fail. I suspect the block device behind
the controller woul= d still be detected, but any actual I/O would fail since the right
mem= ory 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
cont= ainer, which includes the GpuDevs, of which Sdhc.asl is child. That
c= ontainer has a _DMA method which specifies the 1G translation+limit
w= hich matches what you would expect based on the DT.

So this furt= her _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 proper= ly.
The code walks up the ACPI device tree until it finds a _DMA metho= d and uses
the information from that instance to calculate the transla= tion offset and the
mask for the DMA limit. That would result in a tra= nslation offset of 0 for the emmc2
device which wouldn't work.
Given that this 2nd _DMA method isn't really necessary, I think it woul= d be
better to drop it. --oa8KpB5gE9PnQn1txRkB--