[PATCH net-next 0/7] net: stmmac: Improve default addend/subsecond increment readability

[PATCH net-next 0/7] net: stmmac: Improve default addend/subsecond increment readability

[Andrew Halaney]

This series aims to improve the readability of the calculations
for the default addend and subsecond increment values.

I recently had to understand what the hardware did by reading this code,
and it took me longer than I care to admit. These patches aim to make it
more self explanatory.

Suggestions to further improve this are very welcomed.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
Andrew Halaney (7):
      net: stmmac: Use consistent variable name for subsecond increment
      net: stmmac: Use NSEC_PER_SEC for hwtstamp calculations
      net: stmmac: Precede entire addend calculation with its comment
      net: stmmac: Remove a pointless cast
      net: stmmac: Correct addend typo
      net: stmmac: Fix comment about default addend calculation
      net: stmmac: Make PTP reference clock references more clear

 drivers/net/ethernet/stmicro/stmmac/hwif.h         |  5 +++--
 .../net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c  | 14 +++++++-------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 22 ++++++++++++----------
 3 files changed, 22 insertions(+), 19 deletions(-)
---
base-commit: 9f6708a668186dc5b38532fc1d1ff2f5311722d6
change-id: 20230824-stmmac-subsecond-inc-cleanup-305397c6ca8d

Best regards,
-- 
Andrew Halaney <ahalaney@redhat.com>

[PATCH net-next 1/7] net: stmmac: Use consistent variable name for subsecond increment

[Andrew Halaney]

Subsecond increment is the name of the field being programmed.
Let's stop using a bunch of variations of that name and just
use sub_second_inc throughout.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/hwif.h            |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c |  6 +++---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     | 10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 238f17c50a1e..bd607da65037 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -524,7 +524,7 @@ struct stmmac_ops {
 struct stmmac_hwtimestamp {
 	void (*config_hw_tstamping) (void __iomem *ioaddr, u32 data);
 	void (*config_sub_second_increment)(void __iomem *ioaddr, u32 ptp_clock,
-					   int gmac4, u32 *ssinc);
+					   int gmac4, u32 *sub_second_inc);
 	int (*init_systime) (void __iomem *ioaddr, u32 sec, u32 nsec);
 	int (*config_addend) (void __iomem *ioaddr, u32 addend);
 	int (*adjust_systime) (void __iomem *ioaddr, u32 sec, u32 nsec,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index 540f6a4ec0b8..6dcf8582a70e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -24,7 +24,7 @@ static void config_hw_tstamping(void __iomem *ioaddr, u32 data)
 }
 
 static void config_sub_second_increment(void __iomem *ioaddr,
-		u32 ptp_clock, int gmac4, u32 *ssinc)
+		u32 ptp_clock, int gmac4, u32 *sub_second_inc)
 {
 	u32 value = readl(ioaddr + PTP_TCR);
 	unsigned long data;
@@ -56,8 +56,8 @@ static void config_sub_second_increment(void __iomem *ioaddr,
 
 	writel(reg_value, ioaddr + PTP_SSIR);
 
-	if (ssinc)
-		*ssinc = data;
+	if (sub_second_inc)
+		*sub_second_inc = data;
 }
 
 static void hwtstamp_correct_latency(struct stmmac_priv *priv)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7a9bbcf03ea5..67e4f65f0f68 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -836,7 +836,7 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
 {
 	bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
 	struct timespec64 now;
-	u32 sec_inc = 0;
+	u32 sub_second_inc = 0;
 	u64 temp = 0;
 
 	if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp))
@@ -848,16 +848,16 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
 	/* program Sub Second Increment reg */
 	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
 					   priv->plat->clk_ptp_rate,
-					   xmac, &sec_inc);
-	temp = div_u64(1000000000ULL, sec_inc);
+					   xmac, &sub_second_inc);
+	temp = div_u64(1000000000ULL, sub_second_inc);
 
 	/* Store sub second increment for later use */
-	priv->sub_second_inc = sec_inc;
+	priv->sub_second_inc = sub_second_inc;
 
 	/* calculate default added value:
 	 * formula is :
 	 * addend = (2^32)/freq_div_ratio;
-	 * where, freq_div_ratio = 1e9ns/sec_inc
+	 * where, freq_div_ratio = 1e9ns/sub_second_inc
 	 */
 	temp = (u64)(temp << 32);
 	priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);

