[PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Phil Elwell]

The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
the bit means that the FIFO is empty (although there may still be a
byte in the transmit register), but on 2835 it indicates that the FIFO
is not full. This causes interrupts after every byte is transmitted,
with the FIFO providing some interrupt latency tolerance.

A consequence of this difference is that the usual strategy of writing
multiple bytes into the TX FIFO after checking THRE once is unsafe.
In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
but the first since by then the FIFO is full.

There is an HFIFO ("Hidden FIFO") capability that causes the transmit
loop to terminate when both THRE and TEMT are set, i.e. when the TX
block is completely idle. This is unnecessarily cautious, potentially
causing gaps in transmission.

Add a new conditional to the transmit loop, predicated on CAP_MINI,
that exits when THRE is no longer set (the FIFO is full). This allows
the FIFO to fill quickly but subsequent writes are paced by the
transmission rate.

Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
---
 drivers/tty/serial/8250/8250_port.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4c620be..a5fe0e6 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1764,6 +1764,10 @@ void serial8250_tx_chars(struct uart_8250_port *up)
 		if ((up->capabilities & UART_CAP_HFIFO) &&
 		    (serial_in(up, UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
 			break;
+		/* The BCM2835 MINI UART THRE bit is really a not-full bit. */
+		if ((up->capabilities & UART_CAP_MINI) &&
+		    !(serial_in(up, UART_LSR) & UART_LSR_THRE))
+			break;
 	} while (--count > 0);
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-- 
1.9.1

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Stefan Wahren]

Hi Phil,

Am 26.06.2017 um 15:59 schrieb Phil Elwell:
> The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
> the bit means that the FIFO is empty (although there may still be a
> byte in the transmit register), but on 2835 it indicates that the FIFO
> is not full. This causes interrupts after every byte is transmitted,
> with the FIFO providing some interrupt latency tolerance.
>
> A consequence of this difference is that the usual strategy of writing
> multiple bytes into the TX FIFO after checking THRE once is unsafe.
> In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
> but the first since by then the FIFO is full.
>
> There is an HFIFO ("Hidden FIFO") capability that causes the transmit
> loop to terminate when both THRE and TEMT are set, i.e. when the TX
> block is completely idle. This is unnecessarily cautious, potentially
> causing gaps in transmission.
>
> Add a new conditional to the transmit loop, predicated on CAP_MINI,
> that exits when THRE is no longer set (the FIFO is full). This allows
> the FIFO to fill quickly but subsequent writes are paced by the
> transmission rate.

thanks for this good explanation.

But this looks like a bug / quirk than a capability?

It would be better to name this define more self-explaining.

Btw can't see the definition of UART_CAP_MINI?

Thanks
Stefan

>
> Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 4c620be..a5fe0e6 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1764,6 +1764,10 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>  		if ((up->capabilities & UART_CAP_HFIFO) &&
>  		    (serial_in(up, UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
>  			break;
> +		/* The BCM2835 MINI UART THRE bit is really a not-full bit. */
> +		if ((up->capabilities & UART_CAP_MINI) &&
> +		    !(serial_in(up, UART_LSR) & UART_LSR_THRE))
> +			break;
>  	} while (--count > 0);
>  
>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Phil Elwell]

Hi Stefan,

On 26/06/2017 16:05, Stefan Wahren wrote:
> Hi Phil,
> 
> Am 26.06.2017 um 15:59 schrieb Phil Elwell:
>> The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
>> the bit means that the FIFO is empty (although there may still be a
>> byte in the transmit register), but on 2835 it indicates that the FIFO
>> is not full. This causes interrupts after every byte is transmitted,
>> with the FIFO providing some interrupt latency tolerance.
>>
>> A consequence of this difference is that the usual strategy of writing
>> multiple bytes into the TX FIFO after checking THRE once is unsafe.
>> In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
>> but the first since by then the FIFO is full.
>>
>> There is an HFIFO ("Hidden FIFO") capability that causes the transmit
>> loop to terminate when both THRE and TEMT are set, i.e. when the TX
>> block is completely idle. This is unnecessarily cautious, potentially
>> causing gaps in transmission.
>>
>> Add a new conditional to the transmit loop, predicated on CAP_MINI,
>> that exits when THRE is no longer set (the FIFO is full). This allows
>> the FIFO to fill quickly but subsequent writes are paced by the
>> transmission rate.
> 
> thanks for this good explanation.
> 
> But this looks like a bug / quirk than a capability?
> 
> It would be better to name this define more self-explaining.
> 
> Btw can't see the definition of UART_CAP_MINI?

The capability was added in d087e7a991f1f61ee2c07db1be7c5cc2aa373f5d, which
is in linux-next. Given that the "capability" already exists and the quirk
is likely to be unique to BCM2835 MINI UART, I don't think we should create
a new quirk.

Besides, the "HFIFO" capability looks a lot like quirk to me.

Thanks,

Phil

>>
>> Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
>> ---
>>  drivers/tty/serial/8250/8250_port.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 4c620be..a5fe0e6 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -1764,6 +1764,10 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>>  		if ((up->capabilities & UART_CAP_HFIFO) &&
>>  		    (serial_in(up, UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
>>  			break;
>> +		/* The BCM2835 MINI UART THRE bit is really a not-full bit. */
>> +		if ((up->capabilities & UART_CAP_MINI) &&
>> +		    !(serial_in(up, UART_LSR) & UART_LSR_THRE))
>> +			break;
>>  	} while (--count > 0);
>>  
>>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Andy Shevchenko]

On Mon, 2017-06-26 at 16:15 +0100, Phil Elwell wrote:

> > But this looks like a bug / quirk than a capability?
> > 
> > It would be better to name this define more self-explaining.
> > 
> > Btw can't see the definition of UART_CAP_MINI?
> 
> The capability was added in d087e7a991f1f61ee2c07db1be7c5cc2aa373f5d,
> which
> is in linux-next. Given that the "capability" already exists and the
> quirk
> is likely to be unique to BCM2835 MINI UART, I don't think we should
> create
> a new quirk.
> 
> Besides, the "HFIFO" capability looks a lot like quirk to me.

To me either, which raises a question "Should it be fixed accordingly?"

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Andy Shevchenko]

On Mon, 2017-06-26 at 14:59 +0100, Phil Elwell wrote:
> The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
> the bit means that the FIFO is empty (although there may still be a
> byte in the transmit register), but on 2835 it indicates that the FIFO
> is not full. This causes interrupts after every byte is transmitted,
> with the FIFO providing some interrupt latency tolerance.
> 
> A consequence of this difference is that the usual strategy of writing
> multiple bytes into the TX FIFO after checking THRE once is unsafe.
> In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
> but the first since by then the FIFO is full.
> 
> There is an HFIFO ("Hidden FIFO") capability that causes the transmit
> loop to terminate when both THRE and TEMT are set, i.e. when the TX
> block is completely idle. This is unnecessarily cautious, potentially
> causing gaps in transmission.
> 
> Add a new conditional to the transmit loop, predicated on CAP_MINI,
> that exits when THRE is no longer set (the FIFO is full). This allows
> the FIFO to fill quickly but subsequent writes are paced by the
> transmission rate.
> 

>  		if ((up->capabilities & UART_CAP_HFIFO) &&
>  		    (serial_in(up, UART_LSR) & BOTH_EMPTY) !=
> BOTH_EMPTY)
>  			break;
> +		/* The BCM2835 MINI UART THRE bit is really a not-
> full bit. */
> +		if ((up->capabilities & UART_CAP_MINI) &&
> +		    !(serial_in(up, UART_LSR) & UART_LSR_THRE))
> +			break;

Might this end up with two sequential read of LSR?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Phil Elwell]

On 27/06/2017 10:18, Andy Shevchenko wrote:
> On Mon, 2017-06-26 at 14:59 +0100, Phil Elwell wrote:
>> The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
>> the bit means that the FIFO is empty (although there may still be a
>> byte in the transmit register), but on 2835 it indicates that the FIFO
>> is not full. This causes interrupts after every byte is transmitted,
>> with the FIFO providing some interrupt latency tolerance.
>>
>> A consequence of this difference is that the usual strategy of writing
>> multiple bytes into the TX FIFO after checking THRE once is unsafe.
>> In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
>> but the first since by then the FIFO is full.
>>
>> There is an HFIFO ("Hidden FIFO") capability that causes the transmit
>> loop to terminate when both THRE and TEMT are set, i.e. when the TX
>> block is completely idle. This is unnecessarily cautious, potentially
>> causing gaps in transmission.
>>
>> Add a new conditional to the transmit loop, predicated on CAP_MINI,
>> that exits when THRE is no longer set (the FIFO is full). This allows
>> the FIFO to fill quickly but subsequent writes are paced by the
>> transmission rate.
>>
> 
>>  		if ((up->capabilities & UART_CAP_HFIFO) &&
>>  		    (serial_in(up, UART_LSR) & BOTH_EMPTY) !=
>> BOTH_EMPTY)
>>  			break;
>> +		/* The BCM2835 MINI UART THRE bit is really a not-
>> full bit. */
>> +		if ((up->capabilities & UART_CAP_MINI) &&
>> +		    !(serial_in(up, UART_LSR) & UART_LSR_THRE))
>> +			break;
> 
> Might this end up with two sequential read of LSR?

The same thought crossed my mind, but CAP_MINI is essentially CAP_BCM2835,
and we won't be setting CAP_HFIFO on BCM2835.

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Phil Elwell]

