Re: [PATCH] Oops! - Re: Power management for au1000_eth.c

[Rodolfo Giometti <giometti@linux.it>]

[-- Attachment #1.1: Type: text/plain, Size: 6811 bytes --]

On Thu, Apr 06, 2006 at 06:43:24PM +0400, Sergei Shtylyov wrote:
> 
>    Actually, the network driver patches should be sent to Jeff Garzik and 
> Andrew Morton.

Ok. Sorry. Hope they can get the patch from this list.

>    Funny, I've also been cooking a patch to straighten Alchemy Ethernet 
> probing code a bit...

:)

> >------------------------------------------------------------------------
> >
> >--- 
> >/home/develop/embedded/mipsel/linux/linux-mips.git/arch/mips/au1000/common/au1xxx_irqmap.c	2006-03-31 16:57:26.000000000 +0200
> >+++ arch/mips/au1000/common/au1xxx_irqmap.c	2006-04-03 
> >17:50:49.000000000 +0200
> >@@ -118,7 +118,7 @@
> > 	{ AU1000_USB_DEV_SUS_INT, INTC_INT_RISE_EDGE, 0 },
> > 	{ AU1000_USB_HOST_INT, INTC_INT_LOW_LEVEL, 0 },
> > 	{ AU1000_ACSYNC_INT, INTC_INT_RISE_EDGE, 0 },
> >-	{ AU1500_MAC0_DMA_INT, INTC_INT_HIGH_LEVEL, 0},
> >+	{ AU1000_MAC0_DMA_INT, INTC_INT_HIGH_LEVEL, 0},
> > 	{ AU1500_MAC1_DMA_INT, INTC_INT_HIGH_LEVEL, 0},
> > 	{ AU1000_AC97C_INT, INTC_INT_RISE_EDGE, 0 },
> > 
> >@@ -152,7 +152,7 @@
> > 	{ AU1000_USB_DEV_SUS_INT, INTC_INT_RISE_EDGE, 0 },
> > 	{ AU1000_USB_HOST_INT, INTC_INT_LOW_LEVEL, 0 },
> > 	{ AU1000_ACSYNC_INT, INTC_INT_RISE_EDGE, 0 },
> >-	{ AU1100_MAC0_DMA_INT, INTC_INT_HIGH_LEVEL, 0},
> >+	{ AU1000_MAC0_DMA_INT, INTC_INT_HIGH_LEVEL, 0},
> > 	/*{ AU1000_GPIO215_208_INT, INTC_INT_HIGH_LEVEL, 0},*/
> > 	{ AU1100_LCD_INT, INTC_INT_HIGH_LEVEL, 0},
> > 	{ AU1000_AC97C_INT, INTC_INT_RISE_EDGE, 0 },
> 
>    Don't think these changes are necessary.

These are necessary to group togheter CPUs that have the same
parameters.

> >--- 
> >/home/develop/embedded/mipsel/linux/linux-mips.git/arch/mips/au1000/common/platform.c	2006-04-03 18:22:05.000000000 +0200
> >+++ arch/mips/au1000/common/platform.c	2006-04-05 
> >23:08:55.000000000 +0200
> >@@ -16,6 +16,78 @@
> > 
> > #include <asm/mach-au1x00/au1xxx.h>
> > 
> >+#if defined(CONFIG_MIPS_AU1X00_ENET) || 
> >defined(CONFIG_MIPS_AU1X00_ENET_MODULE)
> >+/* Ethernet controllers */
> >+static struct resource au1xxx_eth0_resources[] = {
> >+	[0] = {
> >+		.name	= "eth-base",
> >+		.start	= ETH0_BASE,
> >+		.end	= ETH0_BASE + 0x0ffff,
> > +		.flags	= IORESOURCE_MEM,
> > +	},
> 
>    NAK, ETH0_BASE not defined anywhere, and that address differs between 
>    SOCs.
> Note that this must be a *physical* address, not KSEG1-base.

You are right! I forgot to add the attached file... I'm very sorry but
I must work on an older Linux version which still use CVS. I can't
switch to GIT right now and I have to build my patches by hands.

In the attached file you can see that I grouped togheter CPUs that
have the same configuration parameters.

> > 	/* Setup some variables for quick register address access */
> >-	if (ioaddr == iflist[0].base_addr)
> >+	if (port_num == 0)
> 
>    Yep, I disliked this code too. :-)

