Page MenuHomeFreeBSD

Add quirk for ignoring SPCR AccessWidth values on the PL011 UART
ClosedPublic

Authored by greg_unrelenting.technology on Mar 8 2019, 1:53 PM.

Details

Summary

// This was a collection of changes that fixes serial console output on the AWS EC2 a1 (Graviton) instances. Most things were already committed, one thing was pulled into a separate revision, so this is now about the quirk only.

Add workaround for wrong AccessWidth values

The SPCR table on the Lenovo HR330A Ampere eMAG server indicates 8-bit access, but 32-bit access is required for the PL011 to work.

PL011 on SBSA platforms always supports 32-bit access (and that was hardcoded here before my EC2 fix), let's use 32-bit access for PL011 and 32BIT interface types.

Unbreak manual UART settings (hw.uart.console) on aarch64 rS345406

For initial debugging, I configured the loader tunable: hw.uart.console="br:115200,mm:0x90A0000,sb:1" — it didn't work, because:

uart_getenv sets di->bas.bst = uart_bus_space_mem;

uart_cpu_getdev sets uart_bus_space_mem = di->bas.bst;

So it was passing the garbage uninitialized value back and forth. Let's initialize the correct value for the memory space, similar to bus_space_tag_t uart_bus_space_mem = X86_BUS_SPACE_MEM; on amd64.

Pick up regshift from AccessWidth ACPI SPCR property rS345405

The ARM SBSA recommended UARTs need a regshift of 2, but the Amazon EC2 virtual PCI UART is a 16550-compatible one that needs the default 0 value. Let's set the value based on the AccessWidth — hopefully it tells the truth on systems that need 2. (The only system known to have errata for this field is the APM X-Gene, a really old and obsolete aarch64 system.) see below

Match PCI UART devices using PCI data from the ACPI SPCR table Moved to D19896

uart_bus_probe matches discovered devices to system devices using resource addresses (uart_cpu_eqres(&sc->sc_bas, &sysdev->bas)).

The EC2 UART has different addresses in ACPI (0x90A0000) and PCI (0x80118000). Let's use the PCI address from the SPCR table to match PCI UART devices.

