[PATCH 00/13] net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S

[PATCH 00/13] 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 RZ/G3S (R9A08G045) SoC.

Along with it series contains preparatory fixes and cleanups.

Thank you,
Claudiu Beznea

Claudiu Beznea (13):
  net: ravb: Check return value of reset_control_deassert()
  net: ravb: Use pm_runtime_resume_and_get()
  net: ravb: Make write access to CXR35 first before accessing other
    EMAC registers
  net: ravb: Start TX queues after HW initialization succeeded
  net: ravb: Stop DMA in case of failures on ravb_open()
  net: ravb: Let IP specific receive function to interrogate descriptors
  net: ravb: Rely on PM domain to enable gptp_clk
  net: ravb: Rely on PM domain to enable refclk
  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/deassert reset on suspend/resume
  net: ravb: Add runtime PM support

 drivers/net/ethernet/renesas/ravb.h      |   2 +
 drivers/net/ethernet/renesas/ravb_main.c | 220 ++++++++++++++++-------
 2 files changed, 160 insertions(+), 62 deletions(-)

-- 
2.39.2

[PATCH 01/13] net: ravb: Check return value of reset_control_deassert()

[Claudiu]

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

reset_control_deassert() could return an error. Some devices cannot work
if reset signal de-assert operation fails. To avoid this check the return
code of reset_control_deassert() in ravb_probe() and take proper action.

Fixes: 0d13a1a464a0 ("ravb: Add reset support")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index c70cff80cc99..342978bdbd7e 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2645,7 +2645,12 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->features = info->net_features;
 	ndev->hw_features = info->net_hw_features;
 
-	reset_control_deassert(rstc);
+	error = reset_control_deassert(rstc);
+	if (error) {
+		free_netdev(ndev);
+		return error;
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-- 
2.39.2

Re: [PATCH 01/13] net: ravb: Check return value of reset_control_deassert()

[Sergey Shtylyov]

On 11/20/23 11:45 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> reset_control_deassert() could return an error. Some devices cannot work
> if reset signal de-assert operation fails. To avoid this check the return
> code of reset_control_deassert() in ravb_probe() and take proper action.
> 
> Fixes: 0d13a1a464a0 ("ravb: Add reset support")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c70cff80cc99..342978bdbd7e 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2645,7 +2645,12 @@ static int ravb_probe(struct platform_device *pdev)
>  	ndev->features = info->net_features;
>  	ndev->hw_features = info->net_hw_features;
>  
> -	reset_control_deassert(rstc);
> +	error = reset_control_deassert(rstc);
> +	if (error) {
> +		free_netdev(ndev);
> +		return error;

  No, please use *goto* here. And please fix up the order of statements under
the out_release label before doing that.

[...]

MBR, Sergey

Re: [PATCH 01/13] net: ravb: Check return value of reset_control_deassert()

[claudiu beznea]

On 20.11.2023 21:03, Sergey Shtylyov wrote:
> On 11/20/23 11:45 AM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> reset_control_deassert() could return an error. Some devices cannot work
>> if reset signal de-assert operation fails. To avoid this check the return
>> code of reset_control_deassert() in ravb_probe() and take proper action.
>>
>> Fixes: 0d13a1a464a0 ("ravb: Add reset support")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index c70cff80cc99..342978bdbd7e 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2645,7 +2645,12 @@ static int ravb_probe(struct platform_device *pdev)
>>  	ndev->features = info->net_features;
>>  	ndev->hw_features = info->net_hw_features;
>>  
>> -	reset_control_deassert(rstc);
>> +	error = reset_control_deassert(rstc);
>> +	if (error) {
>> +		free_netdev(ndev);
>> +		return error;
> 
>   No, please use *goto* here. And please fix up the order of statements under
> the out_release label before doing that.

This will lead to a bit more complicated patch, AFAICT. I tried to keep it
simple for a fix. Same thing for patch 2.

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

Re: [PATCH 01/13] net: ravb: Check return value of reset_control_deassert()

[Sergey Shtylyov]

On 11/21/23 8:59 AM, claudiu beznea wrote:

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> reset_control_deassert() could return an error. Some devices cannot work
>>> if reset signal de-assert operation fails. To avoid this check the return
>>> code of reset_control_deassert() in ravb_probe() and take proper action.
>>>
>>> Fixes: 0d13a1a464a0 ("ravb: Add reset support")
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb_main.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index c70cff80cc99..342978bdbd7e 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2645,7 +2645,12 @@ static int ravb_probe(struct platform_device *pdev)
>>>  	ndev->features = info->net_features;
>>>  	ndev->hw_features = info->net_hw_features;
>>>  
>>> -	reset_control_deassert(rstc);
>>> +	error = reset_control_deassert(rstc);
>>> +	if (error) {
>>> +		free_netdev(ndev);
>>> +		return error;
>>
>>   No, please use *goto* here. And please fix up the order of statements under
>> the out_release label before doing that.
> 
> This will lead to a bit more complicated patch, AFAICT. I tried to keep it

   It's OK! :-)

> simple for a fix. Same thing for patch 2.

   Patch 2 is not as simple as it could have been...

[...]

MBR, Sergey

[PATCH 02/13] net: ravb: Use pm_runtime_resume_and_get()

[Claudiu]

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

pm_runtime_get_sync() may return with error. In case it returns with error
dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
takes care of this. Thus use it.

Along with this pm_runtime_resume_and_get() and reset_control_deassert()
were moved before alloc_etherdev_mqs() to simplify the error path.

Also, in case pm_runtime_resume_and_get() returns error the reset signal
is deasserted and runtime PM is disabled (by jumping to the proper
error handling label).

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 28 +++++++++++++-----------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 342978bdbd7e..0486add302b3 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2635,25 +2635,26 @@ static int ravb_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(rstc),
 				     "failed to get cpg reset\n");
 
+	error = reset_control_deassert(rstc);
+	if (error)
+		return error;
+
+	pm_runtime_enable(&pdev->dev);
+	error = pm_runtime_resume_and_get(&pdev->dev);
+	if (error < 0)
+		goto pm_runtime_disable;
+
 	ndev = alloc_etherdev_mqs(sizeof(struct ravb_private),
 				  NUM_TX_QUEUE, NUM_RX_QUEUE);
-	if (!ndev)
-		return -ENOMEM;
-
+	if (!ndev) {
+		error = -ENOMEM;
+		goto pm_runtime_put;
+	}
 	info = of_device_get_match_data(&pdev->dev);
 
 	ndev->features = info->net_features;
 	ndev->hw_features = info->net_hw_features;
 
-	error = reset_control_deassert(rstc);
-	if (error) {
-		free_netdev(ndev);
-		return error;
-	}
-
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
-
 	if (info->multi_irqs) {
 		if (info->err_mgmt_irqs)
 			irq = platform_get_irq_byname(pdev, "dia");
@@ -2878,8 +2879,9 @@ static int ravb_probe(struct platform_device *pdev)
 	clk_disable_unprepare(priv->refclk);
 out_release:
 	free_netdev(ndev);
-
+pm_runtime_put:
 	pm_runtime_put(&pdev->dev);
+pm_runtime_disable:
 	pm_runtime_disable(&pdev->dev);
 	reset_control_assert(rstc);
 	return error;
-- 
2.39.2

Re: [PATCH 02/13] net: ravb: Use pm_runtime_resume_and_get()

[Sergey Shtylyov]

On 11/20/23 11:45 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> pm_runtime_get_sync() may return with error. In case it returns with error
> dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
> takes care of this. Thus use it.
> 
> Along with this pm_runtime_resume_and_get() and reset_control_deassert()
> were moved before alloc_etherdev_mqs() to simplify the error path.

   I don't see how it simplifies the error path...
   Re-ordering the statements at the end of the error path seems cheaper than
what you do.

> Also, in case pm_runtime_resume_and_get() returns error the reset signal
> is deasserted and runtime PM is disabled (by jumping to the proper
> error handling label).
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

MBR, Sergey

Re: [PATCH 02/13] net: ravb: Use pm_runtime_resume_and_get()

[claudiu beznea]

On 20.11.2023 21:23, Sergey Shtylyov wrote:
> On 11/20/23 11:45 AM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> pm_runtime_get_sync() may return with error. In case it returns with error
>> dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
>> takes care of this. Thus use it.
>>
>> Along with this pm_runtime_resume_and_get() and reset_control_deassert()
>> were moved before alloc_etherdev_mqs() to simplify the error path.
> 
>    I don't see how it simplifies the error path...

By not changing it... Actually, I took the other approach: you suggested in
patch 1 to re-arrange the error path, I did it the other way around:
changed the initialization path...

>    Re-ordering the statements at the end of the error path seems cheaper than
> what you do.
> 
>> Also, in case pm_runtime_resume_and_get() returns error the reset signal
>> is deasserted and runtime PM is disabled (by jumping to the proper
>> error handling label).
>>
>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> [...]
> 
> MBR, Sergey

Re: [PATCH 02/13] net: ravb: Use pm_runtime_resume_and_get()

[Sergey Shtylyov]

On 11/21/23 8:57 AM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> pm_runtime_get_sync() may return with error. In case it returns with error
>>> dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
>>> takes care of this. Thus use it.
>>>
>>> Along with this pm_runtime_resume_and_get() and reset_control_deassert()
>>> were moved before alloc_etherdev_mqs() to simplify the error path.
>>
>>    I don't see how it simplifies the error path...
> 
> By not changing it... Actually, I took the other approach: you suggested in

   But it does need to be changed! It's not currently in the reverse order
compared to the buildup path...

> patch 1 to re-arrange the error path, I did it the other way around:
> changed the initialization path...

   That way you needlessly obfuscate (by moving the code around) the core
change you do in this patch: switching from calling pm_runtime_get_sync()
to calling pm_runtime_resume_and_get(). :-/

[...]

>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> [...]

MBR, Sergey

[PATCH 03/13] net: ravb: Make write access to CXR35 first before accessing other EMAC registers

[Claudiu]

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

Hardware manual of RZ/G3S (and RZ/G2L) specifies the following on the
description of CXR35 register (chapter "PHY interface select register
(CXR35)"): "After release reset, make write-access to this register before
making write-access to other registers (except MDIOMOD). Even if not need
to change the value of this register, make write-access to this register
at least one time. Because RGMII/MII MODE is recognized by accessing this
register".

The setup procedure for EMAC module (chapter "Setup procedure" of RZ/G3S,
RZ/G2L manuals) specifies the E-MAC.CXR35 register is the first EMAC
register that is to be configured.

Note [A] from chapter "PHY interface select register (CXR35)" specifies
the following:
[A] The case which CXR35 SEL_XMII is used for the selection of RGMII/MII
in APB Clock 100 MHz.
(1) To use RGMII interface, Set ‘H’03E8_0000’ to this register.
(2) To use MII interface, Set ‘H’03E8_0002’ to this register.

Take into account these indication.

Fixes: 1089877ada8d ("ravb: Add RZ/G2L MII interface support")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 0486add302b3..d798a7109a09 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -515,6 +515,15 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 
+	if (priv->phy_interface == PHY_INTERFACE_MODE_MII) {
+		ravb_write(ndev, (1000 << 16) | CXR35_SEL_XMII_MII, CXR35);
+		ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, 0);
+	} else {
+		ravb_write(ndev, (1000 << 16) | CXR35_SEL_XMII_RGMII, CXR35);
+		ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1,
+			    CXR31_SEL_LINK0);
+	}
+
 	/* Receive frame limit set register */
 	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
 
@@ -537,14 +546,6 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 
 	/* E-MAC interrupt enable register */
 	ravb_write(ndev, ECSIPR_ICDIP, ECSIPR);
-
-	if (priv->phy_interface == PHY_INTERFACE_MODE_MII) {
-		ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, 0);
-		ravb_write(ndev, (1000 << 16) | CXR35_SEL_XMII_MII, CXR35);
-	} else {
-		ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1,
-			    CXR31_SEL_LINK0);
-	}
 }
 
 static void ravb_emac_init_rcar(struct net_device *ndev)
-- 
2.39.2

Re: [PATCH 03/13] net: ravb: Make write access to CXR35 first before accessing other EMAC registers

[Sergey Shtylyov]

