[PATCH 0/6] Fixes for davinci_emac

[PATCH 0/6] Fixes for davinci_emac

[Tony Lindgren]

Hi,

Here are some fixes for davinci_emac for the issues I've noticed
recently.

Regards,

Tony

Tony Lindgren (6):
  net: davinci_emac: Fix hangs with interrupts
  net: davinci_emac: Fix runtime pm calls for davinci_emac
  net: davinci_emac: Free clock after checking the frequency
  net: davinci_emac: Fix incomplete code for getting the phy from device
    tree
  net: davinci_emac: Fix ioremap for devices with MDIO within the EMAC
    address space
  net: davinci_emac: Add support for emac on dm816x

 .../devicetree/bindings/net/davinci_emac.txt       |  3 +-
 drivers/net/ethernet/ti/davinci_emac.c             | 77 ++++++++++++++++------
 2 files changed, 58 insertions(+), 22 deletions(-)

-- 
2.1.4

[PATCH 1/6] net: davinci_emac: Fix hangs with interrupts

[Tony Lindgren]

On davinci_emac, we have pulse interrupts. This means that we need to
clear the EOI bits when disabling interrupts as otherwise the interrupts
keep happening. And we also need to not clear the EOI bits again when
enabling the interrupts as otherwise we will get tons of:

unexpected IRQ trap at vector 00

These errors almost certainly mean that the omap-intc.c is signaling
a spurious interrupt with the reserved irq 127 as we've seen earlier
on omap3.

Let's fix the issue by clearing the EOI bits when disabling the
interrupts. Let's also keep the comment for "Rx Threshold and Misc
interrupts are not enabled" for both enable and disable so people
are aware of this when potentially adding more support.

Note that eventually we should handle the RX and TX interrupts
separately like cpsw is now doing. However, so far I have not seen
any issues with this based on my testing, so it seems to behave a
little different compared to the cpsw that had a similar issue.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/ethernet/ti/davinci_emac.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index ea71251..383ed52 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -922,6 +922,16 @@ static void emac_int_disable(struct emac_priv *priv)
 		if (priv->int_disable)
 			priv->int_disable();
 
+		/* NOTE: Rx Threshold and Misc interrupts are not enabled */
+
+		/* ack rxen only then a new pulse will be generated */
+		emac_write(EMAC_DM646X_MACEOIVECTOR,
+			EMAC_DM646X_MAC_EOI_C0_RXEN);
+
+		/* ack txen- only then a new pulse will be generated */
+		emac_write(EMAC_DM646X_MACEOIVECTOR,
+			EMAC_DM646X_MAC_EOI_C0_TXEN);
+
 		local_irq_restore(flags);
 
 	} else {
@@ -951,15 +961,6 @@ static void emac_int_enable(struct emac_priv *priv)
 		 * register */
 
 		/* NOTE: Rx Threshold and Misc interrupts are not enabled */
-
-		/* ack rxen only then a new pulse will be generated */
-		emac_write(EMAC_DM646X_MACEOIVECTOR,
-			EMAC_DM646X_MAC_EOI_C0_RXEN);
-
-		/* ack txen- only then a new pulse will be generated */
-		emac_write(EMAC_DM646X_MACEOIVECTOR,
-			EMAC_DM646X_MAC_EOI_C0_TXEN);
-
 	} else {
 		/* Set DM644x control registers for interrupt control */
 		emac_ctrl_write(EMAC_CTRL_EWCTL, 0x1);
-- 
2.1.4

Re: [PATCH 1/6] net: davinci_emac: Fix hangs with interrupts

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On Tue, Jan 13, 2015 at 11:29:23AM -0800, Tony Lindgren wrote:
> On davinci_emac, we have pulse interrupts. This means that we need to
> clear the EOI bits when disabling interrupts as otherwise the interrupts
> keep happening. And we also need to not clear the EOI bits again when
> enabling the interrupts as otherwise we will get tons of:
> 
> unexpected IRQ trap at vector 00
> 
> These errors almost certainly mean that the omap-intc.c is signaling
> a spurious interrupt with the reserved irq 127 as we've seen earlier
> on omap3.
> 
> Let's fix the issue by clearing the EOI bits when disabling the
> interrupts. Let's also keep the comment for "Rx Threshold and Misc
> interrupts are not enabled" for both enable and disable so people
> are aware of this when potentially adding more support.
> 
> Note that eventually we should handle the RX and TX interrupts
> separately like cpsw is now doing. However, so far I have not seen
> any issues with this based on my testing, so it seems to behave a
> little different compared to the cpsw that had a similar issue.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>

pretty much the same thing that happens with CPSW, I think that a future
patch might want to change things so that we only write EOI to the IRQ
that actually fires, though.

Reviewed-by: Felipe Balbi <balbi@ti.com>

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/net/ethernet/ti/davinci_emac.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index ea71251..383ed52 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -922,6 +922,16 @@ static void emac_int_disable(struct emac_priv *priv)
>  		if (priv->int_disable)
>  			priv->int_disable();
>  
> +		/* NOTE: Rx Threshold and Misc interrupts are not enabled */
> +
> +		/* ack rxen only then a new pulse will be generated */
> +		emac_write(EMAC_DM646X_MACEOIVECTOR,
> +			EMAC_DM646X_MAC_EOI_C0_RXEN);
> +
> +		/* ack txen- only then a new pulse will be generated */
> +		emac_write(EMAC_DM646X_MACEOIVECTOR,
> +			EMAC_DM646X_MAC_EOI_C0_TXEN);
> +
>  		local_irq_restore(flags);
>  
>  	} else {
> @@ -951,15 +961,6 @@ static void emac_int_enable(struct emac_priv *priv)
>  		 * register */
>  
>  		/* NOTE: Rx Threshold and Misc interrupts are not enabled */
> -
> -		/* ack rxen only then a new pulse will be generated */
> -		emac_write(EMAC_DM646X_MACEOIVECTOR,
> -			EMAC_DM646X_MAC_EOI_C0_RXEN);
> -
> -		/* ack txen- only then a new pulse will be generated */
> -		emac_write(EMAC_DM646X_MACEOIVECTOR,
> -			EMAC_DM646X_MAC_EOI_C0_TXEN);
> -
>  	} else {
>  		/* Set DM644x control registers for interrupt control */
>  		emac_ctrl_write(EMAC_CTRL_EWCTL, 0x1);
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] net: davinci_emac: Fix hangs with interrupts

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On Tue, Jan 13, 2015 at 11:29:23AM -0800, Tony Lindgren wrote:
> On davinci_emac, we have pulse interrupts. This means that we need to
> clear the EOI bits when disabling interrupts as otherwise the interrupts
> keep happening. And we also need to not clear the EOI bits again when
> enabling the interrupts as otherwise we will get tons of:
> 
> unexpected IRQ trap at vector 00
> 
> These errors almost certainly mean that the omap-intc.c is signaling
> a spurious interrupt with the reserved irq 127 as we've seen earlier
> on omap3.
> 
> Let's fix the issue by clearing the EOI bits when disabling the
> interrupts. Let's also keep the comment for "Rx Threshold and Misc
> interrupts are not enabled" for both enable and disable so people
> are aware of this when potentially adding more support.
> 
> Note that eventually we should handle the RX and TX interrupts
> separately like cpsw is now doing. However, so far I have not seen
> any issues with this based on my testing, so it seems to behave a
> little different compared to the cpsw that had a similar issue.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>

pretty much the same thing that happens with CPSW, I think that a future
patch might want to change things so that we only write EOI to the IRQ
that actually fires, though.

