arm: replace sxiuart with com(4)

[Patrick Wildt]

Hi,

the Synopsys Designware UART, which is used on a bunch of SoCs, is
mostly compatible to com(4). Currently we use sxiuart, a driver based
on another driver, modified to work on sunxi. I would like to replace
sxiuart with com(4). This diff disables sxiuart and implements the
Allwinner and Synopsys specific code in the existing com(4) wrapper
for OMAP. In a follow up diff we can remove sxiuart completely. Also,
we might want to move it to some other folder and/or rename it. That
should be part of a follow up diff.

There's only a slight twist: The controller has a busy interrupt we
need to handle. Otherwise we will get flooded with interrupts.
mglocker@ is currently experiencing that issue with the sxiuart driver
as well. This diff also implements this busy indicator in com(4).

I guess the com(4) and the armv7 changes should be committed separately.
This diff combines both to show what it will be used for.

I tested it on BBB and Lamobo R1. mglocker@ reported success on his
sunxi-based CHIP.

ok? Comments?

Patrick


diff --git a/sys/arch/armv7/armv7/platform.c b/sys/arch/armv7/armv7/platform.c
index f122c1f..390d907 100644
--- a/sys/arch/armv7/armv7/platform.c
+++ b/sys/arch/armv7/armv7/platform.c
@@ -37,7 +37,6 @@ static struct armv7_platform *platform;
void exuart_init_cons(void);
void imxuart_init_cons(void);
void omapuart_init_cons(void);
-void sxiuart_init_cons(void);
void pl011_init_cons(void);

struct armv7_platform *imx_platform_match(void);
@@ -102,7 +101,6 @@ platform_init_cons(void)
exuart_init_cons();
imxuart_init_cons();
omapuart_init_cons();
- sxiuart_init_cons();
pl011_init_cons();
}

diff --git a/sys/arch/armv7/conf/GENERIC b/sys/arch/armv7/conf/GENERIC
index c3f0311..5ddaea7 100644
--- a/sys/arch/armv7/conf/GENERIC
+++ b/sys/arch/armv7/conf/GENERIC
@@ -91,7 +91,7 @@ sxiccmu* at sunxi? # Clock Control Module/Unit
sxitimer* at sunxi?
sxidog* at sunxi? # watchdog timer
sxirtc* at sunxi? # Real Time Clock
-sxiuart* at fdt? # onboard UARTs
+#sxiuart* at fdt? # onboard UARTs
sxie* at fdt?
ahci* at sunxi? # AHCI/SATA (shim)
ehci* at sunxi? # EHCI (shim)
diff --git a/sys/arch/armv7/conf/RAMDISK b/sys/arch/armv7/conf/RAMDISK
index 4a3c53e..36fb0dc 100644
--- a/sys/arch/armv7/conf/RAMDISK
+++ b/sys/arch/armv7/conf/RAMDISK
@@ -90,7 +90,7 @@ sxiccmu* at sunxi? # Clock Control Module/Unit
sxitimer* at sunxi?
sxidog* at sunxi? # watchdog timer
sxirtc* at sunxi? # Real Time Clock
-sxiuart* at fdt? # onboard UARTs
+#sxiuart* at fdt? # onboard UARTs
sxie* at fdt?
ahci* at sunxi? # AHCI/SATA (shim)
ehci* at sunxi? # EHCI (shim)
diff --git a/sys/arch/armv7/omap/omap_com.c b/sys/arch/armv7/omap/omap_com.c
index 20a51b4..5aa82f7 100644
--- a/sys/arch/armv7/omap/omap_com.c
+++ b/sys/arch/armv7/omap/omap_com.c
@@ -76,14 +76,23 @@ omapuart_init_cons(void)
{
struct fdt_reg reg;
void *node;
+ int freq = 48000000;

- if ((node = fdt_find_cons("ti,omap3-uart")) == NULL)
- if ((node = fdt_find_cons("ti,omap4-uart")) == NULL)
+ if ((node = fdt_find_cons("ti,omap3-uart")) == NULL &&
+ (node = fdt_find_cons("ti,omap4-uart")) == NULL &&
+ (node = fdt_find_cons("snps,dw-apb-uart")) == NULL)
return;
if (fdt_get_reg(node, 0, &reg))
return;

- comcnattach(&armv7_a4x_bs_tag, reg.addr, comcnspeed, 48000000,
+ if ((node = fdt_find_node("/")) != NULL &&
+ (fdt_is_compatible(node, "allwinner,sun4i-a10") ||
+ fdt_is_compatible(node, "allwinner,sun5i-a10s") ||
+ fdt_is_compatible(node, "allwinner,sun5i-r8") ||
+ fdt_is_compatible(node, "allwinner,sun7i-a20")))
+ freq = 24000000;
+
+ comcnattach(&armv7_a4x_bs_tag, reg.addr, comcnspeed, freq,
comcnmode);
comdefaultrate = comcnspeed;
}
@@ -94,7 +103,8 @@ omapuart_match(struct device *parent, void *match, void *aux)
struct fdt_attach_args *faa = aux;

return (OF_is_compatible(faa->fa_node, "ti,omap3-uart") ||
- OF_is_compatible(faa->fa_node, "ti,omap4-uart"));
+ OF_is_compatible(faa->fa_node, "ti,omap4-uart") ||
+ OF_is_compatible(faa->fa_node, "snps,dw-apb-uart"));
}

