Re: [PATCH v3 2/6] iommu/arm-smmu: Add interconnect bandwidth voting support

[Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>]

On 7/6/2026 11:10 PM, Dmitry Baryshkov wrote:
> On Mon, Jul 06, 2026 at 10:26:35PM +0530, Bibek Kumar Patro wrote:
>> On some SoCs the SMMU registers require an active interconnect
>> bandwidth vote to be accessible. While other clients typically
>> satisfy this requirement implicitly, certain corner cases (e.g.
>> during sleep/wakeup transitions) can leave the SMMU without a
>> vote, causing intermittent register access failures.
>>
>> Add support for an optional interconnect path to the arm-smmu
>> driver and vote for bandwidth while the SMMU is active. The path
>> is acquired from DT if present and ignored otherwise.
>>
>> The bandwidth vote is enabled before accessing SMMU registers
>> during probe and runtime resume, and released during runtime
>> suspend and on error paths.
>>
>> Generally, from an architectural perspective, GEM_NOC and DDR are
>> expected to have an active vote whenever the adreno_smmu block is
>> powered on. In most common use cases, this requirement is implicitly
>> satisfied because other GPU-related clients (for example, the GMU
>> device) already hold a GEM_NOC vote when adreno_smmu is enabled.
>>
>> However, there are certain corner cases, such as during sleep/wakeup
>> transitions, where the GEM_NOC vote can be removed before adreno_smmu
>> is powered down. If adreno_smmu is then accessed while the interconnect
>> vote is missing, it can lead to the observed failures. Because of the
>> precise ordering involved, this scenario is difficult to reproduce
>> consistently.
>> (also GDSC is involved in adreno usecases can have an independent vote)
>>
>> Signed-off-by: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 55 +++++++++++++++++++++++++++++-
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |  3 ++
>>   drivers/iommu/arm/arm-smmu/arm-smmu.c      | 27 +++++++++++++--
>>   drivers/iommu/arm/arm-smmu/arm-smmu.h      |  2 ++
>>   4 files changed, 84 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index e2c914fccd6f..5133d3ab023a 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -6,6 +6,7 @@
>>   #include <linux/acpi.h>
>>   #include <linux/adreno-smmu-priv.h>
>>   #include <linux/delay.h>
>> +#include <linux/interconnect.h>
>>   #include <linux/of_device.h>
>>   #include <linux/firmware/qcom/qcom_scm.h>
>>   #include <linux/platform_device.h>
>> @@ -607,6 +608,45 @@ static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
>>   	return ret;
>>   }
>>   
>> +static int qcom_adreno_smmu_icc_init(struct arm_smmu_device *smmu)
>> +{
>> +	struct qcom_smmu *qsmmu = container_of(smmu, struct qcom_smmu, smmu);
>> +	int err;
>> +
>> +	qsmmu->icc_path = devm_of_icc_get(smmu->dev, NULL);
>> +	if (!IS_ERR(qsmmu->icc_path))
>> +		return 0;
>> +
>> +	err = PTR_ERR(qsmmu->icc_path);
>> +
>> +	if (err == -ENODEV) {
>> +		qsmmu->icc_path = NULL;
>> +		return 0;
>> +	}
>> +	return dev_err_probe(smmu->dev, err,
>> +			     "failed to get interconnect path\n");
>> +}
>> +
>> +static int qcom_adreno_smmu_runtime_resume(struct arm_smmu_device *smmu)
>> +{
>> +	struct qcom_smmu *qsmmu = container_of(smmu, struct qcom_smmu, smmu);
>> +	int ret;
>> +
>> +	ret = icc_set_bw(qsmmu->icc_path, 0, 1);
>> +	WARN_ON_ONCE(ret);
>> +	return ret;
>> +}
>> +
>> +static int qcom_adreno_smmu_runtime_suspend(struct arm_smmu_device *smmu)
>> +{
>> +	struct qcom_smmu *qsmmu = container_of(smmu, struct qcom_smmu, smmu);
>> +	int ret;
>> +
>> +	ret = icc_set_bw(qsmmu->icc_path, 0, 0);
>> +	WARN_ON_ONCE(ret);
>> +	return ret;
>> +}
>> +
>>   static const struct arm_smmu_impl qcom_smmu_v2_impl = {
>>   	.init_context = qcom_smmu_init_context,
>>   	.cfg_probe = qcom_smmu_cfg_probe,
>> @@ -648,6 +688,8 @@ static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>>   	.alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
>>   	.write_sctlr = qcom_adreno_smmu_write_sctlr,
>>   	.tlb_sync = qcom_smmu_tlb_sync,
>> +	.runtime_resume = qcom_adreno_smmu_runtime_resume,
>> +	.runtime_suspend = qcom_adreno_smmu_runtime_suspend,
>>   	.context_fault_needs_threaded_irq = true,
>>   };
>>   
>> @@ -658,6 +700,8 @@ static const struct arm_smmu_impl qcom_adreno_smmu_500_impl = {
>>   	.alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
>>   	.write_sctlr = qcom_adreno_smmu_write_sctlr,
>>   	.tlb_sync = qcom_smmu_tlb_sync,
>> +	.runtime_resume = qcom_adreno_smmu_runtime_resume,
>> +	.runtime_suspend = qcom_adreno_smmu_runtime_suspend,
>>   	.context_fault_needs_threaded_irq = true,
>>   };
>>   
>> @@ -667,11 +711,14 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
>>   	const struct device_node *np = smmu->dev->of_node;
>>   	const struct arm_smmu_impl *impl;
>>   	struct qcom_smmu *qsmmu;
>> +	bool is_adreno_smmu;
>> +	int ret;
>>   
>>   	if (!data)
>>   		return ERR_PTR(-EINVAL);
>>   
>> -	if (np && of_device_is_compatible(np, "qcom,adreno-smmu"))
>> +	is_adreno_smmu = np && of_device_is_compatible(np, "qcom,adreno-smmu");
>> +	if (is_adreno_smmu)
>>   		impl = data->adreno_impl;
>>   	else
>>   		impl = data->impl;
>> @@ -691,6 +738,12 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
>>   	qsmmu->smmu.impl = impl;
>>   	qsmmu->data = data;
>>   
>> +	if (is_adreno_smmu) {
>> +		ret = qcom_adreno_smmu_icc_init(&qsmmu->smmu);
>> +		if (ret)
>> +			return ERR_PTR(ret);
>> +	}
> 
> Move this to a runtime hook to be declared in *data.
> 

Ack, will implement this in next revision.
<Missed to make this a runtime hook as it was not being used
anywhere else outside the current scope of this file.>

>> +
>>   	return &qsmmu->smmu;
>>   }
>>   
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> index 8addd453f5f1..6835b40339ce 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> @@ -6,12 +6,15 @@
>>   #ifndef _ARM_SMMU_QCOM_H
>>   #define _ARM_SMMU_QCOM_H
>>   
>> +#include <linux/interconnect.h>
> 
> Not necessary here. Just forward-declare the struct.
> 

Ack, will take care of this in next revision.

Thanks & regards,
Bibek

>> +
>>   struct qcom_smmu {
>>   	struct arm_smmu_device smmu;
>>   	const struct qcom_smmu_match_data *data;
>>   	bool bypass_quirk;
>>   	u8 bypass_cbndx;
>>   	u32 stall_enabled;
>> +	struct icc_path *icc_path;
>>   };
>>   
>>   enum qcom_smmu_impl_reg_offset {
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> index 0bd21d206eb3..a27804e15738 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> @@ -2189,6 +2189,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>   	if (err)
>>   		return err;
>>   
>> +	if (smmu->impl && smmu->impl->runtime_resume) {
>> +		err = smmu->impl->runtime_resume(smmu);
>> +		if (err) {
>> +			clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
>> +			return err;
>> +		}
>> +	}
>> +
>>   	err = arm_smmu_device_cfg_probe(smmu);
>>   	if (err)
>>   		return err;
>> @@ -2273,8 +2281,11 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
>>   
>>   	if (pm_runtime_enabled(smmu->dev))
>>   		pm_runtime_force_suspend(smmu->dev);
>> -	else
>> +	else {
>>   		clk_bulk_disable(smmu->num_clks, smmu->clks);
>> +		if (smmu->impl && smmu->impl->runtime_suspend)
>> +			smmu->impl->runtime_suspend(smmu);
>> +	}
>>   
>>   	clk_bulk_unprepare(smmu->num_clks, smmu->clks);
>>   }
>> @@ -2294,9 +2305,18 @@ static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
>>   	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>>   	int ret;
>>   
>> +	if (smmu->impl && smmu->impl->runtime_resume) {
>> +		ret = smmu->impl->runtime_resume(smmu);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
>> -	if (ret)
>> +	if (ret) {
>> +		if (smmu->impl && smmu->impl->runtime_suspend)
>> +			smmu->impl->runtime_suspend(smmu);
>>   		return ret;
>> +	}
>>   
>>   	arm_smmu_device_reset(smmu);
>>   
>> @@ -2309,6 +2329,9 @@ static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
>>   
>>   	clk_bulk_disable(smmu->num_clks, smmu->clks);
>>   
>> +	if (smmu->impl && smmu->impl->runtime_suspend)
>> +		return smmu->impl->runtime_suspend(smmu);
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> index 26d2e33cd328..ed08f86cf99d 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> @@ -455,6 +455,8 @@ struct arm_smmu_impl {
>>   	void (*write_s2cr)(struct arm_smmu_device *smmu, int idx);
>>   	void (*write_sctlr)(struct arm_smmu_device *smmu, int idx, u32 reg);
>>   	void (*probe_finalize)(struct arm_smmu_device *smmu, struct device *dev);
>> +	int (*runtime_resume)(struct arm_smmu_device *smmu);
>> +	int (*runtime_suspend)(struct arm_smmu_device *smmu);
>>   };
>>   
>>   #define INVALID_SMENDX			-1
>>
>> -- 
>> 2.34.1
>>
>