On 11/20/23 11:45 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Hardware manual of RZ/G3S (and RZ/G2L) specifies the following on the
> description of CXR35 register (chapter "PHY interface select register
> (CXR35)"): "After release reset, make write-access to this register before
> making write-access to other registers (except MDIOMOD). Even if not need
> to change the value of this register, make write-access to this register
> at least one time. Because RGMII/MII MODE is recognized by accessing this
> register".
> 
> The setup procedure for EMAC module (chapter "Setup procedure" of RZ/G3S,
> RZ/G2L manuals) specifies the E-MAC.CXR35 register is the first EMAC
> register that is to be configured.
> 
> Note [A] from chapter "PHY interface select register (CXR35)" specifies
> the following:
> [A] The case which CXR35 SEL_XMII is used for the selection of RGMII/MII
> in APB Clock 100 MHz.
> (1) To use RGMII interface, Set ‘H’03E8_0000’ to this register.
> (2) To use MII interface, Set ‘H’03E8_0002’ to this register.
> 
> Take into account these indication.
> 
> Fixes: 1089877ada8d ("ravb: Add RZ/G2L MII interface support")

   The bug fixes should be submitted separately and against the net.git repo...

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

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

[...]

MBR, Sergey

Re: [PATCH 03/13] net: ravb: Make write access to CXR35 first before accessing other EMAC registers

[claudiu beznea]

On 20.11.2023 21:44, Sergey Shtylyov wrote:
> On 11/20/23 11:45 AM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Hardware manual of RZ/G3S (and RZ/G2L) specifies the following on the
>> description of CXR35 register (chapter "PHY interface select register
>> (CXR35)"): "After release reset, make write-access to this register before
>> making write-access to other registers (except MDIOMOD). Even if not need
>> to change the value of this register, make write-access to this register
>> at least one time. Because RGMII/MII MODE is recognized by accessing this
>> register".
>>
>> The setup procedure for EMAC module (chapter "Setup procedure" of RZ/G3S,
>> RZ/G2L manuals) specifies the E-MAC.CXR35 register is the first EMAC
>> register that is to be configured.
>>
>> Note [A] from chapter "PHY interface select register (CXR35)" specifies
>> the following:
>> [A] The case which CXR35 SEL_XMII is used for the selection of RGMII/MII
>> in APB Clock 100 MHz.
>> (1) To use RGMII interface, Set ‘H’03E8_0000’ to this register.
>> (2) To use MII interface, Set ‘H’03E8_0002’ to this register.
>>
>> Take into account these indication.
>>
>> Fixes: 1089877ada8d ("ravb: Add RZ/G2L MII interface support")
> 
>    The bug fixes should be submitted separately and against the net.git repo...

OK, thanks for pointing it.

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

Re: [PATCH 03/13] net: ravb: Make write access to CXR35 first before accessing other EMAC registers

[Sergey Shtylyov]

On 11/21/23 9:02 AM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Hardware manual of RZ/G3S (and RZ/G2L) specifies the following on the
>>> description of CXR35 register (chapter "PHY interface select register
>>> (CXR35)"): "After release reset, make write-access to this register before
>>> making write-access to other registers (except MDIOMOD). Even if not need
>>> to change the value of this register, make write-access to this register
>>> at least one time. Because RGMII/MII MODE is recognized by accessing this
>>> register".
>>>
>>> The setup procedure for EMAC module (chapter "Setup procedure" of RZ/G3S,
>>> RZ/G2L manuals) specifies the E-MAC.CXR35 register is the first EMAC
>>> register that is to be configured.
>>>
>>> Note [A] from chapter "PHY interface select register (CXR35)" specifies
>>> the following:
>>> [A] The case which CXR35 SEL_XMII is used for the selection of RGMII/MII
>>> in APB Clock 100 MHz.
>>> (1) To use RGMII interface, Set ‘H’03E8_0000’ to this register.
>>> (2) To use MII interface, Set ‘H’03E8_0002’ to this register.
>>>
>>> Take into account these indication.
>>>
>>> Fixes: 1089877ada8d ("ravb: Add RZ/G2L MII interface support")
>>
>>    The bug fixes should be submitted separately and against the net.git repo...
> 
> OK, thanks for pointing it.

   And I think Linus' repo will do as well...

MBR, Sergey

[PATCH 04/13] net: ravb: Start TX queues after HW initialization succeeded

[Claudiu]

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

ravb_phy_start() may fail. If that happens the TX queues remain started.
Thus move the netif_tx_start_all_queues() after PHY is successfully
initialized.

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index d798a7109a09..b7e9035cb989 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1812,13 +1812,13 @@ static int ravb_open(struct net_device *ndev)
 	if (info->gptp)
 		ravb_ptp_init(ndev, priv->pdev);
 
-	netif_tx_start_all_queues(ndev);
-
 	/* PHY control start */
 	error = ravb_phy_start(ndev);
 	if (error)
 		goto out_ptp_stop;
 
+	netif_tx_start_all_queues(ndev);
+
 	return 0;
 
 out_ptp_stop:
-- 
2.39.2

Re: [PATCH 04/13] net: ravb: Start TX queues after HW initialization succeeded

[Sergey Shtylyov]

On 11/20/23 11:45 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> ravb_phy_start() may fail. If that happens the TX queues remain started.
> Thus move the netif_tx_start_all_queues() after PHY is successfully
> initialized.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

[...]

MBR, Sergey

[PATCH 05/13] net: ravb: Stop DMA in case of failures on ravb_open()

[Claudiu]

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

In case ravb_phy_start() returns with error the settings applied in
ravb_dma_init() are not reverted (e.g. config mode). For this call
ravb_stop_dma() on failure path of ravb_open().

Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index b7e9035cb989..588e3be692d3 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1825,6 +1825,7 @@ static int ravb_open(struct net_device *ndev)
 	/* Stop PTP Clock driver */
 	if (info->gptp)
 		ravb_ptp_stop(ndev);
+	ravb_stop_dma(ndev);
 out_free_irq_mgmta:
 	if (!info->multi_irqs)
 		goto out_free_irq;
-- 
2.39.2

Re: [PATCH 05/13] net: ravb: Stop DMA in case of failures on ravb_open()

[Sergey Shtylyov]

On 11/20/23 11:45 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> In case ravb_phy_start() returns with error the settings applied in
> ravb_dma_init() are not reverted (e.g. config mode). For this call

   It's called ravb_dmac_init().

> ravb_stop_dma() on failure path of ravb_open().
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

[...]

MBR, Sergey

[PATCH 06/13] 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 know if ravb_rx() should be called.
This is done for non GPTP IPs. For GPTP IPs the driver PTP specific
information was used to know if receive function should be called. As
every IP has it's own receive function that interrogates the RX descriptor
list in the same way the ravb_poll() was doing there is no need to double
check this in ravb_poll(). Removing the code form ravb_poll() and
adjusting ravb_rx_gbeth() leads to a cleaner code.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 588e3be692d3..0fc9810c5e78 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 	int limit;
 
 	entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+	desc = &priv->gbeth_rx_ring[entry];
+	if (desc->die_dt == DT_FEMPTY)
+		return false;
+
 	boguscnt = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
 	stats = &priv->stats[q];
 
 	boguscnt = min(boguscnt, *quota);
 	limit = boguscnt;
-	desc = &priv->gbeth_rx_ring[entry];
 	while (desc->die_dt != DT_FEMPTY) {
 		/* Descriptor type must be checked before all other reads */
 		dma_rmb();
@@ -1279,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 06/13] net: ravb: Let IP specific receive function to interrogate descriptors

[Sergey Shtylyov]

On 11/20/23 11:45 AM, 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 know if ravb_rx() should be called.
> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific

   It's called gPTP, AFAIK.

> information was used to know if receive function should be called. As
> every IP has it's own receive function that interrogates the RX descriptor

   Its own.

> list in the same way the ravb_poll() was doing there is no need to double
> check this in ravb_poll(). Removing the code form ravb_poll() and

   From ravb_poll().

> adjusting ravb_rx_gbeth() leads to a cleaner code.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 588e3be692d3..0fc9810c5e78 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  	int limit;
>  
>  	entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> +	desc = &priv->gbeth_rx_ring[entry];
> +	if (desc->die_dt == DT_FEMPTY)
> +		return false;
> +

   I don't understand what this buys us, the following *while* loop will
be skipped anyway, and the *for* loop as well, I think... And ravb_rx_rcar()
doesn't have that anyway...

> @@ -1279,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;
> -	}

  I don't really understand this code (despite I've AKCed it)...
Biju, could you explain this (well, you tried already but I don't
buy it anymore)?

> +	if (ravb_rx(ndev, &quota, q))
> +		goto out;

   This restores the behavior before:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3d4e37df882b0f4f28b7223a42492650b71252c5

   So does look correct. :-)

MBR, Sergey

RE: [PATCH 06/13] net: ravb: Let IP specific receive function to interrogate descriptors

[Biju Das]

Hi Sergey Shtylyov,

> Subject: Re: [PATCH 06/13] net: ravb: Let IP specific receive function to
> interrogate descriptors
>
> On 11/20/23 11:45 AM, 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 know if ravb_rx() should be
> called.
> > This is done for non GPTP IPs. For GPTP IPs the driver PTP specific
>
>    It's called gPTP, AFAIK.
>
> > information was used to know if receive function should be called. As
> > every IP has it's own receive function that interrogates the RX
> > descriptor
>
>    Its own.
>
> > list in the same way the ravb_poll() was doing there is no need to
> > double check this in ravb_poll(). Removing the code form ravb_poll()
> > and
>
>    From ravb_poll().
>
> > adjusting ravb_rx_gbeth() leads to a cleaner code.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------
> >  1 file changed, 6 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 588e3be692d3..0fc9810c5e78 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev,
> int *quota, int q)
> >     int limit;
> >
> >     entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> > +   desc = &priv->gbeth_rx_ring[entry];
> > +   if (desc->die_dt == DT_FEMPTY)
> > +           return false;
> > +
>
>    I don't understand what this buys us, the following *while* loop will
> be skipped anyway, and the *for* loop as well, I think... And
> ravb_rx_rcar() doesn't have that anyway...
>
> > @@ -1279,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;
> > -   }
>
>   I don't really understand this code (despite I've AKCed it)...
> Biju, could you explain this (well, you tried already but I don't buy it
> anymore)?

It was initial version of the driver. Now Claudiu optimized.

Cheers,
Biju

>
> > +   if (ravb_rx(ndev, &quota, q))
> > +           goto out;
>
>    This restores the behavior before:
>
> https://git.kern/
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%
> 2F%3Fid%3D3d4e37df882b0f4f28b7223a42492650b71252c5&data=05%7C01%7Cbiju.das
> .jz%40bp.renesas.com%7C3c7d64ca68104738fb3f08dbec427e84%7C53d82571da1947e4
> 9cb4625a166a4a2a%7C0%7C0%7C638363542555838396%7CUnknown%7CTWFpbGZsb3d8eyJW
> IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&sdata=lGBD8FdwY26OygYAV4Sd8bzIIO4rS7rNiYabQKaQAtA%3D&reserved=0
>
>    So does look correct. :-)
>
> MBR, Sergey

Re: [PATCH 06/13] net: ravb: Let IP specific receive function to interrogate descriptors

[Sergey Shtylyov]

On 11/23/23 7:48 PM, Biju Das 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 know if ravb_rx() should be
>> called.
>>> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific
>>
>>    It's called gPTP, AFAIK.
>>
>>> information was used to know if receive function should be called. As
>>> every IP has it's own receive function that interrogates the RX
>>> descriptor
>>
>>    Its own.
>>
>>> list in the same way the ravb_poll() was doing there is no need to
>>> double check this in ravb_poll(). Removing the code form ravb_poll()
>>> and
>>
>>    From ravb_poll().
>>
>>> adjusting ravb_rx_gbeth() leads to a cleaner code.
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------
>>>  1 file changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 588e3be692d3..0fc9810c5e78 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev,
>> int *quota, int q)
>>>     int limit;
>>>
>>>     entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>>> +   desc = &priv->gbeth_rx_ring[entry];
>>> +   if (desc->die_dt == DT_FEMPTY)
>>> +           return false;
>>> +
>>
>>    I don't understand what this buys us, the following *while* loop will
>> be skipped anyway, and the *for* loop as well, I think... And
>> ravb_rx_rcar() doesn't have that anyway...
>>
>>> @@ -1279,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;
>>> -   }
>>
>>   I don't really understand this code (despite I've AKCed it)...
>> Biju, could you explain this (well, you tried already but I don't buy it
>> anymore)?
> 
> It was initial version of the driver. Now Claudiu optimized.

   I fail to understand why you had (GBEth-specific) pre-conditions here to call
ravb_rx()... and involving gPTP only further confused things...

> Cheers,
> Biju

[...]

MBR, Sergey

RE: [PATCH 06/13] net: ravb: Let IP specific receive function to interrogate descriptors

[Biju Das]

Hi Sergey Shtylyov,