-- 
2.41.0

[PATCH net-next 2/7] net: stmmac: Use NSEC_PER_SEC for hwtstamp calculations

[Andrew Halaney]

This makes it more clear what unit conversions are happening.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 6 +++---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index 6dcf8582a70e..29fd51bb853d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -36,12 +36,12 @@ static void config_sub_second_increment(void __iomem *ioaddr,
 	 * to mid-range = 2^31 when the remainder of this division is zero,
 	 * which will make the accumulator overflow once every 2 ptp_clock
 	 * cycles, adding twice the number of nanoseconds of a clock cycle :
-	 * 2000000000ULL / ptp_clock.
+	 * 2 * NSEC_PER_SEC / ptp_clock.
 	 */
 	if (value & PTP_TCR_TSCFUPDT)
-		data = (2000000000ULL / ptp_clock);
+		data = (2 * NSEC_PER_SEC / ptp_clock);
 	else
-		data = (1000000000ULL / ptp_clock);
+		data = (NSEC_PER_SEC / ptp_clock);
 
 	/* 0.465ns accuracy */
 	if (!(value & PTP_TCR_TSCTRLSSR))
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 67e4f65f0f68..ba38ca284e26 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -849,7 +849,7 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
 	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
 					   priv->plat->clk_ptp_rate,
 					   xmac, &sub_second_inc);
-	temp = div_u64(1000000000ULL, sub_second_inc);
+	temp = div_u64(NSEC_PER_SEC, sub_second_inc);
 
 	/* Store sub second increment for later use */
 	priv->sub_second_inc = sub_second_inc;

-- 
2.41.0

[PATCH net-next 3/7] net: stmmac: Precede entire addend calculation with its comment

[Andrew Halaney]

The addend calculation is currently split. The variable to be programmed
is first altered, then a comment explaining the full calculation is
seen, then the variable is altered further before the calculation is
finished.

Make the comment the first thing read. This makes the conversion of
sub_second_increment from nanoseconds to hertz much easier to
understand and reads logically.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ba38ca284e26..f0e585e6ef76 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -849,7 +849,6 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
 	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
 					   priv->plat->clk_ptp_rate,
 					   xmac, &sub_second_inc);
-	temp = div_u64(NSEC_PER_SEC, sub_second_inc);
 
 	/* Store sub second increment for later use */
 	priv->sub_second_inc = sub_second_inc;
@@ -859,6 +858,7 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
 	 * addend = (2^32)/freq_div_ratio;
 	 * where, freq_div_ratio = 1e9ns/sub_second_inc
 	 */
+	temp = div_u64(NSEC_PER_SEC, sub_second_inc);
 	temp = (u64)(temp << 32);
 	priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
 	stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);

-- 
2.41.0

[PATCH net-next 4/7] net: stmmac: Remove a pointless cast

[Andrew Halaney]

The type is already u64, there's no reason to cast it again.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f0e585e6ef76..20ef068b3e6b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -859,7 +859,7 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
 	 * where, freq_div_ratio = 1e9ns/sub_second_inc
 	 */
 	temp = div_u64(NSEC_PER_SEC, sub_second_inc);
-	temp = (u64)(temp << 32);
+	temp = temp << 32;
 	priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
 	stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);
 

-- 
2.41.0

[PATCH net-next 5/7] net: stmmac: Correct addend typo

[Andrew Halaney]

added should be addend in this context.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 20ef068b3e6b..dfead0df6163 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -853,7 +853,7 @@ 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 added value:
+	/* calculate default addend value:
 	 * formula is :
 	 * addend = (2^32)/freq_div_ratio;
 	 * where, freq_div_ratio = 1e9ns/sub_second_inc

-- 
2.41.0

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

[Andrew Halaney]

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;

-- 
2.41.0

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

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

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

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

[Serge Semin]

[-- Attachment #1: Type: text/plain, Size: 6478 bytes --]

On Tue, Aug 29, 2023 at 10:01:20AM -0500, Andrew Halaney wrote:
> 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@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
> 

> X5 = PTP_SSIR_SSINC_MAX (0xFF) is a case as well to consider

I noticed that option too, but then I thought it must have been not
that much probable to be considered as a real case seeing it's a
boundary case. The clamping happens if
if (X1 > 255 || X2 > 255 || X3 > 255 || X4 > 255)
	X5 = 255
so in the worst case PTP-rate period in nanoseconds multiplied by 4.3
must be greater than 255 which is equivalent to X1 >= 60. It means
PTP clock rate must be greater than 16.6MHz to avoid the clamping. In
the best case - 3.9MHz. I doubted that these limits are crossed in
reality. But in anyways you are right saying that it still needs to be
taken into account in case if the implemented algo would be a subject
for optimizations.

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

Good. Then an hour spent for decyphering of that stuff wasn't a waste
of time after all.)

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

I am not that much aware of the PTP internals but it just seems weird
to have clk_ptp_rate not affecting anything except the boundary case.
Do you have a DW *MAC HW databook with the PTP-engine chapter
describing the way the System Time Register Module works?

> process? I'm all ears for suggestions for cleaning this up, especially
> since others like Richard have indicated that it could use some love,

* I would have said more definitive - some _hard_ love.)

> but right now I'm hung up thinking the best I can do is fix the bad
> comment in this patch.

Just at the first very swift glance:
1. See attached patch.
2. Exporting stmmac_init_tstamp_counter() isn't necessary. It doesn't
seem like being utilized anywhere except in the stmmac_main.c module.
3. stmmac_hwtimestamp-based abstraction seems redundant since: just a
single PTP implementation is provided; DW GMAC, DW XGMAC and DW QoS
Eth PTP implementations don't seem like very much different (XGMAC and
QoS Eth seems to have some additional features but the basics looks
the same). Moreover developing a HW-abstraction without having all the
IP-core databooks at hands and having at least two different engines
description seems like a needless over-complication of the code. I
have doubts it was possible to create a comprehensive enough
sub-module to be suitable for the real and any other not yet known PTP
engine.)
4. For the same reason as 2. splitting up the PTP support into two
files seems redundant. stmmac_hwtstamp.c content can be moved to
stmmac_ptp.c .
5. ...

3 and 5 imply bulky and delicate work which I would have attempted
only after much deeper PTP engine studying in all the DW *MAC IP-cores
(I might have missed something) and only having a real PTP-charged
device at hands.

-Serge(y)

> 
> Thanks for the review!
> - Andrew
> 
> 

[-- Attachment #2: 0001-net-stmmac-Stop-overriding-the-PTP-clock-info-static.patch --]
[-- Type: text/x-patch, Size: 3569 bytes --]

From 5ecf0d4f859c42103e69c400dc62b905f423bbe9 Mon Sep 17 00:00:00 2001
From: Serge Semin <fancer.lancer@gmail.com>
Date: Fri, 6 Aug 2021 02:13:36 +0300
Subject: [PATCH] net: stmmac: Stop overriding the PTP clock info static
 instance

It had been defined as constant before commit 9a8a02c9d46d ("net: stmmac:
Add Flexible PPS support"). But then it was converted to be just static,
which fields may get to be modified on each stmmac_ptp_register()
invocation. Since that method is called from the driver probe method, a
concurrent DW *MAC NIC initialization causes the race condition for the
updated stmmac_ptp_clock_ops fields. That also may lead to setting an
inappropriate max_adj value, which was specific for one device, was
written to the stmmac_ptp_clock_ops, but then copied to the private
ptp_clock_info instance unmodified.

So to speak let's leave the stmmac_ptp_clock_ops content untouched and
just copy it to the device-specific instance of the ptp_clock_info
structure, which fields could be then accordingly modified. After that we
can get the const qualifier back to the stmmac_ptp_clock_ops instance
definition.

While at it remove pointless zero-initialization of the
stmmac_ptp_clock_ops fields. It's redundant since the structure is static.

Fixes: 9a8a02c9d46d ("net: stmmac: Add Flexible PPS support")
Fixes: 190f73ab4c43 ("net: stmmac: setup higher frequency clk support for EHL & TGL")
Fixes: f4da56529da6 ("net: stmmac: Add support for external trigger timestamping")
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index b4388ca8d211..19a28b1cc272 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -254,15 +254,10 @@ static int stmmac_getcrosststamp(struct ptp_clock_info *ptp,
 }
 
 /* structure describing a PTP hardware clock */
