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

[Roger Quadros <rogerq@kernel.org>]

Hi Vladimir,

On 19/09/2023 15:47, Vladimir Oltean wrote:
> 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?

OK.

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

For link down case it won't run those checks, but I get your point.
I'll drop these checks.

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

Good catch. In my tests, the network interface was up so controller
was already active. But we will need to do a pm_runtime_get_sync()
if all network interfaces of the controller are down.

So, I will need to move the pm_runtime_get_sync() call before
am65_cpsw_reset_tc_mqprio();

> 
>> +}
>> +
>> +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;
>> +	}

-- 
cheers,
-roger