[PATCH net-next v2 00/21] net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S

[PATCH net-next v2 00/21] net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

This series adds suspend to RAM and runtime PM support for Ethernet
IP available on the RZ/G3S (R9A08G045) SoC.

As there are IP versions that switch to module standby when disabling
the clocks, and because of module standby IP switches to reset and
the register content is lost, to be able to have runtime PM supported
for all IP variants, the configuration operations were moved all to
ravb_open()/ravb_close() letting the ravb_probe() and ravb_remove()
to deal with resource parsing and allocation/free.

The ethtool and IOCTL APIs that could have been run asyncronously
were adapted to return if the interface is down. As explained in
each individual commits description, this should be harmless.

Along with it, the series contains preparatory cleanups.

The series has been tested on the boards with the following device trees:
- r8a7742-iwg21d-q7.dts
- r8a774a1-hihope-rzg2m-ex.dts 
- r9a07g043u11-smarc-rzg2ul.dts
- r9a07g054l2-smarc-rzv2l.dts
- r9a07g044l2-smarc-rzg2l.dts

Patches are based on series at [1].

Thank you,
Claudiu Beznea

Changes in v2:
- rework the driver (mainly, ravb_open() contains now only resource
  allocation and parsing leaving the settings to ravb_open(); ravb_remove()
  has been adapted accordingly) to be able to use runtime PM for all
  IP variants; due to this number of patches increased
- adjust previous series to review comments
- collected tags
- populated driver's own runtime PM ops with enable/disable of reference
  clock

[1] https://lore.kernel.org/all/20231214113137.2450292-1-claudiu.beznea.uj@bp.renesas.com/

Claudiu Beznea (21):
  net: ravb: Let IP-specific receive function to interrogate descriptors
  net: ravb: Rely on PM domain to enable gptp_clk
  net: ravb: Make reset controller support mandatory
  net: ravb: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and
    pm_ptr()
  net: ravb: Use tabs instead of spaces
  net: ravb: Assert/de-assert reset on suspend/resume
  net: ravb: Move reference clock enable/disable on runtime PM APIs
  net: ravb: Move the IRQs get and request in the probe function
  net: ravb: Split GTI computation and set operations
  net: ravb: Move delay mode set in the driver's ndo_open API
  net: ravb: Move DBAT configuration to the driver's ndo_open API
  net: ravb: Move ptp initialization in the driver's ndo_open API for
    ccc_gac platorms
  net: ravb: Set config mode in ndo_open and reset mode in ndo_close
  net: ravb: Simplify ravb_suspend()
  net: ravb: Simplify ravb_resume()
  net: ravb: Keep the reverse order of operations in ravb_close()
  net: ravb: Keep clock request operations grouped together
  net: ravb: Return cached statistics if the interface is down
  net: ravb: Do not set promiscuous mode if the interface is down
  net: ravb: Do not apply RX CSUM settings to hardware if the interface
    is down
  net: ravb: Add runtime PM support

 drivers/net/ethernet/renesas/ravb.h      |   2 +
 drivers/net/ethernet/renesas/ravb_main.c | 783 ++++++++++++-----------
 2 files changed, 417 insertions(+), 368 deletions(-)

-- 
2.39.2

[PATCH net-next v2 01/21] net: ravb: Let IP-specific receive function to interrogate descriptors

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

ravb_poll() initial code used to interrogate the first descriptor of the
RX queue in case gPTP is false to determine if ravb_rx() should be called.
This is done for non-gPTP IPs. For gPTP IPs the driver PTP-specific
information was used to determine if receive function should be called. As
every IP has its own receive function that interrogates the RX descriptors
list in the same way the ravb_poll() was doing there is no need to double
check this in ravb_poll(). Removing the code from ravb_poll() leads to a
cleaner code.

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

Changes in v2:
- addressed review comments and keep stale code out of this patch


 drivers/net/ethernet/renesas/ravb_main.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 1c253403a297..8e67a18b2924 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1282,25 +1282,16 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	struct net_device *ndev = napi->dev;
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
-	bool gptp = info->gptp || info->ccc_gac;
-	struct ravb_rx_desc *desc;
 	unsigned long flags;
 	int q = napi - priv->napi;
 	int mask = BIT(q);
 	int quota = budget;
-	unsigned int entry;
 
-	if (!gptp) {
-		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
-		desc = &priv->gbeth_rx_ring[entry];
-	}
 	/* Processing RX Descriptor Ring */
 	/* Clear RX interrupt */
 	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
-	if (gptp || desc->die_dt != DT_FEMPTY) {
-		if (ravb_rx(ndev, &quota, q))
-			goto out;
-	}
+	if (ravb_rx(ndev, &quota, q))
+		goto out;
 
 	/* Processing TX Descriptor Ring */
 	spin_lock_irqsave(&priv->lock, flags);
-- 
2.39.2

Re: [PATCH net-next v2 01/21] net: ravb: Let IP-specific receive function to interrogate descriptors

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> ravb_poll() initial code used to interrogate the first descriptor of the
> RX queue in case gPTP is false to determine if ravb_rx() should be called.
> This is done for non-gPTP IPs. For gPTP IPs the driver PTP-specific
> information was used to determine if receive function should be called. As
> every IP has its own receive function that interrogates the RX descriptors
> list in the same way the ravb_poll() was doing there is no need to double
> check this in ravb_poll(). Removing the code from ravb_poll() leads to a
> cleaner code.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[PATCH net-next v2 02/21] net: ravb: Rely on PM domain to enable gptp_clk

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

ravb_rzv2m_hw_info::gptp_ref_clk is enabled only for RZ/V2M. RZ/V2M
is an ARM64-based device which selects power domains by default and
CONFIG_PM. The RZ/V2M Ethernet DT node has proper power-domain binding
available in device tree from the commit that added the Ethernet node.
(4872ca1f92b0 ("arm64: dts: renesas: r9a09g011: Add ethernet nodes")).

Power domain support was available in the rzg2l-cpg.c driver when the
Ethernet DT node has been enabled in RZ/V2M device tree.
(ef3c613ccd68 ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC")).

Thus, remove the explicit clock enable for gptp_clk (and treat it as the
other clocks are treated) as it is not needed and removing it doesn't
break the ABI according to the above explanations.

By removing the enable/disable operation from the driver we can add
runtime PM support (which operates on clocks) w/o the need to handle
the gptp_clk in the Ethernet driver functions like ravb_runtime_nop().
PM domain does all that is needed.

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

Changes in v2:
- collected tags

 drivers/net/ethernet/renesas/ravb_main.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 8e67a18b2924..aa5e9b27ed31 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2780,7 +2780,6 @@ static int ravb_probe(struct platform_device *pdev)
 			error = PTR_ERR(priv->gptp_clk);
 			goto out_disable_refclk;
 		}
-		clk_prepare_enable(priv->gptp_clk);
 	}
 
 	ndev->max_mtu = info->rx_max_buf_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
@@ -2806,7 +2805,7 @@ static int ravb_probe(struct platform_device *pdev)
 		/* Set GTI value */
 		error = ravb_set_gti(ndev);
 		if (error)
-			goto out_disable_gptp_clk;
+			goto out_disable_refclk;
 
 		/* Request GTI loading */
 		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
@@ -2830,7 +2829,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_gptp_clk;
+		goto out_disable_refclk;
 	}
 	for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
 		priv->desc_bat[q].die_dt = DT_EOS;
@@ -2893,8 +2892,6 @@ static int ravb_probe(struct platform_device *pdev)
 	/* Stop PTP Clock driver */
 	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
-out_disable_gptp_clk:
-	clk_disable_unprepare(priv->gptp_clk);
 out_disable_refclk:
 	clk_disable_unprepare(priv->refclk);
 out_release:
@@ -2932,7 +2929,6 @@ static void ravb_remove(struct platform_device *pdev)
 	if (error)
 		netdev_err(ndev, "Failed to reset ndev\n");
 
-	clk_disable_unprepare(priv->gptp_clk);
 	clk_disable_unprepare(priv->refclk);
 
 	pm_runtime_put_sync(&pdev->dev);
-- 
2.39.2

[PATCH net-next v2 03/21] net: ravb: Make reset controller support mandatory

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the RZ/G3S SoC the reset controller is mandatory for the IP to work.
The device tree binding documentation for the ravb driver specifies that
the resets are mandatory. Based on this, make the resets mandatory also in
driver for all ravb devices.

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

Changes in v2:
- collected tags

 drivers/net/ethernet/renesas/ravb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index aa5e9b27ed31..b4d5a14ac4e5 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2645,7 +2645,7 @@ static int ravb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 	if (IS_ERR(rstc))
 		return dev_err_probe(&pdev->dev, PTR_ERR(rstc),
 				     "failed to get cpg reset\n");
-- 
2.39.2

[PATCH net-next v2 04/21] net: ravb: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and pm_ptr()

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() are deprecated now
and require __maybe_unused protection against unused function warnings.
The usage of pm_ptr() and SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() allows
the compiler to see the functions, thus suppressing the warning. Thus
drop the __maybe_unused markings.

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

Changes in v2:
- collected tags

 drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index b4d5a14ac4e5..82085bb9b7a3 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2978,7 +2978,7 @@ static int ravb_wol_restore(struct net_device *ndev)
 	return disable_irq_wake(priv->emac_irq);
 }
 
-static int __maybe_unused ravb_suspend(struct device *dev)
+static int ravb_suspend(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -3000,7 +3000,7 @@ static int __maybe_unused ravb_suspend(struct device *dev)
 	return ret;
 }
 
-static int __maybe_unused ravb_resume(struct device *dev)
+static int ravb_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -3063,7 +3063,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	return ret;
 }
 
-static int __maybe_unused ravb_runtime_nop(struct device *dev)
+static int ravb_runtime_nop(struct device *dev)
 {
 	/* Runtime PM callback shared between ->runtime_suspend()
 	 * and ->runtime_resume(). Simply returns success.
@@ -3076,8 +3076,8 @@ static int __maybe_unused ravb_runtime_nop(struct device *dev)
 }
 
 static const struct dev_pm_ops ravb_dev_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(ravb_suspend, ravb_resume)
-	SET_RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
+	SYSTEM_SLEEP_PM_OPS(ravb_suspend, ravb_resume)
+	RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
 };
 
 static struct platform_driver ravb_driver = {
@@ -3085,7 +3085,7 @@ static struct platform_driver ravb_driver = {
 	.remove_new	= ravb_remove,
 	.driver = {
 		.name	= "ravb",
-		.pm	= &ravb_dev_pm_ops,
+		.pm	= pm_ptr(&ravb_dev_pm_ops),
 		.of_match_table = ravb_match_table,
 	},
 };
-- 
2.39.2

[PATCH net-next v2 05/21] net: ravb: Use tabs instead of spaces

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use tabs instead of spaces in the ravb_set_rate_gbeth() function.
This aligns with the coding style requirements.

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

Changes in v2:
- collected tags

 drivers/net/ethernet/renesas/ravb_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 82085bb9b7a3..5a57383762e7 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -85,13 +85,13 @@ static void ravb_set_rate_gbeth(struct net_device *ndev)
 	struct ravb_private *priv = netdev_priv(ndev);
 
 	switch (priv->speed) {
-	case 10:                /* 10BASE */
+	case 10:		/* 10BASE */
 		ravb_write(ndev, GBETH_GECMR_SPEED_10, GECMR);
 		break;
-	case 100:               /* 100BASE */
+	case 100:		/* 100BASE */
 		ravb_write(ndev, GBETH_GECMR_SPEED_100, GECMR);
 		break;
-	case 1000:              /* 1000BASE */
+	case 1000:		/* 1000BASE */
 		ravb_write(ndev, GBETH_GECMR_SPEED_1000, GECMR);
 		break;
 	}
-- 
2.39.2

[PATCH net-next v2 06/21] net: ravb: Assert/de-assert reset on suspend/resume

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

RZ/G3S can go to deep sleep states where power to most of the SoC parts is
off. When resuming from such a state, the Ethernet controller needs to be
reinitialized. De-asserting the reset signal for it should also be done.
Thus, add reset assert/de-assert on suspend/resume functions.

On the resume function, the de-assert was not reverted in case of failures
to give the user a chance to restore the interface (e.g., bringing down/up
the interface) in case suspend/resume failed.

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

Changes in v2:
- fixed typos in patch description and subject
- on ravb_suspend() assert the reset signal in case interface is down;
  due to this the Sergey's Rb tag was left aside in this version

 drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5a57383762e7..9a618d8dbfcd 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2985,7 +2985,7 @@ static int ravb_suspend(struct device *dev)
 	int ret;
 
 	if (!netif_running(ndev))
-		return 0;
+		goto reset_assert;
 
 	netif_device_detach(ndev);
 
@@ -2997,7 +2997,11 @@ static int ravb_suspend(struct device *dev)
 	if (priv->info->ccc_gac)
 		ravb_ptp_stop(ndev);
 
-	return ret;
+	if (priv->wol_enabled)
+		return ret;
+
+reset_assert:
+	return reset_control_assert(priv->rstc);
 }
 
 static int ravb_resume(struct device *dev)
@@ -3005,7 +3009,11 @@ static int ravb_resume(struct device *dev)
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
-	int ret = 0;
+	int ret;
+
+	ret = reset_control_deassert(priv->rstc);
+	if (ret)
+		return ret;
 
 	/* If WoL is enabled set reset mode to rearm the WoL logic */
 	if (priv->wol_enabled) {
-- 
2.39.2

Re: [PATCH net-next v2 06/21] net: ravb: Assert/de-assert reset on suspend/resume

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> RZ/G3S can go to deep sleep states where power to most of the SoC parts is
> off. When resuming from such a state, the Ethernet controller needs to be
> reinitialized. De-asserting the reset signal for it should also be done.
> Thus, add reset assert/de-assert on suspend/resume functions.
> 
> On the resume function, the de-assert was not reverted in case of failures
> to give the user a chance to restore the interface (e.g., bringing down/up
> the interface) in case suspend/resume failed.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[PATCH net-next v2 07/21] net: ravb: Move reference clock enable/disable on runtime PM APIs

[Claudiu]

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.

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

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 | 88 +++++++++++++-----------
 1 file changed, 46 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9a618d8dbfcd..83691a0f0cc2 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,23 @@ static int ravb_probe(struct platform_device *pdev)
 		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
 	}
 
+	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 +2719,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 +2732,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 +2747,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,14 +2756,14 @@ 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;
 		}
@@ -2764,21 +2772,14 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		error = PTR_ERR(priv->clk);
-		goto out_release;
+		goto out_rpm_put;
 	}
 
-	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;
+			goto out_rpm_put;
 		}
 	}
 
@@ -2799,20 +2800,20 @@ 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);
 		/* Check completion status. */
 		error = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
 		if (error)