Reviewed-by: Felipe Balbi <balbi@ti.com>

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/net/ethernet/ti/davinci_emac.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index ea71251..383ed52 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -922,6 +922,16 @@ static void emac_int_disable(struct emac_priv *priv)
>  		if (priv->int_disable)
>  			priv->int_disable();
>  
> +		/* NOTE: Rx Threshold and Misc interrupts are not enabled */
> +
> +		/* ack rxen only then a new pulse will be generated */
> +		emac_write(EMAC_DM646X_MACEOIVECTOR,
> +			EMAC_DM646X_MAC_EOI_C0_RXEN);
> +
> +		/* ack txen- only then a new pulse will be generated */
> +		emac_write(EMAC_DM646X_MACEOIVECTOR,
> +			EMAC_DM646X_MAC_EOI_C0_TXEN);
> +
>  		local_irq_restore(flags);
>  
>  	} else {
> @@ -951,15 +961,6 @@ static void emac_int_enable(struct emac_priv *priv)
>  		 * register */
>  
>  		/* NOTE: Rx Threshold and Misc interrupts are not enabled */
> -
> -		/* ack rxen only then a new pulse will be generated */
> -		emac_write(EMAC_DM646X_MACEOIVECTOR,
> -			EMAC_DM646X_MAC_EOI_C0_RXEN);
> -
> -		/* ack txen- only then a new pulse will be generated */
> -		emac_write(EMAC_DM646X_MACEOIVECTOR,
> -			EMAC_DM646X_MAC_EOI_C0_TXEN);
> -
>  	} else {
>  		/* Set DM644x control registers for interrupt control */
>  		emac_ctrl_write(EMAC_CTRL_EWCTL, 0x1);
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] net: davinci_emac: Fix hangs with interrupts

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [150113 11:40]:
> On Tue, Jan 13, 2015 at 11:29:23AM -0800, Tony Lindgren wrote:
> > 
> > Note that eventually we should handle the RX and TX interrupts
> > separately like cpsw is now doing. However, so far I have not seen
> > any issues with this based on my testing, so it seems to behave a
> > little different compared to the cpsw that had a similar issue.
> 
> pretty much the same thing that happens with CPSW, I think that a future
> patch might want to change things so that we only write EOI to the IRQ
> that actually fires, though.

Yeah I agree that's totally the way to go for the future. I've tried to
expose that issue here with nuttcp but so far no luck.

Regards,

Tony 

[PATCH 2/6] net: davinci_emac: Fix runtime pm calls for davinci_emac

[Tony Lindgren]

Commit 3ba97381343b ("net: ethernet: davinci_emac: add pm_runtime support")
added support for runtime PM, but it causes issues on omap3 related devices
that actually gate the clocks:

Unhandled fault: external abort on non-linefetch (0x1008)
...
[<c04160f0>] (emac_dev_getnetstats) from [<c04d6a3c>] (dev_get_stats+0x78/0xc8)
[<c04d6a3c>] (dev_get_stats) from [<c04e9ccc>] (rtnl_fill_ifinfo+0x3b8/0x938)
[<c04e9ccc>] (rtnl_fill_ifinfo) from [<c04eade4>] (rtmsg_ifinfo+0x68/0xd8)
[<c04eade4>] (rtmsg_ifinfo) from [<c04dd35c>] (register_netdevice+0x3a0/0x4ec)
[<c04dd35c>] (register_netdevice) from [<c04dd4bc>] (register_netdev+0x14/0x24)
[<c04dd4bc>] (register_netdev) from [<c041755c>] (davinci_emac_probe+0x408/0x5c8)
[<c041755c>] (davinci_emac_probe) from [<c0396d78>] (platform_drv_probe+0x48/0xa4)

Let's fix it by moving the pm_runtime_get() call earlier, and also
add it to the emac_dev_getnetstats(). Also note that we want to use
pm_rutime_get_sync() as we don't want to have deferred_resume happen.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Mark A. Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/ethernet/ti/davinci_emac.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 383ed52..deb43b3 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1538,7 +1538,7 @@ static int emac_dev_open(struct net_device *ndev)
 	int i = 0;
 	struct emac_priv *priv = netdev_priv(ndev);
 
-	pm_runtime_get(&priv->pdev->dev);
+	pm_runtime_get_sync(&priv->pdev->dev);
 
 	netif_carrier_off(ndev);
 	for (cnt = 0; cnt < ETH_ALEN; cnt++)
@@ -1726,6 +1726,8 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
 	u32 mac_control;
 	u32 stats_clear_mask;
 
+	pm_runtime_get_sync(&priv->pdev->dev);
+
 	/* update emac hardware stats and reset the registers*/
 
 	mac_control = emac_read(EMAC_MACCONTROL);
@@ -1767,6 +1769,8 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
 	ndev->stats.tx_fifo_errors += emac_read(EMAC_TXUNDERRUN);
 	emac_write(EMAC_TXUNDERRUN, stats_clear_mask);
 
+	pm_runtime_put(&priv->pdev->dev);
+
 	return &ndev->stats;
 }
 
@@ -1981,12 +1985,16 @@ static int davinci_emac_probe(struct platform_device *pdev)
 	ndev->ethtool_ops = &ethtool_ops;
 	netif_napi_add(ndev, &priv->napi, emac_poll, EMAC_POLL_WEIGHT);
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	/* register the network device */
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 	rc = register_netdev(ndev);
 	if (rc) {
 		dev_err(&pdev->dev, "error in register_netdev\n");
 		rc = -ENODEV;
+		pm_runtime_put(&pdev->dev);
 		goto no_cpdma_chan;
 	}
 
@@ -1996,9 +2004,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
 			   "(regs: %p, irq: %d)\n",
 			   (void *)priv->emac_base_phys, ndev->irq);
 	}
-
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_resume(&pdev->dev);
+	pm_runtime_put(&pdev->dev);
 
 	return 0;
 
-- 
2.1.4

Re: [PATCH 2/6] net: davinci_emac: Fix runtime pm calls for davinci_emac

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

On Tue, Jan 13, 2015 at 11:29:24AM -0800, Tony Lindgren wrote:
> Commit 3ba97381343b ("net: ethernet: davinci_emac: add pm_runtime support")
> added support for runtime PM, but it causes issues on omap3 related devices
> that actually gate the clocks:
> 
> Unhandled fault: external abort on non-linefetch (0x1008)
> ...
> [<c04160f0>] (emac_dev_getnetstats) from [<c04d6a3c>] (dev_get_stats+0x78/0xc8)
> [<c04d6a3c>] (dev_get_stats) from [<c04e9ccc>] (rtnl_fill_ifinfo+0x3b8/0x938)
> [<c04e9ccc>] (rtnl_fill_ifinfo) from [<c04eade4>] (rtmsg_ifinfo+0x68/0xd8)
> [<c04eade4>] (rtmsg_ifinfo) from [<c04dd35c>] (register_netdevice+0x3a0/0x4ec)
> [<c04dd35c>] (register_netdevice) from [<c04dd4bc>] (register_netdev+0x14/0x24)
> [<c04dd4bc>] (register_netdev) from [<c041755c>] (davinci_emac_probe+0x408/0x5c8)
> [<c041755c>] (davinci_emac_probe) from [<c0396d78>] (platform_drv_probe+0x48/0xa4)
> 
> Let's fix it by moving the pm_runtime_get() call earlier, and also
> add it to the emac_dev_getnetstats(). Also note that we want to use
> pm_rutime_get_sync() as we don't want to have deferred_resume happen.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/net/ethernet/ti/davinci_emac.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index 383ed52..deb43b3 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1538,7 +1538,7 @@ static int emac_dev_open(struct net_device *ndev)
>  	int i = 0;
>  	struct emac_priv *priv = netdev_priv(ndev);
>  
> -	pm_runtime_get(&priv->pdev->dev);
> +	pm_runtime_get_sync(&priv->pdev->dev);

gotta check return value on all pm_runtime_get_sync() calls. IIRC,
there's a coccinelle script for checking and patching this.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/6] net: davinci_emac: Fix runtime pm calls for davinci_emac

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