> Subject: Re: [PATCH 06/13] net: ravb: Let IP specific receive function to
> interrogate descriptors
> 
> On 11/23/23 7:48 PM, Biju Das 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 know if ravb_rx() should be
> >> called.
> >>> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific
> >>
> >>    It's called gPTP, AFAIK.
> >>
> >>> information was used to know if receive function should be called.
> >>> As every IP has it's own receive function that interrogates the RX
> >>> descriptor
> >>
> >>    Its own.
> >>
> >>> list in the same way the ravb_poll() was doing there is no need to
> >>> double check this in ravb_poll(). Removing the code form ravb_poll()
> >>> and
> >>
> >>    From ravb_poll().
> >>
> >>> adjusting ravb_rx_gbeth() leads to a cleaner code.
> >>>
> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>> ---
> >>>  drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------
> >>>  1 file changed, 6 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>> index 588e3be692d3..0fc9810c5e78 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device
> >>> *ndev,
> >> int *quota, int q)
> >>>     int limit;
> >>>
> >>>     entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >>> +   desc = &priv->gbeth_rx_ring[entry];
> >>> +   if (desc->die_dt == DT_FEMPTY)
> >>> +           return false;
> >>> +
> >>
> >>    I don't understand what this buys us, the following *while* loop
> >> will be skipped anyway, and the *for* loop as well, I think... And
> >> ravb_rx_rcar() doesn't have that anyway...
> >>
> >>> @@ -1279,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;
> >>> -   }
> >>
> >>   I don't really understand this code (despite I've AKCed it)...
> >> Biju, could you explain this (well, you tried already but I don't buy
> >> it anymore)?
> >
> > It was initial version of the driver. Now Claudiu optimized.
> 
>    I fail to understand why you had (GBEth-specific) pre-conditions here
> to call ravb_rx()... and involving gPTP only further confused things...

Initial driver is based on a reference code/bsp driver and that code has this preconditions.
Maybe they have faced some race condition in rx path involving ring buffer/descriptor.
But so far we are not able to reproduce any race condition here. So it is safe to remove now.

Cheers,
Biju

Re: [PATCH 06/13] net: ravb: Let IP specific receive function to interrogate descriptors

[claudiu beznea]

On 23.11.2023 18:37, Sergey Shtylyov wrote:
> On 11/20/23 11:45 AM, 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 know if ravb_rx() should be called.
>> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific
> 
>    It's called gPTP, AFAIK.
> 
>> information was used to know if receive function should be called. As
>> every IP has it's own receive function that interrogates the RX descriptor
> 
>    Its own.
> 
>> list in the same way the ravb_poll() was doing there is no need to double
>> check this in ravb_poll(). Removing the code form ravb_poll() and
> 
>    From ravb_poll().
> 
>> adjusting ravb_rx_gbeth() leads to a cleaner code.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------
>>  1 file changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 588e3be692d3..0fc9810c5e78 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>>  	int limit;
>>  
>>  	entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>> +	desc = &priv->gbeth_rx_ring[entry];
>> +	if (desc->die_dt == DT_FEMPTY)
>> +		return false;
>> +
> 
>    I don't understand what this buys us, the following *while* loop will
> be skipped anyway, and the *for* loop as well, I think... And ravb_rx_rcar()

Yes, I kept it because of the for loop and the update of the *quota.

As commit specifies the code has been moved in IP specific RX function
keeping the initial functionality.

> doesn't have that anyway...


> 
>> @@ -1279,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;
>> -	}
> 
>   I don't really understand this code (despite I've AKCed it)...
> Biju, could you explain this (well, you tried already but I don't
> buy it anymore)?
> 
>> +	if (ravb_rx(ndev, &quota, q))
>> +		goto out;
> 
>    This restores the behavior before:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3d4e37df882b0f4f28b7223a42492650b71252c5
> 
>    So does look correct. :-)
> 
> MBR, Sergey

Re: [PATCH 06/13] net: ravb: Let IP specific receive function to interrogate descriptors

[Sergey Shtylyov]

On 11/23/23 8:15 PM, claudiu beznea 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 know if ravb_rx() should be called.
>>> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific
>>
>>    It's called gPTP, AFAIK.
>>
>>> information was used to know if receive function should be called. As
>>> every IP has it's own receive function that interrogates the RX descriptor
>>
>>    Its own.
>>
>>> list in the same way the ravb_poll() was doing there is no need to double
>>> check this in ravb_poll(). Removing the code form ravb_poll() and
>>
>>    From ravb_poll().
>>
>>> adjusting ravb_rx_gbeth() leads to a cleaner code.
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------
>>>  1 file changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 588e3be692d3..0fc9810c5e78 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>>>  	int limit;
>>>  
>>>  	entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>>> +	desc = &priv->gbeth_rx_ring[entry];
>>> +	if (desc->die_dt == DT_FEMPTY)
>>> +		return false;
>>> +
>>
>>    I don't understand what this buys us, the following *while* loop will
>> be skipped anyway, and the *for* loop as well, I think... And ravb_rx_rcar()
> 
> Yes, I kept it because of the for loop and the update of the *quota.
> 
> As commit specifies the code has been moved in IP specific RX function
> keeping the initial functionality.

   Please pull this perplexed code out completely instead. It's not needed
according to Biju.

>> doesn't have that anyway...

[...]

MBR, Sergey

[PATCH 07/13] 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 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 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>
---
 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 0fc9810c5e78..836fdb4b3bfd 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2762,7 +2762,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);
@@ -2786,7 +2785,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);
@@ -2806,7 +2805,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;
@@ -2869,8 +2868,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:
@@ -2893,7 +2890,6 @@ static void ravb_remove(struct platform_device *pdev)
 	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
 
-	clk_disable_unprepare(priv->gptp_clk);
 	clk_disable_unprepare(priv->refclk);
 
 	/* Set reset mode */
-- 
2.39.2

Re: [PATCH 07/13] net: ravb: Rely on PM domain to enable gptp_clk

[Sergey Shtylyov]

On 11/20/23 11:46 AM, Claudiu wrote:

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

[...]

MBR, Sergey

[PATCH 08/13] net: ravb: Rely on PM domain to enable refclk

[Claudiu]

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

For RZ/G3S and RZ/G2L SoCs the Ethernet's reference clock is part of the
Ethernet's power domain. It is controlled though CPG driver that is
providing the support for power domain that Ethernet belongs. Thus,
to be able to implement runtime PM (at least for RZ/G3S at the moment)
w/o the need to add clock enable/disable specific calls in runtime PM
ops of ravb driver and interfere with other IP specific implementations,
add a new variable to struct_hw_info and enable the reference clock
based on the value of this variable (the variable states if reference
clock is part of the Ethernet's power domain).

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 19 ++++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index e0f8276cffed..c2d8d890031f 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1043,6 +1043,7 @@ struct ravb_hw_info {
 	unsigned nc_queues:1;		/* AVB-DMAC has RX and TX NC queues */
 	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
 	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
+	unsigned refclk_in_pd:1;	/* Reference clock is part of a power domain. */
 };
 
 struct ravb_private {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 836fdb4b3bfd..ddd8cd2c0f89 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2502,6 +2502,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.tx_counters = 1,
 	.carrier_counters = 1,
 	.half_duplex = 1,
+	.refclk_in_pd = 1,
 };
 
 static const struct of_device_id ravb_match_table[] = {
@@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev)
 		goto out_release;
 	}
 
-	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
-	if (IS_ERR(priv->refclk)) {
-		error = PTR_ERR(priv->refclk);
-		goto out_release;
+	if (!info->refclk_in_pd) {
+		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);
 	}
