PATCH: ncr885e PPC patch look ok to you?

PATCH: ncr885e PPC patch look ok to you?

[Jeff Garzik]

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

Does this patch, against 2.4.0-test1-ac13, look ok to you guys?

It includes the following changes for 2.3/2.4:

* Use init_etherdev's second argument to allocate automatically a
private structure for each board instance.  This eliminates some code
during board init

* do not loop on pci_find_device and then break out of the loop, because
we only support finding and initializing a single board

* use pci_enable_device to wake up device, and turn on PIO and MMIO
areas as necessary

* do not read base address registers and IRQ line directly from
hardware.  Read from struct pci_dev instead.  Use pci_resource_xxx where
appropriate to wrap direct pci-dev struct access.

* use pci_set_master to set busmastering and raise min latency, removing
code which did it "the manual way"

NOTE:  Maybe a PPC kernel expert can tell me whether pci_enable_device
on PPC actually enabled PIO/MMIO regions correctly?  If so, this patch
can, in addition to the change above, also remove the "manual" setting
of the PIO bit in PCI_COMMAND.

Regards,

	Jeff






--
Jeff Garzik              | Liberty is always dangerous, but
Building 1024            | it is the safest thing we have.
MandrakeSoft, Inc.       |      -- Harry Emerson Fosdick

[-- Attachment #2: ncr885e.patch --]
[-- Type: text/plain, Size: 2837 bytes --]

Index: drivers/net/ncr885e.c
===================================================================
RCS file: /g/cvslan/linux_2_3/drivers/net/ncr885e.c,v
retrieving revision 1.1.1.9
diff -u -r1.1.1.9 ncr885e.c
--- drivers/net/ncr885e.c	2000/05/21 10:52:34	1.1.1.9
+++ drivers/net/ncr885e.c	2000/06/11 03:19:15
@@ -1144,18 +1144,11 @@
 	unsigned char *p;
 	int  i;

-	dev = init_etherdev(NULL, 0 );
-
-	/* construct private data for the 885 ethernet */
-	dev->priv = kmalloc( sizeof( struct ncr885e_private ), GFP_KERNEL );
-
-	if ( dev->priv == NULL ) {
-		release_region( ioaddr, NCR885E_TOTAL_SIZE );
+	dev = init_etherdev( NULL, sizeof( struct ncr885e_private ) );
+	if (!dev)
 		return -ENOMEM;
-	}

-	sp = (struct ncr885e_private *) dev->priv;
-	memset( sp, 0, sizeof( struct ncr885e_private ));
+	sp = dev->priv;

 	/* snag the station address and display it */
 	for( i = 0; i < 3; i++ ) {
@@ -1210,7 +1203,8 @@
 	unsigned short cmd;
 	unsigned char irq, latency;

-	while(( pdev = pci_find_device( PCI_VENDOR_ID_NCR,
+	/* use 'if' not 'while' where because driver only supports one device */
+	if (( pdev = pci_find_device( PCI_VENDOR_ID_NCR,
 					PCI_DEVICE_ID_NCR_53C885_ETHERNET,
 					pdev )) != NULL ) {

@@ -1219,11 +1213,13 @@
 			printk( KERN_INFO "%s", version );
 		}

+		if (pci_enable_device(pdev))
+			continue;
+
 		/* Use I/O space */
-		pci_read_config_dword( pdev, PCI_BASE_ADDRESS_0, &ioaddr );
-		pci_read_config_byte( pdev, PCI_INTERRUPT_LINE, &irq );
+		ioaddr = pci_resource_start (pdev, 0);
+		irq = pdev->irq;

-		ioaddr &= ~3;
 		/* Adjust around the Grackle... */
 #ifdef CONFIG_GEMINI
 		ioaddr |= 0xfe000000;
@@ -1237,28 +1233,16 @@

 			chips++;

+			pci_set_master (pdev);
+
 			/* Access is via I/O space, bus master enabled... */
 			pci_read_config_word( pdev, PCI_COMMAND, &cmd );

-			if ( !(cmd & PCI_COMMAND_MASTER) ) {
-				printk( KERN_INFO "  PCI master bit not set! Now setting.\n");
-				cmd |= PCI_COMMAND_MASTER;
-				pci_write_config_word( pdev, PCI_COMMAND, cmd );
-			}
-
 			if ( !(cmd & PCI_COMMAND_IO) ) {
 				printk( KERN_INFO "  Enabling I/O space.\n" );
 				cmd |= PCI_COMMAND_IO;
 				pci_write_config_word( pdev, PCI_COMMAND, cmd );
 			}
-
-			pci_read_config_byte( pdev, PCI_LATENCY_TIMER, &latency );
-
-			if ( latency < 10 ) {
-				printk( KERN_INFO "  PCI latency timer (CFLT) is unreasonably"
-					" low at %d.  Setting to 255.\n", latency );
-				pci_write_config_byte( pdev, PCI_LATENCY_TIMER, 255 );
-			}
 		}
 	}

@@ -1401,14 +1385,10 @@

 static void __exit cleanup_module(void)
 {
-	struct ncr885e_private *np;
-
 	if ( root_dev ) {
-
 		unregister_netdev( root_dev );
-		np = (struct ncr885e_private *) root_dev->priv;
 		release_region( root_dev->base_addr, NCR885E_TOTAL_SIZE );
-		kfree( root_dev->priv );
+		kfree( root_dev );
 		root_dev = NULL;
 	}
 }

Re: PATCH: ncr885e PPC patch look ok to you?

[Michel Lanners]

Hi Jeff,

Still cleaning up, I see?

On  10 Jun, this message from Jeff Garzik echoed through cyberspace:
> Does this patch, against 2.4.0-test1-ac13, look ok to you guys?

Looks good to my, AFAIKT. Only potential problem I see is the PCI IO
region access on PPC machines without ISA, where there has to be a
translation applied. But that will probably be only finally resolved in
2.5...

> NOTE:  Maybe a PPC kernel expert can tell me whether pci_enable_device
> on PPC actually enabled PIO/MMIO regions correctly?  If so, this patch
> can, in addition to the change above, also remove the "manual" setting
> of the PIO bit in PCI_COMMAND.

Not that I would call myself a kernel expert, but yes,
pci_enable_device() works correctly on PPC. So you can remove the manual
IO enable.

Cheers

Michel

-------------------------------------------------------------------------
Michel Lanners                 |  " Read Philosophy.  Study Art.
23, Rue Paul Henkes            |    Ask Questions.  Make Mistakes.
L-1710 Luxembourg              |
email   mlan@cpu.lu            |
http://www.cpu.lu/~mlan        |                     Learn Always. "


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/