Received-SPF: pass (google.com: domain of linux-kernel+bounces-63348-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 13 Feb 2024 11:31:56 +0100
From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Maxime Chevallier <maxime.chevallier@bootlin.com>
CC: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>, <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Jonathan Corbet <corbet@lwn.net>, Richard Cochran
	<richardcochran@gmail.com>, <UNGLinuxDriver@microchip.com>,
	<netdev@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <thomas.petazzoni@bootlin.com>,
	=?utf-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.com>
Subject: Re: [PATCH net-next 2/3] net: lan966x: Allow using PCH extension for
 PTP
Message-ID: <20240213103156.rl4kzwpmxn3haz7y@DEN-DL-M31836.microchip.com>
References: <20240212173307.1124120-1-maxime.chevallier@bootlin.com>
 <20240212173307.1124120-3-maxime.chevallier@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20240212173307.1124120-3-maxime.chevallier@bootlin.com>

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.

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

>                 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);
> +       }
 

-- 
/Horatiu