-	clk_prepare_enable(priv->refclk);
 
 	if (info->gptp_ref_clk) {
 		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
@@ -2869,7 +2872,8 @@ static int ravb_probe(struct platform_device *pdev)
 	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
 out_disable_refclk:
-	clk_disable_unprepare(priv->refclk);
+	if (!info->refclk_in_pd)
+		clk_disable_unprepare(priv->refclk);
 out_release:
 	free_netdev(ndev);
 pm_runtime_put:
@@ -2890,7 +2894,8 @@ static void ravb_remove(struct platform_device *pdev)
 	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
 
-	clk_disable_unprepare(priv->refclk);
+	if (!info->refclk_in_pd)
+		clk_disable_unprepare(priv->refclk);
 
 	/* Set reset mode */
 	ravb_write(ndev, CCC_OPC_RESET, CCC);
-- 
2.39.2

Re: [PATCH 08/13] net: ravb: Rely on PM domain to enable refclk

[Sergey Shtylyov]

On 11/20/23 11:46 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> For RZ/G3S and RZ/G2L SoCs the Ethernet's reference clock is part of the
> Ethernet's power domain. It is controlled though CPG driver that is
> providing the support for power domain that Ethernet belongs. Thus,
> to be able to implement runtime PM (at least for RZ/G3S at the moment)
> w/o the need to add clock enable/disable specific calls in runtime PM
> ops of ravb driver and interfere with other IP specific implementations,
> add a new variable to struct_hw_info and enable the reference clock
> based on the value of this variable (the variable states if reference
> clock is part of the Ethernet's power domain).
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

[...]

MBR, Sergey

Re: [PATCH 08/13] net: ravb: Rely on PM domain to enable refclk

[Geert Uytterhoeven]

Hi Claudiu,

Thanks for your patch (which seems to have been delayed by 3 days, ouch)!

On Thu, Nov 23, 2023 at 5:35 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> For RZ/G3S and RZ/G2L SoCs the Ethernet's reference clock is part of the
> Ethernet's power domain. It is controlled though CPG driver that is
> providing the support for power domain that Ethernet belongs. Thus,
> to be able to implement runtime PM (at least for RZ/G3S at the moment)

Why only for RZ/G3S?

> w/o the need to add clock enable/disable specific calls in runtime PM
> ops of ravb driver and interfere with other IP specific implementations,
> add a new variable to struct_hw_info and enable the reference clock
> based on the value of this variable (the variable states if reference
> clock is part of the Ethernet's power domain).
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1043,6 +1043,7 @@ struct ravb_hw_info {
>         unsigned nc_queues:1;           /* AVB-DMAC has RX and TX NC queues */
>         unsigned magic_pkt:1;           /* E-MAC supports magic packet detection */
>         unsigned half_duplex:1;         /* E-MAC supports half duplex mode */
> +       unsigned refclk_in_pd:1;        /* Reference clock is part of a power domain. */
>  };
>
>  struct ravb_private {
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 836fdb4b3bfd..ddd8cd2c0f89 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2502,6 +2502,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>         .tx_counters = 1,
>         .carrier_counters = 1,
>         .half_duplex = 1,
> +       .refclk_in_pd = 1,
>  };
>
>  static const struct of_device_id ravb_match_table[] = {
> @@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev)
>                 goto out_release;
>         }
>
> -       priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
> -       if (IS_ERR(priv->refclk)) {
> -               error = PTR_ERR(priv->refclk);
> -               goto out_release;
> +       if (!info->refclk_in_pd) {
> +               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);
>         }
> -       clk_prepare_enable(priv->refclk);

Is this patch really needed? It doesn't hurt to manually enable a
clock that is also under Runtime PM control.  Clock prepare/enable
refcounting will take care of that.

>
>         if (info->gptp_ref_clk) {
>                 priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
> @@ -2869,7 +2872,8 @@ static int ravb_probe(struct platform_device *pdev)
>         if (info->ccc_gac)
>                 ravb_ptp_stop(ndev);
>  out_disable_refclk:
> -       clk_disable_unprepare(priv->refclk);
> +       if (!info->refclk_in_pd)
> +               clk_disable_unprepare(priv->refclk);
>  out_release:
>         free_netdev(ndev);
>  pm_runtime_put:
> @@ -2890,7 +2894,8 @@ static void ravb_remove(struct platform_device *pdev)
>         if (info->ccc_gac)
>                 ravb_ptp_stop(ndev);
>
> -       clk_disable_unprepare(priv->refclk);
> +       if (!info->refclk_in_pd)
> +               clk_disable_unprepare(priv->refclk);
>
>         /* Set reset mode */
>         ravb_write(ndev, CCC_OPC_RESET, CCC);

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 08/13] net: ravb: Rely on PM domain to enable refclk

[claudiu beznea]

Hi, Geert,

On 23.11.2023 10:48, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> Thanks for your patch (which seems to have been delayed by 3 days, ouch)!
> 
> On Thu, Nov 23, 2023 at 5:35 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> For RZ/G3S and RZ/G2L SoCs the Ethernet's reference clock is part of the
>> Ethernet's power domain. It is controlled though CPG driver that is
>> providing the support for power domain that Ethernet belongs. Thus,
>> to be able to implement runtime PM (at least for RZ/G3S at the moment)
> 
> Why only for RZ/G3S?

(I'm copy pasting here what I already replied to Sergey)

The reasons I've limited only to RZ/G3S are:
1/ I don't have all the platforms to test it
2/ on G1H this doesn't work. I tried to debugged it but I don't have a
   platform at hand, only remotely, and is hardly to debug once the
   ethernet fails to work: probe is working(), open is executed, PHY is
   initialized and then TX/RX is not working... don't know why ATM.

> 
>> w/o the need to add clock enable/disable specific calls in runtime PM
>> ops of ravb driver and interfere with other IP specific implementations,
>> add a new variable to struct_hw_info and enable the reference clock
>> based on the value of this variable (the variable states if reference
>> clock is part of the Ethernet's power domain).
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> @@ -1043,6 +1043,7 @@ struct ravb_hw_info {
>>         unsigned nc_queues:1;           /* AVB-DMAC has RX and TX NC queues */
>>         unsigned magic_pkt:1;           /* E-MAC supports magic packet detection */
>>         unsigned half_duplex:1;         /* E-MAC supports half duplex mode */
>> +       unsigned refclk_in_pd:1;        /* Reference clock is part of a power domain. */
>>  };
>>
>>  struct ravb_private {
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 836fdb4b3bfd..ddd8cd2c0f89 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2502,6 +2502,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>>         .tx_counters = 1,
>>         .carrier_counters = 1,
>>         .half_duplex = 1,
>> +       .refclk_in_pd = 1,
>>  };
>>
>>  static const struct of_device_id ravb_match_table[] = {
>> @@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev)
>>                 goto out_release;
>>         }
>>
>> -       priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
>> -       if (IS_ERR(priv->refclk)) {
>> -               error = PTR_ERR(priv->refclk);
>> -               goto out_release;
>> +       if (!info->refclk_in_pd) {
>> +               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);
>>         }
>> -       clk_prepare_enable(priv->refclk);
> 
> Is this patch really needed? It doesn't hurt to manually enable a
> clock that is also under Runtime PM control.  Clock prepare/enable
> refcounting will take care of that.

I agree with that. I chose this path to not interfere w/ the comments
ravb_runtime_nop() which I didn't understand. Also I fail to understand why
the ravb_runtime_nop() is there...

> 
>>
>>         if (info->gptp_ref_clk) {
>>                 priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
>> @@ -2869,7 +2872,8 @@ static int ravb_probe(struct platform_device *pdev)
>>         if (info->ccc_gac)
>>                 ravb_ptp_stop(ndev);
>>  out_disable_refclk:
>> -       clk_disable_unprepare(priv->refclk);
>> +       if (!info->refclk_in_pd)
>> +               clk_disable_unprepare(priv->refclk);
>>  out_release:
>>         free_netdev(ndev);
>>  pm_runtime_put:
>> @@ -2890,7 +2894,8 @@ static void ravb_remove(struct platform_device *pdev)
>>         if (info->ccc_gac)
>>                 ravb_ptp_stop(ndev);
>>
>> -       clk_disable_unprepare(priv->refclk);
>> +       if (!info->refclk_in_pd)
>> +               clk_disable_unprepare(priv->refclk);
>>
>>         /* Set reset mode */
>>         ravb_write(ndev, CCC_OPC_RESET, CCC);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH 08/13] net: ravb: Rely on PM domain to enable refclk

[Sergey Shtylyov]

On 11/23/23 8:10 PM, claudiu beznea wrote:
[...]

>> On Thu, Nov 23, 2023 at 5:35 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]
>>> w/o the need to add clock enable/disable specific calls in runtime PM
>>> ops of ravb driver and interfere with other IP specific implementations,
>>> add a new variable to struct_hw_info and enable the reference clock
>>> based on the value of this variable (the variable states if reference
>>> clock is part of the Ethernet's power domain).
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

[...]
>>>  static const struct of_device_id ravb_match_table[] = {
>>> @@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev)
>>>                 goto out_release;
>>>         }
>>>
>>> -       priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
>>> -       if (IS_ERR(priv->refclk)) {
>>> -               error = PTR_ERR(priv->refclk);
>>> -               goto out_release;
>>> +       if (!info->refclk_in_pd) {
>>> +               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);
>>>         }
>>> -       clk_prepare_enable(priv->refclk);
>>
>> Is this patch really needed? It doesn't hurt to manually enable a
>> clock that is also under Runtime PM control.  Clock prepare/enable
>> refcounting will take care of that.
> 
> I agree with that. I chose this path to not interfere w/ the comments
> ravb_runtime_nop() which I didn't understand. Also I fail to understand why
> the ravb_runtime_nop() is there...

   Looks like it was blindly copied from the sh_eth driver and doesn't (yet?)
apply to ravb...

[...]

MBR, Sergey

[PATCH 09/13] net: ravb: Make reset controller support mandatory

[Claudiu]

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

On RZ/G3S SoC the reset controller is mandatory for the IP to work.
The device tree binding documentation for 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>
---
 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 ddd8cd2c0f89..8874c48604c0 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2627,7 +2627,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

Re: [PATCH 09/13] net: ravb: Make reset controller support mandatory

[Sergey Shtylyov]

On 11/20/23 11:46 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> On RZ/G3S SoC the reset controller is mandatory for the IP to work.
> The device tree binding documentation for ravb driver specifies that the

   The bindings are not for the driver, they are for the device.

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

[...]

MBR, Sergey

Re: [PATCH 09/13] net: ravb: Make reset controller support mandatory

[Geert Uytterhoeven]

On Fri, Nov 24, 2023 at 7:20 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> On RZ/G3S SoC the reset controller is mandatory for the IP to work.
> The device tree binding documentation for 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: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 10/13] 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>
---
 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 8874c48604c0..15fc494a8b97 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2953,7 +2953,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);
@@ -2975,7 +2975,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);
@@ -3029,7 +3029,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.
@@ -3042,8 +3042,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 = {
@@ -3051,7 +3051,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

Re: [PATCH 10/13] net: ravb: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and pm_ptr()

[Sergey Shtylyov]

On 11/20/23 11:46 AM, Claudiu wrote:

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

[...]

MBR, Sergey

Re: [PATCH 10/13] net: ravb: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and pm_ptr()

[Geert Uytterhoeven]

On Thu, Nov 23, 2023 at 7:15 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> 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: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 11/13] net: ravb: Use tabs instead of spaces

[Claudiu]

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

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

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 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 15fc494a8b97..a93b3d6b1863 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

Re: [PATCH 11/13] net: ravb: Use tabs instead of spaces

[Sergey Shtylyov]

On 11/20/23 11:46 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Use tabs instead of spaces in 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>

[...]

MBR, Sergey

[PATCH 12/13] net: ravb: Assert/deassert 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 resumming from such state the Ethernet controller needs to be
reinitialized. Deasserting the reset signal for it should also be done.
Thus add reset assert/deassert on suspend/resume functions.

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

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index a93b3d6b1863..f4634ac0c972 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2972,7 +2972,10 @@ static int ravb_suspend(struct device *dev)
 	if (priv->info->ccc_gac)
 		ravb_ptp_stop(ndev);
 
-	return ret;
+	if (priv->wol_enabled)
+		return ret;
+
+	return reset_control_assert(priv->rstc);
 }
 
 static int ravb_resume(struct device *dev)
@@ -2980,7 +2983,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 12/13] net: ravb: Assert/deassert reset on suspend/resume

[Sergey Shtylyov]

On 11/20/23 11:46 AM, 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 resumming from such state the Ethernet controller needs to be

   Resuming.

> reinitialized. Deasserting the reset signal for it should also be done.
> Thus add reset assert/deassert on suspend/resume functions.

   Firefox' spell checker trips over deassert[ing] and you have |de-assert"
below:

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

   I'm not seeing us reverting anything on the resume failure...

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

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

[...]

MBR, Sergey

Re: [PATCH 12/13] net: ravb: Assert/deassert reset on suspend/resume

[Sergey Shtylyov]

On 11/21/23 10:13 PM, Sergey Shtylyov wrote:
[...]

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

>> reinitialized. Deasserting the reset signal for it should also be done.
>> Thus add reset assert/deassert on suspend/resume functions.
> 
>    Firefox' spell checker trips over deassert[ing] and you have |de-assert"

   s/Firefox/Thunderbird/. :-)

[...]

MBR, Sergey

[PATCH 13/13] net: ravb: Add runtime PM support

[Claudiu]

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

RZ/G3S supports enabling/disabling clocks for its modules (including
Ethernet module). For this commit adds runtime PM support which
relies on PM domain to enable/disable Ethernet clocks.

At the end of probe ravb_pm_runtime_put() is called which will turn
off the Ethernet clocks (if no other request arrives at the driver).
After that if the interface is brought up (though ravb_open()) then
the clocks remain enabled until interface is brought down (operation
done though ravb_close()).

If any request arrives to the driver while the interface is down the
clocks are enabled to serve the request and then disabled.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 99 ++++++++++++++++++++++--
 2 files changed, 93 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index c2d8d890031f..50f358472aab 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1044,6 +1044,7 @@ struct ravb_hw_info {
 	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
 	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
 	unsigned refclk_in_pd:1;	/* Reference clock is part of a power domain. */
+	unsigned rpm:1;			/* Runtime PM available. */
 };
 
 struct ravb_private {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index f4634ac0c972..d70ed7e5f7f6 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -145,12 +145,41 @@ static void ravb_read_mac_address(struct device_node *np,
 	}
 }
 
+static int ravb_pm_runtime_get(struct ravb_private *priv)
+{
+	const struct ravb_hw_info *info = priv->info;
+
+	if (!info->rpm)
+		return 0;
+
+	return pm_runtime_resume_and_get(&priv->pdev->dev);
+}
+
+static void ravb_pm_runtime_put(struct ravb_private *priv)
+{
+	const struct ravb_hw_info *info = priv->info;
+	struct device *dev = &priv->pdev->dev;
+
+	if (!info->rpm)
+		return;
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+}
+
 static void ravb_mdio_ctrl(struct mdiobb_ctrl *ctrl, u32 mask, int set)
 {
 	struct ravb_private *priv = container_of(ctrl, struct ravb_private,
 						 mdiobb);
+	int ret;
+
+	ret = ravb_pm_runtime_get(priv);
+	if (ret < 0)
+		return;
 
 	ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);
+
+	ravb_pm_runtime_put(priv);
 }
 
 /* MDC pin control */
@@ -176,8 +205,17 @@ static int ravb_get_mdio_data(struct mdiobb_ctrl *ctrl)
 {
 	struct ravb_private *priv = container_of(ctrl, struct ravb_private,
 						 mdiobb);
+	int ret;
 
-	return (ravb_read(priv->ndev, PIR) & PIR_MDI) != 0;
+	ret = ravb_pm_runtime_get(priv);
+	if (ret < 0)
+		return ret;
+
+	ret = (ravb_read(priv->ndev, PIR) & PIR_MDI) != 0;
+
+	ravb_pm_runtime_put(priv);
+
+	return ret;
 }
 
 /* MDIO bus control struct */
@@ -1796,10 +1834,14 @@ static int ravb_open(struct net_device *ndev)
 		}
 	}
 
+	error = ravb_pm_runtime_get(priv);
+	if (error < 0)
+		return error;
+
 	/* Device init */
 	error = ravb_dmac_init(ndev);
 	if (error)
-		goto out_free_irq_mgmta;
+		goto pm_runtime_put;
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
@@ -1820,7 +1862,8 @@ static int ravb_open(struct net_device *ndev)
 	if (info->gptp)
 		ravb_ptp_stop(ndev);
 	ravb_stop_dma(ndev);
-out_free_irq_mgmta:
+pm_runtime_put:
+	ravb_pm_runtime_put(priv);
 	if (!info->multi_irqs)
 		goto out_free_irq;
 	if (info->err_mgmt_irqs)
@@ -2064,6 +2107,11 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
 	struct net_device_stats *nstats, *stats0, *stats1;
+	int ret;
+
+	ret = ravb_pm_runtime_get(priv);
+	if (ret < 0)
+		return NULL;
 
 	nstats = &ndev->stats;
 	stats0 = &priv->stats[RAVB_BE];
@@ -2107,6 +2155,8 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 		nstats->rx_over_errors += stats1->rx_over_errors;
 	}
 
+	ravb_pm_runtime_put(priv);
+
 	return nstats;
 }
 
@@ -2115,11 +2165,18 @@ static void ravb_set_rx_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned long flags;
+	int ret;
+
+	ret = ravb_pm_runtime_get(priv);
+	if (ret < 0)
+		return;
 
 	spin_lock_irqsave(&priv->lock, flags);
 	ravb_modify(ndev, ECMR, ECMR_PRM,
 		    ndev->flags & IFF_PROMISC ? ECMR_PRM : 0);
 	spin_unlock_irqrestore(&priv->lock, flags);
