Re: [RFC PATCH 1/3] drivers: interconnect: Add a driver for i.MX SoC

[Leonard Crestez <leonard.crestez@nxp.com>]

On 3/13/2019 9:36 PM, Alexandre Bailon wrote:
> 
> This adds support of i.MX SoC to interconnect framework.
> This is based on busfreq, from NXP's tree.
> This is is generic enough to be used to add support of interconnect
> framework to any i.MX SoC, and, even, this could used for some other
> SoC.

> Thanks to interconnect framework, devices' driver request for
> bandwidth which is use by busfreq to determine a performance level,
> and then scale the frequency.

This part is difficult for me to understand:

> +static int busfreq_opp_bw_gt(struct busfreq_opp_node *opp_node,
> +			      u32 avg_bw, u32 peak_bw)
> +{
> +	if (!opp_node)
> +		return 0;
> +	if (opp_node->min_avg_bw == BUSFREQ_UNDEFINED_BW) {
> +		if (avg_bw)
> +			return 2;
> +		else
> +			return 1;
> +	}
> +	if (opp_node->min_peak_bw == BUSFREQ_UNDEFINED_BW) {
> +		if (peak_bw)
> +			return 2;
> +		else
> +			return 1;
> +	}
> +	if (avg_bw >= opp_node->min_avg_bw)
> +		return 1;
> +	if (peak_bw >= opp_node->min_peak_bw)
> +		return 1;
> +	return 0;
> +}
> +
> +static struct busfreq_opp *busfreq_cmp_bw_opp(struct busfreq_icc_desc *desc,
> +					      struct busfreq_opp *opp1,
> +					      struct busfreq_opp *opp2)
> +{
> +	int i;
> +	int opp1_valid;
> +	int opp2_valid;
> +	int opp1_count = 0;
> +	int opp2_count = 0;
> +
> +	if (!opp1 && !opp2)
> +		return desc->current_opp;
> +
> +	if (!opp1)
> +		return opp2;
> +
> +	if (!opp2)
> +		return opp1;
> +
> +	if (opp1 == opp2)
> +		return opp1;
> +
> +	for (i = 0; i < opp1->nodes_count; i++) {
> +		struct busfreq_opp_node *opp_node1, *opp_node2;
> +		struct icc_node *icc_node;
> +		u32 avg_bw;
> +		u32 peak_bw;
> +
> +		opp_node1 = &opp1->nodes[i];
> +		opp_node2 = &opp2->nodes[i];
> +		icc_node = opp_node1->icc_node;
> +		avg_bw = icc_node->avg_bw;
> +		peak_bw = icc_node->peak_bw;
> +
> +		opp1_valid = busfreq_opp_bw_gt(opp_node1, avg_bw, peak_bw);
> +		opp2_valid = busfreq_opp_bw_gt(opp_node2, avg_bw, peak_bw);
> +
> +		if (opp1_valid == opp2_valid && opp1_valid == 1) {
> +			if (opp_node1->min_avg_bw > opp_node2->min_avg_bw &&
> +				opp_node1->min_avg_bw != BUSFREQ_UNDEFINED_BW)
> +				opp1_valid++;
> +			if (opp_node1->min_avg_bw < opp_node2->min_avg_bw &&
> +				opp_node2->min_avg_bw != BUSFREQ_UNDEFINED_BW)
> +				opp2_valid++;
> +		}
> +
> +		opp1_count += opp1_valid;
> +		opp2_count += opp2_valid;
> +	}
> +
> +	if (opp1_count > opp2_count)
> +		return opp1;
> +	if (opp1_count < opp2_count)
> +		return opp2;
> +	return opp1->perf_level >= opp2->perf_level ? opp2 : opp1;
> +}
> +
> +static int busfreq_set_best_opp(struct busfreq_icc_desc *desc)
> +{
> +	struct busfreq_opp *opp, *best_opp = desc->current_opp;
> +
> +	list_for_each_entry(opp, &desc->opps, node)
> +		best_opp = busfreq_cmp_bw_opp(desc, opp, best_opp);
> +	return busfreq_set_opp(desc, best_opp);
> +}

The goal seems to be to find the smaller platform-level "busfreq_opp" 
which can meet all aggregated requests.

But why implement this by comparing opps against each other? It would be 
easier to just iterate OPPs from low to high and stop on the first one 
which meets all requirements.

The sdm845 provider seems to take a different approach: there are BCMs 
("Bus Clock Managers") which are attached to some of the icc nodes and 
those are individually scaled in order to meet BW requirements. On imx8m 
we can scale buses via clk so we could just attach clks to some of the 
nodes (NOC, DRAM, few PL301).

--
Regards,
Leonard