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

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

Posted by Claudiu 2 years ago

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

Posted by Sergey Shtylyov 2 years ago

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

Posted by claudiu beznea 2 years ago


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

Posted by Sergey Shtylyov 2 years ago

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