[PATCH 0/2] Add synchronous FIFO and CBUS support for FT232H

[PATCH 0/2] Add synchronous FIFO and CBUS support for FT232H

[Philipp Hachtmann]

Hi folks,

I am currently developing hardware around the FTDI FT232H chip. The chip
can be programmed into asynchronous FIFO mode by EEPROM settings. Using
it with ftdi_sio in asynchronous mode works out of the box.

1. Synchronous FIFO mode
========================

The even more wonderful synchronous FIFO mode has to be enabled in software.
I first tried to use libftdi which worked in *some way*. But I think having
support for the synchronous FIFO mode in the ftdi_sio driver would be very
nice. So I took some inspiration from libftdi and prepared a patch which
adds the support to the driver. Only for FT232H. The patch adds a sysfs 
attribute "syncmode" which can be set and reset by the user.
Currently only exactly the FT232H is supported. But I know that there are
more FTDI chips (which ones? how?) which also support the sync FIFO mode.

The patch won't change any behaviour unless

a) you have an FT232H connected

AND 

b) you use the syncmode attribute.


2. CBUS bitbang support
=======================

The FT232H also supports software control of four CBUS lines.
In my application I use two of them for signalling purposes. The CBUS
patch adds a sysfs attribute which allows to set the CBUS mask and pins
like libftdi does it.
The attribute is read/write but the read function is currently just a
stub which returns -EIO.

3. The buffer thing
===================

With the sync mode patch I get transfer rates about 5 to 8 MB/sec in each
direction and simultaneously. That's not that much.
FTDI says (in an application note) that one should increase send and receive
buffer sizes.
I don't know too much about USB and even less about the usb-serial stuff.

Then I made this experiment:

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 3eabacf..de57931 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1002,8 +1002,10 @@ static struct usb_serial_driver ftdi_sio_device = {
        .description =          "FTDI USB Serial Device",
        .id_table =             id_table_combined,
        .num_ports =            1,
-       .bulk_in_size =         512,
-       .bulk_out_size =        256,
+       //      .bulk_in_size =         512,
+       //      .bulk_out_size =        256,
+       .bulk_in_size =         48*1024,
+       .bulk_out_size =        48*1024,
        .probe =                ftdi_sio_probe,
        .port_probe =           ftdi_sio_port_probe,
        .port_remove =          ftdi_sio_port_remove,
@@ -1326,0 +1328,0 @@ static int change_speed(struct tty_struct *tty, struct usb_serial_port *port)

With that rude hack I get transfer rates of at least 15 MB/s into each
direction simultaneously and approx. 30MB/sec in one direction only.

The usb_serial_driver structure is a static global thing for all FTDI chips.
I *know* that the above diff is absolutely impossible to apply. It was just
an experiment.

I just don't know how to get this third thing right. So I would appreciate any
support getting this right.

Kind regards

Philipp


Philipp Hachtmann (2):
  usb/ftdi_sio: Add synchronous FIFO mode support for FT232H
  usb/ftdi_sio: Add support for setting CBUS pins on FT232H

 drivers/usb/serial/ftdi_sio.c | 129 +++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/serial/ftdi_sio.h |  16 ++++++
 2 files changed, 143 insertions(+), 2 deletions(-)

-- 
2.0.0.rc2

[PATCH 1/2] usb/ftdi_sio: Add synchronous FIFO mode support for FT232H

[Philipp Hachtmann]

This patch adds support for the synchronous FIFO mode of the FT232H
serial converter chip.
This might also be extended to be usable with other FTDI chips.

Signed-off-by: Philipp Hachtmann <hachti@hachti.de>
---
 drivers/usb/serial/ftdi_sio.c | 65 +++++++++++++++++++++++++++++++++++++++++--
 drivers/usb/serial/ftdi_sio.h | 15 ++++++++++
 2 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 7c6e1de..cacba4a 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -73,7 +73,7 @@ struct ftdi_private {
 				   this value */
 	int force_rtscts;	/* if non-zero, force RTS-CTS to always
 				   be enabled */
-
+	int syncmode;           /* FIFO device in synchronous 245 mode */
 	unsigned int latency;		/* latency setting in use */
 	unsigned short max_packet_size;
 	struct mutex cfg_lock; /* Avoid mess by parallel calls of config ioctl() and change_speed() */
@@ -1327,6 +1327,36 @@ static int change_speed(struct tty_struct *tty, struct usb_serial_port *port)
 	return rv;
 }
 
+static int set_syncmode(struct usb_serial_port *port, int enable)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+
+	__u16 urb_value = 0;
+	int rv = 0;
+
+	enable = enable ? 1 : 0;
+	if (enable == priv->syncmode)
+		return 0;
+
+	priv->syncmode = enable;
+
+	/* FTDI seems to say that the urb_value should be or'ed with 0xff. But
+	 * when done this way the port gets quite slow. 0x00 seems to work much
+	 * better.
+	 */
+	if (enable)
+		urb_value = FTDI_BITMODE_SYNCFIFO << 8 | 0x00;
+
+	rv = usb_control_msg(port->serial->dev,
+			     usb_sndctrlpipe(port->serial->dev, 0),
+			     FTDI_SIO_SET_BITBANG_REQUEST,
+			     FTDI_SIO_SET_BITBANG_REQUEST_TYPE,
+			     urb_value, priv->interface,
+			     NULL, 0, WDR_SHORT_TIMEOUT);
+
+	return rv;
+}
+
 static int write_latency_timer(struct usb_serial_port *port)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
