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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
Intel Finland Oy
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 <[email protected]>
This should also go to stable.
Thanks,
--
heikki