diff mbox series

[net,2/2] net: ravb: Check that GTI loading request is done

Message ID 20231214113137.2450292-3-claudiu.beznea.uj@bp.renesas.com
State New
Headers
Series net: ravb: fixes for the ravb driver |

Commit Message

claudiu beznea Dec. 14, 2023, 11:31 a.m. UTC 
  From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hardware manual specifies the following for GCCR.LTI bit:
0: Setting completed
1: When written: Issue a configuration request.
When read: Completion of settings is pending

Thus, check the completion status when setting 1 to GCCR.LTI.

Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L")
Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code")
Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Niklas Söderlund Dec. 14, 2023, 12:33 p.m. UTC | #1 
Hi Claudiu,

Thanks for your work.

On 2023-12-14 13:31:37 +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Hardware manual specifies the following for GCCR.LTI bit:
> 0: Setting completed
> 1: When written: Issue a configuration request.
> When read: Completion of settings is pending

This is hard to parse at first glance, the last row have odd indentation 
and the mixing of read and write info is odd. I know this reflects how 
it's written in the datasheet, but at least there the indentation is 
correct. Also missing here is the fact that only 1 can be written to the 
bit.

> 
> Thus, check the completion status when setting 1 to GCCR.LTI.

Can you describe in the commit why this fix is needed. I agree it is, 
but would be nice to record why. As this have a fixes tags have you hit 
an issue? Or are you correcting the driver based on the datasheet?

Maybe a more informative commit message could be to describe the change 
and why it's needed instead of the register layout?

  The driver do not wait for the confirmation of the configuring request 
  of the gPTP timer increment before moving on. Add a check to make sure 
  the request completes successfully.

> 
> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L")
> Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code")
> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ce95eb5af354..1c253403a297 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2819,6 +2819,10 @@ static int ravb_probe(struct platform_device *pdev)
>  
>  		/* Request GTI loading */
>  		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +		/* Check completion status. */
> +		error = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
> +		if (error)
> +			goto out_disable_refclk;

nit: Maybe create a helper for this so future fixes only need to be 
addressed in one location?

>  	}
>  
>  	if (info->internal_delay) {
> @@ -3041,6 +3045,10 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  
>  		/* Request GTI loading */
>  		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +		/* Check completion status. */
> +		ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	if (info->internal_delay)
> -- 
> 2.39.2
>
claudiu beznea Dec. 14, 2023, 1:02 p.m. UTC | #2 
On 14.12.2023 14:33, Niklas Söderlund wrote:
> Hi Claudiu,
> 
> Thanks for your work.
> 
> On 2023-12-14 13:31:37 +0200, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Hardware manual specifies the following for GCCR.LTI bit:
>> 0: Setting completed
>> 1: When written: Issue a configuration request.
>> When read: Completion of settings is pending
> 
> This is hard to parse at first glance, the last row have odd indentation 
> and the mixing of read and write info is odd. I know this reflects how 
> it's written in the datasheet, but at least there the indentation is 
> correct. Also missing here is the fact that only 1 can be written to the 
> bit.
> 
>>
>> Thus, check the completion status when setting 1 to GCCR.LTI.
> 
> Can you describe in the commit why this fix is needed. I agree it is, 
> but would be nice to record why. As this have a fixes tags have you hit 
> an issue? Or are you correcting the driver based on the datasheet?

Yes, this is a issue I faced while working on runtime PM support for this
driver.

> 
> Maybe a more informative commit message could be to describe the change 
> and why it's needed instead of the register layout?

ok

> 
>   The driver do not wait for the confirmation of the configuring request 
>   of the gPTP timer increment before moving on. Add a check to make sure 
>   the request completes successfully.
> 
>>
>> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L")
>> Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code")
>> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index ce95eb5af354..1c253403a297 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2819,6 +2819,10 @@ static int ravb_probe(struct platform_device *pdev)
>>  
>>  		/* Request GTI loading */
>>  		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>> +		/* Check completion status. */
>> +		error = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
>> +		if (error)
>> +			goto out_disable_refclk;
> 
> nit: Maybe create a helper for this so future fixes only need to be 
> addressed in one location?

The code intended for runtime PM support addresses this.

> 
>>  	}
>>  
>>  	if (info->internal_delay) {
>> @@ -3041,6 +3045,10 @@ static int __maybe_unused ravb_resume(struct device *dev)
>>  
>>  		/* Request GTI loading */
>>  		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>> +		/* Check completion status. */
>> +		ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
>> +		if (ret)
>> +			return ret;
>>  	}
>>  
>>  	if (info->internal_delay)
>> -- 
>> 2.39.2
>>
>
Sergey Shtylyov Dec. 14, 2023, 8:22 p.m. UTC | #3 
On 12/14/23 2:31 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Hardware manual specifies the following for GCCR.LTI bit:
> 0: Setting completed
> 1: When written: Issue a configuration request.
> When read: Completion of settings is pending
> 
> Thus, check the completion status when setting 1 to GCCR.LTI.

   But do we really need to? Seems quite dubious... currently we
just let it the loading complete asynchronously...

> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L")
> Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code")
> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ce95eb5af354..1c253403a297 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2819,6 +2819,10 @@ static int ravb_probe(struct platform_device *pdev)
>  
>  		/* Request GTI loading */
>  		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +		/* Check completion status. */
> +		error = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
> +		if (error)
> +			goto out_disable_refclk;
>  	}
>  
>  	if (info->internal_delay) {
> @@ -3041,6 +3045,10 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  
>  		/* Request GTI loading */
>  		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +		/* Check completion status. */
> +		ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	if (info->internal_delay)
> 

   BTW, seems worth factoring out into a separate function...

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ce95eb5af354..1c253403a297 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2819,6 +2819,10 @@  static int ravb_probe(struct platform_device *pdev)
 
 		/* Request GTI loading */
 		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+		/* Check completion status. */
+		error = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
+		if (error)
+			goto out_disable_refclk;
 	}
 
 	if (info->internal_delay) {
@@ -3041,6 +3045,10 @@  static int __maybe_unused ravb_resume(struct device *dev)
 
 		/* Request GTI loading */
 		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+		/* Check completion status. */
+		ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
+		if (ret)
+			return ret;
 	}
 
 	if (info->internal_delay)