From: Will Deacon <will@kernel.org>
To: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Cc: akhilpo@codeaurora.org, iommu@lists.linux-foundation.org,
jcrouse@codeaurora.org, linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
robdclark@gmail.com, robin.murphy@arm.com,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Jordan Crouse <jordan@cosmicpenguin.net>
Subject: Re: [PATCHv2 2/2] iommu/arm-smmu-qcom: Move the adreno smmu specific impl earlier
Date: Thu, 25 Mar 2021 15:05:07 +0000 [thread overview]
Message-ID: <20210325150506.GD15172@willie-the-truck> (raw)
In-Reply-To: <727fa9fe2e644f88ba35c2877d71788e@codeaurora.org>
On Thu, Mar 25, 2021 at 01:10:12PM +0530, Sai Prakash Ranjan wrote:
> On 2021-03-15 00:31, Sai Prakash Ranjan wrote:
> > On 2021-03-12 04:59, Bjorn Andersson wrote:
> > > On Sat 27 Feb 07:53 CST 2021, Sai Prakash Ranjan wrote:
> > > > On 2021-02-27 00:44, Bjorn Andersson wrote:
> > > > > On Fri 26 Feb 12:23 CST 2021, Rob Clark wrote:
> > > > >
> > > > >
> > > > > The current logic picks one of:
> > > > > 1) is the compatible mentioned in qcom_smmu_impl_of_match[]
> > > > > 2) is the compatible an adreno
> > > > > 3) no quirks needed
> > > > >
> > > > > The change flips the order of these, so the only way I can see this
> > > > > change affecting things is if we expected a match on #2, but we got one
> > > > > on #1.
> > > > >
> > > > > Which implies that the instance that we want to act according to the
> > > > > adreno impl was listed in qcom_smmu_impl_of_match[] - which either is
> > > > > wrong, or there's a single instance that needs both behaviors.
> > > > >
> > > > > (And I believe Jordan's answer confirms the latter - there's a single
> > > > > SMMU instance that needs all them quirks at once)
> > > > >
> > > >
> > > > Let me go through the problem statement in case my commit
> > > > message wasn't
> > > > clear. There are two SMMUs (APSS and GPU) on SC7280 and both are
> > > > SMMU500
> > > > (ARM SMMU IP).
> > > >
> > > > APSS SMMU compatible - ("qcom,sc7280-smmu-500", "arm,mmu-500")
> > > > GPU SMMU compatible - ("qcom,sc7280-smmu-500",
> > > > "qcom,adreno-smmu", "arm,mmu-500")
> > > >
> > > > Now if we take SC7180 as an example, GPU SMMU was QSMMU(QCOM SMMU IP)
> > > > and APSS SMMU was SMMU500(ARM SMMU IP).
> > > >
> > > > APSS SMMU compatible - ("qcom,sc7180-smmu-500", "arm,mmu-500")
> > > > GPU SMMU compatible - ("qcom,sc7180-smmu-v2",
> > > > "qcom,adreno-smmu", "qcom,smmu-v2")
> > > >
> > > > Current code sequence without this patch,
> > > >
> > > > if (of_match_node(qcom_smmu_impl_of_match, np))
> > > > return qcom_smmu_create(smmu, &qcom_smmu_impl);
> > > >
> > > > if (of_device_is_compatible(np, "qcom,adreno-smmu"))
> > > > return qcom_smmu_create(smmu, &qcom_adreno_smmu_impl);
> > > >
> > > > Now if we look at the compatible for SC7180, there is no problem
> > > > because
> > > > the APSS SMMU will match the one in qcom_smmu_impl_of_match[]
> > > > and GPU SMMU
> > > > will match "qcom,adreno-smmu" because the compatible strings are
> > > > different.
> > > > But for SC7280, both the APSS SMMU and GPU SMMU
> > > > compatible("qcom,sc7280-smmu-500")
> > > > are same. So GPU SMMU will match with the one in
> > > > qcom_smmu_impl_of_match[]
> > > > i.e.., "qcom,sc7280-smmu-500" which functionally doesn't cause
> > > > any problem
> > > > but we will miss settings for split pagetables which are part of
> > > > GPU SMMU
> > > > specific implementation.
> > > >
> > > > We can avoid this with yet another new compatible for GPU SMMU
> > > > something like
> > > > "qcom,sc7280-adreno-smmu-500" but since we can handle this
> > > > easily in the
> > > > driver and since the IPs are same, meaning if there was a
> > > > hardware quirk
> > > > required, then we would need to apply to both of them and would
> > > > this additional
> > > > compatible be of any help?
> > > >
> > >
> > > No, I think you're doing the right thing of having them both. I just
> > > didn't remember us doing that.
> > >
> > > > Coming to the part of quirks now, you are right saying both
> > > > SMMUs will need
> > > > to have the same quirks in SC7280 and similar others where both
> > > > are based on
> > > > same IPs but those should probably be *hardware quirks* and if
> > > > they are
> > > > software based like the S2CR quirk depending on the firmware,
> > > > then it might
> > > > not be applicable to both. In case if it is applicable, then as
> > > > Jordan mentioned,
> > > > we can add the same quirks in GPU SMMU implementation.
> > > >
> > >
> > > I do suspect that at some point (probably sooner than later) we'd have
> > > to support both inheriting of stream from the bootloader and the
> > > Adreno
> > > "quirks" in the same instance.
> > >
> > > But for now this is okay to me.
> > >
> >
> > Sure, let me know if you or anyone face any issues without it and I will
> > add it. I will resend this series with the dt-bindings patch for sc7280
> > smmu
> > which wasn't cc'd to smmu folks by mistake.
> >
>
> I think there is consensus on this series. I can resend if required but it
> still applies cleanly, let me know if you have any comments?
Please resend with the bindings patch, and I'd like Bjorn's Ack as well.
Will
next prev parent reply other threads:[~2021-03-25 15:06 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-26 9:55 [PATCHv2 0/2] iommu/arm-smmu-qcom: Add SC7280 support Sai Prakash Ranjan
2021-02-26 9:55 ` [PATCHv2 1/2] iommu/arm-smmu-qcom: Add SC7280 SMMU compatible Sai Prakash Ranjan
2021-02-26 9:55 ` [PATCHv2 2/2] iommu/arm-smmu-qcom: Move the adreno smmu specific impl earlier Sai Prakash Ranjan
2021-02-26 15:51 ` Jordan Crouse
2021-02-26 17:24 ` Bjorn Andersson
2021-02-26 18:23 ` Rob Clark
2021-02-26 19:14 ` Bjorn Andersson
2021-02-27 13:53 ` Sai Prakash Ranjan
2021-03-11 23:29 ` Bjorn Andersson
2021-03-14 19:01 ` Sai Prakash Ranjan
2021-03-25 7:40 ` Sai Prakash Ranjan
2021-03-25 15:05 ` Will Deacon [this message]
2021-04-05 8:42 ` Sai Prakash Ranjan
2021-04-19 3:11 ` Sai Prakash Ranjan
2021-02-26 18:48 ` Jordan Crouse
2021-02-26 19:54 ` Jordan Crouse
2021-04-19 14:38 ` Bjorn Andersson
2021-04-20 6:06 ` Sai Prakash Ranjan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210325150506.GD15172@willie-the-truck \
--to=will@kernel.org \
--cc=akhilpo@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=iommu@lists.linux-foundation.org \
--cc=jcrouse@codeaurora.org \
--cc=jordan@cosmicpenguin.net \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robdclark@gmail.com \
--cc=robin.murphy@arm.com \
--cc=saiprakash.ranjan@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).