-static struct ptp_clock_info stmmac_ptp_clock_ops = {
+static const struct ptp_clock_info stmmac_ptp_clock_ops = {
 	.owner = THIS_MODULE,
 	.name = "stmmac ptp",
 	.max_adj = 62500000,
-	.n_alarm = 0,
-	.n_ext_ts = 0, /* will be overwritten in stmmac_ptp_register */
-	.n_per_out = 0, /* will be overwritten in stmmac_ptp_register */
-	.n_pins = 0,
-	.pps = 0,
 	.adjfine = stmmac_adjust_freq,
 	.adjtime = stmmac_adjust_time,
 	.gettime64 = stmmac_get_time,
@@ -287,21 +282,21 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
 		priv->pps[i].available = true;
 	}
 
-	if (priv->plat->ptp_max_adj)
-		stmmac_ptp_clock_ops.max_adj = priv->plat->ptp_max_adj;
-
 	/* Calculate the clock domain crossing (CDC) error if necessary */
 	priv->plat->cdc_error_adj = 0;
 	if (priv->plat->has_gmac4 && priv->plat->clk_ptp_rate)
 		priv->plat->cdc_error_adj = (2 * NSEC_PER_SEC) / priv->plat->clk_ptp_rate;
 
-	stmmac_ptp_clock_ops.n_per_out = priv->dma_cap.pps_out_num;
-	stmmac_ptp_clock_ops.n_ext_ts = priv->dma_cap.aux_snapshot_n;
-
 	rwlock_init(&priv->ptp_lock);
 	mutex_init(&priv->aux_ts_lock);
 	priv->ptp_clock_ops = stmmac_ptp_clock_ops;
 
+	if (priv->plat->ptp_max_adj)
+		priv->ptp_clock_ops.max_adj = priv->plat->ptp_max_adj;
+
+	priv->ptp_clock_ops.n_per_out = priv->dma_cap.pps_out_num;
+	priv->ptp_clock_ops.n_ext_ts = priv->dma_cap.aux_snapshot_n;
+
 	priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops,
 					     priv->device);
 	if (IS_ERR(priv->ptp_clock)) {
-- 
2.41.0

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

[Serge Semin]

On Wed, Aug 30, 2023 at 01:16:31PM +0300, Serge Semin wrote:
> On Tue, Aug 29, 2023 at 10:01:20AM -0500, Andrew Halaney wrote:
> > 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@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
> > 
> 
> > X5 = PTP_SSIR_SSINC_MAX (0xFF) is a case as well to consider
> 
> I noticed that option too, but then I thought it must have been not
> that much probable to be considered as a real case seeing it's a
> boundary case. The clamping happens if
> if (X1 > 255 || X2 > 255 || X3 > 255 || X4 > 255)
> 	X5 = 255
> so in the worst case PTP-rate period in nanoseconds multiplied by 4.3
> must be greater than 255 which is equivalent to X1 >= 60. It means
> PTP clock rate must be greater than 16.6MHz to avoid the clamping. In
> the best case - 3.9MHz. I doubted that these limits are crossed in
> reality. But in anyways you are right saying that it still needs to be
> taken into account in case if the implemented algo would be a subject
> for optimizations.
> 
> > > 
> > > 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!).
> 
> Good. Then an hour spent for decyphering of that stuff wasn't a waste
> of time after all.)
> 
> > 
> > > 
> > > 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
> 
> I am not that much aware of the PTP internals but it just seems weird
> to have clk_ptp_rate not affecting anything except the boundary case.
> Do you have a DW *MAC HW databook with the PTP-engine chapter
> describing the way the System Time Register Module works?
> 

