[PATCH 1/2] net: stmmac: fix enabling socfpga's ptp_ref_clock

[PATCH 1/2] net: stmmac: fix enabling socfpga's ptp_ref_clock

[Julien Beraud]

There are 2 registers to write to enable a ptp ref clock coming from the
fpga.
One that enables the usage of the clock from the fpga for emac0 and emac1
as a ptp ref clock, and the other to allow signals from the fpga to reach
emac0 and emac1.
Currently, if the dwmac-socfpga has phymode set to PHY_INTERFACE_MODE_MII,
PHY_INTERFACE_MODE_GMII, or PHY_INTERFACE_MODE_SGMII, both registers will
be written and the ptp ref clock will be set as coming from the fpga.
Separate the 2 register writes to only enable signals from the fpga to
reach emac0 or emac1 when ptp ref clock is not coming from the fpga.

Signed-off-by: Julien Beraud <julien.beraud@orolia.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index e0212d2fc2a1..b7087245af26 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -289,16 +289,19 @@ static int socfpga_gen5_set_phy_mode(struct socfpga_dwmac *dwmac)
 	    phymode == PHY_INTERFACE_MODE_MII ||
 	    phymode == PHY_INTERFACE_MODE_GMII ||
 	    phymode == PHY_INTERFACE_MODE_SGMII) {
-		ctrl |= SYSMGR_EMACGRP_CTRL_PTP_REF_CLK_MASK << (reg_shift / 2);
 		regmap_read(sys_mgr_base_addr, SYSMGR_FPGAGRP_MODULE_REG,
 			    &module);
 		module |= (SYSMGR_FPGAGRP_MODULE_EMAC << (reg_shift / 2));
 		regmap_write(sys_mgr_base_addr, SYSMGR_FPGAGRP_MODULE_REG,
 			     module);
-	} else {
-		ctrl &= ~(SYSMGR_EMACGRP_CTRL_PTP_REF_CLK_MASK << (reg_shift / 2));
 	}
 
+	if (dwmac->f2h_ptp_ref_clk)
+		ctrl |= SYSMGR_EMACGRP_CTRL_PTP_REF_CLK_MASK << (reg_shift / 2);
+	else
+		ctrl &= ~(SYSMGR_EMACGRP_CTRL_PTP_REF_CLK_MASK <<
+			  (reg_shift / 2));
+
 	regmap_write(sys_mgr_base_addr, reg_offset, ctrl);
 
 	/* Deassert reset for the phy configuration to be sampled by
-- 
2.25.1

[PATCH 2/2] net: stmmac: Fix sub-second increment

[Julien Beraud]

In fine adjustement mode, which is the current default, the sub-second
increment register is the number of nanoseconds that wil be added to the
clock when the accumulator overflows. At each clock cycle, the value of
the addend register is added to the accumulator.
Currently, we use 20ns = 1e09ns / 50MHz as this value whatever the
frequency of the ptp clock actually is.
The adjustment is then done on the addend register, only incrementing
every X clock cycles X being the ratio between 50MHz and ptp_clock_rate
(addend = 2^31 * 50MHz/ptp_clock_rate).
First of all there is a bug that was already solved in the past in that
it should be 40ns = 2e09ns / 50MHz - (the accumulator can only overflow
once every 2 additions)
Even with this issue fixed, this has following consequences :
- The resolution of the timestamping clock is limited to 40ns while it
  is not needed, thus limiting the accuracy of the timestamping to 40ns.
- In case the frequency of the ptp clock is inferior to 50MHz, the
  addend value will be set to 0 and the clock will simply not work.

Fix this by setting sub-second increment to 2e09ns / ptp_clock_rate and
setting the default value of addend to mid-range = 2^31. By doing this
we can reach the best possible resolution in the timestamps, only
incrementing the clock by the minimum possible value. This will also
work for frequencies below 50MHz (for instance using the internal osc1
clock at 25MHz for socfpga platforms).

Signed-off-by: Julien Beraud <julien.beraud@orolia.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 10 ++++++----
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     | 11 ++---------
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index fcf080243a0f..2438c7bbc7c9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -27,12 +27,14 @@ static void config_sub_second_increment(void __iomem *ioaddr,
 	unsigned long data;
 	u32 reg_value;
 
-	/* For GMAC3.x, 4.x versions, convert the ptp_clock to nano second
-	 *	formula = (1/ptp_clock) * 1000000000
-	 * where ptp_clock is 50MHz if fine method is used to update system
+	/* For GMAC3.x, 4.x versions, in "fine adjustement mode" set sub-second
+	 * increment to twice the number of nanoseconds of a clock cycle.
+	 * Setting the default addend value to mid range will make the
+	 * accumulator overflow once every 2 clock cycles, thus adding twice
+	 * the length of a clock cycle to the clock time.
 	 */
 	if (value & PTP_TCR_TSCFUPDT)
