On Fri, Dec 2, 2016 at 10:56 AM, Richard Genoud
<richard.genoud@xxxxxxxxx> wrote:
> On 01/12/2016 16:10, Greg KH wrote:
>> On Thu, Dec 01, 2016 at 03:20:35PM +0100, Gil Weber wrote:
>>> Hello,
> Hi,
>>> I am unable to communicate in RS485 since I upgrade to the latest kernel
>>> version (v4.9-rc7). Previously I was in 3.12.
>>>
>>> It seems to be caused by commit "0058f0871efe7b01c6f2b3046c68196ab73e96da
>>> tty/serial: atmel: fix RS485 half duplex with DMA".
>>> RX is disabled during TX, but in my case it is not re-enabled after.
>>> If I revert this patch, everything seems to work again.
>>
>> Any reason you didn't cc: the author of that patch? :)
Sorry for that, I will do it next time :-)
>>
>>> Currently enable RX is done in atmel_tx_dma but it is not called if there are
>>> still data to transfer... problem in that it is never done later.
>>> So I think we should also enable RX in atmel_complete_tx_dma.
>>>
>>> I have made this fix, and it seems to work now:
>>>
>>>
>>> diff --git a/drivers/tty/serial/atmel_serial.c
>>> b/drivers/tty/serial/atmel_serial.c
>>> index 74e97dc..4190012 100644
>>> --- a/drivers/tty/serial/atmel_serial.c
>>> +++ b/drivers/tty/serial/atmel_serial.c
>>> @@ -806,6 +806,10 @@ static void atmel_complete_tx_dma(void *arg)
>>> */
>>> if (!uart_circ_empty(xmit))
>>> atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
>>> + else if (port->rs485.flags & SER_RS485_ENABLED) {
> Don't you need to test the SER_RS485_RX_DURING_TX flag as well ?
> Hum. I see. It is not present in atmel_tx_dma().
> Maybe we can move the test from atmel_tx_dma() to here.
>>> + /* DMA done, start RX for RS485 */
>>> + atmel_start_rx(port);
>>> + }
>>>
>>> spin_unlock_irqrestore(&port->lock, flags);
>>>
>>>
>>>
>>>
>>> Is it correct? or maybe there is a better way to fix it?
>>> Thanks,
>>> Gil
>>
>> Alexandre, any ideas?
>>
>
> Gil, could you try something like:
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 168b10cad47b..11c0117af80b 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -798,6 +798,11 @@ static void atmel_complete_tx_dma(void *arg)
> */
> if (!uart_circ_empty(xmit))
> atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
> + else if ((port->rs485.flags & SER_RS485_ENABLED) &&
> + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> + /* DMA done, stop TX, start RX for RS485 */
> + atmel_start_rx(port);
> + }
>
> spin_unlock_irqrestore(&port->lock, flags);
> }
> @@ -900,12 +905,6 @@ static void atmel_tx_dma(struct uart_port *port)
> desc->callback = atmel_complete_tx_dma;
> desc->callback_param = atmel_port;
> atmel_port->cookie_tx = dmaengine_submit(desc);
> -
> - } else {
> - if (port->rs485.flags & SER_RS485_ENABLED) {
> - /* DMA done, stop TX, start RX for RS485 */
> - atmel_start_rx(port);
> - }
> }
>
> if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
Just tested and it is working on my target.
So yes it is probably better to move it to avoid redundant code.
Test of SER_RS485_RX_DURING_TX is also working.
>
>
> Looking at atmel_{start,stop}_{rx,tx} functions, I wonder if it makes sense to have:
>
> if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
> if ((port->rs485.flags & SER_RS485_ENABLED) &&
> !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> atmel_stop_rx(port);
> in atmel_start_tx()
> and just:
> if ((port->rs485.flags & SER_RS485_ENABLED) &&
> !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> atmel_start_rx(port);
> in atmel_stop_tx() ?
>
> The condition on (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port)) doesn't seems to be necessary.
> If there's no DMA nor PDC, atmel_stop_rx(port) should still be called, shouldn't it ?
I tend to agree with you
>
> Richard.
>
Thanks,
Gil
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