On Tue, Jan 13, 2015 at 11:29:24AM -0800, Tony Lindgren wrote:
> Commit 3ba97381343b ("net: ethernet: davinci_emac: add pm_runtime support")
> added support for runtime PM, but it causes issues on omap3 related devices
> that actually gate the clocks:
> 
> Unhandled fault: external abort on non-linefetch (0x1008)
> ...
> [<c04160f0>] (emac_dev_getnetstats) from [<c04d6a3c>] (dev_get_stats+0x78/0xc8)
> [<c04d6a3c>] (dev_get_stats) from [<c04e9ccc>] (rtnl_fill_ifinfo+0x3b8/0x938)
> [<c04e9ccc>] (rtnl_fill_ifinfo) from [<c04eade4>] (rtmsg_ifinfo+0x68/0xd8)
> [<c04eade4>] (rtmsg_ifinfo) from [<c04dd35c>] (register_netdevice+0x3a0/0x4ec)
> [<c04dd35c>] (register_netdevice) from [<c04dd4bc>] (register_netdev+0x14/0x24)
> [<c04dd4bc>] (register_netdev) from [<c041755c>] (davinci_emac_probe+0x408/0x5c8)
> [<c041755c>] (davinci_emac_probe) from [<c0396d78>] (platform_drv_probe+0x48/0xa4)
> 
> Let's fix it by moving the pm_runtime_get() call earlier, and also
> add it to the emac_dev_getnetstats(). Also note that we want to use
> pm_rutime_get_sync() as we don't want to have deferred_resume happen.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/net/ethernet/ti/davinci_emac.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index 383ed52..deb43b3 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1538,7 +1538,7 @@ static int emac_dev_open(struct net_device *ndev)
>  	int i = 0;
>  	struct emac_priv *priv = netdev_priv(ndev);
>  
> -	pm_runtime_get(&priv->pdev->dev);
> +	pm_runtime_get_sync(&priv->pdev->dev);

gotta check return value on all pm_runtime_get_sync() calls. IIRC,
there's a coccinelle script for checking and patching this.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/6] net: davinci_emac: Fix runtime pm calls for davinci_emac

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [150113 11:55]:
> On Tue, Jan 13, 2015 at 11:29:24AM -0800, Tony Lindgren wrote:
> > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > @@ -1538,7 +1538,7 @@ static int emac_dev_open(struct net_device *ndev)
> >  	int i = 0;
> >  	struct emac_priv *priv = netdev_priv(ndev);
> >  
> > -	pm_runtime_get(&priv->pdev->dev);
> > +	pm_runtime_get_sync(&priv->pdev->dev);
> 
> gotta check return value on all pm_runtime_get_sync() calls. IIRC,
> there's a coccinelle script for checking and patching this.

Sure, here's an updated patch with error checking added.

Regards,

Tony

8< ---------------------
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 22 Dec 2014 08:19:06 -0800
Subject: [PATCH] net: davinci_emac: Fix runtime pm calls for davinci_emac

Commit 3ba97381343b ("net: ethernet: davinci_emac: add pm_runtime support")
added support for runtime PM, but it causes issues on omap3 related devices
that actually gate the clocks:

Unhandled fault: external abort on non-linefetch (0x1008)
...
[<c04160f0>] (emac_dev_getnetstats) from [<c04d6a3c>] (dev_get_stats+0x78/0xc8)
[<c04d6a3c>] (dev_get_stats) from [<c04e9ccc>] (rtnl_fill_ifinfo+0x3b8/0x938)
[<c04e9ccc>] (rtnl_fill_ifinfo) from [<c04eade4>] (rtmsg_ifinfo+0x68/0xd8)
[<c04eade4>] (rtmsg_ifinfo) from [<c04dd35c>] (register_netdevice+0x3a0/0x4ec)
[<c04dd35c>] (register_netdevice) from [<c04dd4bc>] (register_netdev+0x14/0x24)
[<c04dd4bc>] (register_netdev) from [<c041755c>] (davinci_emac_probe+0x408/0x5c8)
[<c041755c>] (davinci_emac_probe) from [<c0396d78>] (platform_drv_probe+0x48/0xa4)

Let's fix it by moving the pm_runtime_get() call earlier, and also add it to
the emac_dev_getnetstats(). Also note that we want to use pm_runtime_get_sync()
as we don't want to have deferred_resume happen. And let's also check the
return value for pm_runtime_get_sync() as noted by Felipe Balbi <balbi@ti.com>.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Mark A. Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1538,7 +1538,13 @@ static int emac_dev_open(struct net_device *ndev)
 	int i = 0;
 	struct emac_priv *priv = netdev_priv(ndev);
 
-	pm_runtime_get(&priv->pdev->dev);
+	ret = pm_runtime_get_sync(&priv->pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&priv->pdev->dev);
+		dev_err(&priv->pdev->dev, "%s: failed to get_sync(%d)\n",
+			__func__, ret);
+		return ret;
+	}
 
 	netif_carrier_off(ndev);
 	for (cnt = 0; cnt < ETH_ALEN; cnt++)
@@ -1725,6 +1731,15 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
 	struct emac_priv *priv = netdev_priv(ndev);
 	u32 mac_control;
 	u32 stats_clear_mask;
+	int err;
+
+	err = pm_runtime_get_sync(&priv->pdev->dev);
+	if (err < 0) {
+		pm_runtime_put_noidle(&priv->pdev->dev);
+		dev_err(&priv->pdev->dev, "%s: failed to get_sync(%d)\n",
+			__func__, err);
+		return &ndev->stats;
+	}
 
 	/* update emac hardware stats and reset the registers*/
 
@@ -1767,6 +1782,8 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
 	ndev->stats.tx_fifo_errors += emac_read(EMAC_TXUNDERRUN);
 	emac_write(EMAC_TXUNDERRUN, stats_clear_mask);
 
+	pm_runtime_put(&priv->pdev->dev);
+
 	return &ndev->stats;
 }
 
@@ -1981,12 +1998,22 @@ static int davinci_emac_probe(struct platform_device *pdev)
 	ndev->ethtool_ops = &ethtool_ops;
 	netif_napi_add(ndev, &priv->napi, emac_poll, EMAC_POLL_WEIGHT);
 
+	pm_runtime_enable(&pdev->dev);
+	rc = pm_runtime_get_sync(&pdev->dev);
+	if (rc < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		dev_err(&pdev->dev, "%s: failed to get_sync(%d)\n",
+			__func__, rc);
+		goto no_cpdma_chan;
+	}
+
 	/* register the network device */
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 	rc = register_netdev(ndev);
 	if (rc) {
 		dev_err(&pdev->dev, "error in register_netdev\n");
 		rc = -ENODEV;
+		pm_runtime_put(&pdev->dev);
 		goto no_cpdma_chan;
 	}
 
@@ -1996,9 +2023,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
 			   "(regs: %p, irq: %d)\n",
 			   (void *)priv->emac_base_phys, ndev->irq);
 	}
-
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_resume(&pdev->dev);
+	pm_runtime_put(&pdev->dev);
 
 	return 0;
 

Re: [PATCH 2/6] net: davinci_emac: Fix runtime pm calls for davinci_emac

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4851 bytes --]

On Tue, Jan 13, 2015 at 12:54:40PM -0800, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [150113 11:55]:
> > On Tue, Jan 13, 2015 at 11:29:24AM -0800, Tony Lindgren wrote:
> > > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > > @@ -1538,7 +1538,7 @@ static int emac_dev_open(struct net_device *ndev)
> > >  	int i = 0;
> > >  	struct emac_priv *priv = netdev_priv(ndev);
> > >  
> > > -	pm_runtime_get(&priv->pdev->dev);
> > > +	pm_runtime_get_sync(&priv->pdev->dev);
> > 
> > gotta check return value on all pm_runtime_get_sync() calls. IIRC,
> > there's a coccinelle script for checking and patching this.
> 
> Sure, here's an updated patch with error checking added.
> 
> Regards,
> 
> Tony
> 
> 8< ---------------------
> From: Tony Lindgren <tony@atomide.com>
> Date: Mon, 22 Dec 2014 08:19:06 -0800
> Subject: [PATCH] net: davinci_emac: Fix runtime pm calls for davinci_emac
> 
> Commit 3ba97381343b ("net: ethernet: davinci_emac: add pm_runtime support")
> added support for runtime PM, but it causes issues on omap3 related devices
> that actually gate the clocks:
> 
> Unhandled fault: external abort on non-linefetch (0x1008)
> ...
> [<c04160f0>] (emac_dev_getnetstats) from [<c04d6a3c>] (dev_get_stats+0x78/0xc8)
> [<c04d6a3c>] (dev_get_stats) from [<c04e9ccc>] (rtnl_fill_ifinfo+0x3b8/0x938)
> [<c04e9ccc>] (rtnl_fill_ifinfo) from [<c04eade4>] (rtmsg_ifinfo+0x68/0xd8)
> [<c04eade4>] (rtmsg_ifinfo) from [<c04dd35c>] (register_netdevice+0x3a0/0x4ec)
> [<c04dd35c>] (register_netdevice) from [<c04dd4bc>] (register_netdev+0x14/0x24)
> [<c04dd4bc>] (register_netdev) from [<c041755c>] (davinci_emac_probe+0x408/0x5c8)
> [<c041755c>] (davinci_emac_probe) from [<c0396d78>] (platform_drv_probe+0x48/0xa4)
> 
> Let's fix it by moving the pm_runtime_get() call earlier, and also add it to
> the emac_dev_getnetstats(). Also note that we want to use pm_runtime_get_sync()
> as we don't want to have deferred_resume happen. And let's also check the
> return value for pm_runtime_get_sync() as noted by Felipe Balbi <balbi@ti.com>.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Mark A. Greer <mgreer@animalcreek.com>

