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

From: Rob Herring <robh@kernel.org>
To: Conor Dooley <conor@kernel.org>, Viacheslav <adeep@lexina.in>
Cc: Neil Armstrong <neil.armstrong@linaro.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-amlogic@lists.infradead.org,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property
Date: Thu, 13 Jun 2024 10:42:44 -0600	[thread overview]
Message-ID: <20240613164244.GA1999034-robh@kernel.org> (raw)
In-Reply-To: <20240611-undying-earthy-00236ac251aa@spud>

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? 

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.

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. 

Rob

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