Page MenuHomeFreeBSD

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

Authored by mmacy on Jul 24 2018, 7:31 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sys/dev/acpica/acpi.c
2077 ↗(On Diff #45808)

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

2221 ↗(On Diff #45808)

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

Debug no longer needed?

96 ↗(On Diff #45808)

Debug no longer needed?

sys/dev/uart/uart_cpu_acpi.h
42 ↗(On Diff #45808)

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

45 ↗(On Diff #45808)

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

Spurious?

453 ↗(On Diff #45808)

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

468 ↗(On Diff #45808)

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?

sys/dev/uart/uart_bus_acpi.c
95 ↗(On Diff #45811)

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

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

  • set desc
  • don't give ARM uarts a desc
rajeshasp added inline comments.
sys/dev/uart/uart_dev_ns8250.c
450 ↗(On Diff #45812)

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?

sys/dev/uart/uart_cpu_acpi.h
45–46 ↗(On Diff #45812)

Can we find this information from the acpi tables?

sys/dev/uart/uart_cpu_arm64.c
150 ↗(On Diff #45812)

This is missing the cd_

sys/dev/uart/uart_dev_ns8250.c
424–439 ↗(On Diff #45812)

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

sys/dev/uart/uart_cpu_acpi.h
45–46 ↗(On Diff #45812)

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.

sys/dev/uart/uart_dev_ns8250.c
424–439 ↗(On Diff #45812)

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

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

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

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

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

sys/dev/uart/uart_dev_pl011.c
345 ↗(On Diff #45812)

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

mmacy marked 12 inline comments as done.Jul 26 2018, 1:00 AM
mmacy marked 5 inline comments as done.
sys/dev/uart/uart_cpu_acpi.h
45–46 ↗(On Diff #45812)

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)
        }
    }
}
sys/arm/xscale/pxa/uart_bus_pxa.c
102 ↗(On Diff #45852)

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

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.

  • rebase to HEAD
  • fix up tinderbox

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

not needed.

45–46 ↗(On Diff #46639)

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

107 ↗(On Diff #46639)

not needed

sys/dev/uart/uart_dev_ns8250.c
60 ↗(On Diff #46639)

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

412 ↗(On Diff #46639)

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

This revision is now accepted and ready to land.Aug 16 2018, 8:06 PM
This revision was automatically updated to reflect the committed changes.