Page MenuHomeFreeBSD

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

Authored by val_packett.cool on Mar 8 2019, 1:53 PM.
Tags
Referenced Files
F102045174: D19507.id55866.diff
Wed, Nov 6, 10:26 PM
Unknown Object (File)
Tue, Nov 5, 12:04 PM
Unknown Object (File)
Tue, Nov 5, 8:38 AM
Unknown Object (File)
Thu, Oct 17, 5:28 PM
Unknown Object (File)
Thu, Oct 17, 12:25 AM
Unknown Object (File)
Wed, Oct 16, 3:57 PM
Unknown Object (File)
Tue, Oct 15, 11:40 PM
Unknown Object (File)
Tue, Oct 15, 4:44 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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste added inline comments.
sys/dev/uart/uart_cpu_arm64.c
165–166 ↗(On Diff #54832)

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

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.

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.)

In D19507#417538, @greg_unrelenting.technology wrote:

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).

val_packett.cool updated this revision to Diff 54862.
val_packett.cool 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).
val_packett.cool edited the summary of this revision. (Show Details)
val_packett.cool 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)?

In D19507#423770, @greg_unrelenting.technology wrote:

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.

In D19507#423939, @greg_unrelenting.technology wrote:
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.

val_packett.cool edited the summary of this revision. (Show Details)
val_packett.cool 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?

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 ↗(On Diff #55737)

Do we need the BitWidth and AccessWidth checks?

val_packett.cool edited the test plan for this revision. (Show Details)

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

sys/dev/uart/uart_cpu_arm64.c
156 ↗(On Diff #55866)

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

160 ↗(On Diff #55866)

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.

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 ↗(On Diff #55876)

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 ↗(On Diff #55876)

We should fix the spelling of pl011 one day.

val_packett.cool 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.
val_packett.cool edited the summary of this revision. (Show Details)
This revision now requires review to proceed.Apr 13 2019, 11:08 AM
This revision is now accepted and ready to land.Apr 13 2019, 11:17 AM
This revision was automatically updated to reflect the committed changes.