Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"

From: Marek Vasut
Date: Tue Dec 18 2018 - 19:53:10 EST

Next message: Stephen Rothwell: "Re: linux-next: build warning after merge of the tip tree"
Previous message: Jaegeuk Kim: "Re: [f2fs-dev] [PATCH v2] f2fs: add bio cache for IPU"
In reply to: Marek Vasut: "Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again""
Next in thread: Paul Burton: "Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again""
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 12/17/2018 06:20 PM, Marek Vasut wrote:
> On 12/17/2018 05:30 PM, Ezequiel Garcia wrote:
>> On Sun, 16 Dec 2018 at 19:35, Paul Burton <paul.burton@xxxxxxxx> wrote:
>>>
>>> Hi Ezequiel,
>>>
>>> On Sun, Dec 16, 2018 at 07:28:22PM -0300, Ezequiel Garcia wrote:
>>>> On Sun, 16 Dec 2018 at 19:24, Paul Burton <paul.burton@xxxxxxxx> wrote:
>>>>> This helps, but it only addresses one part of one of the 4 reasons I
>>>>> listed as motivation for my revert. For example serial8250_do_shutdown()
>>>>> also clearly intends to disable the FIFOs.
>>>>>
>>>>
>>>> OK. So, let's fix that :-)
>>>
>>> I already did, or at least tried to, on Thursday [1].
>>>
>>>> By all means, it would be really nice to push forward and fix the garbage
>>>> issue on JZ4780, as well as the transmission issue on AM335x.
>>>>
>>>> AM335x is a wildly popular platform, and it's not funny to break it.
>>>
>>> Well, clearly not if it was broken in v4.10 & only just fixed..? And
>>> from Marek's commit message the patch in v4.10 doesn't break the whole
>>> system just RS485.
>>>
>>
>> Careful here. It's naive to consider v4.10 as old in this context.
>>
>> AM335x is used in hundreds of thousands of products, probably
>> "industrial" products.
>> Manufacturers don't always follow the kernel, and it's entirely
>> likely that they notice a regression only when developing a new product,
>> or when rebasing on top of a newer longterm kernel.
>>
>> Then again, I don't have any details about what is Marek's original fix
>> actually fixing.
>>
>> Marek: could you please post the test case that you used to validate your fix?
>> I can't find that anywhere.
>
> I can't share the testcase itself because it has no license and I didn't
> write it, but I can tell you what it's doing. (I'll check if I can share
> the testcase verbatim too, I just sent an email to the author)
>
> The test spawns two threads, one sending , one receiving. The sending
> thread sends 8 bytes of data from /dev/ttyS4 , the receiving thread
> receives the data on /dev/ttyS2 and compares the pattern. The port
> settings is B1000000 8N1 with rs485conf.flags set to SER_RS485_ENABLED
> and SER_RS485_RTS_AFTER_SEND. Sometimes the received data do not match
> the sent data, but rather look as if one character was left over from
> the previous transmission in the FIFO.
>
> Those two UARTs are connected together by two wires, without any real
> RS485 hardware to minimize the hardware complexity (it's easy to
> implement that on the pocketbeagle, which is the cheap option here).

Test code is below . On the pocketbeagle, connect SPI0:CLK with U4:TX
and SPI0:MISO with U4:RX , apply the DT patch below and run the example.
It'll fail after a few iterations.

#include <errno.h>
#include <fcntl.h>
#include <string.h>
#include <termios.h>
#include <unistd.h>

#include <iostream>
#include <iomanip>
#include <atomic>
#include <thread>

#include <linux/serial.h>
#include <sys/ioctl.h>

std::atomic<bool> running{true};

int set_interface_attribs (int fd, int speed, int parity)
{
struct termios tty;
memset (&tty, 0, sizeof tty);
if (tcgetattr (fd, &tty) != 0)
{
std::cerr << "Error from tcgetattr" << std::endl;
return -1;
}

cfsetospeed (&tty, speed);
cfsetispeed (&tty, speed);

tty.c_cflag = (tty.c_cflag & ~CSIZE) | CS8;
tty.c_iflag &= ~IGNBRK;
tty.c_lflag = 0;

tty.c_oflag = 0;
tty.c_cc[VMIN] = 8;
tty.c_cc[VTIME] = 0;

tty.c_iflag &= ~(IXON | IXOFF | IXANY);

tty.c_cflag |= (CLOCAL | CREAD);
tty.c_cflag &= ~(PARENB | PARODD);
tty.c_cflag |= parity;
tty.c_cflag &= ~CSTOPB;
tty.c_cflag &= ~CRTSCTS;

if (tcsetattr (fd, TCSANOW, &tty) != 0)
{
std::cerr << "Error from tcsetattr" << std::endl;
return -1;
}
return 0;
}

