2023-11-15 09:46:20

by Bibek Kumar Patro

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation



On 11/14/2023 7:45 PM, Dmitry Baryshkov wrote:
> On Tue, 14 Nov 2023 at 15:57, Bibek Kumar Patro
> <[email protected]> wrote:
>>
>> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
>> through SoC specific reset ops, which is disabled in the default MMU-500
>> reset ops, but is expected for context banks using ACTLR register to
>> retain the prefetch value during reset and runtime suspend.
>
> Please refer to Documentation/process/submitting-patches.rst and
> rephrase this following the rules there.
>

Noted, will go through the description once and rephrase it
in next version complying with rules.

>>
>> Signed-off-by: Bibek Kumar Patro <[email protected]>
>> ---
>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 25 ++++++++++++++++++----
>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index 0eaf6f2a2e49..fa867b1d9d16 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -478,11 +478,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);
>> +
>> + /* Re-enable context caching after reset */
>> + 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);
>
> Is this applicable for sdm845? For all other platforms supported by
> qcom_smmu_500 implementation?
>

In arm_mmu500_reset operation drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
CPRE bit is reset for all SoC based on mmu500 platform, hence for all
Qualcomm SoCs including sm845 we are setting back the CPRE bit.

>>
>> /*
>> * To address performance degradation in non-real time clients,
>> @@ -509,7 +526,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,
>> };
>> @@ -528,7 +545,7 @@ static const struct arm_smmu_impl sm8550_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,
>> };
>> @@ -544,7 +561,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.17.1
>>
>
>


2023-11-15 11:04:27

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

On Wed, 15 Nov 2023 at 11:45, Bibek Kumar Patro
<[email protected]> wrote:
>
> On 11/14/2023 7:45 PM, Dmitry Baryshkov wrote:
> > On Tue, 14 Nov 2023 at 15:57, Bibek Kumar Patro
> > <[email protected]> wrote:
> >>
> >> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
> >> through SoC specific reset ops, which is disabled in the default MMU-500
> >> reset ops, but is expected for context banks using ACTLR register to
> >> retain the prefetch value during reset and runtime suspend.
> >
> > Please refer to Documentation/process/submitting-patches.rst and
> > rephrase this following the rules there.
> >
>
> Noted, will go through the description once and rephrase it
> in next version complying with rules.
>
> >>
> >> Signed-off-by: Bibek Kumar Patro <[email protected]>
> >> ---
> >> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 25 ++++++++++++++++++----
> >> 1 file changed, 21 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> index 0eaf6f2a2e49..fa867b1d9d16 100644
> >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> @@ -478,11 +478,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);
> >> +
> >> + /* Re-enable context caching after reset */
> >> + 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);
> >
> > Is this applicable for sdm845? For all other platforms supported by
> > qcom_smmu_500 implementation?
> >
>
> In arm_mmu500_reset operation drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
> CPRE bit is reset for all SoC based on mmu500 platform, hence for all
> Qualcomm SoCs including sm845 we are setting back the CPRE bit.

The errata for the CoreLink MMU-500 requires CPRE to be disabled for
all revisions before r2p2. Do we know whether these SoC used CoreLink
MMU-500 and which version of it?

>
> >>
> >> /*
> >> * To address performance degradation in non-real time clients,
> >> @@ -509,7 +526,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,
> >> };
> >> @@ -528,7 +545,7 @@ static const struct arm_smmu_impl sm8550_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,
> >> };
> >> @@ -544,7 +561,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.17.1
> >>
> >
> >



--
With best wishes
Dmitry

2023-11-16 12:46:03

by Bibek Kumar Patro

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation



On 11/15/2023 4:33 PM, Dmitry Baryshkov wrote:
> On Wed, 15 Nov 2023 at 11:45, Bibek Kumar Patro
> <[email protected]> wrote:
>>
>> On 11/14/2023 7:45 PM, Dmitry Baryshkov wrote:
>>> On Tue, 14 Nov 2023 at 15:57, Bibek Kumar Patro
>>> <[email protected]> wrote:
>>>>
>>>> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
>>>> through SoC specific reset ops, which is disabled in the default MMU-500
>>>> reset ops, but is expected for context banks using ACTLR register to
>>>> retain the prefetch value during reset and runtime suspend.
>>>
>>> Please refer to Documentation/process/submitting-patches.rst and
>>> rephrase this following the rules there.
>>>
>>
>> Noted, will go through the description once and rephrase it
>> in next version complying with rules.
>>
>>>>
>>>> Signed-off-by: Bibek Kumar Patro <[email protected]>
>>>> ---
>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 25 ++++++++++++++++++----
>>>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index 0eaf6f2a2e49..fa867b1d9d16 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -478,11 +478,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);
>>>> +
>>>> + /* Re-enable context caching after reset */
>>>> + 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);
>>>
>>> Is this applicable for sdm845? For all other platforms supported by
>>> qcom_smmu_500 implementation?
>>>
>>
>> In arm_mmu500_reset operation drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
>> CPRE bit is reset for all SoC based on mmu500 platform, hence for all
>> Qualcomm SoCs including sm845 we are setting back the CPRE bit.
>
> The errata for the CoreLink MMU-500 requires CPRE to be disabled for
> all revisions before r2p2. Do we know whether these SoC used CoreLink
> MMU-500 and which version of it?
>

