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

[Daniel Machon]

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

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