This fixes /dev/console (without an UART device, there's no tty, only kernel prints worked).

Fix the PCI ID for Amazon's UART device rS345369

On AArch64 instances, it does not have a 0x1d0f subvendor.

Amazon's "consistency" in setting the card IDs is excellent:

uart0@pci0:0:1:0:	class=0x070003 card=0x00000000 chip=0x82501d0f rev=0x00 hdr=0x00
nvme0@pci0:0:4:0:	class=0x010802 card=0x00001d0f chip=0x80611d0f rev=0x00 hdr=0x00
ena0@pci0:0:5:0:	class=0x020000 card=0xec201d0f chip=0xec201d0f rev=0x00 hdr=0x00

Reference:

Test Plan

Would be nice if someone tested on ThunderX2, etc.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

emaste added a subscriber: cperciva.Mar 8 2019, 2:12 PM
emaste added inline comments.
sys/dev/uart/uart_cpu_arm64.c
167–168

Should still default to *shiftp = 2 in this case I think?

andrew added a comment.Mar 8 2019, 2:30 PM

Could you use spcr->SerialPort.AccessWidth to find this? It's set to 1 in the copy of the spcr table I have indicating byte access.

greg_unrelenting.technology marked an inline comment as done.Mar 8 2019, 2:56 PM

Could you use spcr->SerialPort.AccessWidth to find this? It's set to 1 in the copy of the spcr table I have indicating byte access.

The EC2 one also shows byte access (1). Here's the table.

I'm not familiar with the details of UART hardware, but looks like regshift is something else. It's hardcoded to 2 in uart_dev_pl011 and to 0 in uart_dev_ns8250.

btw, would it be better to integrate all my uart patches into this, or post them separately?

I have a third one now, for connecting the uart_tty console to this PCI device via the Pci{VendorId,DeviceId,Bus,Device} entries in this table. (Which is necessary because currently it only looks at the memory address, and here they are different — 0x90A0000 from ACPI and 0x80118000 from PCI.)

imp added a comment.Mar 8 2019, 8:38 PM

Could you use spcr->SerialPort.AccessWidth to find this? It's set to 1 in the copy of the spcr table I have indicating byte access.

The EC2 one also shows byte access (1). Here's the table.
I'm not familiar with the details of UART hardware, but looks like regshift is something else. It's hardcoded to 2 in uart_dev_pl011 and to 0 in uart_dev_ns8250.

Normally devices have an array of registers. They have offsets 0, 1, 2, etc.
The time passes and hardware engineers do crazy things like making byte longwords and offseting them so the offsets are now 0, 4, 8, 12, etc.
regshift tells us how to offset them. So the access becomes base + (offset << regshift).
which for old hardware, with a regshift of 0, is just the degenerate case it was before.
which is why we can translate register width into a shift by taking regshifh == log2(regwidth).

greg_unrelenting.technology updated this revision to Diff 54862.
greg_unrelenting.technology retitled this revision from Fix parameters for 16550-compatible UARTs discovered via the SPCR ACPI table to Serial console fixes for aarch64 (fixes the virtual UART on EC2 a1 instances).
greg_unrelenting.technology edited the summary of this revision. (Show Details)
greg_unrelenting.technology edited the test plan for this revision. (Show Details)

Dammit. The Ampere eMAG (which uses the pl011 SBSR UART) represents *shiftp = 2; as

[029h 0041   1]                    Bit Width : 20
[02Ah 0042   1]                   Bit Offset : 00
[02Bh 0043   1]         Encoded Access Width : 01 [Byte Access:8]

Would it be correct to calculate *shiftp based on both AccessWidth and BitWidth, picking the larger value?

Or would it be better to choose based on the model (pl011 vs ns8250)?

imp added a comment.Mar 31 2019, 5:24 PM

Dammit. The Ampere eMAG (which uses the pl011 SBSR UART) represents *shiftp = 2; as

[029h 0041   1]                    Bit Width : 20
[02Ah 0042   1]                   Bit Offset : 00
[02Bh 0043   1]         Encoded Access Width : 01 [Byte Access:8]

Yuck! so bit width here is sizeof(reg)*8 rather than log2(sizeof(reg)). I'd suggest that we do something gross like the following function

int uart_width_to_shift(uint8_t width) {
assert(width != 0);
if (width < 8) return width - 1; // Assume it's encoded as log2(size-in-bytes) + 1
return ffs(width) - 1 // ffs returns log2 + 1
}

though if we ever get width == 8 we don't know if it is a uint64_t or a byte. We'd need to know the total size to disambiguate (though we could still have an issue if they really are 8-bytes in address space that can only be accessed 1 byte at a time and not 8 bytes at a time... I've never seen such a design in any hardware so guessing wrong there would be fine since we mask the upper bytes anyway).

Would it be correct to calculate *shiftp based on both AccessWidth and BitWidth, picking the larger value?
Or would it be better to choose based on the model (pl011 vs ns8250)?

I think it would be. IIRC, this violates the ACPI definition for these fields.

AccessWidth is which function you use to read it. BitWidth is the stride between the registers. I think some sanity between the two would be fine.

I think it unwise to do it based on model, because it's just that way today... These tables come from a vendor that's incompetent, which sadly is independent of the hardware :(

In D19507#423825, @imp wrote:

IIRC, this violates the ACPI definition for these fields.

Where are they defined? https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table — Ctrl+F "width" — 0 results. (Thanks Microsoft.)

I think it unwise to do it based on model, because it's just that way today... These tables come from a vendor that's incompetent, which sadly is independent of the hardware :(

Looking at what Linux does, it sets access function to mmio|mmio16|mmio32 based on AccessWidth, and then overrides with mmio32 when InterfaceType is ACPI_DBG2_ARM_SBSA_32BIT.

I think this might be the correct way actually, since the "32bit" is in the name of the interface type. Maybe it's all ARM's fault, they shoved the access width into the type of the interface.

imp added a comment.Apr 1 2019, 2:37 PM
In D19507#423825, @imp wrote:

IIRC, this violates the ACPI definition for these fields.

Where are they defined? https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table — Ctrl+F "width" — 0 results. (Thanks Microsoft.)

They are defined in the definition of ACPI Generic Address Structure, but closer reading of the standard this morning suggests they are only required when accessing bit fields, which byte registers usually aren't considered :( I thought we could use that, but I'm guessing not.

I think it unwise to do it based on model, because it's just that way today... These tables come from a vendor that's incompetent, which sadly is independent of the hardware :(

Looking at what Linux does, it sets access function to mmio|mmio16|mmio32 based on AccessWidth, and then overrides with mmio32 when InterfaceType is ACPI_DBG2_ARM_SBSA_32BIT.
I think this might be the correct way actually, since the "32bit" is in the name of the interface type. Maybe it's all ARM's fault, they shoved the access width into the type of the interface.

I wouldn't be opposed to that either. I dislike special cases, but this may be a good case for an exception.

greg_unrelenting.technology edited the summary of this revision. (Show Details)
greg_unrelenting.technology edited the test plan for this revision. (Show Details)
In D19507#423995, @imp wrote:

I wouldn't be opposed to that either. I dislike special cases, but this may be a good case for an exception.

Update: it's not ACPI_DBG2_ARM_SBSA_32BIT on the Ampere system, it's ACPI_DBG2_ARM_PL011, but still it works in Linux — probably because of the .access_32b = true for SBSA PL011s: https://lore.kernel.org/patchwork/patch/657928/

(That thread confirms that PL011 on SBSA systems always supports 32-bit access.)

I haven't had the chance to re-test the current full patch there because Packet is out of stock, but looking at the table dump I have, the condition should be true.

Anyway… any comments about the PCI part of the patch?

andrew added a comment.Apr 2 2019, 8:55 PM

From looking at the Linux PL011 driver it seems registers always start on a 32b boundary so *shiftp should always be 2. The simplest would be to add a quirk to the PL011 acpi_uart_compat_data entries to force a shift of 2. It would also be cleaner than checking for specific interface types in what should become machine independent code. Later on we may be able to use cd_regshft and cd_regiowidth to override the SPCR details, however it looks like the ns8250 driver may need some careful clean up first.

sys/dev/uart/uart_cpu_arm64.c
158–159

Do we need the BitWidth and AccessWidth checks?

greg_unrelenting.technology edited the test plan for this revision. (Show Details)

The quirk is a much more elegant way, thanks for the suggestion!

andrew added inline comments.Apr 6 2019, 9:07 AM
sys/dev/uart/uart_cpu_arm64.c
156

ACPI_DBG2_ARM_SBSA_32BIT should be added to the pl011 acpi compat data so this is unneeded.

160

This should be

if ((cd->cd_quirks & UART_F_IGNORE_SPCR_REGSHFT) == UART_F_IGNORE_SPCR_REGSHFT) {

Tested eMAG on rS237234 + this change, serial console works as expected for initial boot messages until encountering AcpiExSystemMemorySpaceHandler panic same as reported in PR 237055. Will test on ThunderX2 soon.

imp accepted this revision.Apr 12 2019, 11:09 PM

Looks like Andy's suggestions were followed... I was going to make similar when I last looked at it as well.

sys/dev/uart/uart_cpu_arm64.c
158

small nit: {} optional. no big deal either way.

This revision is now accepted and ready to land.Apr 12 2019, 11:09 PM

I'm happy with the quirk, however I'm not sure about the PCI part. It may pay to split that out into a new review as there are two independent changes in this.

sys/dev/uart/uart_dev_pl011.c
347

We should fix the spelling of pl011 one day.

greg_unrelenting.technology retitled this revision from Serial console fixes for aarch64 (fixes the virtual UART on EC2 a1 instances) to Add quirk for ignoring SPCR AccessWidth values on the PL011 UART.
greg_unrelenting.technology edited the summary of this revision. (Show Details)
This revision now requires review to proceed.Apr 13 2019, 11:08 AM
greg_unrelenting.technology marked an inline comment as done.Apr 13 2019, 11:09 AM
andrew accepted this revision.Apr 13 2019, 11:17 AM
This revision is now accepted and ready to land.Apr 13 2019, 11:17 AM

tested on eMAG and ThunderX2

This revision was automatically updated to reflect the committed changes.