[PATCH v2 0/4] Macb power management support for ZynqMP

[PATCH v2 0/4] Macb power management support for ZynqMP

[Harini Katakam]

This series adds support for macb suspend/resume with system power down.
In relation to the above, this series also updates mdio_read/write
function for PM and adds tsu clock management.

Harini Katakam (4):
  net: macb: Check MDIO state before read/write and use timeouts
  net: macb: Support clock management for tsu_clk
  net: macb: Add pm runtime support
  net: macb: Add support for suspend/resume with full power down

 drivers/net/ethernet/cadence/macb.h      |   6 +-
 drivers/net/ethernet/cadence/macb_main.c | 210 +++++++++++++++++++++++++++----
 2 files changed, 188 insertions(+), 28 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Harini Katakam]

From: Harini Katakam <harinik@xilinx.com>

Replace the while loop in MDIO read/write functions with a timeout.
In addition, add a check for MDIO bus busy before initiating a new
operation as well to make sure there is no ongoing MDIO operation.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
v2 changes:
Use readx_poll_timeout

Changes form RFC:
Cleaned up timeout implementation and moved it to a helper.

 drivers/net/ethernet/cadence/macb.h      |  3 +++
 drivers/net/ethernet/cadence/macb_main.c | 33 ++++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 3d45f4c..df7bee1 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -714,6 +714,9 @@
 		__v; \
 	})
 
+#define MACB_IDLE_MASK		(1 << MACB_IDLE_OFFSET)
+#define MACB_READ_NSR(bp)	macb_readl(bp, NSR)
+
 /* struct macb_dma_desc - Hardware DMA descriptor
  * @addr: DMA address of data buffer
  * @ctrl: Control and status bits
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 1d86b4d..fd86ece 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -36,6 +36,7 @@
 #include <linux/ip.h>
 #include <linux/udp.h>
 #include <linux/tcp.h>
+#include <linux/iopoll.h>
 #include "macb.h"
 
 #define MACB_RX_BUFFER_SIZE	128
@@ -79,6 +80,8 @@
  */
 #define MACB_HALT_TIMEOUT	1230
 
+#define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
+
 /* DMA buffer descriptor might be different size
  * depends on hardware configuration:
  *
@@ -318,10 +321,23 @@ static void macb_get_hwaddr(struct macb *bp)
 	eth_hw_addr_random(bp->dev);
 }
 
+static int macb_mdio_wait_for_idle(struct macb *bp)
+{
+	u32 val;
+
+	return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_IDLE_MASK,
+				  1, MACB_MDIO_TIMEOUT);
+}
+
 static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct macb *bp = bus->priv;
 	int value;
+	int err;
+
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0)
+		return err;
 
 	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
 			      | MACB_BF(RW, MACB_MAN_READ)
@@ -329,9 +345,9 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 			      | MACB_BF(REGA, regnum)
 			      | MACB_BF(CODE, MACB_MAN_CODE)));
 
-	/* wait for end of transfer */
-	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
-		cpu_relax();
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0)
+		return err;
 
 	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
 
@@ -342,6 +358,11 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			   u16 value)
 {
 	struct macb *bp = bus->priv;
+	int err;
+
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0)
+		return err;
 
 	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
 			      | MACB_BF(RW, MACB_MAN_WRITE)
@@ -350,9 +371,9 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			      | MACB_BF(CODE, MACB_MAN_CODE)
 			      | MACB_BF(DATA, value)));
 
-	/* wait for end of transfer */
-	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
-		cpu_relax();
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0)
+		return err;
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Andrew Lunn]

On Mon, Nov 26, 2018 at 12:37:49PM +0530, Harini Katakam wrote:
> From: Harini Katakam <harinik@xilinx.com>
> 
> Replace the while loop in MDIO read/write functions with a timeout.
> In addition, add a check for MDIO bus busy before initiating a new
> operation as well to make sure there is no ongoing MDIO operation.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
> v2 changes:
> Use readx_poll_timeout
> 
> Changes form RFC:
> Cleaned up timeout implementation and moved it to a helper.
> 
>  drivers/net/ethernet/cadence/macb.h      |  3 +++
>  drivers/net/ethernet/cadence/macb_main.c | 33 ++++++++++++++++++++++++++------
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3d45f4c..df7bee1 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -714,6 +714,9 @@
>  		__v; \
>  	})
>  
> +#define MACB_IDLE_MASK		(1 << MACB_IDLE_OFFSET)

I think you could use the MACB_BIT() macro here.

But otherwise,

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Harini Katakam]

Hi Andrew,

<snip>
> > +#define MACB_IDLE_MASK               (1 << MACB_IDLE_OFFSET)
>
> I think you could use the MACB_BIT() macro here.
>
> But otherwise,
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Thanks, will fix this in next series.

Regards,
Harini

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Claudiu.Beznea]

Hi Harini,

On 26.11.2018 09:07, Harini Katakam wrote:
> From: Harini Katakam <harinik@xilinx.com>
> 
> Replace the while loop in MDIO read/write functions with a timeout.
> In addition, add a check for MDIO bus busy before initiating a new
> operation as well to make sure there is no ongoing MDIO operation.

Is this MDIO bus busy check necessary? The caller of
macb_mdio_read/macb_mdio_write locks the mdio bus mutex before calling it
(e.g. mdiobus_read).

> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
> v2 changes:
> Use readx_poll_timeout
> 
> Changes form RFC:
> Cleaned up timeout implementation and moved it to a helper.
> 
>  drivers/net/ethernet/cadence/macb.h      |  3 +++
>  drivers/net/ethernet/cadence/macb_main.c | 33 ++++++++++++++++++++++++++------
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3d45f4c..df7bee1 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -714,6 +714,9 @@
>  		__v; \
>  	})
>  
> +#define MACB_IDLE_MASK		(1 << MACB_IDLE_OFFSET)

You could use MACB_BIT(IDLE) instead.

> +#define MACB_READ_NSR(bp)	macb_readl(bp, NSR)

Is this necessary?


