Patchwork [RFC] Extend smc911x to support LAN921x chips

login
register
mail settings
Submitter Guennadi Liakhovetski
Date Oct. 10, 2008, 11:33 p.m.
Message ID <Pine.LNX.4.64.0810110125540.10196@axis700.grange>
Download mbox | patch
Permalink /patch/3903/
State RFC
Delegated to: David Miller
Headers show

Comments

Guennadi Liakhovetski - Oct. 10, 2008, 11:33 p.m.
LAN921x controllers from SMSC register cmpatible with LAN911x chips, the 
driver only needs to recognise respective chip IDs. Patch also adds 
network support for the pcm037 board, using this chip.

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>

---

This is an RFC just to let everyone comment on it, it probably would also 
be better to split it into two patches - one only for drivers/net and one 
for arch/arm plus the Kconfig hunk. Based on 2.6.27, would be nice to get 
it into 2.6.28. The CC-list is kept from the previous discussion regarding 
this driver-extension.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Korsgaard - Oct. 11, 2008, 7:22 a.m.
>>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:

Hi,

 Guennadi> LAN921x controllers from SMSC register cmpatible with
 Guennadi> LAN911x chips, the driver only needs to recognise
 Guennadi> respective chip IDs. Patch also adds network support for
 Guennadi> the pcm037 board, using this chip.

 Guennadi> Signed-off-by: Guennadi Liakhovetski <lg@denx.de>

 Guennadi> ---

 Guennadi> This is an RFC just to let everyone comment on it, it
 Guennadi> probably would also be better to split it into two patches
 Guennadi> - one only for drivers/net and one for arch/arm plus the
 Guennadi> Kconfig hunk. Based on 2.6.27, would be nice to get it into
 Guennadi> 2.6.28. The CC-list is kept from the previous discussion
 Guennadi> regarding this driver-extension.

It looks good, but you should definately split out the arm and cleanup
stuff from the new IDs.

 Guennadi> @@ -722,6 +724,9 @@ static void smc911x_phy_detect(struct net_device *dev)
 Guennadi>  						break;
 Guennadi>  					}
 Guennadi>  				}
 Guennadi> +				if (phyaddr < 32)
 Guennadi> +					/* Found an external PHY */
 Guennadi> +					break;

What's this for? Isn't that handled just above?
Guennadi Liakhovetski - Oct. 11, 2008, 9 a.m.
On Sat, 11 Oct 2008, Peter Korsgaard wrote:

> >>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:
> 
> It looks good, but you should definately split out the arm and cleanup
> stuff from the new IDs.

You mean remove the new IDs? Why?

>  Guennadi> @@ -722,6 +724,9 @@ static void smc911x_phy_detect(struct net_device *dev)
>  Guennadi>  						break;
>  Guennadi>  					}
>  Guennadi>  				}
>  Guennadi> +				if (phyaddr < 32)
>  Guennadi> +					/* Found an external PHY */
>  Guennadi> +					break;
> 
> What's this for? Isn't that handled just above?

I think, there's a bug in the code there. The break above terminates the 
loop, yes, but then it falls through in the switch statement to the 
default case and overwrites the just found PHY.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Korsgaard - Oct. 11, 2008, 4:48 p.m.
>>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:

Hi,

 Guennadi> On Sat, 11 Oct 2008, Peter Korsgaard wrote:
 >> >>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:
 >> 
 >> It looks good, but you should definately split out the arm and cleanup
 >> stuff from the new IDs.

 Guennadi> You mean remove the new IDs? Why?

No, just have them in seperate patches - They are independent of each
other, so no need to put in the same patch.

 >> What's this for? Isn't that handled just above?

 Guennadi> I think, there's a bug in the code there. The break above
 Guennadi> terminates the loop, yes, but then it falls through in the
 Guennadi> switch statement to the default case and overwrites the
 Guennadi> just found PHY.

Ahh, the good old break-only-escapes-the-innermost-scope. That's a
good fix, but please send it seperately from the patch adding the new
IDs.
Guennadi Liakhovetski - Oct. 11, 2008, 8:30 p.m.
On Sat, 11 Oct 2008, Peter Korsgaard wrote:

> >>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:
> 
> Hi,
> 
>  Guennadi> On Sat, 11 Oct 2008, Peter Korsgaard wrote:
>  >> >>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:
>  >> 
>  >> It looks good, but you should definately split out the arm and cleanup
>  >> stuff from the new IDs.
> 
>  Guennadi> You mean remove the new IDs? Why?
> 
> No, just have them in seperate patches - They are independent of each
> other, so no need to put in the same patch.

No, they are not independent. smc91x.c is comparing the ID read from the 
hardware with the table, which someone strangely enough for me put in the 
header. So, even if one were to split them, you would have to make it a 
patch series and make the patch for .c depend on the one for .h adding 
IDs. So, no, these two belong into one patch.

