Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client

[Krzysztof Kozlowski]

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)?

> 
>>> +
>>> +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...

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.

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.

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).

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?


Best regards,
Krzysztof