I just use the "id" filed instaed of using the base address. It seems
to me more readable...

> > 	{
> > 		/* check env variables first */
> > 		if (!get_ethernet_addr(ethaddr)) { 
> >@@ -1495,7 +1426,7 @@
> > 			argptr = prom_getcmdline();
> > 			if ((pmac = strstr(argptr, "ethaddr=")) == NULL) {
> > 				printk(KERN_INFO "%s: No mac address 
> > 				found\n", -						dev->name);
> >+						ndev->name);
> > 				/* use the hard coded mac addresses */
> > 			} else {
> > 				str2eaddr(ethaddr, pmac + 
> > 				strlen("ethaddr="));
> >@@ -1504,26 +1435,26 @@
> > 			}
> > 		}
> > 			aup->enable = (volatile u32 *) 
> >-				((unsigned long)iflist[0].macen_addr);
> >-		memcpy(dev->dev_addr, au1000_mac_addr, 
> >sizeof(au1000_mac_addr));
> >+				((unsigned long) macen_addr);
> 
>    NAK, macen_addr should have been a physical address at that point too
> (if the plarform definitions were correct :-).  Also, this could have been 
> done
> outside of "if".

I just keeped the original structure...

> > 		setup_hw_rings(aup, MAC0_RX_DMA_ADDR, MAC0_TX_DMA_ADDR);
> > 		aup->mac_id = 0;
> > 		au_macs[0] = aup;
> > 	}
> > 		else
> >-	if (ioaddr == iflist[1].base_addr)
> >+	if (port_num == 1)
> > 	{
> > 			aup->enable = (volatile u32 *) 
> >-				((unsigned long)iflist[1].macen_addr);
> >-		memcpy(dev->dev_addr, au1000_mac_addr, 
> >sizeof(au1000_mac_addr));
> >-		dev->dev_addr[4] += 0x10;
> >+				((unsigned long) macen_addr);
> >+		memcpy(ndev->dev_addr, au1000_mac_addr, 
> >sizeof(au1000_mac_addr));
> >+		ndev->dev_addr[4] += 0x10;
> 
>    Actually, the DBAu15x0 boards have their Ethernet addresess differ by 1 
>    in the last byte, not by 0x10 in the next to last one. This code assigns to 
> the port an address different to what's printed on a port's sticker. This 
> should be fixed I guess...

Yes. However this comes from the previous driver version and I'm
working on an Au1100 based board. :)

