[PATCH] Add support for ITE887x serial chipsets

[PATCH] Add support for ITE887x serial chipsets

[Niels de Vos]

Hi,

the Super I/O 887x-chipsets of ITE, are currently not completely
supported. Only parport_pc has the ability to activate the (optional)
parallel port. This patch adds support for the serial ports.

Signed-off-by: Niels de Vos <niels.devos@wincor-nixdorf.com>

--- linux-2.6.20.3/drivers/serial/8250_pci.c.orig	2007-03-13 19:27:08.000000000 +0100
+++ linux-2.6.20.3/drivers/serial/8250_pci.c	2007-03-26 15:42:17.000000000 +0200
@@ -581,6 +581,174 @@ static int pci_netmos_init(struct pci_de
 	return num_serial;
 }
 
+/*
+ * ITE support by Niels de Vos <niels.devos@wincor-nixdorf.com>
+ */
+
+static int __devinit pci_ite887x_init(struct pci_dev *dev)
+{
+	/* inta_addr are the configuration addresses of the ITE */
+	short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1E0, 0x200,
+								0x280, 0 };
+	int ret, i, type;
+	struct resource *iobase;
+
+	/* search for the base-ioport */
+	i = 0;
+	while (inta_addr[i] && dev->dev.driver_data != NULL) {
+		dev->dev.driver_data = request_region(inta_addr[i], 32,
+								"ite887x");
+		if (dev->dev.driver_data != NULL) {
+			/* write POSIO0R - speed | size | ioport */
+			pci_write_config_dword(dev, 0x60,
+						0xe5000000 | inta_addr[i]);
+			/* write INTCBAR - ioport */
+			pci_write_config_dword(dev, 0x78, inta_addr[i]);
+			ret = inb(inta_addr[i]);
+			if (ret != 0xff) {
+				/* ioport connected */
+				break;
+			}
+			release_resource(dev->dev.driver_data);
+			dev->dev.driver_data = NULL;
+		}
+		i++;
+	}
+
+	if (! inta_addr[i]) {
+		printk(KERN_ERR "ite887x: could not find iobase\n");
+		return -ENODEV;
+	}
+
+	iobase = dev->dev.driver_data;
+
+	/* start of undocumented type checking (see parport_pc.c) */
+	type = inb(iobase->start + 0x18);
+	type &= 0x0f;
+
+	switch (type) {
+	case 0x2:
+		printk(KERN_DEBUG "8250_pci: ITE8871 found (1P)\n");
+		break;
+	case 0xa:
+		printk(KERN_DEBUG "8250_pci: ITE8875 found (1P)\n");
+		break;
+	case 0xe:
+		printk(KERN_INFO "8250_pci: ITE8872 found (2S1P)\n");
+		return 2;
+	case 0x6:
+		printk(KERN_INFO "8250_pci: ITE8873 found (1S)\n");
+		return 1;
+	case 0x8:
+		printk(KERN_INFO "8250_pci: ITE8874 found (2S)\n");
+		return 2;
+	default:
+		moan_device("unknown ITE887x", dev);
+	}
+
+	/* the device has no UARTs if we get here */
+	release_resource(iobase);
+	dev->dev.driver_data = NULL;
+	return -ENODEV;
+}
+
+/*
+ * activate the UART in the MISCR
+ */
+static int
+pci_ite887x_setup(struct serial_private *priv, struct pciserial_board *board,
+		  struct uart_port *port, int idx)
+{
+	int ret;
+	struct pci_dev *dev = priv->dev;
+	u32 miscr, uartbar, ioport;
+	/* iobase is the private driver_data */
+	struct resource *iobase;
+	/* registers */
+	unsigned short MISCR = 0x9c, UARTBAR = 0x7c;
+	unsigned short PS1BAR = 0x14, POSIO1 = 0x64;
+
+	/* enable IO_Space bit */
+	u32 POSIO_ENABLE = 1 << 31;
+	/* Decoding speed (1 = slow, 2 = medium, 3 = fast) */
+	u32 POSIO_SPEED = 3 << 29;	
+
+	iobase = (struct resource*) dev->dev.driver_data;
+	if (iobase == NULL) {
+		printk(KERN_ERR "ite887x: iobase is not available\n");
+		return -ENODEV;
+	}
+
+	/* read the I/O port from the device */
+	ret = pci_read_config_dword(dev, PS1BAR + (0x4 * idx), &ioport);
+	if (ret) {
+		printk(KERN_ERR "ite887x: read error PS%dBAR\n", idx + 1);
+		ret = -ENODEV;
+		goto release_inta;
+	}
+
+	ioport &= 0x0000FF00;	/* the actual I/O space base address */
+	ret = pci_write_config_dword(dev, POSIO1 + (0x4 * idx),
+				POSIO_ENABLE | POSIO_SPEED | ioport);
+	if (ret) {
+		printk(KERN_ERR "ite887x: write error PSIO%d+\n", idx + 1);
+		ret = -ENODEV;
+		goto release_inta;
+	}
+
+	/* write the ioport to the UARTBAR */
+	ret = pci_read_config_dword(dev, UARTBAR, &uartbar);
+	if (ret) {
+		printk(KERN_ERR "ite887x: read error UARTBAR\n");
+		ret = -ENODEV;
+		goto release_inta;
+	}
+	uartbar &= ~(15 << (4 * idx));		/* clear half the reg */
+	uartbar |= ioport << (16 * idx);	/* set the ioport */
+	ret = pci_write_config_dword(dev, UARTBAR, uartbar);
+	if (ret) {
+		printk(KERN_ERR "ite887x: write error UARTBAR\n");
+		ret = -ENODEV;
+		goto release_inta;
+	}
+
+	/* get current config */
+	ret = pci_read_config_dword(dev, MISCR, &miscr);
+	if (ret) {
+		printk(KERN_ERR "ite887x: read error MISCR\n");
+		ret = -ENODEV;
+		goto release_inta;
+	}
+
+	/* disable interrupts (UARTx_Routing[3:0]) */
+	miscr &= ~(0xf << (12 - 4 * idx));
+	/* activate the UART (UARTx_En) */
+	miscr |= 1 << (23 - idx);
+
+	/* write new config with activated UART */
+	ret = pci_write_config_dword(dev, MISCR, miscr);
+	if (ret) {
+		printk(KERN_ERR "ite887x: write error MISCR\n");
+		ret = -ENODEV;
+		goto release_inta;
+	}
+
+	/* idx + 1: using POSIO1 and up */
+	return setup_port(priv, port, idx + 1, board->first_offset, 0);
+
+release_inta:
+	/* release region again if we get here */
+	release_resource(iobase);
+	dev->dev.driver_data = NULL;
+	return ret;
+}
+
+static void __devexit pci_ite887x_exit(struct pci_dev *dev)
+{
+	/* free the private driver data */
+	release_resource(((struct resource*) dev->dev.driver_data));
+}
+
 static int
 pci_default_setup(struct serial_private *priv, struct pciserial_board *board,
 		  struct uart_port *port, int idx)
