On Mon, Dec 13, 2021 at 11:15:00PM +0100, Mark Kettenis wrote:
> > Date: Fri, 10 Dec 2021 07:56:44 +0100
> > From: Anton Lindqvist <[email protected]>
> >
> > On Tue, Dec 07, 2021 at 01:08:45PM +0100, Mark Kettenis wrote:
> > > > 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?
> >
> > Updated diff addressing the feedback from kettenis.
>
> Looks good to me. ok kettenis@ provided that Patrick confirms serial console
> still works on the macchiatobin.
Still works for me. :)
> > diff --git share/man/man4/com.4 share/man/man4/com.4
> > index 73b421f2ca7..efb0722ca11 100644
> > --- share/man/man4/com.4
> > +++ share/man/man4/com.4
> > @@ -61,11 +61,10 @@
> > .Cd "com0 at isa? port 0x3f8 irq 4"
> > .Cd "com1 at isa? port 0x2f8 irq 3"
> > .Pp
> > -.Cd "# armv7"
> > -.Cd "com* at fdt?"
> > -.Pp
> > -.Cd "# arm64"
> > +.Cd "# amd64 and arm64"
> > .Cd "com* at acpi?"
> > +.Pp
> > +.Cd "# arm64 and armv7"
> > .Cd "com* at fdt?"
> > .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
> >
> > 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 1bf5eb134ab..e24ac633905 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..ab72b33a127 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;
> > @@ -88,31 +90,39 @@ com_acpi_attach(struct device *parent, struct device
> > *self, void *aux)
> > printf(" addr 0x%llx/0x%llx", aaa->aaa_addr[0], aaa->aaa_size[0]);
> > 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;
> > -
> > - 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 = acpi_getpropint(sc->sc_node, "clock-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_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 +154,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)
> > {
> >
>
