RFC: WIP: sc16is7xx [v0.4]

From: Jon Ringle <[email protected]>

I started over and rewrote this driver patternized on sccnxp.c

However, I am still experiencing major latency problems with this driver at 19200 speeds.

The method that I'm testing is simply transferring a small file just over 4k in size.

On the target platform I do:

$ socat /dev/ttySC0,raw,echo=0,b19200 - > rx-file

On my development machine, I do:

$ socat /dev/ttyUSB1,echo=0,raw,time=1,min=255,b19200 FILE:./tx-file

When the socat running on the development machine returns to the prompt, it has transmitted all the bytes in tx-file. I then kill the socat running on the target platform. Success is defined as rx-file being identical to tx-file. However, I find that even at only 19200, this driver fails to receive all bytes sent.

I welcome any and all comments.

Thank you,
Jon

---
 drivers/tty/serial/Kconfig       |   7 +
 drivers/tty/serial/Makefile      |   1 +
 drivers/tty/serial/sc16is7xx.c   | 730 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |   3 +
 4 files changed, 741 insertions(+)
 create mode 100644 drivers/tty/serial/sc16is7xx.c

index febd45c..1dfaeec 100644
@@ -1179,6 +1179,13 @@ config SERIAL_SCCNXP_CONSOLE
 	help
 	  Support for console on SCCNXP serial ports.
 
+config SERIAL_SC16IS7XX
+	tristate "SC16IS7xx RS485 serial support"
+	  select SERIAL_CORE
+	  default n
+	  help
+	  This selects support for SC16IS7xx for use as a RS485 serial port
+
 config SERIAL_BFIN_SPORT
 	tristate "Blackfin SPORT emulate UART"
 	depends on BLACKFIN

index 3068c77..c3bac45 100644
@@ -49,6 +49,7 @@ obj-$(CONFIG_SERIAL_SB1250_DUART) += sb1250-duart.o
 obj-$(CONFIG_ETRAX_SERIAL) += crisv10.o
 obj-$(CONFIG_SERIAL_SC26XX) += sc26xx.o
 obj-$(CONFIG_SERIAL_SCCNXP) += sccnxp.o
+obj-$(CONFIG_SERIAL_SC16IS7XX) += sc16is7xx.o
 obj-$(CONFIG_SERIAL_JSM) += jsm/
 obj-$(CONFIG_SERIAL_TXX9) += serial_txx9.o
 obj-$(CONFIG_SERIAL_VR41XX) += vr41xx_siu.o

index b47dba2..a37c79a 100644
@@ -238,4 +238,7 @@
 /* Tilera TILE-Gx UART */
 #define PORT_TILEGX	106
 
+/* SC16IS74xx */
+#define PORT_SC16IS7XX   107
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */

1.8.5.4

--
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

Понедельник, 10 марта 2014, 2:26 -04:00 от [email protected]:

...

You're right. This is just how we are using the chip in our board. Would a configuration flag in platform_data make sense to use the auto rs485 direction?

Any thoughts as to the latency issue I report?

Thanks.

Regards,
Jon

We have interfaces for RS485 runtime configuration so it could be extended to set a lot of parameters that way, but I think for now it might not be a bad idea to support rs485 auto direction in/out/off in the platform data.

Alan
--
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

Понедельник, 10 марта 2014, 7:50 -04:00 от Jon Ringle <[email protected]>:

You don't need a flag, just use IOCTLs.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/tty/serial/max310x.c?id=55367c620aed6bc27a82bb1763366931d7f2ee66

I do not understand why you chose the sccnxp driver as a template. Use as a template max310x driver from linux-next branch. I'm sure it will be better.

About latency.
I'm not exactly understand the problem, but I can say the following:
Since you're using the I2C bus, maximum UART daudrate can be predicted from the I2C bus bandwidth.
Count how many I2C clocks spent on reception and transmission of one byte through UART.

---