On 27/06/2017 10:15, Andy Shevchenko wrote:
> On Mon, 2017-06-26 at 16:15 +0100, Phil Elwell wrote:
> 
>>> But this looks like a bug / quirk than a capability?
>>>
>>> It would be better to name this define more self-explaining.
>>>
>>> Btw can't see the definition of UART_CAP_MINI?
>>
>> The capability was added in d087e7a991f1f61ee2c07db1be7c5cc2aa373f5d,
>> which
>> is in linux-next. Given that the "capability" already exists and the
>> quirk
>> is likely to be unique to BCM2835 MINI UART, I don't think we should
>> create
>> a new quirk.
>>
>> Besides, the "HFIFO" capability looks a lot like quirk to me.
> 
> To me either, which raises a question "Should it be fixed accordingly?"

If I was going to make these quirks, are we simply talking about renaming the
capability or is there another mechanism? I've found the 8250_pci quirks, and
they look quite different.

I'm also happy to make this code conditional on CONFIG_SERIAL_8250_BCM2835AUX
if that is more acceptable.

Phil (still hoping to lose newb status one day)

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Andy Shevchenko]

On Tue, 2017-06-27 at 11:30 +0100, Phil Elwell wrote:
> On 27/06/2017 10:15, Andy Shevchenko wrote:
> > On Mon, 2017-06-26 at 16:15 +0100, Phil Elwell wrote:
> > 
> > > > But this looks like a bug / quirk than a capability?
> > > > 
> > > > It would be better to name this define more self-explaining.
> > > > 
> > > > Btw can't see the definition of UART_CAP_MINI?
> > > 
> > > The capability was added in
> > > d087e7a991f1f61ee2c07db1be7c5cc2aa373f5d,
> > > which
> > > is in linux-next. Given that the "capability" already exists and
> > > the
> > > quirk
> > > is likely to be unique to BCM2835 MINI UART, I don't think we
> > > should
> > > create
> > > a new quirk.
> > > 
> > > Besides, the "HFIFO" capability looks a lot like quirk to me.
> > 
> > To me either, which raises a question "Should it be fixed
> > accordingly?"
> 
> If I was going to make these quirks, are we simply talking about
> renaming the
> capability or is there another mechanism? I've found the 8250_pci
> quirks, and
> they look quite different.

