Re: [PATCH v14 1/6] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Bibek Kumar Patro <quic_bibekkum@quicinc.com>]

On 8/30/2024 6:23 PM, Robin Murphy wrote:
> On 16/08/2024 6:42 pm, 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.
>>
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 36 ++++++++++++++++++++--
>>   1 file changed, 33 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 36c6b36ad4ff..8ac1850b852f 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -16,6 +16,16 @@
>>
>>   #define QCOM_DUMMY_VAL    -1
>>
>> +/*
>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch
>> + * buffer). The remaining bits are implementation defined and vary 
>> across
>> + * SoCs.
>> + */
>> +
>> +#define CPRE            (1 << 1)
>> +#define CMTLB            (1 << 0)
>> +
>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>   {
>>       return container_of(smmu, struct qcom_smmu, smmu);
>> @@ -381,11 +391,31 @@ 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 ret;
>> +    u32 val;
>> +    int i;
>> +
>> +    ret = arm_mmu500_reset(smmu);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* arm_mmu500_reset() disables CPRE which is re-enabled here */
> 
> I still think it would be good to document why we think this is OK, 
> given the reasons for disabling CPRE to begin with.
> 

Hi Robin,

Sure, I agree with your point. I’ll elaborate on the comment here to 
document the reason for re-enabling CPRE.

/* The errata for MMU-500 before the r2p2 revision requires CPRE to be
disabled. The arm_mmu500_reset function disables CPRE to accommodate all
RTL revisions. Since all Qualcomm SoCs are on the r2p4 revision, where
the CPRE bit can be enabled, the qcom_smmu500_reset function re-enables
the CPRE bit for the next-page prefetcher to retain the prefetch value
during reset and runtime suspend operations. */

Does this sound good, let me know of your suggestions as well incase.

Thanks & regards,
Bibek

> Thanks,
> Robin.
> 
>> +    for (i = 0; i < smmu->num_context_banks; ++i) {
>> +        val = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
>> +        val |= CPRE;
>> +        arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_ACTLR, val);
>> +    }
>> +
>> +    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,
>> @@ -412,7 +442,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,
>>   #ifdef CONFIG_ARM_SMMU_QCOM_DEBUG
>> @@ -445,7 +475,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,
>> -- 
>> 2.34.1
>>