I followed your advice on using max310x as a template. One thing that I'm struggling to get right is the regmap. The datasheet[1] specifies the register bits as follows:

	Table 33. Register address byte (I2C)
	Bit	Name	Function
	7 	-	not used
	6:3 	A[3:0]	UART's internal register select
	2:1	CH1,0	channel select: CH1 = 0, CH0 = 0
			Other values are reserved and should not be used.
	0	-	not used

So, I need to shift the register address by 3. Here is how I have defined the regmap stuff:

------>8 [relevant code] >8-------
/* SC16IS7XX register definitions */
#define SC16IS7XX_RHR_REG (0x00) /* RX FIFO */
#define SC16IS7XX_THR_REG (0x00) /* TX FIFO */
#define SC16IS7XX_IER_REG (0x01) /* Interrupt enable */
#define SC16IS7XX_IIR_REG (0x02) /* Interrupt Ident */
#define SC16IS7XX_FCR_REG (0x02) /* FIFO control */
#define SC16IS7XX_LCR_REG (0x03) /* Line Control */
#define SC16IS7XX_MCR_REG (0x04) /* Modem Control */
#define SC16IS7XX_LSR_REG (0x05) /* Line Status */
#define SC16IS7XX_MSR_REG (0x06) /* Modem Status */
#define SC16IS7XX_SPR_REG (0x07) /* Scratch Pad */
#define SC16IS7XX_TXLVL_REG (0x08) /* TX FIFO level */
#define SC16IS7XX_RXLVL_REG (0x09) /* RX FIFO level */
#define SC16IS7XX_IODIR_REG (0x0a) /* I/O Dir (750/760) */
#define SC16IS7XX_IOSTATE_REG (0x0b) /* I/O State (750/760) */
#define SC16IS7XX_IOINTENA_REG (0x0c) /* I/O Int Enable (750/760) */
#define SC16IS7XX_IOCONTROL_REG (0x0e) /* I/O Control (750/760) */
#define SC16IS7XX_EFCR_REG (0x0f) /* Extra Feature Control */

/* TX/Trigger Registers: Only if ((MCR[2] == 0) && (EFR[4] == 1)) */
#define SC16IS7XX_TCR_REG (0x06) /* Transmit control */
#define SC16IS7XX_TLR_REG (0x07) /* Trigger level */

/* Special Register set: Only if ((LCR[7] == 1) && (LCR != 0xBF)) */
#define SC16IS7XX_DLL_REG (0x00) /* Divisor Latch Low */
#define SC16IS7XX_DLH_REG (0x01) /* Divisor Latch High */

/* Enhanced Register set: Only if (LCR == 0xBF) */
#define SC16IS7XX_EFR_REG (0x02) /* Enhanced Features */
#define SC16IS7XX_XON1_REG (0x04) /* Xon1 word */
#define SC16IS7XX_XON2_REG (0x05) /* Xon2 word */
#define SC16IS7XX_XOFF1_REG (0x06) /* Xoff1 word */
#define SC16IS7XX_XOFF2_REG (0x07) /* Xoff2 word */

#define SC16IS7XX_REG_SHIFT 3

static bool sc16is7xx_reg_writeable(struct device *dev, unsigned int reg)

{
	switch ((reg >> SC16IS7XX_REG_SHIFT) & 0xf) {
	case SC16IS7XX_LSR_REG:
	case SC16IS7XX_MSR_REG:
	case SC16IS7XX_TXLVL_REG:
	case SC16IS7XX_RXLVL_REG:
		return false;
	default:
		break;
	}

	return true;
}

static bool sc16is7xx_reg_volatile(struct device *dev, unsigned int reg)

{
	switch ((reg >> SC16IS7XX_REG_SHIFT) & 0xf) {
	case SC16IS7XX_RHR_REG:
	case SC16IS7XX_IIR_REG:
	case SC16IS7XX_LSR_REG:
	case SC16IS7XX_MSR_REG:
	case SC16IS7XX_TXLVL_REG:
	case SC16IS7XX_RXLVL_REG:
	case SC16IS7XX_IOSTATE_REG:
		return true;
	default:
		break;
	}

	return false;
}

static bool sc16is7xx_reg_precious(struct device *dev, unsigned int reg)