Just checked all these SoCs are using r2p4 revision.
So CPRE needs to be enabled back here then?

>>
>>>>
>>>> /*
>>>> * To address performance degradation in non-real time clients,
>>>> @@ -509,7 +526,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,
>>>> };
>>>> @@ -528,7 +545,7 @@ static const struct arm_smmu_impl sm8550_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,
>>>> };
>>>> @@ -544,7 +561,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.17.1
>>>>
>>>
>>>
>
>
>
> --
> With best wishes
> Dmitry

2023-11-16 15:25:23

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

On Thu, 16 Nov 2023 at 14:45, Bibek Kumar Patro
<[email protected]> wrote:
>
>
>
> On 11/15/2023 4:33 PM, Dmitry Baryshkov wrote:
> > On Wed, 15 Nov 2023 at 11:45, Bibek Kumar Patro
> > <[email protected]> wrote:
> >>
> >> On 11/14/2023 7:45 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 14 Nov 2023 at 15:57, Bibek Kumar Patro
> >>> <[email protected]> wrote:
> >>>>
> >>>> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
> >>>> through SoC specific reset ops, which is disabled in the default MMU-500
> >>>> reset ops, but is expected for context banks using ACTLR register to
> >>>> retain the prefetch value during reset and runtime suspend.
> >>>
> >>> Please refer to Documentation/process/submitting-patches.rst and
> >>> rephrase this following the rules there.
> >>>
> >>
> >> Noted, will go through the description once and rephrase it
> >> in next version complying with rules.
> >>
> >>>>
> >>>> Signed-off-by: Bibek Kumar Patro <[email protected]>
> >>>> ---
> >>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 25 ++++++++++++++++++----
> >>>> 1 file changed, 21 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> index 0eaf6f2a2e49..fa867b1d9d16 100644
> >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> @@ -478,11 +478,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);
> >>>> +
> >>>> + /* Re-enable context caching after reset */
> >>>> + 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);
> >>>
> >>> Is this applicable for sdm845? For all other platforms supported by
> >>> qcom_smmu_500 implementation?
> >>>
> >>
> >> In arm_mmu500_reset operation drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
> >> CPRE bit is reset for all SoC based on mmu500 platform, hence for all
> >> Qualcomm SoCs including sm845 we are setting back the CPRE bit.
> >
> > The errata for the CoreLink MMU-500 requires CPRE to be disabled for
> > all revisions before r2p2. Do we know whether these SoC used CoreLink
> > MMU-500 and which version of it?
> >
>
> Just checked all these SoCs are using r2p4 revision.
> So CPRE needs to be enabled back here then?

can be enabled, yes.

>
> >>
> >>>>
> >>>> /*
> >>>> * To address performance degradation in non-real time clients,
> >>>> @@ -509,7 +526,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,
> >>>> };
> >>>> @@ -528,7 +545,7 @@ static const struct arm_smmu_impl sm8550_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,
> >>>> };
> >>>> @@ -544,7 +561,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.17.1
> >>>>
> >>>
> >>>
> >
> >
> >
> > --
> > With best wishes
> > Dmitry



--
With best wishes
Dmitry

2023-11-16 17:04:49

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

On 16/11/2023 3:24 pm, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 14:45, Bibek Kumar Patro
> <[email protected]> wrote:
>>
>>
>>
>> On 11/15/2023 4:33 PM, Dmitry Baryshkov wrote:
>>> On Wed, 15 Nov 2023 at 11:45, Bibek Kumar Patro
>>> <[email protected]> wrote:
>>>>
>>>> On 11/14/2023 7:45 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 14 Nov 2023 at 15:57, Bibek Kumar Patro
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
>>>>>> through SoC specific reset ops, which is disabled in the default MMU-500
>>>>>> reset ops, but is expected for context banks using ACTLR register to
>>>>>> retain the prefetch value during reset and runtime suspend.
>>>>>
>>>>> Please refer to Documentation/process/submitting-patches.rst and
>>>>> rephrase this following the rules there.
>>>>>
>>>>
>>>> Noted, will go through the description once and rephrase it
>>>> in next version complying with rules.
>>>>
>>>>>>
>>>>>> Signed-off-by: Bibek Kumar Patro <[email protected]>
>>>>>> ---
>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 25 ++++++++++++++++++----
>>>>>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> index 0eaf6f2a2e49..fa867b1d9d16 100644
>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> @@ -478,11 +478,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);
>>>>>> +
>>>>>> + /* Re-enable context caching after reset */
>>>>>> + 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);
>>>>>
>>>>> Is this applicable for sdm845? For all other platforms supported by
>>>>> qcom_smmu_500 implementation?
>>>>>
>>>>
>>>> In arm_mmu500_reset operation drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
>>>> CPRE bit is reset for all SoC based on mmu500 platform, hence for all
>>>> Qualcomm SoCs including sm845 we are setting back the CPRE bit.
>>>
>>> The errata for the CoreLink MMU-500 requires CPRE to be disabled for
>>> all revisions before r2p2. Do we know whether these SoC used CoreLink
>>> MMU-500 and which version of it?
>>>
>>
>> Just checked all these SoCs are using r2p4 revision.
>> So CPRE needs to be enabled back here then?
>
> can be enabled, yes.