Reviewed-by: Felipe Balbi <balbi@ti.com>

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1538,7 +1538,13 @@ static int emac_dev_open(struct net_device *ndev)
>  	int i = 0;
>  	struct emac_priv *priv = netdev_priv(ndev);
>  
> -	pm_runtime_get(&priv->pdev->dev);
> +	ret = pm_runtime_get_sync(&priv->pdev->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(&priv->pdev->dev);
> +		dev_err(&priv->pdev->dev, "%s: failed to get_sync(%d)\n",
> +			__func__, ret);
> +		return ret;
> +	}
>  
>  	netif_carrier_off(ndev);
>  	for (cnt = 0; cnt < ETH_ALEN; cnt++)
> @@ -1725,6 +1731,15 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
>  	struct emac_priv *priv = netdev_priv(ndev);
>  	u32 mac_control;
>  	u32 stats_clear_mask;
> +	int err;
> +
> +	err = pm_runtime_get_sync(&priv->pdev->dev);
> +	if (err < 0) {
> +		pm_runtime_put_noidle(&priv->pdev->dev);
> +		dev_err(&priv->pdev->dev, "%s: failed to get_sync(%d)\n",
> +			__func__, err);
> +		return &ndev->stats;
> +	}
>  
>  	/* update emac hardware stats and reset the registers*/
>  
> @@ -1767,6 +1782,8 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
>  	ndev->stats.tx_fifo_errors += emac_read(EMAC_TXUNDERRUN);
>  	emac_write(EMAC_TXUNDERRUN, stats_clear_mask);
>  
> +	pm_runtime_put(&priv->pdev->dev);
> +
>  	return &ndev->stats;
>  }
>  
> @@ -1981,12 +1998,22 @@ static int davinci_emac_probe(struct platform_device *pdev)
>  	ndev->ethtool_ops = &ethtool_ops;
>  	netif_napi_add(ndev, &priv->napi, emac_poll, EMAC_POLL_WEIGHT);
>  
> +	pm_runtime_enable(&pdev->dev);
> +	rc = pm_runtime_get_sync(&pdev->dev);
> +	if (rc < 0) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +		dev_err(&pdev->dev, "%s: failed to get_sync(%d)\n",
> +			__func__, rc);
> +		goto no_cpdma_chan;
> +	}
> +
>  	/* register the network device */
>  	SET_NETDEV_DEV(ndev, &pdev->dev);
>  	rc = register_netdev(ndev);
>  	if (rc) {
>  		dev_err(&pdev->dev, "error in register_netdev\n");
>  		rc = -ENODEV;
> +		pm_runtime_put(&pdev->dev);
>  		goto no_cpdma_chan;
>  	}
>  
> @@ -1996,9 +2023,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
>  			   "(regs: %p, irq: %d)\n",
>  			   (void *)priv->emac_base_phys, ndev->irq);
>  	}
> -
> -	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_resume(&pdev->dev);
> +	pm_runtime_put(&pdev->dev);
>  
>  	return 0;
>  

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/6] net: davinci_emac: Fix runtime pm calls for davinci_emac

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4851 bytes --]

On Tue, Jan 13, 2015 at 12:54:40PM -0800, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [150113 11:55]:
> > On Tue, Jan 13, 2015 at 11:29:24AM -0800, Tony Lindgren wrote:
> > > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > > @@ -1538,7 +1538,7 @@ static int emac_dev_open(struct net_device *ndev)
> > >  	int i = 0;
> > >  	struct emac_priv *priv = netdev_priv(ndev);
> > >  
> > > -	pm_runtime_get(&priv->pdev->dev);
> > > +	pm_runtime_get_sync(&priv->pdev->dev);
> > 
> > gotta check return value on all pm_runtime_get_sync() calls. IIRC,
> > there's a coccinelle script for checking and patching this.
> 
> Sure, here's an updated patch with error checking added.
> 
> Regards,
> 
> Tony
> 
> 8< ---------------------
> From: Tony Lindgren <tony@atomide.com>
> Date: Mon, 22 Dec 2014 08:19:06 -0800
> Subject: [PATCH] net: davinci_emac: Fix runtime pm calls for davinci_emac
> 
> Commit 3ba97381343b ("net: ethernet: davinci_emac: add pm_runtime support")
> added support for runtime PM, but it causes issues on omap3 related devices
> that actually gate the clocks:
> 
> Unhandled fault: external abort on non-linefetch (0x1008)
> ...
> [<c04160f0>] (emac_dev_getnetstats) from [<c04d6a3c>] (dev_get_stats+0x78/0xc8)
> [<c04d6a3c>] (dev_get_stats) from [<c04e9ccc>] (rtnl_fill_ifinfo+0x3b8/0x938)
> [<c04e9ccc>] (rtnl_fill_ifinfo) from [<c04eade4>] (rtmsg_ifinfo+0x68/0xd8)
> [<c04eade4>] (rtmsg_ifinfo) from [<c04dd35c>] (register_netdevice+0x3a0/0x4ec)
> [<c04dd35c>] (register_netdevice) from [<c04dd4bc>] (register_netdev+0x14/0x24)
> [<c04dd4bc>] (register_netdev) from [<c041755c>] (davinci_emac_probe+0x408/0x5c8)
> [<c041755c>] (davinci_emac_probe) from [<c0396d78>] (platform_drv_probe+0x48/0xa4)
> 
> Let's fix it by moving the pm_runtime_get() call earlier, and also add it to
> the emac_dev_getnetstats(). Also note that we want to use pm_runtime_get_sync()
> as we don't want to have deferred_resume happen. And let's also check the
> return value for pm_runtime_get_sync() as noted by Felipe Balbi <balbi@ti.com>.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Mark A. Greer <mgreer@animalcreek.com>