-			goto out_disable_refclk;
+			goto out_rpm_put;
 	}
 
 	if (info->internal_delay) {
@@ -2829,7 +2830,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;
@@ -2875,8 +2876,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:
@@ -2892,12 +2891,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);
@@ -2929,10 +2928,9 @@ static void ravb_remove(struct platform_device *pdev)
 	if (error)
 		netdev_err(ndev, "Failed to reset ndev\n");
 
-	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);
@@ -3071,21 +3069,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 = {
-- 
2.39.2

Re: [PATCH net-next v2 07/21] net: ravb: Move reference clock enable/disable on runtime PM APIs

[Sergey Shtylyov]

On 12/14/23 2:45 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
> 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.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[PATCH net-next v2 08/21] net: ravb: Move the IRQs get and request in the probe function

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Move the IRQs get and request in the driver's probe function. As some IP
variants switches to reset operation mode as a result of setting module
standby through clock enable/disable APIs, to implement runtime PM the
resource parsing and requests are moved in the probe function and IP
settings are moved in the open functions. This is a preparatory change to
add runtime PM support for all IP variants.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 274 +++++++++++------------
 1 file changed, 132 insertions(+), 142 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 83691a0f0cc2..d7f6e8ea8e79 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1731,7 +1731,7 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
 	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
 	if (!name)
 		return -ENOMEM;
-	error = request_irq(irq, handler, 0, name, ndev);
+	error = devm_request_irq(dev, irq, handler, 0, name, ndev);
 	if (error)
 		netdev_err(ndev, "cannot request IRQ %s\n", name);
 
@@ -1755,63 +1755,16 @@ static int ravb_open(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
-	struct platform_device *pdev = priv->pdev;
-	struct device *dev = &pdev->dev;
 	int error;
 
 	napi_enable(&priv->napi[RAVB_BE]);
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
 
-	if (!info->multi_irqs) {
-		error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
-				    ndev->name, ndev);
-		if (error) {
-			netdev_err(ndev, "cannot request IRQ\n");
-			goto out_napi_off;
-		}
-	} else {
-		error = ravb_hook_irq(ndev->irq, ravb_multi_interrupt, ndev,
-				      dev, "ch22:multi");
-		if (error)
-			goto out_napi_off;
-		error = ravb_hook_irq(priv->emac_irq, ravb_emac_interrupt, ndev,
-				      dev, "ch24:emac");
-		if (error)
-			goto out_free_irq;
-		error = ravb_hook_irq(priv->rx_irqs[RAVB_BE], ravb_be_interrupt,
-				      ndev, dev, "ch0:rx_be");
-		if (error)
-			goto out_free_irq_emac;
-		error = ravb_hook_irq(priv->tx_irqs[RAVB_BE], ravb_be_interrupt,
-				      ndev, dev, "ch18:tx_be");
-		if (error)
-			goto out_free_irq_be_rx;
-		error = ravb_hook_irq(priv->rx_irqs[RAVB_NC], ravb_nc_interrupt,
-				      ndev, dev, "ch1:rx_nc");
-		if (error)
-			goto out_free_irq_be_tx;
-		error = ravb_hook_irq(priv->tx_irqs[RAVB_NC], ravb_nc_interrupt,
-				      ndev, dev, "ch19:tx_nc");
-		if (error)
-			goto out_free_irq_nc_rx;
-
-		if (info->err_mgmt_irqs) {
-			error = ravb_hook_irq(priv->erra_irq, ravb_multi_interrupt,
-					      ndev, dev, "err_a");
-			if (error)
-				goto out_free_irq_nc_tx;
-			error = ravb_hook_irq(priv->mgmta_irq, ravb_multi_interrupt,
-					      ndev, dev, "mgmt_a");
-			if (error)
-				goto out_free_irq_erra;
-		}
-	}
-
 	/* Device init */
 	error = ravb_dmac_init(ndev);
 	if (error)
-		goto out_free_irq_mgmta;
+		goto out_napi_off;
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
@@ -1832,26 +1785,6 @@ static int ravb_open(struct net_device *ndev)
 	if (info->gptp)
 		ravb_ptp_stop(ndev);
 	ravb_stop_dma(ndev);
-out_free_irq_mgmta:
-	if (!info->multi_irqs)
-		goto out_free_irq;
-	if (info->err_mgmt_irqs)
-		free_irq(priv->mgmta_irq, ndev);
-out_free_irq_erra:
-	if (info->err_mgmt_irqs)
-		free_irq(priv->erra_irq, ndev);
-out_free_irq_nc_tx:
-	free_irq(priv->tx_irqs[RAVB_NC], ndev);
-out_free_irq_nc_rx:
-	free_irq(priv->rx_irqs[RAVB_NC], ndev);
-out_free_irq_be_tx:
-	free_irq(priv->tx_irqs[RAVB_BE], ndev);
-out_free_irq_be_rx:
-	free_irq(priv->rx_irqs[RAVB_BE], ndev);
-out_free_irq_emac:
-	free_irq(priv->emac_irq, ndev);
-out_free_irq:
-	free_irq(ndev->irq, ndev);
 out_napi_off:
 	if (info->nc_queues)
 		napi_disable(&priv->napi[RAVB_NC]);
@@ -2177,19 +2110,6 @@ static int ravb_close(struct net_device *ndev)
 
 	cancel_work_sync(&priv->work);
 
-	if (info->multi_irqs) {
-		free_irq(priv->tx_irqs[RAVB_NC], ndev);
-		free_irq(priv->rx_irqs[RAVB_NC], ndev);
-		free_irq(priv->tx_irqs[RAVB_BE], ndev);
-		free_irq(priv->rx_irqs[RAVB_BE], ndev);
-		free_irq(priv->emac_irq, ndev);
-		if (info->err_mgmt_irqs) {
-			free_irq(priv->erra_irq, ndev);
-			free_irq(priv->mgmta_irq, ndev);
-		}
-	}
-	free_irq(ndev->irq, ndev);
-
 	if (info->nc_queues)
 		napi_disable(&priv->napi[RAVB_NC]);
 	napi_disable(&priv->napi[RAVB_BE]);
@@ -2616,6 +2536,127 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
 	}
 }
 
+static int ravb_get_irqs(struct ravb_private *priv)
+{
+	const char *err_a_irq_name = NULL, *mgmt_a_irq_name = NULL;
+	const struct ravb_hw_info *info = priv->info;
+	struct platform_device *pdev = priv->pdev;
+	struct net_device *ndev = priv->ndev;
+	const char *irq_name, *emac_irq_name;
+	int i, irq;
+
+	if (!info->multi_irqs) {
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0)
+			return irq;
+
+		ndev->irq = irq;
+		return 0;
+	}
+
+	if (info->err_mgmt_irqs) {
+		irq_name = "dia";
+		emac_irq_name = "line3";
+		err_a_irq_name = "err_a";
+		mgmt_a_irq_name = "mgmt_a";
+	} else {
+		irq_name = "ch22";
+		emac_irq_name = "ch24";
+	}
+
+	irq = platform_get_irq_byname(pdev, irq_name);
+	if (irq < 0)
+		return irq;
+	ndev->irq = irq;
+
+	irq = platform_get_irq_byname(pdev, emac_irq_name);
+	if (irq < 0)
+		return irq;
+	priv->emac_irq = irq;
+
+	if (err_a_irq_name) {
+		irq = platform_get_irq_byname(pdev, "err_a");
+		if (irq < 0)
+			return irq;
+		priv->erra_irq = irq;
+	}
+
+	if (mgmt_a_irq_name) {
+		irq = platform_get_irq_byname(pdev, "mgmt_a");
+		if (irq < 0)
+			return irq;
+		priv->mgmta_irq = irq;
+	}
+
+	for (i = 0; i < NUM_RX_QUEUE; i++) {
+		irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
+		if (irq < 0)
+			return irq;
+		priv->rx_irqs[i] = irq;
+	}
+	for (i = 0; i < NUM_TX_QUEUE; i++) {
+		irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
+		if (irq < 0)
+			return irq;
+		priv->tx_irqs[i] = irq;
+	}
+
+	return 0;
+}
+
+static int ravb_request_irqs(struct ravb_private *priv)
+{
+	const struct ravb_hw_info *info = priv->info;
+	struct net_device *ndev = priv->ndev;
+	struct device *dev = &priv->pdev->dev;
+	int error;
+
+	if (!info->multi_irqs) {
+		error = devm_request_irq(dev, ndev->irq, ravb_interrupt, IRQF_SHARED,
+					 ndev->name, ndev);
+		if (error)
+			netdev_err(ndev, "cannot request IRQ\n");
+
+		return error;
+	}
+
+	error = ravb_hook_irq(ndev->irq, ravb_multi_interrupt, ndev,
+			      dev, "ch22:multi");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->emac_irq, ravb_emac_interrupt, ndev,
+			      dev, "ch24:emac");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->rx_irqs[RAVB_BE], ravb_be_interrupt,
+			      ndev, dev, "ch0:rx_be");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->tx_irqs[RAVB_BE], ravb_be_interrupt,
+			      ndev, dev, "ch18:tx_be");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->rx_irqs[RAVB_NC], ravb_nc_interrupt,
+			      ndev, dev, "ch1:rx_nc");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->tx_irqs[RAVB_NC], ravb_nc_interrupt,
+			      ndev, dev, "ch19:tx_nc");
+	if (error)
+		return error;
+
+	if (!info->err_mgmt_irqs)
+		return 0;
+
+	error = ravb_hook_irq(priv->erra_irq, ravb_multi_interrupt,
+			      ndev, dev, "err_a");
+	if (error)
+		return error;
+
+	return ravb_hook_irq(priv->mgmta_irq, ravb_multi_interrupt,
+			     ndev, dev, "mgmt_a");
+}
+
 static void ravb_set_delay_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -2635,9 +2676,8 @@ static int ravb_probe(struct platform_device *pdev)
 	struct reset_control *rstc;
 	struct ravb_private *priv;
 	struct net_device *ndev;
-	int error, irq, q;
 	struct resource *res;