@@ -654,6 +822,18 @@ static struct pci_serial_quirk pci_seria
 		.setup		= pci_default_setup,
 	},
 	/*
+	 * ITE
+	 */
+	{
+		.vendor		= PCI_VENDOR_ID_ITE,
+		.device		= PCI_DEVICE_ID_ITE_8872,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ite887x_init,
+		.setup		= pci_ite887x_setup,
+		.exit		= __devexit_p(pci_ite887x_exit),
+	},
+	/*
 	 * Panacom
 	 */
 	{
@@ -968,6 +1148,7 @@ enum pci_board_num_t {
 	pbn_plx_romulus,
 	pbn_oxsemi,
 	pbn_intel_i960,
+	pbn_ite_887x,
 	pbn_sgi_ioc3,
 	pbn_nec_nile4,
 	pbn_computone_4,
@@ -1430,6 +1611,15 @@ static struct pciserial_board pci_boards
 	},
 
 	/*
+	 * ITE
+	 */
+	[pbn_ite_887x] = {
+                .flags          = FL_BASE0 | FL_BASE_BARS,
+                .base_baud      = 115200,
+                .uart_offset    = 8,
+        },
+
+	/*
 	 * NEC Vrc-5074 (Nile 4) builtin UART.
 	 */
 	[pbn_nec_nile4] = {
@@ -2370,6 +2560,13 @@ static struct pci_device_id serial_pci_t
 	{	PCI_VENDOR_ID_TOPIC, PCI_DEVICE_ID_TOPIC_TP560,
 		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 		pbn_b0_1_115200 },
+	/*
+	 * ITE
+	 */
+	{	PCI_VENDOR_ID_ITE, PCI_DEVICE_ID_ITE_8872,
+		PCI_ANY_ID, PCI_ANY_ID,
+		0, 0,
+		pbn_ite_887x },	
 
 	/*
 	 * IntaShield IS-200

Re: [PATCH] Add support for ITE887x serial chipsets

[Andrey Panin]

[-- Attachment #1: Type: text/plain, Size: 8140 bytes --]

On 085, 03 26, 2007 at 04:17:02PM +0200, Niels de Vos wrote:
> Hi,
> 
> the Super I/O 887x-chipsets of ITE, are currently not completely
> supported. Only parport_pc has the ability to activate the (optional)
> parallel port. This patch adds support for the serial ports.
> 
> Signed-off-by: Niels de Vos <niels.devos@wincor-nixdorf.com>
> 
> --- linux-2.6.20.3/drivers/serial/8250_pci.c.orig	2007-03-13 19:27:08.000000000 +0100
> +++ linux-2.6.20.3/drivers/serial/8250_pci.c	2007-03-26 15:42:17.000000000 +0200
> @@ -581,6 +581,174 @@ static int pci_netmos_init(struct pci_de
>  	return num_serial;
>  }
>  
> +/*
> + * ITE support by Niels de Vos <niels.devos@wincor-nixdorf.com>
> + */

I think we need a comment here describing how ugly these chips are :)

> +
> +static int __devinit pci_ite887x_init(struct pci_dev *dev)
> +{
> +	/* inta_addr are the configuration addresses of the ITE */
> +	short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1E0, 0x200,
> +								0x280, 0 };

static const, please.

> +	int ret, i, type;
> +	struct resource *iobase;
> +
> +	/* search for the base-ioport */
> +	i = 0;
> +	while (inta_addr[i] && dev->dev.driver_data != NULL) {
> +		dev->dev.driver_data = request_region(inta_addr[i], 32,
> +								"ite887x");
> +		if (dev->dev.driver_data != NULL) {
> +			/* write POSIO0R - speed | size | ioport */
> +			pci_write_config_dword(dev, 0x60,
> +						0xe5000000 | inta_addr[i]);
> +			/* write INTCBAR - ioport */
> +			pci_write_config_dword(dev, 0x78, inta_addr[i]);
> +			ret = inb(inta_addr[i]);
> +			if (ret != 0xff) {
> +				/* ioport connected */
> +				break;
> +			}
> +			release_resource(dev->dev.driver_data);
> +			dev->dev.driver_data = NULL;
> +		}
> +		i++;
> +	}
> +
> +	if (! inta_addr[i]) {
	     ^
Please remove space after !

> +		printk(KERN_ERR "ite887x: could not find iobase\n");
> +		return -ENODEV;
> +	}
> +
> +	iobase = dev->dev.driver_data;
> +
> +	/* start of undocumented type checking (see parport_pc.c) */
> +	type = inb(iobase->start + 0x18);
> +	type &= 0x0f;

Why not:
type = inb(iobase->start + 0x18) & 0x0f;

> +
> +	switch (type) {
> +	case 0x2:
> +		printk(KERN_DEBUG "8250_pci: ITE8871 found (1P)\n");
> +		break;
> +	case 0xa:
> +		printk(KERN_DEBUG "8250_pci: ITE8875 found (1P)\n");
> +		break;
> +	case 0xe:
> +		printk(KERN_INFO "8250_pci: ITE8872 found (2S1P)\n");
> +		return 2;
> +	case 0x6:
> +		printk(KERN_INFO "8250_pci: ITE8873 found (1S)\n");
> +		return 1;
> +	case 0x8:
> +		printk(KERN_INFO "8250_pci: ITE8874 found (2S)\n");
> +		return 2;
> +	default:
> +		moan_device("unknown ITE887x", dev);
> +	}

These printk's are not needed IMHO.

> +
> +	/* the device has no UARTs if we get here */
> +	release_resource(iobase);
> +	dev->dev.driver_data = NULL;
> +	return -ENODEV;
> +}
> +
> +/*
> + * activate the UART in the MISCR
> + */
> +static int
> +pci_ite887x_setup(struct serial_private *priv, struct pciserial_board *board,
> +		  struct uart_port *port, int idx)
> +{
> +	int ret;
> +	struct pci_dev *dev = priv->dev;
> +	u32 miscr, uartbar, ioport;
> +	/* iobase is the private driver_data */
> +	struct resource *iobase;
> +	/* registers */
> +	unsigned short MISCR = 0x9c, UARTBAR = 0x7c;
> +	unsigned short PS1BAR = 0x14, POSIO1 = 0x64;
> +
> +	/* enable IO_Space bit */
> +	u32 POSIO_ENABLE = 1 << 31;
> +	/* Decoding speed (1 = slow, 2 = medium, 3 = fast) */
> +	u32 POSIO_SPEED = 3 << 29;	
> +
> +	iobase = (struct resource*) dev->dev.driver_data;
> +	if (iobase == NULL) {
> +		printk(KERN_ERR "ite887x: iobase is not available\n");
> +		return -ENODEV;
> +	}

Is this check really needed ? I can't see how you can enter this function
with dev->dev.driver_data == NULL.

> +
> +	/* read the I/O port from the device */
> +	ret = pci_read_config_dword(dev, PS1BAR + (0x4 * idx), &ioport);
> +	if (ret) {

Checking return value of pci_read_config_XXX/pci_write_config_XXX is (almost ?)
useless and only complicates things.

> +		printk(KERN_ERR "ite887x: read error PS%dBAR\n", idx + 1);
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	ioport &= 0x0000FF00;	/* the actual I/O space base address */
> +	ret = pci_write_config_dword(dev, POSIO1 + (0x4 * idx),
> +				POSIO_ENABLE | POSIO_SPEED | ioport);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: write error PSIO%d+\n", idx + 1);
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	/* write the ioport to the UARTBAR */
> +	ret = pci_read_config_dword(dev, UARTBAR, &uartbar);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: read error UARTBAR\n");
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +	uartbar &= ~(15 << (4 * idx));		/* clear half the reg */
> +	uartbar |= ioport << (16 * idx);	/* set the ioport */
> +	ret = pci_write_config_dword(dev, UARTBAR, uartbar);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: write error UARTBAR\n");
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	/* get current config */
> +	ret = pci_read_config_dword(dev, MISCR, &miscr);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: read error MISCR\n");
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	/* disable interrupts (UARTx_Routing[3:0]) */
> +	miscr &= ~(0xf << (12 - 4 * idx));
> +	/* activate the UART (UARTx_En) */
> +	miscr |= 1 << (23 - idx);
> +
> +	/* write new config with activated UART */
> +	ret = pci_write_config_dword(dev, MISCR, miscr);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: write error MISCR\n");
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	/* idx + 1: using POSIO1 and up */
> +	return setup_port(priv, port, idx + 1, board->first_offset, 0);
> +
> +release_inta:
> +	/* release region again if we get here */
> +	release_resource(iobase);
> +	dev->dev.driver_data = NULL;
> +	return ret;
> +}
> +
> +static void __devexit pci_ite887x_exit(struct pci_dev *dev)
> +{
> +	/* free the private driver data */
> +	release_resource(((struct resource*) dev->dev.driver_data));
> +}
> +
>  static int
>  pci_default_setup(struct serial_private *priv, struct pciserial_board *board,
>  		  struct uart_port *port, int idx)
> @@ -654,6 +822,18 @@ static struct pci_serial_quirk pci_seria
>  		.setup		= pci_default_setup,
>  	},
>  	/*
> +	 * ITE
> +	 */
> +	{
> +		.vendor		= PCI_VENDOR_ID_ITE,
> +		.device		= PCI_DEVICE_ID_ITE_8872,
> +		.subvendor	= PCI_ANY_ID,
> +		.subdevice	= PCI_ANY_ID,
> +		.init		= pci_ite887x_init,
> +		.setup		= pci_ite887x_setup,
> +		.exit		= __devexit_p(pci_ite887x_exit),
> +	},
> +	/*
>  	 * Panacom
>  	 */
>  	{
> @@ -968,6 +1148,7 @@ enum pci_board_num_t {
>  	pbn_plx_romulus,
>  	pbn_oxsemi,
>  	pbn_intel_i960,
> +	pbn_ite_887x,
>  	pbn_sgi_ioc3,
>  	pbn_nec_nile4,
>  	pbn_computone_4,
> @@ -1430,6 +1611,15 @@ static struct pciserial_board pci_boards
>  	},
>  
>  	/*
> +	 * ITE
> +	 */
> +	[pbn_ite_887x] = {
> +                .flags          = FL_BASE0 | FL_BASE_BARS,
> +                .base_baud      = 115200,
> +                .uart_offset    = 8,
> +        },

IMHO this is not needed. You can use pbn_b0_bt_2_115200 or pbn_b0_bt_1_115200,
since quirks will be used anyway.

> +
> +	/*
>  	 * NEC Vrc-5074 (Nile 4) builtin UART.
>  	 */
>  	[pbn_nec_nile4] = {
> @@ -2370,6 +2560,13 @@ static struct pci_device_id serial_pci_t
>  	{	PCI_VENDOR_ID_TOPIC, PCI_DEVICE_ID_TOPIC_TP560,
>  		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>  		pbn_b0_1_115200 },
> +	/*
> +	 * ITE
> +	 */
> +	{	PCI_VENDOR_ID_ITE, PCI_DEVICE_ID_ITE_8872,
> +		PCI_ANY_ID, PCI_ANY_ID,
> +		0, 0,
> +		pbn_ite_887x },	
>  
>  	/*
>  	 * IntaShield IS-200
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Andrey Panin		| Linux and UNIX system administrator
pazke@donpac.ru		| PGP key: wwwkeys.pgp.net

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add support for ITE887x serial chipsets

[Russell King]

On Mon, Mar 26, 2007 at 04:17:02PM +0200, Niels de Vos wrote:
> +/*
> + * ITE support by Niels de Vos <niels.devos@wincor-nixdorf.com>
> + */
> +
> +static int __devinit pci_ite887x_init(struct pci_dev *dev)
> +{
> +	/* inta_addr are the configuration addresses of the ITE */
> +	short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1E0, 0x200,
> +								0x280, 0 };
> +	int ret, i, type;
> +	struct resource *iobase;
> +
> +	/* search for the base-ioport */
> +	i = 0;
> +	while (inta_addr[i] && dev->dev.driver_data != NULL) {
> +		dev->dev.driver_data = request_region(inta_addr[i], 32,
> +								"ite887x");
> +		if (dev->dev.driver_data != NULL) {
> +			/* write POSIO0R - speed | size | ioport */
> +			pci_write_config_dword(dev, 0x60,
> +						0xe5000000 | inta_addr[i]);
> +			/* write INTCBAR - ioport */
> +			pci_write_config_dword(dev, 0x78, inta_addr[i]);
> +			ret = inb(inta_addr[i]);
> +			if (ret != 0xff) {
> +				/* ioport connected */
> +				break;
> +			}
> +			release_resource(dev->dev.driver_data);
> +			dev->dev.driver_data = NULL;

This doesn't free the memory allocated by request_region.  Always
pair release_region() with request_region().  Ditto for all of your
other uses of "release_resource()".

Might also be cleaner to avoid touching dev->dev.driver_data at all
until you've identified a working resource.

> +		}
> +		i++;
> +	}
> +
> +	if (! inta_addr[i]) {
> +		printk(KERN_ERR "ite887x: could not find iobase\n");
> +		return -ENODEV;
> +	}
> +
> +	iobase = dev->dev.driver_data;
> +
> +	/* start of undocumented type checking (see parport_pc.c) */
> +	type = inb(iobase->start + 0x18);
> +	type &= 0x0f;
> +
> +	switch (type) {
> +	case 0x2:
> +		printk(KERN_DEBUG "8250_pci: ITE8871 found (1P)\n");
> +		break;
> +	case 0xa:
> +		printk(KERN_DEBUG "8250_pci: ITE8875 found (1P)\n");
> +		break;
> +	case 0xe:
> +		printk(KERN_INFO "8250_pci: ITE8872 found (2S1P)\n");
> +		return 2;
> +	case 0x6:
> +		printk(KERN_INFO "8250_pci: ITE8873 found (1S)\n");
> +		return 1;
> +	case 0x8:
> +		printk(KERN_INFO "8250_pci: ITE8874 found (2S)\n");
> +		return 2;
> +	default:
> +		moan_device("unknown ITE887x", dev);
> +	}
> +
> +	/* the device has no UARTs if we get here */
> +	release_resource(iobase);
> +	dev->dev.driver_data = NULL;
> +	return -ENODEV;
> +}
> +
> +/*
> + * activate the UART in the MISCR
> + */
> +static int
> +pci_ite887x_setup(struct serial_private *priv, struct pciserial_board *board,
> +		  struct uart_port *port, int idx)
> +{
> +	int ret;
> +	struct pci_dev *dev = priv->dev;
> +	u32 miscr, uartbar, ioport;
> +	/* iobase is the private driver_data */
> +	struct resource *iobase;
> +	/* registers */
> +	unsigned short MISCR = 0x9c, UARTBAR = 0x7c;
> +	unsigned short PS1BAR = 0x14, POSIO1 = 0x64;
> +
> +	/* enable IO_Space bit */
> +	u32 POSIO_ENABLE = 1 << 31;
> +	/* Decoding speed (1 = slow, 2 = medium, 3 = fast) */
> +	u32 POSIO_SPEED = 3 << 29;	

#define all of these?  They're constants.

> +
> +	iobase = (struct resource*) dev->dev.driver_data;
> +	if (iobase == NULL) {
> +		printk(KERN_ERR "ite887x: iobase is not available\n");
> +		return -ENODEV;
> +	}
> +
> +	/* read the I/O port from the device */
> +	ret = pci_read_config_dword(dev, PS1BAR + (0x4 * idx), &ioport);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: read error PS%dBAR\n", idx + 1);
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	ioport &= 0x0000FF00;	/* the actual I/O space base address */
> +	ret = pci_write_config_dword(dev, POSIO1 + (0x4 * idx),
> +				POSIO_ENABLE | POSIO_SPEED | ioport);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: write error PSIO%d+\n", idx + 1);
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	/* write the ioport to the UARTBAR */
> +	ret = pci_read_config_dword(dev, UARTBAR, &uartbar);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: read error UARTBAR\n");
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +	uartbar &= ~(15 << (4 * idx));		/* clear half the reg */
> +	uartbar |= ioport << (16 * idx);	/* set the ioport */
> +	ret = pci_write_config_dword(dev, UARTBAR, uartbar);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: write error UARTBAR\n");
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	/* get current config */
> +	ret = pci_read_config_dword(dev, MISCR, &miscr);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: read error MISCR\n");
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}
> +
> +	/* disable interrupts (UARTx_Routing[3:0]) */
> +	miscr &= ~(0xf << (12 - 4 * idx));
> +	/* activate the UART (UARTx_En) */
> +	miscr |= 1 << (23 - idx);
> +
> +	/* write new config with activated UART */
> +	ret = pci_write_config_dword(dev, MISCR, miscr);
> +	if (ret) {
> +		printk(KERN_ERR "ite887x: write error MISCR\n");
> +		ret = -ENODEV;
> +		goto release_inta;
> +	}

I think all of the above can be done in the init function, which'll be
a _lot_ clearer.  That also means...

> +
> +	/* idx + 1: using POSIO1 and up */
> +	return setup_port(priv, port, idx + 1, board->first_offset, 0);

that you can (usefully) add pbn_b1_bt_1_115200 and completely avoid
this hack.

> @@ -968,6 +1148,7 @@ enum pci_board_num_t {
>  	pbn_plx_romulus,
>  	pbn_oxsemi,
>  	pbn_intel_i960,
> +	pbn_ite_887x,

Always avoid chipset specific definitions if there's another way to
work around them (as suggested above).

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] Add support for ITE887x serial chipsets

[Niels de Vos]

The Super I/O 887x-chipsets of ITE, are currently not completely
supported. This patch adds support for the serial ports of several
ITE 887x chips.

Signed-off-by: Niels de Vos <niels.devos@wincor-nixdorf.com>

---
Thanks Russell and Andrey for your comments. I've taken your suggestions
into account and reworked the patch. All release_resource()'s are replaced
by release_region() to free up the I/O ports. 

> > +	/* inta_addr are the configuration addresses of the ITE */
> > +	short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1E0, 0x200,
> > +								0x280, 0 };
> 
> static const, please.