There are still open errata #562869 and #1047329 which might need this
workaround. I guess one could argue that we're not (knowingly) using
nested translation at the moment, and also probably not running this in
situations which would end up using short-descriptor format, however
stuff like pKVM and IOMMUFD could potentially change those assumptions
in future, so they still feel a bit sketchy to me.

Thanks,
Robin.

>
>>
>>>>
>>>>>>
>>>>>> /*
>>>>>> * To address performance degradation in non-real time clients,
>>>>>> @@ -509,7 +526,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,
>>>>>> };
>>>>>> @@ -528,7 +545,7 @@ static const struct arm_smmu_impl sm8550_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,
>>>>>> };
>>>>>> @@ -544,7 +561,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.17.1
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>
>
>

2023-11-17 09:10:47

by Bibek Kumar Patro

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation



On 11/16/2023 10:34 PM, Robin Murphy wrote:
> On 16/11/2023 3:24 pm, Dmitry Baryshkov wrote:
>> On Thu, 16 Nov 2023 at 14:45, Bibek Kumar Patro
>> <[email protected]> wrote:
>>>
>>>
>>>
>>> On 11/15/2023 4:33 PM, Dmitry Baryshkov wrote:
>>>> On Wed, 15 Nov 2023 at 11:45, Bibek Kumar Patro
>>>> <[email protected]> wrote:
>>>>>
>>>>> On 11/14/2023 7:45 PM, Dmitry Baryshkov wrote:
>>>>>> On Tue, 14 Nov 2023 at 15:57, Bibek Kumar Patro
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> Context caching is re-enabled in the prefetch buffer for Qualcomm
>>>>>>> SoCs
>>>>>>> through SoC specific reset ops, which is disabled in the default
>>>>>>> MMU-500
>>>>>>> reset ops, but is expected for context banks using ACTLR register to
>>>>>>> retain the prefetch value during reset and runtime suspend.
>>>>>>
>>>>>> Please refer to Documentation/process/submitting-patches.rst and
>>>>>> rephrase this following the rules there.
>>>>>>
>>>>>
>>>>> Noted, will go through the description once and rephrase it
>>>>> in next version complying with rules.
>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Bibek Kumar Patro <[email protected]>
>>>>>>> ---
>>>>>>>     drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 25
>>>>>>> ++++++++++++++++++----
>>>>>>>     1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> index 0eaf6f2a2e49..fa867b1d9d16 100644
>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> @@ -478,11 +478,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);
>>>>>>> +
>>>>>>> +       /* Re-enable context caching after reset */
>>>>>>> +       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);
>>>>>>
>>>>>> Is this applicable for sdm845? For all other platforms supported by
>>>>>> qcom_smmu_500 implementation?
>>>>>>
>>>>>
>>>>> In arm_mmu500_reset operation
>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
>>>>> CPRE bit is reset for all SoC based on mmu500 platform, hence for all
>>>>> Qualcomm SoCs including sm845 we are setting back the CPRE bit.
>>>>
>>>> The errata for the CoreLink MMU-500 requires CPRE to be disabled for
>>>> all revisions before r2p2. Do we know whether these SoC used CoreLink
>>>> MMU-500 and which version of it?
>>>>
>>>
>>> Just checked all these SoCs are using r2p4 revision.
>>> So CPRE needs to be enabled back here then?
>>
>> can be enabled, yes.
>
> There are still open errata #562869 and #1047329 which might need this
> workaround. I guess one could argue that we're not (knowingly) using
> nested translation at the moment, and also probably not running this in
> situations which would end up using short-descriptor format, however
> stuff like pKVM and IOMMUFD could potentially change those assumptions
> in future, so they still feel a bit sketchy to me.
>

Could you help provide some details on these two errata (#562869 and
#1047329).Both of these erratum are there for r2p4 revisions as well?

Thanks & regards,
Bibek

> Thanks,
> Robin.
>
>>
>>>
>>>>>
>>>>>>>
>>>>>>>            /*
>>>>>>>             * To address performance degradation in non-real time
>>>>>>> clients,
>>>>>>> @@ -509,7 +526,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,
>>>>>>>     };
>>>>>>> @@ -528,7 +545,7 @@ static const struct arm_smmu_impl
>>>>>>> sm8550_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,
>>>>>>>     };
>>>>>>> @@ -544,7 +561,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.17.1
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> With best wishes
>>>> Dmitry
>>
>>
>>