Reviewed-by: Felipe Balbi <balbi@ti.com>

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1538,7 +1538,13 @@ static int emac_dev_open(struct net_device *ndev)
>  	int i = 0;
>  	struct emac_priv *priv = netdev_priv(ndev);
>  
> -	pm_runtime_get(&priv->pdev->dev);
> +	ret = pm_runtime_get_sync(&priv->pdev->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(&priv->pdev->dev);
> +		dev_err(&priv->pdev->dev, "%s: failed to get_sync(%d)\n",
> +			__func__, ret);
> +		return ret;
> +	}
>  
>  	netif_carrier_off(ndev);
>  	for (cnt = 0; cnt < ETH_ALEN; cnt++)
> @@ -1725,6 +1731,15 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
>  	struct emac_priv *priv = netdev_priv(ndev);
>  	u32 mac_control;
>  	u32 stats_clear_mask;
> +	int err;
> +
> +	err = pm_runtime_get_sync(&priv->pdev->dev);
> +	if (err < 0) {
> +		pm_runtime_put_noidle(&priv->pdev->dev);
> +		dev_err(&priv->pdev->dev, "%s: failed to get_sync(%d)\n",
> +			__func__, err);
> +		return &ndev->stats;
> +	}
>  
>  	/* update emac hardware stats and reset the registers*/
>  
> @@ -1767,6 +1782,8 @@ static struct net_device_stats *emac_dev_getnetstats(struct net_device *ndev)
>  	ndev->stats.tx_fifo_errors += emac_read(EMAC_TXUNDERRUN);
>  	emac_write(EMAC_TXUNDERRUN, stats_clear_mask);
>  
> +	pm_runtime_put(&priv->pdev->dev);
> +
>  	return &ndev->stats;
>  }
>  
> @@ -1981,12 +1998,22 @@ static int davinci_emac_probe(struct platform_device *pdev)
>  	ndev->ethtool_ops = &ethtool_ops;
>  	netif_napi_add(ndev, &priv->napi, emac_poll, EMAC_POLL_WEIGHT);
>  
> +	pm_runtime_enable(&pdev->dev);
> +	rc = pm_runtime_get_sync(&pdev->dev);
> +	if (rc < 0) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +		dev_err(&pdev->dev, "%s: failed to get_sync(%d)\n",
> +			__func__, rc);
> +		goto no_cpdma_chan;
> +	}
> +
>  	/* register the network device */
>  	SET_NETDEV_DEV(ndev, &pdev->dev);
>  	rc = register_netdev(ndev);
>  	if (rc) {
>  		dev_err(&pdev->dev, "error in register_netdev\n");
>  		rc = -ENODEV;
> +		pm_runtime_put(&pdev->dev);
>  		goto no_cpdma_chan;
>  	}
>  
> @@ -1996,9 +2023,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
>  			   "(regs: %p, irq: %d)\n",
>  			   (void *)priv->emac_base_phys, ndev->irq);
>  	}
> -
> -	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_resume(&pdev->dev);
> +	pm_runtime_put(&pdev->dev);
>  
>  	return 0;
>  

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/6] net: davinci_emac: Fix runtime pm calls for davinci_emac

[Mark A. Greer]

On Tue, Jan 13, 2015 at 12:54:40PM -0800, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [150113 11:55]:
> > On Tue, Jan 13, 2015 at 11:29:24AM -0800, Tony Lindgren wrote:
> > > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > > @@ -1538,7 +1538,7 @@ static int emac_dev_open(struct net_device *ndev)
> > >  	int i = 0;
> > >  	struct emac_priv *priv = netdev_priv(ndev);
> > >  
> > > -	pm_runtime_get(&priv->pdev->dev);
> > > +	pm_runtime_get_sync(&priv->pdev->dev);
> > 
> > gotta check return value on all pm_runtime_get_sync() calls. IIRC,
> > there's a coccinelle script for checking and patching this.
> 
> Sure, here's an updated patch with error checking added.
> 
> Regards,
> 
> Tony
> 
> 8< ---------------------
> From: Tony Lindgren <tony@atomide.com>
> Date: Mon, 22 Dec 2014 08:19:06 -0800
> Subject: [PATCH] net: davinci_emac: Fix runtime pm calls for davinci_emac
> 
> Commit 3ba97381343b ("net: ethernet: davinci_emac: add pm_runtime support")
> added support for runtime PM, but it causes issues on omap3 related devices
> that actually gate the clocks:
> 
> Unhandled fault: external abort on non-linefetch (0x1008)
> ...
> [<c04160f0>] (emac_dev_getnetstats) from [<c04d6a3c>] (dev_get_stats+0x78/0xc8)
> [<c04d6a3c>] (dev_get_stats) from [<c04e9ccc>] (rtnl_fill_ifinfo+0x3b8/0x938)
> [<c04e9ccc>] (rtnl_fill_ifinfo) from [<c04eade4>] (rtmsg_ifinfo+0x68/0xd8)
> [<c04eade4>] (rtmsg_ifinfo) from [<c04dd35c>] (register_netdevice+0x3a0/0x4ec)
> [<c04dd35c>] (register_netdevice) from [<c04dd4bc>] (register_netdev+0x14/0x24)
> [<c04dd4bc>] (register_netdev) from [<c041755c>] (davinci_emac_probe+0x408/0x5c8)
> [<c041755c>] (davinci_emac_probe) from [<c0396d78>] (platform_drv_probe+0x48/0xa4)
> 
> Let's fix it by moving the pm_runtime_get() call earlier, and also add it to
> the emac_dev_getnetstats(). Also note that we want to use pm_runtime_get_sync()
> as we don't want to have deferred_resume happen. And let's also check the
> return value for pm_runtime_get_sync() as noted by Felipe Balbi <balbi@ti.com>.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Tony,

Sorry for the sloppy pm_runtime patch and thanks for fixing it.

Acked-by: Mark A. Greer <mgreer@animalcreek.com>

[PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Tony Lindgren]

We only use clk_get() to get the frequency, the rest is done by
the runtime PM calls. Let's free the clock too.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/ethernet/ti/davinci_emac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index deb43b3..e9efc74 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 	emac_bus_frequency = clk_get_rate(emac_clk);
+	clk_put(emac_clk);
 
 	/* TODO: Probe PHY here if possible */
 
-- 
2.1.4

Re: [PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Tom Lendacky]

On 01/13/2015 01:29 PM, Tony Lindgren wrote:
> We only use clk_get() to get the frequency, the rest is done by
> the runtime PM calls. Let's free the clock too.
>
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/net/ethernet/ti/davinci_emac.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index deb43b3..e9efc74 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
>   		return -EBUSY;
>   	}
>   	emac_bus_frequency = clk_get_rate(emac_clk);
> +	clk_put(emac_clk);

The devm_clk_get call is used to get the clock so either a devm_clk_put
needs to be used here or just let the devm_ call do its thing and
automatically do the put when the module is unloaded.

Thanks,
Tom

>
>   	/* TODO: Probe PHY here if possible */
>
>

Re: [PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Tom Lendacky]

On 01/13/2015 01:29 PM, Tony Lindgren wrote:
> We only use clk_get() to get the frequency, the rest is done by
> the runtime PM calls. Let's free the clock too.
>
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/net/ethernet/ti/davinci_emac.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index deb43b3..e9efc74 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
>   		return -EBUSY;
>   	}
>   	emac_bus_frequency = clk_get_rate(emac_clk);
> +	clk_put(emac_clk);

The devm_clk_get call is used to get the clock so either a devm_clk_put
needs to be used here or just let the devm_ call do its thing and
automatically do the put when the module is unloaded.

Thanks,
Tom

>
>   	/* TODO: Probe PHY here if possible */
>
>

Re: [PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

On Tue, Jan 13, 2015 at 01:48:24PM -0600, Tom Lendacky wrote:
> On 01/13/2015 01:29 PM, Tony Lindgren wrote:
> >We only use clk_get() to get the frequency, the rest is done by
> >the runtime PM calls. Let's free the clock too.
> >
> >Cc: Brian Hutchinson <b.hutchman@gmail.com>
> >Cc: Felipe Balbi <balbi@ti.com>
> >Signed-off-by: Tony Lindgren <tony@atomide.com>
> >---
> >  drivers/net/ethernet/ti/davinci_emac.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> >index deb43b3..e9efc74 100644
> >--- a/drivers/net/ethernet/ti/davinci_emac.c
> >+++ b/drivers/net/ethernet/ti/davinci_emac.c
> >@@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
> >  		return -EBUSY;
> >  	}
> >  	emac_bus_frequency = clk_get_rate(emac_clk);
> >+	clk_put(emac_clk);
> 
> The devm_clk_get call is used to get the clock so either a devm_clk_put
> needs to be used here or just let the devm_ call do its thing and
> automatically do the put when the module is unloaded.

instead, if you really don't need the clock for anything other than
getting its rate, why don't you just remove devm_ prefix from clk_get()?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

