> Date: Tue, 7 Dec 2021 11:30:48 +0100
> From: Anton Lindqvist <[email protected]>
>
> On Mon, Dec 06, 2021 at 10:25:34PM +0100, Mark Kettenis wrote:
> > > Date: Mon, 6 Dec 2021 21:45:03 +0100
> > > From: Anton Lindqvist <[email protected]>
> > >
> > > On Mon, Dec 06, 2021 at 09:23:45PM +0100, Mark Kettenis wrote:
> > > > > Date: Mon, 6 Dec 2021 21:08:04 +0100
> > > > > From: Patrick Wildt <[email protected]>
> > > > >
> > > > > Hi,
> > > > >
> > > > > On one machine I had the pleasure of having to try and use the
> > > > > Serial-over-LAN feature which shows up as just another com(4)
> > > > > device. Instead of having to manually add a com(4) at isa(4)
> > > > > I figured it would be nicer to have them attach via ACPI. At
> > > > > least on that machine, the SOL definitely shows up in the DSDT.
> > > > >
> > > > > Since I don't want to break any legacy machines, I figured I'd
> > > > > keep ignoring the isa(4) addresses specified in amd64's GENERIC.
> > > > >
> > > > > Right now this diff is more about putting it out there, not about
> > > > > asking for OKs, as amd64 isn't really my strong suit. If people
> > > > > are interested, I can definitely put in all the feedback there is.
> > > > >
> > > > > Patrick
> > > >
> > > > anton@ has a better diff he's working on
> > >
> > > Here's the diff in its current state. There's one thing I haven't had
> > > the time to figure out yet: in order to get interrupts working on my
> > > apu2 I had to explicit pass LR_EXTIRQ_MODE (aaa_irq_flags is zero) to
> > > acpi_intr_establish() which causes IST_EDGE to be passed to
> > > intr_establish(). Worth noting is that this matches what com at isa
> > > already does. This was however not necessary on my amd64 build machine
> > > where aaa_irq_flags also is zero.
> >
> > Actually, it seems we're parsing the ACPI interrupt resource
> > descriptor wrong. The decompiled DSDTs I have here seem to use
> > IRQNoFlags() for the interrupts, which apparently implies the
> > interrupt is edge-triggered.
> >
> > Let me see if I can cook a diff.
>
> Updated diff now that kettenis@ fixed parsing of the irq flags.
A few comments below.
> diff --git share/man/man4/com.4 share/man/man4/com.4
> index 73b421f2ca7..e54255fe0d6 100644
> --- share/man/man4/com.4
> +++ share/man/man4/com.4
> @@ -61,11 +61,13 @@
> .Cd "com0 at isa? port 0x3f8 irq 4"
> .Cd "com1 at isa? port 0x2f8 irq 3"
> .Pp
> +.Cd "# arm64 and amd64"
> +.Cd "com* at acpi?"
> +.Pp
> .Cd "# armv7"
> .Cd "com* at fdt?"
> .Pp
> .Cd "# arm64"
> -.Cd "com* at acpi?"
> .Cd "com* at fdt?"
The armv7 and arm64 entries can be combined now
> .Pp
> .Cd "# hppa"
> diff --git sys/arch/amd64/conf/GENERIC sys/arch/amd64/conf/GENERIC
> index ecccd1323d9..87fcaced306 100644
> --- sys/arch/amd64/conf/GENERIC
> +++ sys/arch/amd64/conf/GENERIC
> @@ -65,6 +65,11 @@ amdgpio* at acpi?
> aplgpio* at acpi?
> bytgpio* at acpi?
> chvgpio* at acpi?
> +com0 at acpi?
> +com1 at acpi?
> +com2 at acpi?
> +com3 at acpi?
> +com* at acpi?
> glkgpio* at acpi?
> pchgpio* at acpi?
> sdhc* at acpi?
> diff --git sys/arch/amd64/conf/RAMDISK sys/arch/amd64/conf/RAMDISK
> index 6041293b287..a5f10b357dd 100644
> --- sys/arch/amd64/conf/RAMDISK
> +++ sys/arch/amd64/conf/RAMDISK
> @@ -34,6 +34,9 @@ acpipci* at acpi?
> acpiprt* at acpi?
> acpimadt0 at acpi?
> #acpitz* at acpi?
> +com0 at acpi?
> +com1 at acpi?
> +com* at acpi?
>
> mpbios0 at bios0
As Theo pointed out, this may not be entirely fool-proof. We probe
acpi(4) before isa(4), which is good. Since we prevent ISA devices
from attaching to addresses that have been claimed by something else,
this means that we won't attach a com(4) to isa(4) if we have attached
an ACPI com(4) attached at the same address. But if there is no ACPI
com(4) at an address the an ISA com(4) may still attach. As long as
the ACPI serial ports are listed in canonical order and use the
standard IRQ, everything should be fine. If the ACPI com(4) ports are
not in canonical order any additional ISA com(4) may no longer attach.
For example, if there is a single ACPI com(4) at address 0x2f8, we
will now attach it as com0. But since com0 is now attached, the
isa(4) code will skip its com0 and won't probe com(4) at address
0x3f8. I think this is fine. I expect ACPI to list all usable com(4)
ports so any ports sucessfully probed by isa(4) would be "ghost" ports
that aren't actually connected.
The case of non-standard IRQs is also interesting. Suppose we have an
ACPI com(4) at address 0x3f8 but with the non-standard IRQ 3. We may
still end up probing an ISA com(4) at address 0x2f8. This probe would
succeed if the hardware responds to the probe. But establishing the
interrupt at IRQ 3 would fail since sharing of edge-triggered
interrupts isn't allowed. So the attach should fail in that case.
And that should be fine.
So I think this approach is actually fine. If it turns out there are
corner cases that we actually care about, there are ways to address
this.
> diff --git sys/arch/amd64/conf/RAMDISK_CD sys/arch/amd64/conf/RAMDISK_CD
> index 8bd646b4ea3..d007c0102d0 100644
> --- sys/arch/amd64/conf/RAMDISK_CD
> +++ sys/arch/amd64/conf/RAMDISK_CD
> @@ -51,6 +51,10 @@ bytgpio* at acpi?
> sdhc* at acpi?
> acpihve* at acpi?
> chvgpio* at acpi?
> +com0 at acpi?
> +com1 at acpi?
> +com2 at acpi?
> +com* at acpi?
> glkgpio* at acpi?
>
> mpbios0 at bios0
> diff --git sys/dev/acpi/acpi.c sys/dev/acpi/acpi.c
> index 7577424e8a2..e89869aedbd 100644
> --- sys/dev/acpi/acpi.c
> +++ sys/dev/acpi/acpi.c
> @@ -3140,7 +3140,6 @@ const char *acpi_isa_hids[] = {
> "PNP0303", /* IBM Enhanced Keyboard (101/102-key, PS/2 Mouse) */
> "PNP0400", /* Standard LPT Parallel Port */
> "PNP0401", /* ECP Parallel Port */
> - "PNP0501", /* 16550A-compatible COM Serial Port */
> "PNP0700", /* PC-class Floppy Disk Controller */
> "PNP0F03", /* Microsoft PS/2-style Mouse */
> "PNP0F13", /* PS/2 Mouse */
> diff --git sys/dev/acpi/com_acpi.c sys/dev/acpi/com_acpi.c
> index 852be6c71b3..e4001861032 100644
> --- sys/dev/acpi/com_acpi.c
> +++ sys/dev/acpi/com_acpi.c
> @@ -49,10 +49,12 @@ struct cfattach com_acpi_ca = {
>
> const char *com_hids[] = {
> "HISI0031",
> + "PNP0501",
> NULL
> };
>
> int com_acpi_is_console(struct com_acpi_softc *);
> +int com_acpi_is_designware(const char *);
> int com_acpi_intr_designware(void *);
>
> int
> @@ -69,7 +71,7 @@ com_acpi_attach(struct device *parent, struct device *self,
> void *aux)
> {
> struct com_acpi_softc *sc = (struct com_acpi_softc *)self;
> struct acpi_attach_args *aaa = aux;
> - uint32_t freq;
> + int (*intrfn)(void *) = comintr;
>
> sc->sc_acpi = (struct acpi_softc *)parent;
> sc->sc_node = aaa->aaa_node;
> @@ -85,34 +87,43 @@ com_acpi_attach(struct device *parent, struct device
> *self, void *aux)
> return;
> }
>
> - printf(" addr 0x%llx/0x%llx", aaa->aaa_addr[0], aaa->aaa_size[0]);
> + printf(" addr 0x%llx/%llu", aaa->aaa_addr[0], aaa->aaa_size[0]);
I would like to keep printing of ACPI addresses/sizes consistent. So
please drop this.
> printf(" irq %d", aaa->aaa_irq[0]);
>
> - freq = acpi_getpropint(sc->sc_node, "clock-frequency", 0);
> -
> sc->sc.sc_iot = aaa->aaa_bst[0];
> sc->sc.sc_iobase = aaa->aaa_addr[0];
> - sc->sc.sc_uarttype = COM_UART_16550;
> - sc->sc.sc_frequency = freq ? freq : COM_FREQ;
I'd prefer if you'd keep the frequency code here; it isn't really
specific to the Designware code.
> -
> - sc->sc.sc_reg_width = acpi_getpropint(sc->sc_node, "reg-io-width", 4);
> - sc->sc.sc_reg_shift = acpi_getpropint(sc->sc_node, "reg-shift", 2);
> -
> - if (com_acpi_is_console(sc)) {
> - SET(sc->sc.sc_hwflags, COM_HW_CONSOLE);
> - SET(sc->sc.sc_swflags, COM_SW_SOFTCAR);
> - comconsfreq = sc->sc.sc_frequency;
> - comconsrate = B115200;
> + sc->sc.sc_frequency = COM_FREQ;
> +
> + if (com_acpi_is_designware(aaa->aaa_dev)) {
> + intrfn = com_acpi_intr_designware;
> + sc->sc.sc_uarttype = COM_UART_16550;
> + sc->sc.sc_frequency = acpi_getpropint(sc->sc_node,
> + "clock-frequency", COM_FREQ);
> + sc->sc.sc_reg_width = acpi_getpropint(sc->sc_node,
> + "reg-io-width", 4);
> + sc->sc.sc_reg_shift = acpi_getpropint(sc->sc_node,
> + "reg-shift", 2);
> +
> + if (com_acpi_is_console(sc)) {
> + SET(sc->sc.sc_hwflags, COM_HW_CONSOLE);
> + SET(sc->sc.sc_swflags, COM_SW_SOFTCAR);
> + comconsfreq = sc->sc.sc_frequency;
> + comconsrate = B115200;
> + }
> }
>
> - if (bus_space_map(sc->sc.sc_iot, aaa->aaa_addr[0], aaa->aaa_size[0],
> - 0, &sc->sc.sc_ioh)) {
> - printf(": can't map registers\n");
> - return;
> + if (sc->sc.sc_iobase == comconsaddr) {
> + sc->sc.sc_ioh = comconsioh;
> + } else {
> + if (bus_space_map(sc->sc.sc_iot,
> + aaa->aaa_addr[0], aaa->aaa_size[0], 0, &sc->sc.sc_ioh)) {
> + printf(": can't map registers\n");
> + return;
> + }
> }
>
> sc->sc_ih = acpi_intr_establish(aaa->aaa_irq[0], aaa->aaa_irq_flags[0],
> - IPL_TTY, com_acpi_intr_designware, sc, sc->sc.sc_dev.dv_xname);
> + IPL_TTY, intrfn, sc, sc->sc.sc_dev.dv_xname);
> if (sc->sc_ih == NULL) {
> printf(": can't establish interrupt\n");
> return;
> @@ -144,6 +155,12 @@ com_acpi_is_console(struct com_acpi_softc *sc)
> return 0;
> }
>
> +int
> +com_acpi_is_designware(const char *hid)
> +{
> + return strcmp("HISI0031", hid) == 0;
> +}
> +
> int
> com_acpi_intr_designware(void *cookie)
> {
>
This will have to be tested on arm64. Patrick, do you have an mcbin
up and running that you could try a (revised) diff on?
