Re: [RFC PATCH v2 08/11] iommu/arm-smmu-qcom: provide separate implementation for SDM845-smmu-500

[Richard Acayan <mailingradian@gmail.com>]

On Sat, Nov 05, 2022 at 03:02:15AM +0300, Dmitry Baryshkov wrote:
> On Sat, 5 Nov 2022 at 01:16, Richard Acayan <mailingradian@gmail.com> wrote:
>>
>> On Wed, Nov 02, 2022 at 09:44:17PM +0300, Dmitry Baryshkov wrote:
>> > There is only one platform, which needs special care in the reset
>> > function, the SDM845. Add special handler for sdm845 and drop the
>> > qcom_smmu500_reset() function.
>> >
>> > Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>
>> > Tested-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>
>> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> > ---
>> >  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++----------
>> >  1 file changed, 19 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> > index c3bcd6eb2f42..75bc770ccf8c 100644
>> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> > @@ -361,6 +361,8 @@ static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
>> >  {
>> >       int ret;
>> >
>> > +     arm_mmu500_reset(smmu);
>> > +
>> >       /*
>> >        * To address performance degradation in non-real time clients,
>> >        * such as USB and UFS, turn off wait-for-safe on sdm845 based boards,
>> > @@ -374,23 +376,20 @@ static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
>> >       return ret;
>> >  }
>> >
>> > -static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
>> > -{
>> > -     const struct device_node *np = smmu->dev->of_node;
>> > -
>> > -     arm_mmu500_reset(smmu);
>> > -
>> > -     if (of_device_is_compatible(np, "qcom,sdm845-smmu-500"))
>> > -             return qcom_sdm845_smmu500_reset(smmu);
>> > -
>> > -     return 0;
>> > -}
>> > -
>> >  static const struct arm_smmu_impl qcom_smmu_impl = {
>> >       .init_context = qcom_smmu_init_context,
>> >       .cfg_probe = qcom_smmu_cfg_probe,
>> >       .def_domain_type = qcom_smmu_def_domain_type,
>> > -     .reset = qcom_smmu500_reset,
>> > +     .reset = arm_mmu500_reset,
>> > +     .write_s2cr = qcom_smmu_write_s2cr,
>> > +     .tlb_sync = qcom_smmu_tlb_sync,
>> > +};
>> > +
>> > +static const struct arm_smmu_impl sdm845_smmu_500_impl = {
>> > +     .init_context = qcom_smmu_init_context,
>> > +     .cfg_probe = qcom_smmu_cfg_probe,
>> > +     .def_domain_type = qcom_smmu_def_domain_type,
>> > +     .reset = qcom_sdm845_smmu500_reset,
>> >       .write_s2cr = qcom_smmu_write_s2cr,
>> >       .tlb_sync = qcom_smmu_tlb_sync,
>> >  };
>> > @@ -398,7 +397,7 @@ static const struct arm_smmu_impl qcom_smmu_impl = {
>> >  static const struct arm_smmu_impl qcom_adreno_smmu_impl = {
>> >       .init_context = qcom_adreno_smmu_init_context,
>> >       .def_domain_type = qcom_smmu_def_domain_type,
>> > -     .reset = qcom_smmu500_reset,
>> > +     .reset = arm_mmu500_reset,
>> >       .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
>> >       .write_sctlr = qcom_adreno_smmu_write_sctlr,
>> >       .tlb_sync = qcom_smmu_tlb_sync,
>> > @@ -450,6 +449,11 @@ static const struct qcom_smmu_match_data qcom_smmu_data = {
>> >       .adreno_impl = &qcom_adreno_smmu_impl,
>> >  };
>> >
>> > +static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
>> > +     .impl = &sdm845_smmu_500_impl,
>> > +     /* No adreno impl, on sdm845 it is handled by separete sdm845-smmu-v2. */
>> separete -> separate
>
> Ack.
>
>> Also, while I'm here, does "No adreno impl" constitute adding a
>> compatible in the driver?
>
> Not sure that I got your question, please excuse me. Could you please
> describe what you meant?
> We already have qcom,sdm845-smmu-v2 in the match table, if that's your
> question. And there is no need for Adreno impl here, on sdm845 the
> SMMU connected to Adreno is v2 rather than mmu-500.

I'm asking because I wrote this patch:

https://lore.kernel.org/linux-iommu/20221103232632.217324-3-mailingradian@gmail.com/

on the basis that the SDM670 SMMU shouldn't have an adreno_impl. I
looked at the other code in this series, and it shouldn't be a problem
to use the fallback entry for SDM670. The adreno_impl is simply unused,
and would cause no problems if it were in the match data for any
platform. Going through the code, I'm considering dropping that patch I
wrote. My question should have been, "if I want to add support for an
SMMU, with no differences from a regular Qualcomm MMU-500, except
without an Adreno variant, does that deserve another entry and match
data?"

I would guess that this is not the case. The sdm845-smmu-v2 uses the
qcom_smmu_data, which includes the regular impl, even though there is no
regular sdm845-smmu-v2 that is not for Adreno.

> Probably I should change this to "No need for adreno impl....". Would
> that be better?

Yes, I think it would be better to clarify that this omission wasn't
made because it's necessary for the driver to work. That will possibly
save people from jumping to the match table, seeing this omission, and
writing a similar match data, only to have it dropped after reading
through the full driver.

>
>> > +};
>> > +
>> >  static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>> >       { .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
>> >       { .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_data },
>> > @@ -460,7 +464,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>> >       { .compatible = "qcom,sc8280xp-smmu-500", .data = &qcom_smmu_data },
>> >       { .compatible = "qcom,sdm630-smmu-v2", .data = &qcom_smmu_data },
>> >       { .compatible = "qcom,sdm845-smmu-v2", .data = &qcom_smmu_data },
>> > -     { .compatible = "qcom,sdm845-smmu-500", .data = &qcom_smmu_data },
>> > +     { .compatible = "qcom,sdm845-smmu-500", .data = &sdm845_smmu_500_data },
>> >       { .compatible = "qcom,sm6125-smmu-500", .data = &qcom_smmu_data },
>> >       { .compatible = "qcom,sm6350-smmu-500", .data = &qcom_smmu_data },
>> >       { .compatible = "qcom,sm6375-smmu-500", .data = &qcom_smmu_data },
>> > --
>> > 2.35.1
>> >
>
>
>
> -- 
> With best wishes
> Dmitry