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
