> Date: Sun, 09 Jan 2022 06:16:12 +0900
> From: SASANO Takayoshi <u...@mx5.nisiq.net>
>
> Hello,
>
> > The com_read_reg() function returns a uint8_t, which means that only
> > the lower 8 bits of the CPR register are returned. So what you need
> > here is something like
> >
> > cpr = bus_space_read_4(sc->sc_iot, sc->sc_ioh, com_cpr << 2);
> > sc->sc_fifolen = CPR_FIFO_MODE(cpr) * 16;
>
> > With that change, the Rockchip RK3399 UART reports a 64-byte FIFO,
> > which matches the datasheet for that SoC.
>
> okay, fixed.
>
> > What happens on the H6 and H616 with that code? Does it also report a
> > non-zero FIFO size? In that case we probably should change the code a
> > bit.
>
> Allwinner's chip has no CPR and returns zero value, so that code
> results "com0 at simplebus0: DesignWare APB UART, no fifo"
> even if (the actual FIFO size is unknwon but) FIFO is implemented.
>
> How about this new diff for com.c? Minimal FIFO size is increased to 16byte
> by comment from chrisz@. And FIFO size will be displayed when meaningful
> FIFO size is obtained from CPR.
I think keeping the minimal FIFO size at 1 byte is better. That way
we'll still get the benefits of the Rx FIFO if it is implemented but
don't run the risk of overwriting characters in the output buffer if
the FIFO isn't implemented.
I have a few more suggestions below. I think the other bits were
fine, but can you post the full diff with the fixes suggested below to
be sure?
Thanks,
Mark
> Regards,
> --
> SASANO Takayoshi (JG1UAA) <u...@mx5.nisiq.net>
>
> Index: com.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/com.c,v
> retrieving revision 1.174
> diff -u -p -r1.174 com.c
> --- com.c 6 May 2021 20:35:21 -0000 1.174
> +++ com.c 8 Jan 2022 21:14:44 -0000
> @@ -1300,7 +1300,8 @@ void
> com_attach_subr(struct com_softc *sc)
> {
> int probe = 0;
> - u_int8_t lcr;
> + u_int8_t lcr, fifo;
> + u_int32_t cpr;
>
> sc->sc_ier = 0;
> /* disable interrupts */
> @@ -1480,6 +1481,26 @@ com_attach_subr(struct com_softc *sc)
> SET(sc->sc_hwflags, COM_HW_FIFO);
> sc->sc_fifolen = 256;
> break;
> + case COM_UART_DW_APB:
> + printf(": DesignWare APB UART");
Can you make this
printf(": dw16550");
to make it more consistent with the other printfs in this block?
> + SET(sc->sc_hwflags, COM_HW_FIFO);
> + cpr = bus_space_read_4(sc->sc_iot, sc->sc_ioh, com_cpr << 2);
> + sc->sc_fifolen = CPR_FIFO_MODE(cpr) * 16;
> + if (sc->sc_fifolen) {
> + printf(", %d byte fifo\n", sc->sc_fifolen);
> + } else {
> + printf("\n");
> + /*
> + * Allwinner H6's DW-APB configuration does not have
> + * CPR register and detect as no fifo.
> + * But this UART has 256 bytes FIFO and disabling FIFO
> + * makes problem; LSR_RXRDY is still set after
> + * reading com_data when FIFO is disabled (errata?).
> + * For workaround, treat as 16 byte FIFO.
> + */
If you allow me to to suggest a little bit better english:
/*
* The DW-APB configuration on the Allwinner H6 SoC
* does not provide the CPR register and will be
* detected as having no FIFO. But it does have a
* 256-byte FIFO and with the FIFO disabled the
* LSR_RXRDY bit remains set even if the input
* buffer is empty. As a workaround, treat as a
* 1-byte FIFO.
*/
> + sc->sc_fifolen = 16;
So as suggested above:
sc->sc_fifolen = 1;
> + }
> + break;
> default:
> panic("comattach: bad fifo type");
> }
> @@ -1496,10 +1517,13 @@ com_attach_subr(struct com_softc *sc)
> }
>
> /* clear and disable fifo */
> - com_write_reg(sc, com_fifo, FIFO_RCV_RST | FIFO_XMT_RST);
> + /* DW-APB UART cannot turn off FIFO here (ddb will not work) */
> + fifo = (sc->sc_uarttype == COM_UART_DW_APB) ?
> + (FIFO_ENABLE | FIFO_TRIGGER_1) : 0;
> + com_write_reg(sc, com_fifo, fifo | FIFO_RCV_RST | FIFO_XMT_RST);
> if (ISSET(com_read_reg(sc, com_lsr), LSR_RXRDY))
> (void)com_read_reg(sc, com_data);
> - com_write_reg(sc, com_fifo, 0);
> + com_write_reg(sc, com_fifo, fifo);
I think we should investigate leaving the FIFO enabled (if supported)
for all chip variants. But that is probably best done as a separate
change, so this is fine for now.
>
> sc->sc_mcr = 0;
> com_write_reg(sc, com_mcr, sc->sc_mcr);
>
