[PATCH RFC/RFT net-next 0/3] net: stmmac: Increase clk_ptp_ref rate

[PATCH RFC/RFT net-next 0/3] net: stmmac: Increase clk_ptp_ref rate

[Andrew Halaney]

DO NOT MERGE, patch 2 and 3 are duplications at differing levels
(platform vs driver wide). They work fine together but it makes no sense
to take both.

Disclosure: I don't know much about PTP beyond what you can google in an
afternoon, don't have access to documentation about the stmmac IP,
and have only tested that (based on code comments and git commit
history) the programming of the subsecond register (and the clock rate)
makes more sense with these changes.

I'm hoping to start some discussion and get some insight about this.
Recently I found myself discussing PTP and some possible changes from
downstream that might need to be upstreamed. In doing so, I noticed that
the PTP reference clock (clk_ptp_ref) was running at a much lower value
than was being discussed. Digging in a bit, nobody is calling
clk_set_rate() of any value on clk_ptp_ref, so you get whatever the
default rate is when enabled. On Qualcomm platforms I have access to
this results in a 19.2 MHz clock instead of a possible 230.4 MHz clock.

This series proposes setting the clock rate. Patch 2 is the "safe"
approach where a platform must handle it, patch 3 is the big hammer
where we max out the clock for all users. I think patch 2 is using
a proper callback (I want to document those a bit in the future to make
it easier for future folks using them). My guess is that doing this
driver wide might be undesirable for some reasons I'm not
aware of (right now I blindly request the max frequency but the IP
could have an upper limit here, platform maintainers maybe upset if
their careful validation at prior frequencies changes, etc).

I've only tested that the Qualcomm boards I have access to in a remote
lab still work (i.e. throughput testing, etc) and that the PTP
programming is now what I expected it to be theoretically.

I'd really appreciate someone with the ability (and know how!) to test
PTP tried this on at least the Qualcomm platforms. Bonus points if
someone explains how one would even test PTP networks like this.

Thanks,
Andrew

Andrew Halaney (3):
  net: stmmac: Make ptp_clk_freq_config variable type explicit
  net: stmmac: dwmac-qcom-ethqos: Use max frequency for clk_ptp_ref
  net: stmmac: Use the max frequency possible for clk_ptp_ref

 .../net/ethernet/stmicro/stmmac/dwmac-intel.c  |  3 +--
 .../stmicro/stmmac/dwmac-qcom-ethqos.c         | 18 ++++++++++++++++++
 .../ethernet/stmicro/stmmac/stmmac_platform.c  |  5 +++++
 include/linux/stmmac.h                         |  4 +++-
 4 files changed, 27 insertions(+), 3 deletions(-)

-- 
2.41.0

[PATCH RFC/RFT net-next 1/3] net: stmmac: Make ptp_clk_freq_config variable type explicit

[Andrew Halaney]

The priv variable is _always_ of type (struct stmmac_priv *), so let's
stop using (void *) since it isn't abstracting anything.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 3 +--
 include/linux/stmmac.h                            | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index ab9f876b6df7..718bae6872da 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -257,9 +257,8 @@ static void intel_speed_mode_2500(struct net_device *ndev, void *intel_data)
 /* Program PTP Clock Frequency for different variant of
  * Intel mGBE that has slightly different GPO mapping
  */
-static void intel_mgbe_ptp_clk_freq_config(void *npriv)
+static void intel_mgbe_ptp_clk_freq_config(struct stmmac_priv *priv)
 {
-	struct stmmac_priv *priv = (struct stmmac_priv *)npriv;
 	struct intel_priv_data *intel_priv;
 	u32 gpio_value;
 
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 06090538fe2d..c0cbcef1a2f0 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -76,6 +76,8 @@
 			| DMA_AXI_BLEN_32 | DMA_AXI_BLEN_64 \
 			| DMA_AXI_BLEN_128 | DMA_AXI_BLEN_256)
 
