| Message ID | 20161213162756.16139-1-richard.genoud@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Le 13/12/2016 à 17:27, Richard Genoud a écrit : > If we don't disable the transmitter in atmel_stop_tx, the DMA buffer > continues to send data until it is emptied. > This cause problems with the flow control (CTS is asserted and data are > still sent). > > So, disabling the transmitter in atmel_stop_tx is a sane thing to do. > > Tested on at91sam9g35-cm(DMA) > Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) > > Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > drivers/tty/serial/atmel_serial.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > NB: this is not for the 4.10 merge window, I'm just sending it now to > have some comments if someone is againts it. > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index 168b10cad47b..f9d42de5ab2d 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) > /* disable PDC transmit */ > atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); > } > + > + /* > + * Disable the transmitter. > + * This is mandatory when DMA is used, otherwise the DMA buffer > + * is fully transmitted. > + */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); > + > /* Disable interrupts */ > atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); > > @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) > > /* Enable interrupts */ > atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); > + > + /* re-enable the transmitter */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); > } > > /* >
2017-01-02 12:13 GMT+01:00 Nicolas Ferre <nicolas.ferre@atmel.com>: > Le 13/12/2016 à 17:27, Richard Genoud a écrit : >> If we don't disable the transmitter in atmel_stop_tx, the DMA buffer >> continues to send data until it is emptied. >> This cause problems with the flow control (CTS is asserted and data are >> still sent). >> >> So, disabling the transmitter in atmel_stop_tx is a sane thing to do. >> >> Tested on at91sam9g35-cm(DMA) >> Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) >> >> Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > >> --- >> drivers/tty/serial/atmel_serial.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> NB: this is not for the 4.10 merge window, I'm just sending it now to >> have some comments if someone is againts it. >> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> index 168b10cad47b..f9d42de5ab2d 100644 >> --- a/drivers/tty/serial/atmel_serial.c >> +++ b/drivers/tty/serial/atmel_serial.c >> @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) >> /* disable PDC transmit */ >> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); >> } >> + >> + /* >> + * Disable the transmitter. >> + * This is mandatory when DMA is used, otherwise the DMA buffer >> + * is fully transmitted. >> + */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); >> + >> /* Disable interrupts */ >> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); >> >> @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) >> >> /* Enable interrupts */ >> atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); >> + >> + /* re-enable the transmitter */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); >> } >> >> /* >> > > > -- > Nicolas Ferre Greg, could you also take this patch in your tree ? Thanks ! Richard.
On Mon, Jan 02, 2017 at 03:16:46PM +0100, Richard Genoud wrote: > 2017-01-02 12:13 GMT+01:00 Nicolas Ferre <nicolas.ferre@atmel.com>: > > Le 13/12/2016 à 17:27, Richard Genoud a écrit : > >> If we don't disable the transmitter in atmel_stop_tx, the DMA buffer > >> continues to send data until it is emptied. > >> This cause problems with the flow control (CTS is asserted and data are > >> still sent). > >> > >> So, disabling the transmitter in atmel_stop_tx is a sane thing to do. > >> > >> Tested on at91sam9g35-cm(DMA) > >> Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) > >> > >> Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) > >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > > > >> --- > >> drivers/tty/serial/atmel_serial.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> NB: this is not for the 4.10 merge window, I'm just sending it now to > >> have some comments if someone is againts it. > >> > >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > >> index 168b10cad47b..f9d42de5ab2d 100644 > >> --- a/drivers/tty/serial/atmel_serial.c > >> +++ b/drivers/tty/serial/atmel_serial.c > >> @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) > >> /* disable PDC transmit */ > >> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); > >> } > >> + > >> + /* > >> + * Disable the transmitter. > >> + * This is mandatory when DMA is used, otherwise the DMA buffer > >> + * is fully transmitted. > >> + */ > >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); > >> + > >> /* Disable interrupts */ > >> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); > >> > >> @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) > >> > >> /* Enable interrupts */ > >> atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); > >> + > >> + /* re-enable the transmitter */ > >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); > >> } > >> > >> /* > >> > > > > > > -- > > Nicolas Ferre > > Greg, could you also take this patch in your tree ? Yup!
Le 13/12/2016 à 17:27, Richard Genoud a écrit : > If we don't disable the transmitter in atmel_stop_tx, the DMA buffer > continues to send data until it is emptied. > This cause problems with the flow control (CTS is asserted and data are > still sent). > > So, disabling the transmitter in atmel_stop_tx is a sane thing to do. > > Tested on at91sam9g35-cm(DMA) > Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) > > Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > --- > drivers/tty/serial/atmel_serial.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > NB: this is not for the 4.10 merge window, I'm just sending it now to > have some comments if someone is againts it. > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index 168b10cad47b..f9d42de5ab2d 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) > /* disable PDC transmit */ > atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); > } > + > + /* > + * Disable the transmitter. > + * This is mandatory when DMA is used, otherwise the DMA buffer > + * is fully transmitted. > + */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); > + > /* Disable interrupts */ > atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); > > @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) > > /* Enable interrupts */ > atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); > + > + /* re-enable the transmitter */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); > } > > /* Hi Richard, I've just discovered that I have some weird behavior with this patch. On current Linus' tree, with sama5d2 + DMA, I see some garbage characters coming out of the console when I try to stop my system (reboot/halt) [1]. Moreover, and I do understand that it's not the problem right here, when applied on our linux-4.4-at91 branch (our vendor tree actually), it hangs the boot process as it seems that a burst of open/close of the serial port happens while starting the rootfs. It's definitively my own problem, but it can bring light to what we are seeing on Mainline... I think that we may need to flush the DMA channel in this atmel_stop_tx() function. Best regards, [1] If you want to test, you need to apply this patch for eMMC BTW: https://patchwork.kernel.org/patch/9617489/
2017-03-15 12:37 GMT+01:00 Nicolas Ferre <nicolas.ferre@atmel.com>: > Le 13/12/2016 à 17:27, Richard Genoud a écrit : >> If we don't disable the transmitter in atmel_stop_tx, the DMA buffer >> continues to send data until it is emptied. >> This cause problems with the flow control (CTS is asserted and data are >> still sent). >> >> So, disabling the transmitter in atmel_stop_tx is a sane thing to do. >> >> Tested on at91sam9g35-cm(DMA) >> Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) >> >> Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> >> --- >> drivers/tty/serial/atmel_serial.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> NB: this is not for the 4.10 merge window, I'm just sending it now to >> have some comments if someone is againts it. >> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> index 168b10cad47b..f9d42de5ab2d 100644 >> --- a/drivers/tty/serial/atmel_serial.c >> +++ b/drivers/tty/serial/atmel_serial.c >> @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) >> /* disable PDC transmit */ >> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); >> } >> + >> + /* >> + * Disable the transmitter. >> + * This is mandatory when DMA is used, otherwise the DMA buffer >> + * is fully transmitted. >> + */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); >> + >> /* Disable interrupts */ >> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); >> >> @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) >> >> /* Enable interrupts */ >> atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); >> + >> + /* re-enable the transmitter */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); >> } >> >> /* > > Hi Richard, Hi ! > > I've just discovered that I have some weird behavior with this patch. On > current Linus' tree, with sama5d2 + DMA, I see some garbage characters > coming out of the console when I try to stop my system (reboot/halt) [1]. Yes, I've also seen that on my board, it was on my todo list. (Although I didn't know this patch was the culprit !) > > Moreover, and I do understand that it's not the problem right here, when > applied on our linux-4.4-at91 branch (our vendor tree actually), it > hangs the boot process as it seems that a burst of open/close of the > serial port happens while starting the rootfs. It's definitively my own > problem, but it can bring light to what we are seeing on Mainline... aïe ! This is clearly something we'll have to understand ! > I think that we may need to flush the DMA channel in this > atmel_stop_tx() function. > yes, I'll look into that. > Best regards, > > [1] If you want to test, you need to apply this patch for eMMC BTW: > https://patchwork.kernel.org/patch/9617489/ > > -- > Nicolas Ferre Thanks ! Richard.
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index 168b10cad47b..f9d42de5ab2d 100644 --- a/drivers/tty/serial/atmel_serial.c +++ b/drivers/tty/serial/atmel_serial.c @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) /* disable PDC transmit */ atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); } + + /* + * Disable the transmitter. + * This is mandatory when DMA is used, otherwise the DMA buffer + * is fully transmitted. + */ + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); + /* Disable interrupts */ atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) /* Enable interrupts */ atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); + + /* re-enable the transmitter */ + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); } /*
If we don't disable the transmitter in atmel_stop_tx, the DMA buffer continues to send data until it is emptied. This cause problems with the flow control (CTS is asserted and data are still sent). So, disabling the transmitter in atmel_stop_tx is a sane thing to do. Tested on at91sam9g35-cm(DMA) Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) Signed-off-by: Richard Genoud <richard.genoud@gmail.com> --- drivers/tty/serial/atmel_serial.c | 11 +++++++++++ 1 file changed, 11 insertions(+) NB: this is not for the 4.10 merge window, I'm just sending it now to have some comments if someone is againts it.