Message-ID: <87k0z7n0m9.fsf@kurt>
Date:   Mon, 13 Jul 2020 12:57:34 +0200
From:   Kurt Kanzenbach <kurt@...utronix.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Richard Cochran <richardcochran@...il.com>,
        Kamil Alkhouri <kamil.alkhouri@...offenburg.de>,
        ilias.apalodimas@...aro.org
Subject: Re: [PATCH v1 4/8] net: dsa: hellcreek: Add support for hardware timestamping

Hi Vladimir,

On Mon Jul 13 2020, Vladimir Oltean wrote:
> Hi Kurt,
>
> On Fri, Jul 10, 2020 at 01:36:07PM +0200, Kurt Kanzenbach wrote:
>> From: Kamil Alkhouri <kamil.alkhouri@...offenburg.de>
>> 
>> The switch has the ability to take hardware generated time stamps per port for
>> PTPv2 event messages in Rx and Tx direction. That is useful for achieving needed
>> time synchronization precision for TSN devices/switches. So add support for it.
>> 
>> There are two directions:
>> 
>>  * RX
>> 
>>    The switch has a single register per port to capture a timestamp. That
>>    mechanism is not used due to correlation problems. If the software processing
>>    is too slow and a PTPv2 event message is received before the previous one has
>>    been processed, false timestamps will be captured. Therefore, the switch can
>>    do "inline" timestamping which means it can insert the nanoseconds part of
>>    the timestamp directly into the PTPv2 event message. The reserved field (4
>>    bytes) is leveraged for that. This might not be in accordance with (older)
>>    PTP standards, but is the only way to get reliable results.
>> 
>>  * TX
>> 
>>    In Tx direction there is no correlation problem, because the software and the
>>    driver has to ensure that only one event message is "on the fly". However,
>>    the switch provides also a mechanism to check whether a timestamp is
>>    lost. That can only happen when a timestamp is read and at this point another
>>    message is timestamped. So, that lost bit is checked just in case to indicate
>>    to the user that the driver or the software is somewhat buggy.
>> 
>> Signed-off-by: Kamil Alkhouri <kamil.alkhouri@...offenburg.de>
>> Signed-off-by: Kurt Kanzenbach <kurt@...utronix.de>
>> ---
>>  drivers/net/dsa/hirschmann/Makefile           |   1 +
>>  drivers/net/dsa/hirschmann/hellcreek.c        |  15 +
>>  drivers/net/dsa/hirschmann/hellcreek.h        |  25 +
>>  .../net/dsa/hirschmann/hellcreek_hwtstamp.c   | 498 ++++++++++++++++++
>>  .../net/dsa/hirschmann/hellcreek_hwtstamp.h   |  58 ++
>>  drivers/net/dsa/hirschmann/hellcreek_ptp.c    |  48 +-
>>  drivers/net/dsa/hirschmann/hellcreek_ptp.h    |   4 +
>>  7 files changed, 635 insertions(+), 14 deletions(-)
>>  create mode 100644 drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
>>  create mode 100644 drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
>> 
>> diff --git a/drivers/net/dsa/hirschmann/Makefile b/drivers/net/dsa/hirschmann/Makefile
>> index 39de02a03640..f4075c2998b5 100644
>> --- a/drivers/net/dsa/hirschmann/Makefile
>> +++ b/drivers/net/dsa/hirschmann/Makefile
>> @@ -2,3 +2,4 @@
>>  obj-$(CONFIG_NET_DSA_HIRSCHMANN_HELLCREEK)	+= hellcreek_sw.o
>>  hellcreek_sw-objs := hellcreek.o
>>  hellcreek_sw-objs += hellcreek_ptp.o
>> +hellcreek_sw-objs += hellcreek_hwtstamp.o
>> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
>> index 9901d6435d97..3941a9a3252d 100644
>> --- a/drivers/net/dsa/hirschmann/hellcreek.c
>> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
>> @@ -26,6 +26,7 @@
>>  
>>  #include "hellcreek.h"
>>  #include "hellcreek_ptp.h"
>> +#include "hellcreek_hwtstamp.h"
>>  
>>  static const struct hellcreek_counter hellcreek_counter[] = {
>>  	{ 0x00, "RxFiltered", },
>> @@ -1103,6 +1104,11 @@ static const struct dsa_switch_ops hellcreek_ds_ops = {
>>  	.port_bridge_leave   = hellcreek_port_bridge_leave,
>>  	.port_stp_state_set  = hellcreek_port_stp_state_set,
>>  	.phylink_validate    = hellcreek_phylink_validate,
>> +	.port_hwtstamp_set   = hellcreek_port_hwtstamp_set,
>> +	.port_hwtstamp_get   = hellcreek_port_hwtstamp_get,
>> +	.port_txtstamp	     = hellcreek_port_txtstamp,
>> +	.port_rxtstamp	     = hellcreek_port_rxtstamp,
>> +	.get_ts_info	     = hellcreek_get_ts_info,
>>  };
>>  
>>  static int hellcreek_probe(struct platform_device *pdev)
>> @@ -1202,10 +1208,18 @@ static int hellcreek_probe(struct platform_device *pdev)
>>  		goto err_ptp_setup;
>>  	}
>>  
>> +	ret = hellcreek_hwtstamp_setup(hellcreek);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to setup hardware timestamping!\n");
>> +		goto err_tstamp_setup;
>> +	}
>> +
>>  	platform_set_drvdata(pdev, hellcreek);
>>  
>>  	return 0;
>>  
>> +err_tstamp_setup:
>> +	hellcreek_ptp_free(hellcreek);
>>  err_ptp_setup:
>>  	dsa_unregister_switch(hellcreek->ds);
>>  
>> @@ -1216,6 +1230,7 @@ static int hellcreek_remove(struct platform_device *pdev)
>>  {
>>  	struct hellcreek *hellcreek = platform_get_drvdata(pdev);
>>  
>> +	hellcreek_hwtstamp_free(hellcreek);
>>  	hellcreek_ptp_free(hellcreek);
>>  	dsa_unregister_switch(hellcreek->ds);
>>  	platform_set_drvdata(pdev, NULL);
>> diff --git a/drivers/net/dsa/hirschmann/hellcreek.h b/drivers/net/dsa/hirschmann/hellcreek.h
>> index 2d4422fd2567..1d3de72a48a5 100644
>> --- a/drivers/net/dsa/hirschmann/hellcreek.h
>> +++ b/drivers/net/dsa/hirschmann/hellcreek.h
>> @@ -212,11 +212,36 @@ struct hellcreek_counter {
>>  
>>  struct hellcreek;
>>  
>> +/* State flags for hellcreek_port_hwtstamp::state */
>> +enum {
>> +	HELLCREEK_HWTSTAMP_ENABLED,
>> +	HELLCREEK_HWTSTAMP_TX_IN_PROGRESS,
>> +};
>> +
>> +/* A structure to hold hardware timestamping information per port */
>> +struct hellcreek_port_hwtstamp {
>> +	/* Timestamping state */
>> +	unsigned long state;
>> +
>> +	/* Resources for receive timestamping */
>> +	struct sk_buff_head rx_queue; /* For synchronization messages */
>> +
>> +	/* Resources for transmit timestamping */
>> +	unsigned long tx_tstamp_start;
>> +	struct sk_buff *tx_skb;
>> +
>> +	/* Current timestamp configuration */
>> +	struct hwtstamp_config tstamp_config;
>> +};
>> +
>>  struct hellcreek_port {
>>  	struct hellcreek *hellcreek;
>>  	int port;
>>  	u16 ptcfg;		/* ptcfg shadow */
>>  	u64 *counter_values;
>> +
>> +	/* Per-port timestamping resources */
>> +	struct hellcreek_port_hwtstamp port_hwtstamp;
>>  };
>>  
>>  struct hellcreek_fdb_entry {
>> diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
>> new file mode 100644
>> index 000000000000..dc0ab75d099b
>> --- /dev/null
>> +++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
>> @@ -0,0 +1,498 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/*
>> + * DSA driver for:
>> + * Hirschmann Hellcreek TSN switch.
>> + *
>> + * Copyright (C) 2019,2020 Hochschule Offenburg
>> + * Copyright (C) 2019,2020 Linutronix GmbH
>> + * Authors: Kamil Alkhouri <kamil.alkhouri@...offenburg.de>
>> + *	    Kurt Kanzenbach <kurt@...utronix.de>
>> + */
>> +
>> +#include <linux/ptp_classify.h>
>> +
>> +#include "hellcreek.h"
>> +#include "hellcreek_hwtstamp.h"
>> +#include "hellcreek_ptp.h"
>> +
>> +int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
>> +			  struct ethtool_ts_info *info)
>> +{
>> +	struct hellcreek *hellcreek = ds->priv;
>> +
>> +	info->phc_index = hellcreek->ptp_clock ?
>> +		ptp_clock_index(hellcreek->ptp_clock) : -1;
>> +	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
>> +		SOF_TIMESTAMPING_RX_HARDWARE |
>> +		SOF_TIMESTAMPING_RAW_HARDWARE;
>> +
>> +	/* enabled tx timestamping */
>> +	info->tx_types = BIT(HWTSTAMP_TX_ON);
>> +
>> +	/* L2 & L4 PTPv2 event rx messages are timestamped */
>> +	info->rx_filters = BIT(HWTSTAMP_FILTER_PTP_V2_EVENT);
>> +
>> +	return 0;
>> +}
>> +
>> +/* Enabling/disabling TX and RX HW timestamping for different PTP messages is
>> + * not available in the switch. Thus, this function only serves as a check if
>> + * the user requested what is actually available or not
>> + */
>> +static int hellcreek_set_hwtstamp_config(struct hellcreek *hellcreek, int port,
>> +					 struct hwtstamp_config *config)
>> +{
>> +	struct hellcreek_port_hwtstamp *ps =
>> +		&hellcreek->ports[port].port_hwtstamp;
>> +	bool tx_tstamp_enable = false;
>> +	bool rx_tstamp_enable = false;
>> +
>> +	/* Interaction with the timestamp hardware is prevented here.  It is
>> +	 * enabled when this config function ends successfully
>> +	 */
>> +	clear_bit_unlock(HELLCREEK_HWTSTAMP_ENABLED, &ps->state);
>> +
>> +	/* Reserved for future extensions */
>> +	if (config->flags)
>> +		return -EINVAL;
>> +
>> +	switch (config->tx_type) {
>> +	case HWTSTAMP_TX_ON:
>> +		tx_tstamp_enable = true;
>> +		break;
>> +
>> +	/* TX HW timestamping can't be disabled on the switch */
>> +	case HWTSTAMP_TX_OFF:
>> +		config->tx_type = HWTSTAMP_TX_ON;
>> +		break;
>> +
>> +	default:
>> +		return -ERANGE;
>> +	}
>> +
>> +	switch (config->rx_filter) {
>> +	/* RX HW timestamping can't be disabled on the switch */
>> +	case HWTSTAMP_FILTER_NONE:
>> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
>> +		break;
>> +
>> +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
>> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
>> +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
>> +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
>> +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
>> +	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
>> +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
>> +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
>> +	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
>> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
>> +		rx_tstamp_enable = true;
>> +		break;
>> +
>> +	/* RX HW timestamping can't be enabled for all messages on the switch */
>> +	case HWTSTAMP_FILTER_ALL:
>> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
>> +		break;
>> +
>> +	default:
>> +		return -ERANGE;
>> +	}
>> +
>> +	if (!tx_tstamp_enable)
>> +		return -ERANGE;
>> +
>> +	if (!rx_tstamp_enable)
>> +		return -ERANGE;
>> +
>> +	/* If this point is reached, then the requested hwtstamp config is
>> +	 * compatible with the hwtstamp offered by the switch.  Therefore,
>> +	 * enable the interaction with the HW timestamping
>> +	 */
>> +	set_bit(HELLCREEK_HWTSTAMP_ENABLED, &ps->state);
>> +
>> +	return 0;
>> +}
>> +
>> +int hellcreek_port_hwtstamp_set(struct dsa_switch *ds, int port,
>> +				struct ifreq *ifr)
>> +{
>> +	struct hellcreek *hellcreek = ds->priv;
>> +	struct hellcreek_port_hwtstamp *ps;
>> +	struct hwtstamp_config config;
>> +	int err;
>> +
>> +	ps = &hellcreek->ports[port].port_hwtstamp;
>> +
>> +	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
>> +		return -EFAULT;
>> +
>> +	err = hellcreek_set_hwtstamp_config(hellcreek, port, &config);
>> +	if (err)
>> +		return err;
>> +
>> +	/* Save the chosen configuration to be returned later */
>> +	memcpy(&ps->tstamp_config, &config, sizeof(config));
>> +
>> +	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
>> +		-EFAULT : 0;
>> +}
>> +
>> +int hellcreek_port_hwtstamp_get(struct dsa_switch *ds, int port,
>> +				struct ifreq *ifr)
>> +{
>> +	struct hellcreek *hellcreek = ds->priv;
>> +	struct hellcreek_port_hwtstamp *ps;
>> +	struct hwtstamp_config *config;
>> +
>> +	ps = &hellcreek->ports[port].port_hwtstamp;
>> +	config = &ps->tstamp_config;
>> +
>> +	return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ?
>> +		-EFAULT : 0;
>> +}
>> +
>> +/* Get a pointer to the PTP header in this skb */
>> +static u8 *parse_ptp_header(struct sk_buff *skb, unsigned int type)
>
> Maybe this and the function from mv88e6xxx could share the same
> implementation somehow.

Actually both functions are identical. Should it be moved to the ptp
core, maybe? Then, all drivers could use that. I guess we should also
define a PTP offset for the reserved field which is accessed in
hellcreek_get_reserved_field() just with 16 instead of a proper macro
constant.

>
>> +{
>> +	u8 *data = skb_mac_header(skb);
>> +	unsigned int offset = 0;
>> +
>> +	if (type & PTP_CLASS_VLAN)
>> +		offset += VLAN_HLEN;
>> +
>> +	switch (type & PTP_CLASS_PMASK) {
>> +	case PTP_CLASS_IPV4:
>> +		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
>> +		break;
>> +	case PTP_CLASS_IPV6:
>> +		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
>> +		break;
>> +	case PTP_CLASS_L2:
>> +		offset += ETH_HLEN;
>> +		break;
>> +	default:
>> +		return NULL;
>> +	}
>> +
>> +	/* Ensure that the entire header is present in this packet. */
>> +	if (skb->len + ETH_HLEN < offset + 34)
>> +		return NULL;
>> +
>> +	return data + offset;
>> +}
>> +
>> +/* Returns a pointer to the PTP header if the caller should time stamp, or NULL
>> + * if the caller should not.
>> + */
>> +static u8 *hellcreek_should_tstamp(struct hellcreek *hellcreek, int port,
>> +				   struct sk_buff *skb, unsigned int type)
>> +{
>> +	struct hellcreek_port_hwtstamp *ps =
>> +		&hellcreek->ports[port].port_hwtstamp;
>> +	u8 *hdr;
>> +
>> +	hdr = parse_ptp_header(skb, type);
>> +	if (!hdr)
>> +		return NULL;
>> +
>> +	if (!test_bit(HELLCREEK_HWTSTAMP_ENABLED, &ps->state))
>> +		return NULL;
>> +
>> +	return hdr;
>> +}
>> +
>> +static u64 hellcreek_get_reserved_field(u8 *ptp_hdr)
>> +{
>> +	__be32 *ts;
>> +
>> +	/* length is checked by parse_ptp_header() */
>> +	ts = (__force __be32 *)&ptp_hdr[16];
>> +
>> +	return be32_to_cpup(ts);
>> +}
>> +
>> +static int hellcreek_ptp_hwtstamp_available(struct hellcreek *hellcreek,
>> +					    unsigned int ts_reg)
>> +{
>> +	u16 status;
>> +
>> +	status = hellcreek_ptp_read(hellcreek, ts_reg);
>> +
>> +	if (status & PR_TS_STATUS_TS_LOST)
>> +		dev_err(hellcreek->dev,
>> +			"Tx time stamp lost! This should never happen!\n");
>> +
>> +	/* If hwtstamp is not available, this means the previous hwtstamp was
>> +	 * successfully read, and the one we need is not yet available
>> +	 */
>> +	return (status & PR_TS_STATUS_TS_AVAIL) ? 1 : 0;
>> +}
>> +
>> +/* Get nanoseconds timestamp from timestamping unit */
>> +static u64 hellcreek_ptp_hwtstamp_read(struct hellcreek *hellcreek,
>> +				       unsigned int ts_reg)
>> +{
>> +	u16 nsl, nsh;
>> +
>> +	nsh = hellcreek_ptp_read(hellcreek, ts_reg);
>> +	nsh = hellcreek_ptp_read(hellcreek, ts_reg);
>> +	nsh = hellcreek_ptp_read(hellcreek, ts_reg);
>> +	nsh = hellcreek_ptp_read(hellcreek, ts_reg);
>> +	nsl = hellcreek_ptp_read(hellcreek, ts_reg);
>> +
>> +	return (u64)nsl | ((u64)nsh << 16);
>> +}
>> +
>> +static int hellcreek_txtstamp_work(struct hellcreek *hellcreek,
>> +				   struct hellcreek_port_hwtstamp *ps, int port)
>> +{
>> +	struct skb_shared_hwtstamps shhwtstamps;
>> +	unsigned int status_reg, data_reg;
>> +	struct sk_buff *tmp_skb;
>> +	int ts_status;
>> +	u64 ns = 0;
>> +
>> +	if (!ps->tx_skb)
>> +		return 0;
>> +
>> +	switch (port) {
>> +	case 2:
>> +		status_reg = PR_TS_TX_P1_STATUS_C;
>> +		data_reg   = PR_TS_TX_P1_DATA_C;
>> +		break;
>> +	case 3:
>> +		status_reg = PR_TS_TX_P2_STATUS_C;
>> +		data_reg   = PR_TS_TX_P2_DATA_C;
>> +		break;
>> +	default:
>> +		dev_err(hellcreek->dev, "Wrong port for timestamping!\n");
>> +		return 0;
>> +	}
>> +
>> +	ts_status = hellcreek_ptp_hwtstamp_available(hellcreek, status_reg);
>> +
>> +	/* Not available yet? */
>> +	if (ts_status == 0) {
>> +		/* Check whether the operation of reading the tx timestamp has
>> +		 * exceeded its allowed period
>> +		 */
>> +		if (time_is_before_jiffies(ps->tx_tstamp_start +
>> +					   TX_TSTAMP_TIMEOUT)) {
>> +			dev_err(hellcreek->dev,
>> +				"Timeout while waiting for Tx timestamp!\n");
>> +			goto free_and_clear_skb;
>> +		}
>> +
>> +		/* The timestamp should be available quickly, while getting it
>> +		 * in high priority. Restart the work
>> +		 */
>> +		return 1;
>> +	}
>> +
>> +	spin_lock(&hellcreek->ptp_lock);
>> +	ns  = hellcreek_ptp_hwtstamp_read(hellcreek, data_reg);
>> +	ns += hellcreek_ptp_gettime_seconds(hellcreek, ns);
>> +	spin_unlock(&hellcreek->ptp_lock);
>> +
>> +	/* Now we have the timestamp in nanoseconds, store it in the correct
>> +	 * structure in order to send it to the user
>> +	 */
>> +	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
>> +	shhwtstamps.hwtstamp = ns_to_ktime(ns);
>> +
>> +	tmp_skb = ps->tx_skb;
>> +	ps->tx_skb = NULL;
>> +
>> +	/* skb_complete_tx_timestamp() frees up the client to make another
>> +	 * timestampable transmit.  We have to be ready for it by clearing the
>> +	 * ps->tx_skb "flag" beforehand
>> +	 */
>> +	clear_bit_unlock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
>> +
>> +	/* Deliver a clone of the original outgoing tx_skb with tx hwtstamp */
>> +	skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
>> +
>> +	return 0;
>> +
>> +free_and_clear_skb:
>> +	dev_kfree_skb_any(ps->tx_skb);
>> +	ps->tx_skb = NULL;
>> +	clear_bit_unlock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
>> +
>> +	return 0;
>> +}
>> +
>> +static void hellcreek_get_rxts(struct hellcreek *hellcreek,
>> +			       struct hellcreek_port_hwtstamp *ps,
>> +			       struct sk_buff *skb, struct sk_buff_head *rxq,
>> +			       int port)
>> +{
>> +	struct skb_shared_hwtstamps *shwt;
>> +	struct sk_buff_head received;
>> +	unsigned long flags;
>> +
>> +	/* The latched timestamp belongs to one of the received frames. */
>> +	__skb_queue_head_init(&received);
>> +
>> +	/* Lock & disable interrupts */
>> +	spin_lock_irqsave(&rxq->lock, flags);
>> +
>> +	/* Add the reception queue "rxq" to the "received" queue an reintialize
>> +	 * "rxq".  From now on, we deal with "received" not with "rxq"
>> +	 */
>> +	skb_queue_splice_tail_init(rxq, &received);
>> +
>> +	spin_unlock_irqrestore(&rxq->lock, flags);
>> +
>> +	for (; skb; skb = __skb_dequeue(&received)) {
>> +		unsigned int type;
>> +		u8 *hdr;
>> +		u64 ns;
>> +
>> +		/* Get nanoseconds from ptp packet */
>> +		type = SKB_PTP_TYPE(skb);
>> +		hdr  = parse_ptp_header(skb, type);
>> +		ns   = hellcreek_get_reserved_field(hdr);
>> +
>> +		/* Add seconds part */
>> +		spin_lock(&hellcreek->ptp_lock);
>> +		ns += hellcreek_ptp_gettime_seconds(hellcreek, ns);
>> +		spin_unlock(&hellcreek->ptp_lock);
>> +
>> +		/* Save time stamp */
>> +		shwt = skb_hwtstamps(skb);
>> +		memset(shwt, 0, sizeof(*shwt));
>> +		shwt->hwtstamp = ns_to_ktime(ns);
>> +		netif_rx_ni(skb);
>> +	}
>> +}
>> +
>> +static void hellcreek_rxtstamp_work(struct hellcreek *hellcreek,
>> +				    struct hellcreek_port_hwtstamp *ps,
>> +				    int port)
>> +{
>> +	struct sk_buff *skb;
>> +
>> +	skb = skb_dequeue(&ps->rx_queue);
>> +	if (skb)
>> +		hellcreek_get_rxts(hellcreek, ps, skb, &ps->rx_queue, port);
>> +}
>> +
>> +long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp)
>> +{
>> +	struct hellcreek *hellcreek = ptp_to_hellcreek(ptp);
>> +	struct dsa_switch *ds = hellcreek->ds;
>> +	struct hellcreek_port_hwtstamp *ps;
>> +	int i, restart = 0;
>> +
>> +	for (i = 2; i < ds->num_ports; i++) {
>> +		ps = &hellcreek->ports[i].port_hwtstamp;
>> +
>> +		if (test_bit(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
>> +			restart |= hellcreek_txtstamp_work(hellcreek, ps, i);
>> +
>> +		hellcreek_rxtstamp_work(hellcreek, ps, i);
>> +	}
>> +
>> +	return restart ? 1 : -1;
>> +}
>> +
>> +bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
>> +			     struct sk_buff *clone, unsigned int type)
>> +{
>> +	struct hellcreek *hellcreek = ds->priv;
>> +	struct hellcreek_port_hwtstamp *ps;
>> +	u8 *hdr;
>> +
>> +	ps = &hellcreek->ports[port].port_hwtstamp;
>> +
>> +	/* Check if the driver is expected to do HW timestamping */
>> +	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
>> +		return false;
>> +
>
> I would like to get some clarification on whether "SKBTX_IN_PROGRESS"
> should be set in shtx->tx_flags or not. On one hand, it's asking for
> trouble, on the other hand, it's kind of required for proper compliance
> to API pre-SO_TIMESTAMPING...

Hm. We actually oriented our code on the mv88e6xxx time stamping code base.

>
>> +	/* Make sure the message is a PTP message that needs to be timestamped
>> +	 * and the interaction with the HW timestamping is enabled. If not, stop
>> +	 * here
>> +	 */
>> +	hdr = hellcreek_should_tstamp(hellcreek, port, clone, type);
>> +	if (!hdr)
>> +		return false;
>> +
>> +	if (test_and_set_bit_lock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS,
>> +				  &ps->state))
>> +		return false;
>> +
>> +	ps->tx_skb = clone;
>> +
>> +	/* store the number of ticks occurred since system start-up till this
>> +	 * moment
>> +	 */
>> +	ps->tx_tstamp_start = jiffies;
>> +
>> +	ptp_schedule_worker(hellcreek->ptp_clock, 0);
>> +
>> +	return true;
>> +}
>> +
>> +bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
>> +			     struct sk_buff *skb, unsigned int type)
>> +{
>> +	struct hellcreek *hellcreek = ds->priv;
>> +	struct hellcreek_port_hwtstamp *ps;
>> +	u8 *hdr;
>> +
>> +	ps = &hellcreek->ports[port].port_hwtstamp;
>> +
>> +	/* This check only fails if the user did not initialize hardware
>> +	 * timestamping beforehand.
>> +	 */
>> +	if (ps->tstamp_config.rx_filter != HWTSTAMP_FILTER_PTP_V2_EVENT)
>> +		return false;
>> +
>> +	/* Make sure the message is a PTP message that needs to be timestamped
>> +	 * and the interaction with the HW timestamping is enabled. If not, stop
>> +	 * here
>> +	 */
>> +	hdr = hellcreek_should_tstamp(hellcreek, port, skb, type);
>> +	if (!hdr)
>> +		return false;
>> +
>> +	SKB_PTP_TYPE(skb) = type;
>> +
>> +	skb_queue_tail(&ps->rx_queue, skb);
>> +
>> +	ptp_schedule_worker(hellcreek->ptp_clock, 0);
>> +
>> +	return true;
>> +}
>> +
>> +static void hellcreek_hwtstamp_port_setup(struct hellcreek *hellcreek, int port)
>> +{
>> +	struct hellcreek_port_hwtstamp *ps =
>> +		&hellcreek->ports[port].port_hwtstamp;
>> +
>> +	skb_queue_head_init(&ps->rx_queue);
>> +}
>> +
>> +int hellcreek_hwtstamp_setup(struct hellcreek *hellcreek)
>> +{
>> +	int i;
>> +
>> +	/* Initialize timestamping ports. */
>> +	for (i = 2; i < NUM_PORTS; ++i)
>> +		hellcreek_hwtstamp_port_setup(hellcreek, i);
>> +
>
> Would something like this work better instead?
>
> 	for (port = 0; port < ds->num_ports; port++) {
> 		if (!dsa_is_user_port(ds, port))
> 			continue;
>
> 		hellcreek_hwtstamp_port_setup(hellcreek, port);
> 	}
>
> It is easier to follow for the non-expert reviewer (the information that
> port 0 is CPU and port 1 is "tunnel port" is not immediately findable)
> and (I don't know if this is going to be true or not) in the long term,
> you'd need to do less driver rework when this switch IP is instantiated
> in other chips that will have a different port layout.

That's true. It might not be obvious for someone else. Your code should
work. I'll adjust it. I assume there are more instances in the code
starting at i = 2. And sometimes it uses NUM_PORTS and sometimes
ds->num_ports...

>
>> +	/* Select the synchronized clock as the source timekeeper for the
>> +	 * timestamps and enable inline timestamping.
>> +	 */
>> +	hellcreek_ptp_write(hellcreek, PR_SETTINGS_C_TS_SRC_TK_MASK |
>> +			    PR_SETTINGS_C_RES3TS,
>> +			    PR_SETTINGS_C);
>> +
>> +	return 0;
>> +}
>> +
>> +void hellcreek_hwtstamp_free(struct hellcreek *hellcreek)
>> +{
>> +	/* Nothing todo */
>> +}
>> diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
>> new file mode 100644
>> index 000000000000..c0745ffa1ebb
>> --- /dev/null
>> +++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
>> @@ -0,0 +1,58 @@
>> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
>> +/*
>> + * DSA driver for:
>> + * Hirschmann Hellcreek TSN switch.
>> + *
>> + * Copyright (C) 2019,2020 Hochschule Offenburg
>> + * Copyright (C) 2019,2020 Linutronix GmbH
>> + * Authors: Kurt Kanzenbach <kurt@...utronix.de>
>> + *	    Kamil Alkhouri <kamil.alkhouri@...offenburg.de>
>> + */
>> +
>> +#ifndef _HELLCREEK_HWTSTAMP_H_
>> +#define _HELLCREEK_HWTSTAMP_H_
>> +
>> +#include <net/dsa.h>
>> +#include "hellcreek.h"
>> +
>> +/* Timestamp Register */
>> +#define PR_TS_RX_P1_STATUS_C	(0x1d * 2)
>> +#define PR_TS_RX_P1_DATA_C	(0x1e * 2)
>> +#define PR_TS_TX_P1_STATUS_C	(0x1f * 2)
>> +#define PR_TS_TX_P1_DATA_C	(0x20 * 2)
>> +#define PR_TS_RX_P2_STATUS_C	(0x25 * 2)
>> +#define PR_TS_RX_P2_DATA_C	(0x26 * 2)
>> +#define PR_TS_TX_P2_STATUS_C	(0x27 * 2)
>> +#define PR_TS_TX_P2_DATA_C	(0x28 * 2)
>> +
>> +#define PR_TS_STATUS_TS_AVAIL	BIT(2)
>> +#define PR_TS_STATUS_TS_LOST	BIT(3)
>> +
>> +#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
>> +
>
> Since mv88e6xxx also uses this, maybe we could consider adding it to
> DSA_SKB_CB.

I'm not sure if that's something which should belong in DSA_SKB_CB.

>
>> +/* TX_TSTAMP_TIMEOUT: This limits the time spent polling for a TX
>> + * timestamp. When working properly, hardware will produce a timestamp
>> + * within 1ms. Software may enounter delays, so the timeout is set
>> + * accordingly.
>> + */
>> +#define TX_TSTAMP_TIMEOUT	msecs_to_jiffies(40)
>> +
>> +int hellcreek_port_hwtstamp_set(struct dsa_switch *ds, int port,
>> +				struct ifreq *ifr);
>> +int hellcreek_port_hwtstamp_get(struct dsa_switch *ds, int port,
>> +				struct ifreq *ifr);
>> +
>> +bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
>> +			     struct sk_buff *clone, unsigned int type);
>> +bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
>> +			     struct sk_buff *clone, unsigned int type);
>> +
>> +int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
>> +			  struct ethtool_ts_info *info);
>> +
>> +long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp);
>> +
>> +int hellcreek_hwtstamp_setup(struct hellcreek *chip);
>> +void hellcreek_hwtstamp_free(struct hellcreek *chip);
>> +
>> +#endif /* _HELLCREEK_HWTSTAMP_H_ */
>> diff --git a/drivers/net/dsa/hirschmann/hellcreek_ptp.c b/drivers/net/dsa/hirschmann/hellcreek_ptp.c
>> index c606a26a130e..8c2cef2b60fb 100644
>> --- a/drivers/net/dsa/hirschmann/hellcreek_ptp.c
>> +++ b/drivers/net/dsa/hirschmann/hellcreek_ptp.c
>> @@ -12,14 +12,15 @@
>>  #include <linux/ptp_clock_kernel.h>
>>  #include "hellcreek.h"
>>  #include "hellcreek_ptp.h"
>> +#include "hellcreek_hwtstamp.h"
>>  
>> -static u16 hellcreek_ptp_read(struct hellcreek *hellcreek, unsigned int offset)
>> +u16 hellcreek_ptp_read(struct hellcreek *hellcreek, unsigned int offset)
>>  {
>>  	return readw(hellcreek->ptp_base + offset);
>>  }
>>  
>> -static void hellcreek_ptp_write(struct hellcreek *hellcreek, u16 data,
>> -				unsigned int offset)
>> +void hellcreek_ptp_write(struct hellcreek *hellcreek, u16 data,
>> +			 unsigned int offset)
>>  {
>>  	writew(data, hellcreek->ptp_base + offset);
>>  }
>> @@ -61,6 +62,24 @@ static u64 __hellcreek_ptp_gettime(struct hellcreek *hellcreek)
>>  	return ns;
>>  }
>>  
>> +/* Retrieve the seconds parts in nanoseconds for a packet timestamped with @ns.
>> + * There has to be a check whether an overflow occurred between the packet
>> + * arrival and now. If so use the correct seconds (-1) for calculating the
>> + * packet arrival time.
>> + */
>> +u64 hellcreek_ptp_gettime_seconds(struct hellcreek *hellcreek, u64 ns)
>> +{
>> +	u64 s;
>> +
>> +	__hellcreek_ptp_gettime(hellcreek);
>> +	if (hellcreek->last_ts > ns)
>> +		s = hellcreek->seconds * NSEC_PER_SEC;
>> +	else
>> +		s = (hellcreek->seconds - 1) * NSEC_PER_SEC;
>> +
>> +	return s;
>> +}
>> +
>>  static int hellcreek_ptp_gettime(struct ptp_clock_info *ptp,
>>  				 struct timespec64 *ts)
>>  {
>> @@ -238,17 +257,18 @@ int hellcreek_ptp_setup(struct hellcreek *hellcreek)
>>  	 * accumulator_overflow_rate shall not exceed 62.5 MHz (which adjusts
>>  	 * the nominal frequency by 6.25%)
>>  	 */
>> -	hellcreek->ptp_clock_info.max_adj   = 62500000;
>> -	hellcreek->ptp_clock_info.n_alarm   = 0;
>> -	hellcreek->ptp_clock_info.n_pins    = 0;
>> -	hellcreek->ptp_clock_info.n_ext_ts  = 0;
>> -	hellcreek->ptp_clock_info.n_per_out = 0;
>> -	hellcreek->ptp_clock_info.pps	    = 0;
>> -	hellcreek->ptp_clock_info.adjfine   = hellcreek_ptp_adjfine;
>> -	hellcreek->ptp_clock_info.adjtime   = hellcreek_ptp_adjtime;
>> -	hellcreek->ptp_clock_info.gettime64 = hellcreek_ptp_gettime;
>> -	hellcreek->ptp_clock_info.settime64 = hellcreek_ptp_settime;
>> -	hellcreek->ptp_clock_info.enable    = hellcreek_ptp_enable;
>> +	hellcreek->ptp_clock_info.max_adj     = 62500000;
>> +	hellcreek->ptp_clock_info.n_alarm     = 0;
>> +	hellcreek->ptp_clock_info.n_pins      = 0;
>> +	hellcreek->ptp_clock_info.n_ext_ts    = 0;
>> +	hellcreek->ptp_clock_info.n_per_out   = 0;
>> +	hellcreek->ptp_clock_info.pps	      = 0;
>> +	hellcreek->ptp_clock_info.adjfine     = hellcreek_ptp_adjfine;
>> +	hellcreek->ptp_clock_info.adjtime     = hellcreek_ptp_adjtime;
>> +	hellcreek->ptp_clock_info.gettime64   = hellcreek_ptp_gettime;
>> +	hellcreek->ptp_clock_info.settime64   = hellcreek_ptp_settime;
>> +	hellcreek->ptp_clock_info.enable      = hellcreek_ptp_enable;
>> +	hellcreek->ptp_clock_info.do_aux_work = hellcreek_hwtstamp_work;
>>  
>
> Could you minimize the diff here by indenting these assignments properly
> in the first place, to avoid reindenting them later? It's hard to follow
> what changed. There are also some tabs vs spaces inconsistencies.

Sure.

Thanks,
Kurt

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)

Powered by blists - more mailing lists