void
@@ -102,7 +112,7 @@ omapuart_attach(struct device *parent, struct device *self, void *aux)
{
struct com_softc *sc = (struct com_softc *)self;
struct fdt_attach_args *faa = aux;
- int irq;
+ int irq, node;

if (faa->fa_nreg != 1 || (faa->fa_nintr != 1 && faa->fa_nintr != 3))
return;
@@ -123,7 +133,21 @@ omapuart_attach(struct device *parent, struct device *self, void *aux)
return;
}

- sitara_cm_pinctrlbyname(faa->fa_node, "default");
+ if (OF_is_compatible(faa->fa_node, "ti,omap3-uart") ||
+ OF_is_compatible(faa->fa_node, "ti,omap4-uart"))
+ sitara_cm_pinctrlbyname(faa->fa_node, "default");
+
+ if (OF_is_compatible(faa->fa_node, "snps,dw-apb-uart")) {
+ sc->sc_hwflags |= COM_HW_IIR_BUSY;
+ sc->sc_uarttype = COM_UART_16550;
+ }
+
+ if ((node = OF_finddevice("/")) != 0 &&
+ (OF_is_compatible(node, "allwinner,sun4i-a10") ||
+ OF_is_compatible(node, "allwinner,sun5i-a10s") ||
+ OF_is_compatible(node, "allwinner,sun5i-r8") ||
+ OF_is_compatible(node, "allwinner,sun7i-a20")))
+ sc->sc_frequency = 24000000;

com_attach_subr(sc);

diff --git a/sys/dev/ic/com.c b/sys/dev/ic/com.c
index 000eea3..0bf7181 100644
--- a/sys/dev/ic/com.c
+++ b/sys/dev/ic/com.c
@@ -1219,12 +1219,30 @@ comintr(void *arg)
bus_space_tag_t iot = sc->sc_iot;
bus_space_handle_t ioh = sc->sc_ioh;
struct tty *tp;
- u_char lsr, data, msr, delta;
+ u_char iir, lsr, data, msr, delta;
+ int timeout;
+
+ iir = bus_space_read_1(iot, ioh, com_iir);
+
+ /* Handle ns16750-specific busy interrupt. */
+ if (ISSET(sc->sc_hwflags, COM_HW_IIR_BUSY) &&
+ (iir & IIR_BUSY) == IIR_BUSY) {
+ for (timeout = 10000;
+ (bus_space_read_1(iot, ioh, com_usr) & 0x1) != 0;
+ timeout--)
+ if (timeout <= 0) {
+ printf("%s: timeout while waiting for BUSY "
+ "interrupt acknowledge\n",
+ sc->sc_dev.dv_xname);
+ return (0);
+ }
+ iir = bus_space_read_1(iot, ioh, com_iir);
+ }

