Message ID | 20230124222023.316089-1-robh@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp2405002wrn; Tue, 24 Jan 2023 14:34:30 -0800 (PST) X-Google-Smtp-Source: AMrXdXviPhfptWR2s/M6t3ZHXiDW+3pT82S51fxOB4Pzh7sLiFzqxHPxxdVC0i5am6uV/4kjHSQb X-Received: by 2002:a17:902:d888:b0:192:991f:d8e8 with SMTP id b8-20020a170902d88800b00192991fd8e8mr31242340plz.53.1674599669942; Tue, 24 Jan 2023 14:34:29 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1674599669; cv=none; d=google.com; s=arc-20160816; b=NcU8NX4UraQBHfnvZZgGyIO2eL8k/8uOx6RkdHVYYMxHcgnoP1/FgLOSRFkBlLEZcJ 1j43WMmMYYMzS7Keecp8BG/Sg4FpIjJkCErnAL+W7Ocsr3khwXwyXNgsVfZ7c+26Z4iX ahR3Em8Jz6mftj2WmR4rcZGPIXj4kY5OY/u4i7hTV3QLgkMo5CeA1cQOIs4rARM7fgqW FXH/ltXVaBOmtQ16jroYzJenyj4ZZjCZHblnrIni0/Zw8K3JlhrzGRmuXo6wooQRUd5j gk/yxaoC9HLmgXMSKCQjFRwswTlIVrjsWbD11XMWNl3YIIdlVTupwhIQwd8+nCW1eNmI pqQA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=Eez1m+PXfOfMtBkO1erQPzslo6stvsXK/McJNzS+bhE=; b=BufRc60UTEKA2T/Clhx15FyHFss/98Z9NM2bfxoQeFAGWtFeXBTdofgNdj7fyNg81j J99IT18k/kE1VY2YRuvapX39EZQ2vIcqJFHWM4D18QxhuZGF0Gw6/xCJ5fSIGbRHoAdC uj3OrR4y3O2MXGkAmKvqMv+7wL9gEZjsWWH+TVUR/gCk5mMYAJ1Rl+lNNms8CIHsV5ca BYXVXBFGy2GopS+uieHVT5oZQSBJhkGAgVadaaqzcqBxfkFpr1jsoHVXKSZ+OkTk55YU Ft6dekzigc6naUqAmtipW0Nh+Ayo3TZ0Pm7KrkVhc9nLlwb7TwH+kJDo6g2s8JIso0tg WSBA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id bi8-20020a170902bf0800b001893b7f5ed7si3442476plb.205.2023.01.24.14.33.52; Tue, 24 Jan 2023 14:34:29 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229798AbjAXWUh (ORCPT <rfc822;rust.linux@gmail.com> + 99 others); Tue, 24 Jan 2023 17:20:37 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51684 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231538AbjAXWUd (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 24 Jan 2023 17:20:33 -0500 Received: from mail-ot1-f46.google.com (mail-ot1-f46.google.com [209.85.210.46]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9F6DC37F2F; Tue, 24 Jan 2023 14:20:32 -0800 (PST) Received: by mail-ot1-f46.google.com with SMTP id g2-20020a9d6b02000000b006864bf5e658so10110446otp.1; Tue, 24 Jan 2023 14:20:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Eez1m+PXfOfMtBkO1erQPzslo6stvsXK/McJNzS+bhE=; b=Y0EqZxzKKFrunEnDMpbyNm9vqKMsPBH3WCDPHj9UnR35f1+8069rQgItBroZcZH5Zc gAdtnLWgyPNFCj/rLHCKlzoXoJZ9/eMT49qFr6Zvs/DauhYOYkYnEbRuafQILXsS9Stv ZjkeDFoafh/sLe8R78IDx4v8Y0sMb62vxDFhUnsw7do8Mx0+fqVE2kt+/ogmlt2AzKga q4Y+xYsdvkREY0dSayC2rBbZ0H7/GFhUpSjz1hwc4VA3qX28PiS58pB/5ojzYC93/NAS 33xzVijwY/vnQ+O7OfxrnDa0GL5S0A9+osdu2Efasqu8CM+69muvnpeZx7h16u5wAB/L Zotw== X-Gm-Message-State: AFqh2kobLE5Lb15AEnouuCj3VHum8ArM29u1yOYNSPRK/pacYyRbXPHw iOFVfJjmmOC7GFXcFGeO5A== X-Received: by 2002:a05:6830:244d:b0:675:410e:7533 with SMTP id x13-20020a056830244d00b00675410e7533mr16096069otr.9.1674598831857; Tue, 24 Jan 2023 14:20:31 -0800 (PST) Received: from robh_at_kernel.org (66-90-144-107.dyn.grandenetworks.net. [66.90.144.107]) by smtp.gmail.com with ESMTPSA id a18-20020a9d74d2000000b00684eaf9018csm1449579otl.34.2023.01.24.14.20.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 24 Jan 2023 14:20:31 -0800 (PST) Received: (nullmailer pid 316282 invoked by uid 1000); Tue, 24 Jan 2023 22:20:30 -0000 From: Rob Herring <robh@kernel.org> To: Sudeep Holla <sudeep.holla@arm.com>, Cristian Marussi <cristian.marussi@arm.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] dt-bindings: firmware: arm,scmi: Restrict protocol child node properties Date: Tue, 24 Jan 2023 16:20:23 -0600 Message-Id: <20230124222023.316089-1-robh@kernel.org> X-Mailer: git-send-email 2.39.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00, FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1755945023537900098?= X-GMAIL-MSGID: =?utf-8?q?1755945023537900098?= |
Series |
dt-bindings: firmware: arm,scmi: Restrict protocol child node properties
|
|
Commit Message
Rob Herring
Jan. 24, 2023, 10:20 p.m. UTC
The SCMI protocol child nodes are missing any constraints on unknown
properties. Specifically, either 'unevaluatedProperties' or
'additionalProperties' is needed. The current structure with a regex
match for all child nodes doesn't work for this purpose, so let's move
the common properties '$defs' entry which each specific protocol node
can reference and set 'unevaluatedProperties: false'.
Signed-off-by: Rob Herring <robh@kernel.org>
---
.../bindings/firmware/arm,scmi.yaml | 43 ++++++++++++++-----
1 file changed, 33 insertions(+), 10 deletions(-)
Comments
On Tue, Jan 24, 2023 at 04:20:23PM -0600, Rob Herring wrote: > The SCMI protocol child nodes are missing any constraints on unknown > properties. Specifically, either 'unevaluatedProperties' or > 'additionalProperties' is needed. The current structure with a regex > match for all child nodes doesn't work for this purpose, so let's move > the common properties '$defs' entry which each specific protocol node > can reference and set 'unevaluatedProperties: false'. > Makes sense to me. Also thanks for $defs example, wasn't aware of how to do that. Can you please take it though your tree ? Assuming that, Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep
On Tue, Jan 24, 2023 at 04:20:23PM -0600, Rob Herring wrote: > The SCMI protocol child nodes are missing any constraints on unknown > properties. Specifically, either 'unevaluatedProperties' or > 'additionalProperties' is needed. The current structure with a regex > match for all child nodes doesn't work for this purpose, so let's move > the common properties '$defs' entry which each specific protocol node > can reference and set 'unevaluatedProperties: false'. > Hi Rob, > Signed-off-by: Rob Herring <robh@kernel.org> > --- > .../bindings/firmware/arm,scmi.yaml | 43 ++++++++++++++----- > 1 file changed, 33 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > index 176796931a22..2f7c51c75e85 100644 > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > @@ -100,7 +100,9 @@ properties: > Channel specifier required when using OP-TEE transport. > > protocol@11: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x11 > @@ -112,7 +114,9 @@ properties: > - '#power-domain-cells' > > protocol@13: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x13 > @@ -124,7 +128,9 @@ properties: > - '#clock-cells' > > protocol@14: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x14 > @@ -136,7 +142,9 @@ properties: > - '#clock-cells' > > protocol@15: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x15 > @@ -148,7 +156,9 @@ properties: > - '#thermal-sensor-cells' > > protocol@16: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x16 > @@ -160,20 +170,31 @@ properties: > - '#reset-cells' > > protocol@17: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x17 > > regulators: > type: object > + additionalProperties: false > description: > The list of all regulators provided by this SCMI controller. > > + properties: > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > patternProperties: > - '^regulators@[0-9a-f]+$': > + '^regulator@[0-9a-f]+$': > type: object > $ref: "../regulator/regulator.yaml#" > + unevaluatedProperties: false > > properties: > reg: > @@ -184,15 +205,17 @@ properties: > - reg > > protocol@18: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x18 > > additionalProperties: false > > -patternProperties: > - '^protocol@[0-9a-f]+$': > +$defs: > + protocol-node: > type: object > description: so now that the catch-all protocol@ patternProperty is gone in favour of the 'protocol-node' definition and $refs, does that mean that any current and future SCMI officially published protocol <N> has to be added to the above explicit protocol list, even though it does not have any special additional required property beside reg ? (like protocol@18 above...) As an example SystemPower protocol@12 is not listed above too and it has nothing more than a reg=0x12 prop (liek 0x18), but before this patch was 'covered' by the patternProperty (so Krzysztof shot down, rightly, my recent attempt to add a distinct protocol@12 def), but now it does not seem anymore the case...so will we need to add an explicit protocol node for any future protocol addition ? (SCMI is extensible up to 255 protos..) Thanks, Cristian
On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > so now that the catch-all protocol@ patternProperty is gone in favour > of the 'protocol-node' definition and $refs, does that mean that any > current and future SCMI officially published protocol <N> has to be > added to the above explicit protocol list, even though it does not > have any special additional required property beside reg ? > (like protocol@18 above...) > If there are no consumers, should we just not add and deal with it entirely within the kernel. I know we rely today on presence of node before we initialise, but hey we have exception for system power protocol for other reasons, why not add this one too. In short we shouldn't have to add a node if there are no consumers. It was one of the topic of discussion initially when SCMI binding was added and they exist only for the consumers otherwise we don't need it as everything is discoverable from the interface. -- Regards, Sudeep
On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > so now that the catch-all protocol@ patternProperty is gone in favour > > of the 'protocol-node' definition and $refs, does that mean that any > > current and future SCMI officially published protocol <N> has to be > > added to the above explicit protocol list, even though it does not > > have any special additional required property beside reg ? > > (like protocol@18 above...) > > > > If there are no consumers, should we just not add and deal with it > entirely within the kernel. I know we rely today on presence of node > before we initialise, but hey we have exception for system power protocol > for other reasons, why not add this one too. > > In short we shouldn't have to add a node if there are no consumers. It > was one of the topic of discussion initially when SCMI binding was added > and they exist only for the consumers otherwise we don't need it as > everything is discoverable from the interface. > I'm fine with that, just wanted to understand/clarify the rule here. Thanks, Cristian
On Wed, Jan 25, 2023 at 8:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > so now that the catch-all protocol@ patternProperty is gone in favour > > of the 'protocol-node' definition and $refs, does that mean that any > > current and future SCMI officially published protocol <N> has to be > > added to the above explicit protocol list, even though it does not > > have any special additional required property beside reg ? > > (like protocol@18 above...) > > > > If there are no consumers, should we just not add and deal with it > entirely within the kernel. I know we rely today on presence of node > before we initialise, but hey we have exception for system power protocol > for other reasons, why not add this one too. > > In short we shouldn't have to add a node if there are no consumers. It > was one of the topic of discussion initially when SCMI binding was added > and they exist only for the consumers otherwise we don't need it as > everything is discoverable from the interface. As you might guess, I agree. We need to keep 0x18 I suppose, right? I assume it is already in use. Are there any others that didn't get documented? We'd need to keep them because old kernels would still need them. Rob
On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > so now that the catch-all protocol@ patternProperty is gone in favour > > of the 'protocol-node' definition and $refs, does that mean that any > > current and future SCMI officially published protocol <N> has to be > > added to the above explicit protocol list, even though it does not > > have any special additional required property beside reg ? > > (like protocol@18 above...) > > > > If there are no consumers, should we just not add and deal with it > entirely within the kernel. I know we rely today on presence of node > before we initialise, but hey we have exception for system power protocol > for other reasons, why not add this one too. > > In short we shouldn't have to add a node if there are no consumers. It > was one of the topic of discussion initially when SCMI binding was added > and they exist only for the consumers otherwise we don't need it as > everything is discoverable from the interface. It is fine for me the no-consumers/no-node argument (which anyway would require a few changes in the core init logic anyway to work this way...), BUT is it not that ANY protocol (even future-ones) does have, potentially, consumers indeed, since each protocol-node can potentially have a dedicated channel and related DT channel-descriptor ? (when multiple channels are allowed by the transport) I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for anything (if we patch the core init as said) UNLESS you want to dedicate a channel to those protocols; so I'm just checking here if these kind of scenarios will still be allowed with this binding change, or if I am missing something. Thanks, Cristian
On Thu, Jan 26, 2023 at 09:43:44AM +0000, Cristian Marussi wrote: > On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > > so now that the catch-all protocol@ patternProperty is gone in favour > > > of the 'protocol-node' definition and $refs, does that mean that any > > > current and future SCMI officially published protocol <N> has to be > > > added to the above explicit protocol list, even though it does not > > > have any special additional required property beside reg ? > > > (like protocol@18 above...) > > > > > > > If there are no consumers, should we just not add and deal with it > > entirely within the kernel. I know we rely today on presence of node > > before we initialise, but hey we have exception for system power protocol > > for other reasons, why not add this one too. > > > > In short we shouldn't have to add a node if there are no consumers. It > > was one of the topic of discussion initially when SCMI binding was added > > and they exist only for the consumers otherwise we don't need it as > > everything is discoverable from the interface. > > It is fine for me the no-consumers/no-node argument (which anyway would > require a few changes in the core init logic anyway to work this way...), > BUT is it not that ANY protocol (even future-ones) does have, potentially, > consumers indeed, since each protocol-node can potentially have a dedicated > channel and related DT channel-descriptor ? (when multiple channels are > allowed by the transport) > > I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for > anything (if we patch the core init as said) UNLESS you want to dedicate > a channel to those protocols; so I'm just checking here if these kind of > scenarios will still be allowed with this binding change, or if I am > missing something. Ah, good point on the transport information. Yes we will need a node if a protocol has a dedicated transport. No one has used so far other than Juno perf, but we never know. We can always extended the bindings if needed. Sorry for missing the dedicated transport part.
On Thu, Jan 26, 2023 at 8:46 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Jan 26, 2023 at 09:43:44AM +0000, Cristian Marussi wrote: > > On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > > > so now that the catch-all protocol@ patternProperty is gone in favour > > > > of the 'protocol-node' definition and $refs, does that mean that any > > > > current and future SCMI officially published protocol <N> has to be > > > > added to the above explicit protocol list, even though it does not > > > > have any special additional required property beside reg ? > > > > (like protocol@18 above...) > > > > > > > > > > If there are no consumers, should we just not add and deal with it > > > entirely within the kernel. I know we rely today on presence of node > > > before we initialise, but hey we have exception for system power protocol > > > for other reasons, why not add this one too. > > > > > > In short we shouldn't have to add a node if there are no consumers. It > > > was one of the topic of discussion initially when SCMI binding was added > > > and they exist only for the consumers otherwise we don't need it as > > > everything is discoverable from the interface. > > > > It is fine for me the no-consumers/no-node argument (which anyway would > > require a few changes in the core init logic anyway to work this way...), > > BUT is it not that ANY protocol (even future-ones) does have, potentially, > > consumers indeed, since each protocol-node can potentially have a dedicated > > channel and related DT channel-descriptor ? (when multiple channels are > > allowed by the transport) > > > > I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for > > anything (if we patch the core init as said) UNLESS you want to dedicate > > a channel to those protocols; so I'm just checking here if these kind of > > scenarios will still be allowed with this binding change, or if I am > > missing something. > > Ah, good point on the transport information. Yes we will need a node if > a protocol has a dedicated transport. No one has used so far other than > Juno perf, but we never know. We can always extended the bindings if > needed. > > Sorry for missing the dedicated transport part. So I need to add back 'protocol@.*' or not? Rob
On Thu, Jan 26, 2023 at 09:25:12AM -0600, Rob Herring wrote: > On Thu, Jan 26, 2023 at 8:46 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Thu, Jan 26, 2023 at 09:43:44AM +0000, Cristian Marussi wrote: > > > On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > > > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > > > > so now that the catch-all protocol@ patternProperty is gone in favour > > > > > of the 'protocol-node' definition and $refs, does that mean that any > > > > > current and future SCMI officially published protocol <N> has to be > > > > > added to the above explicit protocol list, even though it does not > > > > > have any special additional required property beside reg ? > > > > > (like protocol@18 above...) > > > > > > > > > > > > > If there are no consumers, should we just not add and deal with it > > > > entirely within the kernel. I know we rely today on presence of node > > > > before we initialise, but hey we have exception for system power protocol > > > > for other reasons, why not add this one too. > > > > > > > > In short we shouldn't have to add a node if there are no consumers. It > > > > was one of the topic of discussion initially when SCMI binding was added > > > > and they exist only for the consumers otherwise we don't need it as > > > > everything is discoverable from the interface. > > > > > > It is fine for me the no-consumers/no-node argument (which anyway would > > > require a few changes in the core init logic anyway to work this way...), > > > BUT is it not that ANY protocol (even future-ones) does have, potentially, > > > consumers indeed, since each protocol-node can potentially have a dedicated > > > channel and related DT channel-descriptor ? (when multiple channels are > > > allowed by the transport) > > > > > > I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for > > > anything (if we patch the core init as said) UNLESS you want to dedicate > > > a channel to those protocols; so I'm just checking here if these kind of > > > scenarios will still be allowed with this binding change, or if I am > > > missing something. > > > > Ah, good point on the transport information. Yes we will need a node if > > a protocol has a dedicated transport. No one has used so far other than > > Juno perf, but we never know. We can always extended the bindings if > > needed. > > > > Sorry for missing the dedicated transport part. > > So I need to add back 'protocol@.*' or not? IMO it is better to know what exactly gets added under each of these protocol sub-nodes and so better to have entry specific to each known protocols. I liked that fact with this change as I have seen some crazy vendor extensions adding all sorts of non-sense defining some vendor protocol. For example [1], in which case we can catch those better than existing schema which matches all. So let's not add protocol@.* if possible or until that becomes the only cleaner way to maintain this. -- Regards, Sudeep [1] https://lore.kernel.org/all/1667451512-9655-2-git-send-email-quic_sibis@quicinc.com/
On Thu, Jan 26, 2023 at 11:04 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Jan 26, 2023 at 09:25:12AM -0600, Rob Herring wrote: > > On Thu, Jan 26, 2023 at 8:46 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Thu, Jan 26, 2023 at 09:43:44AM +0000, Cristian Marussi wrote: > > > > On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > > > > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > > > > > so now that the catch-all protocol@ patternProperty is gone in favour > > > > > > of the 'protocol-node' definition and $refs, does that mean that any > > > > > > current and future SCMI officially published protocol <N> has to be > > > > > > added to the above explicit protocol list, even though it does not > > > > > > have any special additional required property beside reg ? > > > > > > (like protocol@18 above...) > > > > > > > > > > > > > > > > If there are no consumers, should we just not add and deal with it > > > > > entirely within the kernel. I know we rely today on presence of node > > > > > before we initialise, but hey we have exception for system power protocol > > > > > for other reasons, why not add this one too. > > > > > > > > > > In short we shouldn't have to add a node if there are no consumers. It > > > > > was one of the topic of discussion initially when SCMI binding was added > > > > > and they exist only for the consumers otherwise we don't need it as > > > > > everything is discoverable from the interface. > > > > > > > > It is fine for me the no-consumers/no-node argument (which anyway would > > > > require a few changes in the core init logic anyway to work this way...), > > > > BUT is it not that ANY protocol (even future-ones) does have, potentially, > > > > consumers indeed, since each protocol-node can potentially have a dedicated > > > > channel and related DT channel-descriptor ? (when multiple channels are > > > > allowed by the transport) > > > > > > > > I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for > > > > anything (if we patch the core init as said) UNLESS you want to dedicate > > > > a channel to those protocols; so I'm just checking here if these kind of > > > > scenarios will still be allowed with this binding change, or if I am > > > > missing something. > > > > > > Ah, good point on the transport information. Yes we will need a node if > > > a protocol has a dedicated transport. No one has used so far other than > > > Juno perf, but we never know. We can always extended the bindings if > > > needed. > > > > > > Sorry for missing the dedicated transport part. > > > > So I need to add back 'protocol@.*' or not? > > IMO it is better to know what exactly gets added under each of these protocol > sub-nodes and so better to have entry specific to each known protocols. I > liked that fact with this change as I have seen some crazy vendor extensions > adding all sorts of non-sense defining some vendor protocol. For example [1], > in which case we can catch those better than existing schema which matches > all. So let's not add protocol@.* if possible or until that becomes the only > cleaner way to maintain this. TBC, 'protocol@.*' would not allow anything but the properties defined in the /$defs/protocol-node. So [1] would throw errors without a schema addition. We should either do that along with dropping 'protocol@18' or we keep protocol 0x18 node and add all other providerless protocols. I don't think we need the latter to just check unit-address vs. reg. I want to come up with a better way to do that (dtc does some, but only for defined bus types). Rob
Hi Rob, On Fri, Jan 27, 2023 at 12:52:33PM -0600, Rob Herring wrote: > > TBC, 'protocol@.*' would not allow anything but the properties defined > in the /$defs/protocol-node. So [1] would throw errors without a > schema addition. Right I clearly missed that, somehow I assumed it would allow. > We should either do that along with dropping 'protocol@18' or we keep > protocol 0x18 node and add all other providerless protocols. I don't > think we need the latter to just check unit-address vs. reg. I only argument today it to allow protocol specific transport. So we could delay addition of it until someone needs that way. So far we haven't seen anyone using it other than performance(even that is not needed with the introduction of fast channels that are auto discoverable in relatively newer versions of the spec). -- Regards, Sudeep
On Mon, Feb 6, 2023 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > Hi Rob, > > On Fri, Jan 27, 2023 at 12:52:33PM -0600, Rob Herring wrote: > > > > TBC, 'protocol@.*' would not allow anything but the properties defined > > in the /$defs/protocol-node. So [1] would throw errors without a > > schema addition. > > Right I clearly missed that, somehow I assumed it would allow. > > > We should either do that along with dropping 'protocol@18' or we keep > > protocol 0x18 node and add all other providerless protocols. I don't > > think we need the latter to just check unit-address vs. reg. > > I only argument today it to allow protocol specific transport. So we could > delay addition of it until someone needs that way. So far we haven't seen > anyone using it other than performance(even that is not needed with the > introduction of fast channels that are auto discoverable in relatively > newer versions of the spec). I failed to think about 'protocol@.*' would match on every protocol, so we have to list them explicitly: '^protocol@(18|xx|yy|zz)$' Anyways, I think the conclusion is the patch should stay as-is and so I've applied it. Rob
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml index 176796931a22..2f7c51c75e85 100644 --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml @@ -100,7 +100,9 @@ properties: Channel specifier required when using OP-TEE transport. protocol@11: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x11 @@ -112,7 +114,9 @@ properties: - '#power-domain-cells' protocol@13: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x13 @@ -124,7 +128,9 @@ properties: - '#clock-cells' protocol@14: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x14 @@ -136,7 +142,9 @@ properties: - '#clock-cells' protocol@15: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x15 @@ -148,7 +156,9 @@ properties: - '#thermal-sensor-cells' protocol@16: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x16 @@ -160,20 +170,31 @@ properties: - '#reset-cells' protocol@17: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x17 regulators: type: object + additionalProperties: false description: The list of all regulators provided by this SCMI controller. + properties: + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + patternProperties: - '^regulators@[0-9a-f]+$': + '^regulator@[0-9a-f]+$': type: object $ref: "../regulator/regulator.yaml#" + unevaluatedProperties: false properties: reg: @@ -184,15 +205,17 @@ properties: - reg protocol@18: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x18 additionalProperties: false -patternProperties: - '^protocol@[0-9a-f]+$': +$defs: + protocol-node: type: object description: Each sub-node represents a protocol supported. If the platform