diff mbox series

[net-next,v3,07/19] net: ravb: Move reference clock enable/disable on runtime PM APIs

Message ID 20240105082339.1468817-8-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches (and no cover letter)
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 17 this patch: 1113
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang fail Errors and warnings before: 18 this patch: 1140
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 17 this patch: 1140
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 224 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Claudiu Jan. 5, 2024, 8:23 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reference clock could be or not part of the power domain. If it is part of
the power domain, the power domain takes care of propertly setting it. In
case it is not part of the power domain and full runtime PM support is
available in driver the clock will not be propertly disabled/enabled at
runtime. For this, keep the prepare/unprepare operations in the driver's
probe()/remove() functions and move the enable/disable in runtime PM
functions.

Along with it, the other clock request operations were moved close to
reference clock request and prepare to have all the clock requests
specific code grouped together.

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- squashed with patch 17/21 ("net: ravb: Keep clock request operations grouped
  together") from v2
- collected tags

Changes in v2:
- this patch is new and follows the recommendations proposed in the
  discussion of patch 08/13 ("net: ravb: Rely on PM domain to enable refclk")
  from v2

 drivers/net/ethernet/renesas/ravb_main.c | 110 ++++++++++++-----------
 1 file changed, 57 insertions(+), 53 deletions(-)

Comments

Sergey Shtylyov Jan. 5, 2024, 7:52 p.m. UTC | #1 
On 1/5/24 11:23 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Reference clock could be or not part of the power domain. If it is part of
> the power domain, the power domain takes care of propertly setting it. In
> case it is not part of the power domain and full runtime PM support is
> available in driver the clock will not be propertly disabled/enabled at
> runtime. For this, keep the prepare/unprepare operations in the driver's
> probe()/remove() functions and move the enable/disable in runtime PM
> functions.
> 
> Along with it, the other clock request operations were moved close to
> reference clock request and prepare to have all the clock requests
> specific code grouped together.
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

   It's not that I reviewed the squashed version of this patch...

> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v3:
> - squashed with patch 17/21 ("net: ravb: Keep clock request operations grouped
>   together") from v2
> - collected tags
> 
> Changes in v2:
> - this patch is new and follows the recommendations proposed in the
>   discussion of patch 08/13 ("net: ravb: Rely on PM domain to enable refclk")
>   from v2
> 
>  drivers/net/ethernet/renesas/ravb_main.c | 110 ++++++++++++-----------
>  1 file changed, 57 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 844ac3306e93..4673cc2faec0 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -2697,10 +2692,37 @@ static int ravb_probe(struct platform_device *pdev)
>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>  	}
>  
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		error = PTR_ERR(priv->clk);
> +		goto out_reset_assert;
> +	}
> +
> +	if (info->gptp_ref_clk) {
> +		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
> +		if (IS_ERR(priv->gptp_clk)) {
> +			error = PTR_ERR(priv->gptp_clk);
> +			goto out_reset_assert;
> +		}
> +	}
> +
> +	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
> +	if (IS_ERR(priv->refclk)) {
> +		error = PTR_ERR(priv->refclk);
> +		goto out_reset_assert;
> +	}
> +	clk_prepare(priv->refclk);
> +
> +	platform_set_drvdata(pdev, ndev);

   Why exactly you had to move this line?

