Re: [PATCH net-next 2/3] net: lan966x: Allow using PCH extension for PTP

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Horatiu,

On Tue, 13 Feb 2024 11:31:56 +0100
Horatiu Vultur <horatiu.vultur@microchip.com> wrote:

> The 02/12/2024 18:33, Maxime Chevallier wrote:
> > [Some people who received this message don't often get email from maxime.chevallier@bootlin.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]  
> 
> Hi Maxime,
> 
> I have tried your patches on pcb8291, which is a lan966x without PHYs
> that support timestamping. And on this platform this patch breaks up the
> things. Because it should just do the timestamping the MAC in that case,
> but with this patch it doesn't get any time.
> The same issue can be reproduced on pcb8280 and then disable PHY
> timestamping, or change the lan8814 not to support HW timestamping.
> 
> Please see bellow the reason why.

You are entirely correct and I apparently messed-up my series as these
changes were implemented locally and somehow lost in the rebase. Indeed
this codes doesn't work at all... I'll resend that, thanks a lot for
the test and sorry !

> 
> > 
> > +/* Enable or disable PCH timestamp transmission. This uses the USGMII PCH
> > + * extensions to transmit the timestamps in the frame preamble.
> > + */
> > +static void lan966x_ptp_pch_configure(struct lan966x_port *port, bool *enable)
> > +{
> > +       struct phy_device *phydev = port->dev->phydev;
> > +       int ret;
> > +
> > +       if (!phydev)
> > +               *enable = false;
> > +
> > +       if (*enable) {
> > +               /* If we cannot enable inband PCH mode, we fallback to classic
> > +                * timestamping
> > +                */
> > +               if (phy_inband_ext_available(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP)) {
> > +                       ret = phy_inband_ext_enable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
> > +                       if (ret)
> > +                               *enable = false;
> > +               } else {
> > +                       *enable = false;
> > +               }
> > +       } else {
> > +               phy_inband_ext_disable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
> > +       }
> > +
> > +       lan_rmw(SYS_PCH_CFG_PCH_SUB_PORT_ID_SET(port->chip_port % 4) |
> > +               SYS_PCH_CFG_PCH_TX_MODE_SET(*enable) |
> > +               SYS_PCH_CFG_PCH_RX_MODE_SET(*enable),
> > +               SYS_PCH_CFG_PCH_SUB_PORT_ID |
> > +               SYS_PCH_CFG_PCH_TX_MODE |
> > +               SYS_PCH_CFG_PCH_RX_MODE,
> > +               port->lan966x, SYS_PCH_CFG(port->chip_port));
> > +}
> > +
> >  int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
> >                              struct kernel_hwtstamp_config *cfg,
> >                              struct netlink_ext_ack *extack)
> >  {
> >         struct lan966x *lan966x = port->lan966x;
> > +       bool timestamp_in_pch = false;
> >         struct lan966x_phc *phc;
> > 
> >         switch (cfg->tx_type) {
> > @@ -303,10 +339,18 @@ int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
> >                 return -ERANGE;
> >         }
> > 
> > +       if (cfg->source == HWTSTAMP_SOURCE_PHYLIB &&
> > +           cfg->tx_type == HWTSTAMP_TX_ON &&
> > +           port->config.portmode == PHY_INTERFACE_MODE_QUSGMII)
> > +               timestamp_in_pch = true;
> > +
> > +       lan966x_ptp_pch_configure(port, &timestamp_in_pch);
> > +
> >         /* Commit back the result & save it */
> >         mutex_lock(&lan966x->ptp_lock);
> >         phc = &lan966x->phc[LAN966X_PHC_PORT];
> >         phc->hwtstamp_config = *cfg;
> > +       phc->pch = timestamp_in_pch;  
> 
> Here we figure out if pch is enabled or not. If the cfg->source is not
> PHYLIB or the interface is not QUSGMII then timestamp_in_pch will stay
> false.
> 
> >         mutex_unlock(&lan966x->ptp_lock);
> > 
> >         return 0;
> > @@ -397,6 +441,7 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
> >         LAN966X_SKB_CB(skb)->jiffies = jiffies;
> > 
> >         lan966x->ptp_skbs++;
> > +  
> 
> I think this is just a small style change. So maybe it shouldn't be in
> here.
> 
> >         port->ts_id++;
> >         if (port->ts_id == LAN966X_MAX_PTP_ID)
> >                 port->ts_id = 0;
> > @@ -500,6 +545,27 @@ irqreturn_t lan966x_ptp_irq_handler(int irq, void *args)
> >                 /* Read RX timestamping to get the ID */
> >                 id = lan_rd(lan966x, PTP_TWOSTEP_STAMP);
> > 
> > +               /* If PCH is enabled, there is a "feature" that also the MAC
> > +                * will generate an interrupt for transmitted frames. This
> > +                * interrupt should be ignored, so clear the allocated resources
> > +                * and try to get the next timestamp. Maybe should clean the
> > +                * resources on the TX side?
> > +                */
> > +               if (phy_inband_ext_enabled(port->dev->phydev,
> > +                                          PHY_INBAND_EXT_PCH_TIMESTAMP)) {
> > +                       spin_lock(&lan966x->ptp_ts_id_lock);
> > +                       lan966x->ptp_skbs--;
> > +                       spin_unlock(&lan966x->ptp_ts_id_lock);
> > +
> > +                       dev_kfree_skb_any(skb_match);
> > +
> > +                       lan_rmw(PTP_TWOSTEP_CTRL_NXT_SET(1),
> > +                               PTP_TWOSTEP_CTRL_NXT,
> > +                               lan966x, PTP_TWOSTEP_CTRL);
> > +
> > +                       continue;
> > +               }
> > +
> >                 spin_lock_irqsave(&port->tx_skbs.lock, flags);
> >                 skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> >                         if (LAN966X_SKB_CB(skb)->ts_id != id)
> > @@ -1088,19 +1154,27 @@ void lan966x_ptp_rxtstamp(struct lan966x *lan966x, struct sk_buff *skb,
> >         struct timespec64 ts;
> >         u64 full_ts_in_ns;
> > 
> > +       phc = &lan966x->phc[LAN966X_PHC_PORT];
> > +
> >         if (!lan966x->ptp ||
> > -           !lan966x->ports[src_port]->ptp_rx_cmd)
> > +           !lan966x->ports[src_port]->ptp_rx_cmd ||
> > +           !phc->pch)  
> 
> And here because phc->pch is false, it would just return.
> Meaning that it would never be able to get the time.
> I presume that this check should not be modified.

Dammit you are right and I had these modifications locally, but
apparently I messed my rebase and lost that... 

> 
> >                 return;
> > 
> > -       phc = &lan966x->phc[LAN966X_PHC_PORT];
> > -       lan966x_ptp_gettime64(&phc->info, &ts);
> > -
> > -       /* Drop the sub-ns precision */
> > -       timestamp = timestamp >> 2;
> > -       if (ts.tv_nsec < timestamp)
> > -               ts.tv_sec--;
> > -       ts.tv_nsec = timestamp;
> > -       full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
> > +       if (phc->pch) {
> > +               /* Drop the sub-ns precision */
> > +               timestamp = timestamp >> 2;
> > +               full_ts_in_ns = lower_32_bits(timestamp);
> > +       } else {
> > +               lan966x_ptp_gettime64(&phc->info, &ts);
> > +
> > +               /* Drop the sub-ns precision */
> > +               timestamp = timestamp >> 2;
> > +               if (ts.tv_nsec < timestamp)
> > +                       ts.tv_sec--;
> > +               ts.tv_nsec = timestamp;
> > +               full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
> > +       }  
>  
> 


Thanks for the review and analysis Horatiu, and sorry for this hiccup !

Maxime