| Message ID | e8566dc34c4fd544ac630cf7908e4fb613e3ec1a.1300724245.git.LW@KARO-electronics.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Lothar, On Mon, Mar 21, 2011 at 05:37:35PM +0100, Lothar Waßmann wrote: > The variant of the FEC chip found on i.MX28 features two distinct > ethernet MACs that both share a single MII interface. When > deconfiguring the eth0 interface, the MII interface is currently being > switched off, so that the second MAC won't work any more. > I'm wondering what the use case is you intend to support with this patch. > Make sure, the first MAC is never disabled when the second FEC device > is registered. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > --- > drivers/net/fec.c | 38 +++++++++++++++++++++++++++++++++----- > 1 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > index 2436db8..3666524 100644 > --- a/drivers/net/fec.c > +++ b/drivers/net/fec.c > @@ -82,6 +82,9 @@ static unsigned char macaddr[ETH_ALEN]; > module_param_array(macaddr, byte, NULL, 0); > MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > > +static int dual_mac; > +module_param(dual_mac, int, S_IRUGO); > + As you are detecting dual_mac in probe function, is this necessary? > #if defined(CONFIG_M5272) > /* > * Some hardware gets it MAC address out of local flash memory. > @@ -456,6 +459,8 @@ static void > fec_stop(struct net_device *ndev) > { > struct fec_enet_private *fep = netdev_priv(ndev); > + const struct platform_device_id *id_entry = > + platform_get_device_id(fep->pdev); > > /* We cannot expect a graceful transmit stop without link !!! */ > if (fep->link) { > @@ -466,11 +471,22 @@ fec_stop(struct net_device *ndev) > printk("fec_stop : Graceful transmit stop did not complete !\n"); > } > > - /* Whack a reset. We should wait for this. */ > - writel(FEC_ECNTRL_RESET, fep->hwp + FEC_ECNTRL); > - udelay(10); > - writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > - writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); > + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && dual_mac && > + fep->pdev->id == 0) { > + /* clear the RDAR/TDAR bits to stop recv/xmit, > + * but do not reset the controller since the second MAC > + * may still be using the MII interface and requires ETHER_EN > + * on the first MAC to be asserted for MII interrupts! > + */ > + writel(0, fep->hwp + FEC_ECNTRL); > + writel(FEC_ECNTRL_ETHER_EN, fep->hwp + FEC_ECNTRL); > + } else { > + /* Whack a reset. We should wait for this. */ > + writel(FEC_ECNTRL_RESET, fep->hwp + FEC_ECNTRL); > + udelay(10); > + writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > + writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); > + } > } > > static void > @@ -1135,6 +1151,8 @@ static int > fec_enet_open(struct net_device *ndev) > { > struct fec_enet_private *fep = netdev_priv(ndev); > + const struct platform_device_id *id_entry = > + platform_get_device_id(fep->pdev); > int ret; > > /* I should reset the ring buffers here, but I don't yet know > @@ -1145,6 +1163,14 @@ fec_enet_open(struct net_device *ndev) > if (ret) > return ret; > > + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && > + fep->pdev->id == 0) { > + /* ETHER_EN on first MAC has to be asserted > + * in order to generate MII interrupts > + */ > + fec_restart(ndev, 0); > + } > + Is this relevant to the use case you intend to support? Or something is broken without these codes? > /* Probe and connect to PHY when open the interface */ > ret = fec_enet_mii_probe(ndev); > if (ret) { > @@ -1435,6 +1461,8 @@ fec_probe(struct platform_device *pdev) > if (ret) > goto failed_register; > > + if (pdev->id > 0) > + dual_mac = 1; > return 0; > Blank line above 'return 0;'?
Hi, Shawn Guo writes: > Hi Lothar, > > On Mon, Mar 21, 2011 at 05:37:35PM +0100, Lothar Waßmann wrote: > > The variant of the FEC chip found on i.MX28 features two distinct > > ethernet MACs that both share a single MII interface. When > > deconfiguring the eth0 interface, the MII interface is currently being > > switched off, so that the second MAC won't work any more. > > > I'm wondering what the use case is you intend to support with this > patch. > The patch enables the use of both MACs independently from each other. Without this patch deconfiguring eth0 would leave eth1 in an unusable state. > > Make sure, the first MAC is never disabled when the second FEC device > > is registered. > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > --- > > drivers/net/fec.c | 38 +++++++++++++++++++++++++++++++++----- > > 1 files changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > > index 2436db8..3666524 100644 > > --- a/drivers/net/fec.c > > +++ b/drivers/net/fec.c > > @@ -82,6 +82,9 @@ static unsigned char macaddr[ETH_ALEN]; > > module_param_array(macaddr, byte, NULL, 0); > > MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > > > > +static int dual_mac; > > +module_param(dual_mac, int, S_IRUGO); > > + > > As you are detecting dual_mac in probe function, is this necessary? > Not really. I added this is only for debugging. > > #if defined(CONFIG_M5272) > > /* > > * Some hardware gets it MAC address out of local flash memory. > > @@ -456,6 +459,8 @@ static void > > fec_stop(struct net_device *ndev) > > { > > struct fec_enet_private *fep = netdev_priv(ndev); > > + const struct platform_device_id *id_entry = > > + platform_get_device_id(fep->pdev); > > > > /* We cannot expect a graceful transmit stop without link !!! */ > > if (fep->link) { > > @@ -466,11 +471,22 @@ fec_stop(struct net_device *ndev) > > printk("fec_stop : Graceful transmit stop did not complete !\n"); > > } > > > > - /* Whack a reset. We should wait for this. */ > > - writel(FEC_ECNTRL_RESET, fep->hwp + FEC_ECNTRL); > > - udelay(10); > > - writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > > - writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); > > + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && dual_mac && > > + fep->pdev->id == 0) { > > + /* clear the RDAR/TDAR bits to stop recv/xmit, > > + * but do not reset the controller since the second MAC > > + * may still be using the MII interface and requires ETHER_EN > > + * on the first MAC to be asserted for MII interrupts! > > + */ > > + writel(0, fep->hwp + FEC_ECNTRL); > > + writel(FEC_ECNTRL_ETHER_EN, fep->hwp + FEC_ECNTRL); > > + } else { > > + /* Whack a reset. We should wait for this. */ > > + writel(FEC_ECNTRL_RESET, fep->hwp + FEC_ECNTRL); > > + udelay(10); > > + writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > > + writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); > > + } > > } > > > > static void > > @@ -1135,6 +1151,8 @@ static int > > fec_enet_open(struct net_device *ndev) > > { > > struct fec_enet_private *fep = netdev_priv(ndev); > > + const struct platform_device_id *id_entry = > > + platform_get_device_id(fep->pdev); > > int ret; > > > > /* I should reset the ring buffers here, but I don't yet know > > @@ -1145,6 +1163,14 @@ fec_enet_open(struct net_device *ndev) > > if (ret) > > return ret; > > > > + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && > > + fep->pdev->id == 0) { > > + /* ETHER_EN on first MAC has to be asserted > > + * in order to generate MII interrupts > > + */ > > + fec_restart(ndev, 0); > > + } > > + > > Is this relevant to the use case you intend to support? Or something > is broken without these codes? > Actually this is not necessary any more, since with this patch the first MAC is not being disabled on close any more. Looks like a single problem fixed in two different places... > > /* Probe and connect to PHY when open the interface */ > > ret = fec_enet_mii_probe(ndev); > > if (ret) { > > @@ -1435,6 +1461,8 @@ fec_probe(struct platform_device *pdev) > > if (ret) > > goto failed_register; > > > > + if (pdev->id > 0) > > + dual_mac = 1; > > return 0; > > > Blank line above 'return 0;'? > OK. Lothar Waßmann
From: Lothar Waßmann <LW@KARO-electronics.de> Date: Tue, 22 Mar 2011 09:04:00 +0100 > Shawn Guo writes: >> > +static int dual_mac; >> > +module_param(dual_mac, int, S_IRUGO); >> > + >> >> As you are detecting dual_mac in probe function, is this necessary? >> > Not really. I added this is only for debugging. Please do not add driver-specific module parameters. -- 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
On Tue, Mar 22, 2011 at 09:04:00AM +0100, Lothar Waßmann wrote: > Hi, > > Shawn Guo writes: > > Hi Lothar, > > > > On Mon, Mar 21, 2011 at 05:37:35PM +0100, Lothar Waßmann wrote: > > > The variant of the FEC chip found on i.MX28 features two distinct > > > ethernet MACs that both share a single MII interface. When > > > deconfiguring the eth0 interface, the MII interface is currently being > > > switched off, so that the second MAC won't work any more. > > > > > I'm wondering what the use case is you intend to support with this > > patch. > > > The patch enables the use of both MACs independently from each other. > Without this patch deconfiguring eth0 would leave eth1 in an unusable > state. > Ah, I see. Just tested it. It really helps. Thanks for the patch. With the patch cleaned up, please add: Acked-by: <shawn.guo@freescale.com> Tested-by: <shawn.guo@freescale.com>
diff --git a/drivers/net/fec.c b/drivers/net/fec.c index 2436db8..3666524 100644 --- a/drivers/net/fec.c +++ b/drivers/net/fec.c @@ -82,6 +82,9 @@ static unsigned char macaddr[ETH_ALEN]; module_param_array(macaddr, byte, NULL, 0); MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); +static int dual_mac; +module_param(dual_mac, int, S_IRUGO); + #if defined(CONFIG_M5272) /* * Some hardware gets it MAC address out of local flash memory. @@ -456,6 +459,8 @@ static void fec_stop(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); + const struct platform_device_id *id_entry = + platform_get_device_id(fep->pdev); /* We cannot expect a graceful transmit stop without link !!! */ if (fep->link) { @@ -466,11 +471,22 @@ fec_stop(struct net_device *ndev) printk("fec_stop : Graceful transmit stop did not complete !\n"); } - /* Whack a reset. We should wait for this. */ - writel(FEC_ECNTRL_RESET, fep->hwp + FEC_ECNTRL); - udelay(10); - writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); - writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && dual_mac && + fep->pdev->id == 0) { + /* clear the RDAR/TDAR bits to stop recv/xmit, + * but do not reset the controller since the second MAC + * may still be using the MII interface and requires ETHER_EN + * on the first MAC to be asserted for MII interrupts! + */ + writel(0, fep->hwp + FEC_ECNTRL); + writel(FEC_ECNTRL_ETHER_EN, fep->hwp + FEC_ECNTRL); + } else { + /* Whack a reset. We should wait for this. */ + writel(FEC_ECNTRL_RESET, fep->hwp + FEC_ECNTRL); + udelay(10); + writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); + writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); + } } static void @@ -1135,6 +1151,8 @@ static int fec_enet_open(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); + const struct platform_device_id *id_entry = + platform_get_device_id(fep->pdev); int ret; /* I should reset the ring buffers here, but I don't yet know @@ -1145,6 +1163,14 @@ fec_enet_open(struct net_device *ndev) if (ret) return ret; + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && + fep->pdev->id == 0) { + /* ETHER_EN on first MAC has to be asserted + * in order to generate MII interrupts + */ + fec_restart(ndev, 0); + } + /* Probe and connect to PHY when open the interface */ ret = fec_enet_mii_probe(ndev); if (ret) { @@ -1435,6 +1461,8 @@ fec_probe(struct platform_device *pdev) if (ret) goto failed_register; + if (pdev->id > 0) + dual_mac = 1; return 0; failed_register:
The variant of the FEC chip found on i.MX28 features two distinct ethernet MACs that both share a single MII interface. When deconfiguring the eth0 interface, the MII interface is currently being switched off, so that the second MAC won't work any more. Make sure, the first MAC is never disabled when the second FEC device is registered. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- drivers/net/fec.c | 38 +++++++++++++++++++++++++++++++++----- 1 files changed, 33 insertions(+), 5 deletions(-)