Use ACPI to probe serial devices & workaround limitations in acpi support

[mmacy]

Summary

This is an amalgam of a patch by Doug Ambrisko to generalize uart_acpi_find_device, @imp moving the ACPI table to uart_dev_ns8250.c and advice by @jhb to workaround the too many levels of indirection in the EPYC BIOS. With these changes I have serial support on my preproduction EPYC 3151.

[mmacy]

leave methods static

[jhb]

sys/dev/acpica/acpi.c

2077

Perhaps leave this as-is for now (the PCI link) bits.

2224

I would reword the comment perhaps to something like:

/*
  * Onboard serial ports on certain AMD motherboards have an invalid _STA
  * method that always returns 0.  Force them to always be treated as present.
  */

sys/dev/uart/uart_bus_acpi.c

78

Debug no longer needed?

94

Debug no longer needed?

sys/dev/uart/uart_cpu_acpi.h

40–41

I suspect the old name was to be C++ friendly.

43

Perhaps make it 'regiowidth' to match the name of the parameter to uart_bus_probe()? ('regshift' would also seem sensible but uart_bus_probe() uses 'regshft' already so probably best to leave that one as-is)

sys/dev/uart/uart_dev_ns8250.c

72

Spurious?

452

The whitespace for this table looks a bit odd as if there's a mix of tabs and spaces perhaps?

467

Can probably drop this? Maybe add a name field to acpi_uart_compat_data for the description strings that is then used with device_set_desc() during probe?

[mmacy]

Respond to feedback.

[jhb]

sys/dev/uart/uart_bus_acpi.c

95

To make the 'desc' stuff work, add this before the call to uart_bus_probe():

if (cd->cd_desc != NULL)
    device_set_desc(dev, cd->cd_desc);

This would mean that entries could also leave cd_desc as NULL to use the generic name.

sys/dev/uart/uart_dev_pl011.c

345

Maybe just set 'desc' to NULL for these two and let the arm folks decide if they want more specific names.

[mmacy]

• set desc
• don't give ARM uarts a desc

[rajeshasp]

sys/dev/uart/uart_dev_ns8250.c

450

Can we use the above SNPS elements from uart_dev_snps.c (by including that file in Makefile)? Or is it done, because it is part of uart_snps driver?

[andrew]

sys/dev/uart/uart_cpu_acpi.h

45–46

Can we find this information from the acpi tables?

sys/dev/uart/uart_cpu_arm64.c

150

This is missing the cd_

sys/dev/uart/uart_dev_ns8250.c

424–439

Why is this in the ns8250 driver and not in uart_dev_snps.c?

[ambrisko]

sys/dev/uart/uart_cpu_acpi.h

45–46

I'm not sure, Linux had it hard coded in their code. I added it as part of the table to make it
a little easier to use and make the code more generic.

[mmacy]

sys/dev/uart/uart_dev_ns8250.c

424–439

There's nothing of value to x86 in uart_dev_snps.c apart from this table. Literally _all_ the driver does on x86 is is set the busy_detect flag. We could compile in uart_dev_snps.c, but then we'd ifdef out all of it with #ifdef FDT. That doesn't make a a whole lot of sense to me. @jhb @imp

[imp]

https://pastebin.com/zxRTPX2A has the sketch of what I'd like to see:
change uart_probe to take a quirks argument. It sets a new sc_quirks member. The driver classes then look at this to know. We then add quirks to the FDT and ACPI driver tables as needed. Lots of the FDT ifdefs may go away if I'm reading the code right...

sys/dev/uart/uart_cpu_acpi.h

45–46

We likely can guess fairly well. We know the size of the region, we know the region type. Based on that we can make a damn-good guess as to regshift and regiowidth (the latter is the size which isn't a guess, and the former is 0 if IO and 2 if MEM though that's just been our experience so far).

49

This is missing the most important thing of all: Quirks. We should include a set of defined quirks, including the busy wait so that we don't have to worry about the bogus snps crap below (which is wrong, btw).

sys/dev/uart/uart_dev_ns8250.c

60

You can and should delete this. It's not needed.

sys/dev/uart/uart_dev_pl011.c

345–347

These are the PL011 based UARTS, so should likely have that name.

[mmacy]

incorporate feedback

[mmacy]

sys/dev/uart/uart_cpu_acpi.h

45–46

this is all that is in the ACPI entry:

Device (FUR1)
{
    Name (_HID, "AMDI0020")  // _HID: Hardware ID
    Name (_UID, One)  // _UID: Unique ID
    Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
    {
        IRQ (Edge, ActiveHigh, Exclusive, )
            {4}
        Memory32Fixed (ReadWrite,
            0xFEDCA000,         // Address Base
            0x00001000,         // Address Length
            )
        Memory32Fixed (ReadWrite,
            0xFEDC8000,         // Address Base
            0x00001000,         // Address Length
            )
    })
    Method (_STA, 0, NotSerialized)  // _STA: Status
    {
        If (CKST (One))
        {
            Return (UT1E) /* \_SB_.UT1E */
        }
        Else
        {
            Return (Zero)
        }
    }
}

[rajeshasp]

sys/arm/xscale/pxa/uart_bus_pxa.c

102 ↗

(On Diff #45852)

More parameters than expected. Seems added 3 zeros instead of 1.

[imp]

sys/arm/xscale/i8134x/uart_bus_i81342.c

86 ↗

(On Diff #45852)

I removed this file.

sys/arm/xscale/pxa/uart_bus_pxa.c

102 ↗

(On Diff #45852)

I removed this file.

[mmacy]

• rebase to HEAD
• fix up tinderbox

[mmacy]

@imp any further comments?

[imp]

Modulo the https://reviews.freebsd.org/P209 I suggested, and maybe a stray include of isavar.h, I think this is good to go.

sys/dev/uart/uart_bus_acpi.c

30–31

not needed.

44–45

not needed.
https://reviews.freebsd.org/P209 for the proper fix

102

not needed

sys/dev/uart/uart_dev_ns8250.c

60

why is this line still there? I don't think it's needed...

415

Yea, I don't like this and the FDT data, but it's not terrible and fixing it is kinda beyond the scope.