Re: [PATCH v2] net: ethernet: ti: am65-cpsw: add mqprio qdisc offload in channel mode

[Vladimir Oltean]

Hi Roger,

On Mon, Sep 18, 2023 at 10:53:58AM +0300, Roger Quadros wrote:
> -int am65_cpsw_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> -			       void *type_data)
> -{
> -	switch (type) {
> -	case TC_QUERY_CAPS:
> -		return am65_cpsw_tc_query_caps(ndev, type_data);
> -	case TC_SETUP_QDISC_TAPRIO:
> -		return am65_cpsw_setup_taprio(ndev, type_data);
> -	case TC_SETUP_BLOCK:
> -		return am65_cpsw_qos_setup_tc_block(ndev, type_data);
> -	default:
> -		return -EOPNOTSUPP;
> -	}
> -}
> -
> -void am65_cpsw_qos_link_up(struct net_device *ndev, int link_speed)
> -{
> -	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> -
> -	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
> -		return;
> -
> -	am65_cpsw_est_link_up(ndev, link_speed);
> -	port->qos.link_down_time = 0;
> -}
> -
> -void am65_cpsw_qos_link_down(struct net_device *ndev)
> -{
> -	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> -
> -	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
> -		return;
> -
> -	if (!port->qos.link_down_time)
> -		port->qos.link_down_time = ktime_get();
> -
> -	port->qos.link_speed = SPEED_UNKNOWN;
> -}
> -

Could you split the code movement to a separate change?

> +	if (port->qos.link_speed != SPEED_UNKNOWN) {
> +		if (min_rate_total > port->qos.link_speed) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack, "TX rate min %llu exceeds link speed %d\n",
> +					       min_rate_total, port->qos.link_speed);
> +			return -EINVAL;
> +		}
> +
> +		if (max_rate_total > port->qos.link_speed) {
> +			NL_SET_ERR_MSG_FMT_MOD(extack, "TX rate max %llu exceeds link speed %d\n",
> +					       max_rate_total, port->qos.link_speed);
> +			return -EINVAL;
> +		}
> +	}

Link speeds can be renegotiated, and the mqprio offload can be installed
while the link is down. So this restriction, while honorable, has limited
usefulness.

> +
> +	p_mqprio->shaper_en = 1;

s/1/true/

> +	p_mqprio->max_rate_total = max_t(u64, min_rate_total, max_rate_total);
> +
> +	return 0;
> +}
> +
> +static void am65_cpsw_reset_tc_mqprio(struct net_device *ndev)
> +{
> +	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> +	struct am65_cpsw_mqprio *p_mqprio = &port->qos.mqprio;
> +	struct am65_cpsw_common *common = port->common;
> +
> +	p_mqprio->shaper_en = 0;

s/0/false/

> +	p_mqprio->max_rate_total = 0;
> +
> +	am65_cpsw_tx_pn_shaper_reset(port);
> +	netdev_reset_tc(ndev);
> +	netif_set_real_num_tx_queues(ndev, common->tx_ch_num);
> +
> +	/* Reset all Queue priorities to 0 */
> +	writel(0,
> +	       port->port_base + AM65_CPSW_PN_REG_TX_PRI_MAP);

What exactly needs pm_runtime_get_sync()? This writel() doesn't?

> +}
> +
> +static int am65_cpsw_setup_mqprio(struct net_device *ndev, void *type_data)
> +{
> +	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> +	struct am65_cpsw_mqprio *p_mqprio = &port->qos.mqprio;
> +	struct tc_mqprio_qopt_offload *mqprio = type_data;
> +	struct am65_cpsw_common *common = port->common;
> +	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
> +	int tc, offset, count, ret, prio;
> +	u8 num_tc = qopt->num_tc;
> +	u32 tx_prio_map = 0;
> +	int i;
> +
> +	memcpy(&p_mqprio->mqprio_hw, mqprio, sizeof(*mqprio));
> +
> +	if (!num_tc) {
> +		am65_cpsw_reset_tc_mqprio(ndev);
> +		return 0;
> +	}
> +
> +	ret = pm_runtime_get_sync(common->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(common->dev);
> +		return ret;
> +	}