Received-SPF: pass (google.com: domain of linux-kernel+bounces-146849-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 16 Apr 2024 08:07:03 -0500
From: Rob Herring <robh@kernel.org>
To: Peng Fan <peng.fan@nxp.com>
Cc: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
	Jonathan Corbet <corbet@lwn.net>, Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Cristian Marussi <cristian.marussi@arm.com>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"imx@lists.linux.dev" <imx@lists.linux.dev>,
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH v3 2/6] dt-bindings: firmware: add i.MX95 SCMI Extension
 protocol
Message-ID: <20240416130703.GA1992329-robh@kernel.org>
References: <20240412-imx95-bbm-misc-v2-v3-0-4380a4070980@nxp.com>
 <20240412-imx95-bbm-misc-v2-v3-2-4380a4070980@nxp.com>
 <20240412133400.GA2282201-robh@kernel.org>
 <PA4PR04MB941680A7FCBCDF926417F6A388042@PA4PR04MB9416.eurprd04.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PA4PR04MB941680A7FCBCDF926417F6A388042@PA4PR04MB9416.eurprd04.prod.outlook.com>

On Fri, Apr 12, 2024 at 01:50:37PM +0000, Peng Fan wrote:
> Hi Rob,
> 
> > Subject: Re: [PATCH v3 2/6] dt-bindings: firmware: add i.MX95 SCMI
> > Extension protocol
> > 
> > On Fri, Apr 12, 2024 at 06:47:08PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Add i.MX SCMI Extension protocols bindings for:
> > >  - Battery Backed Module(BBM) Protocol
> > >    This contains persistent storage (GPR), an RTC, and the ON/OFF button.
> > >    The protocol can also provide access to similar functions implemented via
> > >    external board components.
> > >  - MISC Protocol.
> > >    This includes controls that are misc settings/actions that must be exposed
> > >    from the SM to agents. They are device specific and are usually define to
> > >    access bit fields in various mix block control modules, IOMUX_GPR, and
> > other
> > >    GPR/CSR owned by the SM.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  .../devicetree/bindings/firmware/arm,scmi.yaml     | 21 +++++++++++++
> > >  .../bindings/firmware/nxp,imx95-scmi.yaml          | 36
> > ++++++++++++++++++++++
> > >  2 files changed, 57 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > index 93fb7d05f849..fa2cc910c485 100644
> > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > @@ -247,6 +247,27 @@ properties:
> > >        reg:
> > >          const: 0x18
> > >
> > > +  protocol@81:
> > > +    $ref: '#/$defs/protocol-node'
> > > +    unevaluatedProperties: false
> > > +
> > > +    properties:
> > > +      reg:
> > > +        const: 0x81
> > > +
> > > +  protocol@84:
> > > +    type: object
> > > +    anyOf:
> > > +      - allOf:
> > > +          - $ref: /schemas/firmware/nxp,imx95-scmi.yaml
> > > +          - $ref: '#/$defs/protocol-node'
> > 
> > If you put the ref under the protocol node, then it's 1 schema file per protocol
> > per vendor. Also, we then have to list every possible protocol node here, and
> > every one listed here will be valid for every vendor.
> > What we discussed is putting the list of vendor protocol schemas at the top-
> > level here and then the vendor schemas can list out all the protocol nodes.
> > 
> > Also, move "$ref: '#/$defs/protocol-node'" to nxp,imx95-scmi.yaml.
> 
> In arm,scmi.yaml top level, add below:
> +anyOf:
> +  - $ref: /schemas/firmware/nxp,imx95-scmi.yaml
> 
> And also add a protocol node:
>   protocol@84:                                                                                      
>     $ref: '#/$defs/protocol-node'                                                                   
>                                                                                                     
>     properties:                                                                                     
>       reg:                                                                                          
>         const: 0x84
> But here I not add unevaludatedProperties = false; otherwise the vendor
> yaml new properties will not work.

Just drop 'protocol@84' entirely here and change the top-level 
additionalProperties to unevaluatedProperties.

> 
> In nxp,imx95-scmi.yaml:
> properties:                                                                                         
>   protocol@84:                                                                                      
>     $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node'                                    
>     unevaluatedProperties: false                                                                    
>                                                                                                     
>     properties:                                                                                     
>       reg:                                                                                          
>         const: 0x84                                                                                 
>                                                                                                     
>       nxp,wakeup-sources:                                                                           
>         description:                                                                                
>           Each entry consists of 2 integers, represents the source and electric signal edge         
>         items:                                                                                      
>           items:                                                                                    
>             - description: the wakeup source                                                        
>             - description: the wakeup electric signal edge                                          

Constraints on the items values?

>         minItems: 1                                                                                 
>         maxItems: 32                                                                                
>         $ref: /schemas/types.yaml#/definitions/uint32-matrix                                        
>                                                                                                     
> additionalProperties: true
> 
> Are the upper looks good to you?

Yes, other than the one comment.

Rob