[PATCH 2.6] Altix - add ioc3 serial driver support

[PATCH 2.6] Altix - add ioc3 serial driver support

[Pat Gefre]

I have a patch : ftp://oss.sgi.com/projects/sn2/sn2-update/042-ioc3-support

This driver adds support for a 2 port PCI IOC3 serial card on Altix boxes.

Signed-off-by: Patrick Gefre <pfg@sgi.com>

-- 

Patrick Gefre
Silicon Graphics, Inc.                     (E-Mail)  pfg@sgi.com
2750 Blue Water Rd                         (Voice)   (651) 683-3127
Eagan, MN 55121-1400                       (FAX)     (651) 683-3054

Re: [PATCH 2.6] Altix - add ioc3 serial driver support

[Russell King]

On Wed, Jun 22, 2005 at 03:24:05PM -0500, Pat Gefre wrote:
> 
> I have a patch : ftp://oss.sgi.com/projects/sn2/sn2-update/042-ioc3-support
> 
> This driver adds support for a 2 port PCI IOC3 serial card on Altix boxes.
> 
> Signed-off-by: Patrick Gefre <pfg@sgi.com>

Here's some initial comments:

+write_ireg(struct ioc3_mem __iomem *mem, struct ioc3_soft *ioc3_soft,
+				uint32_t val, int which)
...
+	if (!mem || !ioc3_soft)
+		return;

Can either of these really be null here?

+static inline int set_mcr(struct uart_port *the_port, int set,
+			  int mask1, int mask2)
...
+	/* Set new value */
+	if (set) {
+		mcr |= mask1;
+		shadow |= mask2;
+	} else {
+		mcr &= ~mask1;
+		shadow &= ~mask2;
+	}
...
+static void ic3_set_mctrl(struct uart_port *the_port, unsigned int mctrl)
+	unsigned char mcr = 0;
...
+	set_mcr(the_port, 1, mcr, IOC3_SHADOW_DTR);

How can RTS/DTR/OUT1/OUT2/LOOP be disabled if "set" is always passed as '1' ?

+static void
+ioc3_change_speed(struct uart_port *the_port,
+		  struct termios *new_termios, struct termios *old_termios)
...
+	struct uart_info *info = the_port->info;	**
...
+	if (cflag & CRTSCTS) {
+		info->flags |= ASYNC_CTS_FLOW;		**
+		/* enable hardware flow control */
+		port->ip_sscr |= IOC3_SSCR_HFC_EN;
+		writel(port->ip_sscr, &port->ip_serial_regs->sscr);
+	}
+	else {
+		info->flags &= ~ASYNC_CTS_FLOW;		**
+		/* disable hardware flow control */
+		port->ip_sscr &= ~IOC3_SSCR_HFC_EN;
+		writel(port->ip_sscr, &port->ip_serial_regs->sscr);
+	}

The serial core appropriately takes account of this when this structure
exists.  It may not always exist when your change_speed method is called,
so you have a potential oops situation there.

+	baud = uart_get_baud_rate(the_port, new_termios, old_termios,
+				  MIN_BAUD_SUPPORTED, MAX_BAUD_SUPPORTED);
...
+	/* default is 9600 */
+	if (!baud)
+		baud = 9600;

baud being zero should never happen - uart_get_baud_rate tries very
hard to ensure that it returns something sensible, unless 9600 baud
is not covered by the min...max range you gave it.

+static inline int ic3_startup_local(struct uart_port *the_port)
...
+	info = the_port->info;
+	if (info->flags & UIF_INITIALIZED) {
+		NOT_PROGRESS();
+		return retval;
+	}
...
+	ioc3_change_speed(the_port, info->tty->termios, (struct termios *)0);
+
+	info->flags |= UIF_INITIALIZED;

+static void ic3_shutdown(struct uart_port *the_port)
...
+	info = the_port->info;
+
+	if (!(info->flags & UIF_INITIALIZED))
+		return;
...
+	info->flags &= ~UIF_INITIALIZED;

Again, you should not do this.  You're accessing things which you
should not be accessing here.  Please explain why you're doing this.

+static unsigned int ic3_get_mctrl(struct uart_port *the_port)
...
+	if (shadow & IOC3_SHADOW_RTS)
+		ret |= TIOCM_RTS;

The serial core layer returns RTS as appropriate from the settings
it was asked to set.  You should not return the actual setting if
you're using automatic flow control, since that's technically an
API change.

+static int ic3_startup(struct uart_port *the_port)
...
+	if (port->ip_inuse) {
+		NOT_PROGRESS();
+		return -EBUSY;
+	}
...
+		port->ip_inuse = 1;

You're guaranteed to be called exactly once to startup a port, and
exactly once to shut it down before you'll be called to start it
up again.  You should not need to track "inuse"-ness.

+void ioc3_remove_one(struct pci_dev *pdev)

Should be static.

+	if (card_ptr->ic_serial) {
+		release_region((unsigned long)card_ptr->ic_serial,
+			sizeof(struct ioc3_serial));
+	}
+	if (card_ptr->ic_mem) {
+		release_region((unsigned long)card_ptr->ic_mem,
+			sizeof(struct ioc3_mem));
+	}

This seems to imply that ic_serial and ic_mem are IO port regions.
However...