{
	switch ((reg >> SC16IS7XX_REG_SHIFT) & 0xf) {
	case SC16IS7XX_RHR_REG:
	case SC16IS7XX_IIR_REG:
	case SC16IS7XX_MSR_REG:
		return true;
	default:
		break;
	}

	return false;
}

static struct regmap_config regcfg = {
	.reg_bits = 8,
	.reg_stride = (1 << SC16IS7XX_REG_SHIFT),
	.val_bits = 8,
	.max_register = (0xf << SC16IS7XX_REG_SHIFT),
	.cache_type = REGCACHE_RBTREE,
	.writeable_reg = sc16is7xx_reg_writeable,
	.volatile_reg = sc16is7xx_reg_volatile,
	.precious_reg = sc16is7xx_reg_precious,
};

------------->8-----------------

I'm not sure that I have this defined correctly. I also saw in regmap_config pad_bits, which I've tried setting to SC16IS7XX_REG_SHIFT, but when I do that, I get -EINVAL failure.

Thanks,
Jon

[1] http://www.nxp.com/documents/data_sheet/SC16IS740_750_760.pdf

I left out some relevant functions (sc16is7xx_port_read(), sc16is7xx_port_write(), sc16is7xx_port_update()) in my previous email...

I followed your advice on using max310x as a template. One thing that I'm struggling to get right is the regmap. The datasheet[1] specifies the register bits as follows:

	Table 33. Register address byte (I2C)
	Bit	Name	Function
	7 	-	not used
	6:3 	A[3:0]	UART's internal register select
	2:1	CH1,0	channel select: CH1 = 0, CH0 = 0
			Other values are reserved and should not be used.
	0	-	not used

So, I need to shift the register address by 3. Here is how I have defined the regmap stuff:

------>8 [relevant code] >8-------
/* SC16IS7XX register definitions */
#define SC16IS7XX_RHR_REG (0x00) /* RX FIFO */
#define SC16IS7XX_THR_REG (0x00) /* TX FIFO */
#define SC16IS7XX_IER_REG (0x01) /* Interrupt enable */
#define SC16IS7XX_IIR_REG (0x02) /* Interrupt Ident */
#define SC16IS7XX_FCR_REG (0x02) /* FIFO control */
#define SC16IS7XX_LCR_REG (0x03) /* Line Control */
#define SC16IS7XX_MCR_REG (0x04) /* Modem Control */
#define SC16IS7XX_LSR_REG (0x05) /* Line Status */
#define SC16IS7XX_MSR_REG (0x06) /* Modem Status */
#define SC16IS7XX_SPR_REG (0x07) /* Scratch Pad */
#define SC16IS7XX_TXLVL_REG (0x08) /* TX FIFO level */
#define SC16IS7XX_RXLVL_REG (0x09) /* RX FIFO level */
#define SC16IS7XX_IODIR_REG (0x0a) /* I/O Dir (750/760) */
#define SC16IS7XX_IOSTATE_REG (0x0b) /* I/O State (750/760) */
#define SC16IS7XX_IOINTENA_REG (0x0c) /* I/O Int Enable (750/760) */
#define SC16IS7XX_IOCONTROL_REG (0x0e) /* I/O Control (750/760) */
#define SC16IS7XX_EFCR_REG (0x0f) /* Extra Feature Control */

/* TX/Trigger Registers: Only if ((MCR[2] == 0) && (EFR[4] == 1)) */
#define SC16IS7XX_TCR_REG (0x06) /* Transmit control */
#define SC16IS7XX_TLR_REG (0x07) /* Trigger level */

/* Special Register set: Only if ((LCR[7] == 1) && (LCR != 0xBF)) */
#define SC16IS7XX_DLL_REG (0x00) /* Divisor Latch Low */
#define SC16IS7XX_DLH_REG (0x01) /* Divisor Latch High */

