On Mon, Jun 17, 2024 at 05:57:53PM +0100, Conor Dooley wrote: > 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? > > > > 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. Another thing, without having a driver expose an API, is calling of_find_compatible_node() to find the node. That also doesn't require retrofitting properties.
Attachment:
signature.asc
Description: PGP signature