> +
>  /* struct macb_dma_desc - Hardware DMA descriptor
>   * @addr: DMA address of data buffer
>   * @ctrl: Control and status bits
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 1d86b4d..fd86ece 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -36,6 +36,7 @@
>  #include <linux/ip.h>
>  #include <linux/udp.h>
>  #include <linux/tcp.h>
> +#include <linux/iopoll.h>
>  #include "macb.h"
>  
>  #define MACB_RX_BUFFER_SIZE	128
> @@ -79,6 +80,8 @@
>   */
>  #define MACB_HALT_TIMEOUT	1230
>  
> +#define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
> +
>  /* DMA buffer descriptor might be different size
>   * depends on hardware configuration:
>   *
> @@ -318,10 +321,23 @@ static void macb_get_hwaddr(struct macb *bp)
>  	eth_hw_addr_random(bp->dev);
>  }
>  
> +static int macb_mdio_wait_for_idle(struct macb *bp)
> +{
> +	u32 val;
> +
> +	return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_IDLE_MASK,
> +				  1, MACB_MDIO_TIMEOUT);
> +}
> +
>  static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
>  	struct macb *bp = bus->priv;
>  	int value;
> +	int err;
> +
> +	err = macb_mdio_wait_for_idle(bp);
> +	if (err < 0)
> +		return err;
>  
>  	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
>  			      | MACB_BF(RW, MACB_MAN_READ)
> @@ -329,9 +345,9 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  			      | MACB_BF(REGA, regnum)
>  			      | MACB_BF(CODE, MACB_MAN_CODE)));
>  
> -	/* wait for end of transfer */
> -	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> -		cpu_relax();
> +	err = macb_mdio_wait_for_idle(bp);
> +	if (err < 0)
> +		return err;
>  
>  	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
>  
> @@ -342,6 +358,11 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  			   u16 value)
>  {
>  	struct macb *bp = bus->priv;
> +	int err;
> +
> +	err = macb_mdio_wait_for_idle(bp);
> +	if (err < 0)
> +		return err;
>  
>  	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
>  			      | MACB_BF(RW, MACB_MAN_WRITE)
> @@ -350,9 +371,9 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  			      | MACB_BF(CODE, MACB_MAN_CODE)
>  			      | MACB_BF(DATA, value)));
>  
> -	/* wait for end of transfer */
> -	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> -		cpu_relax();
> +	err = macb_mdio_wait_for_idle(bp);
> +	if (err < 0)
> +		return err;
>  
>  	return 0;
>  }
> 

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Andrew Lunn]

On Mon, Nov 26, 2018 at 02:46:01PM +0000, Claudiu.Beznea@microchip.com wrote:
> Hi Harini,
> 
> On 26.11.2018 09:07, Harini Katakam wrote:
> > From: Harini Katakam <harinik@xilinx.com>
> > 
> > Replace the while loop in MDIO read/write functions with a timeout.
> > In addition, add a check for MDIO bus busy before initiating a new
> > operation as well to make sure there is no ongoing MDIO operation.
> 
> Is this MDIO bus busy check necessary? The caller of
> macb_mdio_read/macb_mdio_write locks the mdio bus mutex before calling it
> (e.g. mdiobus_read).

Hi Claudiu

It depends on the implementation. A write operation you could just
launch, but not wait for it to complete, allowing you to do other
stuff while the hardware is busy. For the next operation you then do
need to check the previous operation has completed.

I've not checked it is actually implemented this way.

     Andrew

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Harini Katakam]

Hi Claudiu,
On Mon, Nov 26, 2018 at 8:22 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Nov 26, 2018 at 02:46:01PM +0000, Claudiu.Beznea@microchip.com wrote:
> > Hi Harini,
> >
> > On 26.11.2018 09:07, Harini Katakam wrote:
> > > From: Harini Katakam <harinik@xilinx.com>
> > >
> > > Replace the while loop in MDIO read/write functions with a timeout.
> > > In addition, add a check for MDIO bus busy before initiating a new
> > > operation as well to make sure there is no ongoing MDIO operation.
> >
> > Is this MDIO bus busy check necessary? The caller of
> > macb_mdio_read/macb_mdio_write locks the mdio bus mutex before calling it
> > (e.g. mdiobus_read).
>
> Hi Claudiu
>
> It depends on the implementation. A write operation you could just
> launch, but not wait for it to complete, allowing you to do other
> stuff while the hardware is busy. For the next operation you then do
> need to check the previous operation has completed.
>
> I've not checked it is actually implemented this way.

Yes, as Andrew mentioned, a previous MDIO bus operation cannot
be assumed to be complete always - for ex., in case of a timeout.
There is a chance that the MDIO bus bit is busy when mdio_read/write
is called.

Regards,
Harini

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Claudiu.Beznea]

Hi Andrew, Harini,

On 27.11.2018 07:36, Harini Katakam wrote:
> Hi Claudiu,
> On Mon, Nov 26, 2018 at 8:22 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Mon, Nov 26, 2018 at 02:46:01PM +0000, Claudiu.Beznea@microchip.com wrote:
>>> Hi Harini,
>>>
>>> On 26.11.2018 09:07, Harini Katakam wrote:
>>>> From: Harini Katakam <harinik@xilinx.com>
>>>>
>>>> Replace the while loop in MDIO read/write functions with a timeout.
>>>> In addition, add a check for MDIO bus busy before initiating a new
>>>> operation as well to make sure there is no ongoing MDIO operation.
>>>
>>> Is this MDIO bus busy check necessary? The caller of
>>> macb_mdio_read/macb_mdio_write locks the mdio bus mutex before calling it
>>> (e.g. mdiobus_read).
>>
>> Hi Claudiu
>>
>> It depends on the implementation. A write operation you could just
>> launch, but not wait for it to complete, allowing you to do other
>> stuff while the hardware is busy. For the next operation you then do
>> need to check the previous operation has completed.
>>
>> I've not checked it is actually implemented this way.
> 
> Yes, as Andrew mentioned, a previous MDIO bus operation cannot
> be assumed to be complete always - for ex., in case of a timeout.
> There is a chance that the MDIO bus bit is busy when mdio_read/write
> is called.

Thank you for your responses. I see it now.

Then, do you think it would be better to have a new API part of struct
mii_bus that drivers should register so that, the mii core to check if the
bus is idle before lunching a new request?

Thank you,
Claudiu Beznea

> 
> Regards,
> Harini
> 

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Harini Katakam]

Hi Claudiu,

<snip>
> >>>>
> >>>> Replace the while loop in MDIO read/write functions with a timeout.
> >>>> In addition, add a check for MDIO bus busy before initiating a new
> >>>> operation as well to make sure there is no ongoing MDIO operation.
> >>>
> >>> Is this MDIO bus busy check necessary? The caller of
> >>> macb_mdio_read/macb_mdio_write locks the mdio bus mutex before calling it
> >>> (e.g. mdiobus_read).
> >>
> >> Hi Claudiu
> >>
> >> It depends on the implementation. A write operation you could just
> >> launch, but not wait for it to complete, allowing you to do other
> >> stuff while the hardware is busy. For the next operation you then do
> >> need to check the previous operation has completed.
> >>
> >> I've not checked it is actually implemented this way.
> >
> > Yes, as Andrew mentioned, a previous MDIO bus operation cannot
> > be assumed to be complete always - for ex., in case of a timeout.
> > There is a chance that the MDIO bus bit is busy when mdio_read/write
> > is called.
>
> Thank you for your responses. I see it now.
>
> Then, do you think it would be better to have a new API part of struct
> mii_bus that drivers should register so that, the mii core to check if the
> bus is idle before lunching a new request?

Sure, let me explore this option.

Regards,
Harini

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Andrew Lunn]

> Then, do you think it would be better to have a new API part of struct
> mii_bus that drivers should register so that, the mii core to check if the
> bus is idle before lunching a new request?

I'm not sure that actually brings anything useful.  The core does not
benefit from it, and it does not make the driver simpler.

	Andrew

Re: [PATCH v2 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Claudiu.Beznea]

On 28.11.2018 02:35, Andrew Lunn wrote:
>> Then, do you think it would be better to have a new API part of struct
>> mii_bus that drivers should register so that, the mii core to check if the
>> bus is idle before lunching a new request?
> 
> I'm not sure that actually brings anything useful.  The core does not
> benefit from it, and it does not make the driver simpler.

Ok, thank you for your input.

Claudiu Beznea

> 
> 	Andrew
> 
> 

[PATCH v2 2/4] net: macb: Support clock management for tsu_clk

[Harini Katakam]

From: Harini Katakam <harinik@xilinx.com>

TSU clock needs to be enabled/disabled as per support in devicetree
and it should also be controlled during suspend/resume (WOL has no
dependency on this clock).

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
v2:
No changes

 drivers/net/ethernet/cadence/macb.h      |  3 ++-
 drivers/net/ethernet/cadence/macb_main.c | 30 +++++++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index df7bee1..e0fddff 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1085,7 +1085,7 @@ struct macb_config {
 	unsigned int		dma_burst_length;
 	int	(*clk_init)(struct platform_device *pdev, struct clk **pclk,
 			    struct clk **hclk, struct clk **tx_clk,
-			    struct clk **rx_clk);
+			    struct clk **rx_clk, struct clk **tsu_clk);
 	int	(*init)(struct platform_device *pdev);
 	int	jumbo_max_len;
 };
@@ -1165,6 +1165,7 @@ struct macb {
 	struct clk		*hclk;
 	struct clk		*tx_clk;
 	struct clk		*rx_clk;
+	struct clk		*tsu_clk;
 	struct net_device	*dev;
 	union {
 		struct macb_stats	macb;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index fd86ece..32453d4 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3293,7 +3293,7 @@ static void macb_probe_queues(void __iomem *mem,
 
 static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
 			 struct clk **hclk, struct clk **tx_clk,
-			 struct clk **rx_clk)
+			 struct clk **rx_clk, struct clk **tsu_clk)
 {
 	struct macb_platform_data *pdata;
 	int err;
@@ -3327,6 +3327,10 @@ static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
 	if (IS_ERR(*rx_clk))
 		*rx_clk = NULL;
 
+	*tsu_clk = devm_clk_get(&pdev->dev, "tsu_clk");
+	if (IS_ERR(*tsu_clk))
+		*tsu_clk = NULL;
+
 	err = clk_prepare_enable(*pclk);
 	if (err) {
 		dev_err(&pdev->dev, "failed to enable pclk (%u)\n", err);
@@ -3351,8 +3355,17 @@ static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
 		goto err_disable_txclk;
 	}
 
+	err = clk_prepare_enable(*tsu_clk);
+	if (err) {
+		dev_err(&pdev->dev, "failed to enable tsu_clk (%u)\n", err);
+		goto err_disable_rxclk;
+	}
+
 	return 0;
 
+err_disable_rxclk:
+	clk_disable_unprepare(*rx_clk);
+
 err_disable_txclk:
 	clk_disable_unprepare(*tx_clk);
 
@@ -3803,13 +3816,14 @@ static const struct net_device_ops at91ether_netdev_ops = {
 
 static int at91ether_clk_init(struct platform_device *pdev, struct clk **pclk,
 			      struct clk **hclk, struct clk **tx_clk,
-			      struct clk **rx_clk)
+			      struct clk **rx_clk, struct clk **tsu_clk)
 {
 	int err;
 
 	*hclk = NULL;
 	*tx_clk = NULL;
 	*rx_clk = NULL;
+	*tsu_clk = NULL;
 
 	*pclk = devm_clk_get(&pdev->dev, "ether_clk");
 	if (IS_ERR(*pclk))
@@ -3957,11 +3971,12 @@ static int macb_probe(struct platform_device *pdev)
 {
 	const struct macb_config *macb_config = &default_gem_config;
 	int (*clk_init)(struct platform_device *, struct clk **,
-			struct clk **, struct clk **,  struct clk **)
-					      = macb_config->clk_init;
+			struct clk **, struct clk **,  struct clk **,
+			struct clk **) = macb_config->clk_init;
 	int (*init)(struct platform_device *) = macb_config->init;
 	struct device_node *np = pdev->dev.of_node;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL, *rx_clk = NULL;
+	struct clk *tsu_clk = NULL;
 	unsigned int queue_mask, num_queues;
 	struct macb_platform_data *pdata;
 	bool native_io;
@@ -3989,7 +4004,7 @@ static int macb_probe(struct platform_device *pdev)
 		}
 	}
 
-	err = clk_init(pdev, &pclk, &hclk, &tx_clk, &rx_clk);
+	err = clk_init(pdev, &pclk, &hclk, &tx_clk, &rx_clk, &tsu_clk);
 	if (err)
 		return err;
 
@@ -4026,6 +4041,7 @@ static int macb_probe(struct platform_device *pdev)
 	bp->hclk = hclk;
 	bp->tx_clk = tx_clk;
 	bp->rx_clk = rx_clk;
+	bp->tsu_clk = tsu_clk;
 	if (macb_config)
 		bp->jumbo_max_len = macb_config->jumbo_max_len;
 
@@ -4141,6 +4157,7 @@ static int macb_probe(struct platform_device *pdev)
 	clk_disable_unprepare(hclk);
 	clk_disable_unprepare(pclk);
 	clk_disable_unprepare(rx_clk);
+	clk_disable_unprepare(tsu_clk);
 
 	return err;
 }
@@ -4168,6 +4185,7 @@ static int macb_remove(struct platform_device *pdev)
 		clk_disable_unprepare(bp->hclk);
 		clk_disable_unprepare(bp->pclk);
 		clk_disable_unprepare(bp->rx_clk);
+		clk_disable_unprepare(bp->tsu_clk);
 		of_node_put(bp->phy_node);
 		free_netdev(dev);
 	}
@@ -4193,6 +4211,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 		clk_disable_unprepare(bp->pclk);
 		clk_disable_unprepare(bp->rx_clk);
 	}
+	clk_disable_unprepare(bp->tsu_clk);
 
 	return 0;
 }
@@ -4212,6 +4231,7 @@ static int __maybe_unused macb_resume(struct device *dev)
 		clk_prepare_enable(bp->tx_clk);
 		clk_prepare_enable(bp->rx_clk);
 	}
+	clk_prepare_enable(bp->tsu_clk);
 
 	netif_device_attach(netdev);
 
-- 
2.7.4

[PATCH v2 3/4] net: macb: Add pm runtime support

[Harini Katakam]

From: Harini Katakam <harinik@xilinx.com>

Add runtime pm functions and move clock handling there.
Add runtime PM calls to mdio functions to allow for active mdio bus.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
v2 changes:
Allow for mdio bus to be active

Changes from RFC:
Updated pm get sync/put sync calls.
Removed unecessary clk up in mdio helpers.

 drivers/net/ethernet/cadence/macb_main.c | 121 ++++++++++++++++++++++++++-----
 1 file changed, 101 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 32453d4..4b85ad7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -37,6 +37,7 @@
 #include <linux/udp.h>
 #include <linux/tcp.h>
 #include <linux/iopoll.h>
+#include <linux/pm_runtime.h>
 #include "macb.h"
 
 #define MACB_RX_BUFFER_SIZE	128
@@ -80,6 +81,8 @@
  */
 #define MACB_HALT_TIMEOUT	1230
 
+#define MACB_PM_TIMEOUT  100 /* ms */
+
 #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
 
 /* DMA buffer descriptor might be different size
@@ -335,6 +338,10 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	int value;
 	int err;
 
+	err = pm_runtime_get_sync(&bp->pdev->dev);
+	if (err < 0)
+		return err;
+
 	err = macb_mdio_wait_for_idle(bp);
 	if (err < 0)
 		return err;
@@ -346,11 +353,17 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 			      | MACB_BF(CODE, MACB_MAN_CODE)));
 
 	err = macb_mdio_wait_for_idle(bp);
-	if (err < 0)
+	if (err < 0) {
+		pm_runtime_mark_last_busy(&bp->pdev->dev);
+		pm_runtime_put_autosuspend(&bp->pdev->dev);
 		return err;
+	}
 
 	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
 
+	pm_runtime_mark_last_busy(&bp->pdev->dev);
+	pm_runtime_put_autosuspend(&bp->pdev->dev);
+
 	return value;
 }
 
@@ -360,10 +373,17 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	struct macb *bp = bus->priv;
 	int err;
 
-	err = macb_mdio_wait_for_idle(bp);
+	err = pm_runtime_get_sync(&bp->pdev->dev);
 	if (err < 0)
 		return err;
 
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0) {
+		pm_runtime_mark_last_busy(&bp->pdev->dev);
+		pm_runtime_put_autosuspend(&bp->pdev->dev);
+		return err;
+	}
+
 	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
 			      | MACB_BF(RW, MACB_MAN_WRITE)
 			      | MACB_BF(PHYA, mii_id)
@@ -375,6 +395,9 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	if (err < 0)
 		return err;
 
+	pm_runtime_mark_last_busy(&bp->pdev->dev);
+	pm_runtime_put_autosuspend(&bp->pdev->dev);
+
 	return 0;
 }
 
@@ -2386,12 +2409,18 @@ static int macb_open(struct net_device *dev)
 
 	netdev_dbg(bp->dev, "open\n");
 
+	err = pm_runtime_get_sync(&bp->pdev->dev);
+	if (err < 0)
+		goto pm_exit;
+
 	/* carrier starts down */
 	netif_carrier_off(dev);
 
 	/* if the phy is not yet register, retry later*/
-	if (!dev->phydev)
-		return -EAGAIN;
+	if (!dev->phydev) {
+		err = -EAGAIN;
+		goto pm_exit;
+	}
 
 	/* RX buffers initialization */
 	macb_init_rx_buffer_size(bp, bufsz);
@@ -2400,7 +2429,7 @@ static int macb_open(struct net_device *dev)
 	if (err) {
 		netdev_err(dev, "Unable to allocate DMA memory (error %d)\n",
 			   err);
-		return err;
+		goto pm_exit;
 	}
 
 	bp->macbgem_ops.mog_init_rings(bp);
@@ -2417,6 +2446,11 @@ static int macb_open(struct net_device *dev)
 	if (bp->ptp_info)
 		bp->ptp_info->ptp_init(dev);
 
+pm_exit:
+	if (err) {
+		pm_runtime_put_sync(&bp->pdev->dev);
+		return err;
+	}
 	return 0;
 }
 
@@ -2445,6 +2479,8 @@ static int macb_close(struct net_device *dev)
 	if (bp->ptp_info)
 		bp->ptp_info->ptp_remove(dev);
 
+	pm_runtime_put(&bp->pdev->dev);
+
 	return 0;
 }
 
@@ -4008,6 +4044,11 @@ static int macb_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	pm_runtime_set_autosuspend_delay(&pdev->dev, MACB_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
 	native_io = hw_is_native_io(mem);
 
 	macb_probe_queues(mem, native_io, &queue_mask, &num_queues);
@@ -4139,6 +4180,9 @@ static int macb_probe(struct platform_device *pdev)
 		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
 		    dev->base_addr, dev->irq, dev->dev_addr);
 
+	pm_runtime_mark_last_busy(&bp->pdev->dev);
+	pm_runtime_put_autosuspend(&bp->pdev->dev);
+
 	return 0;
 
 err_out_unregister_mdio:
@@ -4158,6 +4202,9 @@ static int macb_probe(struct platform_device *pdev)
 	clk_disable_unprepare(pclk);
 	clk_disable_unprepare(rx_clk);
 	clk_disable_unprepare(tsu_clk);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 
 	return err;
 }
@@ -4181,11 +4228,16 @@ static int macb_remove(struct platform_device *pdev)
 		mdiobus_free(bp->mii_bus);
 
 		unregister_netdev(dev);
-		clk_disable_unprepare(bp->tx_clk);
-		clk_disable_unprepare(bp->hclk);
-		clk_disable_unprepare(bp->pclk);
-		clk_disable_unprepare(bp->rx_clk);
-		clk_disable_unprepare(bp->tsu_clk);
+		pm_runtime_disable(&pdev->dev);
+		pm_runtime_dont_use_autosuspend(&pdev->dev);
+		if (!pm_runtime_suspended(&pdev->dev)) {
+			clk_disable_unprepare(bp->tx_clk);
+			clk_disable_unprepare(bp->hclk);
+			clk_disable_unprepare(bp->pclk);
+			clk_disable_unprepare(bp->rx_clk);
+			clk_disable_unprepare(bp->tsu_clk);
+			pm_runtime_set_suspended(&pdev->dev);
+		}
 		of_node_put(bp->phy_node);
 		free_netdev(dev);
 	}
@@ -4205,13 +4257,9 @@ static int __maybe_unused macb_suspend(struct device *dev)
 		macb_writel(bp, IER, MACB_BIT(WOL));
 		macb_writel(bp, WOL, MACB_BIT(MAG));
 		enable_irq_wake(bp->queues[0].irq);
-	} else {
-		clk_disable_unprepare(bp->tx_clk);
-		clk_disable_unprepare(bp->hclk);
-		clk_disable_unprepare(bp->pclk);
-		clk_disable_unprepare(bp->rx_clk);
 	}
-	clk_disable_unprepare(bp->tsu_clk);
+
+	pm_runtime_force_suspend(dev);
 
 	return 0;
 }
@@ -4221,11 +4269,43 @@ static int __maybe_unused macb_resume(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
 
+	pm_runtime_force_resume(dev);
+
 	if (bp->wol & MACB_WOL_ENABLED) {
 		macb_writel(bp, IDR, MACB_BIT(WOL));
 		macb_writel(bp, WOL, 0);
 		disable_irq_wake(bp->queues[0].irq);
-	} else {
+	}
+
+	netif_device_attach(netdev);
+
+	return 0;
+}
+
+static int __maybe_unused macb_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct net_device *netdev = platform_get_drvdata(pdev);
+	struct macb *bp = netdev_priv(netdev);
+
+	if (!(device_may_wakeup(&bp->dev->dev))) {
+		clk_disable_unprepare(bp->tx_clk);
+		clk_disable_unprepare(bp->hclk);
+		clk_disable_unprepare(bp->pclk);
+		clk_disable_unprepare(bp->rx_clk);
+	}
+	clk_disable_unprepare(bp->tsu_clk);
+
+	return 0;
+}
+
+static int __maybe_unused macb_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct net_device *netdev = platform_get_drvdata(pdev);
+	struct macb *bp = netdev_priv(netdev);
+
+	if (!(device_may_wakeup(&bp->dev->dev))) {
 		clk_prepare_enable(bp->pclk);
 		clk_prepare_enable(bp->hclk);
 		clk_prepare_enable(bp->tx_clk);
@@ -4233,12 +4313,13 @@ static int __maybe_unused macb_resume(struct device *dev)
 	}
 	clk_prepare_enable(bp->tsu_clk);
 
-	netif_device_attach(netdev);
-
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(macb_pm_ops, macb_suspend, macb_resume);
+static const struct dev_pm_ops macb_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(macb_suspend, macb_resume)
+	SET_RUNTIME_PM_OPS(macb_runtime_suspend, macb_runtime_resume, NULL)
+};
 
 static struct platform_driver macb_driver = {
 	.probe		= macb_probe,
-- 
2.7.4

Re: [PATCH v2 3/4] net: macb: Add pm runtime support

[Andrew Lunn]

> @@ -335,6 +338,10 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  	int value;
>  	int err;
>  
> +	err = pm_runtime_get_sync(&bp->pdev->dev);
> +	if (err < 0)
> +		return err;
> +
>  	err = macb_mdio_wait_for_idle(bp);
>  	if (err < 0)
>  		return err;

Hi Harini

Thanks for adding runtime PM support to the MDIO bus.

It looks like on the error paths you are not always undoing the
pm_runtime_get_sync(). You probably need some sort of goto err;
construct. macb_mdio_write() has the same issue.

	Andrew

Re: [PATCH v2 3/4] net: macb: Add pm runtime support

[Claudiu.Beznea]

On 26.11.2018 09:07, Harini Katakam wrote:
> From: Harini Katakam <harinik@xilinx.com>
> 
> Add runtime pm functions and move clock handling there.
> Add runtime PM calls to mdio functions to allow for active mdio bus.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
> v2 changes:
> Allow for mdio bus to be active
> 
> Changes from RFC:
> Updated pm get sync/put sync calls.
> Removed unecessary clk up in mdio helpers.
> 
>  drivers/net/ethernet/cadence/macb_main.c | 121 ++++++++++++++++++++++++++-----
>  1 file changed, 101 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 32453d4..4b85ad7 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -37,6 +37,7 @@
>  #include <linux/udp.h>
>  #include <linux/tcp.h>
>  #include <linux/iopoll.h>
> +#include <linux/pm_runtime.h>
>  #include "macb.h"
>  
>  #define MACB_RX_BUFFER_SIZE	128
> @@ -80,6 +81,8 @@
>   */
>  #define MACB_HALT_TIMEOUT	1230
>  
> +#define MACB_PM_TIMEOUT  100 /* ms */
> +
>  #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
>  
>  /* DMA buffer descriptor might be different size
> @@ -335,6 +338,10 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  	int value;
>  	int err;
>  
> +	err = pm_runtime_get_sync(&bp->pdev->dev);
> +	if (err < 0)
> +		return err;
> +
>  	err = macb_mdio_wait_for_idle(bp);
>  	if (err < 0)
>  		return err;
> @@ -346,11 +353,17 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  			      | MACB_BF(CODE, MACB_MAN_CODE)));
>  
>  	err = macb_mdio_wait_for_idle(bp);
> -	if (err < 0)
> +	if (err < 0) {
> +		pm_runtime_mark_last_busy(&bp->pdev->dev);
> +		pm_runtime_put_autosuspend(&bp->pdev->dev);

This may look nicer with some sort of goto, if this will remain here.

>  		return err;
> +	}
>  
>  	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
>  
> +	pm_runtime_mark_last_busy(&bp->pdev->dev);
> +	pm_runtime_put_autosuspend(&bp->pdev->dev);
> +
>  	return value;
>  }
>  
> @@ -360,10 +373,17 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  	struct macb *bp = bus->priv;
>  	int err;
>  
> -	err = macb_mdio_wait_for_idle(bp);
> +	err = pm_runtime_get_sync(&bp->pdev->dev);
>  	if (err < 0)
>  		return err;
>  
> +	err = macb_mdio_wait_for_idle(bp);
> +	if (err < 0) {
> +		pm_runtime_mark_last_busy(&bp->pdev->dev);
> +		pm_runtime_put_autosuspend(&bp->pdev->dev);

Ditto

> +		return err;
> +	}
> +
>  	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
>  			      | MACB_BF(RW, MACB_MAN_WRITE)
>  			      | MACB_BF(PHYA, mii_id)
> @@ -375,6 +395,9 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  	if (err < 0)
>  		return err;
>  
> +	pm_runtime_mark_last_busy(&bp->pdev->dev);
> +	pm_runtime_put_autosuspend(&bp->pdev->dev);
> +
>  	return 0;
>  }
>  
> @@ -2386,12 +2409,18 @@ static int macb_open(struct net_device *dev)
>  
>  	netdev_dbg(bp->dev, "open\n");
>  
> +	err = pm_runtime_get_sync(&bp->pdev->dev);
> +	if (err < 0)
> +		goto pm_exit;
> +
>  	/* carrier starts down */
>  	netif_carrier_off(dev);
>  
>  	/* if the phy is not yet register, retry later*/
> -	if (!dev->phydev)
> -		return -EAGAIN;
> +	if (!dev->phydev) {
> +		err = -EAGAIN;
> +		goto pm_exit;
> +	}
>  
>  	/* RX buffers initialization */
>  	macb_init_rx_buffer_size(bp, bufsz);
> @@ -2400,7 +2429,7 @@ static int macb_open(struct net_device *dev)
>  	if (err) {
>  		netdev_err(dev, "Unable to allocate DMA memory (error %d)\n",
>  			   err);
> -		return err;
> +		goto pm_exit;
>  	}
>  
>  	bp->macbgem_ops.mog_init_rings(bp);
> @@ -2417,6 +2446,11 @@ static int macb_open(struct net_device *dev)
>  	if (bp->ptp_info)
>  		bp->ptp_info->ptp_init(dev);
>  
> +pm_exit:
> +	if (err) {
> +		pm_runtime_put_sync(&bp->pdev->dev);
> +		return err;

You could get this of this "return err" and used it bellow, instead of
"return 0"

> +	}
>  	return 0;

Here:
	return err;

>  }
>  
> @@ -2445,6 +2479,8 @@ static int macb_close(struct net_device *dev)
>  	if (bp->ptp_info)
>  		bp->ptp_info->ptp_remove(dev);
>  
> +	pm_runtime_put(&bp->pdev->dev);
> +
>  	return 0;
>  }
>  
> @@ -4008,6 +4044,11 @@ static int macb_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, MACB_PM_TIMEOUT);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
>  	native_io = hw_is_native_io(mem);
>  
>  	macb_probe_queues(mem, native_io, &queue_mask, &num_queues);
> @@ -4139,6 +4180,9 @@ static int macb_probe(struct platform_device *pdev)
>  		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
>  		    dev->base_addr, dev->irq, dev->dev_addr);
>  
> +	pm_runtime_mark_last_busy(&bp->pdev->dev);
> +	pm_runtime_put_autosuspend(&bp->pdev->dev);
> +
>  	return 0;
>  
>  err_out_unregister_mdio:
> @@ -4158,6 +4202,9 @@ static int macb_probe(struct platform_device *pdev)
>  	clk_disable_unprepare(pclk);
>  	clk_disable_unprepare(rx_clk);
>  	clk_disable_unprepare(tsu_clk);
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  
>  	return err;
>  }
> @@ -4181,11 +4228,16 @@ static int macb_remove(struct platform_device *pdev)
>  		mdiobus_free(bp->mii_bus);
>  
>  		unregister_netdev(dev);
> -		clk_disable_unprepare(bp->tx_clk);
> -		clk_disable_unprepare(bp->hclk);
> -		clk_disable_unprepare(bp->pclk);
> -		clk_disable_unprepare(bp->rx_clk);
> -		clk_disable_unprepare(bp->tsu_clk);
> +		pm_runtime_disable(&pdev->dev);
> +		pm_runtime_dont_use_autosuspend(&pdev->dev);
> +		if (!pm_runtime_suspended(&pdev->dev)) {
> +			clk_disable_unprepare(bp->tx_clk);
> +			clk_disable_unprepare(bp->hclk);
> +			clk_disable_unprepare(bp->pclk);
> +			clk_disable_unprepare(bp->rx_clk);
> +			clk_disable_unprepare(bp->tsu_clk);
> +			pm_runtime_set_suspended(&pdev->dev);
> +		}
>  		of_node_put(bp->phy_node);
>  		free_netdev(dev);
>  	}
> @@ -4205,13 +4257,9 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  		macb_writel(bp, IER, MACB_BIT(WOL));
>  		macb_writel(bp, WOL, MACB_BIT(MAG));
>  		enable_irq_wake(bp->queues[0].irq);
> -	} else {
> -		clk_disable_unprepare(bp->tx_clk);
> -		clk_disable_unprepare(bp->hclk);
> -		clk_disable_unprepare(bp->pclk);
> -		clk_disable_unprepare(bp->rx_clk);
>  	}
> -	clk_disable_unprepare(bp->tsu_clk);
> +
> +	pm_runtime_force_suspend(dev);
>  
>  	return 0;
>  }
> @@ -4221,11 +4269,43 @@ static int __maybe_unused macb_resume(struct device *dev)
>  	struct net_device *netdev = dev_get_drvdata(dev);
>  	struct macb *bp = netdev_priv(netdev);
>  
> +	pm_runtime_force_resume(dev);
> +
>  	if (bp->wol & MACB_WOL_ENABLED) {
>  		macb_writel(bp, IDR, MACB_BIT(WOL));
>  		macb_writel(bp, WOL, 0);
>  		disable_irq_wake(bp->queues[0].irq);
> -	} else {
> +	}
> +
> +	netif_device_attach(netdev);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused macb_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct net_device *netdev = platform_get_drvdata(pdev);
> +	struct macb *bp = netdev_priv(netdev);
> +
> +	if (!(device_may_wakeup(&bp->dev->dev))) {
> +		clk_disable_unprepare(bp->tx_clk);
> +		clk_disable_unprepare(bp->hclk);
> +		clk_disable_unprepare(bp->pclk);
> +		clk_disable_unprepare(bp->rx_clk);
> +	}
> +	clk_disable_unprepare(bp->tsu_clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused macb_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct net_device *netdev = platform_get_drvdata(pdev);
> +	struct macb *bp = netdev_priv(netdev);
> +
> +	if (!(device_may_wakeup(&bp->dev->dev))) {
>  		clk_prepare_enable(bp->pclk);
>  		clk_prepare_enable(bp->hclk);
>  		clk_prepare_enable(bp->tx_clk);
> @@ -4233,12 +4313,13 @@ static int __maybe_unused macb_resume(struct device *dev)
>  	}
>  	clk_prepare_enable(bp->tsu_clk);
>  
> -	netif_device_attach(netdev);
> -
>  	return 0;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(macb_pm_ops, macb_suspend, macb_resume);
> +static const struct dev_pm_ops macb_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(macb_suspend, macb_resume)
> +	SET_RUNTIME_PM_OPS(macb_runtime_suspend, macb_runtime_resume, NULL)
> +};
>  
>  static struct platform_driver macb_driver = {
>  	.probe		= macb_probe,
> 

[PATCH v2 4/4] net: macb: Add support for suspend/resume with full power down

[Harini Katakam]

When macb device is suspended and system is powered down, the clocks
are removed and hence macb should be closed gracefully and restored
upon resume. This patch does the same by switching off the net device,
suspending phy and performing necessary cleanup of interrupts and BDs.
Upon resume, all these are reinitialized again.

Reset of macb device is done only when GEM is not a wake device.
Even when gem is a wake device, tx queues can be stopped and ptp device
can be closed (tsu clock will be disabled in pm_runtime_suspend) as
wake event detection has no dependency on this.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
---
v2 changes:
Fixed parameter passed to phy calls.

 drivers/net/ethernet/cadence/macb_main.c | 38 ++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 4b85ad7..dcb0194 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4249,16 +4249,33 @@ static int __maybe_unused macb_suspend(struct device *dev)
 {
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
+	struct macb_queue *queue = bp->queues;
+	unsigned long flags;
+	unsigned int q;
+
+	if (!netif_running(netdev))
+		return 0;
 
-	netif_carrier_off(netdev);
-	netif_device_detach(netdev);
 
 	if (bp->wol & MACB_WOL_ENABLED) {
 		macb_writel(bp, IER, MACB_BIT(WOL));
 		macb_writel(bp, WOL, MACB_BIT(MAG));
 		enable_irq_wake(bp->queues[0].irq);
+		netif_device_detach(netdev);
+	} else {
+		netif_device_detach(netdev);
+		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+			napi_disable(&queue->napi);
+		phy_stop(netdev->phydev);
+		phy_suspend(netdev->phydev);
+		spin_lock_irqsave(&bp->lock, flags);
+		macb_reset_hw(bp);
+		spin_unlock_irqrestore(&bp->lock, flags);
 	}
 
+	netif_carrier_off(netdev);
+	if (bp->ptp_info)
+		bp->ptp_info->ptp_remove(netdev);
 	pm_runtime_force_suspend(dev);
 
 	return 0;
@@ -4268,6 +4285,11 @@ static int __maybe_unused macb_resume(struct device *dev)
 {
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
+	struct macb_queue *queue = bp->queues;
+	unsigned int q;
+
+	if (!netif_running(netdev))
+		return 0;
 
 	pm_runtime_force_resume(dev);
 
@@ -4275,9 +4297,21 @@ static int __maybe_unused macb_resume(struct device *dev)
 		macb_writel(bp, IDR, MACB_BIT(WOL));
 		macb_writel(bp, WOL, 0);
 		disable_irq_wake(bp->queues[0].irq);
+	} else {
+		macb_writel(bp, NCR, MACB_BIT(MPE));
+		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+			napi_enable(&queue->napi);
+		phy_resume(netdev->phydev);
+		phy_init_hw(netdev->phydev);
+		phy_start(netdev->phydev);
 	}
 
+	bp->macbgem_ops.mog_init_rings(bp);
+	macb_init_hw(bp);
+	macb_set_rx_mode(netdev);
 	netif_device_attach(netdev);
+	if (bp->ptp_info)
+		bp->ptp_info->ptp_init(netdev);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v2 4/4] net: macb: Add support for suspend/resume with full power down

[Claudiu.Beznea]

On 26.11.2018 09:07, Harini Katakam wrote:
> When macb device is suspended and system is powered down, the clocks
> are removed and hence macb should be closed gracefully and restored
> upon resume. This patch does the same by switching off the net device,
> suspending phy and performing necessary cleanup of interrupts and BDs.
> Upon resume, all these are reinitialized again.
> 
> Reset of macb device is done only when GEM is not a wake device.
> Even when gem is a wake device, tx queues can be stopped and ptp device
> can be closed (tsu clock will be disabled in pm_runtime_suspend) as
> wake event detection has no dependency on this.
> 
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> ---
> v2 changes:
> Fixed parameter passed to phy calls.
> 
>  drivers/net/ethernet/cadence/macb_main.c | 38 ++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 4b85ad7..dcb0194 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4249,16 +4249,33 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  {
>  	struct net_device *netdev = dev_get_drvdata(dev);
>  	struct macb *bp = netdev_priv(netdev);
> +	struct macb_queue *queue = bp->queues;
> +	unsigned long flags;
> +	unsigned int q;
> +
> +	if (!netif_running(netdev))
> +		return 0;
>  
> -	netif_carrier_off(netdev);
> -	netif_device_detach(netdev);

Is it necessary to remove this from here and have it on every if branch?

>  
>  	if (bp->wol & MACB_WOL_ENABLED) {
>  		macb_writel(bp, IER, MACB_BIT(WOL));
>  		macb_writel(bp, WOL, MACB_BIT(MAG));
>  		enable_irq_wake(bp->queues[0].irq);
> +		netif_device_detach(netdev);
> +	} else {
> +		netif_device_detach(netdev);
> +		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> +			napi_disable(&queue->napi);
> +		phy_stop(netdev->phydev);
> +		phy_suspend(netdev->phydev);
> +		spin_lock_irqsave(&bp->lock, flags);
> +		macb_reset_hw(bp);
> +		spin_unlock_irqrestore(&bp->lock, flags);

In the previous version you said you encountered some crashes while
stressing this part if macb_open()/macb_close() was used in here. Could you
share the tests so that I can debug it on my side?

>  	}
>  
> +	netif_carrier_off(netdev);
> +	if (bp->ptp_info)
> +		bp->ptp_info->ptp_remove(netdev);
>  	pm_runtime_force_suspend(dev);
>  
>  	return 0;
> @@ -4268,6 +4285,11 @@ static int __maybe_unused macb_resume(struct device *dev)
>  {
>  	struct net_device *netdev = dev_get_drvdata(dev);
>  	struct macb *bp = netdev_priv(netdev);
> +	struct macb_queue *queue = bp->queues;
> +	unsigned int q;
> +
> +	if (!netif_running(netdev))
> +		return 0;
>  
>  	pm_runtime_force_resume(dev);
>  
> @@ -4275,9 +4297,21 @@ static int __maybe_unused macb_resume(struct device *dev)
>  		macb_writel(bp, IDR, MACB_BIT(WOL));
>  		macb_writel(bp, WOL, 0);
>  		disable_irq_wake(bp->queues[0].irq);
> +	} else {
> +		macb_writel(bp, NCR, MACB_BIT(MPE));

Just asking... shouldn't other registers be restored here after SoC power
is cut off?

Thank you,
Claudiu Beznea

> +		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> +			napi_enable(&queue->napi);
> +		phy_resume(netdev->phydev);
> +		phy_init_hw(netdev->phydev);
> +		phy_start(netdev->phydev);
>  	}
>  
> +	bp->macbgem_ops.mog_init_rings(bp);
> +	macb_init_hw(bp);
> +	macb_set_rx_mode(netdev);
>  	netif_device_attach(netdev);
> +	if (bp->ptp_info)
> +		bp->ptp_info->ptp_init(netdev);
>  
>  	return 0;
>  }
> 

Re: [PATCH v2 4/4] net: macb: Add support for suspend/resume with full power down

[Harini Katakam]

Hi Claudiu,

On Mon, Nov 26, 2018 at 8:16 PM <Claudiu.Beznea@microchip.com> wrote:
>
>
>
> On 26.11.2018 09:07, Harini Katakam wrote:
<snip>
>
> In the previous version you said you encountered some crashes while
> stressing this part if macb_open()/macb_close() was used in here. Could you
> share the tests so that I can debug it on my side?

Sure, my tests include doing a series of suspend/resume -- it is done
as part of a
random regression script. On ZynqMP, it includes the following to do an
FPD off suspend and then use either UART or WOL as a wake source and
repeat the same, sometimes pinging in between:

echo pm_request_wakeup 8 1 0 1 > /sys/kernel/debug/zynqmp-firmware/pm
echo pm_force_powerdown 6 > /sys/kernel/debug/zynqmp-firmware/pm
echo enabled > /sys/devices/platform/amba/ff000000.serial/tty/ttyPS0/power/wakeup
echo enabled > /sys/devices/platform/amba/ff0e0000.ethernet/net/eth0/power/wakeup
echo mem > /sys/power/state

ping does not work sometimes and after a few iterations (sometimes
20), a crash can be
observed tracing from "kfree_skb_list" to "macb_free_consistent". From
the flow, the
only explanation I could come up with was that there was an attempt to
free buffers that
may not have been allocated. Also, this was time consuming in the
suspend/resume path.

<snip>
> > +     } else {
> > +             macb_writel(bp, NCR, MACB_BIT(MPE));
>
> Just asking... shouldn't other registers be restored here after SoC power
> is cut off?
Ideally yes. But in this series, I'm just attempting to re-initialize
to a working
state. I can try context save/restore later.

Regards,
Harini

Re: [PATCH v2 4/4] net: macb: Add support for suspend/resume with full power down

[Claudiu.Beznea]

Hi Harini,

On 27.11.2018 08:25, Harini Katakam wrote:
> Hi Claudiu,
> 
> On Mon, Nov 26, 2018 at 8:16 PM <Claudiu.Beznea@microchip.com> wrote:
>>
>>
>>
>> On 26.11.2018 09:07, Harini Katakam wrote:
> <snip>
>>
>> In the previous version you said you encountered some crashes while
>> stressing this part if macb_open()/macb_close() was used in here. Could you
>> share the tests so that I can debug it on my side?
> 
> Sure, my tests include doing a series of suspend/resume -- it is done
> as part of a
> random regression script. On ZynqMP, it includes the following to do an
> FPD off suspend and then use either UART or WOL as a wake source and
> repeat the same, sometimes pinging in between:
> 
> echo pm_request_wakeup 8 1 0 1 > /sys/kernel/debug/zynqmp-firmware/pm
> echo pm_force_powerdown 6 > /sys/kernel/debug/zynqmp-firmware/pm
> echo enabled > /sys/devices/platform/amba/ff000000.serial/tty/ttyPS0/power/wakeup
> echo enabled > /sys/devices/platform/amba/ff0e0000.ethernet/net/eth0/power/wakeup
> echo mem > /sys/power/state
> 
> ping does not work sometimes and after a few iterations (sometimes
> 20), a crash can be
> observed tracing from "kfree_skb_list" to "macb_free_consistent". From
> the flow, the
> only explanation I could come up with was that there was an attempt to
> free buffers that
> may not have been allocated. Also, this was time consuming in the
> suspend/resume path.

Thank you for all these. I will try it on my side to see if I can reproduce it.

> 
> <snip>
>>> +     } else {
>>> +             macb_writel(bp, NCR, MACB_BIT(MPE));
>>
>> Just asking... shouldn't other registers be restored here after SoC power
>> is cut off?
> Ideally yes. But in this series, I'm just attempting to re-initialize
> to a working
> state. I can try context save/restore later.

I see. I have tried it on my own with SAMA5D2 backup and self-refresh mode
where the core is shut down and it doesn't resume OK. Anyway, no issue, I
can add my context save/restore after this is accepted.

The idea with macb_open()/macb_close() was that it is doing everything for
you and every time new things are implemented you wouldn't have to add them
also in suspend/resume functions.

Thank you,
Claudiu Beznea

> Regards,
> Harini
>