Okay, we have several types of flags in the code
1. Capabilities: UART_CAP: looks like it defines features of hardware
solely for 8250 compatible devices.
2. Flags as quirks UPF_<something, not all of them> (I have a patch to
convert them to quirks, need by the way to update and resend): they are
for any serial devices.
3. Flags as capabilities: UPF_<the rest>, similar function as UART_CAP,
but for any serial device.

> 
> I'm also happy to make this code conditional on
> CONFIG_SERIAL_8250_BCM2835AUX
> if that is more acceptable.

No, it is undesired.

Can you describe which one from the above suits the best for your case?

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Phil Elwell]

On 27/06/2017 18:52, Andy Shevchenko wrote:
> On Tue, 2017-06-27 at 11:30 +0100, Phil Elwell wrote:
>> On 27/06/2017 10:15, Andy Shevchenko wrote:
>>> On Mon, 2017-06-26 at 16:15 +0100, Phil Elwell wrote:
>>>
>>>>> But this looks like a bug / quirk than a capability?
>>>>>
>>>>> It would be better to name this define more self-explaining.
>>>>>
>>>>> Btw can't see the definition of UART_CAP_MINI?
>>>>
>>>> The capability was added in
>>>> d087e7a991f1f61ee2c07db1be7c5cc2aa373f5d,
>>>> which
>>>> is in linux-next. Given that the "capability" already exists and
>>>> the
>>>> quirk
>>>> is likely to be unique to BCM2835 MINI UART, I don't think we
>>>> should
>>>> create
>>>> a new quirk.
>>>>
>>>> Besides, the "HFIFO" capability looks a lot like quirk to me.
>>>
>>> To me either, which raises a question "Should it be fixed
>>> accordingly?"
>>
>> If I was going to make these quirks, are we simply talking about
>> renaming the
>> capability or is there another mechanism? I've found the 8250_pci
>> quirks, and
>> they look quite different.
>
> Okay, we have several types of flags in the code
> 1. Capabilities: UART_CAP: looks like it defines features of hardware
> solely for 8250 compatible devices.
> 2. Flags as quirks UPF_<something, not all of them> (I have a patch to
> convert them to quirks, need by the way to update and resend): they are
> for any serial devices.
> 3. Flags as capabilities: UPF_<the rest>, similar function as UART_CAP,
> but for any serial device.
>
>>
>> I'm also happy to make this code conditional on
>> CONFIG_SERIAL_8250_BCM2835AUX
>> if that is more acceptable.
>
> No, it is undesired.
>
> Can you describe which one from the above suits the best for your case?