-	int i;
+	int error, q;
 
 	if (!np) {
 		dev_err(&pdev->dev,
@@ -2664,20 +2704,6 @@ static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_free_netdev;
 
-	if (info->multi_irqs) {
-		if (info->err_mgmt_irqs)
-			irq = platform_get_irq_byname(pdev, "dia");
-		else
-			irq = platform_get_irq_byname(pdev, "ch22");
-	} else {
-		irq = platform_get_irq(pdev, 0);
-	}
-	if (irq < 0) {
-		error = irq;
-		goto out_reset_assert;
-	}
-	ndev->irq = irq;
-
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 
 	priv = netdev_priv(ndev);
@@ -2692,6 +2718,14 @@ static int ravb_probe(struct platform_device *pdev)
 		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
 	}
 
+	error = ravb_get_irqs(priv);
+	if (error)
+		goto out_reset_assert;
+
+	error = ravb_request_irqs(priv);
+	if (error)
+		goto out_reset_assert;
+
 	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
 	if (IS_ERR(priv->refclk)) {
 		error = PTR_ERR(priv->refclk);
@@ -2725,50 +2759,6 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	if (info->multi_irqs) {
-		if (info->err_mgmt_irqs)
-			irq = platform_get_irq_byname(pdev, "line3");
-		else
-			irq = platform_get_irq_byname(pdev, "ch24");
-		if (irq < 0) {
-			error = irq;
-			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_rpm_put;
-			}
-			priv->rx_irqs[i] = irq;
-		}
-		for (i = 0; i < NUM_TX_QUEUE; i++) {
-			irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->tx_irqs[i] = irq;
-		}
-
-		if (info->err_mgmt_irqs) {
-			irq = platform_get_irq_byname(pdev, "err_a");
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->erra_irq = irq;
-
-			irq = platform_get_irq_byname(pdev, "mgmt_a");
-			if (irq < 0) {
-				error = irq;
-				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);
-- 
2.39.2

Re: [PATCH net-next v2 08/21] net: ravb: Move the IRQs get and request in the probe function

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Move the IRQs get and request in the driver's probe function. As some IP
> variants switches to reset operation mode as a result of setting module

   s/switches/switch/.
   Also, the manuals call this "operating mode", not to mix with one of
the modes -- "operation mode".

> standby through clock enable/disable APIs, to implement runtime PM the
> resource parsing and requests are moved in the probe function and IP

   Requesting.
   Could you explain in more detail why you need to do this?

> settings are moved in the open functions. This is a preparatory change to

   I don't see you moving anything into ravb_open() here...

> add runtime PM support for all IP variants.
> 
> 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 83691a0f0cc2..d7f6e8ea8e79 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1731,7 +1731,7 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
>  	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);

   Ugh, I didn't realize we had the managed device API call in a function
called from ravb_open()... :-/

[...]
> @@ -2616,6 +2536,127 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
>  	}
>  }
>  
> +static int ravb_get_irqs(struct ravb_private *priv)
> +{
> +	const char *err_a_irq_name = NULL, *mgmt_a_irq_name = NULL;

   You don't seem to use these as the pointers. Could be bool instead?
But even that doesn't seem necessary..

> +	const struct ravb_hw_info *info = priv->info;
> +	struct platform_device *pdev = priv->pdev;
> +	struct net_device *ndev = priv->ndev;
> +	const char *irq_name, *emac_irq_name;
> +	int i, irq;
> +
> +	if (!info->multi_irqs) {
> +		irq = platform_get_irq(pdev, 0);
> +		if (irq < 0)
> +			return irq;
> +
> +		ndev->irq = irq;
> +		return 0;
> +	}
> +
> +	if (info->err_mgmt_irqs) {
> +		irq_name = "dia";
> +		emac_irq_name = "line3";
> +		err_a_irq_name = "err_a";
> +		mgmt_a_irq_name = "mgmt_a";
> +	} else {
> +		irq_name = "ch22";
> +		emac_irq_name = "ch24";
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, irq_name);
> +	if (irq < 0)
> +		return irq;
> +	ndev->irq = irq;
> +
> +	irq = platform_get_irq_byname(pdev, emac_irq_name);
> +	if (irq < 0)
> +		return irq;
> +	priv->emac_irq = irq;
> +
> +	if (err_a_irq_name) {

   Why not just ctest info->err_mgmt_irqs here, as it was before
this patch?

> +		irq = platform_get_irq_byname(pdev, "err_a");
> +		if (irq < 0)
> +			return irq;
> +		priv->erra_irq = irq;
> +	}
> +
> +	if (mgmt_a_irq_name) {
> +		irq = platform_get_irq_byname(pdev, "mgmt_a");
> +		if (irq < 0)
> +			return irq;
> +		priv->mgmta_irq = irq;
> +	}
> +
> +	for (i = 0; i < NUM_RX_QUEUE; i++) {
> +		irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
> +		if (irq < 0)
> +			return irq;
> +		priv->rx_irqs[i] = irq;
> +	}
> +	for (i = 0; i < NUM_TX_QUEUE; i++) {
> +		irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
> +		if (irq < 0)
> +			return irq;
> +		priv->tx_irqs[i] = irq;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ravb_request_irqs(struct ravb_private *priv)

   I'm not sure separating getting and requesting IRQs is a good idea.
As you're switching to using the managed device API anyway, you could
save on some IRQ-related fields in the *struct* ravb_private, I think...

[...]

MBR, Sergey

Re: [PATCH net-next v2 08/21] net: ravb: Move the IRQs get and request in the probe function

[claudiu beznea]

On 16.12.2023 17:53, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Move the IRQs get and request in the driver's probe function. As some IP
>> variants switches to reset operation mode as a result of setting module
> 
>    s/switches/switch/.
>    Also, the manuals call this "operating mode", not to mix with one of
> the modes -- "operation mode".

ok

> 
>> standby through clock enable/disable APIs, to implement runtime PM the
>> resource parsing and requests are moved in the probe function and IP
> 
>    Requesting.
>    Could you explain in more detail why you need to do this?

Ok, I'll update it in the next version.

> 
>> settings are moved in the open functions. This is a preparatory change to
> 
>    I don't see you moving anything into ravb_open() here...

Indeed, this is the general explanation. I'll adapt it to explain it what
has been done in the commit (as it should have been).

> 
>> add runtime PM support for all IP variants.
>>
>> 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 83691a0f0cc2..d7f6e8ea8e79 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1731,7 +1731,7 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
>>  	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
> 
>    Ugh, I didn't realize we had the managed device API call in a function
> called from ravb_open()... :-/
> 
> [...]
>> @@ -2616,6 +2536,127 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
>>  	}
>>  }
>>  
>> +static int ravb_get_irqs(struct ravb_private *priv)
>> +{
>> +	const char *err_a_irq_name = NULL, *mgmt_a_irq_name = NULL;
> 
>    You don't seem to use these as the pointers. Could be bool instead?
> But even that doesn't seem necessary..

Indeed, I've messed it a bit. I'll update it in the next version.

> 
>> +	const struct ravb_hw_info *info = priv->info;
>> +	struct platform_device *pdev = priv->pdev;
>> +	struct net_device *ndev = priv->ndev;
>> +	const char *irq_name, *emac_irq_name;
>> +	int i, irq;
>> +
>> +	if (!info->multi_irqs) {
>> +		irq = platform_get_irq(pdev, 0);
>> +		if (irq < 0)
>> +			return irq;
>> +
>> +		ndev->irq = irq;
>> +		return 0;
>> +	}
>> +
>> +	if (info->err_mgmt_irqs) {
>> +		irq_name = "dia";
>> +		emac_irq_name = "line3";
>> +		err_a_irq_name = "err_a";
>> +		mgmt_a_irq_name = "mgmt_a";
>> +	} else {
>> +		irq_name = "ch22";
>> +		emac_irq_name = "ch24";
>> +	}
>> +
>> +	irq = platform_get_irq_byname(pdev, irq_name);
>> +	if (irq < 0)
>> +		return irq;
>> +	ndev->irq = irq;
>> +
>> +	irq = platform_get_irq_byname(pdev, emac_irq_name);
>> +	if (irq < 0)
>> +		return irq;
>> +	priv->emac_irq = irq;
>> +
>> +	if (err_a_irq_name) {
> 
>    Why not just ctest info->err_mgmt_irqs here, as it was before
> this patch?

I can't tell ATM what I've wanted to achieve here. Indeed, just checking
info->err_mgmt_irqs should be better.

> 
>> +		irq = platform_get_irq_byname(pdev, "err_a");
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->erra_irq = irq;
>> +	}
>> +
>> +	if (mgmt_a_irq_name) {
>> +		irq = platform_get_irq_byname(pdev, "mgmt_a");
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->mgmta_irq = irq;
>> +	}
>> +
>> +	for (i = 0; i < NUM_RX_QUEUE; i++) {
>> +		irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->rx_irqs[i] = irq;
>> +	}
>> +	for (i = 0; i < NUM_TX_QUEUE; i++) {
>> +		irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->tx_irqs[i] = irq;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ravb_request_irqs(struct ravb_private *priv)
> 
>    I'm not sure separating getting and requesting IRQs is a good idea.
> As you're switching to using the managed device API anyway, you could
> save on some IRQ-related fields in the *struct* ravb_private, I think...

I'll have a look. By keeping them separated I tried to have the code doing
the similar things grouped together, tried to keep code similar to what was
previously and tried to avoid huge functions (having parse and request in a
single function will lead, AFAICT, at a function with more lines of code
(difficult to browse in my opinion)).

Thank you for your review,
Claudiu Beznea

> 
> [...]
> 
> MBR, Sergey

[PATCH net-next v2 09/21] net: ravb: Split GTI computation and set operations

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

ravb_set_gti() was computing the value of GTI based on the reference clock
rate and then applied it to register. This was done on the driver's probe
function. In order to implement runtime PM for all IP variants (as some IP
variants switches to reset operation mode (and thus the register's content
is lost) when module standby is configured through clock APIs) the GTI was
split in 2 parts: one computing the value of the GTI register (done in the
driver's probe function) and one applying the computed value to register
(done in the driver's ndo_open API).

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb.h      |   2 +
 drivers/net/ethernet/renesas/ravb_main.c | 110 ++++++++++++-----------
 2 files changed, 58 insertions(+), 54 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index e0f8276cffed..76202395b68d 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1106,6 +1106,8 @@ struct ravb_private {
 
 	const struct ravb_hw_info *info;
 	struct reset_control *rstc;
+
+	uint64_t gti_tiv;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index d7f6e8ea8e79..5e01e03e1b43 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1750,6 +1750,51 @@ static int ravb_set_reset_mode(struct net_device *ndev)
 	return error;
 }
 
+static int ravb_set_gti(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_hw_info *info = priv->info;
+
+	if (!(info->gptp || info->ccc_gac))
+		return 0;
+
+	ravb_write(ndev, priv->gti_tiv, GTI);
+
+	/* Request GTI loading */
+	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+
+	/* Check completion status. */
+	return ravb_wait(ndev, GCCR, GCCR_LTI, 0);
+}
+
+static int ravb_compute_gti(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_hw_info *info = priv->info;
+	struct device *dev = ndev->dev.parent;
+	unsigned long rate;
+
+	if (!(info->gptp || info->ccc_gac))
+		return 0;
+
+	if (info->gptp_ref_clk)
+		rate = clk_get_rate(priv->gptp_clk);
+	else
+		rate = clk_get_rate(priv->clk);
+	if (!rate)
+		return -EINVAL;
+
+	priv->gti_tiv = div64_ul(1000000000ULL << 20, rate);
+
+	if (priv->gti_tiv < GTI_TIV_MIN || priv->gti_tiv > GTI_TIV_MAX) {
+		dev_err(dev, "gti.tiv increment 0x%llx is outside the range 0x%x - 0x%x\n",
+			priv->gti_tiv, GTI_TIV_MIN, GTI_TIV_MAX);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /* Network device open function for Ethernet AVB */
 static int ravb_open(struct net_device *ndev)
 {
@@ -1767,6 +1812,10 @@ static int ravb_open(struct net_device *ndev)
 		goto out_napi_off;
 	ravb_emac_init(ndev);
 
+	error = ravb_set_gti(ndev);
+	if (error)
+		goto out_dma_stop;
+
 	/* Initialise PTP Clock driver */
 	if (info->gptp)
 		ravb_ptp_init(ndev, priv->pdev);
@@ -1784,6 +1833,7 @@ static int ravb_open(struct net_device *ndev)
 	/* Stop PTP Clock driver */
 	if (info->gptp)
 		ravb_ptp_stop(ndev);
+out_dma_stop:
 	ravb_stop_dma(ndev);
 out_napi_off:
 	if (info->nc_queues)
@@ -2449,34 +2499,6 @@ static const struct of_device_id ravb_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
 
-static int ravb_set_gti(struct net_device *ndev)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
-	struct device *dev = ndev->dev.parent;
-	unsigned long rate;
-	uint64_t inc;
-
-	if (info->gptp_ref_clk)
-		rate = clk_get_rate(priv->gptp_clk);
-	else
-		rate = clk_get_rate(priv->clk);
-	if (!rate)
-		return -EINVAL;
-
-	inc = div64_ul(1000000000ULL << 20, rate);
-
-	if (inc < GTI_TIV_MIN || inc > GTI_TIV_MAX) {
-		dev_err(dev, "gti.tiv increment 0x%llx is outside the range 0x%x - 0x%x\n",
-			inc, GTI_TIV_MIN, GTI_TIV_MAX);
-		return -EINVAL;
-	}
-
-	ravb_write(ndev, inc, GTI);
-
-	return 0;
-}
-
 static int ravb_set_config_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -2792,19 +2814,9 @@ static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_rpm_put;
 
-	if (info->gptp || info->ccc_gac) {
-		/* Set GTI value */
-		error = ravb_set_gti(ndev);
-		if (error)
-			goto out_rpm_put;
-
-		/* Request GTI loading */
-		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
-		/* Check completion status. */
-		error = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
-		if (error)
-			goto out_rpm_put;
-	}
+	error = ravb_compute_gti(ndev);
+	if (error)
+		goto out_rpm_put;
 
 	if (info->internal_delay) {
 		ravb_parse_delay_mode(np, ndev);
@@ -3020,19 +3032,9 @@ static int ravb_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (info->gptp || info->ccc_gac) {
-		/* Set GTI value */
-		ret = ravb_set_gti(ndev);
-		if (ret)
-			return ret;
-
-		/* Request GTI loading */
-		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
-		/* Check completion status. */
-		ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
-		if (ret)
-			return ret;
-	}
+	ret = ravb_set_gti(ndev);
+	if (ret)
+		return ret;
 
 	if (info->internal_delay)
 		ravb_set_delay_mode(ndev);
-- 
2.39.2

Re: [PATCH net-next v2 09/21] net: ravb: Split GTI computation and set operations

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> ravb_set_gti() was computing the value of GTI based on the reference clock
> rate and then applied it to register. This was done on the driver's probe
> function. In order to implement runtime PM for all IP variants (as some IP
> variants switches to reset operation mode (and thus the register's content

   Again, operating mode...

> is lost) when module standby is configured through clock APIs) the GTI was

   The GTI what? Setup?

> split in 2 parts: one computing the value of the GTI register (done in the
> driver's probe function) and one applying the computed value to register
> (done in the driver's ndo_open API).
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index e0f8276cffed..76202395b68d 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1106,6 +1106,8 @@ struct ravb_private {
>  
>  	const struct ravb_hw_info *info;
>  	struct reset_control *rstc;
> +
> +	uint64_t gti_tiv;

   Please use the kernel type, u64; uint64_t is for userland, IIRC.

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index d7f6e8ea8e79..5e01e03e1b43 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1750,6 +1750,51 @@ static int ravb_set_reset_mode(struct net_device *ndev)
>  	return error;
>  }
>  
> +static int ravb_set_gti(struct net_device *ndev)
> +{
[...]
> +	/* Request GTI loading */
> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +
> +	/* Check completion status. */
> +	return ravb_wait(ndev, GCCR, GCCR_LTI, 0);

   Is this really necessary?

[...]
> @@ -1767,6 +1812,10 @@ static int ravb_open(struct net_device *ndev)
>  		goto out_napi_off;
>  	ravb_emac_init(ndev);
>  
> +	error = ravb_set_gti(ndev);
> +	if (error)
> +		goto out_dma_stop;
> +

   Hm... belongs in ravb_dmac_init() now, as it seems... 

[...]

MBR, Sergey

Re: [PATCH net-next v2 09/21] net: ravb: Split GTI computation and set operations

[claudiu beznea]

On 16.12.2023 18:38, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> ravb_set_gti() was computing the value of GTI based on the reference clock
>> rate and then applied it to register. This was done on the driver's probe
>> function. In order to implement runtime PM for all IP variants (as some IP
>> variants switches to reset operation mode (and thus the register's content
> 
>    Again, operating mode...
> 
>> is lost) when module standby is configured through clock APIs) the GTI was
> 
>    The GTI what? Setup?
> 
>> split in 2 parts: one computing the value of the GTI register (done in the
>> driver's probe function) and one applying the computed value to register
>> (done in the driver's ndo_open API).
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>> index e0f8276cffed..76202395b68d 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> @@ -1106,6 +1106,8 @@ struct ravb_private {
>>  
>>  	const struct ravb_hw_info *info;
>>  	struct reset_control *rstc;
>> +
>> +	uint64_t gti_tiv;
> 
>    Please use the kernel type, u64; uint64_t is for userland, IIRC.

I just kept the initial type here. Anyway, uint64_t should translate to u64
AFAICT.

Looking at it again the field here is enough to be 32 bit as the register
field is no longer than that. It is needed on 64 bits when checking the
ranges in compute function.

> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index d7f6e8ea8e79..5e01e03e1b43 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1750,6 +1750,51 @@ static int ravb_set_reset_mode(struct net_device *ndev)
>>  	return error;
>>  }
>>  
>> +static int ravb_set_gti(struct net_device *ndev)
>> +{
> [...]
>> +	/* Request GTI loading */
>> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>> +
>> +	/* Check completion status. */
>> +	return ravb_wait(ndev, GCCR, GCCR_LTI, 0);
> 
>    Is this really necessary?

I've just updated it to respect the manual specifications. Please let me
know if you want me to drop it. For this series it should be harmless
keeping it as it was previously (I will double check it).

> 
> [...]
>> @@ -1767,6 +1812,10 @@ static int ravb_open(struct net_device *ndev)
>>  		goto out_napi_off;
>>  	ravb_emac_init(ndev);
>>  
>> +	error = ravb_set_gti(ndev);
>> +	if (error)
>> +		goto out_dma_stop;
>> +
> 
>    Hm... belongs in ravb_dmac_init() now, as it seems... 

Isn't it PTP specific?

> 
> [...]
> 
> MBR, Sergey

Re: [PATCH net-next v2 09/21] net: ravb: Split GTI computation and set operations

[Sergey Shtylyov]

On 12/17/23 3:40 PM, claudiu beznea wrote:

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> ravb_set_gti() was computing the value of GTI based on the reference clock
>>> rate and then applied it to register. This was done on the driver's probe
>>> function. In order to implement runtime PM for all IP variants (as some IP
>>> variants switches to reset operation mode (and thus the register's content
>>
>>    Again, operating mode...
>>
>>> is lost) when module standby is configured through clock APIs) the GTI was
>>
>>    The GTI what? Setup?
>>
>>> split in 2 parts: one computing the value of the GTI register (done in the
>>> driver's probe function) and one applying the computed value to register
>>> (done in the driver's ndo_open API).
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>> index e0f8276cffed..76202395b68d 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -1106,6 +1106,8 @@ struct ravb_private {
>>>  
>>>  	const struct ravb_hw_info *info;
>>>  	struct reset_control *rstc;
>>> +
>>> +	uint64_t gti_tiv;
>>
>>    Please use the kernel type, u64; uint64_t is for userland, IIRC.
> 
> I just kept the initial type here.

   Oops, that type slipped in while I wasn't yet a reviewer. :-/

> Anyway, uint64_t should translate to u64 AFAICT.

   Yes.

> Looking at it again the field here is enough to be 32 bit as the register
> field is no longer than that. It is needed on 64 bits when checking the
> ranges in compute function.

   Indeed. The actual GTI.TIV field is even 28-bit wide only...

[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index d7f6e8ea8e79..5e01e03e1b43 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1750,6 +1750,51 @@ static int ravb_set_reset_mode(struct net_device *ndev)
>>>  	return error;
>>>  }
>>>  
>>> +static int ravb_set_gti(struct net_device *ndev)
>>> +{
>> [...]
>>> +	/* Request GTI loading */
>>> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>>> +
>>> +	/* Check completion status. */
>>> +	return ravb_wait(ndev, GCCR, GCCR_LTI, 0);
>>
>>    Is this really necessary?
> 
> I've just updated it to respect the manual specifications. Please let me
> know if you want me to drop it. For this series it should be harmless
> keeping it as it was previously (I will double check it).

   Looks like you'll have to frop the fix patch #2, so this ravb_wait()
call shouldn't be placed here as well...

>> [...]
>>> @@ -1767,6 +1812,10 @@ static int ravb_open(struct net_device *ndev)
>>>  		goto out_napi_off;
>>>  	ravb_emac_init(ndev);
>>>  
>>> +	error = ravb_set_gti(ndev);
>>> +	if (error)
>>> +		goto out_dma_stop;
>>> +
>>
>>    Hm... belongs in ravb_dmac_init() now, as it seems... 
> 
> Isn't it PTP specific?

   I just had an impression it belonged to the AVB-DMAC register range
but perhaps I'm wrong...

[...]

MBR, Sergey

[PATCH net-next v2 10/21] net: ravb: Move delay mode set in the driver's ndo_open API

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Delay parse and set were done in the driver's probe API. As some IP
variants switch to reset mode (and thus registers' content is lost) when
setting clocks (due to module standby functionality) to be able to
implement runtime PM keep the delay parsing in the driver's probe function
and move the delay apply function to the driver's ndo_open API.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 37 ++++++++++++++----------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5e01e03e1b43..04eaa1967651 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1795,6 +1795,21 @@ static int ravb_compute_gti(struct net_device *ndev)
 	return 0;
 }
 
+static void ravb_set_delay_mode(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	u32 set = 0;
+
+	if (!priv->info->internal_delay)
+		return;
+
+	if (priv->rxcidm)
+		set |= APSR_RDM;
+	if (priv->txcidm)
+		set |= APSR_TDM;
+	ravb_modify(ndev, APSR, APSR_RDM | APSR_TDM, set);
+}
+
 /* Network device open function for Ethernet AVB */
 static int ravb_open(struct net_device *ndev)
 {
@@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
 
+	ravb_set_delay_mode(ndev);
+
 	/* Device init */
 	error = ravb_dmac_init(ndev);
 	if (error)
@@ -2530,6 +2547,9 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
 	bool explicit_delay = false;
 	u32 delay;
 
+	if (!priv->info->internal_delay)
+		return;
+
 	if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay)) {
 		/* Valid values are 0 and 1800, according to DT bindings */
 		priv->rxcidm = !!delay;
@@ -2679,18 +2699,6 @@ static int ravb_request_irqs(struct ravb_private *priv)
 			     ndev, dev, "mgmt_a");
 }
 
-static void ravb_set_delay_mode(struct net_device *ndev)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	u32 set = 0;
-
-	if (priv->rxcidm)
-		set |= APSR_RDM;
-	if (priv->txcidm)
-		set |= APSR_TDM;
-	ravb_modify(ndev, APSR, APSR_RDM | APSR_TDM, set);
-}
-
 static int ravb_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -2818,10 +2826,7 @@ static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_rpm_put;
 
-	if (info->internal_delay) {
-		ravb_parse_delay_mode(np, ndev);
-		ravb_set_delay_mode(ndev);
-	}
+	ravb_parse_delay_mode(np, ndev);
 
 	/* Allocate descriptor base address table */
 	priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
-- 
2.39.2

Re: [PATCH net-next v2 10/21] net: ravb: Move delay mode set in the driver's ndo_open API

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Delay parse and set were done in the driver's probe API. As some IP

   Parsing and setting?

> variants switch to reset mode (and thus registers' content is lost) when
> setting clocks (due to module standby functionality) to be able to
> implement runtime PM keep the delay parsing in the driver's probe function
> and move the delay apply function to the driver's ndo_open API.

   Applying?

> 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 5e01e03e1b43..04eaa1967651 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  
> +	ravb_set_delay_mode(ndev);
> +

   I suspect this belongs in ravb_dmac_init() now...

>  	/* Device init */
>  	error = ravb_dmac_init(ndev);
>  	if (error)
[...]

MRB, Sergey

Re: [PATCH net-next v2 10/21] net: ravb: Move delay mode set in the driver's ndo_open API

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Delay parse and set were done in the driver's probe API. As some IP

   Parsing and setting?

> variants switch to reset mode (and thus registers' content is lost) when

   Register.

> setting clocks (due to module standby functionality) to be able to
> implement runtime PM keep the delay parsing in the driver's probe function
> and move the delay apply function to the driver's ndo_open API.

   Applying?

> 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 5e01e03e1b43..04eaa1967651 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  
> +	ravb_set_delay_mode(ndev);
> +

   I suspect this belongs in ravb_dmac_init() now...

>  	/* Device init */
>  	error = ravb_dmac_init(ndev);
>  	if (error)
[...]

MRB, Sergey

Re: [PATCH net-next v2 10/21] net: ravb: Move delay mode set in the driver's ndo_open API

[claudiu beznea]

On 15.12.2023 21:58, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Delay parse and set were done in the driver's probe API. As some IP
> 
>    Parsing and setting?
> 
>> variants switch to reset mode (and thus registers' content is lost) when
> 
>    Register.
> 
>> setting clocks (due to module standby functionality) to be able to
>> implement runtime PM keep the delay parsing in the driver's probe function
>> and move the delay apply function to the driver's ndo_open API.
> 
>    Applying?
> 
>> 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 5e01e03e1b43..04eaa1967651 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
>>  	if (info->nc_queues)
>>  		napi_enable(&priv->napi[RAVB_NC]);
>>  
>> +	ravb_set_delay_mode(ndev);
>> +
> 
>    I suspect this belongs in ravb_dmac_init() now...

I'm confused... Why? To me this seems more like MAC-PHY interface related.

Though I'm not sure what ravb_dmac_init() purpose is.

> 
>>  	/* Device init */
>>  	error = ravb_dmac_init(ndev);
>>  	if (error)
> [...]
> 
> MRB, Sergey

Re: [PATCH net-next v2 10/21] net: ravb: Move delay mode set in the driver's ndo_open API

[Sergey Shtylyov]

On 12/17/23 3:49 PM, claudiu beznea wrote:

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Delay parse and set were done in the driver's probe API. As some IP
>>
>>    Parsing and setting?
>>
>>> variants switch to reset mode (and thus registers' content is lost) when
>>
>>    Register.
>>
>>> setting clocks (due to module standby functionality) to be able to
>>> implement runtime PM keep the delay parsing in the driver's probe function
>>> and move the delay apply function to the driver's ndo_open API.
>>
>>    Applying?
>>
>>> 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 5e01e03e1b43..04eaa1967651 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> [...]
>>> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
>>>  	if (info->nc_queues)
>>>  		napi_enable(&priv->napi[RAVB_NC]);
>>>  
>>> +	ravb_set_delay_mode(ndev);
>>> +
>>
>>    I suspect this belongs in ravb_dmac_init() now...
> 
> I'm confused... Why? To me this seems more like MAC-PHY interface related.

   APSR's full name is AVB-DMAC product specific register. :-)

> Though I'm not sure what ravb_dmac_init() purpose is.

   To configure and start the AVB-DMAC, apparently. :-)

[...]

MRB, Sergey

Re: [PATCH net-next v2 10/21] net: ravb: Move delay mode set in the driver's ndo_open API

[claudiu beznea]

On 19.12.2023 20:40, Sergey Shtylyov wrote:
> On 12/17/23 3:49 PM, claudiu beznea wrote:
> 
> [...]
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> Delay parse and set were done in the driver's probe API. As some IP
>>>
>>>    Parsing and setting?
>>>
>>>> variants switch to reset mode (and thus registers' content is lost) when
>>>
>>>    Register.
>>>
>>>> setting clocks (due to module standby functionality) to be able to
>>>> implement runtime PM keep the delay parsing in the driver's probe function
>>>> and move the delay apply function to the driver's ndo_open API.
>>>
>>>    Applying?
>>>
>>>> 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 5e01e03e1b43..04eaa1967651 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> [...]
>>>> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
>>>>  	if (info->nc_queues)
>>>>  		napi_enable(&priv->napi[RAVB_NC]);
>>>>  
>>>> +	ravb_set_delay_mode(ndev);
>>>> +
>>>
>>>    I suspect this belongs in ravb_dmac_init() now...
>>
>> I'm confused... Why? To me this seems more like MAC-PHY interface related.
> 
>    APSR's full name is AVB-DMAC product specific register. :-)

As ravb_dmac_init() is called in multiple places I don't think it worth
configuring delays more than once in ravb_open().

Moreover TX/RX delay is something specific to the MAC-PHY interface (and
could be influenced also by the wiring length b/w MAC and PHY).

Just because it is in the DMAC address space I don't think it worth having
it in ravb_dmac_init() (for the above mentioned reasons).

> 
>> Though I'm not sure what ravb_dmac_init() purpose is.
> 
>    To configure and start the AVB-DMAC, apparently. :-)
> 
> [...]
> 
> MRB, Sergey

[PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

DBAT setup was done in the driver's probe API. As some IP variants switch
to reset mode (and thus registers' content is lost) when setting clocks
(due to module standby functionality) to be able to implement runtime PM
move the DBAT configuration in the driver's ndo_open API.

This commit prepares the code for the addition of runtime PM.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 04eaa1967651..6b8ca08be35e 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev)
 		napi_enable(&priv->napi[RAVB_NC]);
 
 	ravb_set_delay_mode(ndev);
+	ravb_write(ndev, priv->desc_bat_dma, DBAT);
 
 	/* Device init */
 	error = ravb_dmac_init(ndev);
@@ -2841,7 +2842,6 @@ static int ravb_probe(struct platform_device *pdev)
 	}
 	for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
 		priv->desc_bat[q].die_dt = DT_EOS;
-	ravb_write(ndev, priv->desc_bat_dma, DBAT);
 
 	/* Initialise HW timestamp list */
 	INIT_LIST_HEAD(&priv->ts_skb_list);
-- 
2.39.2

Re: [PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> DBAT setup was done in the driver's probe API. As some IP variants switch
> to reset mode (and thus registers' content is lost) when setting clocks
> (due to module standby functionality) to be able to implement runtime PM
> move the DBAT configuration in the driver's ndo_open API.
> 
> This commit prepares the code for the addition of runtime PM.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 04eaa1967651..6b8ca08be35e 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  
>  	ravb_set_delay_mode(ndev);
> +	ravb_write(ndev, priv->desc_bat_dma, DBAT);
>  
>  	/* Device init */
>  	error = ravb_dmac_init(ndev);
> @@ -2841,7 +2842,6 @@ static int ravb_probe(struct platform_device *pdev)
>  	}
>  	for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
>  		priv->desc_bat[q].die_dt = DT_EOS;
> -	ravb_write(ndev, priv->desc_bat_dma, DBAT);
>  
>  	/* Initialise HW timestamp list */
>  	INIT_LIST_HEAD(&priv->ts_skb_list);
> 

  How about also removing the DBAT write from ravb_resume()?

MBR, Sergey

Re: [PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API

[Sergey Shtylyov]

On 12/15/23 12:03 AM, Sergey Shtylyov wrote:
[...]

>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> DBAT setup was done in the driver's probe API. As some IP variants switch
>> to reset mode (and thus registers' content is lost) when setting clocks
>> (due to module standby functionality) to be able to implement runtime PM
>> move the DBAT configuration in the driver's ndo_open API.
>>
>> This commit prepares the code for the addition of runtime PM.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 04eaa1967651..6b8ca08be35e 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev)
>>  		napi_enable(&priv->napi[RAVB_NC]);
>>  
>>  	ravb_set_delay_mode(ndev);
>> +	ravb_write(ndev, priv->desc_bat_dma, DBAT);

   Looking at it again, I suspect this belong in ravb_dmac_init()...

>>  
>>  	/* Device init */
>>  	error = ravb_dmac_init(ndev);
[...]

MBR, Sergey

Re: [PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API

[claudiu beznea]

On 15.12.2023 22:01, Sergey Shtylyov wrote:
> On 12/15/23 12:03 AM, Sergey Shtylyov wrote:
> [...]
> 
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> DBAT setup was done in the driver's probe API. As some IP variants switch
>>> to reset mode (and thus registers' content is lost) when setting clocks
>>> (due to module standby functionality) to be able to implement runtime PM
>>> move the DBAT configuration in the driver's ndo_open API.
>>>
>>> This commit prepares the code for the addition of runtime PM.
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>
>> [...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 04eaa1967651..6b8ca08be35e 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev)
>>>  		napi_enable(&priv->napi[RAVB_NC]);
>>>  
>>>  	ravb_set_delay_mode(ndev);
>>> +	ravb_write(ndev, priv->desc_bat_dma, DBAT);
> 
>    Looking at it again, I suspect this belong in ravb_dmac_init()...

ravb_dmac_init() is called from multiple places in this driver, e.g.,
ravb_set_ringparam(), ravb_tx_timeout_work(). I'm afraid we may broke the
behavior of these if DBAT setup is moved in ravb_dmac_init(). This is also
valid for setting delay (see patch 10/12).

> 
>>>  
>>>  	/* Device init */
>>>  	error = ravb_dmac_init(ndev);
> [...]
> 
> MBR, Sergey

Re: [PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API

[Sergey Shtylyov]

On 12/17/23 3:54 PM, claudiu beznea wrote:

[...]

>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> DBAT setup was done in the driver's probe API. As some IP variants switch
>>>> to reset mode (and thus registers' content is lost) when setting clocks
>>>> (due to module standby functionality) to be able to implement runtime PM
>>>> move the DBAT configuration in the driver's ndo_open API.
>>>>
>>>> This commit prepares the code for the addition of runtime PM.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>
>>> [...]
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index 04eaa1967651..6b8ca08be35e 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev)
>>>>  		napi_enable(&priv->napi[RAVB_NC]);
>>>>  
>>>>  	ravb_set_delay_mode(ndev);
>>>> +	ravb_write(ndev, priv->desc_bat_dma, DBAT);
>>
>>    Looking at it again, I suspect this belong in ravb_dmac_init()...
> 
> ravb_dmac_init() is called from multiple places in this driver, e.g.,

   It's purpose is to configure AVB-DMAC and DBAT is the AVB-DMAC register,
right?

> ravb_set_ringparam(), ravb_tx_timeout_work().

   I know. Its value is only calculated once, in ravb_probe(), right?

> I'm afraid we may broke the behavior of these if DBAT setup is moved

   Do not be afraid! :-)

> in ravb_dmac_init(). This is also
> valid for setting delay (see patch 10/12).

   I don't think there will be a problem either... but maybe we
should call it in ravb_emac_init() indeed.

[...]

MBR, Sergey

Re: [PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API

[claudiu beznea]

On 19.12.2023 20:54, Sergey Shtylyov wrote:
> On 12/17/23 3:54 PM, claudiu beznea wrote:
> 
> [...]
> 
>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>
>>>>> DBAT setup was done in the driver's probe API. As some IP variants switch
>>>>> to reset mode (and thus registers' content is lost) when setting clocks
>>>>> (due to module standby functionality) to be able to implement runtime PM
>>>>> move the DBAT configuration in the driver's ndo_open API.
>>>>>
>>>>> This commit prepares the code for the addition of runtime PM.
>>>>>
>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>>
>>>> [...]
>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> index 04eaa1967651..6b8ca08be35e 100644
>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev)
>>>>>  		napi_enable(&priv->napi[RAVB_NC]);
>>>>>  
>>>>>  	ravb_set_delay_mode(ndev);
>>>>> +	ravb_write(ndev, priv->desc_bat_dma, DBAT);
>>>
>>>    Looking at it again, I suspect this belong in ravb_dmac_init()...
>>
>> ravb_dmac_init() is called from multiple places in this driver, e.g.,
> 
>    It's purpose is to configure AVB-DMAC and DBAT is the AVB-DMAC register,
> right?

It is. But it is pointless to configure it more than one time after
ravb_open() has been called as the register content is not changed until IP
enters reset mode (though ravb_close() now).

> 
>> ravb_set_ringparam(), ravb_tx_timeout_work().
> 
>    I know. Its value is only calculated once, in ravb_probe(), right?

right

> 
>> I'm afraid we may broke the behavior of these if DBAT setup is moved

I was wrong here. DBAT is not changed by IP while TX/RX is working.

> 
>    Do not be afraid! :-)
> 
>> in ravb_dmac_init(). This is also
>> valid for setting delay (see patch 10/12).
> 
>    I don't think there will be a problem either... but maybe we
> should call it in ravb_emac_init() indeed.
> 
> [...]
> 
> MBR, Sergey

Re: [PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API

[Sergey Shtylyov]

On 12/20/23 2:41 PM, claudiu beznea wrote:

[...]

>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>
>>>>>> DBAT setup was done in the driver's probe API. As some IP variants switch
>>>>>> to reset mode (and thus registers' content is lost) when setting clocks
>>>>>> (due to module standby functionality) to be able to implement runtime PM
>>>>>> move the DBAT configuration in the driver's ndo_open API.
>>>>>>
>>>>>> This commit prepares the code for the addition of runtime PM.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>
>>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>>>
>>>>> [...]
>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> index 04eaa1967651..6b8ca08be35e 100644
>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev)
>>>>>>  		napi_enable(&priv->napi[RAVB_NC]);
>>>>>>  
>>>>>>  	ravb_set_delay_mode(ndev);
>>>>>> +	ravb_write(ndev, priv->desc_bat_dma, DBAT);
>>>>
>>>>    Looking at it again, I suspect this belong in ravb_dmac_init()...
>>>
>>> ravb_dmac_init() is called from multiple places in this driver, e.g.,
>>
>>    It's purpose is to configure AVB-DMAC and DBAT is the AVB-DMAC register,
>> right?
> 
> It is. But it is pointless to configure it more than one time after
> ravb_open() has been called as the register content is not changed until IP
> enters reset mode (though ravb_close() now).

   The same is true for the most registers set by ravb_dmac_init()!

[...]

MBR, Sergey

[PATCH net-next v2 12/21] net: ravb: Move ptp initialization in the driver's ndo_open API for ccc_gac platorms

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The initialization sequence for PTP is the same for platforms with ccc_gac
and gptp (according to chapter "Figure 50.71 Flow of gPTP Initialization
(Normal, Common to All Modes)" of the R-Car Series, 3rd generation hardware
manual and chapter "Figure 37A.53 Flow of gPTP Initialization (Normal,
Common to All Modes)" of the RZ/G Series hardware manual).

As some IP variants switch to reset mode (and thus registers' content is
lost) when setting clocks (due to module standby functionality) to be able
to implement runtime PM, move the PTP initialization to the driver's
ndo_open API.

This commit prepares the code for the addition of runtime PM.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 6b8ca08be35e..db9222fc57c2 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1835,7 +1835,7 @@ static int ravb_open(struct net_device *ndev)
 		goto out_dma_stop;
 
 	/* Initialise PTP Clock driver */
-	if (info->gptp)
+	if (info->gptp || info->ccc_gac)
 		ravb_ptp_init(ndev, priv->pdev);
 
 	/* PHY control start */
@@ -1849,7 +1849,7 @@ static int ravb_open(struct net_device *ndev)
 
 out_ptp_stop:
 	/* Stop PTP Clock driver */
-	if (info->gptp)
+	if (info->gptp || info->ccc_gac)
 		ravb_ptp_stop(ndev);
 out_dma_stop:
 	ravb_stop_dma(ndev);
@@ -2151,7 +2151,7 @@ static int ravb_close(struct net_device *ndev)
 	ravb_write(ndev, 0, TIC);
 
 	/* Stop PTP Clock driver */
-	if (info->gptp)
+	if (info->gptp || info->ccc_gac)
 		ravb_ptp_stop(ndev);
 
 	/* Set the config mode to stop the AVB-DMAC's processes */
@@ -2846,10 +2846,6 @@ static int ravb_probe(struct platform_device *pdev)
 	/* Initialise HW timestamp list */
 	INIT_LIST_HEAD(&priv->ts_skb_list);
 
-	/* Initialise PTP Clock driver */
-	if (info->ccc_gac)
-		ravb_ptp_init(ndev, pdev);
-
 	/* Debug message level */
 	priv->msg_enable = RAVB_DEF_MSG_ENABLE;
 
@@ -2894,10 +2890,6 @@ static int ravb_probe(struct platform_device *pdev)
 out_dma_free:
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
-
-	/* Stop PTP Clock driver */
-	if (info->ccc_gac)
-		ravb_ptp_stop(ndev);
 out_rpm_put:
 	pm_runtime_put(&pdev->dev);
 out_rpm_disable:
@@ -2924,10 +2916,6 @@ static void ravb_remove(struct platform_device *pdev)
 
 	ravb_mdio_release(priv);
 
-	/* Stop PTP Clock driver */
-	if (info->ccc_gac)
-		ravb_ptp_stop(ndev);
-
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 
-- 
2.39.2

Re: [PATCH net-next v2 12/21] net: ravb: Move ptp initialization in the driver's ndo_open API for ccc_gac platorms

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The initialization sequence for PTP is the same for platforms with ccc_gac
> and gptp (according to chapter "Figure 50.71 Flow of gPTP Initialization
> (Normal, Common to All Modes)" of the R-Car Series, 3rd generation hardware
   Figure is hardly a chapter. :-)

> manual and chapter "Figure 37A.53 Flow of gPTP Initialization (Normal,

   Here as well...

> Common to All Modes)" of the RZ/G Series hardware manual).
> 
> As some IP variants switch to reset mode (and thus registers' content is

   The register content.

> lost) when setting clocks (due to module standby functionality) to be able
> to implement runtime PM, move the PTP initialization to the driver's
> ndo_open API.
> 
> This commit prepares the code for the addition of runtime PM.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[PATCH net-next v2 13/21] net: ravb: Set config mode in ndo_open and reset mode in ndo_close

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

As some IP variants switch to reset mode (and thus registers' content is
lost) when setting clocks (due to module standby functionality) to be able
to implement runtime PM and save more power, set the IP's operation mode to
reset at the end of the probe. Along with it, in the ndo_open API the IP
will be switched to configuration, then operational mode. In the ndo_close
API, the IP will be switched back to reset mode. This allows implementing
runtime PM and, along with it, save more power when the IP is not used.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 91 ++++++++++++++----------
 1 file changed, 54 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index db9222fc57c2..31a1f8a83652 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1738,6 +1738,30 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
 	return error;
 }
 
+static int ravb_set_config_mode(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_hw_info *info = priv->info;
+	int error;
+
+	if (info->gptp) {
+		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
+		/* Set CSEL value */
+		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
+	} else if (info->ccc_gac) {
+		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
+			    CCC_GAC | CCC_CSEL_HPB);
+	} else {
+		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
+	}
+
+	error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
+	if (error)
+		netdev_err(ndev, "failed to switch device to config mode\n");
+
+	return error;
+}
+
 static int ravb_set_reset_mode(struct net_device *ndev)
 {
 	int error;
@@ -1821,13 +1845,19 @@ static int ravb_open(struct net_device *ndev)
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
 
+	/* Set AVB config mode */
+	error = ravb_set_config_mode(ndev);
+	if (error)
+		goto out_napi_off;
+
 	ravb_set_delay_mode(ndev);
 	ravb_write(ndev, priv->desc_bat_dma, DBAT);
 
 	/* Device init */
 	error = ravb_dmac_init(ndev);
 	if (error)
-		goto out_napi_off;
+		goto out_set_reset;
+
 	ravb_emac_init(ndev);
 
 	error = ravb_set_gti(ndev);
@@ -1853,6 +1883,8 @@ static int ravb_open(struct net_device *ndev)
 		ravb_ptp_stop(ndev);
 out_dma_stop:
 	ravb_stop_dma(ndev);
+out_set_reset:
+	ravb_set_reset_mode(ndev);
 out_napi_off:
 	if (info->nc_queues)
 		napi_disable(&priv->napi[RAVB_NC]);
@@ -2187,6 +2219,9 @@ static int ravb_close(struct net_device *ndev)
 	if (info->nc_queues)
 		ravb_ring_free(ndev, RAVB_NC);
 
+	/* Set reset mode. */
+	ravb_set_reset_mode(ndev);
+
 	return 0;
 }
 
@@ -2517,30 +2552,6 @@ static const struct of_device_id ravb_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
 
-static int ravb_set_config_mode(struct net_device *ndev)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
-	int error;
-
-	if (info->gptp) {
-		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
-		/* Set CSEL value */
-		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
-	} else if (info->ccc_gac) {
-		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
-			    CCC_GAC | CCC_CSEL_HPB);
-	} else {
-		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
-	}
-
-	error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
-	if (error)
-		netdev_err(ndev, "failed to switch device to config mode\n");
-
-	return error;
-}
-
 /* Set tx and rx clock internal delay modes */
 static void ravb_parse_delay_mode(struct device_node *np, struct net_device *ndev)
 {
@@ -2818,11 +2829,6 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->netdev_ops = &ravb_netdev_ops;
 	ndev->ethtool_ops = &ravb_ethtool_ops;
 
-	/* Set AVB config mode */
-	error = ravb_set_config_mode(ndev);
-	if (error)
-		goto out_rpm_put;
-
 	error = ravb_compute_gti(ndev);
 	if (error)
 		goto out_rpm_put;
@@ -2857,11 +2863,16 @@ static int ravb_probe(struct platform_device *pdev)
 		eth_hw_addr_random(ndev);
 	}
 
+	/* Set config mode as this is needed for PHY initialization. */
+	error = ravb_config(ndev);
+	if (error)
+		goto out_rpm_put;
+
 	/* MDIO bus init */
 	error = ravb_mdio_init(priv);
 	if (error) {
 		dev_err(&pdev->dev, "failed to initialize MDIO\n");
-		goto out_dma_free;
+		goto out_reset_mode;
 	}
 
 	netif_napi_add(ndev, &priv->napi[RAVB_BE], ravb_poll);
@@ -2875,19 +2886,30 @@ static int ravb_probe(struct platform_device *pdev)
 
 	device_set_wakeup_capable(&pdev->dev, 1);
 
+	/* Reset MAC as the module will be runtime disabled at this moment.
+	 * This saves power. MAC will be switched back to configuration mode
+	 * in ravb_open().
+	 */
+	error = ravb_set_reset_mode(ndev);
+	if (error)
+		goto out_netdev_unregister;
+
 	/* Print device information */
 	netdev_info(ndev, "Base address at %#x, %pM, IRQ %d.\n",
 		    (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
 
 	return 0;
 
+out_netdev_unregister:
+	unregister_netdev(ndev);
 out_napi_del:
 	if (info->nc_queues)
 		netif_napi_del(&priv->napi[RAVB_NC]);
 
 	netif_napi_del(&priv->napi[RAVB_BE]);
 	ravb_mdio_release(priv);
-out_dma_free:
+out_reset_mode:
+	ravb_set_reset_mode(ndev);
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 out_rpm_put:
@@ -2907,7 +2929,6 @@ static void ravb_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
-	int error;
 
 	unregister_netdev(ndev);
 	if (info->nc_queues)
@@ -2919,10 +2940,6 @@ static void ravb_remove(struct platform_device *pdev)
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 
-	error = ravb_set_reset_mode(ndev);
-	if (error)
-		netdev_err(ndev, "Failed to reset ndev\n");
-
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	clk_unprepare(priv->refclk);
-- 
2.39.2

Re: [PATCH net-next v2 13/21] net: ravb: Set config mode in ndo_open and reset mode in ndo_close

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> As some IP variants switch to reset mode (and thus registers' content is

   Register.

> lost) when setting clocks (due to module standby functionality) to be able
> to implement runtime PM and save more power, set the IP's operation mode to

   Operating.

> reset at the end of the probe. Along with it, in the ndo_open API the IP
> will be switched to configuration, then operational mode. In the ndo_close
> API, the IP will be switched back to reset mode. This allows implementing
> runtime PM and, along with it, save more power when the IP is not used.
> 
> 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 db9222fc57c2..31a1f8a83652 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1821,13 +1845,19 @@ static int ravb_open(struct net_device *ndev)
>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  
> +	/* Set AVB config mode */
> +	error = ravb_set_config_mode(ndev);
> +	if (error)
> +		goto out_napi_off;
> +

   I suspect this too belongs in ravb_dmac_init() now...

[...]
> @@ -2875,19 +2886,30 @@ static int ravb_probe(struct platform_device *pdev)
>  
>  	device_set_wakeup_capable(&pdev->dev, 1);
>  
> +	/* Reset MAC as the module will be runtime disabled at this moment.
> +	 * This saves power. MAC will be switched back to configuration mode
> +	 * in ravb_open().
> +	 */
> +	error = ravb_set_reset_mode(ndev);
> +	if (error)
> +		goto out_netdev_unregister;
> +

   I think this now races with the register_netdev() call above (the device
can be opened before it returns)! Should be called before register_netdev()...

[...]

MBR, Sergey

Re: [PATCH net-next v2 13/21] net: ravb: Set config mode in ndo_open and reset mode in ndo_close

[claudiu beznea]

On 16.12.2023 19:28, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> As some IP variants switch to reset mode (and thus registers' content is
> 
>    Register.
> 
>> lost) when setting clocks (due to module standby functionality) to be able
>> to implement runtime PM and save more power, set the IP's operation mode to
> 
>    Operating.
> 
>> reset at the end of the probe. Along with it, in the ndo_open API the IP
>> will be switched to configuration, then operational mode. In the ndo_close
>> API, the IP will be switched back to reset mode. This allows implementing
>> runtime PM and, along with it, save more power when the IP is not used.
>>
>> 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 db9222fc57c2..31a1f8a83652 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -1821,13 +1845,19 @@ static int ravb_open(struct net_device *ndev)
>>  	if (info->nc_queues)
>>  		napi_enable(&priv->napi[RAVB_NC]);
>>  
>> +	/* Set AVB config mode */
>> +	error = ravb_set_config_mode(ndev);
>> +	if (error)
>> +		goto out_napi_off;
>> +
> 
>    I suspect this too belongs in ravb_dmac_init() now...

What I can do here is to keep PTP/GAC specific settings from
ravb_set_config_mode() in a separate function close to PTP setup and remove
ravb_set_config_mode() at all as ravb_dmac_init() switches anyway the IP to
config mode. But with this I don't know how the PTP/GAC will be influenced
as I don't have a setup to check it. From my memories, the commit that
introduces the setup of PTP when switching to config mode did this by
intention, so I'm not sure weather playing around with this is the way to
go forward. Do you remember something specific about this?

> 
> [...]
>> @@ -2875,19 +2886,30 @@ static int ravb_probe(struct platform_device *pdev)
>>  
>>  	device_set_wakeup_capable(&pdev->dev, 1);
>>  
>> +	/* Reset MAC as the module will be runtime disabled at this moment.
>> +	 * This saves power. MAC will be switched back to configuration mode
>> +	 * in ravb_open().
>> +	 */
>> +	error = ravb_set_reset_mode(ndev);
>> +	if (error)
>> +		goto out_netdev_unregister;
>> +
> 
>    I think this now races with the register_netdev() call above (the device
> can be opened before it returns)! Should be called before register_netdev()...
> 

Good point! Thanks!

> [...]
> 
> MBR, Sergey

Re: [PATCH net-next v2 13/21] net: ravb: Set config mode in ndo_open and reset mode in ndo_close

[Sergey Shtylyov]

On 12/17/23 4:15 PM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> As some IP variants switch to reset mode (and thus registers' content is
>>
>>    Register.
>>
>>> lost) when setting clocks (due to module standby functionality) to be able
>>> to implement runtime PM and save more power, set the IP's operation mode to
>>
>>    Operating.
>>
>>> reset at the end of the probe. Along with it, in the ndo_open API the IP
>>> will be switched to configuration, then operational mode. In the ndo_close
>>> API, the IP will be switched back to reset mode. This allows implementing
>>> runtime PM and, along with it, save more power when the IP is not used.
>>>
>>> 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 db9222fc57c2..31a1f8a83652 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> [...]
>>> @@ -1821,13 +1845,19 @@ static int ravb_open(struct net_device *ndev)
>>>  	if (info->nc_queues)
>>>  		napi_enable(&priv->napi[RAVB_NC]);
>>>  
>>> +	/* Set AVB config mode */
>>> +	error = ravb_set_config_mode(ndev);
>>> +	if (error)
>>> +		goto out_napi_off;
>>> +
>>
>>    I suspect this too belongs in ravb_dmac_init() now...
> 
> What I can do here is to keep PTP/GAC specific settings from
> ravb_set_config_mode() in a separate function close to PTP setup and remove
> ravb_set_config_mode() at all as ravb_dmac_init() switches anyway the IP to
> config mode. But with this I don't know how the PTP/GAC will be influenced

   My manuals say there are certain limitations (currently reflected in
ravb_set_config_mode()) WRT setting CCC.CSEL and CCC.GAC bits.

> as I don't have a setup to check it. From my memories, the commit that
> introduces the setup of PTP when switching to config mode did this by
> intention, so I'm not sure weather playing around with this is the way to
> go forward. Do you remember something specific about this?

   I haven't ever tested PTP, IIRC...

[...]

MBR, Sergey

[PATCH net-next v2 14/21] net: ravb: Simplify ravb_suspend()

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

As ravb_close() contains now the call to ravb_ptp_stop() for both ccc_gac
and gPTP aware platforms, there is no need to keep the separated call in
ravb_suspend(). Instead, move it to ravb_wol_setup(). In this way the
resulting code is cleaner.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 31a1f8a83652..16450bf241cd 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2968,6 +2968,9 @@ static int ravb_wol_setup(struct net_device *ndev)
 	/* Enable MagicPacket */
 	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
 
+	if (priv->info->ccc_gac)
+		ravb_ptp_stop(ndev);
+
 	return enable_irq_wake(priv->emac_irq);
 }
 
@@ -3000,14 +3003,10 @@ static int ravb_suspend(struct device *dev)
 	netif_device_detach(ndev);
 
 	if (priv->wol_enabled)
-		ret = ravb_wol_setup(ndev);
-	else
-		ret = ravb_close(ndev);
+		return ravb_wol_setup(ndev);
 
-	if (priv->info->ccc_gac)
-		ravb_ptp_stop(ndev);
-
-	if (priv->wol_enabled)
+	ret = ravb_close(ndev);
+	if (ret)
 		return ret;
 
 reset_assert:
-- 
2.39.2

Re: [PATCH net-next v2 14/21] net: ravb: Simplify ravb_suspend()

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> As ravb_close() contains now the call to ravb_ptp_stop() for both ccc_gac
> and gPTP aware platforms, there is no need to keep the separated call in

   Separate.

> ravb_suspend(). Instead, move it to ravb_wol_setup(). In this way the
> resulting code is cleaner.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[PATCH net-next v2 15/21] net: ravb: Simplify ravb_resume()

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Remove explicit calls to functions that are called ravb_open(). There is
no need to have them doubled now that the ravb_open() already contains
what is needed for the interface configuration. Along with it, PTP needed
configurations were moved to ravb_wol_restore(). With this, code in
ravb_resume() is simpler.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 60 ++++++++++--------------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 16450bf241cd..b581666e341f 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2978,6 +2978,20 @@ static int ravb_wol_restore(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
+	int error;
+
+	/* Set reset mode to rearm the WoL logic. */
+	error = ravb_set_reset_mode(ndev);
+	if (error)
+		return error;
+
+	/* Set AVB config mode. */
+	error = ravb_set_config_mode(ndev);
+	if (error)
+		return error;
+
+	if (priv->info->ccc_gac)
+		ravb_ptp_init(ndev, priv->pdev);
 
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
@@ -3017,55 +3031,29 @@ static int ravb_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
 	int ret;
 
 	ret = reset_control_deassert(priv->rstc);
 	if (ret)
 		return ret;
 
-	/* If WoL is enabled set reset mode to rearm the WoL logic */
+	if (!netif_running(ndev))
+		return 0;
+
+	/* If WoL is enabled restore the interface. */
 	if (priv->wol_enabled) {
-		ret = ravb_set_reset_mode(ndev);
+		ret = ravb_wol_restore(ndev);
 		if (ret)
 			return ret;
 	}
 
-	/* All register have been reset to default values.
-	 * Restore all registers which where setup at probe time and
-	 * reopen device if it was running before system suspended.
-	 */
-
-	/* Set AVB config mode */
-	ret = ravb_set_config_mode(ndev);
-	if (ret)
+	/* Reopening the interface will restore the device to the working state. */
+	ret = ravb_open(ndev);
+	if (ret < 0)
 		return ret;
 
-	ret = ravb_set_gti(ndev);
-	if (ret)
-		return ret;
-
-	if (info->internal_delay)
-		ravb_set_delay_mode(ndev);
-
-	/* Restore descriptor base address table */
-	ravb_write(ndev, priv->desc_bat_dma, DBAT);
-
-	if (priv->info->ccc_gac)
-		ravb_ptp_init(ndev, priv->pdev);
-
-	if (netif_running(ndev)) {
-		if (priv->wol_enabled) {
-			ret = ravb_wol_restore(ndev);
-			if (ret)
-				return ret;
-		}
-		ret = ravb_open(ndev);
-		if (ret < 0)
-			return ret;
-		ravb_set_rx_mode(ndev);
-		netif_device_attach(ndev);
-	}
+	ravb_set_rx_mode(ndev);
+	netif_device_attach(ndev);
 
 	return ret;
 }
-- 
2.39.2

Re: [PATCH net-next v2 15/21] net: ravb: Simplify ravb_resume()

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Remove explicit calls to functions that are called ravb_open(). There is
                                                    ^ by?

> no need to have them doubled now that the ravb_open() already contains
> what is needed for the interface configuration. Along with it, PTP needed
> configurations were moved to ravb_wol_restore(). With this, code in

   Configurations needed by PTP?

> ravb_resume() is simpler.

   s/is/becomes/?

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

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[PATCH net-next v2 16/21] net: ravb: Keep the reverse order of operations in ravb_close()

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Keep the reverse order of operations in ravb_close() when comparing with
ravb_open(). This is the recommended configuration sequence.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index b581666e341f..38999ef1ea85 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2182,6 +2182,14 @@ static int ravb_close(struct net_device *ndev)
 	ravb_write(ndev, 0, RIC2);
 	ravb_write(ndev, 0, TIC);
 
+	/* PHY disconnect */
+	if (ndev->phydev) {
+		phy_stop(ndev->phydev);
+		phy_disconnect(ndev->phydev);
+		if (of_phy_is_fixed_link(np))
+			of_phy_deregister_fixed_link(np);
+	}
+
 	/* Stop PTP Clock driver */
 	if (info->gptp || info->ccc_gac)
 		ravb_ptp_stop(ndev);
@@ -2200,14 +2208,6 @@ static int ravb_close(struct net_device *ndev)
 		}
 	}
 
-	/* PHY disconnect */
-	if (ndev->phydev) {
-		phy_stop(ndev->phydev);
-		phy_disconnect(ndev->phydev);
-		if (of_phy_is_fixed_link(np))
-			of_phy_deregister_fixed_link(np);
-	}
-
 	cancel_work_sync(&priv->work);
 
 	if (info->nc_queues)
-- 
2.39.2

Re: [PATCH net-next v2 16/21] net: ravb: Keep the reverse order of operations in ravb_close()

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Keep the reverse order of operations in ravb_close() when comparing with

   Compared.

> ravb_open(). This is the recommended configuration sequence.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[PATCH net-next v2 17/21] net: ravb: Keep clock request operations grouped together

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Keep clock request operations grouped togeter to have all clock-related
code in a single place. This makes the code simpler to follow.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 38999ef1ea85..a2a64c22ec41 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2768,6 +2768,20 @@ static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_reset_assert;
 
+	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);
@@ -2801,20 +2815,6 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(priv->clk)) {
-		error = PTR_ERR(priv->clk);
-		goto out_rpm_put;
-	}
-
-	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_rpm_put;
-		}
-	}
-
 	ndev->max_mtu = info->rx_max_buf_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;
 
-- 
2.39.2

Re: [PATCH net-next v2 17/21] net: ravb: Keep clock request operations grouped together

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Keep clock request operations grouped togeter to have all clock-related
> code in a single place. This makes the code simpler to follow.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this patch is new
> 
>  drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 38999ef1ea85..a2a64c22ec41 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2768,6 +2768,20 @@ static int ravb_probe(struct platform_device *pdev)
>  	if (error)
>  		goto out_reset_assert;
>  
> +	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);

   Hmm... I think we currently have all these calls in one place.
Perhaps you just shouldn't have moved this code around?

MBR, Sergey

Re: [PATCH net-next v2 17/21] net: ravb: Keep clock request operations grouped together

[claudiu beznea]

On 16.12.2023 21:43, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Keep clock request operations grouped togeter to have all clock-related
>> code in a single place. This makes the code simpler to follow.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this patch is new
>>
>>  drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 38999ef1ea85..a2a64c22ec41 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2768,6 +2768,20 @@ static int ravb_probe(struct platform_device *pdev)
>>  	if (error)
>>  		goto out_reset_assert;
>>  
>> +	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);
> 
>    Hmm... I think we currently have all these calls in one place.
> Perhaps you just shouldn't have moved this code around?

refclk have been moved at this point due to runtime PM. As refclk was
changed to be part of driver's runtime PM APIs we need to have it requested
(and prepared) before pm_runtime_resume_and_get(). Calling
pm_runtime_resume_and_get() will call driver's runtime PM resume.

The idea with this patch was to have all clock requests (clk, gptp, refclk)
in a single place (it's easier to follow the code this way, in my opinion).
If you prefer I can squash this patch with patch 07/21 "net: ravb: Move
reference clock enable/disable on runtime PM APIs". Please, let me know
what do you think.

> 
> MBR, Sergey

Re: [PATCH net-next v2 17/21] net: ravb: Keep clock request operations grouped together

[Sergey Shtylyov]

On 12/17/23 4:22 PM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Keep clock request operations grouped togeter to have all clock-related
>>> code in a single place. This makes the code simpler to follow.
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> ---
>>>
>>> Changes in v2:
>>> - none; this patch is new
>>>
>>>  drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++------------
>>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 38999ef1ea85..a2a64c22ec41 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2768,6 +2768,20 @@ static int ravb_probe(struct platform_device *pdev)
>>>  	if (error)
>>>  		goto out_reset_assert;
>>>  
>>> +	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);
>>
>>    Hmm... I think we currently have all these calls in one place.
>> Perhaps you just shouldn't have moved this code around?
> 
> refclk have been moved at this point due to runtime PM. As refclk was
> changed to be part of driver's runtime PM APIs we need to have it requested
> (and prepared) before pm_runtime_resume_and_get(). Calling
> pm_runtime_resume_and_get() will call driver's runtime PM resume.
> 
> The idea with this patch was to have all clock requests (clk, gptp, refclk)
> in a single place (it's easier to follow the code this way, in my opinion).
> If you prefer I can squash this patch with patch 07/21 "net: ravb: Move
> reference clock enable/disable on runtime PM APIs". Please, let me know
> what do you think.

   Yes, please move all 3 clocks at once.

MBR, Sergey

[PATCH net-next v2 18/21] net: ravb: Return cached statistics if the interface is down

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Return the cached statistics in case the interface is down. There should be
no drawback to this, as most of the statistics are updated on the data path
and if runtime PM is enabled and the interface is down, the registers that
are explicitly read on ravb_get_stats() are zero anyway on most of the IP
variants.

The commit prepares the code for the addition of runtime PM support.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index a2a64c22ec41..1995cf7ff084 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 	const struct ravb_hw_info *info = priv->info;
 	struct net_device_stats *nstats, *stats0, *stats1;
 
+	if (!netif_running(ndev))
+		return &ndev->stats;
+
 	nstats = &ndev->stats;
 	stats0 = &priv->stats[RAVB_BE];
 
-- 
2.39.2

Re: [PATCH net-next v2 18/21] net: ravb: Return cached statistics if the interface is down

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Return the cached statistics in case the interface is down. There should be
> no drawback to this, as most of the statistics are updated on the data path
> and if runtime PM is enabled and the interface is down, the registers that
> are explicitly read on ravb_get_stats() are zero anyway on most of the IP
> variants.
> 
> The commit prepares the code for the addition of runtime PM support.
> 
> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this patch is new
> 
>  drivers/net/ethernet/renesas/ravb_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a2a64c22ec41..1995cf7ff084 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>  	const struct ravb_hw_info *info = priv->info;
>  	struct net_device_stats *nstats, *stats0, *stats1;
>  
> +	if (!netif_running(ndev))

   I'm afraid this is racy as __LINK_STATE_START bit gets set
by __dev_open() before calling the ndo_open() method... :-(

> +		return &ndev->stats;
> +

   The sh_eth driver calls sh_eth_get_stats() from sh_eth_dev_exit();
perhaps it's worth doing something similar?

MBR, Sergey

Re: [PATCH net-next v2 18/21] net: ravb: Return cached statistics if the interface is down

[claudiu beznea]

On 16.12.2023 22:02, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Return the cached statistics in case the interface is down. There should be
>> no drawback to this, as most of the statistics are updated on the data path
>> and if runtime PM is enabled and the interface is down, the registers that
>> are explicitly read on ravb_get_stats() are zero anyway on most of the IP
>> variants.
>>
>> The commit prepares the code for the addition of runtime PM support.
>>
>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this patch is new
>>
>>  drivers/net/ethernet/renesas/ravb_main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index a2a64c22ec41..1995cf7ff084 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>>  	const struct ravb_hw_info *info = priv->info;
>>  	struct net_device_stats *nstats, *stats0, *stats1;
>>  
>> +	if (!netif_running(ndev))
> 
>    I'm afraid this is racy as __LINK_STATE_START bit gets set
> by __dev_open() before calling the ndo_open() method... :-(

But (at least on my setup), both ndo_get_stats and ndo_open are called with
rtnl_mutex locked.

> 
>> +		return &ndev->stats;
>> +
> 
>    The sh_eth driver calls sh_eth_get_stats() from sh_eth_dev_exit();
> perhaps it's worth doing something similar?

Indeed, it might help to keep updated those few registers that gets updated
only in ndo_get_stats.


> 
> MBR, Sergey

Re: [PATCH net-next v2 18/21] net: ravb: Return cached statistics if the interface is down

[Sergey Shtylyov]

On 12/17/23 4:54 PM, claudiu beznea wrote:

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Return the cached statistics in case the interface is down. There should be
>>> no drawback to this, as most of the statistics are updated on the data path
>>> and if runtime PM is enabled and the interface is down, the registers that
>>> are explicitly read on ravb_get_stats() are zero anyway on most of the IP
>>> variants.
>>>
>>> The commit prepares the code for the addition of runtime PM support.
>>>
>>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> ---
>>>
>>> Changes in v2:
>>> - none; this patch is new
>>>
>>>  drivers/net/ethernet/renesas/ravb_main.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index a2a64c22ec41..1995cf7ff084 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>>>  	const struct ravb_hw_info *info = priv->info;
>>>  	struct net_device_stats *nstats, *stats0, *stats1;
>>>  
>>> +	if (!netif_running(ndev))
>>
>>    I'm afraid this is racy as __LINK_STATE_START bit gets set
>> by __dev_open() before calling the ndo_open() method... :-(
> 
> But (at least on my setup), both ndo_get_stats and ndo_open are called with
> rtnl_mutex locked.

   Unfortunately, it's not always so -- see e.g. netstat_show() in net/core/net-sysfs.c...
 
[...]

MBR, Sergey

Re: [PATCH net-next v2 18/21] net: ravb: Return cached statistics if the interface is down

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Return the cached statistics in case the interface is down. There should be
> no drawback to this, as most of the statistics are updated on the data path
> and if runtime PM is enabled and the interface is down, the registers that
> are explicitly read on ravb_get_stats() are zero anyway on most of the IP
> variants.
> 
> The commit prepares the code for the addition of runtime PM support.
> 
> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this patch is new
> 
>  drivers/net/ethernet/renesas/ravb_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a2a64c22ec41..1995cf7ff084 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>  	const struct ravb_hw_info *info = priv->info;
>  	struct net_device_stats *nstats, *stats0, *stats1;
>  
> +	if (!netif_running(ndev))

   I'm afraid this is racy as __LINK_STATE_START bit gets set
by __dev_open() before calling the ndo_open() method... :-(

> +		return &ndev->stats;
> +

   The sh_eth driver calls sh_eth_get_stats() from sh_eth_dev_exit();
perhaps it's worth doing something similar?

MBR, Sergey

[PATCH net-next v2 19/21] net: ravb: Do not set promiscuous mode if the interface is down

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Do not allow setting promiscuous mode if the interface is down. In case
runtime PM is enabled, and while interface is down, the IP will be in reset
mode (as for some platforms disabling/enabling the clocks will switch the
IP to standby mode which will lead to losing registers' content).

Commit prepares for the addition of runtime PM.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 1995cf7ff084..633346b6cd7c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2164,6 +2164,9 @@ static void ravb_set_rx_mode(struct net_device *ndev)
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned long flags;
 
+	if (!netif_running(ndev))
+		return;
+
 	spin_lock_irqsave(&priv->lock, flags);
 	ravb_modify(ndev, ECMR, ECMR_PRM,
 		    ndev->flags & IFF_PROMISC ? ECMR_PRM : 0);
-- 
2.39.2

Re: [PATCH net-next v2 19/21] net: ravb: Do not set promiscuous mode if the interface is down

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Do not allow setting promiscuous mode if the interface is down. In case
> runtime PM is enabled, and while interface is down, the IP will be in reset
> mode (as for some platforms disabling/enabling the clocks will switch the
> IP to standby mode which will lead to losing registers' content).

   Register.
   Have this issue actually occurred for you?

> Commit prepares for the addition of runtime PM.
> 
> 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 1995cf7ff084..633346b6cd7c 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2164,6 +2164,9 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	unsigned long flags;
>  
> +	if (!netif_running(ndev))

   Seems racy as well...

> +		return;

   Hm, sh_eth.c doesn't have such check -- perhaps should be fixed
as well...

[...]

MBR, Sergey

Re: [PATCH net-next v2 19/21] net: ravb: Do not set promiscuous mode if the interface is down

[claudiu beznea]

On 16.12.2023 22:16, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Do not allow setting promiscuous mode if the interface is down. In case
>> runtime PM is enabled, and while interface is down, the IP will be in reset
>> mode (as for some platforms disabling/enabling the clocks will switch the
>> IP to standby mode which will lead to losing registers' content).
> 
>    Register.
>    Have this issue actually occurred for you?
> 
>> Commit prepares for the addition of runtime PM.
>>
>> 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 1995cf7ff084..633346b6cd7c 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2164,6 +2164,9 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	unsigned long flags;
>>  
>> +	if (!netif_running(ndev))
> 
>    Seems racy as well...

It is also called with rtnl_mutex locked at least though __dev_change_flags().

> 
>> +		return;
> 
>    Hm, sh_eth.c doesn't have such check -- perhaps should be fixed
> as well...
> 
> [...]
> 
> MBR, Sergey

Re: [PATCH net-next v2 19/21] net: ravb: Do not set promiscuous mode if the interface is down

[Sergey Shtylyov]

On 12/17/23 5:02 PM, claudiu beznea wrote:

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Do not allow setting promiscuous mode if the interface is down. In case
>>> runtime PM is enabled, and while interface is down, the IP will be in reset
>>> mode (as for some platforms disabling/enabling the clocks will switch the
>>> IP to standby mode which will lead to losing registers' content).
>>
>>    Register.
>>    Have this issue actually occurred for you?
>>
>>> Commit prepares for the addition of runtime PM.
>>>
>>> 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 1995cf7ff084..633346b6cd7c 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2164,6 +2164,9 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>  	unsigned long flags;
>>>  
>>> +	if (!netif_running(ndev))
>>
>>    Seems racy as well...
> 
> It is also called with rtnl_mutex locked at least though __dev_change_flags().

   I'm tired of chasing the call tree for you. :-)
   Since I'm hoping we'll agree on the ndo_get_stats() case, it would
seem safer to use an is_opened flag here as well, like sh_eth.c does.

[...]

MBR, Sergey

[PATCH net-next v2 20/21] net: ravb: Do not apply RX CSUM settings to hardware if the interface is down

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Do not apply the RX CSUM settings to hardware if the interface is down. In
case runtime PM is enabled, and while the interface is down, the IP will be
in reset mode (as for some platforms disabling/enabling the clocks will
switch the IP to standby mode, which will lead to losing registers'
content) and applying settings in reset mode is not an option. Instead,
cache the RX CSUM settings and apply them in ravb_open().

Commit prepares for the addition of runtime PM.

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

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 633346b6cd7c..9ff943dff522 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1868,6 +1868,15 @@ static int ravb_open(struct net_device *ndev)
 	if (info->gptp || info->ccc_gac)
 		ravb_ptp_init(ndev, priv->pdev);
 
+	/* Apply features that might have been changed while the interface
+	 * was down.
+	 */
+	if (ndev->hw_features & NETIF_F_RXCSUM) {
+		u32 val = (ndev->features & NETIF_F_RXCSUM) ? ECMR_RCSC : 0;
+
+		ravb_modify(ndev, ECMR, ECMR_RCSC, val);
+	}
+
 	/* PHY control start */
 	error = ravb_phy_start(ndev);
 	if (error)
@@ -2337,6 +2346,9 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned long flags;
 
+	if (!netif_running(ndev))
+		return;
+
 	spin_lock_irqsave(&priv->lock, flags);
 
 	/* Disable TX and RX */
-- 
2.39.2

Re: [PATCH net-next v2 20/21] net: ravb: Do not apply RX CSUM settings to hardware if the interface is down

[Sergey Shtylyov]

On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Do not apply the RX CSUM settings to hardware if the interface is down. In
> case runtime PM is enabled, and while the interface is down, the IP will be
> in reset mode (as for some platforms disabling/enabling the clocks will
> switch the IP to standby mode, which will lead to losing registers'

   To/From perhaps?
   And just "register".

> content) and applying settings in reset mode is not an option. Instead,
> cache the RX CSUM settings and apply them in ravb_open().

   Have this issue actually occurred for you?

> Commit prepares for the addition of runtime PM.
> 
> 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 633346b6cd7c..9ff943dff522 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1868,6 +1868,15 @@ static int ravb_open(struct net_device *ndev)
>  	if (info->gptp || info->ccc_gac)
>  		ravb_ptp_init(ndev, priv->pdev);
>  
> +	/* Apply features that might have been changed while the interface
> +	 * was down.
> +	 */
> +	if (ndev->hw_features & NETIF_F_RXCSUM) {

   I'm afraid this is a wrong field; we need ndev->features, no?

> +		u32 val = (ndev->features & NETIF_F_RXCSUM) ? ECMR_RCSC : 0;
> +
> +		ravb_modify(ndev, ECMR, ECMR_RCSC, val);
> +	}
> +

   The ECMR setting is already done in ravb_emac_init_rcar(), no need
to duplicate it here, I think...

>  	/* PHY control start */
>  	error = ravb_phy_start(ndev);
>  	if (error)
> @@ -2337,6 +2346,9 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	unsigned long flags;
>  
> +	if (!netif_running(ndev))

   Racy as well...

> +		return;
> +

   Hm, sh_eth.c doesn't have such check -- perhaps should be fixed
as well...

[...]

MBR, Sergey

Re: [PATCH net-next v2 20/21] net: ravb: Do not apply RX CSUM settings to hardware if the interface is down

[claudiu beznea]

On 16.12.2023 22:36, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Do not apply the RX CSUM settings to hardware if the interface is down. In
>> case runtime PM is enabled, and while the interface is down, the IP will be
>> in reset mode (as for some platforms disabling/enabling the clocks will
>> switch the IP to standby mode, which will lead to losing registers'
> 
>    To/From perhaps?
>    And just "register".
> 
>> content) and applying settings in reset mode is not an option. Instead,
>> cache the RX CSUM settings and apply them in ravb_open().
> 
>    Have this issue actually occurred for you?

Setting RX CSUM while the if is down? No.

> 
>> Commit prepares for the addition of runtime PM.
>>
>> 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 633346b6cd7c..9ff943dff522 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1868,6 +1868,15 @@ static int ravb_open(struct net_device *ndev)
>>  	if (info->gptp || info->ccc_gac)
>>  		ravb_ptp_init(ndev, priv->pdev);
>>  
>> +	/* Apply features that might have been changed while the interface
>> +	 * was down.
>> +	 */
>> +	if (ndev->hw_features & NETIF_F_RXCSUM) {
> 
>    I'm afraid this is a wrong field; we need ndev->features, no?

RX CSUM is not enabled for all ravb aware devices (see struct
ravb_hw_info::net_hw_features). We should be setting the ECMR only for
these ones. ravb_hw_info::net_hw_features is set in ndev->hw_features in
probe(). So here code checks if platforms supports RXCSUM and then below it
applies what has been requested though ndo_set_features(), if any.

> 
>> +		u32 val = (ndev->features & NETIF_F_RXCSUM) ? ECMR_RCSC : 0;
>> +
>> +		ravb_modify(ndev, ECMR, ECMR_RCSC, val);
>> +	}
>> +
> 
>    The ECMR setting is already done in ravb_emac_init_rcar(), no need
> to duplicate it here, I think...

Ok, it worth being moved there.

> 
>>  	/* PHY control start */
>>  	error = ravb_phy_start(ndev);
>>  	if (error)
>> @@ -2337,6 +2346,9 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	unsigned long flags;
>>  
>> +	if (!netif_running(ndev))
> 
>    Racy as well...

It's also called with rtnl_mutex locked.

> 
>> +		return;
>> +
> 
>    Hm, sh_eth.c doesn't have such check -- perhaps should be fixed
> as well...
> 
> [...]
> 
> MBR, Sergey

Re: [PATCH net-next v2 20/21] net: ravb: Do not apply RX CSUM settings to hardware if the interface is down

[Sergey Shtylyov]

On 12/17/23 5:34 PM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Do not apply the RX CSUM settings to hardware if the interface is down. In
>>> case runtime PM is enabled, and while the interface is down, the IP will be
>>> in reset mode (as for some platforms disabling/enabling the clocks will
>>> switch the IP to standby mode, which will lead to losing registers'
>>
>>    To/From perhaps?
>>    And just "register".
>>
>>> content) and applying settings in reset mode is not an option. Instead,
>>> cache the RX CSUM settings and apply them in ravb_open().
>>
>>    Have this issue actually occurred for you?
> 
> Setting RX CSUM while the if is down? No.
> 
>>> Commit prepares for the addition of runtime PM.
>>>
>>> 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 633346b6cd7c..9ff943dff522 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1868,6 +1868,15 @@ static int ravb_open(struct net_device *ndev)
>>>  	if (info->gptp || info->ccc_gac)
>>>  		ravb_ptp_init(ndev, priv->pdev);
>>>  
>>> +	/* Apply features that might have been changed while the interface
>>> +	 * was down.
>>> +	 */
>>> +	if (ndev->hw_features & NETIF_F_RXCSUM) {
>>
>>    I'm afraid this is a wrong field; we need ndev->features, no?
> 
> RX CSUM is not enabled for all ravb aware devices (see struct
> ravb_hw_info::net_hw_features). We should be setting the ECMR only for
> these ones. ravb_hw_info::net_hw_features is set in ndev->hw_features in
> probe(). So here code checks if platforms supports RXCSUM and then below it
> applies what has been requested though ndo_set_features(), if any.

  OK. But we don't need this snippet here anyway...

>>> +		u32 val = (ndev->features & NETIF_F_RXCSUM) ? ECMR_RCSC : 0;
>>> +
>>> +		ravb_modify(ndev, ECMR, ECMR_RCSC, val);
>>> +	}
>>> +
>>
>>    The ECMR setting is already done in ravb_emac_init_rcar(), no need
>> to duplicate it here, I think...
> 
> Ok, it worth being moved there.

   No need to move, it's already there...

[...]
>>> @@ -2337,6 +2346,9 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>  	unsigned long flags;
>>>  
>>> +	if (!netif_running(ndev))
>>
>>    Racy as well...
> 
> It's also called with rtnl_mutex locked.

   Well, at least the only place that calls the ndo_set_features()
method, __netdev_update_features() calls ASSERT_RTNL() at the start.
However, since we'd have to use the is_opened flag anyway, let's rely
on it instead...

[...]

MBR, Sergey

[PATCH net-next v2 21/21] net: ravb: Add runtime PM support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Add runtime PM support for the ravb driver. As the driver is used by
different IP variants, with different behaviors, to be able to have the
runtime PM support available for all devices, the preparatory commits
moved all the resources parsing and allocations in the driver's probe
function and kept the settings for ravb_open(). This is due to the fact
that on some IP variants-platforms tuples disabling/enabling the clocks
will switch the IP to the reset operation mode where registers' content is
lost and reconfiguration needs to be done. For this the rabv_open()
function enables the clocks, switches the IP to configuration mode, applies
all the registers settings and switches the IP to the operational mode. At
the end of ravb_open() IP is ready to send/receive data.

In ravb_close() necessary reverts are done (compared with ravb_open()), the
IP is switched to reset mode and clocks are disabled.

The ethtool APIs or IOCTLs that might execute while the interface is down
are either cached (and applied in ravb_open()) or rejected (as at that time
the IP is in reset mode). Keeping the IP in the reset mode also increases
the power saved (according to the hardware manual).

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

Changes in v2:
- keep RPM support for all platforms

 drivers/net/ethernet/renesas/ravb_main.c | 47 ++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9ff943dff522..0733b63ff910 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1839,16 +1839,21 @@ static int ravb_open(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
+	struct device *dev = &priv->pdev->dev;
 	int error;
 
 	napi_enable(&priv->napi[RAVB_BE]);
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
 
+	error = pm_runtime_resume_and_get(dev);
+	if (error < 0)
+		goto out_napi_off;
+
 	/* Set AVB config mode */
 	error = ravb_set_config_mode(ndev);
 	if (error)
-		goto out_napi_off;
+		goto out_rpm_put;
 
 	ravb_set_delay_mode(ndev);
 	ravb_write(ndev, priv->desc_bat_dma, DBAT);
@@ -1894,6 +1899,9 @@ static int ravb_open(struct net_device *ndev)
 	ravb_stop_dma(ndev);
 out_set_reset:
 	ravb_set_reset_mode(ndev);
+out_rpm_put:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 out_napi_off:
 	if (info->nc_queues)
 		napi_disable(&priv->napi[RAVB_NC]);
@@ -2189,6 +2197,7 @@ static int ravb_close(struct net_device *ndev)
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
 	struct ravb_tstamp_skb *ts_skb, *ts_skb2;
+	struct device *dev = &priv->pdev->dev;
 
 	netif_tx_stop_all_queues(ndev);
 
@@ -2237,6 +2246,9 @@ static int ravb_close(struct net_device *ndev)
 	/* Set reset mode. */
 	ravb_set_reset_mode(ndev);
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 }
 
@@ -2808,6 +2820,8 @@ static int ravb_probe(struct platform_device *pdev)
 	clk_prepare(priv->refclk);
 
 	platform_set_drvdata(pdev, ndev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
+	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 	error = pm_runtime_resume_and_get(&pdev->dev);
 	if (error < 0)
@@ -2916,6 +2930,9 @@ 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);
 
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
+
 	return 0;
 
 out_netdev_unregister:
@@ -2934,6 +2951,7 @@ static int ravb_probe(struct platform_device *pdev)
 	pm_runtime_put(&pdev->dev);
 out_rpm_disable:
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	clk_unprepare(priv->refclk);
 out_reset_assert:
 	reset_control_assert(rstc);
@@ -2947,6 +2965,12 @@ static void ravb_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
+	struct device *dev = &priv->pdev->dev;
+	int error;
+
+	error = pm_runtime_resume_and_get(dev);
+	if (error < 0)
+		return;
 
 	unregister_netdev(ndev);
 	if (info->nc_queues)
@@ -2958,8 +2982,9 @@ static void ravb_remove(struct platform_device *pdev)
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 
-	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_put_sync_suspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(dev);
 	clk_unprepare(priv->refclk);
 	reset_control_assert(priv->rstc);
 	free_netdev(ndev);
@@ -3041,6 +3066,10 @@ static int ravb_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = pm_runtime_force_suspend(&priv->pdev->dev);
+	if (ret)
+		return ret;
+
 reset_assert:
 	return reset_control_assert(priv->rstc);
 }
@@ -3063,16 +3092,28 @@ static int ravb_resume(struct device *dev)
 		ret = ravb_wol_restore(ndev);
 		if (ret)
 			return ret;
+	} else {
+		ret = pm_runtime_force_resume(dev);
+		if (ret)
+			return ret;
 	}
 
 	/* Reopening the interface will restore the device to the working state. */
 	ret = ravb_open(ndev);
 	if (ret < 0)
-		return ret;
+		goto out_rpm_put;
 
 	ravb_set_rx_mode(ndev);
 	netif_device_attach(ndev);
 
+	return 0;
+
+out_rpm_put:
+	if (!priv->wol_enabled) {
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+	}
+
 	return ret;
 }
 
-- 
2.39.2

Re: [PATCH net-next v2 21/21] net: ravb: Add runtime PM support

[Sergey Shtylyov]

On 12/14/23 2:46 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Add runtime PM support for the ravb driver. As the driver is used by
> different IP variants, with different behaviors, to be able to have the
> runtime PM support available for all devices, the preparatory commits
> moved all the resources parsing and allocations in the driver's probe
> function and kept the settings for ravb_open(). This is due to the fact
> that on some IP variants-platforms tuples disabling/enabling the clocks
> will switch the IP to the reset operation mode where registers' content is

   The register contents.

> lost and reconfiguration needs to be done. For this the rabv_open()
> function enables the clocks, switches the IP to configuration mode, applies
> all the registers settings and switches the IP to the operational mode. At
> the end of ravb_open() IP is ready to send/receive data.
> 
> In ravb_close() necessary reverts are done (compared with ravb_open()), the
> IP is switched to reset mode and clocks are disabled.
> 
> The ethtool APIs or IOCTLs that might execute while the interface is down
> are either cached (and applied in ravb_open()) or rejected (as at that time
> the IP is in reset mode). Keeping the IP in the reset mode also increases
> the power saved (according to the hardware manual).
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 9ff943dff522..0733b63ff910 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1839,16 +1839,21 @@ static int ravb_open(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
> +	struct device *dev = &priv->pdev->dev;
>  	int error;
>  
>  	napi_enable(&priv->napi[RAVB_BE]);
>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  
> +	error = pm_runtime_resume_and_get(dev);
> +	if (error < 0)
> +		goto out_napi_off;
> +

   Note that sh_eth.c does this before enabling NAPI...

[...]

MBR, Sergey

Re: [PATCH net-next v2 00/21] net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S

[claudiu beznea]

On 14.12.2023 13:45, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Hi,
> 
> This series adds suspend to RAM and runtime PM support for Ethernet
> IP available on the RZ/G3S (R9A08G045) SoC.
> 
> As there are IP versions that switch to module standby when disabling
> the clocks, and because of module standby IP switches to reset and
> the register content is lost, to be able to have runtime PM supported
> for all IP variants, the configuration operations were moved all to
> ravb_open()/ravb_close() letting the ravb_probe() and ravb_remove()
> to deal with resource parsing and allocation/free.
> 
> The ethtool and IOCTL APIs that could have been run asyncronously
> were adapted to return if the interface is down. As explained in
> each individual commits description, this should be harmless.
> 
> Along with it, the series contains preparatory cleanups.
> 
> The series has been tested on the boards with the following device trees:
> - r8a7742-iwg21d-q7.dts
> - r8a774a1-hihope-rzg2m-ex.dts 
> - r9a07g043u11-smarc-rzg2ul.dts
> - r9a07g054l2-smarc-rzv2l.dts
> - r9a07g044l2-smarc-rzg2l.dts

And also (I forgot to mention):
- r9a08g045s33-smarc.dts (RZ/G3S).

> 
> Patches are based on series at [1].
> 
> Thank you,
> Claudiu Beznea
> 
> Changes in v2:
> - rework the driver (mainly, ravb_open() contains now only resource
>   allocation and parsing leaving the settings to ravb_open(); ravb_remove()
>   has been adapted accordingly) to be able to use runtime PM for all
>   IP variants; due to this number of patches increased
> - adjust previous series to review comments
> - collected tags
> - populated driver's own runtime PM ops with enable/disable of reference
>   clock
> 
> [1] https://lore.kernel.org/all/20231214113137.2450292-1-claudiu.beznea.uj@bp.renesas.com/
> 
> Claudiu Beznea (21):
>   net: ravb: Let IP-specific receive function to interrogate descriptors
>   net: ravb: Rely on PM domain to enable gptp_clk
>   net: ravb: Make reset controller support mandatory
>   net: ravb: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and
>     pm_ptr()
>   net: ravb: Use tabs instead of spaces
>   net: ravb: Assert/de-assert reset on suspend/resume
>   net: ravb: Move reference clock enable/disable on runtime PM APIs
>   net: ravb: Move the IRQs get and request in the probe function
>   net: ravb: Split GTI computation and set operations
>   net: ravb: Move delay mode set in the driver's ndo_open API
>   net: ravb: Move DBAT configuration to the driver's ndo_open API
>   net: ravb: Move ptp initialization in the driver's ndo_open API for
>     ccc_gac platorms
>   net: ravb: Set config mode in ndo_open and reset mode in ndo_close
>   net: ravb: Simplify ravb_suspend()
>   net: ravb: Simplify ravb_resume()
>   net: ravb: Keep the reverse order of operations in ravb_close()
>   net: ravb: Keep clock request operations grouped together
>   net: ravb: Return cached statistics if the interface is down
>   net: ravb: Do not set promiscuous mode if the interface is down
>   net: ravb: Do not apply RX CSUM settings to hardware if the interface
>     is down
>   net: ravb: Add runtime PM support
> 
>  drivers/net/ethernet/renesas/ravb.h      |   2 +
>  drivers/net/ethernet/renesas/ravb_main.c | 783 ++++++++++++-----------
>  2 files changed, 417 insertions(+), 368 deletions(-)
> 

Re: [PATCH net-next v2 00/21] net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S

[Jakub Kicinski]

On Thu, 14 Dec 2023 13:45:39 +0200 Claudiu wrote:
> Subject: [PATCH net-next v2 00/21] 

We got 260 patches in the review queue. Please pace your patches:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#tl-dr

> Patches are based on series at [1].
>
> [1] https://lore.kernel.org/all/20231214113137.2450292-1-claudiu.beznea.uj@bp.renesas.com/

Meaning there's a dependency we're supposed to track?
You have to wait for fixes to land, we marge the trees every week.
-- 
pw-bot: cr

Re: [PATCH net-next v2 00/21] net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S

[claudiu beznea]

On 14.12.2023 21:26, Jakub Kicinski wrote:
> On Thu, 14 Dec 2023 13:45:39 +0200 Claudiu wrote:
>> Subject: [PATCH net-next v2 00/21] 
> 
> We got 260 patches in the review queue. Please pace your patches:
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#tl-dr
> 
>> Patches are based on series at [1].
>>
>> [1] https://lore.kernel.org/all/20231214113137.2450292-1-claudiu.beznea.uj@bp.renesas.com/
> 
> Meaning there's a dependency we're supposed to track?

The intention was to have a review on this series (from driver's
maintainers) while the fixes are integrated, if any.

> You have to wait for fixes to land, we marge the trees every week.

The intention was to let the reviewers know they should apply [1] (if any)
for reviewing this series.

Sorry for any inconvenience,
Claudiu Beznea

Re: [PATCH net-next v2 00/21] net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S

[Jakub Kicinski]

On Fri, 15 Dec 2023 11:44:32 +0200 claudiu beznea wrote:
> > You have to wait for fixes to land, we marge the trees every week.  
> 
> The intention was to let the reviewers know they should apply [1] (if any)
> for reviewing this series.

If there's a dependency please post the "later" thing as RFC.
We can't apply it, and it saves us clicking it away in patchwork.