Re: [PATCH 2/2] interconnect: qcom: Add SM7250 interconnect provider driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] interconnect: qcom: sm7250: missing enable_mask on bcm_acv
- [High] interconnect: qcom: sm7250: ipa-virt bcm_ip0 conflicts with clk-rpmh
--

commit d08b59b8f7913184c97b90b1dc52c20688a7059e
Author: Ishu Brar <patches@brar.si>

interconnect: qcom: Add SM7250 interconnect provider driver

This commit adds the Network-On-Chip interconnect provider driver for the
Qualcomm SM7250 SoC, defining the interconnect nodes and Bus Clock Managers
for its RPMh-based providers.

> diff --git a/drivers/interconnect/qcom/sm7250.c b/drivers/interconnect/qcom/sm7250.c
> new file mode 100644
> index 0000000000000..cef29b80508d8
> --- /dev/null
> +++ b/drivers/interconnect/qcom/sm7250.c

[ ... ]

> +static struct qcom_icc_bcm bcm_acv = {
> +	.name = "ACV",
> +	.keepalive = false,
> +	.num_nodes = 1,
> +	.nodes = { &ebi },
> +};

[Severity: High]
Should bcm_acv set .enable_mask = BIT(3) (or 0x8) here?

On Qualcomm RPMh platforms (such as SM6350, SM7150, SC7180, SC7280, SM8150,
and SM8250), the ACV Bus Clock Manager resource uses a one-hot bitmask in
vote_y to identify the APPS RSC rather than a scaled bandwidth value.

With enable_mask left zero-initialized, qcom_icc_bcm_voter_commit() in
drivers/interconnect/qcom/bcm-voter.c takes the bcm_aggregate() branch
instead of bcm_aggregate_mask() whenever bandwidth is updated on a path
reaching SLAVE_EBI_CH0 (ebi):

drivers/interconnect/qcom/bcm-voter.c:qcom_icc_bcm_voter_commit() {
	list_for_each_entry(bcm, &voter->commit_list, list) {
		if (bcm->enable_mask)
			bcm_aggregate_mask(bcm);
		else
			bcm_aggregate(bcm);
	}
}

This programs scaled bandwidth values into vote_x and vote_y for the ACV
resource instead of setting vote_y = BIT(3).

Similarly, when qcom_icc_rpmh_probe() calls qcom_icc_get_bw() in
drivers/interconnect/qcom/icc-rpmh.c during sm7250_mc_virt probe,
if (bcm->enable_mask) is false, causing the boot-time AMC bitmask on ebi to
be misinterpreted as a bandwidth multiplier.

[ ... ]

> +static struct qcom_icc_bcm * const ipa_virt_bcms[] = {
> +	&bcm_ip0,
> +};
> +
> +static struct qcom_icc_node * const ipa_virt_nodes[] = {
> +	[MASTER_IPA_CORE] = &ipa_core_master,
> +	[SLAVE_IPA_CORE] = &ipa_core_slave,
> +};
> +
> +static const struct qcom_icc_desc sm7250_ipa_virt = {
> +	.nodes = ipa_virt_nodes,
> +	.num_nodes = ARRAY_SIZE(ipa_virt_nodes),
> +	.bcms = ipa_virt_bcms,
> +	.num_bcms = ARRAY_SIZE(ipa_virt_bcms),
> +};

[ ... ]

> +static const struct of_device_id qnoc_of_match[] = {
> +	{ .compatible = "qcom,sm7250-aggre1-noc", .data = &sm7250_aggre1_noc },
> +	{ .compatible = "qcom,sm7250-aggre2-noc", .data = &sm7250_aggre2_noc },
> +	{ .compatible = "qcom,sm7250-compute-noc", .data = &sm7250_compute_noc },
> +	{ .compatible = "qcom,sm7250-config-noc", .data = &sm7250_config_noc },
> +	{ .compatible = "qcom,sm7250-dc-noc", .data = &sm7250_dc_noc },
> +	{ .compatible = "qcom,sm7250-gem-noc", .data = &sm7250_gem_noc },
> +	{ .compatible = "qcom,sm7250-ipa-virt", .data = &sm7250_ipa_virt },

[Severity: High]
Will registering sm7250_ipa_virt and bcm_ip0 ("IP0") conflict with clk-rpmh
and disable the IPA core clock at icc_sync_state()?

In upstream Linux, the "IP0" RPMh BCM resource is managed by the RPMh clock
driver (DEFINE_CLK_RPMH_BCM(ipa, "IP0") in drivers/clk/qcom/clk-rpmh.c).
The IPA driver (drivers/net/ipa/ipa_power.c) controls IP0 as its core clock
via RPMH_IPA_CLK and only requests the memory, imem, and config interconnect
paths, never MASTER_IPA_CORE to SLAVE_IPA_CORE.

Because RPMh does not aggregate votes for the same resource from two separate
kernel drivers within the APPS RSC, when icc_sync_state() runs after all
interconnect providers probe, the unused ipa_core_slave node aggregates to
zero bandwidth and qcom_icc_bcm_voter_commit() sends a zero vote for "IP0"
to RPMh. This overwrites clk-rpmh's vote and turns off the IPA core clock
behind the clock framework's back, leading to unclocked register access
SErrors when the IPA driver accesses hardware.

Should sm7250_ipa_virt, bcm_ip0, ipa_core_master, and ipa_core_slave be
dropped here, as was done for the other upstream drivers in
drivers/interconnect/qcom/?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-icc-sm7250-v1-0-1e47c1226857@brar.si?part=2