On 2/9/24 8:04 PM, Claudiu wrote:
> From: Claudiu Beznea <[email protected]>
>
> Add runtime PM support for the ravb driver. As the driver is used by
> different IP variants, with different behaviors, to be able to have the
> runtime PM support available for all devices, the preparatory commits
> moved all the resources parsing and allocations in the driver's probe
> function and kept the settings for ravb_open(). This is due to the fact
> that on some IP variants-platforms tuples disabling/enabling the clocks
> will switch the IP to the reset operation mode where registers' content is
This pesky "registers' content" somehow evaded me -- should be "register
contents" as well...
> lost and reconfiguration needs to be done. For this the rabv_open()
> function enables the clocks, switches the IP to configuration mode, applies
> all the registers settings and switches the IP to the operational mode. At
> the end of ravb_open() IP is ready to send/receive data.
>
> In ravb_close() necessary reverts are done (compared with ravb_open()), the
> IP is switched to reset mode and clocks are disabled.
>
> The ethtool APIs or IOCTLs that might execute while the interface is down
> are either cached (and applied in ravb_open()) or rejected (as at that time
> the IP is in reset mode). Keeping the IP in the reset mode also increases
> the power saved (according to the hardware manual).
>
> Signed-off-by: Claudiu Beznea <[email protected]>
> Reviewed-by: Sergey Shtylyov <[email protected]>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index f4be08f0198d..5bbfdfeef8a9 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> const struct ravb_hw_info *info = priv->info;
> + struct device *dev = &priv->pdev->dev;
> int error;
>
> napi_enable(&priv->napi[RAVB_BE]);
> if (info->nc_queues)
> napi_enable(&priv->napi[RAVB_NC]);
>
> + error = pm_runtime_resume_and_get(dev);
> + if (error < 0)
> + goto out_napi_off;
Well, s/error/ret/ -- it would fit better here...
[...]
> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev)
> struct net_device *ndev = platform_get_drvdata(pdev);
> struct ravb_private *priv = netdev_priv(ndev);
> const struct ravb_hw_info *info = priv->info;
> + struct device *dev = &priv->pdev->dev;
> + int error;
> +
> + error = pm_runtime_resume_and_get(dev);
> + if (error < 0)
> + return;
Again, s/erorr/ret/ in this case.
[...]
MBR, Sergey
On 09.02.2024 23:00, Sergey Shtylyov wrote:
> On 2/9/24 8:04 PM, Claudiu wrote:
>
>> From: Claudiu Beznea <[email protected]>
>>
>> Add runtime PM support for the ravb driver. As the driver is used by
>> different IP variants, with different behaviors, to be able to have the
>> runtime PM support available for all devices, the preparatory commits
>> moved all the resources parsing and allocations in the driver's probe
>> function and kept the settings for ravb_open(). This is due to the fact
>> that on some IP variants-platforms tuples disabling/enabling the clocks
>> will switch the IP to the reset operation mode where registers' content is
>
> This pesky "registers' content" somehow evaded me -- should be "register
> contents" as well...
>
>> lost and reconfiguration needs to be done. For this the rabv_open()
>> function enables the clocks, switches the IP to configuration mode, applies
>> all the registers settings and switches the IP to the operational mode. At
>> the end of ravb_open() IP is ready to send/receive data.
>>
>> In ravb_close() necessary reverts are done (compared with ravb_open()), the
>> IP is switched to reset mode and clocks are disabled.
>>
>> The ethtool APIs or IOCTLs that might execute while the interface is down
>> are either cached (and applied in ravb_open()) or rejected (as at that time
>> the IP is in reset mode). Keeping the IP in the reset mode also increases
>> the power saved (according to the hardware manual).
>>
>> Signed-off-by: Claudiu Beznea <[email protected]>
>> Reviewed-by: Sergey Shtylyov <[email protected]>
> [...]
>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index f4be08f0198d..5bbfdfeef8a9 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev)
>> {
>> struct ravb_private *priv = netdev_priv(ndev);
>> const struct ravb_hw_info *info = priv->info;
>> + struct device *dev = &priv->pdev->dev;
>> int error;
>>
>> napi_enable(&priv->napi[RAVB_BE]);
>> if (info->nc_queues)
>> napi_enable(&priv->napi[RAVB_NC]);
>>
>> + error = pm_runtime_resume_and_get(dev);
>> + if (error < 0)
>> + goto out_napi_off;
>
> Well, s/error/ret/ -- it would fit better here...
Using error is the "trademark" of this driver, it is used all around the
driver. I haven't introduced it here, I don't like it. The variable error
in this particular function is here from the beginning of the driver.
So, I don't consider changing error to ret is the scope of this series.
>
> [...]
>> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev)
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct ravb_private *priv = netdev_priv(ndev);
>> const struct ravb_hw_info *info = priv->info;
>> + struct device *dev = &priv->pdev->dev;
>> + int error;
>> +
>> + error = pm_runtime_resume_and_get(dev);
>> + if (error < 0)
>> + return;
>
> Again, s/erorr/ret/ in this case.
error was used here to comply with the rest of the driver. So, if you still
want me to change it here and in ravb_remove() please confirm.
Thank you,
Claudiu Beznea
>
> [...]
>
> MBR, Sergey
On 2/12/24 10:56 AM, claudiu beznea wrote:
[...]
>>> From: Claudiu Beznea <[email protected]>
>>>
>>> Add runtime PM support for the ravb driver. As the driver is used by
>>> different IP variants, with different behaviors, to be able to have the
>>> runtime PM support available for all devices, the preparatory commits
>>> moved all the resources parsing and allocations in the driver's probe
>>> function and kept the settings for ravb_open(). This is due to the fact
>>> that on some IP variants-platforms tuples disabling/enabling the clocks
>>> will switch the IP to the reset operation mode where registers' content is
>>
>> This pesky "registers' content" somehow evaded me -- should be "register
>> contents" as well...
>>
>>> lost and reconfiguration needs to be done. For this the rabv_open()
>>> function enables the clocks, switches the IP to configuration mode, applies
>>> all the registers settings and switches the IP to the operational mode. At
>>> the end of ravb_open() IP is ready to send/receive data.
>>>
>>> In ravb_close() necessary reverts are done (compared with ravb_open()), the
>>> IP is switched to reset mode and clocks are disabled.
>>>
>>> The ethtool APIs or IOCTLs that might execute while the interface is down
>>> are either cached (and applied in ravb_open()) or rejected (as at that time
>>> the IP is in reset mode). Keeping the IP in the reset mode also increases
>>> the power saved (according to the hardware manual).
>>>
>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>> Reviewed-by: Sergey Shtylyov <[email protected]>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index f4be08f0198d..5bbfdfeef8a9 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev)
>>> {
>>> struct ravb_private *priv = netdev_priv(ndev);
>>> const struct ravb_hw_info *info = priv->info;
>>> + struct device *dev = &priv->pdev->dev;
>>> int error;
>>>
>>> napi_enable(&priv->napi[RAVB_BE]);
>>> if (info->nc_queues)
>>> napi_enable(&priv->napi[RAVB_NC]);
>>>
>>> + error = pm_runtime_resume_and_get(dev);
>>> + if (error < 0)
>>> + goto out_napi_off;
>>
>> Well, s/error/ret/ -- it would fit better here...
>
> Using error is the "trademark" of this driver, it is used all around the
> driver. I haven't introduced it here, I don't like it. The variable error
Heh, because it's my usual style. Too bad you don't like it... :-)
> in this particular function is here from the beginning of the driver.
I think it's well suited for the functions returning either 0 or a
(negative) error code. It's *if* (error < 0) that confuses me (as this
API can return positive numbers in case of success...
> So, I don't consider changing error to ret is the scope of this series.
OK, you're probably right... are you going to respin the series because
of Biju's comments?
[...]
>>> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev)
>>> struct net_device *ndev = platform_get_drvdata(pdev);
>>> struct ravb_private *priv = netdev_priv(ndev);
>>> const struct ravb_hw_info *info = priv->info;
>>> + struct device *dev = &priv->pdev->dev;
>>> + int error;
>>> +
>>> + error = pm_runtime_resume_and_get(dev);
>>> + if (error < 0)
>>> + return;
>>
>> Again, s/erorr/ret/ in this case.
>
> error was used here to comply with the rest of the driver. So, if you still
> want me to change it here and in ravb_remove() please confirm.
No, we are good enough without that; I'll consider doing a cleanup
when/if I have time. :-)
> Thank you,
> Claudiu Beznea
MBR, Sergey
On 12.02.2024 22:19, Sergey Shtylyov wrote:
> On 2/12/24 10:56 AM, claudiu beznea wrote:
>
> [...]
>
>>>> From: Claudiu Beznea <[email protected]>
>>>>
>>>> Add runtime PM support for the ravb driver. As the driver is used by
>>>> different IP variants, with different behaviors, to be able to have the
>>>> runtime PM support available for all devices, the preparatory commits
>>>> moved all the resources parsing and allocations in the driver's probe
>>>> function and kept the settings for ravb_open(). This is due to the fact
>>>> that on some IP variants-platforms tuples disabling/enabling the clocks
>>>> will switch the IP to the reset operation mode where registers' content is
>>>
>>> This pesky "registers' content" somehow evaded me -- should be "register
>>> contents" as well...
>>>
>>>> lost and reconfiguration needs to be done. For this the rabv_open()
>>>> function enables the clocks, switches the IP to configuration mode, applies
>>>> all the registers settings and switches the IP to the operational mode. At
>>>> the end of ravb_open() IP is ready to send/receive data.
>>>>
>>>> In ravb_close() necessary reverts are done (compared with ravb_open()), the
>>>> IP is switched to reset mode and clocks are disabled.
>>>>
>>>> The ethtool APIs or IOCTLs that might execute while the interface is down
>>>> are either cached (and applied in ravb_open()) or rejected (as at that time
>>>> the IP is in reset mode). Keeping the IP in the reset mode also increases
>>>> the power saved (according to the hardware manual).
>>>>
>>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>>> Reviewed-by: Sergey Shtylyov <[email protected]>
>>> [...]
>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index f4be08f0198d..5bbfdfeef8a9 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev)
>>>> {
>>>> struct ravb_private *priv = netdev_priv(ndev);
>>>> const struct ravb_hw_info *info = priv->info;
>>>> + struct device *dev = &priv->pdev->dev;
>>>> int error;
>>>>
>>>> napi_enable(&priv->napi[RAVB_BE]);
>>>> if (info->nc_queues)
>>>> napi_enable(&priv->napi[RAVB_NC]);
>>>>
>>>> + error = pm_runtime_resume_and_get(dev);
>>>> + if (error < 0)
>>>> + goto out_napi_off;
>>>
>>> Well, s/error/ret/ -- it would fit better here...
>>
>> Using error is the "trademark" of this driver, it is used all around the
>> driver. I haven't introduced it here, I don't like it. The variable error
>
> Heh, because it's my usual style. Too bad you don't like it... :-)
>
>> in this particular function is here from the beginning of the driver.
>
> I think it's well suited for the functions returning either 0 or a
> (negative) error code. It's *if* (error < 0) that confuses me (as this
> API can return positive numbers in case of success...
>
>> So, I don't consider changing error to ret is the scope of this series.
>
> OK, you're probably right... are you going to respin the series because
> of Biju's comments?
Yes!
Thank you,
Claudiu Beznea
>
> [...]
>>>> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev)
>>>> struct net_device *ndev = platform_get_drvdata(pdev);
>>>> struct ravb_private *priv = netdev_priv(ndev);
>>>> const struct ravb_hw_info *info = priv->info;
>>>> + struct device *dev = &priv->pdev->dev;
>>>> + int error;
>>>> +
>>>> + error = pm_runtime_resume_and_get(dev);
>>>> + if (error < 0)
>>>> + return;
>>>
>>> Again, s/erorr/ret/ in this case.
>>
>> error was used here to comply with the rest of the driver. So, if you still
>> want me to change it here and in ravb_remove() please confirm.
>
> No, we are good enough without that; I'll consider doing a cleanup
> when/if I have time. :-)
>
>> Thank you,
>> Claudiu Beznea
>
> MBR, Sergey