Done.

> > +	if (! inta_addr[i]) {
> 	     ^
> Please remove space after !

Ofcourse.

> > +	/* start of undocumented type checking (see parport_pc.c) */
> > +	type = inb(iobase->start + 0x18);
> > +	type &= 0x0f;
> 
> Why not:
> type = inb(iobase->start + 0x18) & 0x0f;

Yes, no need to know type before & 0xf.

> > +	switch (type) {
> > +	case 0x2:
> > +		printk(KERN_DEBUG "8250_pci: ITE8871 found (1P)\n");
> > +		break;
> > +	case 0xa:
> > +		printk(KERN_DEBUG "8250_pci: ITE8875 found (1P)\n");
> > +		break;
> > +	case 0xe:
> > +		printk(KERN_INFO "8250_pci: ITE8872 found (2S1P)\n");
> > +		return 2;
> > +	case 0x6:
> > +		printk(KERN_INFO "8250_pci: ITE8873 found (1S)\n");
> > +		return 1;
> > +	case 0x8:
> > +		printk(KERN_INFO "8250_pci: ITE8874 found (2S)\n");
> > +		return 2;
> > +	default:
> > +		moan_device("unknown ITE887x", dev);
> > +	}
> 
> These printk's are not needed IMHO.

You're correct. These come directly from parport_pc. Now removed.

> > +	iobase = (struct resource*) dev->dev.driver_data;
> > +	if (iobase == NULL) {
> > +		printk(KERN_ERR "ite887x: iobase is not available\n");
> > +		return -ENODEV;
> > +	}
> 
> Is this check really needed ? I can't see how you can enter this function
> with dev->dev.driver_data == NULL.

No need, and not anymore either. Rewrote the .init so that .setup is not
needed anymore. Russell suggested this solution to keep it cleaner.


> > +	/* read the I/O port from the device */
> > +	ret = pci_read_config_dword(dev, PS1BAR + (0x4 * idx), &ioport);
> > +	if (ret) {
> 
> Checking return value of pci_read_config_XXX/pci_write_config_XXX is (almost ?)
> useless and only complicates things.

Okay, lot's of other drivers also don't check this. Removed.

> >  	/*
> > +	 * ITE
> > +	 */
> > +	[pbn_ite_887x] = {
> > +                .flags          = FL_BASE0 | FL_BASE_BARS,
> > +                .base_baud      = 115200,
> > +                .uart_offset    = 8,
> > +        },
> 
> IMHO this is not needed. You can use pbn_b0_bt_2_115200 or pbn_b0_bt_1_115200,
> since quirks will be used anyway.

I've added pbn_b1_bt_1_115200 now, Russells remark.


--- linux-2.6.20.3/drivers/serial/8250_pci.c.orig	2007-03-13 19:27:08.000000000 +0100
+++ linux-2.6.20.3/drivers/serial/8250_pci.c	2007-03-27 14:59:47.000000000 +0200
@@ -581,6 +581,137 @@ static int pci_netmos_init(struct pci_de
 	return num_serial;
 }
 
+/*
+ * ITE support by Niels de Vos <niels.devos@wincor-nixdorf.com>
+ *
+ * These chips are available with optionally one parallel port and up to
+ * two serial ports. Unfortunately they all have the same product id.
+ *
+ * Basic configuration is done over a region of 32 I/O ports. The base
+ * ioport is called INTA or INTC, depending on docs/other drivers.
+ *
+ * The region of the 32 I/O ports is configured in POSIO0R...
+ */
+
+/* registers */
+#define ITE_887x_MISCR		0x9c
+#define ITE_887x_INTCBAR	0x78
+#define ITE_887x_UARTBAR	0x7c
+#define ITE_887x_PS0BAR		0x10
+#define ITE_887x_POSIO0		0x60
+
+/* I/O space size */
+#define ITE_887x_IOSIZE		32
+/* I/O space size (bits 26-24; 32 bytes = 101b) */
+#define ITE_887x_POSIO_IOSIZE	(5 << 24)
+/* Decoding speed (1 = slow, 2 = medium, 3 = fast) */
+#define ITE_887x_POSIO_SPEED	(3 << 29)	
+/* enable IO_Space bit */
+#define ITE_887x_POSIO_ENABLE	(1 << 31)	
+
+static int __devinit pci_ite887x_init(struct pci_dev *dev)
+{
+	/* inta_addr are the configuration addresses of the ITE */
+	static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0,
+							0x200, 0x280, 0 };
+	int ret, i, type;
+	struct resource *iobase = NULL;
+	u32 miscr, uartbar, ioport;
+
+	/* search for the base-ioport */
+	i = 0;
+	while (inta_addr[i] && iobase == NULL) {
+		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE,
+								"ite887x");
+		if (iobase != NULL) {
+			/* write POSIO0R - speed | size | ioport */
+			pci_write_config_dword(dev, ITE_887x_POSIO0,
+				ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED |
+				ITE_887x_POSIO_IOSIZE | inta_addr[i]);
+			/* write INTCBAR - ioport */
+			pci_write_config_dword(dev, ITE_887x_INTCBAR,
+								inta_addr[i]);
+			ret = inb(inta_addr[i]);
+			if (ret != 0xff) {
+				/* ioport connected */
+				break;
+			}
+			release_region(iobase->start, ITE_887x_IOSIZE);
+			iobase = NULL;
+		}
+		i++;
+	}
+
+	if (!inta_addr[i]) {
+		printk(KERN_ERR "ite887x: could not find iobase\n");
+		return -ENODEV;
+	}
+
+	/* start of undocumented type checking (see parport_pc.c) */
+	type = inb(iobase->start + 0x18) & 0x0f;
+
+	switch (type) {
+	case 0x2:	/* ITE8871 (1P) */
+	case 0xa:	/* ITE8875 (1P) */
+		ret = 0;
+		break;
+	case 0xe:	/* ITE8872 (2S1P) */
+		ret = 2;
+		break;
+	case 0x6:	/* ITE8873 (1S) */
+		ret = 1;
+		break;
+	case 0x8:	/* ITE8874 (2S) */
+		ret = 2;
+		break;
+	default:
+		moan_device("Unknown ITE887x", dev);
+		ret = -ENODEV;
+	}
+
+	/* configure all serial ports */
+	for (i = 0; i < ret; i++) {
+		/* read the I/O port from the device */
+		pci_read_config_dword(dev, ITE_887x_PS0BAR + (0x4 * (i + 1)),
+								&ioport);
+		ioport &= 0x0000FF00;	/* the actual base address */
+		pci_write_config_dword(dev, ITE_887x_POSIO0 + (0x4 * (i + 1)),
+			ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED | ioport);
+
+		/* write the ioport to the UARTBAR */
+		pci_read_config_dword(dev, ITE_887x_UARTBAR, &uartbar);
+		uartbar &= ~(15 << (4 * i));	/* clear half the reg */
+		uartbar |= ioport << (16 * i);	/* set the ioport */
+		pci_write_config_dword(dev, ITE_887x_UARTBAR, uartbar);
+
+		/* get current config */
+		pci_read_config_dword(dev, ITE_887x_MISCR, &miscr);
+		/* disable interrupts (UARTx_Routing[3:0]) */
+		miscr &= ~(0xf << (12 - 4 * i));
+		/* activate the UART (UARTx_En) */
+		miscr |= 1 << (23 - i);
+		/* write new config with activated UART */
+		pci_write_config_dword(dev, ITE_887x_MISCR, miscr);
+	}
+
+	if (ret > 0) {
+		dev->dev.driver_data = iobase;
+	} else {
+		/* the device has no UARTs if we get here */
+		release_region(iobase->start, ITE_887x_IOSIZE);
+	}
+
+	return ret;
+}
+
+static void __devexit pci_ite887x_exit(struct pci_dev *dev)
+{
+	struct resource *iobase = (struct resource*) dev->dev.driver_data;
+	/* free the private driver data */
+	release_region(iobase->start, ITE_887x_IOSIZE);
+	dev->dev.driver_data = NULL;
+}
+
 static int
 pci_default_setup(struct serial_private *priv, struct pciserial_board *board,
 		  struct uart_port *port, int idx)