/* Enhanced Register set: Only if (LCR == 0xBF) */
#define SC16IS7XX_EFR_REG (0x02) /* Enhanced Features */
#define SC16IS7XX_XON1_REG (0x04) /* Xon1 word */
#define SC16IS7XX_XON2_REG (0x05) /* Xon2 word */
#define SC16IS7XX_XOFF1_REG (0x06) /* Xoff1 word */
#define SC16IS7XX_XOFF2_REG (0x07) /* Xoff2 word */

#define SC16IS7XX_REG_SHIFT 3

static u8 sc16is7xx_port_read(struct uart_port *port, u8 reg)

{
	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
	unsigned int val = 0;

	regmap_read(s->regmap, reg << SC16IS7XX_REG_SHIFT, &val);

	return val;
}

static void sc16is7xx_port_write(struct uart_port *port, u8 reg, u8 val)

{
	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);

	regmap_write(s->regmap, reg << SC16IS7XX_REG_SHIFT, val);
}

static void sc16is7xx_port_update(struct uart_port *port, u8 reg, u8 mask, u8 val)

{
	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);

	regmap_update_bits(s->regmap, reg << SC16IS7XX_REG_SHIFT, mask, val);
}

static bool sc16is7xx_reg_writeable(struct device *dev, unsigned int reg)

{
	switch ((reg >> SC16IS7XX_REG_SHIFT) & 0xf) {
	case SC16IS7XX_LSR_REG:
	case SC16IS7XX_MSR_REG:
	case SC16IS7XX_TXLVL_REG:
	case SC16IS7XX_RXLVL_REG:
		return false;
	default:
		break;
	}

	return true;
}

static bool sc16is7xx_reg_volatile(struct device *dev, unsigned int reg)

{
	switch ((reg >> SC16IS7XX_REG_SHIFT) & 0xf) {
	case SC16IS7XX_RHR_REG:
	case SC16IS7XX_IIR_REG:
	case SC16IS7XX_LSR_REG:
	case SC16IS7XX_MSR_REG:
	case SC16IS7XX_TXLVL_REG:
	case SC16IS7XX_RXLVL_REG:
	case SC16IS7XX_IOSTATE_REG:
		return true;
	default:
		break;
	}

	return false;
}

static bool sc16is7xx_reg_precious(struct device *dev, unsigned int reg)

{
	switch ((reg >> SC16IS7XX_REG_SHIFT) & 0xf) {
	case SC16IS7XX_RHR_REG:
	case SC16IS7XX_IIR_REG:
	case SC16IS7XX_MSR_REG:
		return true;
	default:
		break;
	}

	return false;
}

static struct regmap_config regcfg = {
	.reg_bits = 8,
	.reg_stride = (1 << SC16IS7XX_REG_SHIFT),
	.val_bits = 8,
	.max_register = (0xf << SC16IS7XX_REG_SHIFT),
	.cache_type = REGCACHE_RBTREE,
	.writeable_reg = sc16is7xx_reg_writeable,
	.volatile_reg = sc16is7xx_reg_volatile,
	.precious_reg = sc16is7xx_reg_precious,
};

------------->8-----------------

I'm not sure that I have this defined correctly. I also saw in regmap_config pad_bits, which I've tried setting to SC16IS7XX_REG_SHIFT, but when I do that, I get -EINVAL failure.

Thanks,
Jon

[1] http://www.nxp.com/documents/data_sheet/SC16IS740_750_760.pdf

Вторник, 11 марта 2014, 14:44 -04:00 от Jon Ringle <[email protected]>:

Here are the approximate changes:

#define SC16IS7XX_REG_SHIFT 2

regmap_read(s->regmap, (reg << SC16IS7XX_REG_SHIFT) | port->index, &val);

Same for "write" and "update".

.reg_bits = 7,

Kill this line (.reg_stride), but add:
.pad_bits = 1,

---

I wouldn't inline this - the compiler will figure it out for you and you've got a fair bit code there.

This gets called from handle_events which gets called from the ist handler with IRQs disabled. I'm surprised it works at all but you definitely don't want to be doing all this work with interrupts blocked. That may be part of your problem.

Ditto

		termios->c_cflag &= ~CSIZE;
		termios->c_cflag |= CS8;