Skip to contentSkip to conversation

: mv643xx_eth_pcidev - implements hotplug for the marvell gige functionality by probing the northbridge pci id.

PATCH In review Sven Luther 6 participants 12 messages Oct 25, 2005 – Nov 1, 2005 Network drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 : mv643xx_eth_pcidev - implements hotplug for the marvell gige functionality by probing the northbridge pci id.

Add Marvell gigabit ethernet hotplug through the northbridge pci id support for the Pegasos machines.

---
This patch is not optimal, as the proper way to handling this is to bring full hotplug support to the platform drivers, but this would be much more invasive and far reaching a project. This approach matches the marvell discovery northbridge pci id to load the driver for the builtin gigabit ethernet function, and is similar of what is done already using the discover module loading technique. With more and more distributions moving to using hotplug only to probe for modules to load, this patch becomes necessary.

Friendly,

Sven Luther

drivers/net/mv643xx_eth.c +14 -0
diff -Naur a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
@@ -36,6 +36,10 @@
 #include <linux/udp.h>
 #include <linux/etherdevice.h>
 
+#ifdef CONFIG_PPC_MULTIPLATFORM
+#include <linux/pci.h>
+#endif
+
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/ethtool.h>
@@ -1640,6 +1644,16 @@
 		" and Dale Farnsworth");
 MODULE_DESCRIPTION("Ethernet driver for Marvell MV643XX");
 
+#ifdef CONFIG_PPC_MULTIPLATFORM
+static struct pci_device_id pci_marvell_mv64360[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64360) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(pci, pci_marvell_mv64360);
+#endif
+
+
 /*
  * The second part is the low level driver of the gigE ethernet ports.
  */
#1 lore raw
Quoted 40 lines · Sven Luther

Add Marvell gigabit ethernet hotplug through the northbridge pci id support for the Pegasos machines.

---
This patch is not optimal, as the proper way to handling this is to bring full hotplug support to the platform drivers, but this would be much more invasive and far reaching a project. This approach matches the marvell discovery northbridge pci id to load the driver for the builtin gigabit ethernet function, and is similar of what is done already using the discover module loading technique. With more and more distributions moving to using hotplug only to probe for modules to load, this patch becomes necessary.

Friendly,

Sven Luther

diff -Naur a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c

drivers/net/mv643xx_eth.c +7 -0
@@ -36,6 +36,10 @@
 #include <linux/udp.h>
 #include <linux/etherdevice.h>
 
+#ifdef CONFIG_PPC_MULTIPLATFORM
+#include <linux/pci.h>
+#endif
+
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/ethtool.h>
@@ -1640,6 +1644,16 @@
 		" and Dale Farnsworth");
 MODULE_DESCRIPTION("Ethernet driver for Marvell MV643XX");
 
+#ifdef CONFIG_PPC_MULTIPLATFORM
+static struct pci_device_id pci_marvell_mv64360[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64360) },

drivers/net/mv643xx_eth.c line 1659

Quoted 10 lines
drivers/net/mv643xx_eth.c +7 -0
+	{ }
+};
+
+MODULE_DEVICE_TABLE(pci, pci_marvell_mv64360);
+#endif
+
+
 /*
  * The second part is the low level driver of the gigE ethernet ports.
  */

I don't think this is really dependent on PPC_MULTIPLATFORM, and the driver also supports 64460.

How about the patch below.
-Dale

drivers/net/mv643xx_eth.c +9 -0
Index: linux-2.6.14-rc2-mm1-mv643xx-enet/drivers/net/mv643xx_eth.c
===================================================================
@@ -39,6 +39,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/ethtool.h>
+#include <linux/pci.h>
 #include <asm/io.h>
 #include <asm/types.h>
 #include <asm/pgtable.h>
@@ -1597,6 +1598,14 @@ MODULE_AUTHOR(	"Rabeeh Khoury, Assaf Hof
 		" and Dale Farnsworth");
 MODULE_DESCRIPTION("Ethernet driver for Marvell MV643XX");
 
+static struct pci_device_id pci_mv64xxx_enet[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64360) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64460) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(pci, pci_mv64xxx_enet);
+
 /*
  * The second part is the low level driver of the gigE ethernet ports.
  */
#2 lore raw
Quoted 38 lines · Dale Farnsworth
Quoted 37 lines · Sven Luther

Add Marvell gigabit ethernet hotplug through the northbridge pci id support for the Pegasos machines.

---
This patch is not optimal, as the proper way to handling this is to bring full hotplug support to the platform drivers, but this would be much more invasive and far reaching a project. This approach matches the marvell discovery northbridge pci id to load the driver for the builtin gigabit ethernet function, and is similar of what is done already using the discover module loading technique. With more and more distributions moving to using hotplug only to probe for modules to load, this patch becomes necessary.

Friendly,

Sven Luther

diff -Naur a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c

drivers/net/mv643xx_eth.c +4 -0
@@ -36,6 +36,10 @@
 #include <linux/udp.h>
 #include <linux/etherdevice.h>
 
+#ifdef CONFIG_PPC_MULTIPLATFORM
+#include <linux/pci.h>
+#endif
+
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/ethtool.h>
@@ -1640,6 +1644,16 @@
 		" and Dale Farnsworth");
 MODULE_DESCRIPTION("Ethernet driver for Marvell MV643XX");
 
Quoted 16 lines

