Messages in this thread | | | Date | Sun, 27 Aug 2023 03:02:07 +0300 | From | Serge Semin <> | Subject | Re: [PATCH net-next 6/7] net: stmmac: Fix comment about default addend calculation |
| |
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@redhat.com> > --- > 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 > >
| |