ARM : SAMSUNG : Add RS485 support.

PATCH In review Paul Schilling 4 participants 10 messages Oct 22, 2011 – Nov 4, 2011 1 message not in the archive

Subsystems (4)

Arm (32-bit) Arm sub-architectures Arm Samsung Exynos TTY layer and serial drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): samsung_rs485

Paul Schilling

author

2011-10-22 03:46 UTC #1 lore raw

PATCH 2/14 ARM : SAMSUNG : Add RS485 support.

Add RS485 tranmit/recieve control line capabilities This RS485 driver uses two methodes to determine if the transmit should be disabled.

1. It uses a timer and polling to determine when transmit has completed.
2. Optionally uses a token used to terminate the message to be used in the recieve interrupt to disable the transmit.

Option 1 alone can be used but doesn't garentee that the transmit line wont be disabled within 1 to 2 character times depending on system load.

This patch adds an additional variable to the serial RS-485 struct so that a program can change the terminate token value via the IOCTL.

Signed-off-by Paul Schilling <[email protected]>

---
 arch/arm/plat-samsung/include/plat/regs-serial.h |    7 +
 drivers/tty/serial/Kconfig                       |   16 +
 drivers/tty/serial/samsung.c                     |  738 ++++++++++++++++++++--
 drivers/tty/serial/samsung.h                     |   15 +
 include/linux/serial.h                           |    5 +-
 5 files changed, 722 insertions(+), 59 deletions(-)

arch/arm/plat-samsung/include/plat/regs-serial.h +7 -0

index bac36fa..9618e7d 100644
@@ -261,6 +261,13 @@ struct s3c2410_uartcfg {
 
 	struct s3c24xx_uart_clksrc *clocks;
 	unsigned int		    clocks_size;
+
+#ifdef CONFIG_SAMSUNG_HAS_RS485
+	signed int       gpio_transmit_en;
+	signed int       gpio_receive_en;
+	unsigned char	 enable_token;
+	unsigned char	 token;
+#endif
 };
 
 /* s3c24xx_uart_devs

drivers/tty/serial/Kconfig +16 -0

index 4dcb37b..dab33c2 100644
@@ -464,6 +464,22 @@ config SERIAL_SAMSUNG_UARTS
 	  Select the number of available UART ports for the Samsung S3C
 	  serial driver
 	
+config SAMSUNG_HAS_RS485
+	bool "Enable RS485 support for Samsung"
+	depends on SERIAL_SAMSUNG && (MACH_CONDOR2440 || MACH_CONDOR2416 || MACH_MINI2440)
+	default y if (MACH_CONDOR2440 || MACH_CONDOR2416)
+	default n if (MACH_MINI2440)
+
+config SAMSUNG_485_LOW_RES_TIMER
+    bool "Samsung RS-485 use low res timer during transmit"
+    depends on SERIAL_SAMSUNG && SAMSUNG_HAS_RS485
+    default n
+    help
+      Use low resolution jiffies at 200 Hz for timer the RS-485
+      transmitter.  This works but doesn't garantee hard realtime.
+      Say no if you need the transmitter off immediatly after sending
+      a string or character.
+
 config SERIAL_SAMSUNG_DEBUG
 	bool "Samsung SoC serial debug"
 	depends on SERIAL_SAMSUNG && DEBUG_LL

drivers/tty/serial/samsung.c +680 ~~-58~~

933 lines not on this page · raw message

drivers/tty/serial/samsung.h +15 -0

index a69d9a5..f550dc3 100644
@@ -4,6 +4,10 @@
  * Ben Dooks, Copyright (c) 2003-2008 Simtec Electronics
  *	http://armlinux.simtec.co.uk/
  *
+ * Paul Schilling, Copyright (c) 2011 Ecolab Corp.
+ * Added RS-485 support to the Samsung driver..
+ *
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.

@@ -31,6 +35,7 @@ struct s3c24xx_uart_info {
 
 	/* uart controls */
 	int (*reset_port)(struct uart_port *, struct s3c2410_uartcfg *);
