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@xxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> Signed-off-by: Kurt Kanzenbach <kurt@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx>
>> + * Kurt Kanzenbach <kurt@xxxxxxxxxxxxx>
>> + */
>> +
>> +#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@xxxxxxxxxxxxx>
>> + * Kamil Alkhouri <kamil.alkhouri@xxxxxxxxxxxxxxx>
>> + */
>> +
>> +#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
Attachment:
signature.asc
Description: PGP signature
