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

On Tue Aug 25 2020, Vladimir Oltean wrote:

On Thu, Aug 20, 2020 at 10:11:15AM +0200, Kurt Kanzenbach wrote:

[snip]

quoted

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

quoted

+{
+	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.

quoted

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

quoted

+	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