if (!sc->sc_tty)
return (0); /* Can't do squat. */

- if (ISSET(bus_space_read_1(iot, ioh, com_iir), IIR_NOPEND))
+ if (ISSET(iir, IIR_NOPEND))
return (0);

tp = sc->sc_tty;
diff --git a/sys/dev/ic/comreg.h b/sys/dev/ic/comreg.h
index 6084698..d2e52a0 100644
--- a/sys/dev/ic/comreg.h
+++ b/sys/dev/ic/comreg.h
@@ -82,6 +82,7 @@
#define IIR_MLSC 0x0 /* Modem status */
#define IIR_NOPEND 0x1 /* No pending interrupts */
#define IIR_FIFO_MASK 0xc0 /* set if FIFOs are enabled */
+#define IIR_BUSY 0x7 /* NS16750: Busy indicator */

/* fifo control register */
#define FIFO_ENABLE 0x01 /* Turn the FIFO on */
diff --git a/sys/dev/ic/comvar.h b/sys/dev/ic/comvar.h
index 893ffde..5f7e5fe 100644
--- a/sys/dev/ic/comvar.h
+++ b/sys/dev/ic/comvar.h
@@ -106,6 +106,7 @@ struct com_softc {
u_char sc_hwflags;
#define COM_HW_NOIEN 0x01
#define COM_HW_FIFO 0x02
+#define COM_HW_IIR_BUSY 0x04
#define COM_HW_SIR 0x20
#define COM_HW_CONSOLE 0x40
#define COM_HW_KGDB 0x80
@@ -117,7 +118,7 @@ struct com_softc {
#define COM_SW_PPS 0x10
#define COM_SW_DEAD 0x20
int sc_fifolen;
- u_char sc_msr, sc_mcr, sc_lcr, sc_ier;
+ u_char sc_msr, sc_mcr, sc_lcr, sc_ier, sc_iir;
u_char sc_dtr;

u_char sc_cua;
diff --git a/sys/dev/ic/ns16550reg.h b/sys/dev/ic/ns16550reg.h
index 5db1a27..d8eb8b3 100644
--- a/sys/dev/ic/ns16550reg.h
+++ b/sys/dev/ic/ns16550reg.h
@@ -50,3 +50,4 @@
#define com_lsr 5 /* line status register (R/W) */
#define com_msr 6 /* modem status register (R/W) */
#define com_scratch 7 /* scratch register (R/W) */
+#define com_usr 31 /* NS16750: status register (R) */

[Theo de Raadt]

Can the iir read be after the tty check, or must it be before?

[Patrick Wildt]

On Wed, Aug 03, 2016 at 02:56:53PM -0600, Theo de Raadt wrote:
> Can the iir read be after the tty check, or must it be before?
>

When I did that diff initially I had the read and busy check
after the sc->sc_tty check. The hardware I created this diff
for has a shared interrupt line for two uarts. So I opened
one uart, and the other not-opened (but attached) uart flooded
me. Then I moved the lines around, so that even though there
is no console opened, the interrupt flood would be taken care
of.

The Allwinner, OMAP and Marvell hardware do not share a single
line between the uarts. Thus this part of the diff is not needed.

For the FreeScale LS1 hardware it might be a better fix to find
a way to disable an un-opened uart completely, so that it does
not even try throwing interrupts around. Then this part of the
diff would also not be needed.

Long story short, I guess I can keep it as it was before.

[Mark Kettenis]

> Date: Wed, 3 Aug 2016 22:36:02 +0200
> From: Patrick Wildt <pat...@blueri.se>

>
> Hi,
>
> the Synopsys Designware UART, which is used on a bunch of SoCs, is
> mostly compatible to com(4). Currently we use sxiuart, a driver based
> on another driver, modified to work on sunxi. I would like to replace
> sxiuart with com(4). This diff disables sxiuart and implements the
> Allwinner and Synopsys specific code in the existing com(4) wrapper
> for OMAP. In a follow up diff we can remove sxiuart completely. Also,
> we might want to move it to some other folder and/or rename it. That
> should be part of a follow up diff.
>
> There's only a slight twist: The controller has a busy interrupt we
> need to handle. Otherwise we will get flooded with interrupts.
> mglocker@ is currently experiencing that issue with the sxiuart driver
> as well. This diff also implements this busy indicator in com(4).
>

> ok? Comments?

Documentation for the Designware IP can be found at:

http://read.pudn.com/downloads163/doc/741013/dw_apb_uart_db.pdf

I don't think the com(4) part of the diff is quite right. This busy
indicator feature seems to be specific to the Designware core. I
can't find the original NS16750 datasheet (was there ever one?), but
it isn't present on any of the true 16750 clones. So I think that
instead of adding a COM_HW_IIR_BUSY flag, we should add a
COM_UART_DWAPB type and use that to signal the presence of this
feature. The comments should certainly not mention the NS16750.

But I'd also like to look at the documentation to see if there way to
work around this issue. It seems that the interrupt handlers is
simply waiting for the FIFO to drain. It would really surprise me if
that is really necessary.

[Theo de Raadt]

> I don't think the com(4) part of the diff is quite right. This busy
> indicator feature seems to be specific to the Designware core. I
> can't find the original NS16750 datasheet (was there ever one?), but
> it isn't present on any of the true 16750 clones. So I think that
> instead of adding a COM_HW_IIR_BUSY flag, we should add a
> COM_UART_DWAPB type and use that to signal the presence of this
> feature.

Patrick send me a newer version of the diff with better documentation.

And that makes the same is clear to me. At least one of these flags
should be changed to make sure it is narrowly specific to this
particular defective chip implementation.

> The comments should certainly not mention the NS16750.

Right. In any case it is not a NS16750. It is a fresh piece of
hardware, that mostly behaves like a NS16750, except in at least this
way. (A whole generation of shitty chips are supersets of NS16750).

> But I'd also like to look at the documentation to see if there way to
> work around this issue. It seems that the interrupt handlers is
> simply waiting for the FIFO to drain. It would really surprise me if
> that is really necessary.

I suspect the chip was implimented as a microcode, and they made a
mistake, so they added a flag. It am guessing the interrupt stays
asserted. Stalling in the interrupt handler seems odd. I'm going to
guess this only happens once, at startup? That might match Mark's
theory.

[Artturi Alm]

On Wed, Aug 03, 2016 at 03:33:21PM -0600, Theo de Raadt wrote:
> > I don't think the com(4) part of the diff is quite right. This busy
> > indicator feature seems to be specific to the Designware core. I
> > can't find the original NS16750 datasheet (was there ever one?), but
> > it isn't present on any of the true 16750 clones. So I think that
> > instead of adding a COM_HW_IIR_BUSY flag, we should add a
> > COM_UART_DWAPB type and use that to signal the presence of this
> > feature.
>
> Patrick send me a newer version of the diff with better documentation.
>
> And that makes the same is clear to me. At least one of these flags
> should be changed to make sure it is narrowly specific to this
> particular defective chip implementation.
>
> > The comments should certainly not mention the NS16750.
>
> Right. In any case it is not a NS16750. It is a fresh piece of
> hardware, that mostly behaves like a NS16750, except in at least this
> way. (A whole generation of shitty chips are supersets of NS16750).
>

Iirc. it is 8250.

"* The Synopsys DesignWare 8250 has an extra feature whereby it detects if the
* LCR is written whilst busy. If it is, then a busy detect interrupt is
* raised, the LCR needs to be rewritten and the uart status register read."

(about placing uart.c; fwiw., i'd start by moving arch/arm/cortex/*
under arch/armv7/dev/, and do rest of the likely upcoming generic/shared
code/glue/drivers there.)

-Artturi

[Theo de Raadt]

> > Patrick send me a newer version of the diff with better documentation.
> >
> > And that makes the same is clear to me. At least one of these flags
> > should be changed to make sure it is narrowly specific to this
> > particular defective chip implementation.
> >
> > > The comments should certainly not mention the NS16750.
> >
> > Right. In any case it is not a NS16750. It is a fresh piece of
> > hardware, that mostly behaves like a NS16750, except in at least this
> > way. (A whole generation of shitty chips are supersets of NS16750).
> >
>
> Iirc. it is 8250.

It absolutely is not an 8250!

I estimate the 8250 was last delivered in a new computer around 1995.
They became extinct around that year when Intel replaced them with a
newer product. Which were buggy in new ways.

Calling a thing that arrives in 2016 product an 8250 is like calling a
Ford Fiesta a Model T. My grandfather crashed a Model T going down a
hill, and in a way that would never happen with a Ford Fiesta (over
rev on gearbox without a full-disengage clutch -> piston out the
side). Get outside a little more, understand this is not the same
era.

If it was an 8250, it would not have the extra registers which are
probable, and "hint" that additional behaviours are present. And it
is worse -- those new behaviours are not just present... almost
immediately upon introduction of the (broken) FIFO features in newer
chips, BIOS's started initializing th additional behaviours, and so
drivers had to cope with them being in existance.

It was, and to this day, remains a mess.

It is not an 8250, it is something entirely different. If it was an
8250, > 70% of com.c could be deleted.

Rest of what you say is muted by that comment -- it is such incredible
balony! Things are not simple, 1 line comments like that are not
helpful.

[Marcus Glocker]

On Wed, Aug 03, 2016 at 03:33:21PM -0600, Theo de Raadt wrote:

> > I don't think the com(4) part of the diff is quite right. This busy
> > indicator feature seems to be specific to the Designware core. I
> > can't find the original NS16750 datasheet (was there ever one?), but
> > it isn't present on any of the true 16750 clones. So I think that
> > instead of adding a COM_HW_IIR_BUSY flag, we should add a
> > COM_UART_DWAPB type and use that to signal the presence of this
> > feature.
>

> Patrick send me a newer version of the diff with better documentation.
>
> And that makes the same is clear to me. At least one of these flags
> should be changed to make sure it is narrowly specific to this
> particular defective chip implementation.
>
> > The comments should certainly not mention the NS16750.
>
> Right. In any case it is not a NS16750. It is a fresh piece of
> hardware, that mostly behaves like a NS16750, except in at least this
> way. (A whole generation of shitty chips are supersets of NS16750).
>

> > But I'd also like to look at the documentation to see if there way to
> > work around this issue. It seems that the interrupt handlers is
> > simply waiting for the FIFO to drain. It would really surprise me if
> > that is really necessary.
>
> I suspect the chip was implimented as a microcode, and they made a
> mistake, so they added a flag. It am guessing the interrupt stays
> asserted. Stalling in the interrupt handler seems odd. I'm going to
> guess this only happens once, at startup? That might match Mark's
> theory.

In my case the sxiuart console works fine after startup until a certain
situation happens. E.g. I can to 99% trigger the issue by starting top,
and hit 's'. Afterwards sxiuart interrupts are stalled until the next
reboot. Strange thing is that initially sxiuart can handle some busy
interrupts. I don't know exactly what happens on top hit 's', maybe a
lot of busy interrupts get triggered by then.

I tried Patricks part of the diff which handles the busy interrupts
directly in sxiuart, but the issue remains there.