[PATCH] 2.6.10 Altix : ioc4 serial driver support

[PATCH] 2.6.10 Altix : ioc4 serial driver support

[Pat Gefre]

I have a serial driver for Altix I'd like to submit.

The code is at:
ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

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


-- Pat

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.10 Altix : ioc4 serial driver support

[Matthew Wilcox]

On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote:
> I have a serial driver for Altix I'd like to submit.

Why put it in arch/ia64/sn/io/sn2/driver/ioc4_serial.c ?!
drivers/serial/ioc4.c would be the right place for it.  You put the
Kconfig there -- that should be a clue.

It seems like you're directly dereferencing pointers to io memory instead
of calling readb and friends.  I know, this driver doesn't need to be
portable, but it helps any casual reader of this driver figure out what's
going on.  And you can get rid of the 'volatile' that way ;-)

Linux Device Drivers, Second edition says you shouldn't use SA_INTERRUPT
without good reason (http://www.xml.com/ldd/chapter/book/ch09.html#t3)

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Christoph Hellwig]

On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote:
> I have a serial driver for Altix I'd like to submit.
> 
> The code is at:
> ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
> 
> Signed-off-by: Patrick Gefre <pfg@sgi.com>

I took a very short look and what spring to mind first is that the
device probing/remoal is rather bogus.  The ->probe/->remove callbacks
of a PCI driver can be called at any time, and any initialization /
teardown actions must happen from those.  A logical consequence of that
is that a proper PCI driver should have no global state.

I'd also like to second Matthews commens, please move the driver to
drivers/serial and use proper readX/writeX accessors.  Please run the
driver through sparse to find the iomem derferences and possibly other
issues.

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Matthew Wilcox]

On Thu, Dec 16, 2004 at 11:15:19PM +0000, Christoph Hellwig wrote:
> I took a very short look and what spring to mind first is that the

Rats, I'd hoped you'd have time to do a more thorough review.  Here's
some more comments:

Don't define your own names for standard PCI config space -- use the
ones in linux/pci.h instead.  This whole section should be deleted:

+/*
+ * PCI Configuration Space Register Address Map, use offset from IOC4 PCI
+ * configuration base such that this can be used for multiple IOC4s
+ */
+#define IOC4_PCI_SCR	   0x4	/* Status/Command */
+#define IOC4_PCI_REV	   0x8	/* Revision */
+#define IOC4_PCI_LAT	   0xC	/* Latency Timer */
+#define IOC4_PCI_BAR0	   0x10	/* IOC4 base address 0 */
+#define IOC4_PCI_SIDV	   0x2c	/* Subsys ID and vendor */
+#define IOC4_PCI_CAP	   0x34	/* Capability pointer */
+#define IOC4_PCI_LATGNTINT 0x3c	/* Max_lat, min_gnt, int_pin, int_line */


Calling a pci_dev a "pci_handle" is confusing; most code uses "pdev".

+	pci_read_config_dword(pci_handle, IOC4_PCI_SCR, &tmp);
+	pci_write_config_dword(pci_handle, IOC4_PCI_SCR,
+			       tmp | PCI_COMMAND_MASTER |
+			       PCI_COMMAND_MEMORY |
+			       PCI_COMMAND_PARITY | PCI_COMMAND_SERR);

You call pci_set_master() before this which takes care of PCI_COMMAND_MASTER.
You also call pci_enable_device() which calls pcibios_enable_device()
which ensures PCI_COMMAND_MEMORY is set if it needs to be.

So the code above should be:

	pci_read_config_dword(pdev, PCI_COMMAND, &cmd);
	pci_write_config_dword(pdev, PCI_COMMAND, cmd | \
			PCI_COMMAND_PARITY | PCI_COMMAND_SERR);

Personally, I believe we should be setting PCI_COMMAND_PARITY and
PCI_COMMAND_SERR on all devices by default in pcibios_enable_device,
if not in pci_enable_device().  But we don't, so it's fine to do it
in your driver for the moment.


You don't need the:

+	if (!ia64_platform_is("sn2"))
+		return -ENODEV;

since this code will only ever be called if someone has an ioc4 in
their system.  If it's not an sn2, something's very strange ;-)


+struct pci_driver ioc4_s_driver = {
+      name	:"IOC4 Serial",
+      id_table	:ioc4_s_id_table,
+      probe	:ioc4_attach,
+};

please use C99 initialisers instead


+    {PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC4, PCI_ANY_ID, PCI_ANY_ID, 0,0,0},
you don't need the trailing zeroes


I don't see why you need valid_icount_path().  Everywhere it's called,
you seem to have been handed an ioc4_port back by the kernel core.
Are you just checking for data corruption elsewhere, or is this masking
a bug elsewhere in the driver?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Patrick Gefre]

Christoph Hellwig wrote:
> On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote:
> 
>>I have a serial driver for Altix I'd like to submit.
>>
>>The code is at:
>>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>>
>>Signed-off-by: Patrick Gefre <pfg@sgi.com>
> 
> 
> I took a very short look and what spring to mind first is that the
> device probing/remoal is rather bogus.  The ->probe/->remove callbacks
> of a PCI driver can be called at any time, and any initialization /
> teardown actions must happen from those.  A logical consequence of that
> is that a proper PCI driver should have no global state.
> 

Christoph,

I'm not sure what you mean here. I don't have an entry for ->remove and the driver is self-contained.

-- Pat

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Christoph Hellwig]

> I'm not sure what you mean here. I don't have an entry for ->remove and the 
> driver is self-contained.

In a PCI driver (well, just about any driver for a modern bus) you have
an probe and an remove entry.  All code to setup and teardown a device must
happen in those functions, and must not use global state but only the device
instance pointers. 

btw, no need to Cc linux-ia64, there's nothing ia64-specific in this driver.