@@ -1628,6 +1658,32 @@ static ssize_t latency_timer_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(latency_timer);
 
+static ssize_t syncmode_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct usb_serial_port *port = to_usb_serial_port(dev);
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	return sprintf(buf, "%i\n", priv->syncmode);
+}
+
+static ssize_t syncmode_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *valbuf, size_t count)
+{
+	unsigned long value;
+	int rv;
+	struct usb_serial_port *port = to_usb_serial_port(dev);
+	int ret = kstrtoul(valbuf, 0, &value);
+	if (ret)
+		return -EINVAL;
+
+	rv = set_syncmode(port, value);
+	if (rv < 0)
+		return -EIO;
+	return count;
+}
+static DEVICE_ATTR_RW(syncmode);
+
 /* Write an event character directly to the FTDI register.  The ASCII
    value is in the low 8 bits, with the enable bit in the 9th bit. */
 static ssize_t store_event_char(struct device *dev,
@@ -1678,6 +1734,10 @@ static int create_sysfs_attrs(struct usb_serial_port *port)
 						    &dev_attr_latency_timer);
 		}
 	}
+	if ((!retval) && priv->chip_type == FT232H) {
+		retval = device_create_file(&port->dev,
+					    &dev_attr_syncmode);
+	}
 	return retval;
 }
 
@@ -1698,7 +1758,8 @@ static void remove_sysfs_attrs(struct usb_serial_port *port)
 			device_remove_file(&port->dev, &dev_attr_latency_timer);
 		}
 	}
-
+	if (priv->chip_type == FT232H)
+		device_remove_file(&port->dev, &dev_attr_syncmode);
 }
 
 /*
diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h
index ed58c6f..04a29f8 100644
--- a/drivers/usb/serial/ftdi_sio.h
+++ b/drivers/usb/serial/ftdi_sio.h
@@ -35,6 +35,7 @@
 #define FTDI_SIO_SET_ERROR_CHAR		7 /* Set the error character */
 #define FTDI_SIO_SET_LATENCY_TIMER	9 /* Set the latency timer */
 #define FTDI_SIO_GET_LATENCY_TIMER	10 /* Get the latency timer */
+#define FTDI_SIO_SET_BITBANG            11 /* Set the bitbang mode */
 
 /* Interface indices for FT2232, FT2232H and FT4232H devices */
 #define INTERFACE_A		1
