diff mbox series

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

Message ID 20240123125829.3970325-8-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series net: ravb: Prepare for suspend to RAM and runtime PM support (part 1) | expand

Commit Message

Claudiu Beznea Jan. 23, 2024, 12:58 p.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.

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

Changes in v4:
- dropped tag

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. 23, 2024, 8:43 p.m. UTC | #1 
On 1/23/24 3:58 PM, 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

   Could be or not be, perhaps?

> the power domain, the power domain takes care of propertly setting it. In

   Properly. :-)

> 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.
> 
> 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 9fc0e39e33c2..4673cc2faec0 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -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.
> -	 */

   I want to pull out the dummy {ravb|sh_eth}_runtime_nop() funcs --
they don't seem to be necessary... Then we can implement your clock
dance with freshly added ravb_runtime_{suspend|resume}()...

[...]

MBR, Sergey
Claudiu Beznea Jan. 29, 2024, 1:53 p.m. UTC | #2 
On 23.01.2024 22:43, Sergey Shtylyov wrote:
> On 1/23/24 3:58 PM, 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
> 
>    Could be or not be, perhaps?
> 
>> the power domain, the power domain takes care of propertly setting it. In
> 
>    Properly. :-)
> 
>> 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.
>>
>> 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 9fc0e39e33c2..4673cc2faec0 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -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.
>> -	 */
> 
>    I want to pull out the dummy {ravb|sh_eth}_runtime_nop() funcs --
> they don't seem to be necessary... Then we can implement your clock
> dance with freshly added ravb_runtime_{suspend|resume}()...

For this series, does it worth having a patch that removes ravb runtime
suspend/resume ops to then add a new patch that add it it again?
I can do it but it I see no reason in doing it in this series...

The dummy functions were there and the commit description explains the
reason they were updated.

Thank you,
Claudiu Beznea

> 
> [...]
> 
> MBR, Sergey
Sergey Shtylyov Jan. 30, 2024, 6:22 p.m. UTC | #3 
On 1/29/24 4:53 PM, claudiu beznea 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
>>
>>    Could be or not be, perhaps?
>>
>>> the power domain, the power domain takes care of propertly setting it. In
>>
>>    Properly. :-)
>>
>>> 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.
>>>
>>> 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 9fc0e39e33c2..4673cc2faec0 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> [...]
>>> @@ -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.
>>> -	 */
>>
>>    I want to pull out the dummy {ravb|sh_eth}_runtime_nop() funcs --
>> they don't seem to be necessary... Then we can implement your clock

   The need to have the dummy RPM suspend/resume methods is gone since:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=63d00be69348fda431ae59aba6af268a5cf5058e

>> dance with freshly added ravb_runtime_{suspend|resume}()...
> 
> For this series, does it worth having a patch that removes ravb runtime
> suspend/resume ops to then add a new patch that add it it again?

    Probably not, indeed... I just wanted to have 2 symmetric patches
for sh_eth and ravb removing the dummy methods...

> I can do it but it I see no reason in doing it in this series...
> 
> The dummy functions were there and the commit description explains the
> reason they were updated.

   Yet you don't say a word about the big comment in ravb_runtime_nop()
that you remove. This comment doesn't really make much sense as this
driver currently has the RPM calls and ndo_{open|stop}() methods decoupled...
This stuff was copied from sh_eth.c verbatim -- I clearly overlooked it when
prepping this driver for upstream... :-<
   You can keep this patch as is (but not its description!) or have a separate
patch that removes just the big comment not making much sense, both options
would be fine by me. I will take care of sh_eth.c myself (not really sure
whether you have targets having this IP)...

> 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 9fc0e39e33c2..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_refclk;
+		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 = {