+
+	ravb_pm_runtime_put(priv);
 }
 
 /* Device close function for Ethernet AVB */
@@ -2187,6 +2244,11 @@ static int ravb_close(struct net_device *ndev)
 	if (info->nc_queues)
 		ravb_ring_free(ndev, RAVB_NC);
 
+	/* Note that if RPM is enabled on plaforms with ccc_gac=1 this needs to be skipped and
+	 * added to suspend function after PTP is stopped.
+	 */
+	ravb_pm_runtime_put(priv);
+
 	return 0;
 }
 
@@ -2503,6 +2565,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.carrier_counters = 1,
 	.half_duplex = 1,
 	.refclk_in_pd = 1,
+	.rpm = 1,
 };
 
 static const struct of_device_id ravb_match_table[] = {
@@ -2636,6 +2699,12 @@ static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
+	info = of_device_get_match_data(&pdev->dev);
+
+	if (info->rpm) {
+		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)
@@ -2647,7 +2716,6 @@ static int ravb_probe(struct platform_device *pdev)
 		error = -ENOMEM;
 		goto pm_runtime_put;
 	}
-	info = of_device_get_match_data(&pdev->dev);
 
 	ndev->features = info->net_features;
 	ndev->hw_features = info->net_hw_features;
@@ -2856,6 +2924,8 @@ static int ravb_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ndev);
 
+	ravb_pm_runtime_put(priv);
+
 	return 0;
 
 out_napi_del:
@@ -2880,6 +2950,8 @@ static int ravb_probe(struct platform_device *pdev)
 	pm_runtime_put(&pdev->dev);
 pm_runtime_disable:
 	pm_runtime_disable(&pdev->dev);
+	if (info->rpm)
+		pm_runtime_dont_use_autosuspend(&pdev->dev);
 	reset_control_assert(rstc);
 	return error;
 }
@@ -2889,6 +2961,11 @@ 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;
+
+	error = ravb_pm_runtime_get(priv);
+	if (error < 0)
+		return;
 
 	/* Stop PTP Clock driver */
 	if (info->ccc_gac)
@@ -2908,6 +2985,8 @@ static void ravb_remove(struct platform_device *pdev)
 			  priv->desc_bat_dma);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	if (info->rpm)
+		pm_runtime_dont_use_autosuspend(&pdev->dev);
 	reset_control_assert(priv->rstc);
 	free_netdev(ndev);
 	platform_set_drvdata(pdev, NULL);
@@ -2989,6 +3068,10 @@ static int ravb_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = ravb_pm_runtime_get(priv);
+	if (ret < 0)
+		return ret;
+
 	/* If WoL is enabled set reset mode to rearm the WoL logic */
 	if (priv->wol_enabled)
 		ravb_write(ndev, CCC_OPC_RESET, CCC);
@@ -3005,7 +3088,7 @@ static int ravb_resume(struct device *dev)
 		/* Set GTI value */
 		ret = ravb_set_gti(ndev);
 		if (ret)
-			return ret;
+			goto pm_runtime_put;
 
 		/* Request GTI loading */
 		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
@@ -3024,15 +3107,17 @@ static int ravb_resume(struct device *dev)
 		if (priv->wol_enabled) {
 			ret = ravb_wol_restore(ndev);
 			if (ret)
-				return ret;
+				goto pm_runtime_put;
 		}
 		ret = ravb_open(ndev);
 		if (ret < 0)
-			return ret;
+			goto pm_runtime_put;
 		ravb_set_rx_mode(ndev);
 		netif_device_attach(ndev);
 	}
 
+pm_runtime_put:
+	ravb_pm_runtime_put(priv);
 	return ret;
 }
 
-- 
2.39.2

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[Sergey Shtylyov]

On 11/20/23 11:46 AM, Claudiu wrote:

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

> RZ/G3S supports enabling/disabling clocks for its modules (including
> Ethernet module). For this commit adds runtime PM support which
> relies on PM domain to enable/disable Ethernet clocks.

   That's not exactly something new in RZ/G3S. The ravb driver has unconditional
RPM calls already in the probe() and remove() methods... And the sh_eth driver
has RPM support since 2009...

> At the end of probe ravb_pm_runtime_put() is called which will turn

   I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
it doesn't seem hardly needed...

> off the Ethernet clocks (if no other request arrives at the driver).
> After that if the interface is brought up (though ravb_open()) then
> the clocks remain enabled until interface is brought down (operation
> done though ravb_close()).
> 
> If any request arrives to the driver while the interface is down the
> clocks are enabled to serve the request and then disabled.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  1 +
>  drivers/net/ethernet/renesas/ravb_main.c | 99 ++++++++++++++++++++++--
>  2 files changed, 93 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index c2d8d890031f..50f358472aab 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>  	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
>  	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
>  	unsigned refclk_in_pd:1;	/* Reference clock is part of a power domain. */
> +	unsigned rpm:1;			/* Runtime PM available. */

   No, I don't think this flag makes any sense. We should support RPM
unconditionally...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index f4634ac0c972..d70ed7e5f7f6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -145,12 +145,41 @@ static void ravb_read_mac_address(struct device_node *np,
[...]
> +static void ravb_pm_runtime_put(struct ravb_private *priv)
> +{
> +	const struct ravb_hw_info *info = priv->info;
> +	struct device *dev = &priv->pdev->dev;
> +
> +	if (!info->rpm)
> +		return;
> +
> +	pm_runtime_mark_last_busy(dev);

   Not very familiar with RPM... what's this for?

> +	pm_runtime_put_autosuspend(dev);

   Why not the usual pm_runtime_put()?

> +}
> +
>  static void ravb_mdio_ctrl(struct mdiobb_ctrl *ctrl, u32 mask, int set)
>  {
>  	struct ravb_private *priv = container_of(ctrl, struct ravb_private,
>  						 mdiobb);
> +	int ret;
> +
> +	ret = ravb_pm_runtime_get(priv);
> +	if (ret < 0)
> +		return;
>  
>  	ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);
> +
> +	ravb_pm_runtime_put(priv);

   Hmm, does this even work? :-/ Do the MDIO bits retain the values while
the AVB core is not clocked or even powered down?
   Note that the sh_eth driver has RPM calls in the {read|write}_c{22?45}()
methods which do the full register read/write while the core is powere up
and clocked...

[...]
> @@ -2064,6 +2107,11 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
>  	struct net_device_stats *nstats, *stats0, *stats1;
> +	int ret;
> +
> +	ret = ravb_pm_runtime_get(priv);
> +	if (ret < 0)
> +		return NULL;

   Hm, sh_eth.c doesn't have any RPM calls in this method. Again, do
the hardware counters remain valid across powering the MAC core down?

[...]
> @@ -2115,11 +2165,18 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	unsigned long flags;
> +	int ret;
> +
> +	ret = ravb_pm_runtime_get(priv);
> +	if (ret < 0)
> +		return;

   Hm, sh_eth.c doesn't have any RPM calls in this method either.
Does changing the promiscous mode have sense for an offlined interface?

[...]
> @@ -2187,6 +2244,11 @@ static int ravb_close(struct net_device *ndev)
>  	if (info->nc_queues)
>  		ravb_ring_free(ndev, RAVB_NC);
>  
> +	/* Note that if RPM is enabled on plaforms with ccc_gac=1 this needs to be

   It's "platforms". :-)

> skipped and

   Overly long line?

> +	 * added to suspend function after PTP is stopped.

   I guess we'll have to do that because RPM is actually not RZ/G3
specific...

> +	 */
> +	ravb_pm_runtime_put(priv);
> +
>  	return 0;
>  }
>  
> @@ -2636,6 +2699,12 @@ static int ravb_probe(struct platform_device *pdev)
>  	if (error)
>  		return error;
>  
> +	info = of_device_get_match_data(&pdev->dev);
> +
> +	if (info->rpm) {
> +		pm_runtime_set_autosuspend_delay(&pdev->dev, 100);

   Why exactly 100 ms?

> +		pm_runtime_use_autosuspend(&pdev->dev);
> +	}

   Before calling pm_runtime_enable()?

>  	pm_runtime_enable(&pdev->dev);
[...]
> @@ -2880,6 +2950,8 @@ static int ravb_probe(struct platform_device *pdev)
>  	pm_runtime_put(&pdev->dev);
>  pm_runtime_disable:
>  	pm_runtime_disable(&pdev->dev);
> +	if (info->rpm)
> +		pm_runtime_dont_use_autosuspend(&pdev->dev);

   After calling pm_runtime_disable()?

[...]
> @@ -2908,6 +2985,8 @@ static void ravb_remove(struct platform_device *pdev)
>  			  priv->desc_bat_dma);
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	if (info->rpm)
> +		pm_runtime_dont_use_autosuspend(&pdev->dev);

   After calling pm_runtime_disable()?

[...]

MBR, Sergey

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[claudiu beznea]

On 22.11.2023 18:25, Sergey Shtylyov wrote:
> On 11/20/23 11:46 AM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
>> RZ/G3S supports enabling/disabling clocks for its modules (including
>> Ethernet module). For this commit adds runtime PM support which
>> relies on PM domain to enable/disable Ethernet clocks.
> 
>    That's not exactly something new in RZ/G3S. The ravb driver has unconditional
> RPM calls already in the probe() and remove() methods... 
> And the sh_eth driver
> has RPM support since 2009...
> 
>> At the end of probe ravb_pm_runtime_put() is called which will turn
> 
>    I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
> it doesn't seem hardly needed...
> 
>> off the Ethernet clocks (if no other request arrives at the driver).
>> After that if the interface is brought up (though ravb_open()) then
>> the clocks remain enabled until interface is brought down (operation
>> done though ravb_close()).
>>
>> If any request arrives to the driver while the interface is down the
>> clocks are enabled to serve the request and then disabled.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb.h      |  1 +
>>  drivers/net/ethernet/renesas/ravb_main.c | 99 ++++++++++++++++++++++--
>>  2 files changed, 93 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>> index c2d8d890031f..50f358472aab 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>  	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
>>  	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
>>  	unsigned refclk_in_pd:1;	/* Reference clock is part of a power domain. */
>> +	unsigned rpm:1;			/* Runtime PM available. */
> 
>    No, I don't think this flag makes any sense. We should support RPM
> unconditionally...

The reasons I've limited only to RZ/G3S are:
1/ I don't have all the platforms to test it
2/ on G1H this doesn't work. I tried to debugged it but I don't have a
   platform at hand, only remotely, and is hardly to debug once the
   ethernet fails to work: probe is working(), open is executed, PHY is
   initialized and then TX/RX is not working... don't know why ATM.

> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index f4634ac0c972..d70ed7e5f7f6 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -145,12 +145,41 @@ static void ravb_read_mac_address(struct device_node *np,
> [...]
>> +static void ravb_pm_runtime_put(struct ravb_private *priv)
>> +{
>> +	const struct ravb_hw_info *info = priv->info;
>> +	struct device *dev = &priv->pdev->dev;
>> +
>> +	if (!info->rpm)
>> +		return;
>> +
>> +	pm_runtime_mark_last_busy(dev);
> 
>    Not very familiar with RPM... what's this for?

It timestamps the last time the device has been used and then after
autosuspend delay ms passes starting from this timestamp RPM core suspends
the device. It's useful if there are consecutive requests to driver to
avoid actually doing hardware changes for each RPM get/put.

> 
>> +	pm_runtime_put_autosuspend(dev);
> 
>    Why not the usual pm_runtime_put()?

For the same reason explained above.

> 
>> +}
>> +
>>  static void ravb_mdio_ctrl(struct mdiobb_ctrl *ctrl, u32 mask, int set)
>>  {
>>  	struct ravb_private *priv = container_of(ctrl, struct ravb_private,
>>  						 mdiobb);
>> +	int ret;
>> +
>> +	ret = ravb_pm_runtime_get(priv);
>> +	if (ret < 0)
>> +		return;
>>  
>>  	ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);
>> +
>> +	ravb_pm_runtime_put(priv);
> 
>    Hmm, does this even work? :-/ Do the MDIO bits retain the values while
> the AVB core is not clocked or even powered down?

This actually is not needed. It's a leftover. I double checked with
mii-tools to access the device while the interface is down and the IOCTL is
blocked in this case by
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/renesas/ravb_main.c#L2266

>    Note that the sh_eth driver has RPM calls in the {read|write}_c{22?45}()
> methods which do the full register read/write while the core is powere up
> and clocked...
> 
> [...]
>> @@ -2064,6 +2107,11 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>>  	struct net_device_stats *nstats, *stats0, *stats1;
>> +	int ret;
>> +
>> +	ret = ravb_pm_runtime_get(priv);
>> +	if (ret < 0)
>> +		return NULL;
> 
>    Hm, sh_eth.c doesn't have any RPM calls in this method. Again, do

In setups where systemd is enabled, user space calls this method in
different stages (e.g. at boot time or when running ifconfig ethX, even if
interface is down). W/o runtime resuming here the system will fail to boot.

The other approach I wanted to take was to:

if (!netif_running(dev))
	return &ndev->stats;

But I didn't choose this path as there are some counters updated to nstat
only in this function, e.g. nstats->tx_dropped += ravb_read(ndev, TROCR);
and wanted an opinion about it.


> the hardware counters remain valid across powering the MAC core down?

The power domain that the Ethernet clocks of RZ/G3S belong disables the
clock and switches the Ethernet module to standby. There is no information
in HW manual that the content of registers will be lost.

> 
> [...]
>> @@ -2115,11 +2165,18 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	unsigned long flags;
>> +	int ret;
>> +
>> +	ret = ravb_pm_runtime_get(priv);
>> +	if (ret < 0)
>> +		return;
> 
>    Hm, sh_eth.c doesn't have any RPM calls in this method either.
> Does changing the promiscous mode have sense for an offlined interface?

I've added it for scenarios when the interface is down and user tries to
configure it. I don't know to answer your question. W/o RPM resume here
user space blocks if tries to access it and interface is down. I can just
return if interface is down. Let me know if you prefer this way.

> 
> [...]
>> @@ -2187,6 +2244,11 @@ static int ravb_close(struct net_device *ndev)
>>  	if (info->nc_queues)
>>  		ravb_ring_free(ndev, RAVB_NC);
>>  
>> +	/* Note that if RPM is enabled on plaforms with ccc_gac=1 this needs to be
> 
>    It's "platforms". :-)
> 
>> skipped and
> 
>    Overly long line?

Not more than 100 chars. Do you want it to 80?

> 
>> +	 * added to suspend function after PTP is stopped.
> 
>    I guess we'll have to do that because RPM is actually not RZ/G3
> specific...

yes

> 
>> +	 */
>> +	ravb_pm_runtime_put(priv);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -2636,6 +2699,12 @@ static int ravb_probe(struct platform_device *pdev)
>>  	if (error)
>>  		return error;
>>  
>> +	info = of_device_get_match_data(&pdev->dev);
>> +
>> +	if (info->rpm) {
>> +		pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
> 
>    Why exactly 100 ms?

I noticed Ethernet drivers using this functionality are using this default
value. It can be anyway changed though sysfs.

> 
>> +		pm_runtime_use_autosuspend(&pdev->dev);
>> +	}
> 
>    Before calling pm_runtime_enable()?

Drivers that I know and checked sets autosuspend before enabling RPM. Code
seems written to allow both scenarios but I think it's better to have
everything setup before enabling RPM.

> 
>>  	pm_runtime_enable(&pdev->dev);
> [...]
>> @@ -2880,6 +2950,8 @@ static int ravb_probe(struct platform_device *pdev)
>>  	pm_runtime_put(&pdev->dev);
>>  pm_runtime_disable:
>>  	pm_runtime_disable(&pdev->dev);
>> +	if (info->rpm)
>> +		pm_runtime_dont_use_autosuspend(&pdev->dev);
> 
>    After calling pm_runtime_disable()?

Based on the above explanation and the fact that I would like to keep the
calls here in reverse order I prefer to have it in this way.

> 
> [...]
>> @@ -2908,6 +2985,8 @@ static void ravb_remove(struct platform_device *pdev)
>>  			  priv->desc_bat_dma);
>>  	pm_runtime_put_sync(&pdev->dev);
>>  	pm_runtime_disable(&pdev->dev);
>> +	if (info->rpm)
>> +		pm_runtime_dont_use_autosuspend(&pdev->dev);
> 
>    After calling pm_runtime_disable()?

Ditto.

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

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[Sergey Shtylyov]

On 11/23/23 8:04 PM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>> Ethernet module). For this commit adds runtime PM support which
>>> relies on PM domain to enable/disable Ethernet clocks.
>>
>>    That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>> RPM calls already in the probe() and remove() methods... 
>> And the sh_eth driver
>> has RPM support since 2009...
>>
>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>
>>    I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>> it doesn't seem hardly needed...

   Does seem, sorry. :-)

>>> off the Ethernet clocks (if no other request arrives at the driver).
>>> After that if the interface is brought up (though ravb_open()) then
>>> the clocks remain enabled until interface is brought down (operation
>>> done though ravb_close()).
>>>
>>> If any request arrives to the driver while the interface is down the
>>> clocks are enabled to serve the request and then disabled.
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb.h      |  1 +
>>>  drivers/net/ethernet/renesas/ravb_main.c | 99 ++++++++++++++++++++++--
>>>  2 files changed, 93 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>> index c2d8d890031f..50f358472aab 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>>  	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
>>>  	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
>>>  	unsigned refclk_in_pd:1;	/* Reference clock is part of a power domain. */
>>> +	unsigned rpm:1;			/* Runtime PM available. */
>>
>>    No, I don't think this flag makes any sense. We should support RPM
>> unconditionally...

   If RPM calls work in the probe()/remove() methods, they should work
in the ndo_{open|stop}() methods, right?

> The reasons I've limited only to RZ/G3S are:
> 1/ I don't have all the platforms to test it

   That's a usual problem with the kernel development...

> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
>    platform at hand, only remotely, and is hardly to debug once the
>    ethernet fails to work: probe is working(), open is executed, PHY is
>    initialized and then TX/RX is not working... don't know why ATM.

   That's why we have the long bug fixing period after -rc1...