@@ -654,6 +785,18 @@ static struct pci_serial_quirk pci_seria
 		.setup		= pci_default_setup,
 	},
 	/*
+	 * ITE
+	 */
+	{
+		.vendor		= PCI_VENDOR_ID_ITE,
+		.device		= PCI_DEVICE_ID_ITE_8872,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ite887x_init,
+		.setup		= pci_default_setup,
+		.exit		= __devexit_p(pci_ite887x_exit),
+	},
+	/*
 	 * Panacom
 	 */
 	{
@@ -927,6 +1070,7 @@ enum pci_board_num_t {
 
 	pbn_b1_2_1250000,
 
+	pbn_b1_bt_1_115200,
 	pbn_b1_bt_2_921600,
 
 	pbn_b1_1_1382400,
@@ -1205,6 +1349,13 @@ static struct pciserial_board pci_boards
 		.uart_offset	= 8,
 	},
 
+	[pbn_b1_bt_1_115200] = {
+		.flags		= FL_BASE1|FL_BASE_BARS,
+		.num_ports	= 1,
+		.base_baud	= 115200,
+		.uart_offset	= 8,
+	},
+
 	[pbn_b1_bt_2_921600] = {
 		.flags		= FL_BASE1|FL_BASE_BARS,
 		.num_ports	= 2,
@@ -2370,6 +2521,13 @@ static struct pci_device_id serial_pci_t
 	{	PCI_VENDOR_ID_TOPIC, PCI_DEVICE_ID_TOPIC_TP560,
 		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 		pbn_b0_1_115200 },
+	/*
+	 * ITE
+	 */
+	{	PCI_VENDOR_ID_ITE, PCI_DEVICE_ID_ITE_8872,
+		PCI_ANY_ID, PCI_ANY_ID,
+		0, 0,
+		pbn_b1_bt_1_115200 },	
 
 	/*
 	 * IntaShield IS-200

Re: [PATCH] Add support for ITE887x serial chipsets

[Russell King]

On Tue, Mar 27, 2007 at 04:03:03PM +0200, Niels de Vos wrote:
> +static int __devinit pci_ite887x_init(struct pci_dev *dev)
> +{
> ...
> +	if (ret > 0) {
> +		dev->dev.driver_data = iobase;
> +	} else {
> +		/* the device has no UARTs if we get here */
> +		release_region(iobase->start, ITE_887x_IOSIZE);
> +	}
> +
> +	return ret;
> +}
> +
> +static void __devexit pci_ite887x_exit(struct pci_dev *dev)
> +{
> +	struct resource *iobase = (struct resource*) dev->dev.driver_data;
> +	/* free the private driver data */
> +	release_region(iobase->start, ITE_887x_IOSIZE);
> +	dev->dev.driver_data = NULL;
> +}
> +