> +	pm_runtime_enable(&pdev->dev);
> +	error = pm_runtime_resume_and_get(&pdev->dev);
> +	if (error < 0)
> +		goto out_rpm_disable;
> +
>  	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>  	if (IS_ERR(priv->addr)) {
>  		error = PTR_ERR(priv->addr);
> -		goto out_release;
> +		goto out_rpm_put;
>  	}
>  
>  	/* The Ether-specific entries in the device structure. */
[...]
> @@ -2871,8 +2872,6 @@ static int ravb_probe(struct platform_device *pdev)
>  	netdev_info(ndev, "Base address at %#x, %pM, IRQ %d.\n",
>  		    (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
>  
> -	platform_set_drvdata(pdev, ndev);

   Hm, wasn't calling it here racy?

> -
>  	return 0;
>  
>  out_napi_del:
[...]
> @@ -3060,21 +3058,27 @@ static int ravb_resume(struct device *dev)
>  	return ret;
>  }
>  
> -static int ravb_runtime_nop(struct device *dev)
> +static int ravb_runtime_suspend(struct device *dev)
>  {
> -	/* Runtime PM callback shared between ->runtime_suspend()
> -	 * and ->runtime_resume(). Simply returns success.
> -	 *
> -	 * This driver re-initializes all registers after
> -	 * pm_runtime_get_sync() anyway so there is no need
> -	 * to save and restore registers here.
> -	 */

   Perhaps even worth a separate patch to completely remove this function
which doesn't seem to make sense?

[...]

MBR, Sergey
Claudiu Jan. 8, 2024, 8:03 a.m. UTC | #2 
On 05.01.2024 21:52, Sergey Shtylyov wrote:
> On 1/5/24 11:23 AM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Reference clock could be or not part of the power domain. If it is part of
>> the power domain, the power domain takes care of propertly setting it. In
>> case it is not part of the power domain and full runtime PM support is
>> available in driver the clock will not be propertly disabled/enabled at
>> runtime. For this, keep the prepare/unprepare operations in the driver's
>> probe()/remove() functions and move the enable/disable in runtime PM
>> functions.
>>
>> Along with it, the other clock request operations were moved close to
>> reference clock request and prepare to have all the clock requests
>> specific code grouped together.
>>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
>    It's not that I reviewed the squashed version of this patch...

I had a Rb on "net: ravb: Move reference clock enable/disable on runtime PM
APIs" from v2 and an OK from you (no other comments) to do the squash on
"net: ravb: Keep clock request operations grouped together" from v2 thus I
consider keeping Rb is OK.

> 
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v3:
>> - squashed with patch 17/21 ("net: ravb: Keep clock request operations grouped
>>   together") from v2
>> - collected tags
>>
>> Changes in v2:
>> - this patch is new and follows the recommendations proposed in the
>>   discussion of patch 08/13 ("net: ravb: Rely on PM domain to enable refclk")
>>   from v2
>>
>>  drivers/net/ethernet/renesas/ravb_main.c | 110 ++++++++++++-----------
>>  1 file changed, 57 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 844ac3306e93..4673cc2faec0 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -2697,10 +2692,37 @@ static int ravb_probe(struct platform_device *pdev)
>>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>>  	}
>>  
>> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(priv->clk)) {
>> +		error = PTR_ERR(priv->clk);
>> +		goto out_reset_assert;
>> +	}
>> +
>> +	if (info->gptp_ref_clk) {
>> +		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
>> +		if (IS_ERR(priv->gptp_clk)) {
>> +			error = PTR_ERR(priv->gptp_clk);
>> +			goto out_reset_assert;
>> +		}
>> +	}
>> +
>> +	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
>> +	if (IS_ERR(priv->refclk)) {
>> +		error = PTR_ERR(priv->refclk);
>> +		goto out_reset_assert;
>> +	}
>> +	clk_prepare(priv->refclk);
>> +
>> +	platform_set_drvdata(pdev, ndev);
> 
>    Why exactly you had to move this line?

Calling pm_runtime_resume_and_get() above will implicitly call the
ravb_runtime_resume() which calls dev_get_drvdata() to get proper data for
refclk.

> 
>> +	pm_runtime_enable(&pdev->dev);
>> +	error = pm_runtime_resume_and_get(&pdev->dev);
>> +	if (error < 0)
>> +		goto out_rpm_disable;
>> +
>>  	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>  	if (IS_ERR(priv->addr)) {
>>  		error = PTR_ERR(priv->addr);
>> -		goto out_release;
>> +		goto out_rpm_put;
>>  	}
>>  
>>  	/* The Ether-specific entries in the device structure. */
> [...]
>> @@ -2871,8 +2872,6 @@ static int ravb_probe(struct platform_device *pdev)
>>  	netdev_info(ndev, "Base address at %#x, %pM, IRQ %d.\n",
>>  		    (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
>>  
>> -	platform_set_drvdata(pdev, ndev);
> 
>    Hm, wasn't calling it here racy?

Haven't noticed that. Racing with who? AFAICT the only functions that uses
this are remove, suspend, resume specific ones.

> 
>> -
>>  	return 0;
>>  
>>  out_napi_del:
> [...]
>> @@ -3060,21 +3058,27 @@ static int ravb_resume(struct device *dev)
>>  	return ret;
>>  }
>>  
>> -static int ravb_runtime_nop(struct device *dev)
>> +static int ravb_runtime_suspend(struct device *dev)
>>  {
>> -	/* Runtime PM callback shared between ->runtime_suspend()
>> -	 * and ->runtime_resume(). Simply returns success.
>> -	 *
>> -	 * This driver re-initializes all registers after
>> -	 * pm_runtime_get_sync() anyway so there is no need
>> -	 * to save and restore registers here.
>> -	 */
> 
>    Perhaps even worth a separate patch to completely remove this function
> which doesn't seem to make sense?

Why? With that the refclk will not be properly enabled/disabled when it
will not be part of the power domain. Take
https://elixir.bootlin.com/linux/v6.7/source/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi#L57
as an example. Here refclk is from an external source (not part of power
domain).

Thank you,
Claudiu Beznea

> 
> [...]
> 
> MBR, Sergey
Sergey Shtylyov Jan. 23, 2024, 8:06 p.m. UTC | #3 
On 1/8/24 11:03 AM, claudiu beznea wrote:

   Oops, looks like I forgot to reply to this one...

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Reference clock could be or not part of the power domain. If it is part of
>>> the power domain, the power domain takes care of propertly setting it. In
>>> case it is not part of the power domain and full runtime PM support is
>>> available in driver the clock will not be propertly disabled/enabled at
>>> runtime. For this, keep the prepare/unprepare operations in the driver's
>>> probe()/remove() functions and move the enable/disable in runtime PM
>>> functions.
>>>
>>> Along with it, the other clock request operations were moved close to
>>> reference clock request and prepare to have all the clock requests
>>> specific code grouped together.
>>>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 844ac3306e93..4673cc2faec0 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>> +	pm_runtime_enable(&pdev->dev);
>>> +	error = pm_runtime_resume_and_get(&pdev->dev);
>>> +	if (error < 0)
>>> +		goto out_rpm_disable;
>>> +
>>>  	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>>  	if (IS_ERR(priv->addr)) {
>>>  		error = PTR_ERR(priv->addr);
>>> -		goto out_release;
>>> +		goto out_rpm_put;
>>>  	}
>>>  
>>>  	/* The Ether-specific entries in the device structure. */
[...]
>>> @@ -3060,21 +3058,27 @@ static int ravb_resume(struct device *dev)
>>>  	return ret;
>>>  }
>>>  
>>> -static int ravb_runtime_nop(struct device *dev)
>>> +static int ravb_runtime_suspend(struct device *dev)
>>>  {
>>> -	/* Runtime PM callback shared between ->runtime_suspend()
>>> -	 * and ->runtime_resume(). Simply returns success.
>>> -	 *
>>> -	 * This driver re-initializes all registers after
>>> -	 * pm_runtime_get_sync() anyway so there is no need
>>> -	 * to save and restore registers here.
>>> -	 */
>>
>>    Perhaps even worth a separate patch to completely remove this function
>> which doesn't seem to make sense?
> 
> Why? With that the refclk will not be properly enabled/disabled when it
> will not be part of the power domain. Take

   That's what you are adding in this patch, right? Before this patch
this was ravb_runtime_nop(), always returning 0. Did it make any sense?

> https://elixir.bootlin.com/linux/v6.7/source/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi#L57
> as an example. Here refclk is from an external source (not part of power
> domain).
> 
> Thank you,
> Claudiu Beznea

[...]

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 844ac3306e93..4673cc2faec0 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2664,11 +2664,6 @@  static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_free_netdev;
 
-	pm_runtime_enable(&pdev->dev);
-	error = pm_runtime_resume_and_get(&pdev->dev);
-	if (error < 0)
-		goto out_rpm_disable;
-
 	if (info->multi_irqs) {
 		if (info->err_mgmt_irqs)
 			irq = platform_get_irq_byname(pdev, "dia");
@@ -2679,7 +2674,7 @@  static int ravb_probe(struct platform_device *pdev)
 	}
 	if (irq < 0) {
 		error = irq;
-		goto out_release;
+		goto out_reset_assert;
 	}
 	ndev->irq = irq;
 
@@ -2697,10 +2692,37 @@  static int ravb_probe(struct platform_device *pdev)
 		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
 	}
 
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		error = PTR_ERR(priv->clk);
+		goto out_reset_assert;
+	}
+
+	if (info->gptp_ref_clk) {
+		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
+		if (IS_ERR(priv->gptp_clk)) {
+			error = PTR_ERR(priv->gptp_clk);
+			goto out_reset_assert;
+		}
+	}
+
+	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
+	if (IS_ERR(priv->refclk)) {
+		error = PTR_ERR(priv->refclk);
+		goto out_reset_assert;
+	}
+	clk_prepare(priv->refclk);
+
+	platform_set_drvdata(pdev, ndev);
+	pm_runtime_enable(&pdev->dev);
+	error = pm_runtime_resume_and_get(&pdev->dev);
+	if (error < 0)
+		goto out_rpm_disable;
+
 	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(priv->addr)) {
 		error = PTR_ERR(priv->addr);
-		goto out_release;
+		goto out_rpm_put;
 	}
 
 	/* The Ether-specific entries in the device structure. */