-		data = (1000000000ULL / 50000000);
+		data = (2000000000ULL / ptp_clock);
 	else
 		data = (1000000000ULL / ptp_clock);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e6898fd5223f..4ceddfa64b1d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -511,7 +511,6 @@ static int stmmac_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
 	struct stmmac_priv *priv = netdev_priv(dev);
 	struct hwtstamp_config config;
 	struct timespec64 now;
-	u64 temp = 0;
 	u32 ptp_v2 = 0;
 	u32 tstamp_all = 0;
 	u32 ptp_over_ipv4_udp = 0;
@@ -698,19 +697,13 @@ static int stmmac_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
 		stmmac_config_sub_second_increment(priv,
 				priv->ptpaddr, priv->plat->clk_ptp_rate,
 				xmac, &sec_inc);
-		temp = div_u64(1000000000ULL, sec_inc);
 
 		/* Store sub second increment and flags for later use */
 		priv->sub_second_inc = sec_inc;
 		priv->systime_flags = value;
 
-		/* calculate default added value:
-		 * formula is :
-		 * addend = (2^32)/freq_div_ratio;
-		 * where, freq_div_ratio = 1e9ns/sec_inc
-		 */
-		temp = (u64)(temp << 32);
-		priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
+		/* set default addend to mid-range */
+		priv->default_addend = (1 << 31);
 		stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);
 
 		/* initialize system time */
-- 
2.25.1

RE: [PATCH 2/2] net: stmmac: Fix sub-second increment

[Jose Abreu]

From: Julien Beraud <julien.beraud@orolia.com>
Date: Apr/14/2020, 10:10:03 (UTC+00:00)

> This will also
> work for frequencies below 50MHz (for instance using the internal osc1
> clock at 25MHz for socfpga platforms).

No please. Per HW specifications the minimum value for PTP reference clock 
is 50MHz. If this does not work in your setup you'll need to add some kind 
of quirk to stmmac instead of changing the existing logic which works fine 
in all other setups.

---
Thanks,
Jose Miguel Abreu

Re: [PATCH 2/2] net: stmmac: Fix sub-second increment

[Julien Beraud]

Jose

On 14/04/2020 11:15, Jose Abreu wrote:
> From: Julien Beraud <julien.beraud@orolia.com>
> Date: Apr/14/2020, 10:10:03 (UTC+00:00)
> 
>> This will also
>> work for frequencies below 50MHz (for instance using the internal osc1
>> clock at 25MHz for socfpga platforms).
> 
> No please. Per HW specifications the minimum value for PTP reference clock
> is 50MHz. If this does not work in your setup you'll need to add some kind
> of quirk to stmmac instead of changing the existing logic which works fine
> in all other setups.

The numbers I have in the documentation say that the minimum clock 
frequency for PTP is determined by "3 * PTP clock period + 4 * GMII/MII 
clock period <= Minimum gap between two SFDs". The example values say 
5MHz for 1000-Mbps Full Duplex. Is this documentation incorrect ?

If this clock is really limited to 50MHz minimum, we could explicitely 
forbid values under 50MHz but silently making the ptp clock not 
increment doesn't look right.

Apart from that, the existing logic doesn't work. The calculation is off 
by a factor 2, making the ptp clock increment twice slower as it should, 
at least on socfpga platform but I expect that it is the same on other 
platforms. Please check commit 19d857c, which has kind of been reverted 
since for more explanation on the sub-seconds + addend calculation.
Also, it artificially sets the increment to a value while we should 
allow it to be as small as posible for higher frequencies, in order to 
gain accuracy in timestamping.

Regards,
Julien

