Re: [PATCH 2/2] ns16550: add Exar dual PCIe UART card support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2021 12:25, Marek Marczykowski-Górecki wrote:
> On Mon, Aug 16, 2021 at 11:18:31AM +0200, Jan Beulich wrote:
>> On 16.08.2021 10:39, Marek Marczykowski-Górecki wrote:
>>> On Mon, Aug 16, 2021 at 09:55:16AM +0200, Jan Beulich wrote:
>>>> On 13.08.2021 20:31, Marek Marczykowski-Górecki wrote:
>>>>> Besides standard UART setup, this device needs enabling
>>>>> (vendor-specific) "Enhanced Control Bits" - otherwise disabling hardware
>>>>> control flow (MCR[2]) is ignored. Add appropriate quirk to the
>>>>> ns16550_setup_preirq(), similar to the handle_dw_usr_busy_quirk(). The
>>>>> new function act on Exar cards only (based on vendor ID).
>>>>
>>>> While on IRC you did say you have a datasheet or alike for the specific
>>>> card you have in use, may I ask that you clarify why the logic is
>>>> applicable to all (past, present, and future) Exar cards?
>>>
>>> The spec I looked is specifically about 2-port variant (XR17V352), but
>>> there are also 4 and 8 port variants (XR17V354 and XR17V358) and the
>>> Linux driver applies this change there as well. But indeed applying it
>>> to all the future cards may not be the smartest thing to do.
>>>
>>> The Linux driver checks Exar specific register to identify the device,
>>> instead of using PCI product ID, for some reason - I guess they use the
>>> same chip in different devices?
>>> Would you like thing like that (after checking vendor id), or turn it on
>>> just for this product id I have?
>>
>> Hard to tell without knowing whether the extra reg - as per the spec -
>> is connected to any of these. Is the spec you have publicly available?
> 
> Yes, here: 
> https://www.maxlinear.com/document/index?id=1585&languageid=1033&type=Datasheet&partnumber=XR17V352&filename=XR17V352.pdf&part=XR17V352
> (and few more links on 
> https://www.maxlinear.com/product/interface/uarts/pcie-uarts/xr17v352, but 
> mostly the above PDF)

Ah yes, thanks.

> Hmm, maybe I should add the link to the commit message?

Wouldn't hurt; question is how likely it is for the link to become stale
in the next couple of years.

>>>>> @@ -169,6 +170,21 @@ static void handle_dw_usr_busy_quirk(struct ns16550 
>>>>> *uart)
>>>>>      }
>>>>>  }
>>>>>  
>>>>> +static void enable_exar_enhanced_bits(struct ns16550 *uart)
>>>>> +{
>>>>> +#ifdef NS16550_PCI
>>>>> +    if ( uart->bar &&
>>>>> +         pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[2],
>>>>> +                         uart->ps_bdf[2]), PCI_VENDOR_ID) == 
>>>>> PCI_VENDOR_ID_EXAR )
>>>>> +    {
>>>>> +        /* Exar cards ignores setting MCR[2] (hardware flow control) 
>>>>> unless
>>>>> +         * "Enhanced control bits" is enabled.
>>>>> +         */
>>>>
>>>> Style nit: /* belongs on its own line as per ./CODING_STYLE.
>>>>
>>>>> +        ns_write_reg(uart, UART_XR_EFR, UART_EFR_ECB);
>>>>
>>>> Wouldn't this better be a read-modify-write operation?
>>>
>>> Honestly, I'm simply mirroring Linux driver behavior here. But also,
>>> all the bits in EFR are 0 after device reset, so it should work fine.
>>
>> Firmware or a boot loader may play with hardware before Xen takes control.
>> I'm also not convinced there would have been a device reset in all cases
>> where execution may make it here. (Note in particular that the function
>> and its caller aren't __init.)
>>
>> A plain write might be okay if the spec for devices with the specific
>> device ID documented all other bits as "must be zero" ("reserved" would
>> not be sufficient imo), and if the function was invoked for only such
>> devices.
> 
> Other bits are defined and are things IMO we want to keep disabled. See top
> of the page 40 in the PDF.

To be honest, in particular for the low 4 bits I'm not sure we should
alter them if they turn out non-zero (e.g. due to firmware or boot
loader action).

Jan