On 08/07/2014 07:03 PM, Peter Hurley wrote:
----Cut-------------
1. The patch enables UPF_HARD_FLOW, but provides no throttle() and
unthrottle()
methods for 8250, which is guaranteed to blow-up when either
uart_throttle() or
uart_unthrottle() is called.
2. The patch adds capabilities which already exist, namely
UART_CAP_AFE.
AFAIK, UART_CAP_AFE is a software assisted hw flow control and it
was described in my commit log as well where as this patch add
support for pure h/w controlled flow control and no software
intervention is needed. Do you think uart_throttle() or
uart_unthrottle() is applicable
in this case?
UART_CAP_AFE is used to indicate 16750-compatible hw flow control,
which is
auto-CTS and auto-RTS flow control as described in the TI datasheet at
http://www.ti.com/lit/ds/symlink/tl16c750.pdf
Peter,
This patch was added to support hw flow control on boards based on
Keystone SoCs that has UART with h/w flow control capability.
Keystone SoCs UART spec is at
http://www.ti.com/lit/ug/sprugp1/sprugp1.pdf
and it uses tl16550c as per the document. The equivalent spec seems
to be http://www.ti.com/lit/ds/symlink/tl16c550c.pdf. Only difference
between tl16c750 and tl16c550 seems to be 16 byte FIFO available
instead of 16 or 64 byte FIFO in 16c750.
So this is just a way to avoid the fifo size check.
Then I'd rather see a capability that only overrides the fifo size check
and does not enable UPF_HARD_FLOW.
Or show that the fifo size check is not actually required for AFE.
And about your original question
Why is a modem status interrupt being generated for DCTS
if autoflow control is enabled?
Are you asking why this patch didn't disable generating CTS interrupt
when h/w flow control is enable?
No. I was asking what chip had AFE on but still generated modem status
interrupts for delta CTS.
But I realize now that a different question needs asking:
Is the MSR read showing delta CTS set when AFE is on, ever?
Unfortunately this was tested on a customer board that I don't have
access to and can't check this out right away. I am trying to findout if
I can get some hardware to test the patch to address the issue being
discussed. Customer board is currently using RTS and CTS lines and the
same works fine for them with this patch.
Because serial8250_modem_status() assumes the answer is no for
_all_ AFE-capable devices, and if yes, would mean that
serial8250_modem_status()
is broken if AFE is on.
As per Keystone UART spec
bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates that the
CTS input has changed state since the last time it was read by the CPU.
When DCTS is set (autoflow control is not enabled and the modem status
interrupt is enabled), a modem status interrupt is generated. When
autoflow control is enabled, no interrupt is generated
So based on this, there shouldn't be any CTS change if AFE is enabled
and will indicate change if AFE is disabled. Probably add WARN_ON_ONCE()
as you suggested to detect any offending h/w.
If so,
unsigned int serial8250_modem_status(struct uart_8250_port *up)
{
...
if (status& UART_MSR_DCTS)
uart_handle_cts_change(port, status& UART_MSR_CTS);
}
Probably in the above check if UPF_HARD_FLOW is set, we can avoid
calling
>> uart_handle_cts_change() and undo the current code change in
uart_handle_cts_change()
>> and just add a WARN_ON_ONCE() as you have suggested.
Let's come back to this question after determining the answer for the
above question.
uart_throttle() and uart_unthrottle() are used indirectly by line
disciplines
for high-level rx flow control, such as when a read buffer fills up
because
there is no userspace reader. The 8250 core doesn't define a
throttle/unthrottle
method because writing MCR to drop RTS is sufficient to disable
auto-RTS.
As per spec. hardware has rx threshold levels set to trigger an RTS
level change to tell
>> the remote from sending more bytes. So if h/w flow control is
enabled, then not sure why
>> uart_throttle() is to be doing anything when h/w flow control is
supported? A dummy
>> function required to satisfy the line discipline?
I understand how auto-RTS works.
Let's pretend for a moment that uart_throttle() does nothing when
auto-RTS is enabled:
1. tty buffers start to fill up because no process is reading the data.
2. The throttle threshold is reached.
3. uart_throttle() is called but does nothing.
4. more data arrives and the DR interrupt is triggered
5. serial8250_rx_chars() reads in the new data.
6. tty buffers keep filling up even though the driver was told to
throttle
7. eventually the tty buffers will cap at about 64KB and start counting
buf_overrun errors
Ok.
Couple of observation on the AFE implementation in 8250 driver prior to
my patch.
From the discussion so far, AFE is actually hardware assisted hardware
flow control. Auto CTS is sw assisted hardware flow control
where sw uses RTS line for recieve side flow control and I assume it
uses MCR RTS bit for this where AFE does this automatically. From
the 16550 or Keystone UART spec, I can't find how RTS line can be
asserted to do this through sw instead of hardware doing it
automatically. Spec says
MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
e autoflow control enabled. Note that all UARTs do not support this
feature. See the device-specific data manual for supported features. If
this feature is not available, this bit is reserved and should be
cleared to 0.
0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
1 = UARTn_RTS and UARTn_CTS are enabled.
Then since AFE was already supported before my patch for FIFO size
32bytes or higher, I am wondering why there was no implementation of
throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag is not
set at all if AFE implemented in 8250 driver is hw assisted, hw flow
control. Also what do these API supposed to do?
in serial_core.c, uart_throttle() function
if CRTSCTS is set
port->ops->throttle(port);
Also call at the end
uart_clear_mctrl(port, TIOCM_RTS);
and this go an clear the MCR RTS bit.
So what does uart_throttle() API expected to do since MCR is updated
using uart_clear_mctrl().
I searched who sets the UPF_HARD_FLOW in port->flags and only driver
that does set this flag is omap-serial.c. The check was introduced by
commit from Ruessel King to support h/w assisted h/w flow control.
======================================================================
commit dba05832cbe4f305dfd998fb26d7c685d91fbbd8
Author: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
Date: Tue Apr 17 16:41:10 2012 +0100
SERIAL: core: add hardware assisted h/w flow control support
Ports which are handling h/w flow control in hardware must not have
their RTS state altered depending on the tty's hardware-stopped state.
Avoid this additional logic when setting the termios state.
Acked-by: Alan Cox <alan@xxxxxxxxxxxxxxx>
Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
======================================================================
This flag is checked in uart_set_termios() and the reason is in the
commit log above.
So shouldn't AFE support in 8250 make use of this flag? My patch uses DT
attribute to set this flag so above uart_set_termios() function behave
as expected. But as you have rightly pointed out throttle()/unthrottle()
is missing that needs to be added. Only driver that I can find that has
implemented this is omap_serial.c. It does disable RDI and RLSI as part
of throttle() interrupt and re-enable it as part of unthrottle().
Probably I can add this in this driver as well if this is what is
expected. I will post a patch for this anyways, with some basic testing,
but testing on customer h/w for this was initialially implemented has to
wait until my return from vacation (on vacation from 08/11-08/16).
Also 8250.c support other port types that doesn't have AFE. So shoudl
this
>> be driving RTS line to stop the sender and resume when
uart_unthrottle() is called?
Yes, because that's how sw-assisted CTS flow control works, and the
default behavior of uart_throttle/uart_unthrottle.
IOW, with no extra code or special-casing, AFE just works.
Based on my above discussion, there are few things required to be done
on top of AFE and some of it is done by my patch and the remaining thing
to be addressed in another patch.
I want to work to fix this rather than revert this change as our
customer is already using this feature.
3.16 was released 4 days ago.
As I said, I will work to address this with priority.