+int __devinit
+ioc3_probe_one(struct pci_dev *pdev, const struct pci_device_id *pci_id)

Should also be static.

+	tmp_addr = pci_resource_start(pdev, 0);
+	if (!request_region(tmp_addr, sizeof(struct ioc3_mem), "sioc3_mem")) {

tmp_addr seems to be an IO port region, but...

+	mem = ioremap(tmp_addr, sizeof(struct ioc3_mem));

you ioremap it, so it must be a MMIO region.  But then...

+	card_ptr->ic_mem = mem;

and in ioc3_remove_one, you release_region this, which is completely
unrelated to that which you claimed.  Plus, I don't see an iounmap in
the remove_one function, so you're leaking memory.  Same goes for
ic_serial and associated code.

+	/* Init the IOC3 */
+	pci_read_config_dword(pdev, PCI_COMMAND, &tmp);
+	pci_write_config_dword(pdev, PCI_COMMAND,
+                               tmp | PCI_COMMAND_MEMORY |
+                               PCI_COMMAND_PARITY | PCI_COMMAND_SERR |
+                               IOC3_PCI_SCR_DROP_MODE_EN);

I think you need to talk to PCI folk about this.  They aren't keen on
drivers reading/writing directly the PCI command register.

+static int __devinit ioc3_detect(void)
...
+	if ((ret = uart_register_driver(&ioc3_uart)) < 0) {
...
+	}
+	return pci_register_driver(&ioc3_s_driver);

If pci_register_driver returns an error, we remove the module without
first unregistering the uart driver.  That's not good.

And one final question: how does ioc3 differ from ioc4?

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

Re: [PATCH 2.6] Altix - add ioc3 serial driver support

[Christoph Hellwig]

On Wed, Jun 22, 2005 at 03:24:05PM -0500, Pat Gefre wrote:
> 
> I have a patch : ftp://oss.sgi.com/projects/sn2/sn2-update/042-ioc3-support
> 
> This driver adds support for a 2 port PCI IOC3 serial card on Altix boxes.

We already have an ioc3 driver, and despite beeing in drivers/net/ it
also registers the uarts.  If the very simple serial support in there
is not enough for you please improve it instead of adding a new separate
driver.  That improvement could involve a split similar to what Brent
did for ioc4 recently.

Re: [PATCH 2.6] Altix - add ioc3 serial driver support

[Pat Gefre]

On Sun, 26 Jun 2005, Christoph Hellwig wrote:

+ On Wed, Jun 22, 2005 at 03:24:05PM -0500, Pat Gefre wrote:
+ > 
+ > I have a patch : ftp://oss.sgi.com/projects/sn2/sn2-update/042-ioc3-support
+ > 
+ > This driver adds support for a 2 port PCI IOC3 serial card on Altix boxes.
+ 
+ We already have an ioc3 driver, and despite beeing in drivers/net/ it
+ also registers the uarts.  If the very simple serial support in there
+ is not enough for you please improve it instead of adding a new separate
+ driver.  That improvement could involve a split similar to what Brent
+ did for ioc4 recently.
+ 

Something I didn't make clear - the driver that I am adding is a pci
card based on the IOC3 serial part - it is a single function card - 2
serial ports. This is supported on Altix.

The driver that is in drivers/net is to support a (full) IOC3 card -
ethernet and serial ports. This is not supported on Altix. Only the
newer IOC4 is supported (it also has serial ports among other things).

They are two different pieces of hardware.

-- Pat

Re: [PATCH 2.6] Altix - add ioc3 serial driver support

[Christoph Hellwig]

On Mon, Jun 27, 2005 at 04:25:48PM -0500, Pat Gefre wrote:
> Something I didn't make clear - the driver that I am adding is a pci
> card based on the IOC3 serial part - it is a single function card - 2
> serial ports. This is supported on Altix.
> 
> The driver that is in drivers/net is to support a (full) IOC3 card -
> ethernet and serial ports. This is not supported on Altix. Only the
> newer IOC4 is supported (it also has serial ports among other things).
> 
> They are two different pieces of hardware.

Actually both of them use the same chip and same pci id (hint: you could
have used the proper PCI ID definition from pci_ids.h instead of adding
your own which is discouraged :)), so it's not as easy.

Stanislaw Skowronek has been done a lot of work on supporting the IOC3
fully, maybe you could get into contact with him?  I'm pretty sure
everyone in mips land is appreciating your fully featured serial
driver, let's just make sure it fits into the general framework.

Re: [PATCH 2.6] Altix - add ioc3 serial driver support

[Pat Gefre]

On Tue, 28 Jun 2005, Stanislaw Skowronek wrote:

+ > > Something I didn't make clear - the driver that I am adding is a pci
+ > > card based on the IOC3 serial part - it is a single function card - 2
+ > > serial ports. This is supported on Altix.
+ 
+ OK. Does it play along with the Ethernet part of the IOC3? And with the
+ pckm part? And with the different devices which hang off the IOC3?
+ (Especially the RTC on Octanes?) If yes, then I'm all over it :)

There is an ioc3 serial part on the same ioc3 as the ethernet - I'm not
sure what the differneces are - I haven't looked at it.

+ 
+ Does your driver use DMA for serial? If not, then it is not really needed
+ as I have a driver that uses 16550-style IRQs.

Yes.

-- Pat