You can't do that.  dev->dev.driver_data is used by this very driver
itself to store its own data, or alternatively the parport_serial
driver to store its private data.  Therefore, dev->dev.driver_data
will be corrupted between your _init function setting it and your
_exit function using it.

Suggest you read the iobase back from the device to release it.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] Add support for ITE887x serial chipsets

[Niels de Vos]

The Super I/O 887x-chipsets of ITE, are currently not completely
supported. This patch adds support for the serial ports of several
ITE 887x chips.

Signed-off-by: Niels de Vos <niels.devos@wincor-nixdorf.com>

---
On Tue, 2007-03-27 at 15:17 +0100, Russell King wrote:
> > +static void __devexit pci_ite887x_exit(struct pci_dev *dev)
> > +{
> > +	struct resource *iobase = (struct resource*) dev->dev.driver_data;
> > +	/* free the private driver data */
> > +	release_region(iobase->start, ITE_887x_IOSIZE);
> > +	dev->dev.driver_data = NULL;
> > +}
> > +
> 
> You can't do that.  dev->dev.driver_data is used by this very driver
> itself to store its own data, or alternatively the parport_serial
> driver to store its private data.  Therefore, dev->dev.driver_data
> will be corrupted between your _init function setting it and your
> _exit function using it.
> 
> Suggest you read the iobase back from the device to release it.

