Re: [PATCH] HW Flocontrol fix for SC16IS7XX UART driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Maarten,

On 03/14/2016 09:43 AM, Maarten Brock wrote:
> ----- Original Message -----
> From: Peter Hurley [mailto:peter@xxxxxxxxxxxxxxxxxx]
> To: Kim Bøndergaard Poulsen [mailto:Kim.BondergaardPoulsen@xxxxxxxxx], linux-serial@xxxxxxxxxxxxxxx [mailto:linux-serial@xxxxxxxxxxxxxxx], joshc@xxxxxx [mailto:joshc@xxxxxx], kubakici@xxxxx [mailto:kubakici@xxxxx]
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> Sent: Mon, 14 Mar 2016 16:25:37 +0100
> Subject: Re: [PATCH] HW Flocontrol fix for SC16IS7XX UART driver

Please fix your mail client or use a different one.
None of your replies are threaded.


>> On 03/14/2016 02:07 AM, Kim Bøndergaard Poulsen wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Peter Hurley [mailto:peter@xxxxxxxxxxxxxxxxxx]
>>>> Sent: 11. marts 2016 02:44
>>>> To: Kim Bøndergaard Poulsen; linux-serial@xxxxxxxxxxxxxxx; joshc@xxxxxx;
>>>> kubakici@xxxxx
>>>> Cc: gregkh@xxxxxxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH] HW Flocontrol fix for SC16IS7XX UART driver
>>>>
>>>> Hi Kim,
>>>>
>>>> On 03/08/2016 04:30 AM, Kim Bøndergaard Poulsen wrote:
>>>>> Following patch fixes HW flowcontrol issues with UARTs based in
>>>>> SC16IS7XX
>>>>>
>>>>> I've only tested the patch with an SC16IS750.
>>>>>
>>>>> commit 9390e7e2952cac5e0119a656a0d8a4e8efe33d07
>>>>> Author: Kim Bøndergaard <kibo@xxxxxxxxx>
>>>>> Date:   Tue Mar 8 12:31:21 2016 +0100
>>>>>
>>>>>     sc16is7xx: Fix HW flow control
>>>>>
>>>>>     Following problems fixed:
>>>>>
>>>>>     In sc16is7xx_set_baud() what ever was already set in
>>>>>     EFR.CTS and EFT.RTS was cleared.
>>>>>
>>>>>     In sc16is7xx_set_termios() enhanced functions where disabled
>>>>>
>>>>>     In sc16is6xx_set_termios() upper layer is informed that this driver
>>>>>     support automatic HW flowcontrol. Without this knowledge the two
>>>>>     layers eventually ended up in different states, causing
>>>>>     communication to stop
>>>>
>>>> Missing Signed-off-by:
>>>
>>> I'll add it
>>>
>>>>
>>>>>
>>>>> diff --git a/drivers/tty/serial/sc16is7xx.c
>>>>> b/drivers/tty/serial/sc16is7xx.c index 13f8d5f..5a51024 100644
>>>>> --- a/drivers/tty/serial/sc16is7xx.c
>>>>> +++ b/drivers/tty/serial/sc16is7xx.c
>>>>> @@ -511,8 +511,9 @@ static int sc16is7xx_set_baud(struct uart_port
>>>>> *port, int baud)
>>>>>
>>>>>         /* Enable enhanced features */
>>>>>         regcache_cache_bypass(s->regmap, true);
>>>>> -       sc16is7xx_port_write(port, SC16IS7XX_EFR_REG,
>>>>> -                            SC16IS7XX_EFR_ENABLE_BIT);
>>>>> +       sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
>>>>> +                       SC16IS7XX_EFR_ENABLE_BIT,
>>>>> +                       SC16IS7XX_EFR_ENABLE_BIT);
>>>>>         regcache_cache_bypass(s->regmap, false);
>>>>>
>>>>>         /* Put LCR back to the normal mode */ @@ -901,14 +902,19 @@
>>>>> static void sc16is7xx_set_termios(struct uart_port *port,
>>>>>         regcache_cache_bypass(s->regmap, true);
>>>>>         sc16is7xx_port_write(port, SC16IS7XX_XON1_REG, termios-
>>>>> c_cc[VSTART]);
>>>>>         sc16is7xx_port_write(port, SC16IS7XX_XOFF1_REG, termios-
>>>>> c_cc[VSTOP]);
>>>>> -       if (termios->c_cflag & CRTSCTS)
>>>>> +       if (termios->c_cflag & CRTSCTS) {
>>>>>                 flow |= SC16IS7XX_EFR_AUTOCTS_BIT |
>>>>>                         SC16IS7XX_EFR_AUTORTS_BIT;
>>>>> +               port->status |= UPSTAT_AUTOCTS;
>>>>
>>>> This is correct.
>>>>
>>>>
>>>> However a quick read of the SC16IS740 datasheet suggests that RTS
>>>> handling is broken when autoRTS is enabled.
>>>>
>>>
>>>
>>> Where do you see that?
>>
>> http://www.nxp.com/documents/data_sheet/SC16IS740_750_760.pdf
>>
>> page 27/63, 8.6 Modem Control Register,
>> "If Auto RTS is enabled, the RTS output is controlled by hardware flow
>> control"
>>
>> It's possible I'm misreading that, in that the statement only applies if
>> MCR[1] == 1.
>>
>>> Like written I've only tested at SC16IS750 but the datasheet I've used
>> cover the whole range 740/750/760
>>> I think I have to get that information to understand your next statements.
>> Can't really see if it relates to my fix.
>>
>> If MCR[1] == 0 disables RTS even when autoRTS is enabled, then please ignore
>> my comments regarding TIOCMSET ioctl and set_mctrl() method below.
>>
>>
>>
>>>> AFAICT, RTS state in MCR is _ignored_ when autoRTS is enabled.
>>>>
>>>> This means that if userspace has specifically disabled RTS with TIOCMSET
>>>> ioctl and CRTSCTS is enabled, input will not be disabled.
>>>>
>>>> To make this work properly requires disabling/restoring autoRTS in the
>>>> set_mctrl() method when TIOCM_RTS is changed.
>>>>
>>>>
>>>>> +       }
> 
> If the device uses autoRTS and then userspace tries to force RTS one should
> expect this not to work. Userspace should first disable autoRTS.

How? By dropping CRTSCTS in termios?

No, that's exactly what I don't want; ioctl(TIOCMSET) should just work.
>From the POV of userspace with CRTSCTS enabled, disabling RTS via ioctl(TIOCMSET)
is halting input and reenabling RTS via ioctl(TIOCMSET) is resuming input.


> Alternatively the driver could automagically disable autoRTS. But then it
> should never reenable autoRTS as that AFAICT would override the forced setting
> again.

The driver would restore (rather than re-enable) the autoRTS state when RTS
is raised.

The 8250 driver does this with OMAP, and both OMAP and SC16IS7xx are based
on the 16C750.


> I see nothing broken though.

As long as sc16is7xx_set_mctrl(~TIOCM_RTS) disables RTS regardless of autoRTS
state.

Regards,
Peter Hurley

--
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



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux