Re: [PATCH net-next 6/7] net: stmmac: Fix comment about default addend calculation

[Serge Semin]

Hi Andrew

On Thu, Aug 24, 2023 at 01:32:57PM -0500, Andrew Halaney wrote:
> The comment neglects that freq_div_ratio is the ratio between
> the subsecond increment frequency and the clk_ptp_rate frequency.
> 
> Signed-off-by: Andrew Halaney <ahalaney@xxxxxxxxxx>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index dfead0df6163..64185753865f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -853,10 +853,12 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
>  	/* Store sub second increment for later use */
>  	priv->sub_second_inc = sub_second_inc;
>  

> -	/* calculate default addend value:
> -	 * formula is :
> -	 * addend = (2^32)/freq_div_ratio;
> -	 * where, freq_div_ratio = 1e9ns/sub_second_inc
> +	/* Calculate default addend so the accumulator overflows (2^32) in
> +	 * sub_second_inc (ns). The addend is added to the accumulator
> +	 * every clk_ptp cycle.
> +	 *
> +	 * addend = (2^32) / freq_div_ratio
> +	 * where, freq_div_ratio = (1e9ns / sub_second_inc) / clk_ptp_rate
>  	 */
>  	temp = div_u64(NSEC_PER_SEC, sub_second_inc);
>  	temp = temp << 32;

I am not well familiar with the way PTP works but at my naked eyes the
calculation implemented here looks a bit different than what is
described in the comment.

Basically config_sub_second_increment(clk_ptp_rate, sub_second_inc)
returns clk_ptp_rate period in nanoseconds or twice that period, or have it
scaled up on 0.465. So we have one of the next formulae:
X1 = NSEC_PER_SEC / clk_ptp_rate
X2 = 2 * NSEC_PER_SEC / clk_ptp_rate
X3 = X1 / 0.465
X4 = X2 / 0.465

Then stmmac_init_tstamp_counter() handles the retrieved period in the
next manner:
temp = div_u64(NSEC_PER_SEC, sub_second_inc);     // Convert back to frequency
temp = temp << 32;                                // multiply by 2^32
addend = div_u64(temp, priv->plat->clk_ptp_rate); // Divide by clk_ptp_rate

The code above is equivalent:

addend = ((NSEC_PER_SEC / X) * 2^32 ) / clk_ptp_rate = 
         (2^32 * NSEC_PER_SEC / X) / clk_ptp_rate = 
         2^32 / (clk_ptp_rate / (NSEC_PER_SEC / X))

AFAICS this doesn't match to what is in the comment (X = sub_second_inc).
freq_div_ratio gets to be inverted. Does it?

Substituting X to the formulae above we'll have just four possible results:
addend1 = 2^32
addend2 = 2^32 / 2
addend3 = 0.465 * 2^32
addend4 = 0.465 * 2^32 / 2

So basically clk_ptp_rate is irrelevant (neglecting all the
integer divisions rounding). Is that what implied by the implemented
algo?

Am I missing something? (it's quite possible since it's long past
midnight already.)

-Serge(y)

> 
> -- 
> 2.41.0
> 
>