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

[Andrew Halaney]

On Sun, Aug 27, 2023 at 03:02:07AM +0300, Serge Semin wrote:
> 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

X5 = PTP_SSIR_SSINC_MAX (0xFF) is a case as well to consider
> 
> 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?

You're right, my comment needs to be inverted to match all of the above
(which is a great recap, thank you!).

> 
> 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

addend5 = 2^32 / (clk_ptp_rate / (NSEC_PER_SEC / 0xFF))

I think that would be the PTP_SSIR_SSINC_MAX case (X5) I inserted above

> 
> 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.)

I believe you've captured everything, minus the one conditional I added.

I think because of that conditional we can't just nicely code up some
contants here independent of sub_second_inc. Now I can blame the morning
and not enough coffee, do you see anything wrong with that thought
process? I'm all ears for suggestions for cleaning this up, especially
since others like Richard have indicated that it could use some love,
but right now I'm hung up thinking the best I can do is fix the bad
comment in this patch.

Thanks for the review!
- Andrew