On Tue, Jan 13, 2015 at 01:48:24PM -0600, Tom Lendacky wrote:
> On 01/13/2015 01:29 PM, Tony Lindgren wrote:
> >We only use clk_get() to get the frequency, the rest is done by
> >the runtime PM calls. Let's free the clock too.
> >
> >Cc: Brian Hutchinson <b.hutchman@gmail.com>
> >Cc: Felipe Balbi <balbi@ti.com>
> >Signed-off-by: Tony Lindgren <tony@atomide.com>
> >---
> >  drivers/net/ethernet/ti/davinci_emac.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> >index deb43b3..e9efc74 100644
> >--- a/drivers/net/ethernet/ti/davinci_emac.c
> >+++ b/drivers/net/ethernet/ti/davinci_emac.c
> >@@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
> >  		return -EBUSY;
> >  	}
> >  	emac_bus_frequency = clk_get_rate(emac_clk);
> >+	clk_put(emac_clk);
> 
> The devm_clk_get call is used to get the clock so either a devm_clk_put
> needs to be used here or just let the devm_ call do its thing and
> automatically do the put when the module is unloaded.

instead, if you really don't need the clock for anything other than
getting its rate, why don't you just remove devm_ prefix from clk_get()?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [150113 11:54]:
> On Tue, Jan 13, 2015 at 01:48:24PM -0600, Tom Lendacky wrote:
> > On 01/13/2015 01:29 PM, Tony Lindgren wrote:
> > >We only use clk_get() to get the frequency, the rest is done by
> > >the runtime PM calls. Let's free the clock too.
> > >
> > >Cc: Brian Hutchinson <b.hutchman@gmail.com>
> > >Cc: Felipe Balbi <balbi@ti.com>
> > >Signed-off-by: Tony Lindgren <tony@atomide.com>
> > >---
> > >  drivers/net/ethernet/ti/davinci_emac.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > >diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> > >index deb43b3..e9efc74 100644
> > >--- a/drivers/net/ethernet/ti/davinci_emac.c
> > >+++ b/drivers/net/ethernet/ti/davinci_emac.c
> > >@@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
> > >  		return -EBUSY;
> > >  	}
> > >  	emac_bus_frequency = clk_get_rate(emac_clk);
> > >+	clk_put(emac_clk);
> > 
> > The devm_clk_get call is used to get the clock so either a devm_clk_put
> > needs to be used here or just let the devm_ call do its thing and
> > automatically do the put when the module is unloaded.
> 
> instead, if you really don't need the clock for anything other than
> getting its rate, why don't you just remove devm_ prefix from clk_get()?

That would make the fix two lines instead of one :) I guess up to David
to figure out which he prefers, I don't really have a preference here.

Regards,

Tony

Re: [PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Tony Lindgren]

* Tom Lendacky <thomas.lendacky@amd.com> [150113 11:51]:
> On 01/13/2015 01:29 PM, Tony Lindgren wrote:
> >We only use clk_get() to get the frequency, the rest is done by
> >the runtime PM calls. Let's free the clock too.
> >
> >Cc: Brian Hutchinson <b.hutchman@gmail.com>
> >Cc: Felipe Balbi <balbi@ti.com>
> >Signed-off-by: Tony Lindgren <tony@atomide.com>
> >---
> >  drivers/net/ethernet/ti/davinci_emac.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> >index deb43b3..e9efc74 100644
> >--- a/drivers/net/ethernet/ti/davinci_emac.c
> >+++ b/drivers/net/ethernet/ti/davinci_emac.c
> >@@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
> >  		return -EBUSY;
> >  	}
> >  	emac_bus_frequency = clk_get_rate(emac_clk);
> >+	clk_put(emac_clk);
> 
> The devm_clk_get call is used to get the clock so either a devm_clk_put
> needs to be used here or just let the devm_ call do its thing and
> automatically do the put when the module is unloaded.

Thanks good catch, updated patch below.

Runtime PM is really managing the clocks here and the driver
just checks the rate so let's devm_clk_put() it here.

Regards,

Tony.

8< ---------------------
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 22 Dec 2014 08:19:06 -0800
Subject: [PATCH] net: davinci_emac: Free clock after checking the frequency

We only use clk_get() to get the frequency, the rest is done by
the runtime PM calls. Let's free the clock too.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 	emac_bus_frequency = clk_get_rate(emac_clk);
+	devm_clk_put(&pdev->dev, emac_clk);
 
 	/* TODO: Probe PHY here if possible */
 

Re: [PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[David Miller]

From: Tony Lindgren <tony@atomide.com>
Date: Tue, 13 Jan 2015 11:54:16 -0800

> * Tom Lendacky <thomas.lendacky@amd.com> [150113 11:51]:
>> On 01/13/2015 01:29 PM, Tony Lindgren wrote:
>> >We only use clk_get() to get the frequency, the rest is done by
>> >the runtime PM calls. Let's free the clock too.
>> >
>> >Cc: Brian Hutchinson <b.hutchman@gmail.com>
>> >Cc: Felipe Balbi <balbi@ti.com>
>> >Signed-off-by: Tony Lindgren <tony@atomide.com>
>> >---
>> >  drivers/net/ethernet/ti/davinci_emac.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> >diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
>> >index deb43b3..e9efc74 100644
>> >--- a/drivers/net/ethernet/ti/davinci_emac.c
>> >+++ b/drivers/net/ethernet/ti/davinci_emac.c
>> >@@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
>> >  		return -EBUSY;
>> >  	}
>> >  	emac_bus_frequency = clk_get_rate(emac_clk);
>> >+	clk_put(emac_clk);
>> 
>> The devm_clk_get call is used to get the clock so either a devm_clk_put
>> needs to be used here or just let the devm_ call do its thing and
>> automatically do the put when the module is unloaded.
> 
> Thanks good catch, updated patch below.

Please, once all the feedback has been addressed, repost the entire
series.

THanks.

Re: [PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Tony Lindgren]

* David Miller <davem@davemloft.net> [150113 13:08]:
> From: Tony Lindgren <tony@atomide.com>
> Date: Tue, 13 Jan 2015 11:54:16 -0800
> 
> > * Tom Lendacky <thomas.lendacky@amd.com> [150113 11:51]:
> >> On 01/13/2015 01:29 PM, Tony Lindgren wrote:
> >> >We only use clk_get() to get the frequency, the rest is done by
> >> >the runtime PM calls. Let's free the clock too.
> >> >
> >> >Cc: Brian Hutchinson <b.hutchman@gmail.com>
> >> >Cc: Felipe Balbi <balbi@ti.com>
> >> >Signed-off-by: Tony Lindgren <tony@atomide.com>
> >> >---
> >> >  drivers/net/ethernet/ti/davinci_emac.c | 1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> >diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> >> >index deb43b3..e9efc74 100644
> >> >--- a/drivers/net/ethernet/ti/davinci_emac.c
> >> >+++ b/drivers/net/ethernet/ti/davinci_emac.c
> >> >@@ -1881,6 +1881,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
> >> >  		return -EBUSY;
> >> >  	}
> >> >  	emac_bus_frequency = clk_get_rate(emac_clk);
> >> >+	clk_put(emac_clk);
> >> 
> >> The devm_clk_get call is used to get the clock so either a devm_clk_put
> >> needs to be used here or just let the devm_ call do its thing and
> >> automatically do the put when the module is unloaded.
> > 
> > Thanks good catch, updated patch below.
> 
> Please, once all the feedback has been addressed, repost the entire
> series.

Sure, will repost on Thursday in case there will be more comments.

Regards,

Tony

[PATCH 4/6] net: davinci_emac: Fix incomplete code for getting the phy from device tree

[Tony Lindgren]

Looks like the phy_id is never set up beyond getting the phandle.
Note that we can remove the ifdef for phy_node as there is a stub
for of_phy_connec() if CONFIG_OF is not set.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/ethernet/ti/davinci_emac.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index e9efc74..4c8d82c 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -62,6 +62,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_mdio.h>
 #include <linux/of_irq.h>
 #include <linux/of_net.h>
 
