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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Robin Murphy wrote:
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?

No, only Kryo and Falkor V1 based SOCs have this problem. Falkor V2 will have this fixed. We intend to revert these fixes after Falkor V1 SOCs are no longer supported.

> 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?

We are sure that this erratum is restricted to those specific SOCs.


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.

ACPI is not like device tree. You can't just define a "qcom,needs-busy-bit-fix" property and call it a day.

If you're saying that we should create a new ACPI HID, like QCOM0011, that probably would have worked as well, except hindsight is 20/20, and we already have firmware in the field.

--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux