diff mbox series

[v3,5/8] net: dsa: hellcreek: Add TAPRIO offloading support

Message ID 20200820081118.10105-6-kurt@linutronix.de
State Changes Requested
Delegated to: David Miller
Headers show
Series Hirschmann Hellcreek DSA driver | expand

Commit Message

Kurt Kanzenbach Aug. 20, 2020, 8:11 a.m. UTC
The switch has support for the 802.1Qbv Time Aware Shaper (TAS). Traffic
schedules may be configured individually on each front port. Each port has eight
egress queues. The traffic is mapped to a traffic class respectively via the PCP
field of a VLAN tagged frame.

The TAPRIO Qdisc already implements that. Therefore, this interface can simply
be reused. Add .port_setup_tc() accordingly.

The activation of a schedule on a port is split into two parts:

 * Programming the necessary gate control list (GCL)
 * Setup hrtimer for starting the schedule

The hardware supports starting a schedule up to eight seconds in the future. The
TAPRIO interface provides an absolute base time. Therefore, hrtimers are
leveraged.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/dsa/hirschmann/hellcreek.c | 294 +++++++++++++++++++++++++
 drivers/net/dsa/hirschmann/hellcreek.h |  21 ++
 2 files changed, 315 insertions(+)

Comments

Vladimir Oltean Aug. 22, 2020, 2:39 p.m. UTC | #1
Hi Kurt,

On Thu, Aug 20, 2020 at 10:11:15AM +0200, Kurt Kanzenbach wrote:
> The switch has support for the 802.1Qbv Time Aware Shaper (TAS). Traffic
> schedules may be configured individually on each front port. Each port has eight
> egress queues. The traffic is mapped to a traffic class respectively via the PCP
> field of a VLAN tagged frame.
> 
> The TAPRIO Qdisc already implements that. Therefore, this interface can simply
> be reused. Add .port_setup_tc() accordingly.
> 
> The activation of a schedule on a port is split into two parts:
> 
>  * Programming the necessary gate control list (GCL)
>  * Setup hrtimer for starting the schedule
> 
> The hardware supports starting a schedule up to eight seconds in the future. The
> TAPRIO interface provides an absolute base time. Therefore, hrtimers are
> leveraged.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/dsa/hirschmann/hellcreek.c | 294 +++++++++++++++++++++++++
>  drivers/net/dsa/hirschmann/hellcreek.h |  21 ++
>  2 files changed, 315 insertions(+)
> 
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
> index 745ca60342b4..e5b54f42c635 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -22,7 +22,9 @@
>  #include <linux/spinlock.h>
>  #include <linux/delay.h>
>  #include <linux/ktime.h>
> +#include <linux/time.h>
>  #include <net/dsa.h>
> +#include <net/pkt_sched.h>
>  
>  #include "hellcreek.h"
>  #include "hellcreek_ptp.h"
> @@ -153,6 +155,15 @@ static void hellcreek_select_vlan(struct hellcreek *hellcreek, int vid,
>  	hellcreek_write(hellcreek, val, HR_VIDCFG);
>  }
>  
> +static void hellcreek_select_tgd(struct hellcreek *hellcreek, int port)
> +{
> +	u16 val = 0;
> +
> +	val |= port << TR_TGDSEL_TDGSEL_SHIFT;
> +
> +	hellcreek_write(hellcreek, val, TR_TGDSEL);
> +}
> +
>  static int hellcreek_wait_until_ready(struct hellcreek *hellcreek)
>  {
>  	u16 val;
> @@ -958,6 +969,24 @@ static void __hellcreek_setup_tc_identity_mapping(struct hellcreek *hellcreek)
>  	}
>  }
>  
> +static void hellcreek_setup_tc_mapping(struct hellcreek *hellcreek,
> +				       struct net_device *netdev)
> +{
> +	int i, j;
> +
> +	/* Setup mapping between traffic classes and port queues. */
> +	for (i = 0; i < netdev_get_num_tc(netdev); ++i) {
> +		for (j = 0; j < netdev->tc_to_txq[i].count; ++j) {
> +			const int queue = j + netdev->tc_to_txq[i].offset;
> +
> +			hellcreek_select_prio(hellcreek, i);
> +			hellcreek_write(hellcreek,
> +					queue << HR_PRTCCFG_PCP_TC_MAP_SHIFT,
> +					HR_PRTCCFG);
> +		}
> +	}
> +}

What other driver have you seen that does this?

> +
> +static int hellcreek_port_set_schedule(struct dsa_switch *ds, int port,
> +				       const struct tc_taprio_qopt_offload *taprio)
> +{
> +	struct net_device *netdev = dsa_to_port(ds, port)->slave;
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port *hellcreek_port;
> +	struct hellcreek_schedule *schedule;
> +	unsigned long flags;
> +	ktime_t start;
> +	u16 ctrl;
> +
> +	hellcreek_port = &hellcreek->ports[port];
> +
> +	/* Convert taprio data to hellcreek schedule */
> +	schedule = hellcreek_taprio_to_schedule(taprio);
> +	if (IS_ERR(schedule))
> +		return PTR_ERR(schedule);
> +
> +	dev_dbg(hellcreek->dev, "Configure traffic schedule on port %d\n",
> +		port);
> +
> +	/* Cancel an in flight timer */
> +	hrtimer_cancel(&hellcreek_port->cycle_start_timer);
> +
> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
> +
> +	if (hellcreek_port->current_schedule) {
> +		kfree(hellcreek_port->current_schedule->entries);
> +		kfree(hellcreek_port->current_schedule);
> +	}
> +
> +	hellcreek_port->current_schedule = schedule;
> +
> +	/* First select port */
> +	hellcreek_select_tgd(hellcreek, port);
> +
> +	/* Setup traffic class <-> queue mapping */
> +	hellcreek_setup_tc_mapping(hellcreek, netdev);
> +
> +	/* Enable gating and set the admin state to forward everything in the
> +	 * mean time
> +	 */
> +	ctrl = (0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT) | TR_TGDCTRL_GATE_EN;
> +	hellcreek_write(hellcreek, ctrl, TR_TGDCTRL);
> +
> +	/* Cancel pending schedule */
> +	hellcreek_write(hellcreek, 0x00, TR_ESTCMD);
> +
> +	/* Setup a new schedule */
> +	hellcreek_setup_gcl(hellcreek, port, schedule);
> +
> +	/* Configure cycle time */
> +	hellcreek_set_cycle_time(hellcreek, schedule);
> +
> +	/* Setup timer for schedule switch: The IP core only allows to set a
> +	 * cycle start timer 8 seconds in the future. This is why we setup the
> +	 * hritmer to base_time - 5 seconds. Then, we have enough time to
> +	 * activate IP core's EST timer.
> +	 */
> +	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
> +	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
> +			       NSEC_PER_SEC, HRTIMER_MODE_ABS);
> +
> +	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
> +
> +	return 0;
> +}

Thanks,
-Vladimir
Kurt Kanzenbach Aug. 24, 2020, 6:10 a.m. UTC | #2
On Sat Aug 22 2020, Vladimir Oltean wrote:
> Hi Kurt,
>
> On Thu, Aug 20, 2020 at 10:11:15AM +0200, Kurt Kanzenbach wrote:
>> The switch has support for the 802.1Qbv Time Aware Shaper (TAS). Traffic
>> schedules may be configured individually on each front port. Each port has eight
>> egress queues. The traffic is mapped to a traffic class respectively via the PCP
>> field of a VLAN tagged frame.
>> 
>> The TAPRIO Qdisc already implements that. Therefore, this interface can simply
>> be reused. Add .port_setup_tc() accordingly.
>> 
>> The activation of a schedule on a port is split into two parts:
>> 
>>  * Programming the necessary gate control list (GCL)
>>  * Setup hrtimer for starting the schedule
>> 
>> The hardware supports starting a schedule up to eight seconds in the future. The
>> TAPRIO interface provides an absolute base time. Therefore, hrtimers are
>> leveraged.
>> 
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>>  drivers/net/dsa/hirschmann/hellcreek.c | 294 +++++++++++++++++++++++++
>>  drivers/net/dsa/hirschmann/hellcreek.h |  21 ++
>>  2 files changed, 315 insertions(+)
>> 
>> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
>> index 745ca60342b4..e5b54f42c635 100644
>> --- a/drivers/net/dsa/hirschmann/hellcreek.c
>> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
>> @@ -22,7 +22,9 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/delay.h>
>>  #include <linux/ktime.h>
>> +#include <linux/time.h>
>>  #include <net/dsa.h>
>> +#include <net/pkt_sched.h>
>>  
>>  #include "hellcreek.h"
>>  #include "hellcreek_ptp.h"
>> @@ -153,6 +155,15 @@ static void hellcreek_select_vlan(struct hellcreek *hellcreek, int vid,
>>  	hellcreek_write(hellcreek, val, HR_VIDCFG);
>>  }
>>  
>> +static void hellcreek_select_tgd(struct hellcreek *hellcreek, int port)
>> +{
>> +	u16 val = 0;
>> +
>> +	val |= port << TR_TGDSEL_TDGSEL_SHIFT;
>> +
>> +	hellcreek_write(hellcreek, val, TR_TGDSEL);
>> +}
>> +
>>  static int hellcreek_wait_until_ready(struct hellcreek *hellcreek)
>>  {
>>  	u16 val;
>> @@ -958,6 +969,24 @@ static void __hellcreek_setup_tc_identity_mapping(struct hellcreek *hellcreek)
>>  	}
>>  }
>>  
>> +static void hellcreek_setup_tc_mapping(struct hellcreek *hellcreek,
>> +				       struct net_device *netdev)
>> +{
>> +	int i, j;
>> +
>> +	/* Setup mapping between traffic classes and port queues. */
>> +	for (i = 0; i < netdev_get_num_tc(netdev); ++i) {
>> +		for (j = 0; j < netdev->tc_to_txq[i].count; ++j) {
>> +			const int queue = j + netdev->tc_to_txq[i].offset;
>> +
>> +			hellcreek_select_prio(hellcreek, i);
>> +			hellcreek_write(hellcreek,
>> +					queue << HR_PRTCCFG_PCP_TC_MAP_SHIFT,
>> +					HR_PRTCCFG);
>> +		}
>> +	}
>> +}
>
> What other driver have you seen that does this?
>

Probably none.

With TAPRIO traffic classes and the mapping to queues can be
configured. The switch can also map traffic classes. That sounded like a
good match to me.