> >@@ -2255,5 +2162,232 @@
> > 	return 0;
> > }
> > 
> >-module_init(au1000_init_module);
> >-module_exit(au1000_cleanup_module);
> >+/*
> >+ * Setup the base address and interupt of the Au1xxx ethernet macs
> >+ * based on cpu type and whether the interface is enabled in sys_pinfunc
> >+ * register. The last interface is enabled if SYS_PF_NI2 (bit 4) is 0.
> >+ */
> >+static int au1000_drv_probe(struct device *dev)
> >+{
> >+	struct platform_device *pdev = to_platform_device(dev);
> >+	struct net_device *ndev;
> >+	struct au1000_private *aup;
> >+	struct resource *res;
> >+	static unsigned version_printed = 0;
> >+	u32 base_addr, macen_addr;
> >+	int irq, ret;
> >+
> >+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eth-base");
> >+	if (!res) {
> >+		ret = -ENODEV;
> >+		goto out;
> >+	}
> >+	base_addr = res->start;
> >+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eth-mac");
> >+	if (!res) {
> >+		ret = -ENODEV;
> >+		goto out;
> >+	}
> >+	macen_addr = res->start;
> >+	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "eth-irq");
> >+	if (!res) {
> >+		ret = -ENODEV;
> >+		goto out;
> >+	}
> >+	irq = res->start;
> >+
> >+	if (!request_mem_region(CPHYSADDR(base_addr), MAC_IOSIZE, "Au1x00 
> >ENET")) {
> 
>    NAK, base_addr should adready be a physical address. Also, why no 
> request_mem_region()
> for the MAC enable register?

Yes. I forgot to add it.

Thanks for your suggestions! But I'd like to know if I have to change
something and resubmit the patch or these suggestions can be resolved
during the merging of the code...

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

[-- Attachment #1.2: patch-au1000_eth-pm-2 --]
[-- Type: text/plain, Size: 2999 bytes --]

--- /home/develop/embedded/mipsel/linux/linux-mips.git/include/asm-mips/mach-au1x00/au1000.h	2006-04-03 18:24:38.000000000 +0200
+++ include/asm-mips/mach-au1x00/au1000.h	2006-04-06 17:31:04.000000000 +0200
@@ -606,11 +690,10 @@
 #define USB_OHCI_BASE             0x10100000 // phys addr for ioremap
 #define USB_HOST_CONFIG           0xB017fffc
 
-#define AU1000_ETH0_BASE      0xB0500000
-#define AU1000_ETH1_BASE      0xB0510000
-#define AU1000_MAC0_ENABLE       0xB0520000
-#define AU1000_MAC1_ENABLE       0xB0520004
-#define NUM_ETH_INTERFACES 2
+#define ETH0_BASE		  0xB0500000
+#define ETH1_BASE		  0xB0510000
+#define MAC0_ENABLE		  0xB0520000
+#define MAC1_ENABLE		  0xB0520004
 #endif /* CONFIG_SOC_AU1000 */
 
 /* Au1500 */
@@ -635,8 +718,8 @@
 #define AU1000_USB_DEV_SUS_INT    25
 #define AU1000_USB_HOST_INT       26
 #define AU1000_ACSYNC_INT         27
-#define AU1500_MAC0_DMA_INT       28
-#define AU1500_MAC1_DMA_INT       29
+#define AU1000_MAC0_DMA_INT       28
+#define AU1000_MAC1_DMA_INT       29
 #define AU1000_AC97C_INT          31
 #define AU1000_GPIO_0             32
 #define AU1000_GPIO_1             33
@@ -683,11 +766,10 @@
 #define USB_OHCI_BASE             0x10100000 // phys addr for ioremap
 #define USB_HOST_CONFIG           0xB017fffc
 
-#define AU1500_ETH0_BASE	  0xB1500000
-#define AU1500_ETH1_BASE	  0xB1510000
-#define AU1500_MAC0_ENABLE       0xB1520000
-#define AU1500_MAC1_ENABLE       0xB1520004
-#define NUM_ETH_INTERFACES 2
+#define ETH0_BASE		  0xB1500000
+#define ETH1_BASE		  0xB1510000
+#define MAC0_ENABLE		  0xB1520000
+#define MAC1_ENABLE		  0xB1520004
 #endif /* CONFIG_SOC_AU1500 */
 
 /* Au1100 */
@@ -713,8 +795,8 @@
 #define AU1000_USB_DEV_SUS_INT    25
 #define AU1000_USB_HOST_INT       26
 #define AU1000_ACSYNC_INT         27
-#define AU1100_MAC0_DMA_INT       28
-#define	AU1100_GPIO_208_215	29
+#define AU1000_MAC0_DMA_INT	  28
+#define	AU1100_GPIO_208_215	  29
 #define	AU1100_LCD_INT            30
 #define AU1000_AC97C_INT          31
 #define AU1000_GPIO_0             32
@@ -757,8 +839,8 @@
 #define USB_OHCI_BASE             0x10100000 // phys addr for ioremap
 #define USB_HOST_CONFIG           0xB017fffc
 
-#define AU1100_ETH0_BASE	  0xB0500000
-#define AU1100_MAC0_ENABLE       0xB0520000
+#define ETH0_BASE		  0xB0500000
+#define MAC0_ENABLE		  0xB0520000
 #define NUM_ETH_INTERFACES 1
 #endif /* CONFIG_SOC_AU1100 */
 
@@ -841,11 +923,10 @@
 #define USB_OHCI_LEN              0x00060000
 #define USB_HOST_CONFIG           0xB4027ffc
 
-#define AU1550_ETH0_BASE      0xB0500000
-#define AU1550_ETH1_BASE      0xB0510000
-#define AU1550_MAC0_ENABLE       0xB0520000
-#define AU1550_MAC1_ENABLE       0xB0520004
-#define NUM_ETH_INTERFACES 2
+#define ETH0_BASE		  0xB0500000
+#define ETH1_BASE		  0xB0510000
+#define MAC0_ENABLE		  0xB0520000
+#define MAC1_ENABLE		  0xB0520004
 #endif /* CONFIG_SOC_AU1550 */
 
 #ifdef CONFIG_SOC_AU1200

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