> > process? I'm all ears for suggestions for cleaning this up, especially
> > since others like Richard have indicated that it could use some love,
> 
> * I would have said more definitive - some _hard_ love.)
> 
> > but right now I'm hung up thinking the best I can do is fix the bad
> > comment in this patch.
> 
> Just at the first very swift glance:
> 1. See attached patch.
> 2. Exporting stmmac_init_tstamp_counter() isn't necessary. It doesn't
> seem like being utilized anywhere except in the stmmac_main.c module.
> 3. stmmac_hwtimestamp-based abstraction seems redundant since: just a
> single PTP implementation is provided; DW GMAC, DW XGMAC and DW QoS
> Eth PTP implementations don't seem like very much different (XGMAC and
> QoS Eth seems to have some additional features but the basics looks
> the same). Moreover developing a HW-abstraction without having all the
> IP-core databooks at hands and having at least two different engines
> description seems like a needless over-complication of the code. I
> have doubts it was possible to create a comprehensive enough
> sub-module to be suitable for the real and any other not yet known PTP
> engine.)
> 4. For the same reason as 2. splitting up the PTP support into two
> files seems redundant. stmmac_hwtstamp.c content can be moved to
> stmmac_ptp.c .
> 5. ...

Ah, if you were talking about the Sub-second Increment part and the
System Time Register module then alas I can't help with that much
since I have only a very shallow knowledge about PTP in general, not
to say about that particular module.

-Serge(y)

> 
> 3 and 5 imply bulky and delicate work which I would have attempted
> only after much deeper PTP engine studying in all the DW *MAC IP-cores
> (I might have missed something) and only having a real PTP-charged
> device at hands.
> 
> -Serge(y)
> 
> > 
> > Thanks for the review!
> > - Andrew
> > 
> > 

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

[Andrew Halaney]

On Wed, Aug 30, 2023 at 01:16:31PM +0300, Serge Semin wrote:
> On Tue, Aug 29, 2023 at 10:01:20AM -0500, Andrew Halaney wrote:
> > 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@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
> > 
> 
> > X5 = PTP_SSIR_SSINC_MAX (0xFF) is a case as well to consider
> 
> I noticed that option too, but then I thought it must have been not
> that much probable to be considered as a real case seeing it's a
> boundary case. The clamping happens if
> if (X1 > 255 || X2 > 255 || X3 > 255 || X4 > 255)
> 	X5 = 255
> so in the worst case PTP-rate period in nanoseconds multiplied by 4.3
> must be greater than 255 which is equivalent to X1 >= 60. It means
> PTP clock rate must be greater than 16.6MHz to avoid the clamping. In
> the best case - 3.9MHz. I doubted that these limits are crossed in
> reality. But in anyways you are right saying that it still needs to be
> taken into account in case if the implemented algo would be a subject
> for optimizations.
> 
> > > 
> > > 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!).
> 
> Good. Then an hour spent for decyphering of that stuff wasn't a waste
> of time after all.)
> 
> > 
> > > 
> > > 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
> 
> I am not that much aware of the PTP internals but it just seems weird
> to have clk_ptp_rate not affecting anything except the boundary case.
> Do you have a DW *MAC HW databook with the PTP-engine chapter
> describing the way the System Time Register Module works?

Unfortunately I do not have that documentation (or admittedly much
experience in the area).

> 
> > process? I'm all ears for suggestions for cleaning this up, especially
> > since others like Richard have indicated that it could use some love,
> 
> * I would have said more definitive - some _hard_ love.)
> 
> > but right now I'm hung up thinking the best I can do is fix the bad
> > comment in this patch.
> 
> Just at the first very swift glance:
> 1. See attached patch.
> 2. Exporting stmmac_init_tstamp_counter() isn't necessary. It doesn't
> seem like being utilized anywhere except in the stmmac_main.c module.
> 3. stmmac_hwtimestamp-based abstraction seems redundant since: just a
> single PTP implementation is provided; DW GMAC, DW XGMAC and DW QoS
> Eth PTP implementations don't seem like very much different (XGMAC and
> QoS Eth seems to have some additional features but the basics looks
> the same). Moreover developing a HW-abstraction without having all the
> IP-core databooks at hands and having at least two different engines
> description seems like a needless over-complication of the code. I
> have doubts it was possible to create a comprehensive enough
> sub-module to be suitable for the real and any other not yet known PTP
> engine.)
> 4. For the same reason as 2. splitting up the PTP support into two
> files seems redundant. stmmac_hwtstamp.c content can be moved to
> stmmac_ptp.c .
> 5. ...
> 
> 3 and 5 imply bulky and delicate work which I would have attempted
> only after much deeper PTP engine studying in all the DW *MAC IP-cores
> (I might have missed something) and only having a real PTP-charged
> device at hands.
> 