This bug I am trying to work around is found in the 8250 implementation of
one family of CPUs, so I would say capabilities are the best fit because
they are specific to 8250 drivers.

Phil

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1364 bytes --]

Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> writes:

> The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
> the bit means that the FIFO is empty (although there may still be a
> byte in the transmit register), but on 2835 it indicates that the FIFO
> is not full. This causes interrupts after every byte is transmitted,
> with the FIFO providing some interrupt latency tolerance.
>
> A consequence of this difference is that the usual strategy of writing
> multiple bytes into the TX FIFO after checking THRE once is unsafe.
> In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
> but the first since by then the FIFO is full.
>
> There is an HFIFO ("Hidden FIFO") capability that causes the transmit
> loop to terminate when both THRE and TEMT are set, i.e. when the TX
> block is completely idle. This is unnecessarily cautious, potentially
> causing gaps in transmission.
>
> Add a new conditional to the transmit loop, predicated on CAP_MINI,
> that exits when THRE is no longer set (the FIFO is full). This allows
> the FIFO to fill quickly but subsequent writes are paced by the
> transmission rate.
>
> Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>

Confirmed the hw behavior, so as far as the platform goes,

Acked-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Andy Shevchenko]

On Tue, 2017-06-27 at 22:00 +0100, Phil Elwell wrote:
> On 27/06/2017 18:52, Andy Shevchenko wrote:
> > On Tue, 2017-06-27 at 11:30 +0100, Phil Elwell wrote:
> > > On 27/06/2017 10:15, Andy Shevchenko wrote:
> > > > On Mon, 2017-06-26 at 16:15 +0100, Phil Elwell wrote:

> > > > > Besides, the "HFIFO" capability looks a lot like quirk to me.
> > > > 
> > > > To me either, which raises a question "Should it be fixed
> > > > accordingly?"
> > > 
> > > If I was going to make these quirks, are we simply talking about
> > > renaming the
> > > capability or is there another mechanism? I've found the 8250_pci
> > > quirks, and
> > > they look quite different.
> > 
> > Okay, we have several types of flags in the code
> > 1. Capabilities: UART_CAP: looks like it defines features of
> > hardware
> > solely for 8250 compatible devices.
> > 2. Flags as quirks UPF_<something, not all of them> (I have a patch
> > to
> > convert them to quirks, need by the way to update and resend): they
> > are
> > for any serial devices.
> > 3. Flags as capabilities: UPF_<the rest>, similar function as
> > UART_CAP,
> > but for any serial device.
> > 
> > > 
> > > I'm also happy to make this code conditional on
> > > CONFIG_SERIAL_8250_BCM2835AUX
> > > if that is more acceptable.
> > 
> > No, it is undesired.
> > 
> > Can you describe which one from the above suits the best for your
> > case?
> 
> This bug I am trying to work around is found in the 8250
> implementation of
> one family of CPUs, so I would say capabilities are the best fit
> because
> they are specific to 8250 drivers.

Yeah, looks like UART_CAP suits the best for now.

One more comment that UART_CAP_* gather capabilities and quirks at the
same time.



-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Phil Elwell]

On 28/06/2017 08:48, Andy Shevchenko wrote:
> On Tue, 2017-06-27 at 22:00 +0100, Phil Elwell wrote:
>> On 27/06/2017 18:52, Andy Shevchenko wrote:
>>> On Tue, 2017-06-27 at 11:30 +0100, Phil Elwell wrote:
>>>> On 27/06/2017 10:15, Andy Shevchenko wrote:
>>>>> On Mon, 2017-06-26 at 16:15 +0100, Phil Elwell wrote:
> 
>>>>>> Besides, the "HFIFO" capability looks a lot like quirk to me.
>>>>>
>>>>> To me either, which raises a question "Should it be fixed
>>>>> accordingly?"
>>>>
>>>> If I was going to make these quirks, are we simply talking about
>>>> renaming the
>>>> capability or is there another mechanism? I've found the 8250_pci
>>>> quirks, and
>>>> they look quite different.
>>>
>>> Okay, we have several types of flags in the code
>>> 1. Capabilities: UART_CAP: looks like it defines features of
>>> hardware
>>> solely for 8250 compatible devices.
>>> 2. Flags as quirks UPF_<something, not all of them> (I have a patch
>>> to
>>> convert them to quirks, need by the way to update and resend): they
>>> are
>>> for any serial devices.
>>> 3. Flags as capabilities: UPF_<the rest>, similar function as
>>> UART_CAP,
>>> but for any serial device.
>>>
>>>>
>>>> I'm also happy to make this code conditional on
>>>> CONFIG_SERIAL_8250_BCM2835AUX
>>>> if that is more acceptable.
>>>
>>> No, it is undesired.
>>>
>>> Can you describe which one from the above suits the best for your
>>> case?
>>
>> This bug I am trying to work around is found in the 8250
>> implementation of
>> one family of CPUs, so I would say capabilities are the best fit
>> because
>> they are specific to 8250 drivers.
> 
> Yeah, looks like UART_CAP suits the best for now.
> 
> One more comment that UART_CAP_* gather capabilities and quirks at the
> same time.

Is that a request for an additional comment? Is there anything else needed
for an ack?

