Re: [PATCH net-next v8 05/11] net: ethernet: oa_tc6: Support for hardware timestamp

[Andrew Lunn <andrew@lunn.ch>]

> +static bool oa_tc6_req_tx_hwtstamp(struct oa_tc6 *tc6, struct sk_buff *skb)
> +{
> +	bool drop = false;
> +	u8 tsc;
> +	u8 i;
> +
> +	lockdep_assert_held(&tc6->tx_skb_lock);
> +
> +	if (!skb || !(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> +		return drop;
> +
> +	if (!tc6->hw_tstamp_enabled)
> +		goto out;
> +	if (tc6->ts_config.tx_type != HWTSTAMP_TX_ON) {
> +		tc6->tx_hwtstamp_lost++;
> +		goto out;
> +	}
> +
> +	tsc = tc6->ttsc_current_id;
> +	for (i = 0; i < OA_TC6_TTSCC_REG_ID; i++) {
> +		if (!(tc6->ts_ttsc_pending & BIT(tsc -
> +		    OA_TC6_TTSCA_REG_ID)))
> +			goto slot_found;

The indentation is a bit off. Generally the whole of BIT(...) would be
on the next line.

> +		tsc = oa_tc6_next_tsc(tsc);
> +	}
> +
> +	/* All the three slots are waiting for its event. This skb
> +	 * can't request timestamp. Marking it to be dropped.
> +	 */
> +	tc6->tx_hwtstamp_lost++;
> +	drop = true;
> +	goto out;
> +
> +slot_found:
> +	tc6->ts_ttsc_pending |= BIT(tsc - OA_TC6_TTSCA_REG_ID);
> +	tc6->ttsc_current_id = oa_tc6_next_tsc(tsc);
> +	oa_tc6_tsinfo_tx(skb)->tsc = tsc;
> +	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +out:
> +	return drop;

The goto's here are a bit spaghetti code like. Since all out: does is
return, maybe just use "return true" above.

>  static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
>  {
> +	u32 regs[OA_TC6_STATUS0_TTSC_REG_COUNT];
> +	bool ts_valid = !!tc6->ptp_clock;
>  	u32 value;
>  	int ret;
>  
> -	ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, &value);
> +	/* When timestamp is disabled, there is no behavior change
> +	 * as it reads only STATUS0 register. When enabled,
> +	 * TTSCA_HIGH..TTSCC_LOW are fetched together with STATUS0
> +	 * to avoid having to make second SPI transaction. Reading few
> +	 * extra registers, even it may not be needed every time this
> +	 * function is called, it is more efficient than making second
> +	 * SPI transaction, when needed.
> +	 */
> +	ret = oa_tc6_read_registers(tc6, OA_TC6_REG_STATUS0, regs,
> +				    ts_valid ?
> +				    OA_TC6_STATUS0_TTSC_REG_COUNT : 1);
>  	if (ret) {
>  		netdev_err(tc6->netdev, "STATUS0 register read failed: %d\n",
>  			   ret);
>  		return ret;
>  	}
> +	value = regs[0];

I assume there is a #define for register 0? It would be more readable
to use the name.

> +int oa_tc6_ptp_register(struct oa_tc6 *tc6, struct ptp_clock_info *info)
> +{
> +	int ret = 0;
> +
> +	/* Not supporting hardware timestamp isn't an error */
> +	if (!tc6->hw_tstamp_supported)
> +		return ret;
> +
> +	snprintf(info->name, sizeof(info->name), "%s",
> +		 "OA TC6 PTP clock");

Are names meant to be unique? Have you tested this on a board with
multiple devices?


> +	tc6->ptp_clock = ptp_clock_register(info, &tc6->spi->dev);
> +	if (IS_ERR(tc6->ptp_clock)) {
> +		ret = PTR_ERR(tc6->ptp_clock);
> +		tc6->ptp_clock = NULL;
> +		dev_err(&tc6->spi->dev, "Registration of %s failed\n",
> +			info->name);
> +	}
> +
> +	if (tc6->ptp_clock)
> +		dev_info(&tc6->spi->dev, "%s registered. index %d\n",
> +			 info->name, ptp_clock_index(tc6->ptp_clock));
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_ptp_register);
> +
> +/**
> + * oa_tc6_ptp_unregister - Unregisters clock related callbacks
> + * @tc6: oa_tc6 struct.
> + */
> +void oa_tc6_ptp_unregister(struct oa_tc6 *tc6)
> +{
> +	if (tc6->ptp_clock) {
> +		ptp_clock_unregister(tc6->ptp_clock);
> +		tc6->ptp_clock = NULL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_ptp_unregister);
> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h b/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
> index 403a9c22b5f1..06fe88f1ed7c 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
> @@ -18,6 +18,9 @@
>  #include <linux/wait.h>
>  #include <linux/phy.h>
>  
> +/* Tx timestamp capture register A (high) */
> +#define OA_TC6_REG_TTSCA_HIGH			(0x10)
> +
>  /* Control command header */
>  #define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL	BIT(31)
>  #define OA_TC6_CTRL_HEADER_WRITE_NOT_READ	BIT(29)
> @@ -33,6 +36,7 @@
>  #define OA_TC6_DATA_HEADER_START_WORD_OFFSET	GENMASK(19, 16)
>  #define OA_TC6_DATA_HEADER_END_VALID		BIT(14)
>  #define OA_TC6_DATA_HEADER_END_BYTE_OFFSET	GENMASK(13, 8)
> +#define OA_TC6_DATA_HEADER_TSC_OFFSET		GENMASK(7, 6)
>  #define OA_TC6_DATA_HEADER_PARITY		BIT(0)
>  
>  /* Data footer */
> @@ -44,6 +48,8 @@
>  #define OA_TC6_DATA_FOOTER_START_VALID		BIT(20)
>  #define OA_TC6_DATA_FOOTER_START_WORD_OFFSET	GENMASK(19, 16)
>  #define OA_TC6_DATA_FOOTER_END_VALID		BIT(14)
> +#define OA_TC6_DATA_FOOTER_RTSA_VALID		BIT(7)
> +#define OA_TC6_DATA_FOOTER_RTSP_VALID		BIT(6)
>  #define OA_TC6_DATA_FOOTER_END_BYTE_OFFSET	GENMASK(13, 8)
>  #define OA_TC6_DATA_FOOTER_TX_CREDITS		GENMASK(5, 1)
>  
> @@ -70,6 +76,22 @@
>  
>  #define OA_TC6_REG_MMS_MASK		GENMASK(19, 16)
>  
> +#define OA_TC6_TSTAMP_SZ			8
> +
> +#define OA_TC6_TTSCA_REG_ID			1
> +#define OA_TC6_TTSCB_REG_ID			2
> +#define OA_TC6_TTSCC_REG_ID			3
> +
> +/* STATUS0 and the TTSCA/B/C capture registers are contiguous in MMS 0, so
> + * they are fetched with a single control transaction while timestamping is
> + * available.
> + */
> +#define OA_TC6_TTSC_REG_OFFSET		(OA_TC6_REG_TTSCA_HIGH - \
> +					 OA_TC6_REG_STATUS0)
> +#define OA_TC6_TTSC_REG_COUNT		(2 * OA_TC6_TTSCC_REG_ID)
> +#define OA_TC6_STATUS0_TTSC_REG_COUNT	(OA_TC6_TTSC_REG_OFFSET + \
> +					 OA_TC6_TTSC_REG_COUNT)
> +
>  /* Internal structure for MAC-PHY drivers */
>  struct oa_tc6 {
>  	struct net_device *netdev;
> @@ -95,6 +117,16 @@ struct oa_tc6 {
>  	bool prot_ctrl;
>  	enum oa_tc6_quirk_flag quirk_flags;
>  	struct gpio_desc *reset_gpio;
> +	struct hwtstamp_config ts_config;
> +	struct list_head tx_ts_skb_q;
> +	struct ptp_clock *ptp_clock;
> +	bool hw_tstamp_supported;
> +	bool hw_tstamp_enabled;
> +	u64 tx_hwtstamp_pkts;
> +	u64 tx_hwtstamp_lost;
> +	u64 tx_hwtstamp_err;
> +	u8 ts_ttsc_pending;
> +	u8 ttsc_current_id;
>  };
>  
>  enum oa_tc6_header_type {
> @@ -121,5 +153,8 @@ enum oa_tc6_data_end_valid_info {
>  	OA_TC6_DATA_END_INVALID,
>  	OA_TC6_DATA_END_VALID,
>  };
> +
> +void oa_tc6_cleanup_tx_tstamp_skbs(struct oa_tc6 *tc6);
> +
>  #endif /* OA_TC6_STD_DEF_H */
>  
> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
> new file mode 100644
> index 000000000000..d83f6ad9373a
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface framework
> + *
> + * Author: Selva Rajagopal <selvamani.rajagopal@onsemi.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/iopoll.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> +#include <linux/oa_tc6.h>
> +
> +#include "oa_tc6_std_def.h"
> +
> +static int oa_tc6_set_hwtstamp_settings(struct oa_tc6 *tc6,
> +					const struct hwtstamp_config *ts_cfg)
> +{
> +	u32 regs[OA_TC6_REG_INT_MASK0 - OA_TC6_REG_CONFIG0 + 1];
> +	u32 cfg0, irqm, status0;
> +	bool disabled;
> +	int ret;
> +
> +	if (tc6->ts_config.flags == ts_cfg->flags &&
> +	    tc6->ts_config.tx_type == ts_cfg->tx_type &&
> +	    tc6->ts_config.rx_filter == ts_cfg->rx_filter)
> +		return 0;
> +
> +	spin_lock_bh(&tc6->tx_skb_lock);
> +	disabled = tc6->disable_traffic;
> +	spin_unlock_bh(&tc6->tx_skb_lock);
> +	if (disabled)
> +		return -EIO;
> +
> +	/* CONFIG0, STATUS0 and INT_MASK0 span one register block, so a
> +	 * single control transaction covers all three.
> +	 */
> +	ret = oa_tc6_read_registers(tc6, OA_TC6_REG_CONFIG0, regs,
> +				    ARRAY_SIZE(regs));
> +	if (ret) {
> +		dev_err(&tc6->spi->dev, "Failed to read CFG0 block\n");
> +		goto out;
> +	}
> +
> +	cfg0 = regs[0];

OA_TC6_REG_CONFIG0 - OA_TC6_REG_CONFIG0?

> +	status0 = regs[OA_TC6_REG_STATUS0 - OA_TC6_REG_CONFIG0];
> +	irqm = regs[OA_TC6_REG_INT_MASK0 - OA_TC6_REG_CONFIG0];

It then follow the pattern?

   Andrew