Re: [PATCH net-next v7 2/2] net: dsa: mv88e6xxx: add support for credit based shaper

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 09 Jun 2026 14:10:51 +0200 Cedric Jehasse via B4 Relay wrote:
> From: Cedric Jehasse <cedric.jehasse@luminex.be>
> 
> Some of the chips supported by this driver have credit based shaper
> support. Support is added for the 6341, 6352, 6390 and 6393 families.
> This is configured using the Qav registers in the AVB register block.
> There are small differences in the Qav registers between the chip
> families (eg. the unit used for the rate and number of bits in the
> registers). mv88e6xxx_qav_info is introduced to configure this per chip.
> 
> Eg. setting up 20mbps credit based shaper on a 1GBit link:
> tc qdisc add dev p8 parent root handle 100: mqprio \
>                 num_tc 8 \
>                 map 0 0 6 7 0 5 0 0 0 0 0 0 0 0 0 0 \
>                 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \
>                 hw 0
> 
> tc qdisc replace dev p8 parent 100:8 cbs locredit -1470 hicredit 30 \
>     sendslope -980000 idleslope 20000 offload 1

Are DSA ports multi-queue? I would have expected a DSA driver to
offload PRIO not MQPRIO.

I seem to recall other discussion on the ML on the topic.
It'd be great to get some review tags from folks familiar with 
the device.

> Note: only idleslope and hicredit can be programmed in the switch
> registers, other parameters won't affect settings.

> +static int mv88e6xxx_setup_tc_cbs(struct dsa_switch *ds, int port,
> +				  struct tc_cbs_qopt_offload *cbs)

please stick an extack into struct tc_cbs_qopt_offload and use it to
report the reason for rejection back to the user

> +{
> +	const struct mv88e6xxx_avb_ops *avb_ops;
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	const struct mv88e6xxx_qav_info *qav;
> +	const struct mv88e6xxx_ops *ops;
> +	int hilimit_reg;
> +	int rate_reg;
> +	u8 queue_bit;
> +	u32 rate = 0;
> +	u16 hilimit;
> +	int err;
> +
> +	ops = chip->info->ops;
> +	avb_ops = ops->avb_ops;
> +	qav = chip->info->qav;
> +
> +	if (!qav || !avb_ops || !avb_ops->port_qav_write ||
> +	    !ops->port_set_scheduling_mode)
> +		return -EOPNOTSUPP;
> +
> +	if (!dsa_is_user_port(ds, port))
> +		return -EOPNOTSUPP;
> +
> +	if (!(qav->queue_mask & BIT(cbs->queue)))
> +		return -EOPNOTSUPP;
> +
> +	queue_bit = BIT(cbs->queue);
> +	rate_reg = MV88E6XXX_PORT_QAV_CFG_RATE(cbs->queue);
> +	hilimit_reg = MV88E6XXX_PORT_QAV_CFG_HILIMIT(cbs->queue);
> +
> +	if (cbs->enable) {
> +		if (cbs->hicredit <= 0 ||
> +		    cbs->hicredit > qav->hilimit_mask)
> +			return -ERANGE;
> +
> +		rate = DIV_ROUND_UP(cbs->idleslope, qav->rate_unit);
> +		if (rate > qav->rate_mask)
> +			return -ERANGE;
> +		/* avoid using zero rate */
> +		rate = max_t(u16, rate, 1);
> +	}
> +
> +	mv88e6xxx_reg_lock(chip);
> +
> +	if (!cbs->enable) {
> +		err = mv88e6xxx_port_qav_write(chip, port, rate_reg, 0);
> +		if (err)
> +			goto unlock;
> +
> +		if (!(chip->ports[port].cbs_active_queues & ~queue_bit)) {
> +			err = mv88e6xxx_port_set_scheduling_mode(chip, port, 0);
> +			if (err)
> +				goto unlock;
> +		}
> +		chip->ports[port].cbs_active_queues &= ~queue_bit;
> +		goto unlock;
> +	}
> +
> +	hilimit = cbs->hicredit & qav->hilimit_mask;
> +	err = mv88e6xxx_port_qav_write(chip, port, hilimit_reg, hilimit);
> +	if (err)
> +		goto unlock;
> +
> +	err = mv88e6xxx_port_qav_write(chip, port, rate_reg, rate);
> +	if (err)
> +		goto unlock;
> +
> +	if (!chip->ports[port].cbs_active_queues) {
> +		u8 sched_mode = chip->info->num_tx_queues - 1;
> +
> +		err = mv88e6xxx_port_set_scheduling_mode(chip, port,
> +							 sched_mode);
> +		if (err) {
> +			mv88e6xxx_port_qav_write(chip, port, rate_reg, 0);
> +			goto unlock;
> +		}
> +	}
> +	chip->ports[port].cbs_active_queues |= queue_bit;
> +
> +unlock:
> +	mv88e6xxx_reg_unlock(chip);
> +
> +	return err;
> +}

> +int mv88e6390_port_set_scheduling_mode(struct mv88e6xxx_chip *chip, int port,
> +				       u8 mode)
> +{
> +	u16 reg;
> +	int err;
> +
> +	if (mode > MV88E6390_PORT_QUEUE_CTL_SCHEDULE_MASK)
> +		return -EINVAL;
> +
> +	reg = MV88E6390_PORT_QUEUE_CTL_UPDATE |
> +	      (MV88E6390_PORT_QUEUE_CTL_SCHEDULE <<
> +	       MV88E6390_PORT_QUEUE_CTL_PTR_SHIFT) |
> +	      (mode & MV88E6390_PORT_QUEUE_CTL_SCHEDULE_MASK);
> +
> +	err = mv88e6xxx_port_write(chip, port, MV88E6390_PORT_QUEUE_CTL,
> +				   reg);
> +	if (err)
> +		return err;
> +
> +	return mv88e6xxx_port_wait_bit(chip, port, MV88E6390_PORT_QUEUE_CTL,
> +				       __bf_shf(MV88E6390_PORT_QUEUE_CTL_UPDATE)
> +				       , 0);

odd placement of the comma
-- 
pw-bot: cr