@@ -2711,7 +2733,7 @@  static int ravb_probe(struct platform_device *pdev)
 
 	error = of_get_phy_mode(np, &priv->phy_interface);
 	if (error && error != -ENODEV)
-		goto out_release;
+		goto out_rpm_put;
 
 	priv->no_avb_link = of_property_read_bool(np, "renesas,no-ether-link");
 	priv->avb_link_active_low =
@@ -2724,14 +2746,14 @@  static int ravb_probe(struct platform_device *pdev)
 			irq = platform_get_irq_byname(pdev, "ch24");
 		if (irq < 0) {
 			error = irq;
-			goto out_release;
+			goto out_rpm_put;
 		}
 		priv->emac_irq = irq;
 		for (i = 0; i < NUM_RX_QUEUE; i++) {
 			irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
 			if (irq < 0) {
 				error = irq;
-				goto out_release;
+				goto out_rpm_put;
 			}
 			priv->rx_irqs[i] = irq;
 		}
@@ -2739,7 +2761,7 @@  static int ravb_probe(struct platform_device *pdev)
 			irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
 			if (irq < 0) {
 				error = irq;
-				goto out_release;
+				goto out_rpm_put;
 			}
 			priv->tx_irqs[i] = irq;
 		}
@@ -2748,40 +2770,19 @@  static int ravb_probe(struct platform_device *pdev)
 			irq = platform_get_irq_byname(pdev, "err_a");
 			if (irq < 0) {
 				error = irq;
-				goto out_release;
+				goto out_rpm_put;
 			}
 			priv->erra_irq = irq;
 
 			irq = platform_get_irq_byname(pdev, "mgmt_a");
 			if (irq < 0) {
 				error = irq;
-				goto out_release;
+				goto out_rpm_put;
 			}
 			priv->mgmta_irq = irq;
 		}
 	}
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(priv->clk)) {
-		error = PTR_ERR(priv->clk);
-		goto out_release;
-	}
-
-	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
-	if (IS_ERR(priv->refclk)) {
-		error = PTR_ERR(priv->refclk);
-		goto out_release;
-	}
-	clk_prepare_enable(priv->refclk);
-
-	if (info->gptp_ref_clk) {
-		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
-		if (IS_ERR(priv->gptp_clk)) {
-			error = PTR_ERR(priv->gptp_clk);
-			goto out_disable_refclk;
-		}
-	}
-
 	ndev->max_mtu = info->rx_max_buf_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;
 