void enable_rts(int fd, int rts)
{
struct serial_rs485 rs485conf;
if (rts) {
rs485conf.flags = SER_RS485_ENABLED ;
rs485conf.flags |= SER_RS485_RTS_AFTER_SEND;
rs485conf.flags &= ~(SER_RS485_RTS_ON_SEND);
rs485conf.delay_rts_before_send = 0;
rs485conf.delay_rts_after_send = 0;
} else {
rs485conf.flags = 0 ;
}

if (ioctl( fd, TIOCSRS485, &rs485conf) < 0){
std::cerr << "Cannot set device in RS485 mode" << std::endl;
}
}

void output_function(int fd)
{
while(running) {
write (fd, "asdfghjk", 8);
usleep (20000);
}
}

void input_function(int fd)
{
char buf [100];
size_t count=0;
std::cout << std::unitbuf;
while(running){
int n = read (fd, buf, sizeof buf);
if( (n >0) && (buf[0] != 'a') ){
std::cout << "\nunexpected receive! " << std::string(buf,8) <<
std::endl;
running = false;
} else {
++count;
if(count%100 == 0){
std::cout << "\r" << std::setw(8) << count << " possibly ok";
}
}
}
}

int main(int argc, char** argv)
{
std::string in_port = "/dev/ttyS2";
std::string out_port = "/dev/ttyS4";

int c, rts = 1;

while ((c = getopt (argc, argv, "i:o:r")) != -1)
{
switch (c)
{
case 'i':
in_port = std::string(optarg);
break;
case 'o':
out_port = std::string(optarg);
break;
case 'r':
rts = 0;
break;
case '?':
return 1;
default:
break;
}
}

std::cout << "opening output " << out_port << std::endl;
int outfd = open ( out_port.c_str(), O_RDWR | O_NOCTTY | O_SYNC);
if (outfd < 0)
{
std::cerr << "Could not open " << out_port << std::endl;
return 1;
}

std::cout << "opening input " << in_port << std::endl;
int infd = open ( in_port.c_str(), O_RDWR | O_NOCTTY | O_SYNC);
if (infd < 0)
{
std::cerr << "Could not open " << in_port << std::endl;
return 1;
}

set_interface_attribs (infd, B1000000, 0);
set_interface_attribs (outfd, B1000000, 0);

enable_rts(outfd, rts);
enable_rts(infd, rts);

std::thread in(input_function, infd);
std::thread out(output_function, outfd);

in.join();
out.join();

close(infd);
close(outfd);
}

diff --git a/arch/arm/boot/dts/am335x-pocketbeagle.dts
b/arch/arm/boot/dts/am335x-pocketbeagle.dts
index 62fe5cab9fae..d9b8f09920a6 100644
--- a/arch/arm/boot/dts/am335x-pocketbeagle.dts
+++ b/arch/arm/boot/dts/am335x-pocketbeagle.dts
@@ -92,15 +92,6 @@
>;
};

- spi0_pins: pinmux-spi0-pins {
- pinctrl-single,pins = <
- AM33XX_IOPAD(0x950, PIN_INPUT_PULLUP |
MUX_MODE0) /* (A17) spi0_sclk.spi0_sclk */
- AM33XX_IOPAD(0x954, PIN_INPUT_PULLUP |
MUX_MODE0) /* (B17) spi0_d0.spi0_d0 */
- AM33XX_IOPAD(0x958, PIN_INPUT_PULLUP |
MUX_MODE0) /* (B16) spi0_d1.spi0_d1 */
- AM33XX_IOPAD(0x95c, PIN_INPUT_PULLUP |
MUX_MODE0) /* (A16) spi0_cs0.spi0_cs0 */
- >;
- };
-
spi1_pins: pinmux-spi1-pins {
pinctrl-single,pins = <
AM33XX_IOPAD(0x964, PIN_INPUT_PULLUP |
MUX_MODE4) /* (C18) eCAP0_in_PWM0_out.spi1_sclk */
@@ -126,6 +117,13 @@
>;
};

+ uart2_pins: pinmux-uart2-pins {
+ pinctrl-single,pins = <
+ AM33XX_IOPAD(0x950, PIN_INPUT | MUX_MODE1)
/* spi0_sclk.uart2_rxd */
+ AM33XX_IOPAD(0x954, PIN_OUTPUT | MUX_MODE1)
/* spi0_d0.uart2_txd */
+ >;
+ };
+
uart4_pins: pinmux-uart4-pins {
pinctrl-single,pins = <
AM33XX_IOPAD(0x870, PIN_INPUT_PULLUP |
MUX_MODE6) /* (T17) gpmc_wait0.uart4_rxd */
@@ -199,6 +197,13 @@
status = "okay";
};

+&uart2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins>;
+
+ status = "okay";
+};
+
&uart4 {
pinctrl-names = "default";
pinctrl-0 = <&uart4_pins>;

--
Best regards,
Marek Vasut

Next message: Stephen Rothwell: "Re: linux-next: build warning after merge of the tip tree"
Previous message: Jaegeuk Kim: "Re: [f2fs-dev] [PATCH v2] f2fs: add bio cache for IPU"
In reply to: Marek Vasut: "Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again""
Next in thread: Paul Burton: "Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again""
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]