Details
- Reviewers
jhb imp ambrisko - Commits
- rS338074: add snps IP uart support / genaralize UART
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - 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. |
| 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) | I'm not sure, Linux had it hard coded in their code. I added it as part of the table to make it |
| sys/dev/uart/uart_dev_ns8250.c | ||
|---|---|---|
| 424–439 ↗ | (On Diff #45812) | |
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. |
| 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. |
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. |
| 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. |