* Mainline commit "serial: Test for no tx data on tx restart" not correct for i.MX6?
@ 2014-07-23 4:10 Dirk Behme
2014-07-23 16:32 ` Peter Hurley
0 siblings, 1 reply; 6+ messages in thread
From: Dirk Behme @ 2014-07-23 4:10 UTC (permalink / raw)
To: linux-serial, Peter Hurley, Huang Shijie
Cc: Aaro Koskinen, Greg Kroah-Hartman, Wang, Jiada, Dirk Behme
Hi,
this is a question regarding the i.MX6 part (drivers/tty/serial/imx.c)
of the commit
serial: Test for no tx data on tx restart
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c557d392fbf5badd693ea1946a4317c87a26a716
Talking with some i.MX6 experts about this, I've got the comment
-- cut --
The imx serial driver part of this commit isn't correct
as imx.c sends x_char in irq handler, not in imx_start_tx(),
-- cut --
What do you think?
Best regards
Dirk
P.S.: I don't have the initial post in my inbox, so sorry for the new
thread.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Mainline commit "serial: Test for no tx data on tx restart" not correct for i.MX6?
2014-07-23 4:10 Mainline commit "serial: Test for no tx data on tx restart" not correct for i.MX6? Dirk Behme
@ 2014-07-23 16:32 ` Peter Hurley
2014-07-24 8:12 ` jiwang
0 siblings, 1 reply; 6+ messages in thread
From: Peter Hurley @ 2014-07-23 16:32 UTC (permalink / raw)
To: Dirk Behme, linux-serial, Huang Shijie
Cc: Aaro Koskinen, Greg Kroah-Hartman, Wang, Jiada, Dirk Behme
Hi Dirk,
On 07/23/2014 12:10 AM, Dirk Behme wrote:
> Hi,
>
> this is a question regarding the i.MX6 part (drivers/tty/serial/imx.c) of the commit
>
> serial: Test for no tx data on tx restart
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c557d392fbf5badd693ea1946a4317c87a26a716
>
> Talking with some i.MX6 experts about this, I've got the comment
>
> -- cut --
> The imx serial driver part of this commit isn't correct
> as imx.c sends x_char in irq handler, not in imx_start_tx(),
> -- cut --
>
> What do you think?
Thanks for the comment.
Yeah, I missed that the imx driver handled x_char _at all_,
because how the imx driver is handling x_char looks broken.
For example, if data is already in the tx ring buffer,
imx_start_tx() will send that data before the x_char, and if
all the tx ring buffer data is sent successfully, the tx
interrupt is switched back off, so the x_char won't be sent.
Also, the imx driver doesn't send x_char in dma mode?
Regards,
Peter Hurley
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Mainline commit "serial: Test for no tx data on tx restart" not correct for i.MX6?
2014-07-23 16:32 ` Peter Hurley
@ 2014-07-24 8:12 ` jiwang
2014-07-24 11:58 ` Peter Hurley
0 siblings, 1 reply; 6+ messages in thread
From: jiwang @ 2014-07-24 8:12 UTC (permalink / raw)
To: Peter Hurley, Dirk Behme, linux-serial, Huang Shijie
Cc: Aaro Koskinen, Greg Kroah-Hartman, Dirk Behme
Hi Peter
On 07/24/2014 01:32 AM, Peter Hurley wrote:
> Hi Dirk,
>
> On 07/23/2014 12:10 AM, Dirk Behme wrote:
>> Hi,
>>
>> this is a question regarding the i.MX6 part (drivers/tty/serial/imx.c) of the commit
>>
>> serial: Test for no tx data on tx restart
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c557d392fbf5badd693ea1946a4317c87a26a716
>>
>> Talking with some i.MX6 experts about this, I've got the comment
>>
>> -- cut --
>> The imx serial driver part of this commit isn't correct
>> as imx.c sends x_char in irq handler, not in imx_start_tx(),
>> -- cut --
>>
>> What do you think?
> Thanks for the comment.
>
> Yeah, I missed that the imx driver handled x_char _at all_,
> because how the imx driver is handling x_char looks broken.
>
> For example, if data is already in the tx ring buffer,
> imx_start_tx() will send that data before the x_char, and if
> all the tx ring buffer data is sent successfully, the tx
> interrupt is switched back off, so the x_char won't be sent.
imx_start_tx() doesn't do any actual data transfer, it only
switches tx interrupt on, and tx interrupt does the actual data
transfer, so if x_char is pending, it will always be sent before
the data in ring buffer.
> Also, the imx driver doesn't send x_char in dma mode?
Yes, I think so.
Thanks,
Jiada
> Regards,
> Peter Hurley
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Mainline commit "serial: Test for no tx data on tx restart" not correct for i.MX6?
2014-07-24 8:12 ` jiwang
@ 2014-07-24 11:58 ` Peter Hurley
2014-07-29 7:18 ` jiwang
0 siblings, 1 reply; 6+ messages in thread
From: Peter Hurley @ 2014-07-24 11:58 UTC (permalink / raw)
To: jiwang, Dirk Behme, linux-serial, Huang Shijie
Cc: Aaro Koskinen, Greg Kroah-Hartman, Dirk Behme
Hi Jiada,
On 07/24/2014 04:12 AM, jiwang wrote:
> Hi Peter
>
> On 07/24/2014 01:32 AM, Peter Hurley wrote:
>> Hi Dirk,
>>
>> On 07/23/2014 12:10 AM, Dirk Behme wrote:
>>> Hi,
>>>
>>> this is a question regarding the i.MX6 part (drivers/tty/serial/imx.c) of the commit
>>>
>>> serial: Test for no tx data on tx restart
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c557d392fbf5badd693ea1946a4317c87a26a716
>>>
>>> Talking with some i.MX6 experts about this, I've got the comment
>>>
>>> -- cut --
>>> The imx serial driver part of this commit isn't correct
>>> as imx.c sends x_char in irq handler, not in imx_start_tx(),
>>> -- cut --
>>>
>>> What do you think?
>> Thanks for the comment.
>>
>> Yeah, I missed that the imx driver handled x_char _at all_,
>> because how the imx driver is handling x_char looks broken.
>>
>> For example, if data is already in the tx ring buffer,
>> imx_start_tx() will send that data before the x_char, and if
>> all the tx ring buffer data is sent successfully, the tx
>> interrupt is switched back off, so the x_char won't be sent.
>
> imx_start_tx() doesn't do any actual data transfer
???
565 static void imx_start_tx(struct uart_port *port)
566 {
...
610
611 if (readl(sport->port.membase + uts_reg(sport)) & UTS_TXEMPTY)
612 imx_transmit_buffer(sport);
613 }
463 static inline void imx_transmit_buffer(struct imx_port *sport)
464 {
465 struct circ_buf *xmit = &sport->port.state->xmit;
466
467 while (!uart_circ_empty(xmit) &&
468 !(readl(sport->port.membase + uts_reg(sport))
469 & UTS_TXFULL)) {
470 /* send xmit->buf[xmit->tail]
471 * out the port here */
472 writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
473 xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
474 sport->port.icount.tx++;
475 }
476
477 if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
478 uart_write_wakeup(&sport->port);
479
480 if (uart_circ_empty(xmit))
481 imx_stop_tx(&sport->port);
482 }
Regards,
Peter Hurley
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Mainline commit "serial: Test for no tx data on tx restart" not correct for i.MX6?
2014-07-24 11:58 ` Peter Hurley
@ 2014-07-29 7:18 ` jiwang
2014-08-06 23:53 ` Peter Hurley
0 siblings, 1 reply; 6+ messages in thread
From: jiwang @ 2014-07-29 7:18 UTC (permalink / raw)
To: Peter Hurley, Dirk Behme, linux-serial, Huang Shijie
Cc: Aaro Koskinen, Greg Kroah-Hartman, Dirk Behme
Hi Peter
On 07/24/2014 08:58 PM, Peter Hurley wrote:
> Hi Jiada,
>
> On 07/24/2014 04:12 AM, jiwang wrote:
>> Hi Peter
>>
>> On 07/24/2014 01:32 AM, Peter Hurley wrote:
>>> Hi Dirk,
>>>
>>> On 07/23/2014 12:10 AM, Dirk Behme wrote:
>>>> Hi,
>>>>
>>>> this is a question regarding the i.MX6 part (drivers/tty/serial/imx.c) of the commit
>>>>
>>>> serial: Test for no tx data on tx restart
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c557d392fbf5badd693ea1946a4317c87a26a716
>>>>
>>>> Talking with some i.MX6 experts about this, I've got the comment
>>>>
>>>> -- cut --
>>>> The imx serial driver part of this commit isn't correct
>>>> as imx.c sends x_char in irq handler, not in imx_start_tx(),
>>>> -- cut --
>>>>
>>>> What do you think?
>>> Thanks for the comment.
>>>
>>> Yeah, I missed that the imx driver handled x_char _at all_,
>>> because how the imx driver is handling x_char looks broken.
>>>
>>> For example, if data is already in the tx ring buffer,
>>> imx_start_tx() will send that data before the x_char, and if
>>> all the tx ring buffer data is sent successfully, the tx
>>> interrupt is switched back off, so the x_char won't be sent.
>> imx_start_tx() doesn't do any actual data transfer
> ???
>
> 565 static void imx_start_tx(struct uart_port *port)
> 566 {
> ...
> 610
> 611 if (readl(sport->port.membase + uts_reg(sport)) & UTS_TXEMPTY)
> 612 imx_transmit_buffer(sport);
> 613 }
>
>
> 463 static inline void imx_transmit_buffer(struct imx_port *sport)
> 464 {
> 465 struct circ_buf *xmit = &sport->port.state->xmit;
> 466
> 467 while (!uart_circ_empty(xmit) &&
> 468 !(readl(sport->port.membase + uts_reg(sport))
> 469 & UTS_TXFULL)) {
> 470 /* send xmit->buf[xmit->tail]
> 471 * out the port here */
> 472 writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
> 473 xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> 474 sport->port.icount.tx++;
> 475 }
> 476
> 477 if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> 478 uart_write_wakeup(&sport->port);
> 479
> 480 if (uart_circ_empty(xmit))
> 481 imx_stop_tx(&sport->port);
> 482 }
sorry, our imx driver has been modified differently,
yes, you are right, mainline imx driver's handling of x_char looks broken
@Shijie,
do you have any comment?
Thanks,
Jiada
> Regards,
> Peter Hurley
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Mainline commit "serial: Test for no tx data on tx restart" not correct for i.MX6?
2014-07-29 7:18 ` jiwang
@ 2014-08-06 23:53 ` Peter Hurley
0 siblings, 0 replies; 6+ messages in thread
From: Peter Hurley @ 2014-08-06 23:53 UTC (permalink / raw)
To: jiwang, Dirk Behme
Cc: linux-serial, Huang Shijie, Dirk Behme, Greg Kroah-Hartman
On 07/29/2014 03:18 AM, jiwang wrote:
> On 07/24/2014 08:58 PM, Peter Hurley wrote:
>> On 07/24/2014 04:12 AM, jiwang wrote:
>>> On 07/24/2014 01:32 AM, Peter Hurley wrote:
>>>> On 07/23/2014 12:10 AM, Dirk Behme wrote:
>>>>> Hi,
>>>>>
>>>>> this is a question regarding the i.MX6 part (drivers/tty/serial/imx.c) of the commit
>>>>>
>>>>> serial: Test for no tx data on tx restart
>>>>>
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c557d392fbf5badd693ea1946a4317c87a26a716
>>>>>
>>>>> Talking with some i.MX6 experts about this, I've got the comment
>>>>>
>>>>> -- cut --
>>>>> The imx serial driver part of this commit isn't correct
>>>>> as imx.c sends x_char in irq handler, not in imx_start_tx(),
>>>>> -- cut --
>>>>>
>>>>> What do you think?
>>>>
>>>> Yeah, I missed that the imx driver handled x_char _at all_,
>>>> because how the imx driver is handling x_char looks broken.
[...]
> sorry, our imx driver has been modified differently,
> yes, you are right, mainline imx driver's handling of x_char looks broken
Jiada or Dirk or whomever,
Would someone please test the patch below on actual imx hardware?
You can download a standalone x_char unit test from
http://blog.hurleysoftware.com/uploads/tty_xchar.c
Instructions for building, setting up and testing are included in the file.
--- >% ---
Subject: [PATCH] serial: imx: Fix x_char handling and tx flow control
The serial core expects the UART driver to transmit x_char
(START/STOP chars) even if tx is stopped and before data already
in the tx ring buffer if possible. Also, sending x_char must
not cause additional data in the tx ring buffer to transmit
if tx is stopped.
Cause x_char to be transmitted before any other data is sent.
Auto-stop tx if the tx ring buffer is empty or tx should be stopped.
Only perform one write wakeup if tx ring buffer space is below
threshold.
x_char handling in DMA mode is still broken; add FIXME.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/serial/imx.c | 39 ++++++++++++++++-----------------------
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index be1c842..00a37e9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -460,9 +460,19 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
{
struct circ_buf *xmit = &sport->port.state->xmit;
+ if (sport->port.x_char) {
+ /* Send next char */
+ writel(sport->port.x_char, sport->port.membase + URTX0);
+ return;
+ }
+
+ if (uart_circ_empty(xmit) || uart_tx_stopped(&sport->port)) {
+ imx_stop_tx(&sport->port);
+ return;
+ }
+
while (!uart_circ_empty(xmit) &&
- !(readl(sport->port.membase + uts_reg(sport))
- & UTS_TXFULL)) {
+ !(readl(sport->port.membase + uts_reg(sport)) & UTS_TXFULL)) {
/* send xmit->buf[xmit->tail]
* out the port here */
writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
@@ -563,9 +573,6 @@ static void imx_start_tx(struct uart_port *port)
struct imx_port *sport = (struct imx_port *)port;
unsigned long temp;
- if (uart_circ_empty(&port->state->xmit))
- return;
-
if (USE_IRDA(sport)) {
/* half duplex in IrDA mode; have to disable receive mode */
temp = readl(sport->port.membase + UCR4);
@@ -600,7 +607,10 @@ static void imx_start_tx(struct uart_port *port)
}
if (sport->dma_is_enabled) {
- imx_dma_tx(sport);
+ /* FIXME: port->x_char must be transmitted if != 0 */
+ if (!uart_circ_empty(&port->state->xmit) &&
+ !uart_tx_stopped(port))
+ imx_dma_tx(sport);
return;
}
@@ -628,27 +638,10 @@ static irqreturn_t imx_rtsint(int irq, void *dev_id)
static irqreturn_t imx_txint(int irq, void *dev_id)
{
struct imx_port *sport = dev_id;
- struct circ_buf *xmit = &sport->port.state->xmit;
unsigned long flags;
spin_lock_irqsave(&sport->port.lock, flags);
- if (sport->port.x_char) {
- /* Send next char */
- writel(sport->port.x_char, sport->port.membase + URTX0);
- goto out;
- }
-
- if (uart_circ_empty(xmit) || uart_tx_stopped(&sport->port)) {
- imx_stop_tx(&sport->port);
- goto out;
- }
-
imx_transmit_buffer(sport);
-
- if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
- uart_write_wakeup(&sport->port);
-
-out:
spin_unlock_irqrestore(&sport->port.lock, flags);
return IRQ_HANDLED;
}
--
2.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-06 23:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-23 4:10 Mainline commit "serial: Test for no tx data on tx restart" not correct for i.MX6? Dirk Behme
2014-07-23 16:32 ` Peter Hurley
2014-07-24 8:12 ` jiwang
2014-07-24 11:58 ` Peter Hurley
2014-07-29 7:18 ` jiwang
2014-08-06 23:53 ` Peter Hurley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.