Re: [PATCH 1/2] tty: pl011: Work around QDF2400 E44 stuck BUSY bit

From: Robin Murphy
Date: Wed Feb 08 2017 - 07:27:55 EST

Next message: Joe Perches: "[PATCH] xenbus: Neaten xenbus_va_dev_error"
Previous message: Michal Hocko: "Re: mm: deadlock between get_online_cpus/pcpu_alloc"
Next in thread: Timur Tabi: "Re: [PATCH 1/2] tty: pl011: Work around QDF2400 E44 stuck BUSY bit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 08/02/17 00:57, Christopher Covington wrote:
> The Qualcomm Datacenter Technologies QDF2400 family of SoCs contains a
> custom (non-PrimeCell) implementation of the SBSA UART. Occasionally the
> BUSY bit in the Flag Register gets stuck as 1, erratum 44 for both 2432v1
> and 2400v1 SoCs. Checking that the Transmit FIFO Empty (TXFE) bit is 0,
> instead of checking that the BUSY bit is 1, works around the issue. To
> facilitate this substitution when UART AMBA Port (UAP) data is available,
> introduce vendor-specific inversion of Feature Register bits. To keep the
> change small, this patch only works around the full console case, where UAP
> data is available, and does not handle the erratum for earlycon, as the UAP
> data is not available then.
>
> Signed-off-by: Christopher Covington <cov@xxxxxxxxxxxxxx>
> Acked-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
> ---
> Changes between the previous RFC [1] and this PATCH:
> * don't use arch/arm64/kernel/cpu_errata.c at Will's request
> * separate out earlycon case to separate patch
> * rework earlycon case to not depend on UAP as suggested by Timur
>
> Because they need a newly-defined MIDR values, the patches are currently
> based on:
> https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/log/?h=for-next/core
>
> I'm not confident that I know the best route for these two patches. Should
> I request Catalin and Will to take them via arm64 as the essential MIDR
> additions are their purview?
>
> Thanks Russell for the ack. Compared to the RFC, I've made minor changes to
> what is now patch 1/2 and am making an educated guess that the ack sticks
> (but if not please let me know). Patch 2/2 is significantly revised from
> the RFC so I've not included the ack on it.
>
> 1. https://patchwork.codeaurora.org/patch/163281/
> ---
> Documentation/arm64/silicon-errata.txt | 2 ++
> arch/arm64/include/asm/cputype.h | 2 ++
> drivers/tty/serial/Kconfig | 12 ++++++++++
> drivers/tty/serial/amba-pl011.c | 40 +++++++++++++++++++++++++++++++---
> 4 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> index 50da8391e9dd..0993ebb3e86b 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -65,3 +65,5 @@ stable kernels.
> | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 |
> | Qualcomm Tech. | Falkor v1 | E1003 | QCOM_FALKOR_ERRATUM_1003|
> | Qualcomm Tech. | Falkor v1 | E1009 | QCOM_FALKOR_ERRATUM_1009|
> +| Qualcomm Tech. | QDF2432v1 UART | SoC E44 | QCOM_QDF2400_ERRATUM_44 |
> +| Qualcomm Tech. | QDF2400v1 UART | SoC E44 | QCOM_QDF2400_ERRATUM_44 |
> diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
> index fc502713ab37..cb399c7fe6ec 100644
> --- a/arch/arm64/include/asm/cputype.h
> +++ b/arch/arm64/include/asm/cputype.h
> @@ -88,12 +88,14 @@
>
> #define BRCM_CPU_PART_VULCAN 0x516
>
> +#define QCOM_CPU_PART_KRYO_V1 0x281
> #define QCOM_CPU_PART_FALKOR_V1 0x800
>
> #define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
> #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
> #define MIDR_THUNDERX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
> #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX)
> +#define MIDR_QCOM_KRYO_V1 MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, QCOM_CPU_PART_KRYO_V1)
> #define MIDR_QCOM_FALKOR_V1 MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, QCOM_CPU_PART_FALKOR_V1)
>
> #ifndef __ASSEMBLY__
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index e9cf5b67f1b7..4cde8f48a540 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -73,6 +73,18 @@ config SERIAL_AMBA_PL011_CONSOLE
> your boot loader (lilo or loadlin) about how to pass options to the
> kernel at boot time.)
>
> +config QCOM_QDF2400_ERRATUM_44
> + bool "Work around QDF2400 SoC E44 stuck BUSY bit"
> + depends on SERIAL_AMBA_PL011_CONSOLE=y
> + default y
> + help
> + The BUSY bit in the Flag Register of the UART on the QDF2432v1 and
> + QDF2400v1 SoCs may get stuck as 1, resulting in a hung serial console.
> + Say Y here to work around the issue by checking TXFE == 0 instead of
> + BUSY == 1 on affected systems.
> +
> + If unsure, say Y.

Is there a reason anyone would ever want to turn this off? AFAICS you
save a few dozen bytes in return for a kernel image which you know won't
work properly on some hardware. That doesn't seem particularly
worthwhile, and it's not like the PL011 driver isn't already ripe with
unconditional vendor-specific data.

