[edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[athierry]

Make sure the base address and length of the Raspberry Pi miniuart set
in the ACPI tables match those in the official device tree. This will be
useful for adding ACPI support to the miniuart Linux driver.

Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
 .../RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h | 2 +-
 Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h
index 24c0e5fd56..65e8aeb17c 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h
@@ -17,7 +17,7 @@
 #include <IndustryStandard/Bcm2836Gpio.h>
 
 #define PL011_UART_REGISTER_BASE      BCM2836_PL011_UART_BASE_ADDRESS
-#define MINI_UART_REGISTER_BASE       (BCM2836_MINI_UART_BASE_ADDRESS + 0x40)
+#define MINI_UART_REGISTER_BASE       BCM2836_MINI_UART_BASE_ADDRESS
 
 //
 // 16550 UART register offsets and bitfields
diff --git a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
index a930c64af3..f34dddd58d 100644
--- a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
+++ b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
@@ -81,9 +81,9 @@
 #define BCM2836_PL011_UART_BASE_ADDRESS                     (BCM2836_SOC_REGISTERS + BCM2836_PL011_UART_OFFSET)
 #define BCM2836_PL011_UART_LENGTH                           0x00001000
 
-#define BCM2836_MINI_UART_OFFSET                            0x00215000
+#define BCM2836_MINI_UART_OFFSET                            0x00215040
 #define BCM2836_MINI_UART_BASE_ADDRESS                      (BCM2836_SOC_REGISTERS + BCM2836_MINI_UART_OFFSET)
-#define BCM2836_MINI_UART_LENGTH                            0x00000070
+#define BCM2836_MINI_UART_LENGTH                            0x00000040
 
 #define BCM2836_I2C0_OFFSET                                 0x00205000
 #define BCM2836_I2C0_BASE_ADDRESS                           (BCM2836_SOC_REGISTERS + BCM2836_I2C0_OFFSET)
-- 
2.31.1

Re: [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Adrien Thierry]

Hi Ard, Leif, Pete

Do you have any feedback on this patch ?

Regards,
Adrien

Re: [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Ard Biesheuvel]

On Mon, 13 Dec 2021 at 15:54, Adrien Thierry <athierry@redhat.com> wrote:
>
> Hi Ard, Leif, Pete
>
> Do you have any feedback on this patch ?
>

No objections from me but I'd like an ack from someone else as well.

Re: [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Andrei Warkentin]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

If I understand correctly, you want to describe the UART at 0x00215000 to be at 0x00215040.

This will break SPCR and DBG2 - so that's a regression for Windows, ESXi and possibly the NetBSDs.

I guess that's a NAK unless I misunderstood something.
________________________________
From: Ard Biesheuvel <ardb@kernel.org>
Sent: Monday, December 13, 2021 9:14 AM
To: Adrien Thierry <athierry@redhat.com>; Andrei Warkentin <awarkentin@vmware.com>; Pete Batard <pete@akeo.ie>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

On Mon, 13 Dec 2021 at 15:54, Adrien Thierry <athierry@redhat.com> wrote:
>
> Hi Ard, Leif, Pete
>
> Do you have any feedback on this patch ?
>

No objections from me but I'd like an ack from someone else as well.

[-- Attachment #2: Type: text/html, Size: 2149 bytes --]

Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Jeremy Linton]

Hi,

On 12/13/21 13:17, Andrei Warkentin via groups.io wrote:
> If I understand correctly, you want to describe the UART at 0x00215000 to be at 0x00215040.
> 
> This will break SPCR and DBG2 - so that's a regression for Windows, ESXi and possibly the NetBSDs.
> 
> I guess that's a NAK unless I misunderstood something.

Presumably the end goal is to get BT working, or are we trying to get 
the console working too?

Either way, the historical SPCR definition is less than ideal because it 
covers those AUX_IRQ/AUX_ENABLE registers which include information for 
the SPI which isn't included in the "uart" definition here. So, IMHO it 
is wrong, but its stuck that way unless we define another uart. Which if 
all we wanted it for was BT then we could just create another device 
under BCM2836 which is only the 8250 like registers. That is sorta ugly, 
but not having a standard uart is ugly too. The other ugly thing is to 
just use the address as is, and offset it by 0x40 in linux as part of 
the clock and ACPI bindings linux patch. (i've got a patch to make it 
work someone wants to bite into it. Lol).

For linux the base clock-rate is going to have to be added as a _DSD 
too. Which I assume is a large part of why it has a custom SPCR id? Put 
another way, is anyone using the extra AUX_ registers, and what else are 
people (windows/etc) "quirking" with the SPCR id?

For linux I've not particularly felt the need to fix this because I had 
BT working (although unreliably) this time last year when I was working 
on the SD/SDIO drivers, and my answer at the time was that one either 
gets a serial console using the pl011 or one gets BT with the pl011. But 
it looks like at a minimum the linux-firmware project and the brcmfmac 
firmware loader have been tweaked over the past year and getting BT 
working isn't as simple as just taking the miniuart-bt line out of 
config.txt as I have in my not particularly good notes from that time 
period.

So, while its behaving like it did when it had bad firmware, it could be 
something in the lower level firmware since attempting to roll back to 
an older firmware/kernel I had on another disk didn't immediately fix it.



> ________________________________
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Monday, December 13, 2021 9:14 AM
> To: Adrien Thierry <athierry@redhat.com>; Andrei Warkentin <awarkentin@vmware.com>; Pete Batard <pete@akeo.ie>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
> Subject: Re: [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length
> 
> On Mon, 13 Dec 2021 at 15:54, Adrien Thierry <athierry@redhat.com> wrote:
>>
>> Hi Ard, Leif, Pete
>>
>> Do you have any feedback on this patch ?
>>
> 
> No objections from me but I'd like an ack from someone else as well.
> 
> 
> 
> 
> 
> 

Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Andrei Warkentin]

[-- Attachment #1: Type: text/plain, Size: 5204 bytes --]

The Raspberry Pi support in edk2-platforms, including ACPI, is a direct ancestor of the original ms-iot tree (https://github.com/ms-iot/RPi-UEFI, by way of https://github.com/andreiw/RaspberryPiPkg).
The way the miniUART is described in ACPI came from Microsoft. Microsoft introduced DBG2/SPCR type 0x10 (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table) and the BCM2836 _HID to describe the miniUART, and the contract is that the base address includes all those crazy registers.

To the best of my knowledge, today there isn't any other way to correctly describe the miniUART in a DBG2, SPCR or DSDT. Moreover, because there's code out there in at least two operating systems coded against these specific definitions, you don't get to change how a _HID == BCM2836 device or SPCR/DBG2 type 0x10 is described.

If you wanted to introduce an alternate mechanism to describe the miniUART - great. You'd have to pick a new _HID. And re-use one of the generic DBG2/SPCR types or cajole for a new one (I'm guessing in the ASWG but I really don't know). But you surely can't haphazardly change an existing firmware<->OS contract. Moreover, you can't deprecate the existing contract overnight as well, so you'd have to add an option to expose the miniUART using a presumably more-Linux friendly option.

If you do introduce a new mechanism to describe the miniUART in ACPI, I'm happy to add support for it in ESXi, paving the way for eventual deprecation of the current mechanism (assuming you get all the other OSes to play ball too...)

Still a NAK. It's not a fix because it's not broken. And it's not considered broken just because you don't like the definitions. I don't like the definitions either, but that's all we got.

A

________________________________
From: Jeremy Linton <jeremy.linton@arm.com>
Sent: Monday, December 13, 2021 11:39 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Andrei Warkentin <awarkentin@vmware.com>; Ard Biesheuvel <ardb@kernel.org>; Adrien Thierry <athierry@redhat.com>; Pete Batard <pete@akeo.ie>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

Hi,

On 12/13/21 13:17, Andrei Warkentin via groups.io wrote:
> If I understand correctly, you want to describe the UART at 0x00215000 to be at 0x00215040.
>
> This will break SPCR and DBG2 - so that's a regression for Windows, ESXi and possibly the NetBSDs.
>
> I guess that's a NAK unless I misunderstood something.

Presumably the end goal is to get BT working, or are we trying to get
the console working too?

Either way, the historical SPCR definition is less than ideal because it
covers those AUX_IRQ/AUX_ENABLE registers which include information for
the SPI which isn't included in the "uart" definition here. So, IMHO it
is wrong, but its stuck that way unless we define another uart. Which if
all we wanted it for was BT then we could just create another device
under BCM2836 which is only the 8250 like registers. That is sorta ugly,
but not having a standard uart is ugly too. The other ugly thing is to
just use the address as is, and offset it by 0x40 in linux as part of
the clock and ACPI bindings linux patch. (i've got a patch to make it
work someone wants to bite into it. Lol).

For linux the base clock-rate is going to have to be added as a _DSD
too. Which I assume is a large part of why it has a custom SPCR id? Put
another way, is anyone using the extra AUX_ registers, and what else are
people (windows/etc) "quirking" with the SPCR id?

For linux I've not particularly felt the need to fix this because I had
BT working (although unreliably) this time last year when I was working
on the SD/SDIO drivers, and my answer at the time was that one either
gets a serial console using the pl011 or one gets BT with the pl011. But
it looks like at a minimum the linux-firmware project and the brcmfmac
firmware loader have been tweaked over the past year and getting BT
working isn't as simple as just taking the miniuart-bt line out of
config.txt as I have in my not particularly good notes from that time
period.

So, while its behaving like it did when it had bad firmware, it could be
something in the lower level firmware since attempting to roll back to
an older firmware/kernel I had on another disk didn't immediately fix it.



> ________________________________
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Monday, December 13, 2021 9:14 AM
> To: Adrien Thierry <athierry@redhat.com>; Andrei Warkentin <awarkentin@vmware.com>; Pete Batard <pete@akeo.ie>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
> Subject: Re: [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length
>
> On Mon, 13 Dec 2021 at 15:54, Adrien Thierry <athierry@redhat.com> wrote:
>>
>> Hi Ard, Leif, Pete
>>
>> Do you have any feedback on this patch ?
>>
>
> No objections from me but I'd like an ack from someone else as well.
>
>
> 
>
>
>


[-- Attachment #2: Type: text/html, Size: 7577 bytes --]

Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Samer El-Haj-Mahmoud]

[-- Attachment #1: Type: text/plain, Size: 6768 bytes --]

I see that DBG2/SPCR spec (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table) defines 0x10 for Serial type for BCM2835:

0x0010
BCM2835

Should the spec be updated to make this more explicit? i.e. "BCM283x/BCM27xx MiniUART"



From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Andrei Warkentin via groups.io
Sent: Tuesday, December 14, 2021 1:21 AM
To: Jeremy Linton <Jeremy.Linton@arm.com>; devel@edk2.groups.io; Ard Biesheuvel <ardb@kernel.org>; Adrien Thierry <athierry@redhat.com>; Pete Batard <pete@akeo.ie>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

The Raspberry Pi support in edk2-platforms, including ACPI, is a direct ancestor of the original ms-iot tree (https://github.com/ms-iot/RPi-UEFI, by way of https://github.com/andreiw/RaspberryPiPkg).
The way the miniUART is described in ACPI came from Microsoft. Microsoft introduced DBG2/SPCR type 0x10 (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table) and the BCM2836 _HID to describe the miniUART, and the contract is that the base address includes all those crazy registers.

To the best of my knowledge, today there isn't any other way to correctly describe the miniUART in a DBG2, SPCR or DSDT. Moreover, because there's code out there in at least two operating systems coded against these specific definitions, you don't get to change how a _HID == BCM2836 device or SPCR/DBG2 type 0x10 is described.

If you wanted to introduce an alternate mechanism to describe the miniUART - great. You'd have to pick a new _HID. And re-use one of the generic DBG2/SPCR types or cajole for a new one (I'm guessing in the ASWG but I really don't know). But you surely can't haphazardly change an existing firmware<->OS contract. Moreover, you can't deprecate the existing contract overnight as well, so you'd have to add an option to expose the miniUART using a presumably more-Linux friendly option.

If you do introduce a new mechanism to describe the miniUART in ACPI, I'm happy to add support for it in ESXi, paving the way for eventual deprecation of the current mechanism (assuming you get all the other OSes to play ball too...)

Still a NAK. It's not a fix because it's not broken. And it's not considered broken just because you don't like the definitions. I don't like the definitions either, but that's all we got.

A

________________________________
From: Jeremy Linton <jeremy.linton@arm.com<mailto:jeremy.linton@arm.com>>
Sent: Monday, December 13, 2021 11:39 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Andrei Warkentin <awarkentin@vmware.com<mailto:awarkentin@vmware.com>>; Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>>; Adrien Thierry <athierry@redhat.com<mailto:athierry@redhat.com>>; Pete Batard <pete@akeo.ie<mailto:pete@akeo.ie>>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>; Leif Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>>
Subject: Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

Hi,

On 12/13/21 13:17, Andrei Warkentin via groups.io wrote:
> If I understand correctly, you want to describe the UART at 0x00215000 to be at 0x00215040.
>
> This will break SPCR and DBG2 - so that's a regression for Windows, ESXi and possibly the NetBSDs.
>
> I guess that's a NAK unless I misunderstood something.

Presumably the end goal is to get BT working, or are we trying to get
the console working too?

Either way, the historical SPCR definition is less than ideal because it
covers those AUX_IRQ/AUX_ENABLE registers which include information for
the SPI which isn't included in the "uart" definition here. So, IMHO it
is wrong, but its stuck that way unless we define another uart. Which if
all we wanted it for was BT then we could just create another device
under BCM2836 which is only the 8250 like registers. That is sorta ugly,
but not having a standard uart is ugly too. The other ugly thing is to
just use the address as is, and offset it by 0x40 in linux as part of
the clock and ACPI bindings linux patch. (i've got a patch to make it
work someone wants to bite into it. Lol).

For linux the base clock-rate is going to have to be added as a _DSD
too. Which I assume is a large part of why it has a custom SPCR id? Put
another way, is anyone using the extra AUX_ registers, and what else are
people (windows/etc) "quirking" with the SPCR id?

For linux I've not particularly felt the need to fix this because I had
BT working (although unreliably) this time last year when I was working
on the SD/SDIO drivers, and my answer at the time was that one either
gets a serial console using the pl011 or one gets BT with the pl011. But
it looks like at a minimum the linux-firmware project and the brcmfmac
firmware loader have been tweaked over the past year and getting BT
working isn't as simple as just taking the miniuart-bt line out of
config.txt as I have in my not particularly good notes from that time
period.

So, while its behaving like it did when it had bad firmware, it could be
something in the lower level firmware since attempting to roll back to
an older firmware/kernel I had on another disk didn't immediately fix it.



> ________________________________
> From: Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>>
> Sent: Monday, December 13, 2021 9:14 AM
> To: Adrien Thierry <athierry@redhat.com<mailto:athierry@redhat.com>>; Andrei Warkentin <awarkentin@vmware.com<mailto:awarkentin@vmware.com>>; Pete Batard <pete@akeo.ie<mailto:pete@akeo.ie>>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>; Leif Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>>
> Subject: Re: [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length
>
> On Mon, 13 Dec 2021 at 15:54, Adrien Thierry <athierry@redhat.com<mailto:athierry@redhat.com>> wrote:
>>
>> Hi Ard, Leif, Pete
>>
>> Do you have any feedback on this patch ?
>>
>
> No objections from me but I'd like an ack from someone else as well.
>
>
>
>
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[-- Attachment #2: Type: text/html, Size: 12191 bytes --]

Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Jeremy Linton]

Hi,

On 12/14/21 00:21, Andrei Warkentin wrote:
> The Raspberry Pi support in edk2-platforms, including ACPI, is a direct ancestor of the original ms-iot tree (https://github.com/ms-iot/RPi-UEFI, by way of https://github.com/andreiw/RaspberryPiPkg).
> The way the miniUART is described in ACPI came from Microsoft. Microsoft introduced DBG2/SPCR type 0x10 (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table) and the BCM2836 _HID to describe the miniUART, and the contract is that the base address includes all those crazy registers.
> 
> To the best of my knowledge, today there isn't any other way to correctly describe the miniUART in a DBG2, SPCR or DSDT. Moreover, because there's code out there in at least two operating systems coded against these specific definitions, you don't get to change how a _HID == BCM2836 device or SPCR/DBG2 type 0x10 is described.
> 
> If you wanted to introduce an alternate mechanism to describe the miniUART - great. You'd have to pick a new _HID. And re-use one of the generic DBG2/SPCR types or cajole for a new one (I'm guessing in the ASWG but I really don't know). But you surely can't haphazardly change an existing firmware<->OS contract. Moreover, you can't deprecate the existing contract overnight as well, so you'd have to add an option to expose the miniUART using a presumably more-Linux friendly option.

I guess I wasn't clear, I wasn't suggesting we change the existing 
mechanism, so yes, I agree we either need another mechanism, or linux is 
going to have to deal with it as is. The latter is IMHO the best option 
(and as I mentioned I have patches to make it work), but sort of moves 
us away from the standard uart/etc mechanisms we want for systemready. 
So in that regard its not ideal.

> 
> If you do introduce a new mechanism to describe the miniUART in ACPI, I'm happy to add support for it in ESXi, paving the way for eventual deprecation of the current mechanism (assuming you get all the other OSes to play ball too...)
> 
> Still a NAK. It's not a fix because it's not broken. And it's not considered broken just because you don't like the definitions. I don't like the definitions either, but that's all we got.

Yes, I tend to agree WRT to changing the base address for the existing 
HID. So I wasn't disagreeing with your intent, just trying to point out 
a way forward without changing the base addr. Although, I'm going to 
continue thinking its broken :)


Thanks,


> 
> A
> 
> ________________________________
> From: Jeremy Linton <jeremy.linton@arm.com>
> Sent: Monday, December 13, 2021 11:39 PM
> To: devel@edk2.groups.io <devel@edk2.groups.io>; Andrei Warkentin <awarkentin@vmware.com>; Ard Biesheuvel <ardb@kernel.org>; Adrien Thierry <athierry@redhat.com>; Pete Batard <pete@akeo.ie>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
> Subject: Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length
> 
> Hi,
> 
> On 12/13/21 13:17, Andrei Warkentin via groups.io wrote:
>> If I understand correctly, you want to describe the UART at 0x00215000 to be at 0x00215040.
>>
>> This will break SPCR and DBG2 - so that's a regression for Windows, ESXi and possibly the NetBSDs.
>>
>> I guess that's a NAK unless I misunderstood something.
> 
> Presumably the end goal is to get BT working, or are we trying to get
> the console working too?
> 
> Either way, the historical SPCR definition is less than ideal because it
> covers those AUX_IRQ/AUX_ENABLE registers which include information for
> the SPI which isn't included in the "uart" definition here. So, IMHO it
> is wrong, but its stuck that way unless we define another uart. Which if
> all we wanted it for was BT then we could just create another device
> under BCM2836 which is only the 8250 like registers. That is sorta ugly,
> but not having a standard uart is ugly too. The other ugly thing is to
> just use the address as is, and offset it by 0x40 in linux as part of
> the clock and ACPI bindings linux patch. (i've got a patch to make it
> work someone wants to bite into it. Lol).
> 
> For linux the base clock-rate is going to have to be added as a _DSD
> too. Which I assume is a large part of why it has a custom SPCR id? Put
> another way, is anyone using the extra AUX_ registers, and what else are
> people (windows/etc) "quirking" with the SPCR id?
> 
> For linux I've not particularly felt the need to fix this because I had
> BT working (although unreliably) this time last year when I was working
> on the SD/SDIO drivers, and my answer at the time was that one either
> gets a serial console using the pl011 or one gets BT with the pl011. But
> it looks like at a minimum the linux-firmware project and the brcmfmac
> firmware loader have been tweaked over the past year and getting BT
> working isn't as simple as just taking the miniuart-bt line out of
> config.txt as I have in my not particularly good notes from that time
> period.
> 
> So, while its behaving like it did when it had bad firmware, it could be
> something in the lower level firmware since attempting to roll back to
> an older firmware/kernel I had on another disk didn't immediately fix it.
> 
> 
> 
>> ________________________________
>> From: Ard Biesheuvel <ardb@kernel.org>
>> Sent: Monday, December 13, 2021 9:14 AM
>> To: Adrien Thierry <athierry@redhat.com>; Andrei Warkentin <awarkentin@vmware.com>; Pete Batard <pete@akeo.ie>
>> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
>> Subject: Re: [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length
>>
>> On Mon, 13 Dec 2021 at 15:54, Adrien Thierry <athierry@redhat.com> wrote:
>>>
>>> Hi Ard, Leif, Pete
>>>
>>> Do you have any feedback on this patch ?
>>>
>>
>> No objections from me but I'd like an ack from someone else as well.
>>
>>
>> 
>>
>>
>>
> 
> 

Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Adrien Thierry]

> The Raspberry Pi support in edk2-platforms, including ACPI, is a direct ancestor of the original ms-iot tree (https://github.com/ms-iot/RPi-UEFI, by way of https://github.com/andreiw/RaspberryPiPkg).
> The way the miniUART is described in ACPI came from Microsoft. Microsoft introduced DBG2/SPCR type 0x10 (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table) and the BCM2836 _HID to describe the miniUART, and the contract is that the base address includes all those crazy registers.
> 
> To the best of my knowledge, today there isn't any other way to correctly describe the miniUART in a DBG2, SPCR or DSDT. Moreover, because there's code out there in at least two operating systems coded against these specific definitions, you don't get to change how a _HID == BCM2836 device or SPCR/DBG2 type 0x10 is described.

Thanks for your feedback!
I only had Linux in mind and didn't think about the other implementations
that would break. Thank you for stating the historical reasons why things
are set that way, I better understand now and see why this patch wouldn't
be such a great idea.

> I guess I wasn't clear, I wasn't suggesting we change the existing
> mechanism, so yes, I agree we either need another mechanism, or linux is
> going to have to deal with it as is. The latter is IMHO the best option
> (and as I mentioned I have patches to make it work), but sort of moves
> us away from the standard uart/etc mechanisms we want for systemready.
> So in that regard its not ideal.

I'm very interested to play with your patches if you could send them :)
I've been trying to add ACPI support to the miniuart Linux driver, and
stumbled across two issues:
- the miniuart base address "off by 0x40" in the DSDT (the subject of
this thread)
- how to properly get the clock rate with ACPI

Ideally, the end goal would be to have both the serial console and BT
working with the miniuart.

Adrien

Re: [edk2-devel] [edk2-platforms PATCH] Platform/RaspberryPi: Fix miniuart base address and length

[Jeremy Linton]

Hi,

On 12/14/21 16:49, Adrien Thierry wrote:
>> The Raspberry Pi support in edk2-platforms, including ACPI, is a direct ancestor of the original ms-iot tree (https://github.com/ms-iot/RPi-UEFI, by way of https://github.com/andreiw/RaspberryPiPkg).
>> The way the miniUART is described in ACPI came from Microsoft. Microsoft introduced DBG2/SPCR type 0x10 (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table) and the BCM2836 _HID to describe the miniUART, and the contract is that the base address includes all those crazy registers.
>>
>> To the best of my knowledge, today there isn't any other way to correctly describe the miniUART in a DBG2, SPCR or DSDT. Moreover, because there's code out there in at least two operating systems coded against these specific definitions, you don't get to change how a _HID == BCM2836 device or SPCR/DBG2 type 0x10 is described.
> 
> Thanks for your feedback!
> I only had Linux in mind and didn't think about the other implementations
> that would break. Thank you for stating the historical reasons why things
> are set that way, I better understand now and see why this patch wouldn't
> be such a great idea.
> 
>> I guess I wasn't clear, I wasn't suggesting we change the existing
>> mechanism, so yes, I agree we either need another mechanism, or linux is
>> going to have to deal with it as is. The latter is IMHO the best option
>> (and as I mentioned I have patches to make it work), but sort of moves
>> us away from the standard uart/etc mechanisms we want for systemready.
>> So in that regard its not ideal.
> 
> I'm very interested to play with your patches if you could send them :)
> I've been trying to add ACPI support to the miniuart Linux driver, and
> stumbled across two issues:

I sent you a hatchet job that works with the current firmware.

> - the miniuart base address "off by 0x40" in the DSDT (the subject of
> this thread)
I think a reasonable choice is probably to add a structure to the linux 
acpi_device_id table, and put the offset in there, retrieve it during 
_probe with the macro that checks for a acpi device and picks off the 
acpi device match entry and then add the offset to the resource.


> - how to properly get the clock rate with ACPI

Again if you look in the Uart.asl file you modified at the pl011, you 
will see that there is a clock-frequency DSD. Something similar appears 
to work on the pi4 since the miniuart clock doesn't appear to be 
changing over core frequency changes, and if that starts it looks like 
ACPI won't be the only victim.

> 
> Ideally, the end goal would be to have both the serial console and BT
> working with the miniuart.
> 
> Adrien
>