>  >> What's this for? Isn't that handled just above?
> 
>  Guennadi> I think, there's a bug in the code there. The break above
>  Guennadi> terminates the loop, yes, but then it falls through in the
>  Guennadi> switch statement to the default case and overwrites the
>  Guennadi> just found PHY.
> 
> Ahh, the good old break-only-escapes-the-innermost-scope. That's a
> good fix, but please send it seperately from the patch adding the new
> IDs.

Ok... it is a simple enough fix, so, one could put them both in one patch 
and be done with them, just explaining both in the patch description, but 
ok, I can make it a separate patch too...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Korsgaard - Oct. 12, 2008, 7:38 a.m.
>>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:

 Guennadi> On Sat, 11 Oct 2008, Peter Korsgaard wrote:
 >> >>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:
 >> 
 >> Hi,
 >> 
 Guennadi> On Sat, 11 Oct 2008, Peter Korsgaard wrote:
 >> >> >>>>> "Guennadi" == Guennadi Liakhovetski <lg@denx.de> writes:
 >> >> 
 >> >> It looks good, but you should definately split out the arm and cleanup
 >> >> stuff from the new IDs.
 >> 
 Guennadi> You mean remove the new IDs? Why?
 >> 
 >> No, just have them in seperate patches - They are independent of each
 >> other, so no need to put in the same patch.

 Guennadi> No, they are not independent. smc91x.c is comparing the ID
 Guennadi> read from the hardware with the table, which someone
 Guennadi> strangely enough for me put in the header. So, even if one
 Guennadi> were to split them, you would have to make it a patch
 Guennadi> series and make the patch for .c depend on the one for .h
 Guennadi> adding IDs. So, no, these two belong into one patch.

I think we're talking past eachother. The changes in smc911x.c and
smc911x.h to support the new IDs should ofcourse be in the same patch,
but the other changes not - E.G. you end up with a patch series like:

- fix ext phy support
- cleanup debug print
- add 921x support
- pcm037 smc911x support

The first 3 goes to Jeff, and the last one to Russell.

 >> Ahh, the good old break-only-escapes-the-innermost-scope. That's a
 >> good fix, but please send it seperately from the patch adding the new
 >> IDs.

 Guennadi> Ok... it is a simple enough fix, so, one could put them
 Guennadi> both in one patch and be done with them, just explaining
 Guennadi> both in the patch description, but ok, I can make it a
 Guennadi> separate patch too...

I would prefer a seperate patch.

Patch

diff --git a/arch/arm/mach-mx3/pcm037.c b/arch/arm/mach-mx3/pcm037.c
index df8582a..cd3f3c2 100644
--- a/arch/arm/mach-mx3/pcm037.c
+++ b/arch/arm/mach-mx3/pcm037.c
@@ -22,6 +22,9 @@ 
 #include <linux/platform_device.h>
 #include <linux/mtd/physmap.h>
 #include <linux/memory.h>
+#include <linux/interrupt.h>
+#include <linux/gpio.h>
+#include <linux/smc911x.h>
 
 #include <mach/hardware.h>
 #include <asm/mach-types.h>
@@ -57,8 +60,37 @@  static struct imxuart_platform_data uart_pdata = {
 	.flags = IMXUART_HAVE_RTSCTS,
 };
 