That makes sense! I should have noticed that... Now reading the I/O
port from the device and not using dev->dev.driver_data anymore.

--- linux-2.6.20.3/drivers/serial/8250_pci.c.orig	2007-03-13 19:27:08.000000000 +0100
+++ linux-2.6.20.3/drivers/serial/8250_pci.c	2007-03-27 17:43:58.000000000 +0200
@@ -581,6 +581,135 @@ static int pci_netmos_init(struct pci_de
 	return num_serial;
 }
 
+/*
+ * ITE support by Niels de Vos <niels.devos@wincor-nixdorf.com>
+ *
+ * These chips are available with optionally one parallel port and up to
+ * two serial ports. Unfortunately they all have the same product id.
+ *
+ * Basic configuration is done over a region of 32 I/O ports. The base
+ * ioport is called INTA or INTC, depending on docs/other drivers.
+ *
+ * The region of the 32 I/O ports is configured in POSIO0R...
+ */
+
+/* registers */
+#define ITE_887x_MISCR		0x9c
+#define ITE_887x_INTCBAR	0x78
+#define ITE_887x_UARTBAR	0x7c
+#define ITE_887x_PS0BAR		0x10
+#define ITE_887x_POSIO0		0x60
+
+/* I/O space size */
+#define ITE_887x_IOSIZE		32
+/* I/O space size (bits 26-24; 32 bytes = 101b) */
+#define ITE_887x_POSIO_IOSIZE	(5 << 24)
+/* Decoding speed (1 = slow, 2 = medium, 3 = fast) */
+#define ITE_887x_POSIO_SPEED	(3 << 29)	
+/* enable IO_Space bit */
+#define ITE_887x_POSIO_ENABLE	(1 << 31)	
+
+static int __devinit pci_ite887x_init(struct pci_dev *dev)
+{
+	/* inta_addr are the configuration addresses of the ITE */
+	static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0,
+							0x200, 0x280, 0 };
+	int ret, i, type;
+	struct resource *iobase = NULL;
+	u32 miscr, uartbar, ioport;
+
+	/* search for the base-ioport */
+	i = 0;
+	while (inta_addr[i] && iobase == NULL) {
+		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE,
+								"ite887x");
+		if (iobase != NULL) {
+			/* write POSIO0R - speed | size | ioport */
+			pci_write_config_dword(dev, ITE_887x_POSIO0,
+				ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED |
+				ITE_887x_POSIO_IOSIZE | inta_addr[i]);
+			/* write INTCBAR - ioport */
+			pci_write_config_dword(dev, ITE_887x_INTCBAR, inta_addr[i]);
+			ret = inb(inta_addr[i]);
+			if (ret != 0xff) {
+				/* ioport connected */
+				break;
+			}
+			release_region(iobase->start, ITE_887x_IOSIZE);
+			iobase = NULL;
+		}
+		i++;
+	}
+
+	if (!inta_addr[i]) {
+		printk(KERN_ERR "ite887x: could not find iobase\n");
+		return -ENODEV;
+	}
+
+	/* start of undocumented type checking (see parport_pc.c) */
+	type = inb(iobase->start + 0x18) & 0x0f;
+
+	switch (type) {
+	case 0x2:	/* ITE8871 (1P) */
+	case 0xa:	/* ITE8875 (1P) */
+		ret = 0;
+		break;
+	case 0xe:	/* ITE8872 (2S1P) */
+		ret = 2;
+		break;
+	case 0x6:	/* ITE8873 (1S) */
+		ret = 1;
+		break;
+	case 0x8:	/* ITE8874 (2S) */
+		ret = 2;
+		break;
+	default:
+		moan_device("Unknown ITE887x", dev);
+		ret = -ENODEV;
+	}
+
+	/* configure all serial ports */
+	for (i = 0; i < ret; i++) {
+		/* read the I/O port from the device */
+		pci_read_config_dword(dev, ITE_887x_PS0BAR + (0x4 * (i + 1)),
+								&ioport);
+		ioport &= 0x0000FF00;	/* the actual base address */
+		pci_write_config_dword(dev, ITE_887x_POSIO0 + (0x4 * (i + 1)),
+			ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED | ioport);
+
+		/* write the ioport to the UARTBAR */
+		pci_read_config_dword(dev, ITE_887x_UARTBAR, &uartbar);
+		uartbar &= ~(15 << (4 * i));	/* clear half the reg */
+		uartbar |= ioport << (16 * i);	/* set the ioport */
+		pci_write_config_dword(dev, ITE_887x_UARTBAR, uartbar);
+
+		/* get current config */
+		pci_read_config_dword(dev, ITE_887x_MISCR, &miscr);
+		/* disable interrupts (UARTx_Routing[3:0]) */
+		miscr &= ~(0xf << (12 - 4 * i));
+		/* activate the UART (UARTx_En) */
+		miscr |= 1 << (23 - i);
+		/* write new config with activated UART */
+		pci_write_config_dword(dev, ITE_887x_MISCR, miscr);
+	}
+
+	if (ret <= 0) {
+		/* the device has no UARTs if we get here */
+		release_region(iobase->start, ITE_887x_IOSIZE);
+	}
+
+	return ret;
+}
+
+static void __devexit pci_ite887x_exit(struct pci_dev *dev)
+{
+	u32 ioport;
+	/* the ioport is bit 0-15 in POSIO0 */
+	pci_read_config_dword(dev, ITE_887x_POSIO0, &ioport);
+	ioport &= 0xffff;
+	release_region(ioport, ITE_887x_IOSIZE);
+}
+
 static int
 pci_default_setup(struct serial_private *priv, struct pciserial_board *board,
 		  struct uart_port *port, int idx)
