Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/06/2024 09:19, Krzysztof Kozlowski wrote:
> On 20/06/2024 09:14, Viacheslav wrote:
>>
>>
>> 17/06/2024 19.57, Conor Dooley пишет:
>>> On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote:
>>>> Thanks for review.
>>>>
>>>> 13/06/2024 19.42, Rob Herring wrote:
>>>>> On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
>>>>>> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> 10/06/2024 19.08, Conor Dooley wrote:
>>>>>>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
>>>>>>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
>>>>>>>>
>>>>>>>> "bindings are for hardware, not drivers". Why purpose does the "secure
>>>>>>>> monitor" serve that the secure firmware needs a reference to it?
>>>>>>>
>>>>>>> This driver is an extension to the meson-gx-socinfo driver: it supplements
>>>>>>> information obtained from the register with information from the
>>>>>>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
>>>>>>> loading order, we cannot do away with meson-gx-socinfo, as it is used for
>>>>>>> platform identification in some drivers. Therefore, the extended information
>>>>>>> is formatted as a separate driver, which is loaded after the secure-monitor
>>>>>>> driver.
>>>>>>
>>>>>> Please stop talking about drivers, this is a binding which is about
>>>>>> hardware. Please provide, in your next version, a commit message that
>>>>>> justifies adding this property without talking about driver probing
>>>>>> order etc, and instead focuses on what service the "secure monitor"
>>>>>> provides etc.
>>>>>
>>>>> To put it another way, how many secure monitors does 1 system have?
>>>>
>>>> One per system in current device tree.
>>>
>>> One per system, or one is currently described per system, but more might
>>> be added later?
>>
>> it turns out to be one per system. It's either there or it's not.
>>
>>>
>>>>> What do you do if the property is not present? You didn't make it
>>>>> required which is good because that would be an ABI break.
>>>>
>>>> We need an indication of the ability to use the secure-monitor to obtain
>>>> additional information within the soc driver. It seemed to me that using an
>>>> explicit reference to the secure-monitor is the best choice.
>>>>
>>>>>
>>>>> You only need a link in DT if there are different possible providers or
>>>>> some per consumer information to describe (e.g. an interrupt number or
>>>>> clock ID). You don't have the latter and likely there is only 1 possible
>>>>> provider.
>>>>
>>>> Would replacing the reference to sm with an option, for example,
>>>> use-secure-monitor = <1>; look more appropriate in this case?
>>>
>>> Perhaps a silly question, but (provided there's only one per system, why
>>> can't the secure-monitor driver expose a function that you can call to get
>>> a reference to the system-monitor? I did something similar before with
>>> a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted,
>>> mpfs-rng is probed from software so it's slightly different to your
>>> case, but the principle is the same and it's not unheard of for code in
>>> drivers/soc to expose interfaces to other drivers like this. You can
>>> just call a function like that, and know whether there's a secure
>>> monitor, without having to retrofit a DT property.
>>
>> That could be an option. But again, nothing prevents me from searching 
>> for the secure-monitor node throughout the entire DT array.
>>
>> The question is more about something else, let me try to explain from 
>> the beginning:
>>
>> We currently have a soc driver that uses only the register to get basic 
>> information and it must be loaded early because other modules' behavior 
>> depends on its information.
> 
> Please provide name/link to the upstream source code (downstream does
> not matter).
> 
>> There is an option to supplement the register information with 
>> information from the secure-monitor.
>> For this, we had to write a new driver that uses the same register 
>> information as a fallback but can wait for the secure-monitor driver to 
>> load and add its information to soc.
>> It seemed logical to me to keep the DT structure the same and just add a 
>> reference to the secure-monitor (or as a second option, create a 
>> variable indicating support) for those SoCs that have been tested and 
>> can provide this information.
>> Not all Amlogic SoCs support this call, in some (mostly newer 
>> generations of SoCs), this call returns incorrect information and we and 
>> colleagues are still figuring out what has changed. But most established 
>> platforms support this.
>> We could add this information retrieval to the secure-monitor itself, 
>> but that would be a completely different story and would not constitute 
>> a soc driver.
>>
>> In the end, we need information about the support of the secure-monitor 
>> call for obtaining information for the soc driver. In my opinion, this 
>> can only be done by specifying it in the DT in specific files for 
>> Amlogic platforms: either by referencing the SM or by an option that 
>> allows checking the SM.
> 
> That's not the only option. This is SoC specific so can be deduced from
> the compatible as well. And this is kind of obvious from this patchset
> (actually patch 4): you add it per SoC.

BTW, that's one more DT maintainer (so the third) telling you property
is not needed yet. I think we used enough of our time here.

Best regards,
Krzysztof