Thanks, this is a good list to brainstorm with. I agree 3 and 5 might
not be advisable for someone who doesn't have good mastery over the
hardware, etc.

[PATCH net-next 7/7] net: stmmac: Make PTP reference clock references more clear

[Andrew Halaney]

ptp_clock is an overloaded term, and in some instances it is used to
represent the clk_ptp_rate variable. Just use that name as it is
clear that it represents the rate of the PTP reference clock.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/hwif.h            |  5 +++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 10 +++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index bd607da65037..ba92b10cff0e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -523,8 +523,9 @@ struct stmmac_ops {
 /* PTP and HW Timer helpers */
 struct stmmac_hwtimestamp {
 	void (*config_hw_tstamping) (void __iomem *ioaddr, u32 data);
-	void (*config_sub_second_increment)(void __iomem *ioaddr, u32 ptp_clock,
-					   int gmac4, u32 *sub_second_inc);
+	void (*config_sub_second_increment)(void __iomem *ioaddr,
+					    u32 clk_ptp_rate,
+					    int gmac4, u32 *sub_second_inc);
 	int (*init_systime) (void __iomem *ioaddr, u32 sec, u32 nsec);
 	int (*config_addend) (void __iomem *ioaddr, u32 addend);
 	int (*adjust_systime) (void __iomem *ioaddr, u32 sec, u32 nsec,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index 29fd51bb853d..cc0386ee6dee 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -24,7 +24,7 @@ static void config_hw_tstamping(void __iomem *ioaddr, u32 data)
 }
 
 static void config_sub_second_increment(void __iomem *ioaddr,
-		u32 ptp_clock, int gmac4, u32 *sub_second_inc)
+		u32 clk_ptp_rate, int gmac4, u32 *sub_second_inc)
 {
 	u32 value = readl(ioaddr + PTP_TCR);
 	unsigned long data;
@@ -34,14 +34,14 @@ static void config_sub_second_increment(void __iomem *ioaddr,
 	 * increment to twice the number of nanoseconds of a clock cycle.
 	 * The calculation of the default_addend value by the caller will set it
 	 * to mid-range = 2^31 when the remainder of this division is zero,
-	 * which will make the accumulator overflow once every 2 ptp_clock
+	 * which will make the accumulator overflow once every 2 clk_ptp_rate
 	 * cycles, adding twice the number of nanoseconds of a clock cycle :
-	 * 2 * NSEC_PER_SEC / ptp_clock.
+	 * 2 * NSEC_PER_SEC / clk_ptp_rate.
 	 */
 	if (value & PTP_TCR_TSCFUPDT)
-		data = (2 * NSEC_PER_SEC / ptp_clock);
+		data = (2 * NSEC_PER_SEC / clk_ptp_rate);
 	else
-		data = (NSEC_PER_SEC / ptp_clock);
+		data = (NSEC_PER_SEC / clk_ptp_rate);
 
 	/* 0.465ns accuracy */
 	if (!(value & PTP_TCR_TSCTRLSSR))

-- 
2.41.0

Re: [PATCH net-next 7/7] net: stmmac: Make PTP reference clock references more clear

[Richard Cochran]

On Thu, Aug 24, 2023 at 01:32:58PM -0500, Andrew Halaney wrote:

> @@ -34,14 +34,14 @@ static void config_sub_second_increment(void __iomem *ioaddr,
>  	 * increment to twice the number of nanoseconds of a clock cycle.
>  	 * The calculation of the default_addend value by the caller will set it
>  	 * to mid-range = 2^31 when the remainder of this division is zero,
> -	 * which will make the accumulator overflow once every 2 ptp_clock
> +	 * which will make the accumulator overflow once every 2 clk_ptp_rate
>  	 * cycles, adding twice the number of nanoseconds of a clock cycle :
> -	 * 2 * NSEC_PER_SEC / ptp_clock.
> +	 * 2 * NSEC_PER_SEC / clk_ptp_rate.
>  	 */

This part of the driver is complete garbage.  But that isn't your fault.

Richard