@@ -654,6 +783,18 @@ static struct pci_serial_quirk pci_seria
 		.setup		= pci_default_setup,
 	},
 	/*
+	 * ITE
+	 */
+	{
+		.vendor		= PCI_VENDOR_ID_ITE,
+		.device		= PCI_DEVICE_ID_ITE_8872,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.init		= pci_ite887x_init,
+		.setup		= pci_default_setup,
+		.exit		= __devexit_p(pci_ite887x_exit),
+	},
+	/*
 	 * Panacom
 	 */
 	{
@@ -927,6 +1068,7 @@ enum pci_board_num_t {
 
 	pbn_b1_2_1250000,
 
+	pbn_b1_bt_1_115200,
 	pbn_b1_bt_2_921600,
 
 	pbn_b1_1_1382400,
@@ -1205,6 +1347,13 @@ static struct pciserial_board pci_boards
 		.uart_offset	= 8,
 	},
 
+	[pbn_b1_bt_1_115200] = {
+		.flags		= FL_BASE1|FL_BASE_BARS,
+		.num_ports	= 1,
+		.base_baud	= 115200,
+		.uart_offset	= 8,
+	},
+
 	[pbn_b1_bt_2_921600] = {
 		.flags		= FL_BASE1|FL_BASE_BARS,
 		.num_ports	= 2,
@@ -2370,6 +2519,13 @@ static struct pci_device_id serial_pci_t
 	{	PCI_VENDOR_ID_TOPIC, PCI_DEVICE_ID_TOPIC_TP560,
 		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 		pbn_b0_1_115200 },
+	/*
+	 * ITE
+	 */
+	{	PCI_VENDOR_ID_ITE, PCI_DEVICE_ID_ITE_8872,
+		PCI_ANY_ID, PCI_ANY_ID,
+		0, 0,
+		pbn_b1_bt_1_115200 },	
 
 	/*
 	 * IntaShield IS-200

Re: [PATCH] Add support for ITE887x serial chipsets

[Niels de Vos]

On Tue, 2007-03-27 at 14:30 -0400, linux@horizon.com wrote:
> Minor point: the chip part numbers are actually IT887x, not ITE887x.

You are correct. Vendor is ITE, product IT887x (id=8872). What is the
advised naming convention for something like this?

> I STFW for a data sheet, but didn't have immediate luck.  Does anyone know where to find
> documentation?

I can't even find the product on the official ITE site... The sheets I
have come from our BIOS development team. I've uploaded them on our FTP
server. The files will be deleted automatically in a few days.

- <ftp://ftp.wincor-nixdorf.com/outgoing/IT8872_v0.2_07182000.PDF>
- <ftp://ftp.wincor-nixdorf.com/outgoing/IT8874F_V0.1_09202001.PDF>

-- 
Niels de Vos <niels.devos@wincor-nixdorf.com>
Wincor Nixdorf International GmbH

Re: [PATCH] Add support for ITE887x serial chipsets

[linux]

Minor point: the chip part numbers are actually IT887x, not ITE887x.

I STFW for a data sheet, but didn't have immediate luck.  Does anyone know where to find
documentation?