Patchwork [NET] several cleanups and bugfixes for fec.c: preserve MII/RMII setting in fec_stop()

login
register
mail settings
Submitter Lothar Waßmann
Date Dec. 6, 2011, 10:27 a.m.
Message ID <d6f4de9e4cfb393754b42815a2ef1a59c96eab83.1323163127.git.LW@KARO-electronics.de>
Download mbox | patch
Permalink /patch/129605/
State New
Headers show

Comments

Lothar Waßmann - Dec. 6, 2011, 10:27 a.m.
Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
functional.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Shawn Guo - Dec. 7, 2011, 9:41 a.m.
On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> setting in FEC_R_CNTRL needs to be preserved to keep the MII interface

s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
for this setting is MII mode.

> functional.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/net/ethernet/freescale/fec.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)

I assume this is fixing a problem you are seeing on imx28 only.
Do you see the problem on imx53/51?

> 
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index 11534b9..ab0afb5 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -515,6 +515,7 @@ 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);
> +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);

This bit is only available on ENET (imx28 and imx6q).  Do we want to
do the same thing for FEC (imx25/27/35/51/53)?

>  
>  	/* We cannot expect a graceful transmit stop without link !!! */
>  	if (fep->link) {
> @@ -531,8 +532,10 @@ fec_stop(struct net_device *ndev)
>  	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
>  
>  	/* We have to keep ENET enabled to have MII interrupt stay working */
> -	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
> +	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
>  		writel(2, fep->hwp + FEC_ECNTRL);
> +		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
> +	}
>  }

On imx6q, we have two bits, bit 6 and 8 of FEC_R_CNTRL, to select MII
interface among MII, RMII and RGMII modes.  I'm not sure if we will
run into the same problem for RGMII mode.  What's your test setup?
I would try to reproduce it here.
Lothar Waßmann - Dec. 7, 2011, 10:42 a.m.
Hi,

Shawn Guo writes:
> On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> > Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> > setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
> 
> s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
> for this setting is MII mode.
> 
> > functional.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/net/ethernet/freescale/fec.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> I assume this is fixing a problem you are seeing on imx28 only.
> Do you see the problem on imx53/51?
> 
No. i.MX53 uses the RMII gasket which is not affected by resetting the
controller. And imMX51 does not support RMII at all.

> > 
> > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > index 11534b9..ab0afb5 100644
> > --- a/drivers/net/ethernet/freescale/fec.c
> > +++ b/drivers/net/ethernet/freescale/fec.c
> > @@ -515,6 +515,7 @@ 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);
> > +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
> 
> This bit is only available on ENET (imx28 and imx6q).  Do we want to
> do the same thing for FEC (imx25/27/35/51/53)?
> 
No. AFAICT that's not necessary there.

> >  	/* We cannot expect a graceful transmit stop without link !!! */
> >  	if (fep->link) {
> > @@ -531,8 +532,10 @@ fec_stop(struct net_device *ndev)
> >  	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
> >  
> >  	/* We have to keep ENET enabled to have MII interrupt stay working */
> > -	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
> > +	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
> >  		writel(2, fep->hwp + FEC_ECNTRL);
> > +		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
> > +	}
> >  }
> 
> On imx6q, we have two bits, bit 6 and 8 of FEC_R_CNTRL, to select MII
> interface among MII, RMII and RGMII modes.  I'm not sure if we will
> run into the same problem for RGMII mode.  What's your test setup?
> I would try to reproduce it here.
> 
I'm using a TX28 which has the Ethernet PHY connected in RMII
mode. You should see the problem, if you unplug the ethernet cable on
a configured link. Without this patch the kernel will not detect when
the cable is plugged back in.


Lothar Waßmann
Lothar Waßmann - Dec. 7, 2011, 10:49 a.m.
Hi,

