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

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

  • To: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 16 Aug 2021 14:18:33 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=sLdrxTGdZFCryAH4GWRA2jk7HUt4Rx9l8xGQOGwP3H0=; b=loZkZQUyxBJMX22ySK3AAa+joX/75ewAt791bdWzw/mfzDKGIkevXMQoUbaF2U2JSXASuRNHp1Ygzgt2r6itKbUkL9eaQ0ER75cDKEv/ZZl0M6ye8bSPwCBtvsh7PP8JRLPDG91N0tFsUl1GKr2dSNFdOMho9vwusBxnmRot9U/0WdQozcBt/OCQblwcrI5jD+IcvmEwjEJtLRmbGzsI+4YejHrgte/V7T7lpC7kQdTi/30o2wRO1BaFzjfgXrubEGQhgqQk4foy78XLk5fMjxfWoeBxC6gvoIwIfG6XoGGxphe7n+tP68stba/ExOEYIm2CEIuN84e8ReqyA3wDsQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iEb6M5xll7AcrdZP7NgYYJHI5kUrCsn1knmQK/pLw5lSWAC8/K8SSeFv+nWFO4ZCkz5z2soFZ4Gbk/Isd3d85m31yclSYM5BUmjaxzSCOIde0brF6WkMvjRjOaKlgZrSBFzXz7M8ds9l5G7Y65d+ckZ8W1jGW3KOa1oijfJF9MgjZbRmTlovK6ZEYjST9Cl+0CWS43vvqjKcFYs92HSonvZd+Z/C6pe5KwuWVA043d8jE5Y2IP9U0QvC+T+/e9TuZe0EOwsGUb8J5CTZ2tt34dPzOa+UDhSQI9YqgzpU4djDOhqF8rQBaETVclxxZieELFNhA1ZXjfRWcJ3Xx9iRUQ==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 16 Aug 2021 12:18:50 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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) == 
>>>>> +    {
>>>>> +        /* 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).




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.