Thanks,

Phil

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Andy Shevchenko]

On Wed, 2017-06-28 at 09:09 +0100, Phil Elwell wrote:
> On 28/06/2017 08:48, Andy Shevchenko wrote:
> > On Tue, 2017-06-27 at 22:00 +0100, Phil Elwell wrote:
> > > On 27/06/2017 18:52, Andy Shevchenko wrote:
> > > > On Tue, 2017-06-27 at 11:30 +0100, Phil Elwell wrote:
> > > > > On 27/06/2017 10:15, Andy Shevchenko wrote:


> > > > Okay, we have several types of flags in the code
> > > > 1. Capabilities: UART_CAP: looks like it defines features of
> > > > hardware
> > > > solely for 8250 compatible devices.
> > > > 2. Flags as quirks UPF_<something, not all of them> (I have a
> > > > patch
> > > > to
> > > > convert them to quirks, need by the way to update and resend):
> > > > they
> > > > are
> > > > for any serial devices.
> > > > 3. Flags as capabilities: UPF_<the rest>, similar function as
> > > > UART_CAP,
> > > > but for any serial device.
> > > > 
> > > > > 
> > > > > I'm also happy to make this code conditional on
> > > > > CONFIG_SERIAL_8250_BCM2835AUX
> > > > > if that is more acceptable.
> > > > 
> > > > No, it is undesired.
> > > > 
> > > > Can you describe which one from the above suits the best for
> > > > your
> > > > case?
> > > 
> > > This bug I am trying to work around is found in the 8250
> > > implementation of
> > > one family of CPUs, so I would say capabilities are the best fit
> > > because
> > > they are specific to 8250 drivers.
> > 
> > Yeah, looks like UART_CAP suits the best for now.
> > 
> > One more comment that UART_CAP_* gather capabilities and quirks at
> > the
> > same time.
> 
> Is that a request for an additional comment?

No.

>  Is there anything else needed
> for an ack?

No, fine by me. Just resend a new version with tags applied. It would be
easier for Greg to pick up it later.

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Phil Elwell]

On 28/06/2017 09:54, Stefan Wahren wrote:
> Hi Phil,
> 
> Am 28.06.2017 um 10:42 schrieb Phil Elwell:
>> The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
>> the bit means that the FIFO is empty (although there may still be a
>> byte in the transmit register), but on 2835 it indicates that the FIFO
>> is not full. This causes interrupts after every byte is transmitted,
>> with the FIFO providing some interrupt latency tolerance.
>>
>> A consequence of this difference is that the usual strategy of writing
>> multiple bytes into the TX FIFO after checking THRE once is unsafe.
>> In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
>> but the first since by then the FIFO is full.
>>
>> There is an HFIFO ("Hidden FIFO") capability that causes the transmit
>> loop to terminate when both THRE and TEMT are set, i.e. when the TX
>> block is completely idle. This is unnecessarily cautious, potentially
>> causing gaps in transmission.
>>
>> Add a new conditional to the transmit loop, predicated on CAP_MINI,
>> that exits when THRE is no longer set (the FIFO is full). This allows
>> the FIFO to fill quickly but subsequent writes are paced by the
>> transmission rate.
>>
>> Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
>> Acked-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
>> Acked-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>  drivers/tty/serial/8250/8250_port.c | 4 ++++
>>  1 file changed, 4 insertions(+)
> 
> please increase the version of the patch and add your changelog below
> this line, so Greg has the chance to apply the right fix.

But I've seen people reprimanded for posting a new version where the only difference
is in the review history, with claims that it doesn't make the merger's job easier.
I'll repost anyway.

> Also i assume that Andy wanted to suggest you to add the fixes tag.

This patch doesnt fix a bug in previous commit, it adds support for broken hardware,
so I don't think the "Fixes:" tag applies.