Thanks,
Kurt
Vladimir Oltean Aug. 24, 2020, 10:56 p.m. UTC | #3
On Thu, Aug 20, 2020 at 10:11:15AM +0200, Kurt Kanzenbach wrote:
> The switch has support for the 802.1Qbv Time Aware Shaper (TAS). Traffic
> schedules may be configured individually on each front port. Each port has eight
> egress queues. The traffic is mapped to a traffic class respectively via the PCP
> field of a VLAN tagged frame.
> 
> The TAPRIO Qdisc already implements that. Therefore, this interface can simply
> be reused. Add .port_setup_tc() accordingly.
> 
> The activation of a schedule on a port is split into two parts:
> 
>  * Programming the necessary gate control list (GCL)
>  * Setup hrtimer for starting the schedule
> 
> The hardware supports starting a schedule up to eight seconds in the future. The
> TAPRIO interface provides an absolute base time. Therefore, hrtimers are
> leveraged.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/dsa/hirschmann/hellcreek.c | 294 +++++++++++++++++++++++++
>  drivers/net/dsa/hirschmann/hellcreek.h |  21 ++
>  2 files changed, 315 insertions(+)
> 
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
> index 745ca60342b4..e5b54f42c635 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -22,7 +22,9 @@
>  #include <linux/spinlock.h>
>  #include <linux/delay.h>
>  #include <linux/ktime.h>
> +#include <linux/time.h>
>  #include <net/dsa.h>
> +#include <net/pkt_sched.h>
>  
>  #include "hellcreek.h"
>  #include "hellcreek_ptp.h"
> @@ -153,6 +155,15 @@ static void hellcreek_select_vlan(struct hellcreek *hellcreek, int vid,
>  	hellcreek_write(hellcreek, val, HR_VIDCFG);
>  }
>  
> +static void hellcreek_select_tgd(struct hellcreek *hellcreek, int port)
> +{
> +	u16 val = 0;
> +
> +	val |= port << TR_TGDSEL_TDGSEL_SHIFT;
> +
> +	hellcreek_write(hellcreek, val, TR_TGDSEL);
> +}
> +
>  static int hellcreek_wait_until_ready(struct hellcreek *hellcreek)
>  {
>  	u16 val;
> @@ -958,6 +969,24 @@ static void __hellcreek_setup_tc_identity_mapping(struct hellcreek *hellcreek)
>  	}
>  }
>  
> +static void hellcreek_setup_tc_mapping(struct hellcreek *hellcreek,
> +				       struct net_device *netdev)
> +{
> +	int i, j;
> +
> +	/* Setup mapping between traffic classes and port queues. */
> +	for (i = 0; i < netdev_get_num_tc(netdev); ++i) {
> +		for (j = 0; j < netdev->tc_to_txq[i].count; ++j) {
> +			const int queue = j + netdev->tc_to_txq[i].offset;
> +
> +			hellcreek_select_prio(hellcreek, i);
> +			hellcreek_write(hellcreek,
> +					queue << HR_PRTCCFG_PCP_TC_MAP_SHIFT,
> +					HR_PRTCCFG);
> +		}
> +	}
> +}
> +
>  static void hellcreek_setup_tc_identity_mapping(struct hellcreek *hellcreek)
>  {
>  	unsigned long flags;
> @@ -1081,6 +1110,267 @@ static void hellcreek_phylink_validate(struct dsa_switch *ds, int port,
>  		   __ETHTOOL_LINK_MODE_MASK_NBITS);
>  }
>  
> +static void hellcreek_setup_gcl(struct hellcreek *hellcreek, int port,
> +				const struct hellcreek_schedule *schedule)
> +{
> +	size_t i;
> +
> +	for (i = 1; i <= schedule->num_entries; ++i) {
> +		const struct hellcreek_gcl_entry *cur, *initial, *next;
> +		u16 data;
> +		u8 gates;
> +
> +		cur	= &schedule->entries[i - 1];
> +		initial = &schedule->entries[0];
> +		next	= &schedule->entries[i];
> +
> +		if (i == schedule->num_entries)
> +			gates = initial->gate_states ^
> +				cur->gate_states;
> +		else
> +			gates = next->gate_states ^
> +				cur->gate_states;
> +
> +		data = gates;
> +		if (cur->overrun_ignore)
> +			data |= TR_GCLDAT_GCLOVRI;
> +
> +		if (i == schedule->num_entries)
> +			data |= TR_GCLDAT_GCLWRLAST;
> +
> +		/* Gates states */
> +		hellcreek_write(hellcreek, data, TR_GCLDAT);
> +
> +		/* Time intervall */
> +		hellcreek_write(hellcreek,
> +				cur->interval & 0x0000ffff,
> +				TR_GCLTIL);
> +		hellcreek_write(hellcreek,
> +				(cur->interval & 0xffff0000) >> 16,
> +				TR_GCLTIH);
> +
> +		/* Commit entry */
> +		data = ((i - 1) << TR_GCLCMD_GCLWRADR_SHIFT) |
> +			(initial->gate_states <<
> +			 TR_GCLCMD_INIT_GATE_STATES_SHIFT);
> +		hellcreek_write(hellcreek, data, TR_GCLCMD);
> +	}
> +}
> +
> +static void hellcreek_set_cycle_time(struct hellcreek *hellcreek,
> +				     const struct hellcreek_schedule *schedule)
> +{
> +	u32 cycle_time = schedule->cycle_time;
> +
> +	hellcreek_write(hellcreek, cycle_time & 0x0000ffff, TR_CTWRL);
> +	hellcreek_write(hellcreek, (cycle_time & 0xffff0000) >> 16, TR_CTWRH);
> +}
> +
> +static void hellcreek_start_schedule(struct hellcreek *hellcreek,
> +				     ktime_t start_time)
> +{
> +	struct timespec64 ts = ktime_to_timespec64(start_time);
> +
> +	/* Start can be only 8 seconds in the future */
> +	ts.tv_sec %= 8;
> +
> +	/* Start schedule at this point of time */
> +	hellcreek_write(hellcreek, ts.tv_nsec & 0x0000ffff, TR_ESTWRL);
> +	hellcreek_write(hellcreek, (ts.tv_nsec & 0xffff0000) >> 16, TR_ESTWRH);
> +
> +	/* Arm timer, set seconds and switch schedule */
> +	hellcreek_write(hellcreek, TR_ESTCMD_ESTARM | TR_ESTCMD_ESTSWCFG |
> +		     ((ts.tv_sec & TR_ESTCMD_ESTSEC_MASK) <<
> +		      TR_ESTCMD_ESTSEC_SHIFT), TR_ESTCMD);
> +}
> +
> +static struct hellcreek_schedule *hellcreek_taprio_to_schedule(
> +	const struct tc_taprio_qopt_offload *taprio)

Personal indentation preference:

static struct hellcreek_schedule
*hellcreek_taprio_to_schedule(const struct tc_taprio_qopt_offload *taprio)

