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

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

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