Re: [PATCH v2] tty: pl011: Work around QDF2400 E44 stuck BUSY bit

[Timur Tabi <timur@codeaurora.org>]

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. For the
> earlycon case, implement alternative putc and early_write functions.
> Similar to what how ARMv8 ACPI PCI quirks are detected during MCFG parsing,
> check the OEM fields of the Serial Port Console Redirection (SPCR) ACPI
> table to determine if the current platform is known to be affected by the
> erratum.

Please break this up into paragraphs.


> +	if (!memcmp(table->header.oem_id, "QCOM  ", ACPI_OEM_ID_SIZE))
> +		if (!memcmp(table->header.oem_table_id, "QDF2432 ",
> +				ACPI_OEM_TABLE_ID_SIZE) ||
> +				(!memcmp(table->header.oem_table_id,
> +				"QDF2400 ", ACPI_OEM_TABLE_ID_SIZE) &&
> +				table->header.oem_revision == 0))
> +			uart = "qdf2400_e44";
> +

Hey, you just wrote this
And it looks crazy
But here's my function
So call it, maybe?

static bool needs_qdf2400_e44(struct acpi_table_header *h)
{
	const char *id;

	/* The erratum only applies to Qualcomm Technologies SOCs */
	if (memcmp(h->oem_id, "QCOM  ", ACPI_OEM_ID_SIZE))
		return False;

	id = h->oem_table_id;

	if (!memcmp(id, "QDF2432 ", ACPI_OEM_TABLE_ID_SIZE))
		return True;

	if (!memcmp(id, "QDF2000 ", ACPI_OEM_TABLE_ID_SIZE) &&
		h->oem_revision == 0)
		return True;
	
	return False;
}

...

	if (needs_qdf2400_e44(&table->header))
		uart = "qdf2400_e44";


> +/*
> + * Erratum 44 for QDF2432v1 and QDF2400v1 SoCs describes the BUSY bit as
> + * occasionally getting stuck as 1. To avoid the potential for a hang, check
> + * TXFE == 0 instead of BUSY == 1. This may not be suitable for all UART
> + * implementations, so only do so if an affected platform is detected in
> + * parse_spcr().
> + */
> +static bool qdf2400_e44 = false;

Please rename this to needs_qdf2400_e44

>  static u16 pl011_st_offsets[REG_ARRAY_SIZE] = {
>  	[REG_DR] = UART01x_DR,
>  	[REG_ST_DMAWM] = ST_UART011_DMAWM,
> @@ -1518,7 +1543,8 @@ 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);
> +	/* Allow feature register bits to be inverted to work around errata */

Blank line above the comment, please.

> +	unsigned int status = pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr;
>  	return status & (uap->vendor->fr_busy | UART01x_FR_TXFF) ?
>  							0 : TIOCSER_TEMT;
>  }
> @@ -2215,10 +2241,12 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)
>  	uart_console_write(&uap->port, s, count, pl011_console_putchar);
>
>  	/*
> -	 *	Finally, wait for transmitter to become empty
> -	 *	and restore the TCR
> +	 *	Finally, wait for transmitter to become empty and restore the
> +	 *	TCR. Allow feature register bits to be inverted to work around
> +	 *	errata.
>  	 */
> -	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);
> @@ -2340,8 +2368,13 @@ static int __init pl011_console_match(struct console *co, char *name, int idx,
>  	resource_size_t addr;
>  	int i;
>
> -	if (strcmp(name, "pl011") != 0)
> +	if (strcmp(name, "qdf2400_e44") == 0) {
> +		if (!qdf2400_e44)
> +			pr_info("UART: Working around QDF2400 SoC erratum 44");

Just use pr_once(), and you can skip the "if (!qdf2400_e44)".  Although I don't 
understand why you need to do that.  pl011_console_match() should only be called 
once anyway, right?

> +		qdf2400_e44 = true;
> +	} else if (strcmp(name, "pl011") != 0) {
>  		return -ENODEV;
> +	}
>
>  	if (uart_parse_earlycon(options, &iotype, &addr, &options))
>  		return -ENODEV;
> @@ -2383,6 +2416,25 @@ static struct console amba_console = {
>
>  #define AMBA_CONSOLE	(&amba_console)
>
> +static void qdf2400_e44_putc(struct uart_port *port, int c)
> +{
> +	while (readl(port->membase + UART01x_FR) & UART01x_FR_TXFF)
> +		cpu_relax();
> +	if (port->iotype == UPIO_MEM32)
> +		writel(c, port->membase + UART01x_DR);
> +	else
> +		writeb(c, port->membase + UART01x_DR);

I think you can ignore the UPIO_MEM32 test and always use writel(), even on the 
QDF2400.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.