RE: [PATCH 2/2] net: stmmac: Fix sub-second increment

[Jose Abreu]

From: Julien Beraud <julien.beraud@orolia.com>
Date: Apr/14/2020, 10:46:49 (UTC+00:00)

> The numbers I have in the documentation say that the minimum clock 
> frequency for PTP is determined by "3 * PTP clock period + 4 * GMII/MII 
> clock period <= Minimum gap between two SFDs". The example values say 
> 5MHz for 1000-Mbps Full Duplex. Is this documentation incorrect ?

I meant for fine update method (which is the one currently used), the 
clock frequency must be higher than the desired accuracy (which is 
50MHz).

> Apart from that, the existing logic doesn't work. The calculation is off 
> by a factor 2, making the ptp clock increment twice slower as it should, 
> at least on socfpga platform but I expect that it is the same on other 
> platforms. Please check commit 19d857c, which has kind of been reverted 
> since for more explanation on the sub-seconds + addend calculation.
> Also, it artificially sets the increment to a value while we should 
> allow it to be as small as posible for higher frequencies, in order to 
> gain accuracy in timestamping.

I'm sorry but I don't see any "off by factor of 2". I also don't 
understand this:
 "the accumulator can only overflow once every 2 additions"

Can you please provide more details ?

BTW, I applied your patch and I saw no difference at all in my setup 
except for path delay increasing a little bit.

---
Thanks,
Jose Miguel Abreu

Re: [PATCH 2/2] net: stmmac: Fix sub-second increment

[Julien Beraud]

On 14/04/2020 13:14, Jose Abreu wrote:
> From: Julien Beraud <julien.beraud@orolia.com>
> Date: Apr/14/2020, 10:46:49 (UTC+00:00)
> 
>> The numbers I have in the documentation say that the minimum clock
>> frequency for PTP is determined by "3 * PTP clock period + 4 * GMII/MII
>> clock period <= Minimum gap between two SFDs". The example values say
>> 5MHz for 1000-Mbps Full Duplex. Is this documentation incorrect ?
> 
> I meant for fine update method (which is the one currently used), the
> clock frequency must be higher than the desired accuracy (which is
> 50MHz).
For the fine update method, this patch makes it possible to adjust the 
accuracy (sub seconds increment) of the ptp clock to the ptp_ref_clk's 
frequency, making it possible to work with frequencies down to the 
minimal value to the maximum value of the ptp_ref_clk freq. It also 
allow to set a value that is inferior to 20ns, thus improving the 
accuracy for frequencies higher than 100MHz.

> 
>> Apart from that, the existing logic doesn't work. The calculation is off
>> by a factor 2, making the ptp clock increment twice slower as it should,
>> at least on socfpga platform but I expect that it is the same on other
>> platforms. Please check commit 19d857c, which has kind of been reverted
>> since for more explanation on the sub-seconds + addend calculation.
>> Also, it artificially sets the increment to a value while we should
>> allow it to be as small as posible for higher frequencies, in order to
>> gain accuracy in timestamping.
> 
> I'm sorry but I don't see any "off by factor of 2". I also don't
> understand this:
>   "the accumulator can only overflow once every 2 additions"
> 
> Can you please provide more details ?
Sorry, my explanation was incorrect. The current calculation is not off 
by a factor 2, it is compensated later by a division. It is just that in 
the case of a 50MHz clock, it will lead to a value of 0 in the addend 
register, so it will not work either like it is described in commit 19d857c.

About "the accumulator can only overflow once every 2 additions", please 
see commit 19d857c.

> 
> BTW, I applied your patch and I saw no difference at all in my setup
> except for path delay increasing a little bit.
Ok, my bad, the current calculation does work but it limits the the 
accuracy in case of a clock frequency higher than 100MHz, and doesn't 
work for frequencies inferior or equal to 50MHz. What is your 
ptp_ref_clk frequency? If it is 100MHz, then our 2 calculations give the 
same result.

To summarize, I think I can rephrase the commit message to make it 
easier to understand, but this patch gives the ability to solve 2 issues 
with the current code:
-> The impossibility to use a clock freq inferior or equal to 50MHz
-> The limitation of the sub-second-increment that limits the accuracy 
of the timestamping for frequencies higher than 100MHz.