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

[Marek Vasut <marex@denx.de>]

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@mips.com> 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@mips.com> 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