Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Leonard Crestez <leonard.crestez@nxp.com>
To:     Alexandre Bailon <abailon@baylibre.com>,
        Anson Huang <anson.huang@nxp.com>
CC:     "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        "georgi.djakov@linaro.org" <georgi.djakov@linaro.org>,
        "mturquette@baylibre.com" <mturquette@baylibre.com>,
        "ptitiano@baylibre.com" <ptitiano@baylibre.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        Aisheng Dong <aisheng.dong@nxp.com>,
        "khilman@baylibre.com" <khilman@baylibre.com>,
        "ccaione@baylibre.com" <ccaione@baylibre.com>,
        Jacky Bai <ping.bai@nxp.com>, Abel Vesa <abel.vesa@nxp.com>
Subject: Re: [RFC PATCH 1/3] drivers: interconnect: Add a driver for i.MX SoC
Thread-Topic: [RFC PATCH 1/3] drivers: interconnect: Add a driver for i.MX SoC
Thread-Index: AQHU2dPAGEvIM5gAt0ufevh4gpMf+A==
Date:   Mon, 10 Jun 2019 22:10:07 +0000
Message-ID: <VI1PR04MB5055980A24CAC07010E6192AEE130@VI1PR04MB5055.eurprd04.prod.outlook.com>
References: <20190313193408.23740-1-abailon@baylibre.com>
 <20190313193408.23740-2-abailon@baylibre.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 0950b67e-8523-4625-5409-08d6edf0659d
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Jun 2019 22:10:07.0868
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: leonard.crestez@nxp.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5903
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/13/2019 9:36 PM, Alexandre Bailon wrote:=0A=
> =0A=
> This adds support of i.MX SoC to interconnect framework.=0A=
> This is based on busfreq, from NXP's tree.=0A=
> This is is generic enough to be used to add support of interconnect=0A=
> framework to any i.MX SoC, and, even, this could used for some other=0A=
> SoC.=0A=
=0A=
> Thanks to interconnect framework, devices' driver request for=0A=
> bandwidth which is use by busfreq to determine a performance level,=0A=
> and then scale the frequency.=0A=
=0A=
This part is difficult for me to understand:=0A=
=0A=
> +static int busfreq_opp_bw_gt(struct busfreq_opp_node *opp_node,=0A=
> +			      u32 avg_bw, u32 peak_bw)=0A=
> +{=0A=
> +	if (!opp_node)=0A=
> +		return 0;=0A=
> +	if (opp_node->min_avg_bw =3D=3D BUSFREQ_UNDEFINED_BW) {=0A=
> +		if (avg_bw)=0A=
> +			return 2;=0A=
> +		else=0A=
> +			return 1;=0A=
> +	}=0A=
> +	if (opp_node->min_peak_bw =3D=3D BUSFREQ_UNDEFINED_BW) {=0A=
> +		if (peak_bw)=0A=
> +			return 2;=0A=
> +		else=0A=
> +			return 1;=0A=
> +	}=0A=
> +	if (avg_bw >=3D opp_node->min_avg_bw)=0A=
> +		return 1;=0A=
> +	if (peak_bw >=3D opp_node->min_peak_bw)=0A=
> +		return 1;=0A=
> +	return 0;=0A=
> +}=0A=
> +=0A=
> +static struct busfreq_opp *busfreq_cmp_bw_opp(struct busfreq_icc_desc *d=
esc,=0A=
> +					      struct busfreq_opp *opp1,=0A=
> +					      struct busfreq_opp *opp2)=0A=
> +{=0A=
> +	int i;=0A=
> +	int opp1_valid;=0A=
> +	int opp2_valid;=0A=
> +	int opp1_count =3D 0;=0A=
> +	int opp2_count =3D 0;=0A=
> +=0A=
> +	if (!opp1 && !opp2)=0A=
> +		return desc->current_opp;=0A=
> +=0A=
> +	if (!opp1)=0A=
> +		return opp2;=0A=
> +=0A=
> +	if (!opp2)=0A=
> +		return opp1;=0A=
> +=0A=
> +	if (opp1 =3D=3D opp2)=0A=
> +		return opp1;=0A=
> +=0A=
> +	for (i =3D 0; i < opp1->nodes_count; i++) {=0A=
> +		struct busfreq_opp_node *opp_node1, *opp_node2;=0A=
> +		struct icc_node *icc_node;=0A=
> +		u32 avg_bw;=0A=
> +		u32 peak_bw;=0A=
> +=0A=
> +		opp_node1 =3D &opp1->nodes[i];=0A=
> +		opp_node2 =3D &opp2->nodes[i];=0A=
> +		icc_node =3D opp_node1->icc_node;=0A=
> +		avg_bw =3D icc_node->avg_bw;=0A=
> +		peak_bw =3D icc_node->peak_bw;=0A=
> +=0A=
> +		opp1_valid =3D busfreq_opp_bw_gt(opp_node1, avg_bw, peak_bw);=0A=
> +		opp2_valid =3D busfreq_opp_bw_gt(opp_node2, avg_bw, peak_bw);=0A=
> +=0A=
> +		if (opp1_valid =3D=3D opp2_valid && opp1_valid =3D=3D 1) {=0A=
> +			if (opp_node1->min_avg_bw > opp_node2->min_avg_bw &&=0A=
> +				opp_node1->min_avg_bw !=3D BUSFREQ_UNDEFINED_BW)=0A=
> +				opp1_valid++;=0A=
> +			if (opp_node1->min_avg_bw < opp_node2->min_avg_bw &&=0A=
> +				opp_node2->min_avg_bw !=3D BUSFREQ_UNDEFINED_BW)=0A=
> +				opp2_valid++;=0A=
> +		}=0A=
> +=0A=
> +		opp1_count +=3D opp1_valid;=0A=
> +		opp2_count +=3D opp2_valid;=0A=
> +	}=0A=
> +=0A=
> +	if (opp1_count > opp2_count)=0A=
> +		return opp1;=0A=
> +	if (opp1_count < opp2_count)=0A=
> +		return opp2;=0A=
> +	return opp1->perf_level >=3D opp2->perf_level ? opp2 : opp1;=0A=
> +}=0A=
> +=0A=
> +static int busfreq_set_best_opp(struct busfreq_icc_desc *desc)=0A=
> +{=0A=
> +	struct busfreq_opp *opp, *best_opp =3D desc->current_opp;=0A=
> +=0A=
> +	list_for_each_entry(opp, &desc->opps, node)=0A=
> +		best_opp =3D busfreq_cmp_bw_opp(desc, opp, best_opp);=0A=
> +	return busfreq_set_opp(desc, best_opp);=0A=
> +}=0A=
=0A=
The goal seems to be to find the smaller platform-level "busfreq_opp" =0A=
which can meet all aggregated requests.=0A=
=0A=
But why implement this by comparing opps against each other? It would be =
=0A=
easier to just iterate OPPs from low to high and stop on the first one =0A=
which meets all requirements.=0A=
=0A=
The sdm845 provider seems to take a different approach: there are BCMs =0A=
("Bus Clock Managers") which are attached to some of the icc nodes and =0A=
those are individually scaled in order to meet BW requirements. On imx8m =
=0A=
we can scale buses via clk so we could just attach clks to some of the =0A=
nodes (NOC, DRAM, few PL301).=0A=
=0A=
--=0A=
Regards,=0A=
Leonard=0A=