| Message ID | Pine.LNX.4.64.0810110125540.10196@axis700.grange |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
>>>>> "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?
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
>>>>> "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.
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
>>>>> "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.
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 }, };
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