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

[Christopher Covington <cov@codeaurora.org>]

On 02/07/2017 11:05 PM, Timur Tabi wrote:
> 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.

>> +#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
> 
> Instead of the #else, just put the #ifdef inside qdf2400_e44(). That
> way, the function always returns False if
> CONFIG_QCOM_QDF2400_ERRATUM_44 is not defined.

I'll consolidate the #ifdefery around qdf2400_e44().

> Also, don't you need to add a definition of .inv_fr in vendor_sbsa
> and the other vendor_xxx structures?

The current struct definitions appear to (ab)use implicit
zero-initialization, so I did too.

>> +
>>  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)
> 
> I really think the XOR logic needs to be documented wherever it's
> used. It's just too confusing.

References such as the following for basic bit operations like
setting, clearing, and toggling are easy enough to come by.

http://stackoverflow.com/questions/47981/how-do-you-set-clear-and-toggle-a-single-bit-in-c-c

Cov

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.