+struct stmmac_priv;
+
 /* Platfrom data for platform device structure's platform_data field */
 
 struct stmmac_mdio_bus_data {
@@ -245,7 +247,7 @@ struct plat_stmmacenet_data {
 	int (*serdes_powerup)(struct net_device *ndev, void *priv);
 	void (*serdes_powerdown)(struct net_device *ndev, void *priv);
 	void (*speed_mode_2500)(struct net_device *ndev, void *priv);
-	void (*ptp_clk_freq_config)(void *priv);
+	void (*ptp_clk_freq_config)(struct stmmac_priv *priv);
 	int (*init)(struct platform_device *pdev, void *priv);
 	void (*exit)(struct platform_device *pdev, void *priv);
 	struct mac_device_info *(*setup)(void *priv);
-- 
2.41.0

Re: [PATCH RFC/RFT net-next 1/3] net: stmmac: Make ptp_clk_freq_config variable type explicit

[Simon Horman]

On Tue, Jul 11, 2023 at 03:35:30PM -0500, Andrew Halaney wrote:
> The priv variable is _always_ of type (struct stmmac_priv *), so let's
> stop using (void *) since it isn't abstracting anything.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>

Very nice.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH RFC/RFT net-next 2/3] net: stmmac: dwmac-qcom-ethqos: Use max frequency for clk_ptp_ref

[Andrew Halaney]

Qualcomm clocks can set their frequency to a variety of levels
generally. Let's use the max for clk_ptp_ref to ensure the best
timestamping resolution possible.

Without this, the default value of the clock is used. For sa8775p-ride
this is 19.2 MHz, far less than the 230.4 MHz possible.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 .../stmicro/stmmac/dwmac-qcom-ethqos.c         | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 757504ebb676..f9e7440520fa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -694,6 +694,23 @@ static void ethqos_clks_disable(void *data)
 	ethqos_clks_config(data, false);
 }
 
+static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
+{
+	struct plat_stmmacenet_data *plat_dat = priv->plat;
+	int err;
+
+	if (!plat_dat->clk_ptp_ref)
+		return;
+
+	/* Max the PTP ref clock out to get the best resolution possible */
+	err = clk_set_rate(plat_dat->clk_ptp_ref, ULONG_MAX);
+	if (err)
+		netdev_err(priv->dev, "Failed to max out clk_ptp_ref: %d\n", err);
+	plat_dat->clk_ptp_rate = clk_get_rate(plat_dat->clk_ptp_ref);
+
+	netdev_dbg(priv->dev, "PTP rate %d\n", plat_dat->clk_ptp_rate);
+}
+
 static int qcom_ethqos_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -779,6 +796,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 	plat_dat->bsp_priv = ethqos;
 	plat_dat->fix_mac_speed = ethqos_fix_mac_speed;
 	plat_dat->dump_debug_regs = rgmii_dump;
+	plat_dat->ptp_clk_freq_config = ethqos_ptp_clk_freq_config;
 	plat_dat->has_gmac4 = 1;
 	if (ethqos->has_emac_ge_3)
 		plat_dat->dwmac4_addrs = &data->dwmac4_addrs;
-- 
2.41.0

Re: [PATCH RFC/RFT net-next 2/3] net: stmmac: dwmac-qcom-ethqos: Use max frequency for clk_ptp_ref

[Simon Horman]

On Tue, Jul 11, 2023 at 03:35:31PM -0500, Andrew Halaney wrote:
> Qualcomm clocks can set their frequency to a variety of levels
> generally. Let's use the max for clk_ptp_ref to ensure the best
> timestamping resolution possible.
> 
> Without this, the default value of the clock is used. For sa8775p-ride
> this is 19.2 MHz, far less than the 230.4 MHz possible.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH RFC/RFT net-next 3/3] net: stmmac: Use the max frequency possible for clk_ptp_ref

[Andrew Halaney]

Using the max frequency allows for the best PTP timestamping resolution,
so let's default to that.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 231152ee5a32..c9a27a71a3f4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -602,6 +602,11 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 		plat->clk_ptp_ref = NULL;
 		dev_info(&pdev->dev, "PTP uses main clock\n");
 	} else {
+		/* Get the best resolution possible */
+		rc = clk_set_rate(plat->clk_ptp_ref, ULONG_MAX);
+		if (rc)
+			dev_err(&pdev->dev,
+				"Failed to set clk_ptp_ref rate: %d\n", rc);
 		plat->clk_ptp_rate = clk_get_rate(plat->clk_ptp_ref);
 		dev_dbg(&pdev->dev, "PTP rate %d\n", plat->clk_ptp_rate);
 	}
-- 
2.41.0

Re: [PATCH RFC/RFT net-next 3/3] net: stmmac: Use the max frequency possible for clk_ptp_ref

[Simon Horman]

On Tue, Jul 11, 2023 at 03:35:32PM -0500, Andrew Halaney wrote:
> Using the max frequency allows for the best PTP timestamping resolution,
> so let's default to that.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>