[PATCH] disable RTS override on LPSS UART with Auto Flow Control

[PATCH] disable RTS override on LPSS UART with Auto Flow Control

[Dan O'Donovan]

Currently, Auto Flow Control is not working correctly on the Atom
X5-Z8350 "Cherry Trail" SoC, because an "RTS override" feature is
enabled in a vendor-specific register in the LPSS UART. The symptom
is that RTS is not de-asserted as it should be when RTS/CTS flow
control is enabled and the RX FIFO fills up.

This appears to be introduced by commit 1f47a77c4e49 ("ACPI / LPSS:
not using UART RTS override with Auto Flow Control").

To _disable_ the RTS override, bit 3 needs to be _set_ in the
"GENERAL" register at offset 808h.  The power-on default is 0. The
aforementioned commit appears to have assumed the inverse of this.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/acpi/acpi_lpss.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 8ea836c..4b3f2d5 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -45,7 +45,7 @@ ACPI_MODULE_NAME("acpi_lpss");
 #define LPSS_RESETS_RESET_APB		BIT(1)
 #define LPSS_GENERAL			0x08
 #define LPSS_GENERAL_LTR_MODE_SW	BIT(2)
-#define LPSS_GENERAL_UART_RTS_OVRD	BIT(3)
+#define LPSS_GENERAL_UART_RTS_NO_OVRD	BIT(3)
 #define LPSS_SW_LTR			0x10
 #define LPSS_AUTO_LTR			0x14
 #define LPSS_LTR_SNOOP_REQ		BIT(15)
@@ -123,10 +123,10 @@ static void lpss_uart_setup(struct lpss_private_data *pdata)
 	writel(val | LPSS_TX_INT_MASK, pdata->mmio_base + offset);
 
 	val = readl(pdata->mmio_base + LPSS_UART_CPR);
-	if (!(val & LPSS_UART_CPR_AFCE)) {
+	if (val & LPSS_UART_CPR_AFCE) {
 		offset = pdata->dev_desc->prv_offset + LPSS_GENERAL;
 		val = readl(pdata->mmio_base + offset);
-		val |= LPSS_GENERAL_UART_RTS_OVRD;
+		val |= LPSS_GENERAL_UART_RTS_NO_OVRD;
 		writel(val, pdata->mmio_base + offset);
 	}
 }
-- 
2.7.4

Re: [PATCH] disable RTS override on LPSS UART with Auto Flow Control

[Andy Shevchenko]

On Wed, 2017-03-15 at 14:13 +0000, Dan O'Donovan wrote:
> Currently, Auto Flow Control is not working correctly on the Atom
> X5-Z8350 "Cherry Trail" SoC, because an "RTS override" feature is
> enabled in a vendor-specific register in the LPSS UART. The symptom
> is that RTS is not de-asserted as it should be when RTS/CTS flow
> control is enabled and the RX FIFO fills up.
> 
> This appears to be introduced by commit 1f47a77c4e49 ("ACPI / LPSS:
> not using UART RTS override with Auto Flow Control").
> 
> To _disable_ the RTS override, bit 3 needs to be _set_ in the
> "GENERAL" register at offset 808h.  The power-on default is 0. The
> aforementioned commit appears to have assumed the inverse of this.

Thanks for spotting this.

Documentation I have says the same.

Nevertheless I would like to have Heikki's Ack before going with this
one.

Moreover, some tests on Baytrail and Cherrytrail platforms should be
performed. Do you have a test case step-by-step to reproduce the issue?

It might also explain the following (would be nice to test with this
revert reverted):

commit f967fc8f165fadb72166f2bd4785094b3ca21307
Author: Frederic Danis <frederic.danis@linux.intel.com>
Date:   Fri Oct 9 17:14:56 2015 +0200

    Revert "serial: 8250_dma: don't bother DMA with small transfers"


See my comments regarding to code below.

> 
> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> ---
>  drivers/acpi/acpi_lpss.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 8ea836c..4b3f2d5 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -45,7 +45,7 @@ ACPI_MODULE_NAME("acpi_lpss");
>  #define LPSS_RESETS_RESET_APB		BIT(1)
>  #define LPSS_GENERAL			0x08
>  #define LPSS_GENERAL_LTR_MODE_SW	BIT(2)
> -#define LPSS_GENERAL_UART_RTS_OVRD	BIT(3)
> +#define LPSS_GENERAL_UART_RTS_NO_OVRD	BIT(3)

Better to use closer to documentation, i.e.

_RTS_DIS_OVRD


>  #define LPSS_SW_LTR			0x10
>  #define LPSS_AUTO_LTR			0x14
>  #define LPSS_LTR_SNOOP_REQ		BIT(15)
> @@ -123,10 +123,10 @@ static void lpss_uart_setup(struct
> lpss_private_data *pdata)
>  	writel(val | LPSS_TX_INT_MASK, pdata->mmio_base + offset);
>  
>  	val = readl(pdata->mmio_base + LPSS_UART_CPR);
> -	if (!(val & LPSS_UART_CPR_AFCE)) {
> +	if (val & LPSS_UART_CPR_AFCE) {

Perhaps we need to clear it otherwise to be sure

something like

offset = pdata->dev_desc->prv_offset + LPSS_GENERAL;
val = readl(pdata->mmio_base + offset);
if (..._AFCE)
 val |= LPSS_GENERAL_UART_RTS_DIS_OVRD;
else
 val &= ~LPSS_GENERAL_UART_RTS_DIS_OVRD;
writel(val, pdata->mmio_base +
offset);


>  		offset = pdata->dev_desc->prv_offset + LPSS_GENERAL;
>  		val = readl(pdata->mmio_base + offset);
> -		val |= LPSS_GENERAL_UART_RTS_OVRD;
> +		val |= LPSS_GENERAL_UART_RTS_NO_OVRD;
>  		writel(val, pdata->mmio_base + offset);
>  	}
>  }

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

Re: [PATCH] disable RTS override on LPSS UART with Auto Flow Control

[Heikki Krogerus]

On Wed, Mar 15, 2017 at 02:13:48PM +0000, Dan O'Donovan wrote:
> Currently, Auto Flow Control is not working correctly on the Atom
> X5-Z8350 "Cherry Trail" SoC, because an "RTS override" feature is
> enabled in a vendor-specific register in the LPSS UART. The symptom
> is that RTS is not de-asserted as it should be when RTS/CTS flow
> control is enabled and the RX FIFO fills up.
> 
> This appears to be introduced by commit 1f47a77c4e49 ("ACPI / LPSS:
> not using UART RTS override with Auto Flow Control").
> 
> To _disable_ the RTS override, bit 3 needs to be _set_ in the
> "GENERAL" register at offset 808h.  The power-on default is 0. The
> aforementioned commit appears to have assumed the inverse of this.

True. That is what the public documentation for Atom Z8000
(Cherrytrail/Braswell), and also Z36xx-Z37xx (Baytrail) say. The
internal Lynxpoint PCH documentation I used describe the bit
differently, but we should of course follow the public documentation.
The LPSS block housing the UART is in any case the same in all these
products.

After addressing Andy's comments:
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This should also go to stable.


Thanks,

-- 
heikki