Messages in this thread | | | Date | Tue, 26 Jul 2022 17:00:43 +0200 | Subject | Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client | From | Maximilian Luz <> |
| |
On 7/26/22 15:25, Krzysztof Kozlowski wrote: > On 26/07/2022 13:15, Maximilian Luz wrote: >>>> +properties: >>>> + compatible: >>>> + const: qcom,tee-uefisecapp >>> >>> Isn't this SoC-specific device? Generic compatibles are usually not >>> expected. >> >> This is essentially software (kernel driver) talking to software (in the >> TrustZone), so I don't expect there to be anything SoC specific about it. > > You are documenting here firmware in TZ (not kernel driver). Isn't this > a specific piece which might vary from device to device? > > IOW, do you expect the same compatible to work for all possible Qualcomm > boards (past and future like in 10 years from now)?
I'm not sure if Qualcomm will still use the "uefisecapp" approach in 10 years, but I don't expect the interface of uefisecapp to change. The interface is modeled after the respective UEFI functions, which are spec and thus I don't expect those to change. Also, it seems to have been around for a couple of generations and it hasn't changed. The oldest tested is sdm850 (Lenovo Yoga C630), and the latest is sc8280xp (Thinkpad X13s).
Why not make this behave like a "normal" third-party device? If the interface ever changes use qcom,tee-uefisecapp-v2 or something like that? Again, this does not seem to be directly tied to the SoC.
Then again, if you prefer to name everything based on "qcom,<device>-<soc>" I don't have any strong arguments against it and I'm happy to change that. I just think it will unnecessarily introduce a bunch of compatibles and doesn't reflect the interface "versioning" situation as I see it.
>>>> + >>>> +required: >>>> + - compatible >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + firmware { >>>> + scm { >>>> + compatible = "qcom,scm-sc8180x", "qcom,scm"; >>>> + }; >>>> + tee-uefisecapp { >>>> + compatible = "qcom,tee-uefisecapp"; >>> >>> You did not model here any dependency on SCM. This is not full >>> description of the firmware/hardware >> >> How would I do that? A lot of other stuff also depends on SCM being >> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them >> declare this in the device tree. As far as I can tell, SCM is pretty >> much expected to be there at all times (i.e. can't be unloaded) and >> drivers check for it when probing via qcom_scm_is_available(), >> deferring probe if not. > > It seems this will be opening a can of worms...
Indeed.
> The problem with existing approach is: > 1. Lack of any probe ordering or probe deferral support. > 2. Lack of any other dependencies, e.g. for PM.
I'm not entirely sure what you mean by "lack of probe deferral support". We have qcom_scm_is_available() and defer probe if that fails. So deferral works, unless I'm misunderstanding something.
But yes, correct on the other points.
> Unloading is "solved" only by disallowing the unload, not by proper > device links and module get/put. > > I understand that SCM must be there, but the same for several other > components and for these others we have ways to pass reference around > (e.g. syscon regmap, PHYs handles). > >> >> Don't take this as an excuse as in "I want to leave that out", it's just >> that I don't know how one would declare such a dependency explicitly. If >> you can tell me how to fix it, I'll include that for v2. > > I think there are no dedicated subsystem helpers for this (like for > provider/consumer of resets/power domains/clocks etc), so one way would > be something like nvidia,bpmp is doing.
I assume you're referring to tegra_bpmp_get()? Does this correctly handle PM dependencies? At least as far as I can tell it doesn't explicitly establish a device link, it only gets a reference to the device, which doesn't guarantee the presence of a driver. Nor correct PM ordering. Please correct me if I'm wrong. As far as I know automatic creation of device links only works with certain props defined in of_supplier_bindings, right?
So unless I'm wrong there is also a bunch of other stuff that may be subtly broken. (Again, not a justification to include these changes, just wondering whether there should be a conscious approach to find and fix these things... rather than discover them patch-by-patch).
> meson_sm_get is a bit similar - looking by compatible. This is less > portable and I would prefer the bpmp way (just like syscon phandles).
I have another example (that could be improved via a phandle in DT): For the Surface System Aggregator (in ACPI-land), we have ssam_client_bind(). This function 1) checks if the controller is available and ready, 2) if it is gets a reference to it, and 3) establishes a device link for PM-ordering, before 4) returning the reference to that controller to the client. This combined with deferring probe ensures that we will always have a valid reference. And since we're in DT-land, we could hook that up with a phandle reference to SCM and load that instead of having to use a global static.
> The qcom_q6v5_pas could be converted later to use similar approach and > eventually the "tatic struct qcom_scm *__scm;" can be entirely removed. > > Any comments on this approach from Konrad, Bjorn, Dmitry, Vinod and > anyone else?
Regards, Max
| |