diff mbox series

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

Message ID 20231214114600.2451162-14-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand

Commit Message

Claudiu Beznea Dec. 14, 2023, 11:45 a.m. UTC 
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(-)

Comments

Sergey Shtylyov Dec. 16, 2023, 5:28 p.m. UTC | #1 
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
Claudiu Beznea Dec. 17, 2023, 1:15 p.m. UTC | #2 
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
Sergey Shtylyov Dec. 21, 2023, 7:40 p.m. UTC | #3 
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
diff mbox series

Patch

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);