@@ -345,6 +346,20 @@ enum ftdi_sio_baudrate {
  */
 
 /*
+ * FTDI_SIO_SET_BITBANG
+ *
+ * Set the chip's bitbang mode. Used to switch FT232H (which else?) into
+ * synchronous FIFO mode which cannot be configured in the eeprom.
+ *
+ */
+
+#define FTDI_SIO_SET_BITBANG_REQUEST FTDI_SIO_SET_BITBANG
+#define FTDI_SIO_SET_BITBANG_REQUEST_TYPE 0x40
+
+#define FTDI_BITMODE_RESET     0x00  /* Switch back to normal operation */
+#define FTDI_BITMODE_SYNCFIFO  0x40  /* Switch to syncronous FIFO mode  */
+
+/*
  * FTDI_SIO_SET_EVENT_CHAR
  *
  * Set the special event character for the specified communications port.
-- 
2.0.0.rc2

[PATCH 2/2] usb/ftdi_sio: Add support for setting CBUS pins on FT232H

[Philipp Hachtmann]

This patch adds a sysfs attribute "cbus" which allows to set the four CBUS
pins on the FT232H.

CBUS support could probably extended to all supporting FTDI chips.

Signed-off-by: Philipp Hachtmann <hachti@hachti.de>
---
 drivers/usb/serial/ftdi_sio.c | 66 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/serial/ftdi_sio.h |  1 +
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index cacba4a..c84e27c 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1354,9 +1354,39 @@ static int set_syncmode(struct usb_serial_port *port, int enable)
 			     urb_value, priv->interface,
 			     NULL, 0, WDR_SHORT_TIMEOUT);
 
+}
+
+static int set_cbus(struct usb_serial_port *port, u8 data)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+
+	__u16 urb_value = 0;
+	int rv = 0;
+
+	urb_value = FTDI_BITMODE_CBUS << 8 | data;
+
+	rv = usb_control_msg(port->serial->dev,
+			     usb_sndctrlpipe(port->serial->dev, 0),
+			     FTDI_SIO_SET_BITBANG_REQUEST,
+			     FTDI_SIO_SET_BITBANG_REQUEST_TYPE,
+			     urb_value, priv->interface,
+			     NULL, 0, WDR_SHORT_TIMEOUT);
+
+	if (rv)
+		return rv;
+	if (priv->syncmode) {
+		priv->syncmode = 0;
+		rv = set_syncmode(port, 1);
+	}
+
 	return rv;
 }
 
+static int get_cbus(struct usb_serial_port *port)
+{
+	return -EIO;
+}
+
 static int write_latency_timer(struct usb_serial_port *port)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
@@ -1684,6 +1714,34 @@ static ssize_t syncmode_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(syncmode);
 
+static ssize_t cbus_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	int retval;
+	struct usb_serial_port *port = to_usb_serial_port(dev);
+	retval = get_cbus(port);
+	if (retval < 0)
+		return retval;
+	return sprintf(buf, "%#-x\n", get_cbus(port));
+}
+
+static ssize_t cbus_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *valbuf, size_t count)
+{
+	unsigned long value;
+	struct usb_serial_port *port = to_usb_serial_port(dev);
+	int retval = kstrtoul(valbuf, 0, &value);
+
+	if (retval)
+		return -EINVAL;
+	retval = set_cbus(port, value);
+	if (retval < 0)
+		return -EIO;
+	return count;
+}
+static DEVICE_ATTR_RW(cbus);
+
 /* Write an event character directly to the FTDI register.  The ASCII
    value is in the low 8 bits, with the enable bit in the 9th bit. */
 static ssize_t store_event_char(struct device *dev,
@@ -1736,6 +1794,10 @@ static int create_sysfs_attrs(struct usb_serial_port *port)
 	}
 	if ((!retval) && priv->chip_type == FT232H) {
 		retval = device_create_file(&port->dev,
+					    &dev_attr_cbus);
+		if (retval)
+			return retval;
+		retval = device_create_file(&port->dev,
 					    &dev_attr_syncmode);
 	}
 	return retval;
@@ -1758,8 +1820,10 @@ static void remove_sysfs_attrs(struct usb_serial_port *port)
 			device_remove_file(&port->dev, &dev_attr_latency_timer);
 		}
 	}
-	if (priv->chip_type == FT232H)
+	if (priv->chip_type == FT232H) {
 		device_remove_file(&port->dev, &dev_attr_syncmode);
+		device_remove_file(&port->dev, &dev_attr_cbus);
+	}
 }
 
 /*
diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h
index 04a29f8..f9de34c 100644
--- a/drivers/usb/serial/ftdi_sio.h
+++ b/drivers/usb/serial/ftdi_sio.h
@@ -357,6 +357,7 @@ enum ftdi_sio_baudrate {
 #define FTDI_SIO_SET_BITBANG_REQUEST_TYPE 0x40
 
 #define FTDI_BITMODE_RESET     0x00  /* Switch back to normal operation */
