diff mbox series

[v4,5/5] iommu/arm-smmu: re-enable context caching in smmu reset operation

Message ID 20231215101827.30549-6-quic_bibekkum@quicinc.com (mailing list archive)
State Superseded
Headers show
Series iommu/arm-smmu: introduction of ACTLR implementation for Qualcomm SoCs | expand

Commit Message

Bibek Kumar Patro Dec. 15, 2023, 10:18 a.m. UTC 
Default MMU-500 reset operation disables context caching in
prefetch buffer. It is however expected for context banks using
the ACTLR register to retain their prefetch value during reset
and runtime suspend.

Replace default MMU-500 reset operation with Qualcomm specific reset
operation which envelope the default reset operation and re-enables
context caching in prefetch buffer for Qualcomm SoCs.

Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 23 +++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

--
2.17.1

Comments

Konrad Dybcio Dec. 15, 2023, 11:54 p.m. UTC | #1 
On 15.12.2023 11:18, Bibek Kumar Patro wrote:
> Default MMU-500 reset operation disables context caching in
> prefetch buffer. It is however expected for context banks using
> the ACTLR register to retain their prefetch value during reset
> and runtime suspend.
> 
> Replace default MMU-500 reset operation with Qualcomm specific reset
> operation which envelope the default reset operation and re-enables
> context caching in prefetch buffer for Qualcomm SoCs.
> 
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
This probably deserves a fixes tag, but I can't find a good commit for
it, so I guess not having it is fine as well.

Also, since it seems to be independent from the rest of the patches, please
reorder it to become patch 1 in the next spin, so that it can perhaps be
easily picked up independently of the rest.

>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 23 +++++++++++++++++++---
>  1 file changed, 20 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 c8f5dd4186b7..70d2a5d43993 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -516,11 +516,28 @@ static int qcom_smmu_def_domain_type(struct device *dev)
>  	return match ? IOMMU_DOMAIN_IDENTITY : 0;
>  }
> 
> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
> +{
> +	int i;
> +	u32 reg;
That's rather nitty/codestyle-y, but:

- reverse Christmas tree would be nice (it's in a week! :D)
- "reg" to me sounds like "register address", "val" is used widely for
  register values

> +
> +	arm_mmu500_reset(smmu);
We should check the return value here, in case the function is modified
some day in a way that makes it return something else than 0

LGTM otherwise!

Konrad
Bibek Kumar Patro Dec. 18, 2023, 6:37 a.m. UTC | #2 
On 12/16/2023 5:24 AM, Konrad Dybcio wrote:
> On 15.12.2023 11:18, Bibek Kumar Patro wrote:
>> Default MMU-500 reset operation disables context caching in
>> prefetch buffer. It is however expected for context banks using
>> the ACTLR register to retain their prefetch value during reset
>> and runtime suspend.
>>
>> Replace default MMU-500 reset operation with Qualcomm specific reset
>> operation which envelope the default reset operation and re-enables
>> context caching in prefetch buffer for Qualcomm SoCs.
>>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
> This probably deserves a fixes tag, but I can't find a good commit for
> it, so I guess not having it is fine as well.
> 

I was also trying to find any suitable commit, but this default reset
implementation didn't have a separate commit, so did not add any fixes
tag here.

> Also, since it seems to be independent from the rest of the patches, please
> reorder it to become patch 1 in the next spin, so that it can perhaps be
> easily picked up independently of the rest.
> 

Sure, agree on the same. Will keep this as 1st patch, and start the rest
of the patches on top of it.

>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 23 +++++++++++++++++++---
>>   1 file changed, 20 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 c8f5dd4186b7..70d2a5d43993 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -516,11 +516,28 @@ static int qcom_smmu_def_domain_type(struct device *dev)
>>   	return match ? IOMMU_DOMAIN_IDENTITY : 0;
>>   }
>>
>> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
>> +{
>> +	int i;
>> +	u32 reg;
> That's rather nitty/codestyle-y, but:
> 
> - reverse Christmas tree would be nice (it's in a week! :D)
> - "reg" to me sounds like "register address", "val" is used widely for
>    register values
> 

Thanks for pointing to this, will take care of this val name and
sorting in next version.(Will try to post the reserse xmas tree sorted
patch asap for the xmas celebration! :) )

>> +
>> +	arm_mmu500_reset(smmu);
> We should check the return value here, in case the function is modified
> some day in a way that makes it return something else than 0
> 
> LGTM otherwise!
>

Thanks,
Bibek

> Konrad
Bibek Kumar Patro Dec. 19, 2023, 1:06 p.m. UTC | #3 
On 12/16/2023 5:24 AM, Konrad Dybcio wrote:
[...]
>> +
>> +	arm_mmu500_reset(smmu);
> We should check the return value here, in case the function is modified
> some day in a way that makes it return something else than 0
> 

Thanks for pointing this actually. I crosschecked on the
arm_mmu500_reset function behavior, looks like there's no return value
other than 0 and so the functionality won't change. I think we can
keep it as it is in this case.

Thanks,
Bibek

> LGTM otherwise!
> 
> Konrad
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index c8f5dd4186b7..70d2a5d43993 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -516,11 +516,28 @@  static int qcom_smmu_def_domain_type(struct device *dev)
 	return match ? IOMMU_DOMAIN_IDENTITY : 0;
 }

+static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
+{
+	int i;
+	u32 reg;
+
+	arm_mmu500_reset(smmu);
+
+	/* arm_mmu500_reset() disables CPRE which is re-enabled here */
+	for (i = 0; i < smmu->num_context_banks; ++i) {
+		reg = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
+		reg |= CPRE;
+		arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_ACTLR, reg);
+	}
+
+	return 0;
+}
+
 static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
 {
 	int ret;

-	arm_mmu500_reset(smmu);
+	qcom_smmu500_reset(smmu);

 	/*
 	 * To address performance degradation in non-real time clients,
@@ -547,7 +564,7 @@  static const struct arm_smmu_impl qcom_smmu_500_impl = {
 	.init_context = qcom_smmu_init_context,
 	.cfg_probe = qcom_smmu_cfg_probe,
 	.def_domain_type = qcom_smmu_def_domain_type,
-	.reset = arm_mmu500_reset,
+	.reset = qcom_smmu500_reset,
 	.write_s2cr = qcom_smmu_write_s2cr,
 	.tlb_sync = qcom_smmu_tlb_sync,
 };
@@ -572,7 +589,7 @@  static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
 static const struct arm_smmu_impl qcom_adreno_smmu_500_impl = {
 	.init_context = qcom_adreno_smmu_init_context,
 	.def_domain_type = qcom_smmu_def_domain_type,
-	.reset = arm_mmu500_reset,
+	.reset = qcom_smmu500_reset,
 	.alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
 	.write_sctlr = qcom_adreno_smmu_write_sctlr,
 	.tlb_sync = qcom_smmu_tlb_sync,