> +{
> +	struct hellcreek_schedule *schedule;
> +	size_t i;
> +
> +	/* Allocate some memory first */
> +	schedule = kzalloc(sizeof(*schedule), GFP_KERNEL);
> +	if (!schedule)
> +		return ERR_PTR(-ENOMEM);
> +	schedule->entries = kcalloc(taprio->num_entries,
> +				    sizeof(*schedule->entries),
> +				    GFP_KERNEL);
> +	if (!schedule->entries) {
> +		kfree(schedule);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* Construct hellcreek schedule */
> +	schedule->num_entries = taprio->num_entries;
> +	schedule->base_time   = taprio->base_time;
> +
> +	for (i = 0; i < taprio->num_entries; ++i) {
> +		const struct tc_taprio_sched_entry *t = &taprio->entries[i];
> +		struct hellcreek_gcl_entry *k = &schedule->entries[i];
> +
> +		k->interval	  = t->interval;
> +		k->gate_states	  = t->gate_mask;
> +		k->overrun_ignore = 0;

Tab to align with gate_states and interval?
What does overrun_ignore do, anyway?

> +
> +		/* Update complete cycle time */
> +		schedule->cycle_time += t->interval;
> +	}
> +
> +	return schedule;
> +}
> +
> +static enum hrtimer_restart hellcreek_set_schedule(struct hrtimer *timer)
> +{
> +	struct hellcreek_port *hellcreek_port =
> +		hrtimer_to_hellcreek_port(timer);

That moment when not even the helper macro fits in 80 characters..
I think you should let this line have 81 characters.

> +	struct hellcreek *hellcreek = hellcreek_port->hellcreek;
> +	struct hellcreek_schedule *schedule;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
> +
> +	/* First select port */
> +	hellcreek_select_tgd(hellcreek, hellcreek_port->port);
> +
> +	/* Set admin base time and switch schedule */
> +	hellcreek_start_schedule(hellcreek,
> +				 hellcreek_port->current_schedule->base_time);
> +
> +	schedule = hellcreek_port->current_schedule;
> +	hellcreek_port->current_schedule = NULL;
> +
> +	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
> +
> +	dev_dbg(hellcreek->dev, "ARMed EST timer for port %d\n",
> +		hellcreek_port->port);
> +
> +	/* Free resources */
> +	kfree(schedule->entries);
> +	kfree(schedule);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
> +static int hellcreek_port_set_schedule(struct dsa_switch *ds, int port,
> +				       const struct tc_taprio_qopt_offload *taprio)
> +{
> +	struct net_device *netdev = dsa_to_port(ds, port)->slave;
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port *hellcreek_port;
> +	struct hellcreek_schedule *schedule;
> +	unsigned long flags;
> +	ktime_t start;
> +	u16 ctrl;
> +
> +	hellcreek_port = &hellcreek->ports[port];
> +
> +	/* Convert taprio data to hellcreek schedule */
> +	schedule = hellcreek_taprio_to_schedule(taprio);
> +	if (IS_ERR(schedule))
> +		return PTR_ERR(schedule);
> +
> +	dev_dbg(hellcreek->dev, "Configure traffic schedule on port %d\n",
> +		port);
> +
> +	/* Cancel an in flight timer */
> +	hrtimer_cancel(&hellcreek_port->cycle_start_timer);
> +
> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
> +
> +	if (hellcreek_port->current_schedule) {
> +		kfree(hellcreek_port->current_schedule->entries);
> +		kfree(hellcreek_port->current_schedule);
> +	}
> +
> +	hellcreek_port->current_schedule = schedule;
> +
> +	/* First select port */
> +	hellcreek_select_tgd(hellcreek, port);
> +
> +	/* Setup traffic class <-> queue mapping */
> +	hellcreek_setup_tc_mapping(hellcreek, netdev);
> +
> +	/* Enable gating and set the admin state to forward everything in the
> +	 * mean time
> +	 */
> +	ctrl = (0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT) | TR_TGDCTRL_GATE_EN;
> +	hellcreek_write(hellcreek, ctrl, TR_TGDCTRL);
> +
> +	/* Cancel pending schedule */
> +	hellcreek_write(hellcreek, 0x00, TR_ESTCMD);
> +
> +	/* Setup a new schedule */
> +	hellcreek_setup_gcl(hellcreek, port, schedule);
> +
> +	/* Configure cycle time */
> +	hellcreek_set_cycle_time(hellcreek, schedule);
> +
> +	/* Setup timer for schedule switch: The IP core only allows to set a
> +	 * cycle start timer 8 seconds in the future. This is why we setup the
> +	 * hritmer to base_time - 5 seconds. Then, we have enough time to
> +	 * activate IP core's EST timer.
> +	 */
> +	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
> +	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
> +			       NSEC_PER_SEC, HRTIMER_MODE_ABS);

Explain again how this works, please? The hrtimer measures the CLOCK_TAI
of the CPU, but you are offloading the CLOCK_TAI domain of the NIC? So
you are assuming that the CPU and the NIC PHC are synchronized? What if
they aren't?

And what if the base-time is in the past, do you deal with that (how
does the hardware deal with a base-time in the past)?
A base-time in the past (example: 0) should work: you should advance the
base-time into the nearest future multiple of the cycle-time, to at
least preserve phase correctness of the schedule.

Just trying to understand if this whole hrtimer thing is worth it. It
complicates the driver by quite a significant amount.

> +
> +	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int hellcreek_port_del_schedule(struct dsa_switch *ds, int port)
> +{
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port *hellcreek_port;
> +	unsigned long flags;
> +
> +	hellcreek_port = &hellcreek->ports[port];
> +
> +	dev_dbg(hellcreek->dev, "Remove traffic schedule on port %d\n", port);
> +
> +	/* First cancel timer */
> +	hrtimer_cancel(&hellcreek_port->cycle_start_timer);
> +
> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
> +
> +	if (hellcreek_port->current_schedule) {
> +		kfree(hellcreek_port->current_schedule->entries);
> +		kfree(hellcreek_port->current_schedule);
> +		hellcreek_port->current_schedule = NULL;
> +	}
> +
> +	/* Then select port */
> +	hellcreek_select_tgd(hellcreek, port);
> +
> +	/* Revert tc mapping */
> +	__hellcreek_setup_tc_identity_mapping(hellcreek);
> +
> +	/* Disable gating and return to regular switching flow */
> +	hellcreek_write(hellcreek, 0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT,
> +			TR_TGDCTRL);
> +
> +	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int hellcreek_port_setup_tc(struct dsa_switch *ds, int port,
> +				   enum tc_setup_type type, void *type_data)
> +{
> +	const struct tc_taprio_qopt_offload *taprio = type_data;
> +
> +	if (type != TC_SETUP_QDISC_TAPRIO)
> +		return -EOPNOTSUPP;
> +
> +	if (taprio->enable)
> +		return hellcreek_port_set_schedule(ds, port, taprio);
> +
> +	return hellcreek_port_del_schedule(ds, port);
> +}
> +
>  static const struct dsa_switch_ops hellcreek_ds_ops = {
>  	.get_tag_protocol    = hellcreek_get_tag_protocol,
>  	.setup		     = hellcreek_setup,
> @@ -1104,6 +1394,7 @@ static const struct dsa_switch_ops hellcreek_ds_ops = {
>  	.port_hwtstamp_get   = hellcreek_port_hwtstamp_get,
>  	.port_txtstamp	     = hellcreek_port_txtstamp,
>  	.port_rxtstamp	     = hellcreek_port_rxtstamp,
> +	.port_setup_tc	     = hellcreek_port_setup_tc,
>  	.get_ts_info	     = hellcreek_get_ts_info,
>  };
>  
> @@ -1135,6 +1426,9 @@ static int hellcreek_probe(struct platform_device *pdev)
>  		if (!port->counter_values)
>  			return -ENOMEM;
>  
> +		hrtimer_init(&port->cycle_start_timer, CLOCK_TAI,
> +			     HRTIMER_MODE_ABS);
> +		port->cycle_start_timer.function = hellcreek_set_schedule;
>  		port->hellcreek = hellcreek;
>  		port->port	= i;
>  	}
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.h b/drivers/net/dsa/hirschmann/hellcreek.h
> index 1d3de72a48a5..d3d1a1144857 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.h
> +++ b/drivers/net/dsa/hirschmann/hellcreek.h
> @@ -16,6 +16,7 @@
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/timecounter.h>
>  #include <linux/spinlock.h>
> +#include <linux/hrtimer.h>
>  #include <net/dsa.h>
>  
>  /* Ports:
> @@ -210,6 +211,20 @@ struct hellcreek_counter {
>  	const char *name;
>  };
>  
> +struct hellcreek_gcl_entry {
> +	u32 interval;
> +	u8 gate_states;
> +	bool overrun_ignore;
> +};
> +
> +struct hellcreek_schedule {
> +	struct hellcreek_gcl_entry *entries;
> +	size_t num_entries;
> +	ktime_t base_time;
> +	u32 cycle_time;
> +	int port;
> +};
> +
>  struct hellcreek;
>  
>  /* State flags for hellcreek_port_hwtstamp::state */
> @@ -236,6 +251,8 @@ struct hellcreek_port_hwtstamp {
>  
>  struct hellcreek_port {
>  	struct hellcreek *hellcreek;
> +	struct hellcreek_schedule *current_schedule;
> +	struct hrtimer cycle_start_timer;
>  	int port;
>  	u16 ptcfg;		/* ptcfg shadow */
>  	u64 *counter_values;
> @@ -273,4 +290,8 @@ struct hellcreek {
>  	size_t fdb_entries;
>  };
>  
> +#define hrtimer_to_hellcreek_port(timer)		\
> +	container_of(timer, struct hellcreek_port,	\
> +		     cycle_start_timer)
> +
>  #endif /* _HELLCREEK_H_ */
> -- 
> 2.20.1
> 

Thanks,
-Vladimir
Vinicius Costa Gomes Aug. 24, 2020, 11:45 p.m. UTC | #4
Hi Kurt,

Kurt Kanzenbach <kurt@linutronix.de> writes:

>>> +static void hellcreek_setup_tc_mapping(struct hellcreek *hellcreek,
>>> +				       struct net_device *netdev)
>>> +{
>>> +	int i, j;
>>> +
>>> +	/* Setup mapping between traffic classes and port queues. */
>>> +	for (i = 0; i < netdev_get_num_tc(netdev); ++i) {
>>> +		for (j = 0; j < netdev->tc_to_txq[i].count; ++j) {
>>> +			const int queue = j + netdev->tc_to_txq[i].offset;
>>> +
>>> +			hellcreek_select_prio(hellcreek, i);
>>> +			hellcreek_write(hellcreek,
>>> +					queue << HR_PRTCCFG_PCP_TC_MAP_SHIFT,
>>> +					HR_PRTCCFG);
>>> +		}
>>> +	}
>>> +}
>>
>> What other driver have you seen that does this?
>>
>
> Probably none.
>
> With TAPRIO traffic classes and the mapping to queues can be
> configured. The switch can also map traffic classes. That sounded like a
> good match to me.

The only reason I could think that you would need this that *right now*
taprio has pretty glaring oversight: that in the offload parameters each entry
'gate_mask' reference the "Traffic Class" (i.e. bit 0 is Traffic Class
0), and it really should be the HW queue.

I have a patch that does the conversion on taprio before talking to the
driver. Do you think it would help you avoid doing this on the driver
side?

>
> Thanks,
> Kurt


Cheers,
Vinicius Costa Gomes Aug. 24, 2020, 11:57 p.m. UTC | #5
Hi,

Kurt Kanzenbach <kurt@linutronix.de> writes:

> The switch has support for the 802.1Qbv Time Aware Shaper (TAS). Traffic
> schedules may be configured individually on each front port. Each port has eight
> egress queues. The traffic is mapped to a traffic class respectively via the PCP
> field of a VLAN tagged frame.
>
> The TAPRIO Qdisc already implements that. Therefore, this interface can simply
> be reused. Add .port_setup_tc() accordingly.
>
> The activation of a schedule on a port is split into two parts:
>
>  * Programming the necessary gate control list (GCL)
>  * Setup hrtimer for starting the schedule
>
> The hardware supports starting a schedule up to eight seconds in the future. The
> TAPRIO interface provides an absolute base time. Therefore, hrtimers are
> leveraged.

The driver side looks good, looks like a well behaved piece of hardware,
even not supporting schedules with a base time 8 seconds (or later) in
the future is not so bad.

>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/dsa/hirschmann/hellcreek.c | 294 +++++++++++++++++++++++++
>  drivers/net/dsa/hirschmann/hellcreek.h |  21 ++
>  2 files changed, 315 insertions(+)
>
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
> index 745ca60342b4..e5b54f42c635 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -22,7 +22,9 @@
>  #include <linux/spinlock.h>
>  #include <linux/delay.h>
>  #include <linux/ktime.h>
> +#include <linux/time.h>
>  #include <net/dsa.h>
> +#include <net/pkt_sched.h>
>  
>  #include "hellcreek.h"
>  #include "hellcreek_ptp.h"
> @@ -153,6 +155,15 @@ static void hellcreek_select_vlan(struct hellcreek *hellcreek, int vid,
>  	hellcreek_write(hellcreek, val, HR_VIDCFG);
>  }
>  
> +static void hellcreek_select_tgd(struct hellcreek *hellcreek, int port)
> +{
> +	u16 val = 0;
> +
> +	val |= port << TR_TGDSEL_TDGSEL_SHIFT;
> +
> +	hellcreek_write(hellcreek, val, TR_TGDSEL);
> +}
> +
>  static int hellcreek_wait_until_ready(struct hellcreek *hellcreek)
>  {
>  	u16 val;
> @@ -958,6 +969,24 @@ static void __hellcreek_setup_tc_identity_mapping(struct hellcreek *hellcreek)
>  	}
>  }
>  
> +static void hellcreek_setup_tc_mapping(struct hellcreek *hellcreek,
> +				       struct net_device *netdev)
> +{
> +	int i, j;
> +
> +	/* Setup mapping between traffic classes and port queues. */
> +	for (i = 0; i < netdev_get_num_tc(netdev); ++i) {
> +		for (j = 0; j < netdev->tc_to_txq[i].count; ++j) {
> +			const int queue = j + netdev->tc_to_txq[i].offset;
> +
> +			hellcreek_select_prio(hellcreek, i);
> +			hellcreek_write(hellcreek,
> +					queue << HR_PRTCCFG_PCP_TC_MAP_SHIFT,
> +					HR_PRTCCFG);
> +		}
> +	}
> +}
> +
>  static void hellcreek_setup_tc_identity_mapping(struct hellcreek *hellcreek)
>  {
>  	unsigned long flags;
> @@ -1081,6 +1110,267 @@ static void hellcreek_phylink_validate(struct dsa_switch *ds, int port,
>  		   __ETHTOOL_LINK_MODE_MASK_NBITS);
>  }
>  
> +static void hellcreek_setup_gcl(struct hellcreek *hellcreek, int port,
> +				const struct hellcreek_schedule *schedule)
> +{
> +	size_t i;
> +
> +	for (i = 1; i <= schedule->num_entries; ++i) {
> +		const struct hellcreek_gcl_entry *cur, *initial, *next;
> +		u16 data;
> +		u8 gates;
> +
> +		cur	= &schedule->entries[i - 1];
> +		initial = &schedule->entries[0];
> +		next	= &schedule->entries[i];
> +
> +		if (i == schedule->num_entries)
> +			gates = initial->gate_states ^
> +				cur->gate_states;
> +		else
> +			gates = next->gate_states ^
> +				cur->gate_states;
> +
> +		data = gates;
> +		if (cur->overrun_ignore)
> +			data |= TR_GCLDAT_GCLOVRI;
> +
> +		if (i == schedule->num_entries)
> +			data |= TR_GCLDAT_GCLWRLAST;
> +
> +		/* Gates states */
> +		hellcreek_write(hellcreek, data, TR_GCLDAT);
> +
> +		/* Time intervall */
> +		hellcreek_write(hellcreek,
> +				cur->interval & 0x0000ffff,
> +				TR_GCLTIL);
> +		hellcreek_write(hellcreek,
> +				(cur->interval & 0xffff0000) >> 16,
> +				TR_GCLTIH);
> +
> +		/* Commit entry */
> +		data = ((i - 1) << TR_GCLCMD_GCLWRADR_SHIFT) |
> +			(initial->gate_states <<
> +			 TR_GCLCMD_INIT_GATE_STATES_SHIFT);
> +		hellcreek_write(hellcreek, data, TR_GCLCMD);
> +	}
> +}
> +
> +static void hellcreek_set_cycle_time(struct hellcreek *hellcreek,
> +				     const struct hellcreek_schedule *schedule)
> +{
> +	u32 cycle_time = schedule->cycle_time;
> +
> +	hellcreek_write(hellcreek, cycle_time & 0x0000ffff, TR_CTWRL);
> +	hellcreek_write(hellcreek, (cycle_time & 0xffff0000) >> 16, TR_CTWRH);
> +}
> +
> +static void hellcreek_start_schedule(struct hellcreek *hellcreek,
> +				     ktime_t start_time)
> +{
> +	struct timespec64 ts = ktime_to_timespec64(start_time);
> +
> +	/* Start can be only 8 seconds in the future */
> +	ts.tv_sec %= 8;
> +
> +	/* Start schedule at this point of time */
> +	hellcreek_write(hellcreek, ts.tv_nsec & 0x0000ffff, TR_ESTWRL);
> +	hellcreek_write(hellcreek, (ts.tv_nsec & 0xffff0000) >> 16, TR_ESTWRH);
> +
> +	/* Arm timer, set seconds and switch schedule */
> +	hellcreek_write(hellcreek, TR_ESTCMD_ESTARM | TR_ESTCMD_ESTSWCFG |
> +		     ((ts.tv_sec & TR_ESTCMD_ESTSEC_MASK) <<
> +		      TR_ESTCMD_ESTSEC_SHIFT), TR_ESTCMD);
> +}
> +
> +static struct hellcreek_schedule *hellcreek_taprio_to_schedule(
> +	const struct tc_taprio_qopt_offload *taprio)
> +{
> +	struct hellcreek_schedule *schedule;
> +	size_t i;
> +
> +	/* Allocate some memory first */
> +	schedule = kzalloc(sizeof(*schedule), GFP_KERNEL);
> +	if (!schedule)
> +		return ERR_PTR(-ENOMEM);
> +	schedule->entries = kcalloc(taprio->num_entries,
> +				    sizeof(*schedule->entries),
> +				    GFP_KERNEL);
> +	if (!schedule->entries) {
> +		kfree(schedule);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* Construct hellcreek schedule */
> +	schedule->num_entries = taprio->num_entries;
> +	schedule->base_time   = taprio->base_time;
> +
> +	for (i = 0; i < taprio->num_entries; ++i) {
> +		const struct tc_taprio_sched_entry *t = &taprio->entries[i];
> +		struct hellcreek_gcl_entry *k = &schedule->entries[i];
> +
> +		k->interval	  = t->interval;
> +		k->gate_states	  = t->gate_mask;
> +		k->overrun_ignore = 0;
> +
> +		/* Update complete cycle time */
> +		schedule->cycle_time += t->interval;
> +	}
> +
> +	return schedule;
> +}
> +
> +static enum hrtimer_restart hellcreek_set_schedule(struct hrtimer *timer)
> +{
> +	struct hellcreek_port *hellcreek_port =
> +		hrtimer_to_hellcreek_port(timer);
> +	struct hellcreek *hellcreek = hellcreek_port->hellcreek;
> +	struct hellcreek_schedule *schedule;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
> +
> +	/* First select port */
> +	hellcreek_select_tgd(hellcreek, hellcreek_port->port);
> +
> +	/* Set admin base time and switch schedule */
> +	hellcreek_start_schedule(hellcreek,
> +				 hellcreek_port->current_schedule->base_time);
> +
> +	schedule = hellcreek_port->current_schedule;
> +	hellcreek_port->current_schedule = NULL;
> +
> +	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
> +
> +	dev_dbg(hellcreek->dev, "ARMed EST timer for port %d\n",
> +		hellcreek_port->port);
> +
> +	/* Free resources */
> +	kfree(schedule->entries);
> +	kfree(schedule);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
> +static int hellcreek_port_set_schedule(struct dsa_switch *ds, int port,
> +				       const struct tc_taprio_qopt_offload *taprio)
> +{
> +	struct net_device *netdev = dsa_to_port(ds, port)->slave;
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port *hellcreek_port;
> +	struct hellcreek_schedule *schedule;
> +	unsigned long flags;
> +	ktime_t start;
> +	u16 ctrl;
> +
> +	hellcreek_port = &hellcreek->ports[port];
> +
> +	/* Convert taprio data to hellcreek schedule */
> +	schedule = hellcreek_taprio_to_schedule(taprio);
> +	if (IS_ERR(schedule))
> +		return PTR_ERR(schedule);
> +
> +	dev_dbg(hellcreek->dev, "Configure traffic schedule on port %d\n",
> +		port);
> +
> +	/* Cancel an in flight timer */
> +	hrtimer_cancel(&hellcreek_port->cycle_start_timer);
> +
> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
> +
> +	if (hellcreek_port->current_schedule) {
> +		kfree(hellcreek_port->current_schedule->entries);
> +		kfree(hellcreek_port->current_schedule);
> +	}
> +
> +	hellcreek_port->current_schedule = schedule;
> +
> +	/* First select port */
> +	hellcreek_select_tgd(hellcreek, port);
> +
> +	/* Setup traffic class <-> queue mapping */
> +	hellcreek_setup_tc_mapping(hellcreek, netdev);
> +
> +	/* Enable gating and set the admin state to forward everything in the
> +	 * mean time
> +	 */
> +	ctrl = (0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT) | TR_TGDCTRL_GATE_EN;
> +	hellcreek_write(hellcreek, ctrl, TR_TGDCTRL);
> +
> +	/* Cancel pending schedule */
> +	hellcreek_write(hellcreek, 0x00, TR_ESTCMD);
> +
> +	/* Setup a new schedule */
> +	hellcreek_setup_gcl(hellcreek, port, schedule);
> +
> +	/* Configure cycle time */
> +	hellcreek_set_cycle_time(hellcreek, schedule);
> +
> +	/* Setup timer for schedule switch: The IP core only allows to set a
> +	 * cycle start timer 8 seconds in the future. This is why we setup the
> +	 * hritmer to base_time - 5 seconds. Then, we have enough time to
> +	 * activate IP core's EST timer.
> +	 */
> +	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
> +	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
> +			       NSEC_PER_SEC, HRTIMER_MODE_ABS);

If we are talking about seconds here, I don't think you need to use a
hrtimer, you could use a workqueue/delayed_work. Should make things a
bit simpler. 

> +
> +	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int hellcreek_port_del_schedule(struct dsa_switch *ds, int port)
> +{
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port *hellcreek_port;
> +	unsigned long flags;
> +
> +	hellcreek_port = &hellcreek->ports[port];
> +
> +	dev_dbg(hellcreek->dev, "Remove traffic schedule on port %d\n", port);
> +
> +	/* First cancel timer */
> +	hrtimer_cancel(&hellcreek_port->cycle_start_timer);
> +
> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
> +
> +	if (hellcreek_port->current_schedule) {
> +		kfree(hellcreek_port->current_schedule->entries);
> +		kfree(hellcreek_port->current_schedule);
> +		hellcreek_port->current_schedule = NULL;
> +	}
> +
> +	/* Then select port */
> +	hellcreek_select_tgd(hellcreek, port);
> +
> +	/* Revert tc mapping */
> +	__hellcreek_setup_tc_identity_mapping(hellcreek);
> +
> +	/* Disable gating and return to regular switching flow */
> +	hellcreek_write(hellcreek, 0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT,
> +			TR_TGDCTRL);
> +
> +	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int hellcreek_port_setup_tc(struct dsa_switch *ds, int port,
> +				   enum tc_setup_type type, void *type_data)
> +{
> +	const struct tc_taprio_qopt_offload *taprio = type_data;
> +
> +	if (type != TC_SETUP_QDISC_TAPRIO)
> +		return -EOPNOTSUPP;
> +
> +	if (taprio->enable)
> +		return hellcreek_port_set_schedule(ds, port, taprio);
> +
> +	return hellcreek_port_del_schedule(ds, port);
> +}
> +
>  static const struct dsa_switch_ops hellcreek_ds_ops = {
>  	.get_tag_protocol    = hellcreek_get_tag_protocol,
>  	.setup		     = hellcreek_setup,
> @@ -1104,6 +1394,7 @@ static const struct dsa_switch_ops hellcreek_ds_ops = {
>  	.port_hwtstamp_get   = hellcreek_port_hwtstamp_get,
>  	.port_txtstamp	     = hellcreek_port_txtstamp,
>  	.port_rxtstamp	     = hellcreek_port_rxtstamp,
> +	.port_setup_tc	     = hellcreek_port_setup_tc,
>  	.get_ts_info	     = hellcreek_get_ts_info,
>  };
>  
> @@ -1135,6 +1426,9 @@ static int hellcreek_probe(struct platform_device *pdev)
>  		if (!port->counter_values)
>  			return -ENOMEM;
>  
> +		hrtimer_init(&port->cycle_start_timer, CLOCK_TAI,
> +			     HRTIMER_MODE_ABS);
> +		port->cycle_start_timer.function = hellcreek_set_schedule;
>  		port->hellcreek = hellcreek;
>  		port->port	= i;
>  	}
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.h b/drivers/net/dsa/hirschmann/hellcreek.h
> index 1d3de72a48a5..d3d1a1144857 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.h
> +++ b/drivers/net/dsa/hirschmann/hellcreek.h
> @@ -16,6 +16,7 @@
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/timecounter.h>
>  #include <linux/spinlock.h>
> +#include <linux/hrtimer.h>
>  #include <net/dsa.h>
>  
>  /* Ports:
> @@ -210,6 +211,20 @@ struct hellcreek_counter {
>  	const char *name;
>  };
>  
> +struct hellcreek_gcl_entry {
> +	u32 interval;
> +	u8 gate_states;
> +	bool overrun_ignore;
> +};
> +
> +struct hellcreek_schedule {
> +	struct hellcreek_gcl_entry *entries;
> +	size_t num_entries;
> +	ktime_t base_time;
> +	u32 cycle_time;
> +	int port;
> +};
> +
>  struct hellcreek;
>  
>  /* State flags for hellcreek_port_hwtstamp::state */
> @@ -236,6 +251,8 @@ struct hellcreek_port_hwtstamp {
>  
>  struct hellcreek_port {
>  	struct hellcreek *hellcreek;
> +	struct hellcreek_schedule *current_schedule;
> +	struct hrtimer cycle_start_timer;
>  	int port;
>  	u16 ptcfg;		/* ptcfg shadow */
>  	u64 *counter_values;
> @@ -273,4 +290,8 @@ struct hellcreek {
>  	size_t fdb_entries;
>  };
>  
> +#define hrtimer_to_hellcreek_port(timer)		\
> +	container_of(timer, struct hellcreek_port,	\
> +		     cycle_start_timer)
> +
>  #endif /* _HELLCREEK_H_ */
> -- 
> 2.20.1
>

Cheers,
Kurt Kanzenbach Aug. 25, 2020, 9:23 a.m. UTC | #6
On Mon Aug 24 2020, Vinicius Costa Gomes wrote:
> Hi,
>
> Kurt Kanzenbach <kurt@linutronix.de> writes:
>
[snip]
>> +	/* Setup timer for schedule switch: The IP core only allows to set a
>> +	 * cycle start timer 8 seconds in the future. This is why we setup the
>> +	 * hritmer to base_time - 5 seconds. Then, we have enough time to
>> +	 * activate IP core's EST timer.
>> +	 */
>> +	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
>> +	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
>> +			       NSEC_PER_SEC, HRTIMER_MODE_ABS);
>
> If we are talking about seconds here, I don't think you need to use a
> hrtimer, you could use a workqueue/delayed_work. Should make things a
> bit simpler.

I've used hrtimers for one reason: The hrtimer provides a way to fire at
an absolute base time based on CLOCK_TAI. All the other facilities such
as workqueues, timer list timers, etc do not.

In the typical setup, we run ptp4l as boundary clock (or as TAB which is
work in progress) and phc2sys to synchronize the ptp clock to the Linux
system. Let's assume we setup a TAPRIO schedule with a base time X. Now,
the grand master time changes meaning the timer has to go off earlier or
later.

Thanks,
Kurt
Vladimir Oltean Aug. 25, 2020, 9:32 a.m. UTC | #7
On Tue, Aug 25, 2020 at 11:23:56AM +0200, Kurt Kanzenbach wrote:
> On Mon Aug 24 2020, Vinicius Costa Gomes wrote:
> > Hi,
> >
> > Kurt Kanzenbach <kurt@linutronix.de> writes:
> >
> [snip]
> >> +	/* Setup timer for schedule switch: The IP core only allows to set a
> >> +	 * cycle start timer 8 seconds in the future. This is why we setup the
> >> +	 * hritmer to base_time - 5 seconds. Then, we have enough time to
> >> +	 * activate IP core's EST timer.
> >> +	 */
> >> +	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
> >> +	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
> >> +			       NSEC_PER_SEC, HRTIMER_MODE_ABS);
> >
> > If we are talking about seconds here, I don't think you need to use a
> > hrtimer, you could use a workqueue/delayed_work. Should make things a
> > bit simpler.
> 
> I've used hrtimers for one reason: The hrtimer provides a way to fire at
> an absolute base time based on CLOCK_TAI. All the other facilities such
> as workqueues, timer list timers, etc do not.

That still doesn't justify the complexity of irqsave spinlocks and such.
You could just as well schedule a workqueue from that hrtimer and have
process context...

Thanks,
-Vladimir
Kurt Kanzenbach Aug. 25, 2020, 9:33 a.m. UTC | #8
On Tue Aug 25 2020, Vladimir Oltean wrote:
> On Thu, Aug 20, 2020 at 10:11:15AM +0200, Kurt Kanzenbach wrote:
[snip]
>> +static struct hellcreek_schedule *hellcreek_taprio_to_schedule(
>> +	const struct tc_taprio_qopt_offload *taprio)
>
> Personal indentation preference:
>
> static struct hellcreek_schedule
> *hellcreek_taprio_to_schedule(const struct tc_taprio_qopt_offload *taprio)

Sure.

>
>> +{
>> +	struct hellcreek_schedule *schedule;
>> +	size_t i;
>> +
>> +	/* Allocate some memory first */
>> +	schedule = kzalloc(sizeof(*schedule), GFP_KERNEL);
>> +	if (!schedule)
>> +		return ERR_PTR(-ENOMEM);
>> +	schedule->entries = kcalloc(taprio->num_entries,
>> +				    sizeof(*schedule->entries),
>> +				    GFP_KERNEL);
>> +	if (!schedule->entries) {
>> +		kfree(schedule);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	/* Construct hellcreek schedule */
>> +	schedule->num_entries = taprio->num_entries;
>> +	schedule->base_time   = taprio->base_time;
>> +
>> +	for (i = 0; i < taprio->num_entries; ++i) {
>> +		const struct tc_taprio_sched_entry *t = &taprio->entries[i];
>> +		struct hellcreek_gcl_entry *k = &schedule->entries[i];
>> +
>> +		k->interval	  = t->interval;
>> +		k->gate_states	  = t->gate_mask;
>> +		k->overrun_ignore = 0;
>
> Tab to align with gate_states and interval?

Hm. I've used M x align. It should take care of it.

> What does overrun_ignore do, anyway?

I don't remember. The HW engineer suggested to set it to zero.

>
>> +
>> +		/* Update complete cycle time */
>> +		schedule->cycle_time += t->interval;
>> +	}
>> +
>> +	return schedule;
>> +}
>> +
>> +static enum hrtimer_restart hellcreek_set_schedule(struct hrtimer *timer)
>> +{
>> +	struct hellcreek_port *hellcreek_port =
>> +		hrtimer_to_hellcreek_port(timer);
>
> That moment when not even the helper macro fits in 80 characters..
> I think you should let this line have 81 characters.

OK.

>
>> +	struct hellcreek *hellcreek = hellcreek_port->hellcreek;
>> +	struct hellcreek_schedule *schedule;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
>> +
>> +	/* First select port */
>> +	hellcreek_select_tgd(hellcreek, hellcreek_port->port);
>> +
>> +	/* Set admin base time and switch schedule */
>> +	hellcreek_start_schedule(hellcreek,
>> +				 hellcreek_port->current_schedule->base_time);
>> +
>> +	schedule = hellcreek_port->current_schedule;
>> +	hellcreek_port->current_schedule = NULL;
>> +
>> +	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
>> +
>> +	dev_dbg(hellcreek->dev, "ARMed EST timer for port %d\n",
>> +		hellcreek_port->port);
>> +
>> +	/* Free resources */
>> +	kfree(schedule->entries);
>> +	kfree(schedule);
>> +
>> +	return HRTIMER_NORESTART;
>> +}
>> +
>> +static int hellcreek_port_set_schedule(struct dsa_switch *ds, int port,
>> +				       const struct tc_taprio_qopt_offload *taprio)
>> +{
>> +	struct net_device *netdev = dsa_to_port(ds, port)->slave;
>> +	struct hellcreek *hellcreek = ds->priv;
>> +	struct hellcreek_port *hellcreek_port;
>> +	struct hellcreek_schedule *schedule;
>> +	unsigned long flags;
>> +	ktime_t start;
>> +	u16 ctrl;
>> +
>> +	hellcreek_port = &hellcreek->ports[port];
>> +
>> +	/* Convert taprio data to hellcreek schedule */
>> +	schedule = hellcreek_taprio_to_schedule(taprio);
>> +	if (IS_ERR(schedule))
>> +		return PTR_ERR(schedule);
>> +
>> +	dev_dbg(hellcreek->dev, "Configure traffic schedule on port %d\n",
>> +		port);
>> +
>> +	/* Cancel an in flight timer */
>> +	hrtimer_cancel(&hellcreek_port->cycle_start_timer);
>> +
>> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
>> +
>> +	if (hellcreek_port->current_schedule) {
>> +		kfree(hellcreek_port->current_schedule->entries);
>> +		kfree(hellcreek_port->current_schedule);
>> +	}
>> +
>> +	hellcreek_port->current_schedule = schedule;
>> +
>> +	/* First select port */
>> +	hellcreek_select_tgd(hellcreek, port);
>> +
>> +	/* Setup traffic class <-> queue mapping */
>> +	hellcreek_setup_tc_mapping(hellcreek, netdev);
>> +
>> +	/* Enable gating and set the admin state to forward everything in the
>> +	 * mean time
>> +	 */
>> +	ctrl = (0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT) | TR_TGDCTRL_GATE_EN;
>> +	hellcreek_write(hellcreek, ctrl, TR_TGDCTRL);
>> +
>> +	/* Cancel pending schedule */
>> +	hellcreek_write(hellcreek, 0x00, TR_ESTCMD);
>> +
>> +	/* Setup a new schedule */
>> +	hellcreek_setup_gcl(hellcreek, port, schedule);
>> +
>> +	/* Configure cycle time */
>> +	hellcreek_set_cycle_time(hellcreek, schedule);
>> +
>> +	/* Setup timer for schedule switch: The IP core only allows to set a
>> +	 * cycle start timer 8 seconds in the future. This is why we setup the
>> +	 * hritmer to base_time - 5 seconds. Then, we have enough time to
>> +	 * activate IP core's EST timer.
>> +	 */
>> +	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
>> +	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
>> +			       NSEC_PER_SEC, HRTIMER_MODE_ABS);
>
> Explain again how this works, please? The hrtimer measures the CLOCK_TAI
> of the CPU, but you are offloading the CLOCK_TAI domain of the NIC? So
> you are assuming that the CPU and the NIC PHC are synchronized? What if
> they aren't?

Yes, I assume that's synchronized with e.g. phc2sys.

>
> And what if the base-time is in the past, do you deal with that (how
> does the hardware deal with a base-time in the past)?
> A base-time in the past (example: 0) should work: you should advance the
> base-time into the nearest future multiple of the cycle-time, to at
> least preserve phase correctness of the schedule.

If the hrtimer is programmed with a value in the past, it fires
instantly. The callback is executed and the start time is
programmed.

>
> Just trying to understand if this whole hrtimer thing is worth it. It
> complicates the driver by quite a significant amount.

See my other reply mail, why I used hrtimers.

Thanks,
Kurt
Vladimir Oltean Aug. 25, 2020, 9:38 a.m. UTC | #9
On Tue, Aug 25, 2020 at 11:33:53AM +0200, Kurt Kanzenbach wrote:
> On Tue Aug 25 2020, Vladimir Oltean wrote:
> > On Thu, Aug 20, 2020 at 10:11:15AM +0200, Kurt Kanzenbach wrote:
> >
> > Explain again how this works, please? The hrtimer measures the CLOCK_TAI
> > of the CPU, but you are offloading the CLOCK_TAI domain of the NIC? So
> > you are assuming that the CPU and the NIC PHC are synchronized? What if
> > they aren't?
> 
> Yes, I assume that's synchronized with e.g. phc2sys.
> 

My intuition tells me that this isn't the user's expectation, and that
it should do the right thing even if it's not synchronized to the system
clock.

> >
> > And what if the base-time is in the past, do you deal with that (how
> > does the hardware deal with a base-time in the past)?
> > A base-time in the past (example: 0) should work: you should advance the
> > base-time into the nearest future multiple of the cycle-time, to at
> > least preserve phase correctness of the schedule.
> 
> If the hrtimer is programmed with a value in the past, it fires
> instantly.

Yes, it does.

> The callback is executed and the start time is programmed.
> 

With a valid value from the hardware's perspective?

Thanks,
-Vladimir
Kurt Kanzenbach Aug. 25, 2020, 9:42 a.m. UTC | #10
On Mon Aug 24 2020, Vinicius Costa Gomes wrote:
> Hi Kurt,
>
> Kurt Kanzenbach <kurt@linutronix.de> writes:
>
>>>> +static void hellcreek_setup_tc_mapping(struct hellcreek *hellcreek,
>>>> +				       struct net_device *netdev)
>>>> +{
>>>> +	int i, j;
>>>> +
>>>> +	/* Setup mapping between traffic classes and port queues. */
>>>> +	for (i = 0; i < netdev_get_num_tc(netdev); ++i) {
>>>> +		for (j = 0; j < netdev->tc_to_txq[i].count; ++j) {
>>>> +			const int queue = j + netdev->tc_to_txq[i].offset;
>>>> +
>>>> +			hellcreek_select_prio(hellcreek, i);
>>>> +			hellcreek_write(hellcreek,
>>>> +					queue << HR_PRTCCFG_PCP_TC_MAP_SHIFT,
>>>> +					HR_PRTCCFG);
>>>> +		}
>>>> +	}
>>>> +}
>>>
>>> What other driver have you seen that does this?
>>>
>>
>> Probably none.
>>
>> With TAPRIO traffic classes and the mapping to queues can be
>> configured. The switch can also map traffic classes. That sounded like a
>> good match to me.
>
> The only reason I could think that you would need this that *right now*
> taprio has pretty glaring oversight: that in the offload parameters each entry
> 'gate_mask' reference the "Traffic Class" (i.e. bit 0 is Traffic Class
> 0), and it really should be the HW queue.
>
> I have a patch that does the conversion on taprio before talking to the
> driver. Do you think it would help you avoid doing this on the driver
> side?

I think so. As Vladimir pointed out, the driver should setup an identity
mapping which I already did by default.

Can you point me your patch?

Thanks,
Kurt
Vladimir Oltean Aug. 25, 2020, 9:46 a.m. UTC | #11
Hi Vinicius,

On Mon, Aug 24, 2020 at 04:45:50PM -0700, Vinicius Costa Gomes wrote:
> Kurt Kanzenbach <kurt@linutronix.de> writes:
> >
> > With TAPRIO traffic classes and the mapping to queues can be
> > configured. The switch can also map traffic classes. That sounded like a
> > good match to me.
>
> The only reason I could think that you would need this that *right now*
> taprio has pretty glaring oversight: that in the offload parameters each entry
> 'gate_mask' reference the "Traffic Class" (i.e. bit 0 is Traffic Class
> 0), and it really should be the HW queue.
>

Sorry, but could you please explain why having the gate_mask reference
the traffic classes is a glaring oversight, and how changing it would
help here?

Also, Kurt, could you please explain what the
HR_PRTCCFG_PCP_TC_MAP_SHIFT field in HR_PRTCCFG is doing?
To me, it appears that it's configuring ingress QoS classification on
the port (and the reason why this is strange to me is because you're
applying this configuration through an egress qdisc), but I want to make
sure I'm not misunderstanding.

Thanks,
-Vladimir
Kurt Kanzenbach Aug. 25, 2020, 9:55 a.m. UTC | #12
On Tue Aug 25 2020, Vladimir Oltean wrote:
> On Tue, Aug 25, 2020 at 11:33:53AM +0200, Kurt Kanzenbach wrote:
>> On Tue Aug 25 2020, Vladimir Oltean wrote:
>> > On Thu, Aug 20, 2020 at 10:11:15AM +0200, Kurt Kanzenbach wrote:
>> >
>> > Explain again how this works, please? The hrtimer measures the CLOCK_TAI
>> > of the CPU, but you are offloading the CLOCK_TAI domain of the NIC? So
>> > you are assuming that the CPU and the NIC PHC are synchronized? What if
>> > they aren't?
>> 
>> Yes, I assume that's synchronized with e.g. phc2sys.
>> 
>
> My intuition tells me that this isn't the user's expectation, and that
> it should do the right thing even if it's not synchronized to the system
> clock.

I get your point. But how to do it? We would need a timer based on the
PTP clock in the switch.

>
>> >
>> > And what if the base-time is in the past, do you deal with that (how
>> > does the hardware deal with a base-time in the past)?
>> > A base-time in the past (example: 0) should work: you should advance the
>> > base-time into the nearest future multiple of the cycle-time, to at
>> > least preserve phase correctness of the schedule.
>> 
>> If the hrtimer is programmed with a value in the past, it fires
>> instantly.
>
> Yes, it does.
>
>> The callback is executed and the start time is programmed.
>> 
>
> With a valid value from the hardware's perspective?

Yes. That's no problem.

Thanks,
Kurt
Kurt Kanzenbach Aug. 25, 2020, 10:09 a.m. UTC | #13
On Tue Aug 25 2020, Vladimir Oltean wrote:
> Hi Vinicius,
>
> On Mon, Aug 24, 2020 at 04:45:50PM -0700, Vinicius Costa Gomes wrote:
>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>> >
>> > With TAPRIO traffic classes and the mapping to queues can be
>> > configured. The switch can also map traffic classes. That sounded like a
>> > good match to me.
>>
>> The only reason I could think that you would need this that *right now*
>> taprio has pretty glaring oversight: that in the offload parameters each entry
>> 'gate_mask' reference the "Traffic Class" (i.e. bit 0 is Traffic Class
>> 0), and it really should be the HW queue.
>>
>
> Sorry, but could you please explain why having the gate_mask reference
> the traffic classes is a glaring oversight, and how changing it would
> help here?
>
> Also, Kurt, could you please explain what the
> HR_PRTCCFG_PCP_TC_MAP_SHIFT field in HR_PRTCCFG is doing?
> To me, it appears that it's configuring ingress QoS classification on
> the port (and the reason why this is strange to me is because you're
> applying this configuration through an egress qdisc), but I want to make
> sure I'm not misunderstanding.

All the TSN operations in the switch such as the gate control work on
internally on traffic classes. The traffic class is determined by the
PCP field on the VLAN tagged frames. This mapping is configurable via
the pcp to tc map. And the TC also defines the hardware queue.

Thanks,
Kurt
Vinicius Costa Gomes Aug. 25, 2020, 5:33 p.m. UTC | #14
Vladimir Oltean <olteanv@gmail.com> writes:

> Hi Vinicius,
>
> On Mon, Aug 24, 2020 at 04:45:50PM -0700, Vinicius Costa Gomes wrote:
>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>> >
>> > With TAPRIO traffic classes and the mapping to queues can be
>> > configured. The switch can also map traffic classes. That sounded like a
>> > good match to me.
>>
>> The only reason I could think that you would need this that *right now*
>> taprio has pretty glaring oversight: that in the offload parameters each entry
>> 'gate_mask' reference the "Traffic Class" (i.e. bit 0 is Traffic Class
>> 0), and it really should be the HW queue.
>>
>
> Sorry, but could you please explain why having the gate_mask reference
> the traffic classes is a glaring oversight, and how changing it would
> help here?

The glaring oversight is that it when it references the traffic classes,
instead of queues, it basically ignores the 'queues *' mapping that the
user provided. (it was ignored for so long because for many cases the
mapping is 1:1)

On my reading of this part of the hellcreek code, all this was doing was
assigning priorities (based on the traffic classes) to queues, and
taprio is able to "hide" this from the driver, so all the driver needs
to care about are queues.

>
> Also, Kurt, could you please explain what the
> HR_PRTCCFG_PCP_TC_MAP_SHIFT field in HR_PRTCCFG is doing?
> To me, it appears that it's configuring ingress QoS classification on
> the port (and the reason why this is strange to me is because you're
> applying this configuration through an egress qdisc), but I want to make
> sure I'm not misunderstanding.
>
> Thanks,
> -Vladimir
Vinicius Costa Gomes Aug. 25, 2020, 5:50 p.m. UTC | #15
Hi Kurt,

Kurt Kanzenbach <kurt@linutronix.de> writes:

> On Mon Aug 24 2020, Vinicius Costa Gomes wrote:
>> Hi,
>>
>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>>
> [snip]
>>> +	/* Setup timer for schedule switch: The IP core only allows to set a
>>> +	 * cycle start timer 8 seconds in the future. This is why we setup the
>>> +	 * hritmer to base_time - 5 seconds. Then, we have enough time to
>>> +	 * activate IP core's EST timer.
>>> +	 */
>>> +	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
>>> +	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
>>> +			       NSEC_PER_SEC, HRTIMER_MODE_ABS);
>>
>> If we are talking about seconds here, I don't think you need to use a
>> hrtimer, you could use a workqueue/delayed_work. Should make things a
>> bit simpler.
>
> I've used hrtimers for one reason: The hrtimer provides a way to fire at
> an absolute base time based on CLOCK_TAI. All the other facilities such
> as workqueues, timer list timers, etc do not.

Oh, yeah. Good point.


Cheers,
Vinicius Costa Gomes Aug. 25, 2020, 5:58 p.m. UTC | #16
Hi Kurt,

Kurt Kanzenbach <kurt@linutronix.de> writes:

> I think so. As Vladimir pointed out, the driver should setup an identity
> mapping which I already did by default.
>
> Can you point me your patch?

Just sent it for consideration:

http://patchwork.ozlabs.org/project/netdev/patch/20200825174404.2727633-1-vinicius.gomes@intel.com/


Cheers,
Kurt Kanzenbach Aug. 27, 2020, 10:12 a.m. UTC | #17
Hi Vinicius,

On Tue Aug 25 2020, Vinicius Costa Gomes wrote:
> Hi Kurt,
>
> Kurt Kanzenbach <kurt@linutronix.de> writes:
>
>> I think so. As Vladimir pointed out, the driver should setup an identity
>> mapping which I already did by default.
>>
>> Can you point me your patch?
>
> Just sent it for consideration:
>
> http://patchwork.ozlabs.org/project/netdev/patch/20200825174404.2727633-1-vinicius.gomes@intel.com/

Thank you. That looks good. So the driver just has to deal with queues
and I can setup an identity mapping in the hellcreek code.

I see the patch is already merged, otherwise I'd have acked it.

Thanks,
Kurt
Richard Cochran Aug. 27, 2020, 4:25 p.m. UTC | #18
On Tue, Aug 25, 2020 at 11:55:37AM +0200, Kurt Kanzenbach wrote:
> 
> I get your point. But how to do it? We would need a timer based on the
> PTP clock in the switch.

Can't you use an hrtimer based on CLOCK_MONOTONIC?

I would expect the driver to work based solely on the device's clock.

Thanks,
Richard
Kurt Kanzenbach Aug. 28, 2020, 12:31 p.m. UTC | #19
Hi Richard,

On Thu Aug 27 2020, Richard Cochran wrote:
> On Tue, Aug 25, 2020 at 11:55:37AM +0200, Kurt Kanzenbach wrote:
>> 
>> I get your point. But how to do it? We would need a timer based on the
>> PTP clock in the switch.
>
> Can't you use an hrtimer based on CLOCK_MONOTONIC?

When the switch and the Linux machine aren't synchronized, we would
calculate the difference between both systems and could arm the Linux
timer based on CLOCK_MONOTONIC. Given the fact that we eight seconds, it
would *probably* work when the ptp offset adjustments are in that range.

>
> I would expect the driver to work based solely on the device's clock.

Understood.

Thanks,
Kurt
Kurt Kanzenbach Sept. 1, 2020, 2:20 p.m. UTC | #20
On Tue Aug 25 2020, Vladimir Oltean wrote:
> On Tue, Aug 25, 2020 at 11:23:56AM +0200, Kurt Kanzenbach wrote:
>> On Mon Aug 24 2020, Vinicius Costa Gomes wrote:
>> > Hi,
>> >
>> > Kurt Kanzenbach <kurt@linutronix.de> writes:
>> >
>> [snip]
>> >> +	/* Setup timer for schedule switch: The IP core only allows to set a
>> >> +	 * cycle start timer 8 seconds in the future. This is why we setup the
>> >> +	 * hritmer to base_time - 5 seconds. Then, we have enough time to
>> >> +	 * activate IP core's EST timer.
>> >> +	 */
>> >> +	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
>> >> +	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
>> >> +			       NSEC_PER_SEC, HRTIMER_MODE_ABS);
>> >
>> > If we are talking about seconds here, I don't think you need to use a
>> > hrtimer, you could use a workqueue/delayed_work. Should make things a
>> > bit simpler.
>> 
>> I've used hrtimers for one reason: The hrtimer provides a way to fire at
>> an absolute base time based on CLOCK_TAI. All the other facilities such
>> as workqueues, timer list timers, etc do not.
>
> That still doesn't justify the complexity of irqsave spinlocks and such.
> You could just as well schedule a workqueue from that hrtimer and have
> process context...

After giving this a bit more thought, it can be implemented by using
workqueues only. That ptp time is "cached" anyway the we could just
periodically check for the base time arrival. That should solve the
irqsave and the being synchronized problem.

Thanks,
Kurt
Vladimir Oltean Sept. 1, 2020, 2:47 p.m. UTC | #21
On Tue, Sep 01, 2020 at 04:20:00PM +0200, Kurt Kanzenbach wrote:
>
> After giving this a bit more thought, it can be implemented by using
> workqueues only. That ptp time is "cached" anyway the we could just
> periodically check for the base time arrival. That should solve the
> irqsave and the being synchronized problem.
>
> Thanks,
> Kurt

Ok, this sounds simple enough. If the base-time is within 8 seconds of
the current PTP time, then apply the taprio configuration, otherwise
reschedule a delayed workqueue after N seconds (where N has what
value?).

If my math is correct, then N can't simply be the the delta between the
current PTP time and the (base-time minus 8 seconds) value - i.e. just
one schedule_delayed_work - because at large deltas, the PHC frequency
adjustment (+/- 6.25%) starts to matter. At maximum frequency, the PHC
can exceed the monotonic clock of the system by more than 8 seconds in
(8 * 100 / 6.25) = 128 seconds. So if the base-time is in the future by
more than 128 seconds and you plan for a single schedule_delayed_work,
there's a chance that you'll miss the window. And even if you try to
compensate using the current frequency adjustment, that's all that it is
- the current, instantaneous frequency adjustment, not the one from 128
seconds later.

How about N being half that delta? It's not ideal, since there would
need to be log2(delta) reschedules, but at least the error of the first
approximation won't propagate to the next, and the delta will keep
decreasing as time passes, therefore so will the error.

Thanks,
-Vladimir
diff mbox series

Patch

diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 745ca60342b4..e5b54f42c635 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -22,7 +22,9 @@ 
 #include <linux/spinlock.h>
 #include <linux/delay.h>
 #include <linux/ktime.h>
+#include <linux/time.h>
 #include <net/dsa.h>
+#include <net/pkt_sched.h>
 
 #include "hellcreek.h"
 #include "hellcreek_ptp.h"
@@ -153,6 +155,15 @@  static void hellcreek_select_vlan(struct hellcreek *hellcreek, int vid,
 	hellcreek_write(hellcreek, val, HR_VIDCFG);
 }
 
+static void hellcreek_select_tgd(struct hellcreek *hellcreek, int port)
+{
+	u16 val = 0;
+
+	val |= port << TR_TGDSEL_TDGSEL_SHIFT;
+
+	hellcreek_write(hellcreek, val, TR_TGDSEL);
+}
+
 static int hellcreek_wait_until_ready(struct hellcreek *hellcreek)
 {
 	u16 val;
@@ -958,6 +969,24 @@  static void __hellcreek_setup_tc_identity_mapping(struct hellcreek *hellcreek)
 	}
 }
 
+static void hellcreek_setup_tc_mapping(struct hellcreek *hellcreek,
+				       struct net_device *netdev)
+{
+	int i, j;
+
+	/* Setup mapping between traffic classes and port queues. */
+	for (i = 0; i < netdev_get_num_tc(netdev); ++i) {
+		for (j = 0; j < netdev->tc_to_txq[i].count; ++j) {
+			const int queue = j + netdev->tc_to_txq[i].offset;
+
+			hellcreek_select_prio(hellcreek, i);
+			hellcreek_write(hellcreek,
+					queue << HR_PRTCCFG_PCP_TC_MAP_SHIFT,
+					HR_PRTCCFG);
+		}
+	}
+}
+
 static void hellcreek_setup_tc_identity_mapping(struct hellcreek *hellcreek)
 {
 	unsigned long flags;
@@ -1081,6 +1110,267 @@  static void hellcreek_phylink_validate(struct dsa_switch *ds, int port,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);
 }
 
+static void hellcreek_setup_gcl(struct hellcreek *hellcreek, int port,
+				const struct hellcreek_schedule *schedule)
+{
+	size_t i;
+
+	for (i = 1; i <= schedule->num_entries; ++i) {
+		const struct hellcreek_gcl_entry *cur, *initial, *next;
+		u16 data;
+		u8 gates;
+
+		cur	= &schedule->entries[i - 1];
+		initial = &schedule->entries[0];
+		next	= &schedule->entries[i];
+
+		if (i == schedule->num_entries)
+			gates = initial->gate_states ^
+				cur->gate_states;
+		else
+			gates = next->gate_states ^
+				cur->gate_states;
+
+		data = gates;
+		if (cur->overrun_ignore)
+			data |= TR_GCLDAT_GCLOVRI;
+
+		if (i == schedule->num_entries)
+			data |= TR_GCLDAT_GCLWRLAST;
+
+		/* Gates states */
+		hellcreek_write(hellcreek, data, TR_GCLDAT);
+
+		/* Time intervall */
+		hellcreek_write(hellcreek,
+				cur->interval & 0x0000ffff,
+				TR_GCLTIL);
+		hellcreek_write(hellcreek,
+				(cur->interval & 0xffff0000) >> 16,
+				TR_GCLTIH);
+
+		/* Commit entry */
+		data = ((i - 1) << TR_GCLCMD_GCLWRADR_SHIFT) |
+			(initial->gate_states <<
+			 TR_GCLCMD_INIT_GATE_STATES_SHIFT);
+		hellcreek_write(hellcreek, data, TR_GCLCMD);
+	}
+}
+
+static void hellcreek_set_cycle_time(struct hellcreek *hellcreek,
+				     const struct hellcreek_schedule *schedule)
+{
+	u32 cycle_time = schedule->cycle_time;
+
+	hellcreek_write(hellcreek, cycle_time & 0x0000ffff, TR_CTWRL);
+	hellcreek_write(hellcreek, (cycle_time & 0xffff0000) >> 16, TR_CTWRH);
+}
+
+static void hellcreek_start_schedule(struct hellcreek *hellcreek,
+				     ktime_t start_time)
+{
+	struct timespec64 ts = ktime_to_timespec64(start_time);
+
+	/* Start can be only 8 seconds in the future */
+	ts.tv_sec %= 8;
+
+	/* Start schedule at this point of time */
+	hellcreek_write(hellcreek, ts.tv_nsec & 0x0000ffff, TR_ESTWRL);
+	hellcreek_write(hellcreek, (ts.tv_nsec & 0xffff0000) >> 16, TR_ESTWRH);
+
+	/* Arm timer, set seconds and switch schedule */
+	hellcreek_write(hellcreek, TR_ESTCMD_ESTARM | TR_ESTCMD_ESTSWCFG |
+		     ((ts.tv_sec & TR_ESTCMD_ESTSEC_MASK) <<
+		      TR_ESTCMD_ESTSEC_SHIFT), TR_ESTCMD);
+}
+
+static struct hellcreek_schedule *hellcreek_taprio_to_schedule(
+	const struct tc_taprio_qopt_offload *taprio)
+{
+	struct hellcreek_schedule *schedule;
+	size_t i;
+
+	/* Allocate some memory first */
+	schedule = kzalloc(sizeof(*schedule), GFP_KERNEL);
+	if (!schedule)
+		return ERR_PTR(-ENOMEM);
+	schedule->entries = kcalloc(taprio->num_entries,
+				    sizeof(*schedule->entries),
+				    GFP_KERNEL);
+	if (!schedule->entries) {
+		kfree(schedule);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/* Construct hellcreek schedule */
+	schedule->num_entries = taprio->num_entries;
+	schedule->base_time   = taprio->base_time;
+
+	for (i = 0; i < taprio->num_entries; ++i) {
+		const struct tc_taprio_sched_entry *t = &taprio->entries[i];
+		struct hellcreek_gcl_entry *k = &schedule->entries[i];
+
+		k->interval	  = t->interval;
+		k->gate_states	  = t->gate_mask;
+		k->overrun_ignore = 0;
+
+		/* Update complete cycle time */
+		schedule->cycle_time += t->interval;
+	}
+
+	return schedule;
+}
+
+static enum hrtimer_restart hellcreek_set_schedule(struct hrtimer *timer)
+{
+	struct hellcreek_port *hellcreek_port =
+		hrtimer_to_hellcreek_port(timer);
+	struct hellcreek *hellcreek = hellcreek_port->hellcreek;
+	struct hellcreek_schedule *schedule;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hellcreek->reg_lock, flags);
+
+	/* First select port */
+	hellcreek_select_tgd(hellcreek, hellcreek_port->port);
+
+	/* Set admin base time and switch schedule */
+	hellcreek_start_schedule(hellcreek,
+				 hellcreek_port->current_schedule->base_time);
+
+	schedule = hellcreek_port->current_schedule;
+	hellcreek_port->current_schedule = NULL;
+
+	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
+
+	dev_dbg(hellcreek->dev, "ARMed EST timer for port %d\n",
+		hellcreek_port->port);
+
+	/* Free resources */
+	kfree(schedule->entries);
+	kfree(schedule);
+
+	return HRTIMER_NORESTART;
+}
+
+static int hellcreek_port_set_schedule(struct dsa_switch *ds, int port,
+				       const struct tc_taprio_qopt_offload *taprio)
+{
+	struct net_device *netdev = dsa_to_port(ds, port)->slave;
+	struct hellcreek *hellcreek = ds->priv;
+	struct hellcreek_port *hellcreek_port;
+	struct hellcreek_schedule *schedule;
+	unsigned long flags;
+	ktime_t start;
+	u16 ctrl;
+
+	hellcreek_port = &hellcreek->ports[port];
+
+	/* Convert taprio data to hellcreek schedule */
+	schedule = hellcreek_taprio_to_schedule(taprio);
+	if (IS_ERR(schedule))
+		return PTR_ERR(schedule);
+
+	dev_dbg(hellcreek->dev, "Configure traffic schedule on port %d\n",
+		port);
+
+	/* Cancel an in flight timer */
+	hrtimer_cancel(&hellcreek_port->cycle_start_timer);
+
+	spin_lock_irqsave(&hellcreek->reg_lock, flags);
+
+	if (hellcreek_port->current_schedule) {
+		kfree(hellcreek_port->current_schedule->entries);
+		kfree(hellcreek_port->current_schedule);
+	}
+
+	hellcreek_port->current_schedule = schedule;
+
+	/* First select port */
+	hellcreek_select_tgd(hellcreek, port);
+
+	/* Setup traffic class <-> queue mapping */
+	hellcreek_setup_tc_mapping(hellcreek, netdev);
+
+	/* Enable gating and set the admin state to forward everything in the
+	 * mean time
+	 */
+	ctrl = (0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT) | TR_TGDCTRL_GATE_EN;
+	hellcreek_write(hellcreek, ctrl, TR_TGDCTRL);
+
+	/* Cancel pending schedule */
+	hellcreek_write(hellcreek, 0x00, TR_ESTCMD);
+
+	/* Setup a new schedule */
+	hellcreek_setup_gcl(hellcreek, port, schedule);
+
+	/* Configure cycle time */
+	hellcreek_set_cycle_time(hellcreek, schedule);
+
+	/* Setup timer for schedule switch: The IP core only allows to set a
+	 * cycle start timer 8 seconds in the future. This is why we setup the
+	 * hritmer to base_time - 5 seconds. Then, we have enough time to
+	 * activate IP core's EST timer.
+	 */
+	start = ktime_sub_ns(schedule->base_time, (u64)5 * NSEC_PER_SEC);
+	hrtimer_start_range_ns(&hellcreek_port->cycle_start_timer, start,
+			       NSEC_PER_SEC, HRTIMER_MODE_ABS);
+
+	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
+
+	return 0;
+}
+
+static int hellcreek_port_del_schedule(struct dsa_switch *ds, int port)
+{
+	struct hellcreek *hellcreek = ds->priv;
+	struct hellcreek_port *hellcreek_port;
+	unsigned long flags;
+
+	hellcreek_port = &hellcreek->ports[port];
+
+	dev_dbg(hellcreek->dev, "Remove traffic schedule on port %d\n", port);
+
+	/* First cancel timer */
+	hrtimer_cancel(&hellcreek_port->cycle_start_timer);
+
+	spin_lock_irqsave(&hellcreek->reg_lock, flags);
+
+	if (hellcreek_port->current_schedule) {
+		kfree(hellcreek_port->current_schedule->entries);
+		kfree(hellcreek_port->current_schedule);
+		hellcreek_port->current_schedule = NULL;
+	}
+
+	/* Then select port */
+	hellcreek_select_tgd(hellcreek, port);
+
+	/* Revert tc mapping */
+	__hellcreek_setup_tc_identity_mapping(hellcreek);
+
+	/* Disable gating and return to regular switching flow */
+	hellcreek_write(hellcreek, 0xff << TR_TGDCTRL_ADMINGATESTATES_SHIFT,
+			TR_TGDCTRL);
+
+	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
+
+	return 0;
+}
+
+static int hellcreek_port_setup_tc(struct dsa_switch *ds, int port,
+				   enum tc_setup_type type, void *type_data)
+{
+	const struct tc_taprio_qopt_offload *taprio = type_data;
+
+	if (type != TC_SETUP_QDISC_TAPRIO)
+		return -EOPNOTSUPP;
+
+	if (taprio->enable)
+		return hellcreek_port_set_schedule(ds, port, taprio);
+
+	return hellcreek_port_del_schedule(ds, port);
+}
+
 static const struct dsa_switch_ops hellcreek_ds_ops = {
 	.get_tag_protocol    = hellcreek_get_tag_protocol,
 	.setup		     = hellcreek_setup,
@@ -1104,6 +1394,7 @@  static const struct dsa_switch_ops hellcreek_ds_ops = {
 	.port_hwtstamp_get   = hellcreek_port_hwtstamp_get,
 	.port_txtstamp	     = hellcreek_port_txtstamp,
 	.port_rxtstamp	     = hellcreek_port_rxtstamp,
+	.port_setup_tc	     = hellcreek_port_setup_tc,
 	.get_ts_info	     = hellcreek_get_ts_info,
 };
 
@@ -1135,6 +1426,9 @@  static int hellcreek_probe(struct platform_device *pdev)
 		if (!port->counter_values)
 			return -ENOMEM;
 
+		hrtimer_init(&port->cycle_start_timer, CLOCK_TAI,
+			     HRTIMER_MODE_ABS);
+		port->cycle_start_timer.function = hellcreek_set_schedule;
 		port->hellcreek = hellcreek;
 		port->port	= i;
 	}
diff --git a/drivers/net/dsa/hirschmann/hellcreek.h b/drivers/net/dsa/hirschmann/hellcreek.h
index 1d3de72a48a5..d3d1a1144857 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.h
+++ b/drivers/net/dsa/hirschmann/hellcreek.h
@@ -16,6 +16,7 @@ 
 #include <linux/ptp_clock_kernel.h>
 #include <linux/timecounter.h>
 #include <linux/spinlock.h>
+#include <linux/hrtimer.h>
 #include <net/dsa.h>
 
 /* Ports:
@@ -210,6 +211,20 @@  struct hellcreek_counter {
 	const char *name;
 };
 
+struct hellcreek_gcl_entry {
+	u32 interval;
+	u8 gate_states;
+	bool overrun_ignore;
+};
+
+struct hellcreek_schedule {
+	struct hellcreek_gcl_entry *entries;
+	size_t num_entries;
+	ktime_t base_time;
+	u32 cycle_time;
+	int port;
+};
+
 struct hellcreek;
 
 /* State flags for hellcreek_port_hwtstamp::state */
@@ -236,6 +251,8 @@  struct hellcreek_port_hwtstamp {
 
 struct hellcreek_port {
 	struct hellcreek *hellcreek;
+	struct hellcreek_schedule *current_schedule;
+	struct hrtimer cycle_start_timer;
 	int port;
 	u16 ptcfg;		/* ptcfg shadow */
 	u64 *counter_values;
@@ -273,4 +290,8 @@  struct hellcreek {
 	size_t fdb_entries;
 };
 
+#define hrtimer_to_hellcreek_port(timer)		\
+	container_of(timer, struct hellcreek_port,	\
+		     cycle_start_timer)
+
 #endif /* _HELLCREEK_H_ */