+static struct resource smc911x_resources[] = {
+	[0] = {
+		.start		= CS1_BASE_ADDR + 0x300,
+		.end		= CS1_BASE_ADDR + 0x300 + SZ_64K - 1,
+		.flags		= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start		= IOMUX_TO_IRQ(MX31_PIN_GPIO3_1),
+		.end		= IOMUX_TO_IRQ(MX31_PIN_GPIO3_1),
+		.flags		= IORESOURCE_IRQ,
+	},
+};
+
+static struct smc911x_platdata smc911x_info = {
+	.flags		= SMC911X_USE_32BIT,
+	.irq_flags	= IRQF_SHARED | IRQF_TRIGGER_LOW,
+};
+
+static struct platform_device pcm037_eth = {
+	.name		= "smc911x",
+	.id		= -1,
+	.num_resources	= ARRAY_SIZE(smc911x_resources),
+	.resource	= smc911x_resources,
+	.dev		= {
+		.platform_data = &smc911x_info,
+	},
+};
+
 static struct platform_device *devices[] __initdata = {
 	&pcm037_flash,
+	&pcm037_eth,
 };
 
 /*
@@ -79,6 +111,11 @@  static void __init mxc_board_init(void)
 	mxc_iomux_mode(MX31_PIN_CSPI3_MISO__TXD3);
 
 	imx_init_uart(2, &uart_pdata);
+
+	/* SMSC9215 IRQ pin */
+	mxc_iomux_mode(IOMUX_MODE(MX31_PIN_GPIO3_1, IOMUX_CONFIG_GPIO));
+	if (!gpio_request(MX31_PIN_GPIO3_1, "pcm037-eth"))
+		gpio_direction_input(MX31_PIN_GPIO3_1);
 }
 
 /*
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 4a11296..c741627 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -960,7 +960,7 @@  config SMC911X
 	tristate "SMSC LAN911[5678] support"
 	select CRC32
 	select MII
-	depends on ARCH_PXA || SUPERH
+	depends on ARCH_PXA || ARCH_MX3 || SUPERH
 	help
 	  This is a driver for SMSC's LAN911x series of Ethernet chipsets
 	  including the new LAN9115, LAN9116, LAN9117, and LAN9118.
diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
index c587162..95b9bfa 100644
--- a/drivers/net/smc911x.c
+++ b/drivers/net/smc911x.c
@@ -685,8 +685,10 @@  static void smc911x_phy_detect(struct net_device *dev)
 	 * PHY#1 to PHY#31, and then PHY#0 last.
 	 */
 	switch(lp->version) {
-		case 0x115:
-		case 0x117:
+		case 0x0115:
+		case 0x0117:
+		case 0x115A:
+		case 0x117A:
 			cfg = SMC_GET_HW_CFG(lp);
 			if (cfg & HW_CFG_EXT_PHY_DET_) {
 				cfg &= ~HW_CFG_PHY_CLK_SEL_;
@@ -722,6 +724,9 @@  static void smc911x_phy_detect(struct net_device *dev)
 						break;
 					}
 				}
+				if (phyaddr < 32)
+					/* Found an external PHY */
+					break;
 			}
 		default:
 			/* Internal media only */
@@ -992,7 +997,7 @@  static void smc911x_phy_interrupt(struct net_device *dev)
 
 	smc911x_phy_check_media(dev, 0);
 	/* read to clear status bits */
-	SMC_GET_PHY_INT_SRC(lp, phyaddr,status);
+	SMC_GET_PHY_INT_SRC(lp, phyaddr, status);
 	DBG(SMC_DEBUG_MISC, "%s: PHY interrupt status 0x%04x\n",
 		dev->name, status & 0xffff);
 	DBG(SMC_DEBUG_MISC, "%s: AFC_CFG 0x%08x\n",
@@ -1030,7 +1035,6 @@  static irqreturn_t smc911x_interrupt(int irq, void *dev_id)
 	/* set a timeout value, so I don't stay here forever */
 	timeout = 8;
 
-
 	do {
 		status = SMC_GET_INT(lp);
 
@@ -1169,12 +1173,10 @@  static irqreturn_t smc911x_interrupt(int irq, void *dev_id)
 	/* restore mask state */
 	SMC_SET_INT_EN(lp, mask);
 
-	DBG(SMC_DEBUG_MISC, "%s: Interrupt done (%d loops)\n",
-		dev->name, 8-timeout);
-
 	spin_unlock_irqrestore(&lp->lock, flags);
 
-	DBG(3, "%s: Interrupt done (%d loops)\n", dev->name, 8-timeout);
+	DBG(SMC_DEBUG_MISC, "%s: Interrupt done (%d loops)\n",
+		dev->name, 8-timeout);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/net/smc911x.h b/drivers/net/smc911x.h
index 2abfc28..d7a7ca3 100644
--- a/drivers/net/smc911x.h
+++ b/drivers/net/smc911x.h
@@ -666,10 +666,14 @@  smc_pxa_dma_outsl(struct smc911x_local *lp, u_long physaddr,
 #define LAN911X_INTERNAL_PHY_ID		(0x0007C000)
 
 /* Chip ID values */
-#define CHIP_9115	0x115
-#define CHIP_9116	0x116
-#define CHIP_9117	0x117
-#define CHIP_9118	0x118
+#define CHIP_9115	0x0115
+#define CHIP_9116	0x0116
+#define CHIP_9117	0x0117
+#define CHIP_9118	0x0118
+#define CHIP_9215	0x115A
+#define CHIP_9216	0x116A
+#define CHIP_9217	0x117A
+#define CHIP_9218	0x118A
 
 struct chip_id {
 	u16 id;
@@ -681,6 +685,10 @@  static const struct chip_id chip_ids[] =  {
 	{ CHIP_9116, "LAN9116" },
 	{ CHIP_9117, "LAN9117" },
 	{ CHIP_9118, "LAN9118" },
+	{ CHIP_9215, "LAN9215" },
+	{ CHIP_9216, "LAN9216" },
+	{ CHIP_9217, "LAN9217" },
+	{ CHIP_9218, "LAN9218" },
 	{ 0, NULL },
 };