| Message ID | 20200626200033.1528052-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | serial: qcom_geni_serial: Use the FIFOs properly for console | expand |
Hi, On Fri, Jul 10, 2020 at 10:46 AM Evan Green <evgreen@chromium.org> wrote: > > On Fri, Jun 26, 2020 at 1:01 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > The geni serial driver had a rule that we'd only use 1 byte per FIFO > > word for the TX FIFO if we were being used for the serial console. > > This is ugly and a bit of a pain. It's not too hard to fix, so fix > > it. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/tty/serial/qcom_geni_serial.c | 57 +++++++++++++++++---------- > > 1 file changed, 37 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > > index 4610e391e886..583d903321b5 100644 > > --- a/drivers/tty/serial/qcom_geni_serial.c > > +++ b/drivers/tty/serial/qcom_geni_serial.c > > @@ -103,12 +103,18 @@ > > #define DEFAULT_IO_MACRO_IO2_IO3_MASK GENMASK(15, 4) > > #define IO_MACRO_IO2_IO3_SWAP 0x4640 > > > > +/* We always configure 4 bytes per FIFO word */ > > +#define BYTES_PER_FIFO_WORD 4 > > + > > struct qcom_geni_private_data { > > /* NOTE: earlycon port will have NULL here */ > > struct uart_driver *drv; > > > > u32 poll_cached_bytes; > > unsigned int poll_cached_bytes_cnt; > > + > > + u32 write_cached_bytes; > > + unsigned int write_cached_bytes_cnt; > > }; > > > > struct qcom_geni_serial_port { > > @@ -121,8 +127,6 @@ struct qcom_geni_serial_port { > > bool setup; > > int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop); > > unsigned int baud; > > - unsigned int tx_bytes_pw; > > - unsigned int rx_bytes_pw; > > void *rx_fifo; > > u32 loopback; > > bool brk; > > @@ -390,13 +394,25 @@ static void qcom_geni_serial_poll_put_char(struct uart_port *uport, > > #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE > > static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch) > > { > > - writel(ch, uport->membase + SE_GENI_TX_FIFOn); > > + struct qcom_geni_private_data *private_data = uport->private_data; > > + > > + private_data->write_cached_bytes = > > + (private_data->write_cached_bytes >> 8) | (ch << 24); > > + private_data->write_cached_bytes_cnt++; > > + > > + if (private_data->write_cached_bytes_cnt == BYTES_PER_FIFO_WORD) { > > + writel(private_data->write_cached_bytes, > > + uport->membase + SE_GENI_TX_FIFOn); > > + private_data->write_cached_bytes_cnt = 0; > > + } > > } > > > > static void > > __qcom_geni_serial_console_write(struct uart_port *uport, const char *s, > > unsigned int count) > > { > > + struct qcom_geni_private_data *private_data = uport->private_data; > > + > > int i; > > u32 bytes_to_send = count; > > > > @@ -431,6 +447,15 @@ __qcom_geni_serial_console_write(struct uart_port *uport, const char *s, > > SE_GENI_M_IRQ_CLEAR); > > i += chars_to_write; > > } > > + > > + if (private_data->write_cached_bytes_cnt) { > > + private_data->write_cached_bytes >>= BITS_PER_BYTE * > > + (BYTES_PER_FIFO_WORD - private_data->write_cached_bytes_cnt); > > + writel(private_data->write_cached_bytes, > > + uport->membase + SE_GENI_TX_FIFOn); > > + private_data->write_cached_bytes_cnt = 0; > > + } > > How does this not end up sending stray zeros? In other words, how does > the hardware know which bytes of this word are valid? We told it how many bytes we wanted to send in qcom_geni_serial_setup_tx(). If the total number of bytes being sent is not a multiple of the FIFO word size then it knows that the last word will be a partial and it'll extract just the number of needed bytes out of it. Like receiving, sending bytes out of geni is also packet based. Though the packets work a little differently for sending vs. receiving in both cases you are supposed to fully finish a packet before you send more bytes (you can sorta cancel / start a new packet, but that's not what we're doing here). So ahead of time we told it how many bytes to expect and then we sent them all. NOTE: if we wanted to simplify this function at the expense of efficiency, we could change it to always send 1-byte packets. Then we'd start a packet, send 1 byte, wait for done, start a new packet, send 1 byte, wait for done, etc. In fact, that's how the polling code does it... -Doug