drivers/net/mv643xx_eth.c MODULE_AUTHOR( "Rabeeh Khoury, Assaf Hof · line 1611

Quoted 13 lines
drivers/net/mv643xx_eth.c +10 -0
+#ifdef CONFIG_PPC_MULTIPLATFORM
+static struct pci_device_id pci_marvell_mv64360[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64360) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(pci, pci_marvell_mv64360);
+#endif
+
+
 /*
  * The second part is the low level driver of the gigE ethernet ports.
  */

I don't think this is really dependent on PPC_MULTIPLATFORM, and the driver also supports 64460.

Ah, well, the PPC_MULTIPLATFORM was in order to not break potentially other plateforms i know nothing about, and yes, the below patch would be perfect too.

Friendly,

Sven Luther

#4 lore raw
Quoted 4 lines · Christoph Hellwig
Quoted 2 lines

I don't think this is really dependent on PPC_MULTIPLATFORM, and the driver also supports 64460.

This is not a PCI driver so adding a device table is wrong.

Sure, please provide us with a patch to add hotplug support to those platform drivers who are not pci :)

In the meantime, this is no worse than how this was done previously, namely having discover match the northbridge pci id.

Friendly,

Sven Luther

#5 lore raw
In reply to Christoph Hellwig
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines

I don't think this is really dependent on PPC_MULTIPLATFORM, and the driver also supports 64460.

This is not a PCI driver so adding a device table is wrong.

I think we all agree ;-).

The point is to have a 'quick' way to get this module automagicly loaded until we get a proper super duper OFW devices hotplug or whatsoeever support.

Regards

#6 lore raw
In reply to Sven Luther

drivers/net/mv643xx_eth.c line 1649

Quoted 2 lines · Sven Luther
drivers/net/mv643xx_eth.c +2 -0
+static struct pci_device_id pci_marvell_mv64360[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64360) },

Are there other devices in this northbridge whose drivers might also want to bind to this PCI id?

What other devices does this northbridge contain?

Paul.

#7 lore raw
Quoted 9 lines · Paul Mackerras

drivers/net/mv643xx_eth.c line 1649

Quoted 2 lines · Sven Luther
drivers/net/mv643xx_eth.c +2 -0
+static struct pci_device_id pci_marvell_mv64360[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64360) },

Are there other devices in this northbridge whose drivers might also want to bind to this PCI id?

What other devices does this northbridge contain?

Yep, Dale proposed to add :

+ { PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64460) },

Which i was about to resubmit the patch when i saw the commit, ... Err, no i misunderstood your question.

Other devices, well, there is the interrupt controller, some dma engine and the builtin sram, but nothing to my knowledge which would benefit from being bound to the pci id.

Friendly,

Sven Luther

#8 lore raw
In reply to Paul Mackerras
Quoted 9 lines · Paul Mackerras

drivers/net/mv643xx_eth.c line 1649

Quoted 2 lines · Sven Luther
drivers/net/mv643xx_eth.c +2 -0
+static struct pci_device_id pci_marvell_mv64360[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64360) },

Are there other devices in this northbridge whose drivers might also want to bind to this PCI id?

What other devices does this northbridge contain?

It does have a PIC, DMA engine, and SRAM that Sven mentioned but there are also a serial controller (MPSC), an i2c controller, and a watchdog timer. I would guess that the MPSC and i2c drivers, at least, should use MODULE_DEVICE_TABLE. If you agree, I'll add it to the list.

Mark

#9 lore raw
Quoted 9 lines · Mark A. Greer
Quoted 8 lines · Paul Mackerras

drivers/net/mv643xx_eth.c line 1649

Quoted 2 lines · Sven Luther
drivers/net/mv643xx_eth.c +2 -0
+static struct pci_device_id pci_marvell_mv64360[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL, PCI_DEVICE_ID_MARVELL_MV64360) },

Are there other devices in this northbridge whose drivers might also want to bind to this PCI id?

Quoted 6 lines
Quoted 1 line

What other devices does this northbridge contain?

It does have a PIC, DMA engine, and SRAM that Sven mentioned but there are also a serial controller (MPSC), an i2c controller, and a watchdog timer. I would guess that the MPSC and i2c drivers, at least, should use MODULE_DEVICE_TABLE. If you agree, I'll add it to the list.

Yeah, forgot those, we don't use them on the pegasos though.

Friendly,

Sven Luther

#10 lore raw
In reply to Mark A. Greer
Quoted 4 lines · Mark A. Greer

It does have a PIC, DMA engine, and SRAM that Sven mentioned but there are also a serial controller (MPSC), an i2c controller, and a watchdog timer. I would guess that the MPSC and i2c drivers, at least, should use MODULE_DEVICE_TABLE. If you agree, I'll add it to the list.

But the point is that if one driver has claimed the device (because its ID is in its module device table), the other drivers won't be able to. You need something like the macio driver that BenH did, which claims the macio device and creates a macio bus structure and allows individual macio device drivers to claim the pieces of it.

Paul.

#11 lore raw
Quoted 10 lines · Paul Mackerras
Quoted 4 lines · Mark A. Greer

It does have a PIC, DMA engine, and SRAM that Sven mentioned but there are also a serial controller (MPSC), an i2c controller, and a watchdog timer. I would guess that the MPSC and i2c drivers, at least, should use MODULE_DEVICE_TABLE. If you agree, I'll add it to the list.

But the point is that if one driver has claimed the device (because its ID is in its module device table), the other drivers won't be able to.

Okay. TBH, I still have to look at what the MODULE_DEVICE_TABLE does so it was ignorance + laziness on my part.

Quoted 3 lines

You need something like the macio driver that BenH did, which claims the macio device and creates a macio bus structure and allows individual macio device drivers to claim the pieces of it.

I'll take a look.

Mark