Re: [PATCH net] net: sparx5: start the domain 0 TOD counter on non-PTP lan969x variants

quoted

quoted

quoted

Ack. Not initializing the TOD domains on certain variants is a problem.

However, TOD domains shouldn't affect basic forwarding. I tried it on my board
with PTP forced off - no forwarding issues.

Certain TSN features do require TOD domains to be configured, though. How did
you test this, and what exactly did you see fail?

 > > > > Fix by moving the TOD counter start code into a helper and calling it in

quoted

quoted

quoted

quoted

sparx5_ptp_init(). Non-PTP capable lan969x parts now start the domain 0
counter, and PTP-capable parts start all three, as they used to. A similar
workaround that starts all three and registers the PHC clocks is
implemented in the vendor BSP 6.18 kernel [1].

What we do downstream, is to ensure that all domains and PHC's are configured
and registered on all variants, with a very simple is_sparx5() check.

My reasoning for only enabling the first counter and not registering the
clocks is that the part does not have SPX5_FEATURE_PTP, so it should not
expose any PTP features to userspace.

I acknowledge the problem, and I agree that the PHCs should not be registered.
As for the solution, downstream we always start all three TOD domains, including
on sparx5. If you take the same approach here, you don't need the new helper.
Just move the existing TOD start sequence above the if (!sparx5->ptp) early
return, so the counters are always started, while the PHC registration below it
is still skipped on the non-PTP variants.

ok, in v2 I'll drop the helper and always enable all 3 counters.

pw-bot: cr

Thanks. Please also fix the small nit below:

quoted

quoted

quoted

quoted

   int sparx5_ptp_init(struct sparx5 *sparx5)
   {
-       u64 tod_adj = sparx5_ptp_get_nominal_value(sparx5);
          const struct sparx5_ops *ops = sparx5->data->ops;
          struct sparx5_port *port;
          int err, i;

@@ -622,8 +657,17 @@ int sparx5_ptp_init(struct sparx5 *sparx5)
                  sparx5->ptp = 1;
          }

-       if (!sparx5->ptp)
+       if (!sparx5->ptp) {
+               if (!is_sparx5(sparx5)) {
+                       /* the base, non-ptp-capable lan969x variants need the first tod counter

Nit: s/the/The

quoted

+                        * running to forward frames.
+                        */
+                       err = sparx5_ptp_tod_start(sparx5, BIT(0));
+                       if (err)
+                               return err;
+               }
                  return 0;
+       }

          for (i = 0; i < SPARX5_PHC_COUNT; ++i) {
                  err = sparx5_ptp_phc_init(sparx5, i, &sparx5_ptp_clock_info);

@@ -635,27 +679,9 @@ int sparx5_ptp_init(struct sparx5 *sparx5)
          spin_lock_init(&sparx5->ptp_ts_id_lock);
          mutex_init(&sparx5->ptp_lock);

-       /* Disable master counters */
-       spx5_wr(PTP_PTP_DOM_CFG_PTP_ENA_SET(0), sparx5, PTP_PTP_DOM_CFG);
-
-       /* Configure the nominal TOD increment per clock cycle */
-       spx5_rmw(PTP_PTP_DOM_CFG_PTP_CLKCFG_DIS_SET(0x7),
-                PTP_PTP_DOM_CFG_PTP_CLKCFG_DIS,
-                sparx5, PTP_PTP_DOM_CFG);
-
-       for (i = 0; i < SPARX5_PHC_COUNT; ++i) {
-               spx5_wr((u32)tod_adj & 0xFFFFFFFF, sparx5,
-                       PTP_CLK_PER_CFG(i, 0));
-               spx5_wr((u32)(tod_adj >> 32), sparx5,
-                       PTP_CLK_PER_CFG(i, 1));
-       }
-
-       spx5_rmw(PTP_PTP_DOM_CFG_PTP_CLKCFG_DIS_SET(0),
-                PTP_PTP_DOM_CFG_PTP_CLKCFG_DIS,
-                sparx5, PTP_PTP_DOM_CFG);
-
-       /* Enable master counters */
-       spx5_wr(PTP_PTP_DOM_CFG_PTP_ENA_SET(0x7), sparx5, PTP_PTP_DOM_CFG);
+       err = sparx5_ptp_tod_start(sparx5, GENMASK(SPARX5_PHC_COUNT - 1, 0));
+       if (err)
+               return err;

          for (i = 0; i < sparx5->data->consts->n_ports; i++) {
                  port = sparx5->ports[i];
--

2.55.0

/Daniel