On Mon, Sep 08, 2025 at 10:23:31AM +0200, Stephan Gerhold wrote:
> On Mon, Aug 25, 2025 at 04:49:56PM +0530, Mukesh Ojha wrote:
> > On Sat, Aug 23, 2025 at 10:43:52PM +0200, Stephan Gerhold wrote:
> > > On Fri, Aug 22, 2025 at 10:10:30PM +0530, Mukesh Ojha wrote:
> > > > On Fri, Aug 22, 2025 at 06:26:19PM +0200, Stephan Gerhold wrote:
> > > > > On Fri, Aug 22, 2025 at 08:36:11PM +0530, Mukesh Ojha wrote:
> > > > > > On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> > > > > > > On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > > > > > > > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > > > > > > > >>>> +int iris_fw_init(struct iris_core *core)
> > > > > > > > >>>> +{
> > > > > > > > >>>> + struct platform_device_info info;
> > > > > > > > >>>> + struct iommu_domain *iommu_dom;
> > > > > > > > >>>> + struct platform_device *pdev;
> > > > > > > > >>>> + struct device_node *np;
> > > > > > > > >>>> + int ret;
> > > > > > > > >>>> +
> > > > > > > > >>>> + np = of_get_child_by_name(core->dev->of_node,
> > > > > > > > >>>> "video-firmware");
> > > > > > > > >>>> + if (!np)
> > > > > > > > >>>> + return 0;
> > > > > > > > >>> You need a dt-bindings change for this as well. This is
> > > > > > > > >>> documented only
> > > > > > > > >>> for Venus.
> > > > > > > > >> You are right, wanted to send device tree and binding
> > > > > > > > >> support separately.
> > > > > > > > >> But if required, will add with the series in the next
> > > > > > > > >> version.
> > > > > > > > >>
> > > > > > > > > You can send device tree changes separately, but dt-binding
> > > > > > > > > changes
> > > > > > > > > always need to come before the driver changes.
> > > > > > > >
> > > > > > > > Do you mean to update the examples section[1] with the firmware
> > > > > > > > subnode,
> > > > > > > > something similar to venus schema[2] ?
> > > > > > > >
> > > > > > >
> > > > > > > Sorry, I missed the fact that the "video-firmware" subnode is
> > > > > > > already
> > > > > > > documented for iris as well through qcom,venus-common.yaml (which
> > > > > > > is
> > > > > > > included for qcom,sm8550-iris). I don't think it's strictly
> > > > > > > required to
> > > > > > > add every possibility to the examples of the schema, since we'll
> > > > > > > also
> > > > > > > have the actual DTBs later to test this part of the schema.
> > > > > > >
> > > > > > > I would recommend to extend the description of the
> > > > > > > "video-firmware" node
> > > > > > > in qcom,venus-common.yaml a bit. You do use the reset
> > > > > > > functionality of
> > > > > > > TrustZone, so the description there doesn't fit for your use case.
> > > > > > >
> > > > > > > I think we will also have to figure out how to handle the old
> > > > > > > "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> > > > > > > registers) vs the EL2 PAS use case (that resets Iris in TZ but
> > > > > > > still
> > > > > > > handles IOMMU from Linux). Simply checking for the presence of the
> > > > > > > "video-firmware" node is not enough, because that doesn't tell us
> > > > > > > if the
> > > > > > > PAS support is present in TZ.
> > > > > > >
> > > > > > > I have been experimenting with a similar patch that copies the
> > > > > > > "non_tz"
> > > > > > > code paths from Venus into Iris. We need this to upstream the
> > > > > > > Iris DT
> > > > > > > patch for X1E without regressing the community-contributed
> > > > > > > x1-el2.dtso,
> > > > > > > which doesn't have functional PAS when running in EL2.
> > > > > > >
> > > > > > > Perhaps we could check for __qcom_scm_is_call_available() with
> > > > > > > the new
> > > > > > > QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset
> > > > > > > via PAS
> > > > > > > or directly with the registers. I don't have a device with the new
> > > > > > > firmware to verify if that works.
> > > > > >
> > > > > > You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with
> > > > > > __qcom_scm_is_call_available()
> > > > > > but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC
> > > > > > call will be
> > > > > > used even for Gunyah. So, I believe, __qcom_scm_is_call_available()
> > > > > > and
> > > > > > video-firmware's iommu property is also important.
> > > > > >
> > > > >
> > > > > Yeah, this sounds good.
> > > > >
> > > > > > >
> > > > > > > I'll try to send out my patch soon, so you can better see the
> > > > > > > context.
> > > > > >
> > > > > > Are you saying that you are going to send patch to support IRIS on
> > > > > > x1-el2.dtso in non-secure way i.e., non-PAS way.
> > > > > >
> > > > >
> > > > > The background is the following: I have a pending patch to add iris to
> > > > > x1e80100.dtsi, but that currently breaks x1-el2.dtso. My original plan
> > > > > was to disable &iris in x1-el2.dtso (because the PAS way seems to be
> > > > > just broken), but then I saw that e.g. sc7180-el2.dtso does have
> > > > > working
> > > > > Venus with the "video-firmware" node. Copy-pasting the
> > > > > "no_tz"(/non-PAS)
> > > > > code as-is from venus into iris works just fine for x1-el2.dtso, so
> > > > > disabling &iris in x1-el2.dtso just because the "no_tz" code is
> > > > > currently missing in iris doesn't sound right.
> > > > >
> > > > > As far as I understand the approach you use in this series does not
> > > > > work
> > > > > without the TZ changes for older platforms like X1E(?), so adding that
> > > > > code in iris seems to be the best way to move forward.
> > > >
> > > > Yes, this series has dependency on firmware and will not work for older
> > > > platforms.
> > > >
> > > > >
> > > > > I started working on a patch for this a while ago, it just needs a bit
> > > > > more cleanup. I'll try to finish it up and post it so we can discuss
> > > > > it
> > > > > further. I think the IOMMU management in my patch would even work
> > > > > as-is
> > > > > for you, you would just need to toggle a boolean to use the PAS
> > > > > instead
> > > > > of accessing the registers directly.
> > > >
> > > > Sounds like a plan.
> > > > Thanks, please cc me when you send the patches; So, I could test along
> > > > with my changes and make dependency on it.
> > > >
> > >
> > > Krzysztof raised the concern that we shouldn't model the IOMMU specifier
> > > for the firmware using a "video-firmware" subnode [1], similar to the
> > > discussion for the "non-pixel" subnode recently [2].
> > >
> > > I mostly finished up the cleanup of my patch, but I don't see any point
> > > in posting it without an alternative proposal for the dt-bindings. For
> > > this case, I think a simple property like
> > >
> > > firmware-iommus = <&apps_smmu ...>;
> > >
> > > instead of
> > >
> > > video-firmware {
> > > iommus = <&apps_smmu ...>;
> > > };
> > >
> > > could perhaps work. (XYZ-iommus isn't standardized at the moment, but I
> > > think something like XYZ-gpios would make sense in this case. There are
> > > many other possible approaches as well though.)
> > >
> > > Unfortunately, I won't have enough time in the next weeks to fully
> > > implement and propose an alternative. I'm assuming you still have
> > > ongoing work for supporting the "non-pixel" IOMMU, perhaps your new
> > > approach can be adapted for video-firmware as well?
> >
> > I believe, non-pixel case a bit different and thats not depends on whether
> > it is PAS or non-PAS.
> >
> > However, I liked the idea about introducing something similar to -gpios
> > for -iommus as could pottentially solves at least this issue. Here, we need
> > to create a platform device and its domain based on firmware-iommu
> > property.
> >
> > So, its required change in device link to put supplier/consumer dependency
> > and addition of firmware-iommu binding for IRIS and little of changes
> > over your existing changes.
> >
> > But I have doubt, whether @Krzysztof would be fine with it ?
> >
>
> Krzysztof isn't on Cc here so I wouldn't expect him to reply. :-)
> I'm not sure if it's helpful to add him in the middle of the discussion
> either (at least without proper summary of the problem description).
>
> I think it would be best to prepare a patch series with the motivation
> properly described. If making the actual implementation (to create the
> platform device etc) is too much work it could also be sent as RFC with
> only the dt-bindings.
>
> Have you continued working on this to unblock adding the IOMMU needed
> for the IRIS firmware?
We are discussing on this internally and if this can be taken along with
non-pixel case or not, will come back on this.
If it takes too much, will drop video support for now in next version.
>
> Thanks,
> Stephan
--
-Mukesh Ojha