Shawn Guo writes:
> On Wed, Dec 07, 2011 at 11:42:28AM +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > Shawn Guo writes:
> > > On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> > > > Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> > > > setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
> > > 
> > > s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
> > > for this setting is MII mode.
> > > 
> > > > functional.
> > > > 
> > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > > ---
> > > >  drivers/net/ethernet/freescale/fec.c |    5 ++++-
> > > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > > 
> > > I assume this is fixing a problem you are seeing on imx28 only.
> > > Do you see the problem on imx53/51?
> > > 
> > No. i.MX53 uses the RMII gasket which is not affected by resetting the
> > controller. And imMX51 does not support RMII at all.
> > 
> > > > 
> > > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > > index 11534b9..ab0afb5 100644
> > > > --- a/drivers/net/ethernet/freescale/fec.c
> > > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > > @@ -515,6 +515,7 @@ 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);
> > > > +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
> > > 
> > > This bit is only available on ENET (imx28 and imx6q).  Do we want to
> > > do the same thing for FEC (imx25/27/35/51/53)?
> > > 
> > No. AFAICT that's not necessary there.
> > 
> So you need to check it's actually running on ENET before accessing
> the bit.
> 
That's done in the place where the register is being written:
|	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
|		writel(2, fep->hwp + FEC_ECNTRL);
|		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
|	}

We could save one register read by doing the check also for the read,
but that would further complicate the code.


Lothar Waßmann
Shawn Guo - Dec. 7, 2011, 10:58 a.m.
On Wed, Dec 07, 2011 at 11:42:28AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> > > Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> > > setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
> > 
> > s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
> > for this setting is MII mode.
> > 
> > > functional.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > ---
> > >  drivers/net/ethernet/freescale/fec.c |    5 ++++-
> > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > I assume this is fixing a problem you are seeing on imx28 only.
> > Do you see the problem on imx53/51?
> > 
> No. i.MX53 uses the RMII gasket which is not affected by resetting the
> controller. And imMX51 does not support RMII at all.
> 
> > > 
> > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > index 11534b9..ab0afb5 100644
> > > --- a/drivers/net/ethernet/freescale/fec.c
> > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > @@ -515,6 +515,7 @@ 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);
> > > +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
> > 
> > This bit is only available on ENET (imx28 and imx6q).  Do we want to
> > do the same thing for FEC (imx25/27/35/51/53)?
> > 
> No. AFAICT that's not necessary there.
> 
So you need to check it's actually running on ENET before accessing
the bit.
Shawn Guo - Dec. 7, 2011, 11:15 a.m.
On Wed, Dec 07, 2011 at 11:49:09AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Wed, Dec 07, 2011 at 11:42:28AM +0100, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Shawn Guo writes:
> > > > On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> > > > > Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> > > > > setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
> > > > 
> > > > s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
> > > > for this setting is MII mode.
> > > > 
> > > > > functional.
> > > > > 
> > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > > > ---
> > > > >  drivers/net/ethernet/freescale/fec.c |    5 ++++-
> > > > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > > > 
> > > > I assume this is fixing a problem you are seeing on imx28 only.
> > > > Do you see the problem on imx53/51?
> > > > 
> > > No. i.MX53 uses the RMII gasket which is not affected by resetting the
> > > controller. And imMX51 does not support RMII at all.
> > > 
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > > > index 11534b9..ab0afb5 100644
> > > > > --- a/drivers/net/ethernet/freescale/fec.c
> > > > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > > > @@ -515,6 +515,7 @@ 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);
> > > > > +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
> > > > 
> > > > This bit is only available on ENET (imx28 and imx6q).  Do we want to
> > > > do the same thing for FEC (imx25/27/35/51/53)?
> > > > 
> > > No. AFAICT that's not necessary there.
> > > 
> > So you need to check it's actually running on ENET before accessing
> > the bit.
> > 
> That's done in the place where the register is being written:
> |	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
> |		writel(2, fep->hwp + FEC_ECNTRL);
> |		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
> |	}
> 
> We could save one register read by doing the check also for the read,
> but that would further complicate the code.
> 
Ok.  Will have a test and then get back to you.
Shawn Guo - Dec. 7, 2011, 1:37 p.m.
On Wed, Dec 07, 2011 at 11:42:28AM +0100, Lothar Waßmann wrote:
> I'm using a TX28 which has the Ethernet PHY connected in RMII
> mode. You should see the problem, if you unplug the ethernet cable on
> a configured link. Without this patch the kernel will not detect when
> the cable is plugged back in.
> 
Tested-by: Shawn Guo <shawn.guo@linaro.org>

Patch

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 11534b9..ab0afb5 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -515,6 +515,7 @@  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);
+	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
 
 	/* We cannot expect a graceful transmit stop without link !!! */
 	if (fep->link) {
@@ -531,8 +532,10 @@  fec_stop(struct net_device *ndev)
 	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
 
 	/* We have to keep ENET enabled to have MII interrupt stay working */
-	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
+	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
 		writel(2, fep->hwp + FEC_ECNTRL);
+		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
+	}
 }