>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 4c620be..a5fe0e6 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -1764,6 +1764,10 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>>  		if ((up->capabilities & UART_CAP_HFIFO) &&
>>  		    (serial_in(up, UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
>>  			break;
>> +		/* The BCM2835 MINI UART THRE bit is really a not-full bit. */
>> +		if ((up->capabilities & UART_CAP_MINI) &&
>> +		    !(serial_in(up, UART_LSR) & UART_LSR_THRE))
>> +			break;
>>  	} while (--count > 0);
>>  
>>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)

Re: [PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Stefan Wahren]

Hi Phil,

Am 28.06.2017 um 10:42 schrieb Phil Elwell:
> The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
> the bit means that the FIFO is empty (although there may still be a
> byte in the transmit register), but on 2835 it indicates that the FIFO
> is not full. This causes interrupts after every byte is transmitted,
> with the FIFO providing some interrupt latency tolerance.
>
> A consequence of this difference is that the usual strategy of writing
> multiple bytes into the TX FIFO after checking THRE once is unsafe.
> In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
> but the first since by then the FIFO is full.
>
> There is an HFIFO ("Hidden FIFO") capability that causes the transmit
> loop to terminate when both THRE and TEMT are set, i.e. when the TX
> block is completely idle. This is unnecessarily cautious, potentially
> causing gaps in transmission.
>
> Add a new conditional to the transmit loop, predicated on CAP_MINI,
> that exits when THRE is no longer set (the FIFO is full). This allows
> the FIFO to fill quickly but subsequent writes are paced by the
> transmission rate.
>
> Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
> Acked-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Acked-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 4 ++++
>  1 file changed, 4 insertions(+)

please increase the version of the patch and add your changelog below
this line, so Greg has the chance to apply the right fix.

Also i assume that Andy wanted to suggest you to add the fixes tag.

Sorry for the noise before.

Regards
Stefan

>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 4c620be..a5fe0e6 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1764,6 +1764,10 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>  		if ((up->capabilities & UART_CAP_HFIFO) &&
>  		    (serial_in(up, UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
>  			break;
> +		/* The BCM2835 MINI UART THRE bit is really a not-full bit. */
> +		if ((up->capabilities & UART_CAP_MINI) &&
> +		    !(serial_in(up, UART_LSR) & UART_LSR_THRE))
> +			break;
>  	} while (--count > 0);
>  
>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)

[PATCH] serial: 8250: Fix THRE flag usage for CAP_MINI

[Phil Elwell]

The BCM2835 MINI UART has non-standard THRE semantics. Conventionally
the bit means that the FIFO is empty (although there may still be a
byte in the transmit register), but on 2835 it indicates that the FIFO
is not full. This causes interrupts after every byte is transmitted,
with the FIFO providing some interrupt latency tolerance.

A consequence of this difference is that the usual strategy of writing
multiple bytes into the TX FIFO after checking THRE once is unsafe.
In the worst case of 7 bytes in the FIFO, writing 8 bytes loses all
but the first since by then the FIFO is full.

There is an HFIFO ("Hidden FIFO") capability that causes the transmit
loop to terminate when both THRE and TEMT are set, i.e. when the TX
block is completely idle. This is unnecessarily cautious, potentially
causing gaps in transmission.

Add a new conditional to the transmit loop, predicated on CAP_MINI,
that exits when THRE is no longer set (the FIFO is full). This allows
the FIFO to fill quickly but subsequent writes are paced by the
transmission rate.

Signed-off-by: Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
Acked-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Acked-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/tty/serial/8250/8250_port.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4c620be..a5fe0e6 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1764,6 +1764,10 @@ void serial8250_tx_chars(struct uart_8250_port *up)
 		if ((up->capabilities & UART_CAP_HFIFO) &&
 		    (serial_in(up, UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
 			break;
+		/* The BCM2835 MINI UART THRE bit is really a not-full bit. */
+		if ((up->capabilities & UART_CAP_MINI) &&
+		    !(serial_in(up, UART_LSR) & UART_LSR_THRE))
+			break;
 	} while (--count > 0);
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-- 
1.9.1