On 7/27/22 13:24, Krzysztof Kozlowski wrote:
On 26/07/2022 17:00, Maximilian Luz wrote:
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).
Expectation is not the same as having a specification saying it will not
change.
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.
Such approach is not "normal" for third-party devices. Compatible for
devices has model number. If the block has specification, then v2 would
have sense, otherwise you would invent own versioning...
I would say that firmware implementation can easily change. How much of
your code is tied to it, I don't know, but argument "I don't expect
Qualcomm to change something in their firmware" is not the correct argument.
Fair points.
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.
Why bunch? All devices could bind to one specific compatible, as they
are compatible.
Ah, I think I misunderstood you there. I thought you were advocating for
creating compatibles for each SoC just because it's a new SoC and things
might be different. I'm not at all against naming this something like
qcom,tee-uefisecapp-sc8180x then using that on all platforms that work.
I just didn't like the idea of having a bunch of different
qcom,tee-uefisecapp-<soc> pointing to the exact same thing without any
difference at all.
+
+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.
And how do you differentiate that qcom_scm_is_available() failed because
it is not yet available (defer probe) or it is broken and will never
load? All regular consumer-provider interfaces have it sorted out.
Fair point. By shifting that to device links you'll at least know what
it's waiting for and the driver won't attempt to probe until that's
resolved. But your question applies to that then as well: How do you
differentiate between the device link or supplier being broken somehow
and the supplier being just not ready yet?
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?
The Tegra choice is not complete, but it implements at least parts of it
and firmware dependencies are modeled in DTS. Other way would be to add
your device as child of SMC firmware and then you do not need bindings
at all...
Looking at the TrEE driver in [1] and thinking of future additions
(re-entrant calls, callbacks/listeners, ..., all of which would require
some state), combining both might be the best option: Have a TrEE main
device for the interface that links up with SCM and then define the apps
as children under that TrEE device.
Regards,
Max
[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/auto-kernel.lnx.4.14.c34/drivers/misc/qseecom.c
