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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Maximilian Luz <luzmaximilian@gmail.com>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ard Biesheuvel <ardb@kernel.org>
Cc: Konrad Dybcio <konrad.dybcio@somainline.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Steev Klimaszewski <steev@kali.org>,
	Shawn Guo <shawn.guo@linaro.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Cristian Marussi <cristian.marussi@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-arm-msm@vger.kernel.org, linux-efi@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Vinod Koul <vkoul@kernel.org>
Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure Application client
Date: Thu, 28 Jul 2022 09:48:13 +0200	[thread overview]
Message-ID: <95cbcda8-d1bc-376c-b338-92d1b923f04a@linaro.org> (raw)
In-Reply-To: <acd7b231-3167-e35c-5cdf-8b3127a7d710@gmail.com>

On 27/07/2022 15:00, Maximilian Luz wrote:
>>> 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.

You start with one specific compatible and if needed later either add
more specific upfront (qcom,sc8280x-tee-uefisecapp,
qcom,sc8180x-tee-uefisecapp) or as entirely new one if it is not compatible.

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

For example like tegra_bpmp_get() is doing.

Best regards,
Krzysztof