> +
> config SERIAL_EARLYCON_ARM_SEMIHOST
> bool "Early console using ARM semihosting"
> depends on ARM64 || ARM
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index d4171d71a258..41e51901d6ef 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -97,6 +97,7 @@ struct vendor_data {
> unsigned int fr_dsr;
> unsigned int fr_cts;
> unsigned int fr_ri;
> + unsigned int inv_fr;
> bool access_32b;
> bool oversampling;
> bool dma_threshold;
> @@ -141,6 +142,25 @@ static struct vendor_data vendor_sbsa = {
> .fixed_options = true,
> };
>
> +#ifdef CONFIG_QCOM_QDF2400_ERRATUM_44
> +static struct vendor_data vendor_qdt_qdf2400_e44 = {
> + .reg_offset = pl011_std_offsets,
> + .fr_busy = UART011_FR_TXFE,
> + .fr_dsr = UART01x_FR_DSR,
> + .fr_cts = UART01x_FR_CTS,
> + .fr_ri = UART011_FR_RI,
> + .inv_fr = UART011_FR_TXFE,
> + .access_32b = true,
> + .oversampling = false,
> + .dma_threshold = false,
> + .cts_event_workaround = false,
> + .always_enabled = true,
> + .fixed_options = true,
> +};
> +#else
> +#define vendor_qdt_qdf2400_e44 vendor_sbsa
> +#endif
> +
> static u16 pl011_st_offsets[REG_ARRAY_SIZE] = {
> [REG_DR] = UART01x_DR,
> [REG_ST_DMAWM] = ST_UART011_DMAWM,
> @@ -1518,7 +1538,7 @@ static unsigned int pl011_tx_empty(struct uart_port *port)
> {
> struct uart_amba_port *uap =
> container_of(port, struct uart_amba_port, port);
> - unsigned int status = pl011_read(uap, REG_FR);
> + unsigned int status = pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr;
> return status & (uap->vendor->fr_busy | UART01x_FR_TXFF) ?
> 0 : TIOCSER_TEMT;
> }
> @@ -2218,7 +2238,8 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)
> * Finally, wait for transmitter to become empty
> * and restore the TCR
> */
> - while (pl011_read(uap, REG_FR) & uap->vendor->fr_busy)
> + while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr)
> + & uap->vendor->fr_busy)
> cpu_relax();
> if (!uap->vendor->always_enabled)
> pl011_write(old_cr, uap, REG_CR);
> @@ -2383,6 +2404,13 @@ static struct console amba_console = {
>
> #define AMBA_CONSOLE (&amba_console)
>
> +static bool qdf2400_e44(void) {
> + u32 cpu_var_model = read_cpuid_id() & ~MIDR_REVISION_MASK;
> +
> + return (cpu_var_model == MIDR_QCOM_KRYO_V1 ||
> + cpu_var_model == MIDR_QCOM_FALKOR_V1);

Are we to take it that every SoC now and always with any Kryo or Falkor
core which also has an SBSA UART will require this workaround? There's a
guarantee that the UART itself will definitely never be fixed in some
future revision? That it'll never be integrated into, say, some
Kryo/Cortex-Axx big.LITTLE system where you do still need the
workaround, but might be making this check on the wrong core?

If there's really no suitable ID register to identify this particular
UART implementation, it probably needs to be described appropriately by
the firmware - I can't claim knowledge of how that works under ACPI, but
I do note that the only device ID currently being matched is "ARMH0011",
which seems to me to be inappropriate to describe something which is not
an ARM PL011, bugs or no bugs.

Robin.

> +}
> +
> static void pl011_putc(struct uart_port *port, int c)
> {
> while (readl(port->membase + UART01x_FR) & UART01x_FR_TXFF)
> @@ -2645,12 +2673,18 @@ static int sbsa_uart_probe(struct platform_device *pdev)
> uap->port.irq = ret;
>
> uap->reg_offset = vendor_sbsa.reg_offset;
> - uap->vendor = &vendor_sbsa;
> uap->fifosize = 32;
> uap->port.iotype = vendor_sbsa.access_32b ? UPIO_MEM32 : UPIO_MEM;
> uap->port.ops = &sbsa_uart_pops;
> uap->fixed_baud = baudrate;
>
> + if (qdf2400_e44()) {
> + uap->vendor = &vendor_qdt_qdf2400_e44;
> + dev_info(&pdev->dev, "working around QDF2400 SoC erratum 44\n");
> + } else {
> + uap->vendor = &vendor_sbsa;
> + }
> +
> snprintf(uap->type, sizeof(uap->type), "SBSA");
>
> r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>

Next message: Joe Perches: "[PATCH] xenbus: Neaten xenbus_va_dev_error"
Previous message: Michal Hocko: "Re: mm: deadlock between get_online_cpus/pcpu_alloc"
Next in thread: Timur Tabi: "Re: [PATCH 1/2] tty: pl011: Work around QDF2400 E44 stuck BUSY bit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]