Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Thu, 14 Dec 2023 13:33:15 +0100
From:   Niklas =?utf-8?Q?S=C3=B6derlund?= 
        <niklas.soderlund+renesas@ragnatech.se>
To:     Claudiu <claudiu.beznea@tuxon.dev>
Cc:     s.shtylyov@omp.ru, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com,
        claudiu.beznea.uj@bp.renesas.com, yoshihiro.shimoda.uh@renesas.com,
        wsa+renesas@sang-engineering.com, biju.das.jz@bp.renesas.com,
        prabhakar.mahadev-lad.rj@bp.renesas.com,
        mitsuhiro.kimura.kc@renesas.com, geert+renesas@glider.be,
        netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net 2/2] net: ravb: Check that GTI loading request is done
Message-ID: <20231214123315.GL1863068@ragnatech.se>
References: <20231214113137.2450292-1-claudiu.beznea.uj@bp.renesas.com>
 <20231214113137.2450292-3-claudiu.beznea.uj@bp.renesas.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231214113137.2450292-3-claudiu.beznea.uj@bp.renesas.com>
Precedence: bulk

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
> 

-- 
Kind Regards,
Niklas Söderlund