Re: [PATCH v3 00/12] Add RS485 support to DW UART

[Vicente Bergas <vicencb@gmail.com>]

Hi Ilpo, thank you for your quick reply with a solution!

On Fri, Apr 22, 2022 at 3:07 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Fri, 22 Apr 2022, Ilpo Järvinen wrote:
>
> > On Thu, 21 Apr 2022, Vicente Bergas wrote:
> >
> > > 1.- rs485_stop_tx is never called because there are no interrupts.
> > > I worked around this by disabling DMA:
> >
> > I'll need to look into this.
>
> 8250 DMA tx complete path lacks calls to normal 8250 stop handling and
> I think it probably also assumes too much from dmaengine's completion
> callback. ...It also seems bit early to call serial8250_rpm_put_tx from
> there(?).
>
> This patch allowed em485_start/stop_tx to be called in my tests:
> [PATCH 1/1] serial: 8250: use THRE & __stop_tx also with DMA

I confirm that this patch fixes the issue when DMA is enabled.

I also confirm that your other patch
> +                       stop_delay = p->port.frame_time + DIV_ROUND_UP(p->port.frame_time, 7);
fixes the issue with RTS/DriverEnable deassertion time.
I've tested it again at 19200e1 and now RTS is deasserted
approximately at the same time as the end of the stop bit.
Please, note the "e" for even parity bit, that extra bit after the
data byte might have an impact on this timings.

Regards,
  Vicente.

On Fri, Apr 22, 2022 at 3:07 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Fri, 22 Apr 2022, Ilpo Järvinen wrote:
>
> > On Thu, 21 Apr 2022, Vicente Bergas wrote:
> >
> > > i have tested your v3 patch on v3 hardware, that is, using the
> > > emulated em485 because of lack of HW support. It is not working
> > > due to three issues.
> >
> > Thanks for testing!
> >
> > > 1.- rs485_stop_tx is never called because there are no interrupts.
> > > I worked around this by disabling DMA:
> > >
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -577,3 +577,3 @@ static int dw8250_probe(struct platform_device *pdev)
> > >             data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
> > >             data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
> > > -           up->dma = &data->data.dma;
> > > +           up->dma = 0; // Proof of concept, not to be merged!
> >
> > I'll need to look into this.
>
> 8250 DMA tx complete path lacks calls to normal 8250 stop handling and
> I think it probably also assumes too much from dmaengine's completion
> callback. ...It also seems bit early to call serial8250_rpm_put_tx from
> there(?).
>
> This patch allowed em485_start/stop_tx to be called in my tests:
>
>
> [PATCH 1/1] serial: 8250: use THRE & __stop_tx also with DMA
>
> Currently, DMA complete handling in 8250 driver does not use THRE
> to detect true completion of the tx and also doesn't call __stop_tx.
> This leads to problems with em485 that needs to handle RTS timing.
>
> Instead of handling tx stop within 8250 dma code, enable THRE when
> tx data runs out and tweak serial8250_handle_irq to call
> __stop_tx when uart is using DMA.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dma.c  | 4 ++--
>  drivers/tty/serial/8250/8250_port.c | 9 ++++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 890fa7ddaa7f..e84db0abf365 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -29,12 +29,13 @@ static void __dma_tx_complete(void *param)
>         xmit->tail += dma->tx_size;
>         xmit->tail &= UART_XMIT_SIZE - 1;
>         p->port.icount.tx += dma->tx_size;
> +       dma->tx_size = 0;
>
>         if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>                 uart_write_wakeup(&p->port);
>
>         ret = serial8250_tx_dma(p);
> -       if (ret)
> +       if (ret || !dma->tx_size)
>                 serial8250_set_THRI(p);
>
>         spin_unlock_irqrestore(&p->port.lock, flags);
> @@ -71,7 +72,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
>
>         if (uart_tx_stopped(&p->port) || uart_circ_empty(xmit)) {
>                 /* We have been called from __dma_tx_complete() */
> -               serial8250_rpm_put_tx(p);
>                 return 0;
>         }
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index b3289ef117d1..72f144449ee1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1960,9 +1960,12 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
>                         status = serial8250_rx_chars(up, status);
>         }
>         serial8250_modem_status(up);
> -       if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE) &&
> -               (up->ier & UART_IER_THRI))
> -               serial8250_tx_chars(up);
> +       if ((status & UART_LSR_THRE) && (up->ier & UART_IER_THRI)) {
> +               if (!up->dma || up->dma->tx_err)
> +                       serial8250_tx_chars(up);
> +               else
> +                       __stop_tx(up);
> +       }
>
>         uart_unlock_and_check_sysrq_irqrestore(port, flags);
>
> --
> 2.35.1