+
 };
 
 struct s3c24xx_uart_port {

@@ -48,6 +53,16 @@ struct s3c24xx_uart_port {
 	struct clk			*baudclk;
 	struct uart_port		port;
 
+	struct serial_rs485	rs485;				/* RS-485 support */
+
+#ifdef CONFIG_SAMSUNG_485_LOW_RES_TIMER
+	unsigned long char_time_usec;			/* The time for 1 char, in micro secs */
+	struct timer_list rs485_tx_timer;
+#else
+	unsigned long char_time_nanosec;	/* The time for 1 char, in nano secs */
+	struct hrtimer  hr_rs485_tx_timer;		/* Timer for RS-485 Receive enable*/
+#endif
+
 #ifdef CONFIG_CPU_FREQ
 	struct notifier_block		freq_transition;
 #endif

include/linux/serial.h +4 -1

index ef91406..826b1c0 100644
@@ -211,9 +211,12 @@ struct serial_rs485 {
 #define SER_RS485_RTS_ON_SEND		(1 << 1)
 #define SER_RS485_RTS_AFTER_SEND	(1 << 2)
 #define SER_RS485_RTS_BEFORE_SEND	(1 << 3)
+#define SER_RS485_ALWAYS_LISTEN		(1 << 4)
+#define SER_RS485_TOGGLE_ON_TOKEN	(1 << 5)
 	__u32	delay_rts_before_send;	/* Milliseconds */
 	__u32	delay_rts_after_send;	/* Milliseconds */
-	__u32	padding[5];		/* Memory is cheap, new structs
+	__u32	toggle_token;			/* Token used to toggle to receive */
+	__u32	padding[4];		/* Memory is cheap, new structs
 					   are a royal PITA .. */
 };

Signature · 1 line

1.7.6.4

Paul Schilling

author

2011-10-23 17:12 UTC #3 lore raw

See Comments inline.

Quoted 13 lines · Russell King - ARM Linux

Review

Quoted 10 lines · Paul Schilling

(quoted diff) +10 -0

+config SAMSUNG_HAS_RS485
+ ? ? bool "Enable RS485 support for Samsung"
+ ? ? depends on SERIAL_SAMSUNG && (MACH_CONDOR2440 || MACH_CONDOR2416 || MACH_MINI2440)
+ ? ? default y if (MACH_CONDOR2440 || MACH_CONDOR2416)
+ ? ? default n if (MACH_MINI2440)
+
+config SAMSUNG_485_LOW_RES_TIMER
+ ? ?bool "Samsung RS-485 use low res timer during transmit"
+ ? ?depends on SERIAL_SAMSUNG && SAMSUNG_HAS_RS485
+ ? ?default n

n is the default, so this doesn't need to be specified.

Sorry I can fix that.

Quoted 23 lines

(quoted diff) +22 -0

+static void s3c24xx_serial_rx_fifo_enable(
+ ? ? ? ? ? ? struct uart_port *port,
+ ? ? ? ? ? ? unsigned int en)
+{
+ ? ? unsigned long flags;
+ ? ? unsigned int ucon;
+ ? ? static unsigned int last_state = 1;
+/* FIXME */
+ ? ? #if 0
+ ? ? if (last_state != en) {
+
+ ? ? ? ? ? ? struct s3c2410_uartcfg *cfg = s3c24xx_port_to_cfg(port);
+
+ ? ? ? ? ? ? spin_lock_irqsave(&port->lock, flags);
+
+ ? ? ? ? ? ? ucon = rd_regl(port, S3C2410_UCON);
+
+ ? ? ? ? ? ? ucon &= ~(S3C2440_UFCON_RXTRIG32 | S3C2410_UCON_RXILEVEL);
+
+ ? ? ? ? ? ? if (en) {
+ ? ? ? ? ? ? ? ? ? ? ucon |= cfg->ucon;
+ ? ? ? ? ? ? }

Quoted 8 lines

drivers/tty/serial/samsung.c static int s3c24xx_serial_txempty_nofifo(struct uart_port *port) · line 122
Quoted 6 lines
drivers/tty/serial/samsung.c +6 -0
+
+ ? ? ? ? ? ? wr_regl(port, S3C2410_UCON, ucon);
+
+ ? ? ? ? ? ? spin_unlock_irqrestore(&port->lock, flags);
+ ? ? }
+#endif
This looks like dead code.

It is broken dead code. Performance of the RS485 code could be increased if this code could be made to work.
right now I am forced to leave it in one character per interrupt so that each character that's received is checked
against the token. This code was supposed leave it in multi-byte FIFO mode until the token byte is sent then switch to receiving one byte at a time until the token is received.

Quoted 11 lines

Quoted 8 lines
+ ? ? ? ? ? ? } else {
+ ? ? ? ? ? ? ? ? ? ? /* Set a short timer to toggle RTS */
+ ? ? ? ? ? ? ? ? ? ? mod_timer(
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &(ourport->rs485_tx_timer),
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? jiffies + usecs_to_jiffies(
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ourport->char_time_usec
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? / 10));
This could do with being better formatted. ?Also, & doesn't need following parens.

when I ran checkpatch it complained that it exceeded 80 characters. I had trouble keeping this line
under 80 characters.

Quoted 5 lines

Quoted 3 lines
+ ? ? /* Read UART transmit status register */
+ ? ? utrstat = rd_regl(&(ourport->port), S3C2410_UTRSTAT);
Doesn't need the parens.

I can remove the extra parentheses.

Quoted 47 lines

drivers/tty/serial/samsung.c static void s3c24xx_serial_rx_disable(struct uart_port *port) · line 323
Quoted 13 lines
drivers/tty/serial/samsung.c +12 -0
+/* Callback array*/
+enum hrtimer_restart (*callback_list[CONFIG_SERIAL_SAMSUNG_UARTS])(struct hrtimer *) = {
+ ? ? ? ? ? ? &rs485_hr_timer_callback_uart0,
+ ? ? ? ? ? ? &rs485_hr_timer_callback_uart1,
+
+#if CONFIG_SERIAL_SAMSUNG_UARTS > 2
+ ? ? ? ? ? ? &rs485_hr_timer_callback_uart2,
+#endif
+
+#if CONFIG_SERIAL_SAMSUNG_UARTS > 3
+ ? ? ? ? ? ? &rs485_hr_timer_callback_uart3,
+#endif
Silly indentation - this doesn't need two tabs.
Quoted 11 lines
drivers/tty/serial/samsung.c +6 -0
+};
+
+#endif
+#endif /* CONFIG_SAMSUNG_HAS_RS485 */
+
+
?static void s3c24xx_serial_stop_tx(struct uart_port *port) ?{
? ? ? struct s3c24xx_uart_port *ourport = to_ourport(port);
? ? ? if (tx_enabled(port)) {
Review
Quoted 5 lines
(quoted diff) +5 -0
+#ifdef CONFIG_SAMSUNG_HAS_RS485
+ ? ? ? ? ? ? if (ourport->rs485.flags & SER_RS485_ENABLED) {
+#ifdef CONFIG_SAMSUNG_485_LOW_RES_TIMER
+ ? ? ? ? ? ? ? ? ? ? /* Set a short timer to toggle RTS */
+ ? ? ? ? ? ? ? ? ? ? mod_timer(&(ourport->rs485_tx_timer),
Doesn't need parens.
Review
Quoted 9 lines
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? jiffies + usecs_to_jiffies(ourport->char_time_usec * s3c24xx_serial_tx_getfifocnt(ourport)));
(quoted diff) +8 -0
+#else
+ ? ? ? ? ? ? ? ? ? ? ktime_t kt;
+
+ ? ? ? ? ? ? ? ? ? ? /* Set time struct to one char time. */
+ ? ? ? ? ? ? ? ? ? ? kt = ktime_set(0, ourport->char_time_nanosec);
+
+ ? ? ? ? ? ? ? ? ? ? /* Start the high res timer. */
+ ? ? ? ? ? ? ? ? ? ? hrtimer_start(&(ourport->hr_rs485_tx_timer), kt, HRTIMER_MODE_REL);
Doesn't need parens.

Quoted 15 lines

Quoted 7 lines
drivers/tty/serial/samsung.c +7 -0
+#endif /* CONFIG_SAMSUNG_485_LOW_RES_TIMER */
+
+ ? ? ? ? ? ? ? ? ? ? s3c24xx_serial_rx_fifo_enable(port, 0);
+
+ ? ? ? ? ? ? }
+#endif /* CONFIG_SAMSUNG_HAS_RS485 */
+
Quoted 6 lines
? ? ? ? ? ? ? disable_irq_nosync(ourport->tx_irq); ? ? ? ? ? ? ? tx_enabled(port) = 0;
- ? ? ? ? ? ? if (port->flags & UPF_CONS_FLOW)
+ ? ? ? ? ? ? if (port->flags & UPF_CONS_FLOW) { ? ? ? ? ? ? ? ? ? ? ? s3c24xx_serial_rx_enable(port);
+ ? ? ? ? ? ? }
Why are you reformatting code?

I will remove the reformatting of the code.

Quoted 47 lines

Review static void s3c24xx_serial_set_termios(struct uart_port *port, · line 1180
Quoted 7 lines
(quoted diff) +1 -1
@@ -785,7 +1177,7 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
 ? ? ? port->ignore_status_mask = 0;
 ? ? ? if (termios->c_iflag & IGNPAR)
 ? ? ? ? ? ? ? port->ignore_status_mask |= S3C2410_UERSTAT_OVERRUN;
- ? ? if (termios->c_iflag & IGNBRK && termios->c_iflag & IGNPAR)
+ ? ? if ((termios->c_iflag & IGNBRK) && (termios->c_iflag & IGNPAR))
More code reformatting.
Review static void s3c24xx_serial_config_port(struct uart_port *port, int flags) · line 1231
Quoted 6 lines
(quoted diff) +1 -1
@@ -830,7 +1228,7 @@ static void s3c24xx_serial_config_port(struct uart_port *port, int flags)
 ?{
 ? ? ? struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
 
- ? ? if (flags & UART_CONFIG_TYPE &&
+ ? ? if ((flags & UART_CONFIG_TYPE) &&
And some more.
Review static void s3c24xx_serial_config_port(struct uart_port *port, int flags) · line 1233
Quoted 2 lines
(quoted diff) +2 -0
+#if 0
+ ? ? dev_info(port., "rts: on send = %i, after = %i, enabled = %i",
That can't be correct - and as its #if 0'd out, either remove this or fix it to be correct (and use dev_dbg if you want it to be debugging.)
Review static void s3c24xx_serial_config_port(struct uart_port *port, int flags)
Quoted 11 lines
(quoted diff) +11 -0
+static ssize_t s3c24xx_serial_set_485_mode(struct device *dev,
+ ? ? ? ? ? ? struct device_attribute *attr,
+ ? ? ? ? ? ? const char *buf, size_t count)
+
+{
+ ? ? struct uart_port *port = s3c24xx_dev_to_port(dev);
+ ? ? struct s3c24xx_uart_port *ourport = to_ourport(port);
+
+ ? ? if (!strncmp(buf, "Enabled", 7)) {
+ ? ? ? ? ? ? ourport->rs485.flags |= SER_RS485_ENABLED;
+ ? ? } else if (!strncmp(buf, "Disabled", 8)) {
Do you really require the first character to be capitalized?
Review
Quoted 5 lines
(quoted diff) +5 -0
+#ifdef CONFIG_SAMSUNG_HAS_RS485
+
+ ? ? ret = device_create_file(&dev->dev, &dev_attr_485_status);
+ ? ? if (ret < 0)
+ ? ? ? ? ? ? printk(KERN_ERR "%s: failed to add 485 status attr.\n", __func__);
pr_err() ? ?dev_err() ?

Russell King - ARM Linux

2011-10-23 20:20 UTC #4 lore raw

Quoted 10 lines · Paul Schilling

Quoted 8 lines · Russell King - ARM Linux
Quoted 7 lines
+ ? ? ? ? ? ? } else {
+ ? ? ? ? ? ? ? ? ? ? /* Set a short timer to toggle RTS */
+ ? ? ? ? ? ? ? ? ? ? mod_timer(
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &(ourport->rs485_tx_timer),
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? jiffies + usecs_to_jiffies(
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ourport->char_time_usec
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? / 10));

Quoted 6 lines

Quoted 2 lines
This could do with being better formatted. ?Also, & doesn't need following parens.
when I ran checkpatch it complained that it exceeded 80 characters. I had trouble keeping this line
under 80 characters.

		} else {
			mod_timer(&our_port->rs485_tx_timer, jiffies +
				  usecs_to_jiffies(ourport->char_time_usec
						   / 10));

is probably a better way to format it.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
- ? ? ? ? ? ? if (port->flags & UPF_CONS_FLOW)
+ ? ? ? ? ? ? if (port->flags & UPF_CONS_FLOW) { ? ? ? ? ? ? ? ? ? ? ? s3c24xx_serial_rx_enable(port);
+ ? ? ? ? ? ? }
Why are you reformatting code?
I will remove the reformatting of the code.

If you wish to reformat the code to clean up checkpatch warnings, then that needs to be a separate patch from any other changes.

Alan Cox

2011-11-01 12:08 UTC #5 lore raw

In reply to Paul Schilling

drivers/tty/serial/Kconfig config SERIAL_SAMSUNG_UARTS · line 471

Quoted 6 lines

drivers/tty/serial/Kconfig +3 -0

+config SAMSUNG_HAS_RS485
+	bool "Enable RS485 support for Samsung"
+	depends on SERIAL_SAMSUNG && (MACH_CONDOR2440 ||

MACH_CONDOR2416 || MACH_MINI2440)

drivers/tty/serial/Kconfig +2 -0

+	default y if (MACH_CONDOR2440 || MACH_CONDOR2416)
+	default n if (MACH_MINI2440)

There really needs to be more ARM thinking about doing this sort of stuff at runtime. If you can detect the board type at runtime then you need to be moving towards kicking board specifics out of the depends, and treating it as an "include support for this, if the board has it" so you can move towards less build time only configuration.

drivers/tty/serial/samsung.c static void s3c24xx_serial_rx_disable(struct uart_port *port) · line 172

Quoted 4 lines

drivers/tty/serial/samsung.c +4 -0
+/* FIXME */
+	#if 0
+	if (last_state != en) {
+

So this doesn't look ready to submit either...

Review

Quoted 5 lines

drivers/tty/serial/samsung.c +3 -0
+		if ((utrstat & S3C2410_UTRSTAT_TXE) ? 1 : 0) {
+			if (cfg->gpio_transmit_en > -1) {
+
gpio_set_value(cfg->gpio_transmit_en, 0);
(quoted diff) +1 -0
+			}

Lots of excess brackets (see CodingStyle) - removing a few of the excess ones would make it easier to read later.

The later bits become a real mess of ifdefs - much not your fault, the combination of your ifdefs and existing lack of design push it beyond ugly and really want addressing.

Paul Schilling

author

2011-11-01 14:57 UTC #6 lore raw

I am working on resubmitting this stuff right now. I have 14 patches that should be
broken into more like 20 patches. Most of them are ARM S3C and specifically S3C2416 patches.
I am working on a patch right now to fix a Atomic wait in the DMA code. That causes
20 milliseconds of no interrupts whenever a sound is played on the samsung S3C platform.

The code below wasn't supposed to go out and I have patch that removes that chunk
of code almost ready that just needs testing before I send it.

Quoted 7 lines · Alan Cox

Quoted 6 lines
+config SAMSUNG_HAS_RS485
+ ? ? bool "Enable RS485 support for Samsung"
+ ? ? depends on SERIAL_SAMSUNG && (MACH_CONDOR2440 || MACH_CONDOR2416 || MACH_MINI2440)
+ ? ? default y if (MACH_CONDOR2440 || MACH_CONDOR2416)
+ ? ? default n if (MACH_MINI2440)

Quoted 6 lines

There really needs to be more ARM thinking about doing this sort of stuff at runtime. If you can detect the board type at runtime then you need to be moving towards kicking board specifics out of the depends, and treating it as an "include support for this, if the board has it" so you can move towards less build time only configuration.

The chunk of code that says FIXME is an optimization that could be implemented to
decrease interrupts by using DMA until the last byte is ready to be sent. I have a new
patch that explains in a comment what the code is for.

Quoted 7 lines

drivers/tty/serial/samsung.c static void s3c24xx_serial_rx_disable(struct uart_port *port) · line 172
Quoted 4 lines
drivers/tty/serial/samsung.c +4 -0
+/* FIXME */
+ ? ? #if 0
+ ? ? if (last_state != en) {
+
So this doesn't look ready to submit either...

I have a patch that I haven't sent yet that fixes the code formating issues. I need to compile and test it before I send it out.

Quoted 15 lines

Quoted 6 lines
+ ? ? ? ? ? ? if ((utrstat & S3C2410_UTRSTAT_TXE) ? 1 : 0) {
+ ? ? ? ? ? ? ? ? ? ? if (cfg->gpio_transmit_en > -1) { +
gpio_set_value(cfg->gpio_transmit_en, 0);
+ ? ? ? ? ? ? ? ? ? ? }
Lots of excess brackets (see CodingStyle) - removing a few of the excess ones would make it easier to read later.
The later bits become a real mess of ifdefs - much not your fault, the combination of your ifdefs and existing lack of design push it beyond ugly and really want addressing.

Paul Schilling

author

2011-11-01 19:12 UTC #7 lore raw

I need an opinion on two issues before I resubmit the Samsung RS-485 driver.

First of all the driver works. I have tested it on a logic analyser and I found that it
switches from transmit to receive in token mode exactly after 1 bit of idle time as long
as interrupts are kept enabled and nothing tries to wait or sleep inside of an interrupt.
I found 2 maybe 3 places where this occurs up to 22 milliseconds.

The opinion part... I needed a timer to switch from transmit to receive after the FIFO
was empty. I started by using the low resolution timer using jiffies first. I found that
wasn't high enough resolution, so I switched to the Linux HRT. Currently I have both
versions that can be selected by conditional compile. Should I just remove the low
resolution timer completely or leave it in.

Second, I have a chunk of code that if it could be made to work could off load the
receiving to DMA up to the last couple of bytes then switch back to interrupts for
the token byte. Should I leave that code in a #if 0 statement or should I just delete
it.

Thanks,
Paul Schilling

Quoted 61 lines · Paul Schilling

I am working on resubmitting this stuff right now. ?I have 14 patches that should be
broken into more like 20 patches. ?Most of them are ARM S3C and specifically S3C2416 patches.
I am working on a patch right now to fix a Atomic wait in the DMA code. ?That causes
20 milliseconds of no interrupts whenever a sound is played on the samsung S3C platform.
The code below wasn't supposed to go out and I have patch that removes that chunk
of code almost ready that just needs testing before I send it.
Quoted 7 lines · Alan Cox
Quoted 6 lines
+config SAMSUNG_HAS_RS485
+ ? ? bool "Enable RS485 support for Samsung"
+ ? ? depends on SERIAL_SAMSUNG && (MACH_CONDOR2440 || MACH_CONDOR2416 || MACH_MINI2440)
+ ? ? default y if (MACH_CONDOR2440 || MACH_CONDOR2416)
+ ? ? default n if (MACH_MINI2440)
Quoted 6 lines
There really needs to be more ARM thinking about doing this sort of stuff at runtime. If you can detect the board type at runtime then you need to be moving towards kicking board specifics out of the depends, and treating it as an "include support for this, if the board has it" so you can move towards less build time only configuration.
The chunk of code that says FIXME is an optimization that could be implemented to
decrease interrupts by using DMA until the last byte is ready to be sent. ?I have a new
patch that explains in a comment what the code is for.
Quoted 7 lines
drivers/tty/serial/samsung.c static void s3c24xx_serial_rx_disable(struct uart_port *port) · line 172
Quoted 4 lines
drivers/tty/serial/samsung.c +4 -0
+/* FIXME */
+ ? ? #if 0
+ ? ? if (last_state != en) {
+
So this doesn't look ready to submit either...
I have a patch that I haven't sent yet that fixes the code formating issues. ?I need to compile and test it before I send it out.
Quoted 15 lines
Quoted 6 lines
+ ? ? ? ? ? ? if ((utrstat & S3C2410_UTRSTAT_TXE) ? 1 : 0) {
+ ? ? ? ? ? ? ? ? ? ? if (cfg->gpio_transmit_en > -1) { +
gpio_set_value(cfg->gpio_transmit_en, 0);
+ ? ? ? ? ? ? ? ? ? ? }
Lots of excess brackets (see CodingStyle) - removing a few of the excess ones would make it easier to read later.
The later bits become a real mess of ifdefs - much not your fault, the combination of your ifdefs and existing lack of design push it beyond ugly and really want addressing.

Paul Schilling

author

2011-11-01 19:22 UTC #8 lore raw

Sorry for the formatting mess on the previous email. Lets try this again.

I need an opinion on two issues before I resubmit the Samsung RS-485 driver.

First of all the driver works. ?I have tested it on a logic analyser and I found that it switches from transmit to receive in token mode exactly after 1 bit of idle time as long as interrupts are kept enabled and nothing tries to wait or sleep inside of an interrupt. I found 2 maybe 3 places where this occurs up to 22 milliseconds.

The opinion part... ?I needed a timer to switch from transmit to receive after the FIFO was empty. ?I started by using the low resolution timer using jiffies first. ?I found that wasn't high enough resolution, so I switched to the Linux HRT. Currently I have both versions that can be selected by conditional compile. ?Should I just remove the low resolution timer completely or leave it in.

Second, I have a chunk of code that if it could be made to work could off load the receiving to DMA up to the last couple of bytes then switch back to interrupts for the token byte. ?Should I leave that code in a #if 0 statement or should I just delete it.

Thanks,
Paul Schilling

Quoted 64 lines

Quoted 61 lines · Paul Schilling
I am working on resubmitting this stuff right now. ?I have 14 patches that should be
broken into more like 20 patches. ?Most of them are ARM S3C and specifically S3C2416 patches.
I am working on a patch right now to fix a Atomic wait in the DMA code. ?That causes
20 milliseconds of no interrupts whenever a sound is played on the samsung S3C platform.
The code below wasn't supposed to go out and I have patch that removes that chunk
of code almost ready that just needs testing before I send it.
Quoted 7 lines · Alan Cox
Quoted 6 lines
+config SAMSUNG_HAS_RS485
+ ? ? bool "Enable RS485 support for Samsung"
+ ? ? depends on SERIAL_SAMSUNG && (MACH_CONDOR2440 || MACH_CONDOR2416 || MACH_MINI2440)
+ ? ? default y if (MACH_CONDOR2440 || MACH_CONDOR2416)
+ ? ? default n if (MACH_MINI2440)
Quoted 6 lines
There really needs to be more ARM thinking about doing this sort of stuff at runtime. If you can detect the board type at runtime then you need to be moving towards kicking board specifics out of the depends, and treating it as an "include support for this, if the board has it" so you can move towards less build time only configuration.
The chunk of code that says FIXME is an optimization that could be implemented to
decrease interrupts by using DMA until the last byte is ready to be sent. ?I have a new
patch that explains in a comment what the code is for.
Quoted 7 lines
drivers/tty/serial/samsung.c static void s3c24xx_serial_rx_disable(struct uart_port *port) · line 172
Quoted 4 lines
drivers/tty/serial/samsung.c +4 -0
+/* FIXME */
+ ? ? #if 0
+ ? ? if (last_state != en) {
+
So this doesn't look ready to submit either...
I have a patch that I haven't sent yet that fixes the code formating issues. ?I need to compile and test it before I send it out.
Quoted 15 lines
Quoted 6 lines
+ ? ? ? ? ? ? if ((utrstat & S3C2410_UTRSTAT_TXE) ? 1 : 0) {
+ ? ? ? ? ? ? ? ? ? ? if (cfg->gpio_transmit_en > -1) { +
gpio_set_value(cfg->gpio_transmit_en, 0);
+ ? ? ? ? ? ? ? ? ? ? }
Lots of excess brackets (see CodingStyle) - removing a few of the excess ones would make it easier to read later.
The later bits become a real mess of ifdefs - much not your fault, the combination of your ifdefs and existing lack of design push it beyond ugly and really want addressing.

Alan Cox

2011-11-01 19:44 UTC #9 lore raw

Quoted 6 lines

The opinion part... I needed a timer to switch from transmit to receive after the FIFO was empty. I started by using the low resolution timer using jiffies first. I found that wasn't high enough resolution, so I switched to the Linux HRT. Currently I have both versions that can be selected by conditional compile. Should I just remove the low resolution timer completely or leave it in.

I would just remove the low res one. They should degrade to low res timer equivalence anyway.

Quoted 4 lines

Second, I have a chunk of code that if it could be made to work could off load the receiving to DMA up to the last couple of bytes then switch back to interrupts for the token byte. Should I leave that code in a #if 0 statement or should I just delete it.

Is it something that you are likely to debug or someone is going to debug shortly ?
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Nicolas Ferre

2011-11-04 10:18 UTC #10 lore raw

In reply to Paul Schilling

On 10/22/2011 05:46 AM, Paul Schilling :

Quoted 3 lines

Add RS485 tranmit/recieve control line capabilities This RS485 driver uses two methodes to determine if the transmit should be disabled.

[..]

Quoted 2 lines

This patch adds an additional variable to the serial RS-485 struct so that a program can change the terminate token value via the IOCTL.

[..]

include/linux/serial.h struct serial_rs485 { · line 214

Quoted 9 lines

include/linux/serial.h +1 -0

index ef91406..826b1c0 100644
@@ -211,9 +211,12 @@ struct serial_rs485 {
 #define SER_RS485_RTS_ON_SEND		(1 << 1)
 #define SER_RS485_RTS_AFTER_SEND	(1 << 2)
 #define SER_RS485_RTS_BEFORE_SEND	(1 << 3)
+#define SER_RS485_ALWAYS_LISTEN		(1 << 4)

This value is already in another patch (now in Linus' tree).

include/linux/serial.h struct serial_rs485 { · line 218

Quoted 5 lines

include/linux/serial.h +2 -1

+#define SER_RS485_TOGGLE_ON_TOKEN	(1 << 5)
 	__u32	delay_rts_before_send;	/* Milliseconds */
 	__u32	delay_rts_after_send;	/* Milliseconds */
-	__u32	padding[5];		/* Memory is cheap, new structs
+	__u32	toggle_token;			/* Token used to toggle to receive */

Do not forget to add this new value in the RS485 device tree bindings
[1] that appeared during this merge window. Once converted to device tree, your platform may need to retrieve this value from it.

[1]: Documentation/devicetree/bindings/serial/rs485.txt

Best regards,

Signature · 1 line

Nicolas Ferre