[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index f4634ac0c972..d70ed7e5f7f6 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -145,12 +145,41 @@ static void ravb_read_mac_address(struct device_node *np,
[...]
>>> +}
>>> +
>>>  static void ravb_mdio_ctrl(struct mdiobb_ctrl *ctrl, u32 mask, int set)
>>>  {
>>>  	struct ravb_private *priv = container_of(ctrl, struct ravb_private,
>>>  						 mdiobb);
>>> +	int ret;
>>> +
>>> +	ret = ravb_pm_runtime_get(priv);
>>> +	if (ret < 0)
>>> +		return;
>>>  
>>>  	ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);
>>> +
>>> +	ravb_pm_runtime_put(priv);
>>
>>    Hmm, does this even work? :-/ Do the MDIO bits retain the values while
>> the AVB core is not clocked or even powered down?
> 
> This actually is not needed. It's a leftover. I double checked with
> mii-tools to access the device while the interface is down and the IOCTL is
> blocked in this case by
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/renesas/ravb_main.c#L2266

   Have you tested with ethtool as well?

>>    Note that the sh_eth driver has RPM calls in the {read|write}_c{22?45}()

   s/?/|/,

>> methods which do the full register read/write while the core is powere up

   Powered.

>> and clocked...
>>
>> [...]
>>> @@ -2064,6 +2107,11 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>  	const struct ravb_hw_info *info = priv->info;
>>>  	struct net_device_stats *nstats, *stats0, *stats1;
>>> +	int ret;
>>> +
>>> +	ret = ravb_pm_runtime_get(priv);
>>> +	if (ret < 0)
>>> +		return NULL;
>>
>>    Hm, sh_eth.c doesn't have any RPM calls in this method. Again, do
> 
> In setups where systemd is enabled, user space calls this method in
> different stages (e.g. at boot time or when running ifconfig ethX, even if
> interface is down). W/o runtime resuming here the system will fail to boot.
> 
> The other approach I wanted to take was to:
> 
> if (!netif_running(dev))
> 	return &ndev->stats;
> 
> But I didn't choose this path as there are some counters updated to nstat
> only in this function, e.g. nstats->tx_dropped += ravb_read(ndev, TROCR);
> and wanted an opinion about it.

   Have you seen the following commit (that I've already posted for you on
IRC)?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7fa2955ff70ce4532f144d26b8a087095f9c9ffc

   Looks like the RPM calls won't do here...

>> the hardware counters remain valid across powering the MAC core down?
> 
> The power domain that the Ethernet clocks of RZ/G3S belong disables the
> clock and switches the Ethernet module to standby. There is no information
> in HW manual that the content of registers will be lost.

   That's what your current PD driver does... isn't it possible that
in some new SoCs the PD would be completely powered off?

[...]
>>> @@ -2115,11 +2165,18 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>>>  {
>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>  	unsigned long flags;
>>> +	int ret;
>>> +
>>> +	ret = ravb_pm_runtime_get(priv);
>>> +	if (ret < 0)
>>> +		return;
>>
>>    Hm, sh_eth.c doesn't have any RPM calls in this method either.
>> Does changing the promiscous mode have sense for an offlined interface?
> 
> I've added it for scenarios when the interface is down and user tries to
> configure it. I don't know to answer your question. W/o RPM resume here
> user space blocks if tries to access it and interface is down. I can just
> return if interface is down. Let me know if you prefer this way.

   Looking at __dev_set_rx_mode(), the method gets only called when
(dev->flags & IFF_UP) is true -- but that contradicts your experience,
it seems... However, looking at net/core/dev_addr_lists.c, that function
is called from the atomic contexts, so please just return early.

>> [...]
>>> @@ -2187,6 +2244,11 @@ static int ravb_close(struct net_device *ndev)
>>>  	if (info->nc_queues)
>>>  		ravb_ring_free(ndev, RAVB_NC);
>>>  
>>> +	/* Note that if RPM is enabled on plaforms with ccc_gac=1 this needs to be
>>
>>    It's "platforms". :-)
>>
>>> skipped and
>>
>>    Overly long line?
> 
> Not more than 100 chars. Do you want it to 80?

   Yes, it's not the code, no need to go beyond 80 cols, I think...

[...]

MBR, Sergey

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[claudiu beznea]

On 23.11.2023 21:19, Sergey Shtylyov wrote:
> On 11/23/23 8:04 PM, claudiu beznea wrote:
> 
> [...]
> 
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>> Ethernet module). For this commit adds runtime PM support which
>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>
>>>    That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>> RPM calls already in the probe() and remove() methods... 
>>> And the sh_eth driver
>>> has RPM support since 2009...
>>>
>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>
>>>    I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>> it doesn't seem hardly needed...
> 
>    Does seem, sorry. :-)
> 
>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>> After that if the interface is brought up (though ravb_open()) then
>>>> the clocks remain enabled until interface is brought down (operation
>>>> done though ravb_close()).
>>>>
>>>> If any request arrives to the driver while the interface is down the
>>>> clocks are enabled to serve the request and then disabled.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>  drivers/net/ethernet/renesas/ravb.h      |  1 +
>>>>  drivers/net/ethernet/renesas/ravb_main.c | 99 ++++++++++++++++++++++--
>>>>  2 files changed, 93 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>>> index c2d8d890031f..50f358472aab 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>>>  	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
>>>>  	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
>>>>  	unsigned refclk_in_pd:1;	/* Reference clock is part of a power domain. */
>>>> +	unsigned rpm:1;			/* Runtime PM available. */
>>>
>>>    No, I don't think this flag makes any sense. We should support RPM
>>> unconditionally...
> 
>    If RPM calls work in the probe()/remove() methods, they should work
> in the ndo_{open|stop}() methods, right?

It might depend on hardware support... E.g.

I debugged it further the issue I had with this implementation on other
SoCs and it seems we cannot do RPM for those w/o reworking way the driver
is configured.

I wiped out the RPM code from this patch and just called:

pm_runtime_put_sync();		// [1]
usleep_range(300000, 400000);	// [2]
pm_runtime_get_sync();		// [3]

at the end of ravb_probe(); with this the interfaces fails to work. I
continue debugging it and interrogated CSR and this returns RESET after
[3]. I tried to switched it back to configuration mode after [3] but fails
to restore to a proper working state.

Then continued to debug it further to see what happens on the clock driver.
The clk enable/disable reaches function at [4] which sets control_regs[reg]
which is one of the System module stop control registers. Setting this
activates module standby (AFICT). Switch to reset state on Ethernet IP
might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
chapter of the G1H HW manual (which I don't fully understand).

Also, the manual of G1H states from some IPs that register state is
preserved in standby mode but not for AVB.

[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/renesas/renesas-cpg-mssr.c#n190


> 
>> The reasons I've limited only to RZ/G3S are:
>> 1/ I don't have all the platforms to test it
> 
>    That's a usual problem with the kernel development...
> 
>> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
>>    platform at hand, only remotely, and is hardly to debug once the
>>    ethernet fails to work: probe is working(), open is executed, PHY is
>>    initialized and then TX/RX is not working... don't know why ATM.
> 
>    That's why we have the long bug fixing period after -rc1...

I prefer to not introduce any bug by intention.

> 
> [...]
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index f4634ac0c972..d70ed7e5f7f6 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -145,12 +145,41 @@ static void ravb_read_mac_address(struct device_node *np,
> [...]
>>>> +}
>>>> +
>>>>  static void ravb_mdio_ctrl(struct mdiobb_ctrl *ctrl, u32 mask, int set)
>>>>  {
>>>>  	struct ravb_private *priv = container_of(ctrl, struct ravb_private,
>>>>  						 mdiobb);
>>>> +	int ret;
>>>> +
>>>> +	ret = ravb_pm_runtime_get(priv);
>>>> +	if (ret < 0)
>>>> +		return;
>>>>  
>>>>  	ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);
>>>> +
>>>> +	ravb_pm_runtime_put(priv);
>>>
>>>    Hmm, does this even work? :-/ Do the MDIO bits retain the values while
>>> the AVB core is not clocked or even powered down?
>>
>> This actually is not needed. It's a leftover. I double checked with
>> mii-tools to access the device while the interface is down and the IOCTL is
>> blocked in this case by
>> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/renesas/ravb_main.c#L2266
> 
>    Have you tested with ethtool as well?
> 
>>>    Note that the sh_eth driver has RPM calls in the {read|write}_c{22?45}()
> 
>    s/?/|/,
> 
>>> methods which do the full register read/write while the core is powere up
> 
>    Powered.
> 
>>> and clocked...
>>>
>>> [...]
>>>> @@ -2064,6 +2107,11 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>>  	const struct ravb_hw_info *info = priv->info;
>>>>  	struct net_device_stats *nstats, *stats0, *stats1;
>>>> +	int ret;
>>>> +
>>>> +	ret = ravb_pm_runtime_get(priv);
>>>> +	if (ret < 0)
>>>> +		return NULL;
>>>
>>>    Hm, sh_eth.c doesn't have any RPM calls in this method. Again, do
>>
>> In setups where systemd is enabled, user space calls this method in
>> different stages (e.g. at boot time or when running ifconfig ethX, even if
>> interface is down). W/o runtime resuming here the system will fail to boot.
>>
>> The other approach I wanted to take was to:
>>
>> if (!netif_running(dev))
>> 	return &ndev->stats;
>>
>> But I didn't choose this path as there are some counters updated to nstat
>> only in this function, e.g. nstats->tx_dropped += ravb_read(ndev, TROCR);
>> and wanted an opinion about it.
> 
>    Have you seen the following commit (that I've already posted for you on
> IRC)?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7fa2955ff70ce4532f144d26b8a087095f9c9ffc
> 
>    Looks like the RPM calls won't do here...
> 
>>> the hardware counters remain valid across powering the MAC core down?
>>
>> The power domain that the Ethernet clocks of RZ/G3S belong disables the
>> clock and switches the Ethernet module to standby. There is no information
>> in HW manual that the content of registers will be lost.
> 
>    That's what your current PD driver does... isn't it possible that
> in some new SoCs the PD would be completely powered off?
> 
> [...]
>>>> @@ -2115,11 +2165,18 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>>>>  {
>>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>>  	unsigned long flags;
>>>> +	int ret;
>>>> +
>>>> +	ret = ravb_pm_runtime_get(priv);
>>>> +	if (ret < 0)
>>>> +		return;
>>>
>>>    Hm, sh_eth.c doesn't have any RPM calls in this method either.
>>> Does changing the promiscous mode have sense for an offlined interface?
>>
>> I've added it for scenarios when the interface is down and user tries to
>> configure it. I don't know to answer your question. W/o RPM resume here
>> user space blocks if tries to access it and interface is down. I can just
>> return if interface is down. Let me know if you prefer this way.
> 
>    Looking at __dev_set_rx_mode(), the method gets only called when
> (dev->flags & IFF_UP) is true -- but that contradicts your experience,
> it seems... However, looking at net/core/dev_addr_lists.c, that function
> is called from the atomic contexts, so please just return early.
> 
>>> [...]
>>>> @@ -2187,6 +2244,11 @@ static int ravb_close(struct net_device *ndev)
>>>>  	if (info->nc_queues)
>>>>  		ravb_ring_free(ndev, RAVB_NC);
>>>>  
>>>> +	/* Note that if RPM is enabled on plaforms with ccc_gac=1 this needs to be
>>>
>>>    It's "platforms". :-)
>>>
>>>> skipped and
>>>
>>>    Overly long line?
>>
>> Not more than 100 chars. Do you want it to 80?
> 
>    Yes, it's not the code, no need to go beyond 80 cols, I think...
> 
> [...]
> 
> MBR, Sergey

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[Geert Uytterhoeven]

Hi Claudiu,

On Sat, Nov 25, 2023 at 12:00 AM claudiu beznea
<claudiu.beznea@tuxon.dev> wrote:
> On 23.11.2023 21:19, Sergey Shtylyov wrote:
> > On 11/23/23 8:04 PM, claudiu beznea wrote:
> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>
> >>>> RZ/G3S supports enabling/disabling clocks for its modules (including
> >>>> Ethernet module). For this commit adds runtime PM support which
> >>>> relies on PM domain to enable/disable Ethernet clocks.
> >>>
> >>>    That's not exactly something new in RZ/G3S. The ravb driver has unconditional
> >>> RPM calls already in the probe() and remove() methods...
> >>> And the sh_eth driver
> >>> has RPM support since 2009...
> >>>
> >>>> At the end of probe ravb_pm_runtime_put() is called which will turn
> >>>
> >>>    I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
> >>>> off the Ethernet clocks (if no other request arrives at the driver).
> >>>> After that if the interface is brought up (though ravb_open()) then
> >>>> the clocks remain enabled until interface is brought down (operation
> >>>> done though ravb_close()).
> >>>>
> >>>> If any request arrives to the driver while the interface is down the
> >>>> clocks are enabled to serve the request and then disabled.
> >>>>
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> >>>> --- a/drivers/net/ethernet/renesas/ravb.h
> >>>> +++ b/drivers/net/ethernet/renesas/ravb.h
> >>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
> >>>>    unsigned magic_pkt:1;           /* E-MAC supports magic packet detection */
> >>>>    unsigned half_duplex:1;         /* E-MAC supports half duplex mode */
> >>>>    unsigned refclk_in_pd:1;        /* Reference clock is part of a power domain. */
> >>>> +  unsigned rpm:1;                 /* Runtime PM available. */
> >>>
> >>>    No, I don't think this flag makes any sense. We should support RPM
> >>> unconditionally...
> >
> >    If RPM calls work in the probe()/remove() methods, they should work
> > in the ndo_{open|stop}() methods, right?
>
> It might depend on hardware support... E.g.
>
> I debugged it further the issue I had with this implementation on other
> SoCs and it seems we cannot do RPM for those w/o reworking way the driver
> is configured.
>
> I wiped out the RPM code from this patch and just called:
>
> pm_runtime_put_sync();          // [1]
> usleep_range(300000, 400000);   // [2]
> pm_runtime_get_sync();          // [3]
>
> at the end of ravb_probe(); with this the interfaces fails to work. I
> continue debugging it and interrogated CSR and this returns RESET after
> [3]. I tried to switched it back to configuration mode after [3] but fails
> to restore to a proper working state.
>
> Then continued to debug it further to see what happens on the clock driver.
> The clk enable/disable reaches function at [4] which sets control_regs[reg]
> which is one of the System module stop control registers. Setting this
> activates module standby (AFICT). Switch to reset state on Ethernet IP
> might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
> chapter of the G1H HW manual (which I don't fully understand).

You mean 37A.3.1.3 (2) "Transition during power-off by module standby"?

    The AVB-DMAC completes the bus master access in progress,
    and then shifts to reset mode. At this time, the operating mode
    configuration bits in the AVB-DMAC mode register (CCC.OPC) are
    set to B'00.

"reset mode" could be interpreted as "register contents are reset (lost)".
However, the R-Car Gen3 documentation contains the same paragraph,
and register contents are known not to be lost...

37.7.2 for Ether ("sh-eth") states:

    After returning from the standby state, the ether should be reset
and initialized.

Sergey: does sh_eth.c really reinitialize the hardware completely after
pm_runtime_get_sync()?

> Also, the manual of G1H states from some IPs that register state is
> preserved in standby mode but not for AVB.

Indeed, AFAIK all modules on SH/R-Mobile, R-Car, and RZ/G SoCs keep
their register contents when in standby-mode (module standby enabled).
On modules in a (not always-on) power area (e.g. SH/R-Mobile), register
contents are lost when the power area is powered down.
So I'd be surprised if EtherAVB behaves differently.  Of course that
is still possible, there are big difference between EtherAVB in R-Car
Gen2 and RZ/G1, and the revision found in later SoC families.

> >> The reasons I've limited only to RZ/G3S are:
> >> 1/ I don't have all the platforms to test it
> >
> >    That's a usual problem with the kernel development...
> >
> >> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
> >>    platform at hand, only remotely, and is hardly to debug once the
> >>    ethernet fails to work: probe is working(), open is executed, PHY is
> >>    initialized and then TX/RX is not working... don't know why ATM.
> >
> >    That's why we have the long bug fixing period after -rc1...
>
> I prefer to not introduce any bug by intention.

Iff register contents are lost on RZ/G1H, I'd rather add
an extra clk_prepare_enable(priv->clk) to ravb_probe() on
"renesas,etheravb-rcar-gen2"....

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[claudiu beznea]

Hi, Geert,

On 27.11.2023 16:05, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Sat, Nov 25, 2023 at 12:00 AM claudiu beznea
> <claudiu.beznea@tuxon.dev> wrote:
>> On 23.11.2023 21:19, Sergey Shtylyov wrote:
>>> On 11/23/23 8:04 PM, claudiu beznea wrote:
>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>
>>>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>>>> Ethernet module). For this commit adds runtime PM support which
>>>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>>>
>>>>>    That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>>>> RPM calls already in the probe() and remove() methods...
>>>>> And the sh_eth driver
>>>>> has RPM support since 2009...
>>>>>
>>>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>>>
>>>>>    I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>>>> After that if the interface is brought up (though ravb_open()) then
>>>>>> the clocks remain enabled until interface is brought down (operation
>>>>>> done though ravb_close()).
>>>>>>
>>>>>> If any request arrives to the driver while the interface is down the
>>>>>> clocks are enabled to serve the request and then disabled.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>>>>>    unsigned magic_pkt:1;           /* E-MAC supports magic packet detection */
>>>>>>    unsigned half_duplex:1;         /* E-MAC supports half duplex mode */
>>>>>>    unsigned refclk_in_pd:1;        /* Reference clock is part of a power domain. */
>>>>>> +  unsigned rpm:1;                 /* Runtime PM available. */
>>>>>
>>>>>    No, I don't think this flag makes any sense. We should support RPM
>>>>> unconditionally...
>>>
>>>    If RPM calls work in the probe()/remove() methods, they should work
>>> in the ndo_{open|stop}() methods, right?
>>
>> It might depend on hardware support... E.g.
>>
>> I debugged it further the issue I had with this implementation on other
>> SoCs and it seems we cannot do RPM for those w/o reworking way the driver
>> is configured.
>>
>> I wiped out the RPM code from this patch and just called:
>>
>> pm_runtime_put_sync();          // [1]
>> usleep_range(300000, 400000);   // [2]
>> pm_runtime_get_sync();          // [3]
>>
>> at the end of ravb_probe(); with this the interfaces fails to work. I
>> continue debugging it and interrogated CSR and this returns RESET after
>> [3]. I tried to switched it back to configuration mode after [3] but fails
>> to restore to a proper working state.
>>
>> Then continued to debug it further to see what happens on the clock driver.
>> The clk enable/disable reaches function at [4] which sets control_regs[reg]
>> which is one of the System module stop control registers. Setting this
>> activates module standby (AFICT). Switch to reset state on Ethernet IP
>> might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
>> chapter of the G1H HW manual (which I don't fully understand).
> 
> You mean 37A.3.1.3 (2) "Transition during power-off by module standby"?

Yes!

> 
>     The AVB-DMAC completes the bus master access in progress,
>     and then shifts to reset mode. At this time, the operating mode
>     configuration bits in the AVB-DMAC mode register (CCC.OPC) are
>     set to B'00.
> 
> "reset mode" could be interpreted as "register contents are reset (lost)".
> However, the R-Car Gen3 documentation contains the same paragraph,
> and register contents are known not to be lost...

I remember (from the debugging session I've run few weeks ago) that I
checked on G1H an Ethernet register before point [1] and after point [3]
and the values were the same (but I may be wrong, I need to double check it).

I will double check also the value of MSTOP for Ethernet on RZ/G3S (though
I checked that this worked on my code), maybe RZ/G3S doesn't go to standby,
I have a bug in my code and that's why it works for RZ/G3S...

Also, I see that the STANDBY state is missing from CCC.OPC documentation
(chapter "37A.3.1 AVB-DMAC Operating Modes" on RZ/G1H vs "31.5.1 DMAC
Operating Modes" on RZ/G3S).

> 
> 37.7.2 for Ether ("sh-eth") states:
> 
>     After returning from the standby state, the ether should be reset
> and initialized.

Ok, I found that one in my G1H manual. It is not available on RZ/G3S
manual, though.

> 
> Sergey: does sh_eth.c really reinitialize the hardware completely after
> pm_runtime_get_sync()?
> 
>> Also, the manual of G1H states from some IPs that register state is
>> preserved in standby mode but not for AVB.
> 
> Indeed, AFAIK all modules on SH/R-Mobile, R-Car, and RZ/G SoCs keep
> their register contents when in standby-mode (module standby enabled).
> On modules in a (not always-on) power area (e.g. SH/R-Mobile), register
> contents are lost when the power area is powered down.
> So I'd be surprised if EtherAVB behaves differently.  Of course that
> is still possible, there are big difference between EtherAVB in R-Car
> Gen2 and RZ/G1, and the revision found in later SoC families.
> 
>>>> The reasons I've limited only to RZ/G3S are:
>>>> 1/ I don't have all the platforms to test it
>>>
>>>    That's a usual problem with the kernel development...
>>>
>>>> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
>>>>    platform at hand, only remotely, and is hardly to debug once the
>>>>    ethernet fails to work: probe is working(), open is executed, PHY is
>>>>    initialized and then TX/RX is not working... don't know why ATM.
>>>
>>>    That's why we have the long bug fixing period after -rc1...
>>
>> I prefer to not introduce any bug by intention.
> 
> Iff register contents are lost on RZ/G1H, I'd rather add
> an extra clk_prepare_enable(priv->clk) to ravb_probe() on
> "renesas,etheravb-rcar-gen2"....

This should work, though I would go with a pm_runtime_put_noidle().

Thank you,
Claudiu Beznea

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[claudiu beznea]

Hi, Geert,

On 27.11.2023 16:46, claudiu beznea wrote:
> Hi, Geert,
> 
> On 27.11.2023 16:05, Geert Uytterhoeven wrote:
>> Hi Claudiu,
>>
>> On Sat, Nov 25, 2023 at 12:00 AM claudiu beznea
>> <claudiu.beznea@tuxon.dev> wrote:
>>> On 23.11.2023 21:19, Sergey Shtylyov wrote:
>>>> On 11/23/23 8:04 PM, claudiu beznea wrote:
>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>
>>>>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>>>>> Ethernet module). For this commit adds runtime PM support which
>>>>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>>>>
>>>>>>    That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>>>>> RPM calls already in the probe() and remove() methods...
>>>>>> And the sh_eth driver
>>>>>> has RPM support since 2009...
>>>>>>
>>>>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>>>>
>>>>>>    I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>>>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>>>>> After that if the interface is brought up (though ravb_open()) then
>>>>>>> the clocks remain enabled until interface is brought down (operation
>>>>>>> done though ravb_close()).
>>>>>>>
>>>>>>> If any request arrives to the driver while the interface is down the
>>>>>>> clocks are enabled to serve the request and then disabled.
>>>>>>>
>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>>>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>>>>>>    unsigned magic_pkt:1;           /* E-MAC supports magic packet detection */
>>>>>>>    unsigned half_duplex:1;         /* E-MAC supports half duplex mode */
>>>>>>>    unsigned refclk_in_pd:1;        /* Reference clock is part of a power domain. */
>>>>>>> +  unsigned rpm:1;                 /* Runtime PM available. */
>>>>>>
>>>>>>    No, I don't think this flag makes any sense. We should support RPM
>>>>>> unconditionally...
>>>>
>>>>    If RPM calls work in the probe()/remove() methods, they should work
>>>> in the ndo_{open|stop}() methods, right?
>>>
>>> It might depend on hardware support... E.g.
>>>
>>> I debugged it further the issue I had with this implementation on other
>>> SoCs and it seems we cannot do RPM for those w/o reworking way the driver
>>> is configured.
>>>
>>> I wiped out the RPM code from this patch and just called:
>>>
>>> pm_runtime_put_sync();          // [1]
>>> usleep_range(300000, 400000);   // [2]
>>> pm_runtime_get_sync();          // [3]
>>>
>>> at the end of ravb_probe(); with this the interfaces fails to work. I
>>> continue debugging it and interrogated CSR and this returns RESET after
>>> [3]. I tried to switched it back to configuration mode after [3] but fails
>>> to restore to a proper working state.
>>>
>>> Then continued to debug it further to see what happens on the clock driver.
>>> The clk enable/disable reaches function at [4] which sets control_regs[reg]
>>> which is one of the System module stop control registers. Setting this
>>> activates module standby (AFICT). Switch to reset state on Ethernet IP
>>> might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
>>> chapter of the G1H HW manual (which I don't fully understand).
>>
>> You mean 37A.3.1.3 (2) "Transition during power-off by module standby"?
> 
> Yes!
> 
>>
>>     The AVB-DMAC completes the bus master access in progress,
>>     and then shifts to reset mode. At this time, the operating mode
>>     configuration bits in the AVB-DMAC mode register (CCC.OPC) are
>>     set to B'00.
>>
>> "reset mode" could be interpreted as "register contents are reset (lost)".
>> However, the R-Car Gen3 documentation contains the same paragraph,
>> and register contents are known not to be lost...
> 
> I remember (from the debugging session I've run few weeks ago) that I
> checked on G1H an Ethernet register before point [1] and after point [3]
> and the values were the same (but I may be wrong, I need to double check it).

I checked again DBAT before point [1] and after point [3]. Before point [1]
DBAT=0x6c040000, after point [3] DBAT=0x00000000.

However, if all the register settings done before point [1] are re-executed
after point [3] the Ethernet connection seems usable. I tried the above
settings after point [3] to confirm this:

        ravb_set_config_mode(ndev);

        usleep_range(1000, 2000);

        pr_err("%s(): 2: mode=%08x\n", __func__, ravb_read(ndev, CSR));



        if (info->gptp || info->ccc_gac) {

                /* Set GTI value */

                error = ravb_set_gti(ndev);

                if (error)

                        goto out_disable_refclk;



                /* Request GTI loading */

                ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);

        }



        if (info->internal_delay) {

                ravb_parse_delay_mode(np, ndev);

                ravb_set_delay_mode(ndev);

        }



        ravb_write(ndev, priv->desc_bat_dma, DBAT);



        /* Initialise PTP Clock driver */

        ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);

        ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);


However, I don't have a PTP setup to check.

> 
> I will double check also the value of MSTOP for Ethernet on RZ/G3S (though
> I checked that this worked on my code), maybe RZ/G3S doesn't go to standby,
> I have a bug in my code and that's why it works for RZ/G3S...

All is good in RZ/G3S. MSTOP is set accordingly and no issues.

Thank you,
Claudiu Beznea

> 
> Also, I see that the STANDBY state is missing from CCC.OPC documentation
> (chapter "37A.3.1 AVB-DMAC Operating Modes" on RZ/G1H vs "31.5.1 DMAC
> Operating Modes" on RZ/G3S).
> 
>>
>> 37.7.2 for Ether ("sh-eth") states:
>>
>>     After returning from the standby state, the ether should be reset
>> and initialized.
> 
> Ok, I found that one in my G1H manual. It is not available on RZ/G3S
> manual, though.
> 
>>
>> Sergey: does sh_eth.c really reinitialize the hardware completely after
>> pm_runtime_get_sync()?
>>
>>> Also, the manual of G1H states from some IPs that register state is
>>> preserved in standby mode but not for AVB.
>>
>> Indeed, AFAIK all modules on SH/R-Mobile, R-Car, and RZ/G SoCs keep
>> their register contents when in standby-mode (module standby enabled).
>> On modules in a (not always-on) power area (e.g. SH/R-Mobile), register
>> contents are lost when the power area is powered down.
>> So I'd be surprised if EtherAVB behaves differently.  Of course that
>> is still possible, there are big difference between EtherAVB in R-Car
>> Gen2 and RZ/G1, and the revision found in later SoC families.
>>
>>>>> The reasons I've limited only to RZ/G3S are:
>>>>> 1/ I don't have all the platforms to test it
>>>>
>>>>    That's a usual problem with the kernel development...
>>>>
>>>>> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
>>>>>    platform at hand, only remotely, and is hardly to debug once the
>>>>>    ethernet fails to work: probe is working(), open is executed, PHY is
>>>>>    initialized and then TX/RX is not working... don't know why ATM.
>>>>
>>>>    That's why we have the long bug fixing period after -rc1...
>>>
>>> I prefer to not introduce any bug by intention.
>>
>> Iff register contents are lost on RZ/G1H, I'd rather add
>> an extra clk_prepare_enable(priv->clk) to ravb_probe() on
>> "renesas,etheravb-rcar-gen2"....
> 
> This should work, though I would go with a pm_runtime_put_noidle().
> 
> Thank you,
> Claudiu Beznea
> 
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[Sergey Shtylyov]

On 11/27/23 5:05 PM, Geert Uytterhoeven wrote:

[...]
>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>
>>>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>>>> Ethernet module). For this commit adds runtime PM support which
>>>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>>>
>>>>>    That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>>>> RPM calls already in the probe() and remove() methods...
>>>>> And the sh_eth driver
>>>>> has RPM support since 2009...
>>>>>
>>>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>>>
>>>>>    I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>>>> After that if the interface is brought up (though ravb_open()) then
>>>>>> the clocks remain enabled until interface is brought down (operation
>>>>>> done though ravb_close()).
>>>>>>
>>>>>> If any request arrives to the driver while the interface is down the
>>>>>> clocks are enabled to serve the request and then disabled.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

[...]

> Sergey: does sh_eth.c really reinitialize the hardware completely after
> pm_runtime_get_sync()?

   Well, even with the original Magnus' commit that added the RPM support (bcd5149ded6b2edbf3732fa1483600a716b1cba6) it wasn't so -- sh_eth_open()
indeed seemed to re-init everything (but not TSU!) but sh_eth_get_stats()
surely didn't (the RPM calls there have been removed since); other RPM
"wrappers" have been added to the driver methods since -- which also
don't init anything... thus the comment in sh_eth_runtime_nop(() seems
to be wrong from the very start...

[...]

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergey

Re: [PATCH 13/13] net: ravb: Add runtime PM support

[Simon Horman]

On Mon, Nov 20, 2023 at 10:46:06AM +0200, Claudiu wrote:

...

>  		}
>  	}
>  
> +	error = ravb_pm_runtime_get(priv);
> +	if (error < 0)
> +		return error;

Hi Claudiu,

the error handling doesn't seem right here.
I think you need:

		goto out_free_irq_mgmta;

> +
>  	/* Device init */
>  	error = ravb_dmac_init(ndev);
>  	if (error)
> -		goto out_free_irq_mgmta;
> +		goto pm_runtime_put;
>  	ravb_emac_init(ndev);
>  
>  	/* Initialise PTP Clock driver */
> @@ -1820,7 +1862,8 @@ static int ravb_open(struct net_device *ndev)
>  	if (info->gptp)
>  		ravb_ptp_stop(ndev);
>  	ravb_stop_dma(ndev);
> -out_free_irq_mgmta:
> +pm_runtime_put:
> +	ravb_pm_runtime_put(priv);

And the out_free_irq_mgmta label should go here.

Flagged by Smatch.

>  	if (!info->multi_irqs)
>  		goto out_free_irq;
>  	if (info->err_mgmt_irqs)

...

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

[Sergey Shtylyov]

On 11/20/23 11:45 AM, 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 RZ/G3S (R9A08G045) SoC.
> 
> Along with it series contains preparatory fixes and cleanups.
> 
> Thank you,
> Claudiu Beznea
> 
> Claudiu Beznea (13):
>   net: ravb: Check return value of reset_control_deassert()
>   net: ravb: Use pm_runtime_resume_and_get()
>   net: ravb: Make write access to CXR35 first before accessing other
>     EMAC registers
>   net: ravb: Start TX queues after HW initialization succeeded
>   net: ravb: Stop DMA in case of failures on ravb_open()

   Like I've said, the fixes should be submitted separately.

>   net: ravb: Let IP specific receive function to interrogate descriptors
>   net: ravb: Rely on PM domain to enable gptp_clk
>   net: ravb: Rely on PM domain to enable refclk
>   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/deassert reset on suspend/resume
>   net: ravb: Add runtime PM support

   I'll continue reviewing the rest of the series tomorrow...

[...]

MBR, Sergey