linux-kernel - Re: [PATCH v3 tty-next 2/2] serial: 8250: add driver for NI UARTs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f6df7da0-b4c0-0f34-ffe3-6b9ff1d6a2d2@ni.com>
Date:   Fri, 28 Apr 2023 16:03:17 -0500
From:   Brenda Streiff <brenda.streiff@...com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Gratian Crisan <gratian.crisan@...com>,
        Jason Smith <jason.smith@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-serial <linux-serial@...r.kernel.org>,
        devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 tty-next 2/2] serial: 8250: add driver for NI UARTs

On 4/19/23 06:43, Ilpo Järvinen wrote:
> 
>> +	pcr = serial_in(up, NI16550_PCR_OFFSET);
>> +	pcr &= ~NI16550_PCR_WIRE_MODE_MASK;
>> +
>> +	if (rs485->flags & SER_RS485_ENABLED) {
>> +		/* RS-485 */
>> +		dev_dbg(port->dev, "2-wire Auto\n");
>> +		pcr |= NI16550_PCR_AUTO_RS485;
>> +		up->acr |= NI16550_ACR_AUTO_DTR_EN;
>> +	} else {
>> +		/* RS-422 */
>> +		dev_dbg(port->dev, "4-wire\n");
>> +		pcr |= NI16550_PCR_RS422;
>> +		up->acr &= ~NI16550_ACR_AUTO_DTR_EN;
>> +	}
> 
> Does this difference also mean the UART's RS485 could support
> SER_RS485_RX_DURING_TX? (half-duplex vs full-duplex RS-485)

Maybe. At least on some devices it appears that it might be possible to
support this with a combination of the NI16550_PCR_ECHO_RS485 mode (this
is "2-Wire DTR Controlled With Echo" in NI parlance [1]) along with the
AUTO_DTR mode set in ACR (which asserts DTR if there's data in the TX
FIFO).

We've never tried using it in that way (neither in prior versions of this
driver nor our driver for this hardware on Windows), so I'm not sure that
works that way on all revisions of the hardware, so I'm hesitant to
include that in this patchset for now.

[1] https://www.ni.com/en-us/support/documentation/supplemental/18/transceiver-modes-on-ni-rs-485-serial-cards.html

>> +
>> +static const struct serial_rs485 ni16550_rs485_supported = {
>> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
> 
> The driver does not seem to change anything based on the value of
> SER_RS485_RTS_ON_SEND, was this an oversight or is RTS on/off on/after
> send not supported?

Not so much "not supported" as "doesn't matter"?

SER_RS485_RTS_(ON/AFTER)_SEND (which would be a misnomer anyway, as we use
DTR and not RTS) appear to let userspace indicate whether or not the
transmitter is active-high or active-low, under the assumption that the
UART driver doesn't know anything itself about its attached transceiver.
This driver does, so it doesn't need either flag to do the right thing.

However, there is existing userspace software does try to configure the
SER_RS485_RTS_(ON/AFTER)_SEND flags; for instance, pyserial's RS485Settings
sets rts_level_for_tx=True [2] and this maps directly to RTS_ON_SEND. Our
own proprietary Linux software also sets these flags. So the behavior we
want is to we effectively treat them as don't-cares here.

I can add comments on that further though, because I can see how that looks
a bit confusing.

[2] https://github.com/pyserial/pyserial/blob/31fa4807d73ed4eb9891a88a15817b439c4eea2d/serial/rs485.py#L22

>> +
>> +/* NI cRIO-904x RS-485 Interface */
>> +static const struct ni16550_device_info nic792b = {
>> +	/* Sets UART clock rate to 22.222 MHz with 1.125 prescale */
>> +	.uartclk = 25000000,
>> +	.prescaler = 0x09,
>> +};
>> +
>> +/* NI sbRIO 96x8 RS-232/485 Interfaces */
>> +static const struct ni16550_device_info nic7a69 = {
>> +	/* Set UART clock rate to 29.629 MHz with 1.125 prescale */
>> +	.uartclk = 29629629,
>> +	.prescaler = 0x09,
> 
> To me these two comments don't seemingly agree, one states 22.222MHz and
> defines 25M clk, whereas the other states 29.629MHz and defines 29.629M
> clk. I guess one of them comes from prescaled and the other from
> postscaled frequency?

This was an error-- the comment is correct, I had the uartclk value wrong
(should be 22222222).