@@ -343,9 +344,7 @@ struct emac_priv {
 	u32 multicast_hash_cnt[EMAC_NUM_MULTICAST_BITS];
 	u32 rx_addr_type;
 	const char *phy_id;
-#ifdef CONFIG_OF
 	struct device_node *phy_node;
-#endif
 	struct phy_device *phydev;
 	spinlock_t lock;
 	/*platform specific members*/
@@ -1597,8 +1596,20 @@ static int emac_dev_open(struct net_device *ndev)
 	cpdma_ctlr_start(priv->dma);
 
 	priv->phydev = NULL;
+
+	if (priv->phy_node) {
+		priv->phydev = of_phy_connect(ndev, priv->phy_node,
+					      &emac_adjust_link, 0, 0);
+		if (!priv->phydev) {
+			dev_err(emac_dev, "could not connect to phy %s\n",
+				priv->phy_node->full_name);
+			ret = -ENODEV;
+			goto err;
+		}
+	}
+
 	/* use the first phy on the bus if pdata did not give us a phy id */
-	if (!priv->phy_id) {
+	if (!priv->phydev && !priv->phy_id) {
 		struct device *phy;
 
 		phy = bus_find_device(&mdio_bus_type, NULL, NULL,
@@ -1607,7 +1618,7 @@ static int emac_dev_open(struct net_device *ndev)
 			priv->phy_id = dev_name(phy);
 	}
 
-	if (priv->phy_id && *priv->phy_id) {
+	if (!priv->phydev && priv->phy_id && *priv->phy_id) {
 		priv->phydev = phy_connect(ndev, priv->phy_id,
 					   &emac_adjust_link,
 					   PHY_INTERFACE_MODE_MII);
@@ -1628,7 +1639,9 @@ static int emac_dev_open(struct net_device *ndev)
 			"(mii_bus:phy_addr=%s, id=%x)\n",
 			priv->phydev->drv->name, dev_name(&priv->phydev->dev),
 			priv->phydev->phy_id);
-	} else {
+	}
+
+	if (!priv->phydev) {
 		/* No PHY , fix the link, speed and duplex settings */
 		dev_notice(emac_dev, "no phy, defaulting to 100/full\n");
 		priv->link = 1;
-- 
2.1.4

[PATCH 5/6] net: davinci_emac: Fix ioremap for devices with MDIO within the EMAC address space

[Tony Lindgren]

Some devices like dm816x have the MDIO registers within the first EMAC
instance address space. Let's fix the issue by allowing to pass an
optional second IO range for the EMAC control register area.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/ethernet/ti/davinci_emac.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 4c8d82c..0342273 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1877,7 +1877,7 @@ davinci_emac_of_get_pdata(struct platform_device *pdev, struct emac_priv *priv)
 static int davinci_emac_probe(struct platform_device *pdev)
 {
 	int rc = 0;
-	struct resource *res;
+	struct resource *res, *res_ctrl;
 	struct net_device *ndev;
 	struct emac_priv *priv;
 	unsigned long hw_ram_addr;
@@ -1936,11 +1936,20 @@ static int davinci_emac_probe(struct platform_device *pdev)
 		rc = PTR_ERR(priv->remap_addr);
 		goto no_pdata;
 	}
+
+	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res_ctrl) {
+		priv->ctrl_base =
+			devm_ioremap_resource(&pdev->dev, res_ctrl);
+		if (IS_ERR(priv->ctrl_base))
+			goto no_pdata;
+	} else {
+		priv->ctrl_base = priv->remap_addr + pdata->ctrl_mod_reg_offset;
+	}
+
 	priv->emac_base = priv->remap_addr + pdata->ctrl_reg_offset;
 	ndev->base_addr = (unsigned long)priv->remap_addr;
 
-	priv->ctrl_base = priv->remap_addr + pdata->ctrl_mod_reg_offset;
-
 	hw_ram_addr = pdata->hw_ram_addr;
 	if (!hw_ram_addr)
 		hw_ram_addr = (u32 __force)res->start + pdata->ctrl_ram_offset;
-- 
2.1.4

Re: [PATCH 5/6] net: davinci_emac: Fix ioremap for devices with MDIO within the EMAC address space

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

On Tue, Jan 13, 2015 at 11:29:27AM -0800, Tony Lindgren wrote:
> Some devices like dm816x have the MDIO registers within the first EMAC
> instance address space. Let's fix the issue by allowing to pass an
> optional second IO range for the EMAC control register area.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/net/ethernet/ti/davinci_emac.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index 4c8d82c..0342273 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1877,7 +1877,7 @@ davinci_emac_of_get_pdata(struct platform_device *pdev, struct emac_priv *priv)
>  static int davinci_emac_probe(struct platform_device *pdev)
>  {
>  	int rc = 0;
> -	struct resource *res;
> +	struct resource *res, *res_ctrl;
>  	struct net_device *ndev;
>  	struct emac_priv *priv;
>  	unsigned long hw_ram_addr;
> @@ -1936,11 +1936,20 @@ static int davinci_emac_probe(struct platform_device *pdev)
>  		rc = PTR_ERR(priv->remap_addr);
>  		goto no_pdata;
>  	}
> +
> +	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (res_ctrl) {

devm_ioremap_resource() will check for res_ctrl being a valid pointer,
perhaps below would be slightly better ?


	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
	priv->ctrl_base = devm_ioremap_resource(&pdev->dev, res_ctrl);
	if (IS_ERR(priv->ctrl_base))
		priv->ctrl_base = priv->remap_addr + pdata->ctrl_mod_reg_offset;

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/6] net: davinci_emac: Fix ioremap for devices with MDIO within the EMAC address space

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

On Tue, Jan 13, 2015 at 11:29:27AM -0800, Tony Lindgren wrote:
> Some devices like dm816x have the MDIO registers within the first EMAC
> instance address space. Let's fix the issue by allowing to pass an
> optional second IO range for the EMAC control register area.
> 
> Cc: Brian Hutchinson <b.hutchman@gmail.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/net/ethernet/ti/davinci_emac.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index 4c8d82c..0342273 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1877,7 +1877,7 @@ davinci_emac_of_get_pdata(struct platform_device *pdev, struct emac_priv *priv)
>  static int davinci_emac_probe(struct platform_device *pdev)
>  {
>  	int rc = 0;
> -	struct resource *res;
> +	struct resource *res, *res_ctrl;
>  	struct net_device *ndev;
>  	struct emac_priv *priv;
>  	unsigned long hw_ram_addr;
> @@ -1936,11 +1936,20 @@ static int davinci_emac_probe(struct platform_device *pdev)
>  		rc = PTR_ERR(priv->remap_addr);
>  		goto no_pdata;
>  	}
> +
> +	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (res_ctrl) {

devm_ioremap_resource() will check for res_ctrl being a valid pointer,
perhaps below would be slightly better ?


	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
	priv->ctrl_base = devm_ioremap_resource(&pdev->dev, res_ctrl);
	if (IS_ERR(priv->ctrl_base))
		priv->ctrl_base = priv->remap_addr + pdata->ctrl_mod_reg_offset;

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/6] net: davinci_emac: Fix ioremap for devices with MDIO within the EMAC address space

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [150113 11:57]:
> On Tue, Jan 13, 2015 at 11:29:27AM -0800, Tony Lindgren wrote:
> > Some devices like dm816x have the MDIO registers within the first EMAC
> > instance address space. Let's fix the issue by allowing to pass an
> > optional second IO range for the EMAC control register area.
> > 
> > Cc: Brian Hutchinson <b.hutchman@gmail.com>
> > Cc: Felipe Balbi <balbi@ti.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  drivers/net/ethernet/ti/davinci_emac.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> > index 4c8d82c..0342273 100644
> > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > @@ -1877,7 +1877,7 @@ davinci_emac_of_get_pdata(struct platform_device *pdev, struct emac_priv *priv)
> >  static int davinci_emac_probe(struct platform_device *pdev)
> >  {
> >  	int rc = 0;
> > -	struct resource *res;
> > +	struct resource *res, *res_ctrl;
> >  	struct net_device *ndev;
> >  	struct emac_priv *priv;
> >  	unsigned long hw_ram_addr;
> > @@ -1936,11 +1936,20 @@ static int davinci_emac_probe(struct platform_device *pdev)
> >  		rc = PTR_ERR(priv->remap_addr);
> >  		goto no_pdata;
> >  	}
> > +
> > +	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	if (res_ctrl) {
> 
> devm_ioremap_resource() will check for res_ctrl being a valid pointer,
> perhaps below would be slightly better ?
> 
> 
> 	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> 	priv->ctrl_base = devm_ioremap_resource(&pdev->dev, res_ctrl);
> 	if (IS_ERR(priv->ctrl_base))
> 		priv->ctrl_base = priv->remap_addr + pdata->ctrl_mod_reg_offset;
> 

We have a pile of devices using just one ioremap area so the second
ioremap area needs to be optional. That's why we only do it based on
if (res_ctrl).

Regards,

Tony

Re: [PATCH 5/6] net: davinci_emac: Fix ioremap for devices with MDIO within the EMAC address space

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On Tue, Jan 13, 2015 at 11:59:58AM -0800, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [150113 11:57]:
> > On Tue, Jan 13, 2015 at 11:29:27AM -0800, Tony Lindgren wrote:
> > > Some devices like dm816x have the MDIO registers within the first EMAC
> > > instance address space. Let's fix the issue by allowing to pass an
> > > optional second IO range for the EMAC control register area.
> > > 
> > > Cc: Brian Hutchinson <b.hutchman@gmail.com>
> > > Cc: Felipe Balbi <balbi@ti.com>
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > ---
> > >  drivers/net/ethernet/ti/davinci_emac.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> > > index 4c8d82c..0342273 100644
> > > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > > @@ -1877,7 +1877,7 @@ davinci_emac_of_get_pdata(struct platform_device *pdev, struct emac_priv *priv)
> > >  static int davinci_emac_probe(struct platform_device *pdev)
> > >  {
> > >  	int rc = 0;
> > > -	struct resource *res;
> > > +	struct resource *res, *res_ctrl;
> > >  	struct net_device *ndev;
> > >  	struct emac_priv *priv;
> > >  	unsigned long hw_ram_addr;
> > > @@ -1936,11 +1936,20 @@ static int davinci_emac_probe(struct platform_device *pdev)
> > >  		rc = PTR_ERR(priv->remap_addr);
> > >  		goto no_pdata;
> > >  	}
> > > +
> > > +	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > +	if (res_ctrl) {
> > 
> > devm_ioremap_resource() will check for res_ctrl being a valid pointer,
> > perhaps below would be slightly better ?
> > 
> > 
> > 	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > 	priv->ctrl_base = devm_ioremap_resource(&pdev->dev, res_ctrl);
> > 	if (IS_ERR(priv->ctrl_base))
> > 		priv->ctrl_base = priv->remap_addr + pdata->ctrl_mod_reg_offset;
> > 
> 
> We have a pile of devices using just one ioremap area so the second
> ioremap area needs to be optional. That's why we only do it based on
> if (res_ctrl).

fair enough

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/6] net: davinci_emac: Fix ioremap for devices with MDIO within the EMAC address space

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On Tue, Jan 13, 2015 at 11:59:58AM -0800, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [150113 11:57]:
> > On Tue, Jan 13, 2015 at 11:29:27AM -0800, Tony Lindgren wrote:
> > > Some devices like dm816x have the MDIO registers within the first EMAC
> > > instance address space. Let's fix the issue by allowing to pass an
> > > optional second IO range for the EMAC control register area.
> > > 
> > > Cc: Brian Hutchinson <b.hutchman@gmail.com>
> > > Cc: Felipe Balbi <balbi@ti.com>
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > ---
> > >  drivers/net/ethernet/ti/davinci_emac.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> > > index 4c8d82c..0342273 100644
> > > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > > @@ -1877,7 +1877,7 @@ davinci_emac_of_get_pdata(struct platform_device *pdev, struct emac_priv *priv)
> > >  static int davinci_emac_probe(struct platform_device *pdev)
> > >  {
> > >  	int rc = 0;
> > > -	struct resource *res;
> > > +	struct resource *res, *res_ctrl;
> > >  	struct net_device *ndev;
> > >  	struct emac_priv *priv;
> > >  	unsigned long hw_ram_addr;
> > > @@ -1936,11 +1936,20 @@ static int davinci_emac_probe(struct platform_device *pdev)
> > >  		rc = PTR_ERR(priv->remap_addr);
> > >  		goto no_pdata;
> > >  	}
> > > +
> > > +	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > +	if (res_ctrl) {
> > 
> > devm_ioremap_resource() will check for res_ctrl being a valid pointer,
> > perhaps below would be slightly better ?
> > 
> > 
> > 	res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > 	priv->ctrl_base = devm_ioremap_resource(&pdev->dev, res_ctrl);
> > 	if (IS_ERR(priv->ctrl_base))
> > 		priv->ctrl_base = priv->remap_addr + pdata->ctrl_mod_reg_offset;
> > 
> 
> We have a pile of devices using just one ioremap area so the second
> ioremap area needs to be optional. That's why we only do it based on
> if (res_ctrl).

fair enough

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 6/6] net: davinci_emac: Add support for emac on dm816x

[Tony Lindgren]

On dm816x we have two emac controllers with separate memory
areas.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 Documentation/devicetree/bindings/net/davinci_emac.txt | 3 ++-
 drivers/net/ethernet/ti/davinci_emac.c                 | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/davinci_emac.txt b/Documentation/devicetree/bindings/net/davinci_emac.txt
index 0328088..24c5cda 100644
--- a/Documentation/devicetree/bindings/net/davinci_emac.txt
+++ b/Documentation/devicetree/bindings/net/davinci_emac.txt
@@ -4,7 +4,8 @@ This file provides information, what the device node
 for the davinci_emac interface contains.
 
 Required properties:
-- compatible: "ti,davinci-dm6467-emac" or "ti,am3517-emac"
+- compatible: "ti,davinci-dm6467-emac", "ti,am3517-emac" or
+  "ti,dm816-emac"
 - reg: Offset and length of the register set for the device
 - ti,davinci-ctrl-reg-offset: offset to control register
 - ti,davinci-ctrl-mod-reg-offset: offset to control module register
diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 0342273..5caee66 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -2101,9 +2101,14 @@ static const struct emac_platform_data am3517_emac_data = {
 	.hw_ram_addr		= 0x01e20000,
 };
 
+static const struct emac_platform_data dm816_emac_data = {
+	.version		= EMAC_VERSION_2,
+};
+
 static const struct of_device_id davinci_emac_of_match[] = {
 	{.compatible = "ti,davinci-dm6467-emac", },
 	{.compatible = "ti,am3517-emac", .data = &am3517_emac_data, },
+	{.compatible = "ti,dm816-emac", .data = &dm816_emac_data, },
 	{},
 };
 MODULE_DEVICE_TABLE(of, davinci_emac_of_match);
-- 
2.1.4

[PATCHv2 0/6] Fixes for davinci_emac

[Tony Lindgren]

Hi,

Here's a repost of the fixes for davinci_emac with patches
updated for comments and acks collected.

Regards,

Tony

Tony Lindgren (6):
  net: davinci_emac: Fix hangs with interrupts
  net: davinci_emac: Fix runtime pm calls for davinci_emac
  net: davinci_emac: Free clock after checking the frequency
  net: davinci_emac: Fix incomplete code for getting the phy from device
    tree
  net: davinci_emac: Fix ioremap for devices with MDIO within the EMAC
    address space
  net: davinci_emac: Add support for emac on dm816x

 .../devicetree/bindings/net/davinci_emac.txt       |  3 +-
 drivers/net/ethernet/ti/davinci_emac.c             | 96 +++++++++++++++++-----
 2 files changed, 77 insertions(+), 22 deletions(-)

-- 
2.1.4

[PATCH 3/6] net: davinci_emac: Free clock after checking the frequency

[Tony Lindgren]

We only use clk_get() to get the frequency, the rest is done by
the runtime PM calls. Let's free the clock too.

Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/net/ethernet/ti/davinci_emac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 5df339e..59fdcdd 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1894,6 +1894,7 @@ static int davinci_emac_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 	emac_bus_frequency = clk_get_rate(emac_clk);
+	devm_clk_put(&pdev->dev, emac_clk);
 
 	/* TODO: Probe PHY here if possible */
 
-- 
2.1.4