+#define FTDI_BITMODE_CBUS      0x20  /* Configure CBUS pins */
 #define FTDI_BITMODE_SYNCFIFO  0x40  /* Switch to syncronous FIFO mode  */
 
 /*
-- 
2.0.0.rc2

Re: [PATCH 2/2] usb/ftdi_sio: Add support for setting CBUS pins on FT232H

[Peter Stuge]

Philipp Hachtmann wrote:
> This patch adds a sysfs attribute "cbus" which allows to set the
> four CBUS pins on the FT232H.

I think this should be implemented with the gpio subsystem instead.


//Peter

Re: [PATCH 2/2] usb/ftdi_sio: Add support for setting CBUS pins on FT232H

[Philipp Hachtmann]

On 01.06.2014 02:00, Peter Stuge wrote:
> Philipp Hachtmann wrote:
>> This patch adds a sysfs attribute "cbus" which allows to set the
>> four CBUS pins on the FT232H.
>
> I think this should be implemented with the gpio subsystem instead.
The GPIO subsystem seems to be made for GPIO pins connected directly to the 
computer. The Beaglebone with its AM335x SoC seems to be a good example of
a computer with many GPIOs in the sense of the GPIO subsystem.
And the GPIO pins have fixed unique numbers. No, I don't think that the GPIO 
subsystem has anything to do with the additional pins on some USB device.
Please correct me if I'm completely wrong.

Kind regards,

Philipp

Re: [PATCH 2/2] usb/ftdi_sio: Add support for setting CBUS pins on FT232H

[Greg KH]

On Sun, Jun 01, 2014 at 02:14:27AM +0200, Philipp Hachtmann wrote:
> On 01.06.2014 02:00, Peter Stuge wrote:
> >Philipp Hachtmann wrote:
> >>This patch adds a sysfs attribute "cbus" which allows to set the
> >>four CBUS pins on the FT232H.
> >
> >I think this should be implemented with the gpio subsystem instead.
> The GPIO subsystem seems to be made for GPIO pins connected directly to the
> computer. The Beaglebone with its AM335x SoC seems to be a good example of
> a computer with many GPIOs in the sense of the GPIO subsystem.
> And the GPIO pins have fixed unique numbers. No, I don't think that the GPIO
> subsystem has anything to do with the additional pins on some USB device.
> Please correct me if I'm completely wrong.

As they are GPIO pins on this device, it should be the subsystem that
controls it.  That way, userspace programs that are used to talk to a
GPIO device will "just work", and not have to be customized just for
this specific device and sysfs file.

So please use the GPIO subsystem instead of creating your own interface.

thanks,

greg k-h

Re: [PATCH 2/2] usb/ftdi_sio: Add support for setting CBUS pins on FT232H

[Philipp Hachtmann]

On 01.06.2014 04:31, Greg KH wrote:
> As they are GPIO pins on this device, it should be the subsystem that
> controls it.  That way, userspace programs that are used to talk to a
> GPIO device will "just work", and not have to be customized just for
> this specific device and sysfs file.

> So please use the GPIO subsystem instead of creating your own interface.

Yes but... I should have avoided the term "GPIO".

The FT232H (and other chips of the family) are communication controller ICs that 
support several operation modes. The usb-serial driver partially supports the 
chips' functionalities by supplying a tty device to the system.
At least the FT232H has something called CBUS: Four (!) bits of GPIO that 
*might* be available depending on the device's configuration stored in an EEPROM 
attached to the chip. For example if the chip is in sync FIFO mode, only two of 
those bits reach the chip's surface.

I'll try to discuss two use cases:

1. Someone builds hardware with an FT chip and some general functionality 
attached to the four usable CBUS lines, totally unrelated to the chip's 
FIFO/UART etc. functionality.
In that case I would strongly recommend to register the CBUS stuff with the GPIO 
subsystem. The user then could - as you noted - run any program used to deal 
with official GPIO pins on top of the four lines.
But I think that this use case is one of the less likely ones.

2. Someone builds hardware which uses some CBUS pins to control external 
circuitry that also uses the UART/FIFO interface. Both with tight functional 
integration. The CBUS pins become something in the rank of modem status lines.
An application that uses the port also wants to easily access the CBUS pins. And 
it really knows what it does because it knows what it is doing. From my point of 
view this is the more common use case.

Consequently the best way to cover the CBUS pins would be via the device's 
ioctls. But as the device is driven by common tty and usb-serial code which 
handles the ioctls I currently see how to achieve that without breaking a lot.

The second best way I see is adding a property to sysfs. This would already help 
a lot. A program knowing about the hardware then *can* sanely play with the CBUS.

Adding the functionality to sysfs should not interfere with a possible provision 
of an "official" GPIO device (struct gpio_chip).

For me personally it's most important to get access to CBUS in any way where the 
relation of the tty and the CBUS is *not* lost. I see no point in exposing the 
CBUS to any userspace application that thinks it can play blinkenlights on them. 
But I also see no problem if someone wants the possibility and adds general GPIO 
support. The patch will provide a stable base to do so.

So please consider the patches as a starting point.

Re: [PATCH 2/2] usb/ftdi_sio: Add support for setting CBUS pins on FT232H

[Greg KH]

On Mon, Jun 02, 2014 at 02:57:39AM +0200, Philipp Hachtmann wrote:
> On 01.06.2014 04:31, Greg KH wrote:
> >As they are GPIO pins on this device, it should be the subsystem that
> >controls it.  That way, userspace programs that are used to talk to a
> >GPIO device will "just work", and not have to be customized just for
> >this specific device and sysfs file.
> 
> >So please use the GPIO subsystem instead of creating your own interface.
> 
> Yes but... I should have avoided the term "GPIO".

No, you used the right term :)

> The FT232H (and other chips of the family) are communication controller ICs
> that support several operation modes. The usb-serial driver partially
> supports the chips' functionalities by supplying a tty device to the system.
> At least the FT232H has something called CBUS: Four (!) bits of GPIO that
> *might* be available depending on the device's configuration stored in an
> EEPROM attached to the chip. For example if the chip is in sync FIFO mode,
> only two of those bits reach the chip's surface.
> 
> I'll try to discuss two use cases:
> 
> 1. Someone builds hardware with an FT chip and some general functionality
> attached to the four usable CBUS lines, totally unrelated to the chip's
> FIFO/UART etc. functionality.
> In that case I would strongly recommend to register the CBUS stuff with the
> GPIO subsystem. The user then could - as you noted - run any program used to
> deal with official GPIO pins on top of the four lines.
> But I think that this use case is one of the less likely ones.

But you don't know...

> 2. Someone builds hardware which uses some CBUS pins to control external
> circuitry that also uses the UART/FIFO interface. Both with tight functional
> integration. The CBUS pins become something in the rank of modem status
> lines.
> An application that uses the port also wants to easily access the CBUS pins.
> And it really knows what it does because it knows what it is doing. From my
> point of view this is the more common use case.
> 
> Consequently the best way to cover the CBUS pins would be via the device's
> ioctls. But as the device is driven by common tty and usb-serial code which
> handles the ioctls I currently see how to achieve that without breaking a
> lot.

No, I am not going to add custom ioctls to a single driver for this.

Please just use the gpio subsystem, that is what it is there for, and if
you need to interleave led flashing or modem control line signals with
the data you are sending, then just do that in your program.  It's no
different than sending custom ioctls, you have to know what you are
doing.

This comes up every 6 months or so, you aren't the first to ask for
custom ioctls / sysfs files for this chip.  But no one seems to want to
to do the work to make it a proper gpio device, which is sad :(

> For me personally it's most important to get access to CBUS in any way where
> the relation of the tty and the CBUS is *not* lost.

It would not be lost at all, you can see that relationship in sysfs
between the gpio device and the tty device, they are both attached to
the same "parent" usb interface.

thanks,

greg k-h

Re: [PATCH 2/2] usb/ftdi_sio: Add support for setting CBUS pins on FT232H

[Philipp Hachtmann]

On 02.06.2014 03:38, Greg KH wrote:

 >> Yes but... I should have avoided the term "GPIO".> No, you used the right 
term :)
Sorry for using the right term :)

 > No, I am not going to add custom ioctls to a single driver for this.
I did neither request nor expect that - did it sound that way?

 > It would not be lost at all, you can see that relationship in sysfs
 > between the gpio device and the tty device, they are both attached to
 > the same "parent" usb interface.
Ok, I understand. This is a real argument for the GPIO stuff. I'm just beginning 
to understand that using the GPIO stuff will make everyone happy.

 > This comes up every 6 months or so, you aren't the first to ask for
 > custom ioctls / sysfs files for this chip.  But no one seems to want to
 > to do the work to make it a proper gpio device, which is sad :(
Ok, ok. I will do it but it will take some time. I will resend my two patches then.

Kind regards

Philipp

Re: [PATCH 2/2] usb/ftdi_sio: Add support for setting CBUS pins on FT232H

[Philipp Hachtmann]

On 01.06.2014 01:31, Philipp Hachtmann wrote:
> This patch adds a sysfs attribute "cbus" which allows to set the four CBUS
> pins on the FT232H.
The patch contains a *small* error that happened when splitting up my 
modifications. Next series will contain the clean variant.