Date: Mon, 17 Jun 2024 11:21:30 +0300
From: Viacheslav <adeep@...ina.in>
To: Rob Herring <robh@...nel.org>, Conor Dooley <conor@...nel.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>,
 Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH v5 3/4] dt-bindings: arm: amlogic:
 amlogic,meson-gx-ao-secure: add secure-monitor property

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.


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


> 
> Rob
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Viacheslav

Powered by blists - more mailing lists