@@ -2799,13 +2800,13 @@  static int ravb_probe(struct platform_device *pdev)
 	/* Set AVB config mode */
 	error = ravb_set_config_mode(ndev);
 	if (error)
-		goto out_disable_gptp_clk;
+		goto out_rpm_put;
 
 	if (info->gptp || info->ccc_gac) {
 		/* Set GTI value */
 		error = ravb_set_gti(ndev);
 		if (error)
-			goto out_disable_refclk;
+			goto out_rpm_put;
 
 		/* Request GTI loading */
 		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
@@ -2825,7 +2826,7 @@  static int ravb_probe(struct platform_device *pdev)
 			"Cannot allocate desc base address table (size %d bytes)\n",
 			priv->desc_bat_size);
 		error = -ENOMEM;
-		goto out_disable_refclk;
+		goto out_rpm_put;
 	}
 	for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
 		priv->desc_bat[q].die_dt = DT_EOS;
@@ -2871,8 +2872,6 @@  static int ravb_probe(struct platform_device *pdev)
 	netdev_info(ndev, "Base address at %#x, %pM, IRQ %d.\n",
 		    (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
 
-	platform_set_drvdata(pdev, ndev);
-
 	return 0;
 
 out_napi_del:
@@ -2888,12 +2887,12 @@  static int ravb_probe(struct platform_device *pdev)
 	/* Stop PTP Clock driver */
 	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
-out_disable_refclk:
-	clk_disable_unprepare(priv->refclk);
-out_release:
+out_rpm_put:
 	pm_runtime_put(&pdev->dev);
 out_rpm_disable:
 	pm_runtime_disable(&pdev->dev);
+	clk_unprepare(priv->refclk);
+out_reset_assert:
 	reset_control_assert(rstc);
 out_free_netdev:
 	free_netdev(ndev);
@@ -2922,10 +2921,9 @@  static void ravb_remove(struct platform_device *pdev)
 
 	ravb_set_opmode(ndev, CCC_OPC_RESET);
 
-	clk_disable_unprepare(priv->refclk);
-
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	clk_unprepare(priv->refclk);
 	reset_control_assert(priv->rstc);
 	free_netdev(ndev);
 	platform_set_drvdata(pdev, NULL);
@@ -3060,21 +3058,27 @@  static int ravb_resume(struct device *dev)
 	return ret;
 }
 
-static int ravb_runtime_nop(struct device *dev)
+static int ravb_runtime_suspend(struct device *dev)
 {
-	/* Runtime PM callback shared between ->runtime_suspend()
-	 * and ->runtime_resume(). Simply returns success.
-	 *
-	 * This driver re-initializes all registers after
-	 * pm_runtime_get_sync() anyway so there is no need
-	 * to save and restore registers here.
-	 */
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	clk_disable(priv->refclk);
+
 	return 0;
 }
 
+static int ravb_runtime_resume(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	return clk_enable(priv->refclk);
+}
+
 static const struct dev_pm_ops ravb_dev_pm_ops = {
 	SYSTEM_SLEEP_PM_OPS(ravb_suspend, ravb_resume)
-	RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
+	RUNTIME_PM_OPS(ravb_runtime_suspend, ravb_runtime_resume, NULL)
 };
 
 static struct platform_driver ravb_driver = {