Message ID | 20221027181337.8651-3-afd@ti.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp386596wru; Thu, 27 Oct 2022 11:18:35 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6WVDvLMfsui6fCfoIBo6HIKUBKc4mmmveBcxpkWbRjI72v0MHM9IOPEu1tBzycpArWFQ6o X-Received: by 2002:a17:902:edc3:b0:172:8ae3:9778 with SMTP id q3-20020a170902edc300b001728ae39778mr50595313plk.72.1666894715289; Thu, 27 Oct 2022 11:18:35 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666894715; cv=none; d=google.com; s=arc-20160816; b=gqrCqO+kf+Y61xIldIFYBFvd1/N++13VORhWIqY586fPCbG6ihlqKEXxfm/5a8CBFC OnNsjkUGwDt2fNjujMraRQrJ1cr3fsVrLceGk7NSZDTl4EGgGMkXDLppJqbyF55d1Sqr U5xRFMrYiHjhoUk6CH3dfGSDw2sOJgHYmJB3p9BYeITugMyHO6Q5ykkkpJ64DTv4siZh N574hiIOj00UGm9MhRSfiBXk4qKFbxnLlI8MqfQDWfCiOyt0S9cvXVQ7nfmurzT/EjGe BL1z6XG29J70IrwUj52/68wVOkTAJqdhtMwC+MkRVZc8NsF0XomUtXOb9bWoVJN9FRkn Y7Jw== 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 :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=mWERJQ6t/4WOSKqWsUHkMFt1G1PJE7LkTv+/r52Wyjs=; b=pYNT4Umt2bnC3Yz/zJjWs8Q4S7N57Kl9W6stIZR3Om+EBdFaozPxfR0qp/oAaXSTW5 /UpbINBGwkZRjD8dn41TuFhVVSObUOqUNpMEGmKEVH1wsx1Y5PAY7rt57YcHM/vE/0mM /KAuXfgBqsJwNwDjs34UBWJ9CWGWp7IJHMk5qvnYTBwvutdc8OsvXayI0TsnzQqwCqvE s4sRJaLJikcY6/1dNsIpWbWLcdBQCb6Bupr0pHwGUB68xvwTJ549tpaIp0aB1hJhYhqC bVh+HCXtCSTllAX+TrGt+JKc67/bxAPrHX8XHUUBHZF7KDkKyAeGP5b+/L4rxfk031TF qGHQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=Vst2Zndh; 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=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j7-20020a17090a694700b00213213d63bbsi2418019pjm.41.2022.10.27.11.18.22; Thu, 27 Oct 2022 11:18:35 -0700 (PDT) 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; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=Vst2Zndh; 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=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236212AbiJ0SOE (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Thu, 27 Oct 2022 14:14:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46924 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236262AbiJ0SOA (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 27 Oct 2022 14:14:00 -0400 Received: from fllv0016.ext.ti.com (fllv0016.ext.ti.com [198.47.19.142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 47261EE28; Thu, 27 Oct 2022 11:13:59 -0700 (PDT) Received: from fllv0034.itg.ti.com ([10.64.40.246]) by fllv0016.ext.ti.com (8.15.2/8.15.2) with ESMTP id 29RIDdIm037309; Thu, 27 Oct 2022 13:13:39 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1666894419; bh=mWERJQ6t/4WOSKqWsUHkMFt1G1PJE7LkTv+/r52Wyjs=; h=From:To:CC:Subject:Date:In-Reply-To:References; b=Vst2Zndh/kWD9oFOwuqjbX336JCSKrFncfKBo9TcZD6C/IRWVThwdheJuMCPa66GV rZTgEd+6AP5iO9BLRmTnKHqtJDlTxqcdA9eHsqXrxR+lOO4wl+UkQk0WdWL99jnE0d A1aVQXeOfJzAhaLAYxrXU4vLnzSwZIMom+qL7PFQ= Received: from DFLE114.ent.ti.com (dfle114.ent.ti.com [10.64.6.35]) by fllv0034.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 29RIDdpc065679 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 27 Oct 2022 13:13:39 -0500 Received: from DFLE107.ent.ti.com (10.64.6.28) by DFLE114.ent.ti.com (10.64.6.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.6; Thu, 27 Oct 2022 13:13:39 -0500 Received: from lelv0327.itg.ti.com (10.180.67.183) by DFLE107.ent.ti.com (10.64.6.28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.6 via Frontend Transport; Thu, 27 Oct 2022 13:13:39 -0500 Received: from ula0226330.dal.design.ti.com (ileaxei01-snat2.itg.ti.com [10.180.69.6]) by lelv0327.itg.ti.com (8.15.2/8.15.2) with ESMTP id 29RIDbdw108447; Thu, 27 Oct 2022 13:13:38 -0500 From: Andrew Davis <afd@ti.com> To: Lee Jones <lee@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Arnd Bergmann <arnd@arndb.de>, Linus Walleij <linus.walleij@linaro.org>, Geert Uytterhoeven <geert+renesas@glider.be>, Daniel Tang <dt.tangr@gmail.com>, Fabian Vogt <fabian@ritter-vogt.de> CC: <devicetree@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, Andrew Davis <afd@ti.com> Subject: [PATCH v3 2/9] ARM: dts: nspire: Use syscon-reboot to handle restart Date: Thu, 27 Oct 2022 13:13:30 -0500 Message-ID: <20221027181337.8651-3-afd@ti.com> X-Mailer: git-send-email 2.37.3 In-Reply-To: <20221027181337.8651-1-afd@ti.com> References: <20221027181337.8651-1-afd@ti.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham 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?1747865792635142016?= X-GMAIL-MSGID: =?utf-8?q?1747865792635142016?= |
Series |
TI-Nspire cleanups
|
|
Commit Message
Andrew Davis
Oct. 27, 2022, 6:13 p.m. UTC
Writing this bit can be handled by the syscon-reboot driver. Add this node to DT. Signed-off-by: Andrew Davis <afd@ti.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Tested-by: Fabian Vogt <fabian@ritter-vogt.de> Reviewed-by: Fabian Vogt <fabian@ritter-vogt.de> --- arch/arm/boot/dts/nspire.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
Comments
On 27/10/2022 14:13, Andrew Davis wrote: > Writing this bit can be handled by the syscon-reboot driver. > Add this node to DT. > > Signed-off-by: Andrew Davis <afd@ti.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Tested-by: Fabian Vogt <fabian@ritter-vogt.de> > Reviewed-by: Fabian Vogt <fabian@ritter-vogt.de> > --- > arch/arm/boot/dts/nspire.dtsi | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm/boot/dts/nspire.dtsi b/arch/arm/boot/dts/nspire.dtsi > index bb240e6a3a6f..48fbc9d533c3 100644 > --- a/arch/arm/boot/dts/nspire.dtsi > +++ b/arch/arm/boot/dts/nspire.dtsi > @@ -172,7 +172,14 @@ rtc: rtc@90090000 { > }; > > misc: misc@900a0000 { > + compatible = "ti,nspire-misc", "syscon", "simple-mfd"; You have syscon and simple-mfd, but bindings in patch #1 say only syscon. Best regards, Krzysztof
On 10/27/22 2:33 PM, Krzysztof Kozlowski wrote: > On 27/10/2022 14:13, Andrew Davis wrote: >> Writing this bit can be handled by the syscon-reboot driver. >> Add this node to DT. >> >> Signed-off-by: Andrew Davis <afd@ti.com> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> Tested-by: Fabian Vogt <fabian@ritter-vogt.de> >> Reviewed-by: Fabian Vogt <fabian@ritter-vogt.de> >> --- >> arch/arm/boot/dts/nspire.dtsi | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/arch/arm/boot/dts/nspire.dtsi b/arch/arm/boot/dts/nspire.dtsi >> index bb240e6a3a6f..48fbc9d533c3 100644 >> --- a/arch/arm/boot/dts/nspire.dtsi >> +++ b/arch/arm/boot/dts/nspire.dtsi >> @@ -172,7 +172,14 @@ rtc: rtc@90090000 { >> }; >> >> misc: misc@900a0000 { >> + compatible = "ti,nspire-misc", "syscon", "simple-mfd"; > > You have syscon and simple-mfd, but bindings in patch #1 say only syscon. > I'm not following, are you just saying my wording in the patch message just wasn't complete? Or are you saying something more about nodes that are both syscon and simple-mfd? In that case, having both syscon and simple-mfd seems rather common, looks like you added the rule for it[0]. Thinking on this, they almost represent the same thing. simple-mfd says "my child nodes should be considered devices", why do we need that? Couldn't we simply state that "syscon" node's children are always devices, I mean what else could they be, syscon is an MFD after all (and lives in drivers/mfd/). "syscon" often just says, others can use the registers within this node, so as a different option, make "syscon" a property of "simple-mfd" nodes. I'm seeing all these examples of devices that should have been children of the "syscon" device, but instead use regmap = <&x>; syscon = <&x>; or similar and put the device node out somewhere random. And in those cases, wouldn't it have been more correct to use the normal "reg" and "regions" to define the registers belonging to the child node/device?.. Thanks, Andrew [0] https://lore.kernel.org/all/20220817142246.828762-5-krzysztof.kozlowski@linaro.org/ > > Best regards, > Krzysztof >
On 27/10/2022 17:07, Andrew Davis wrote: > On 10/27/22 2:33 PM, Krzysztof Kozlowski wrote: >> On 27/10/2022 14:13, Andrew Davis wrote: >>> Writing this bit can be handled by the syscon-reboot driver. >>> Add this node to DT. >>> >>> Signed-off-by: Andrew Davis <afd@ti.com> >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >>> Tested-by: Fabian Vogt <fabian@ritter-vogt.de> >>> Reviewed-by: Fabian Vogt <fabian@ritter-vogt.de> >>> --- >>> arch/arm/boot/dts/nspire.dtsi | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/nspire.dtsi b/arch/arm/boot/dts/nspire.dtsi >>> index bb240e6a3a6f..48fbc9d533c3 100644 >>> --- a/arch/arm/boot/dts/nspire.dtsi >>> +++ b/arch/arm/boot/dts/nspire.dtsi >>> @@ -172,7 +172,14 @@ rtc: rtc@90090000 { >>> }; >>> >>> misc: misc@900a0000 { >>> + compatible = "ti,nspire-misc", "syscon", "simple-mfd"; >> >> You have syscon and simple-mfd, but bindings in patch #1 say only syscon. >> > > I'm not following, are you just saying my wording in the patch message just > wasn't complete? Your binding patch adds nspire compatible to the list of two items, so you have two items in total - nspire followed by syscon. What you implemented here is different. > > Or are you saying something more about nodes that are both syscon and simple-mfd? > In that case, having both syscon and simple-mfd seems rather common, looks like > you added the rule for it[0]. > > Thinking on this, they almost represent the same thing. simple-mfd says "my child > nodes should be considered devices", why do we need that? Couldn't we simply state > that "syscon" node's children are always devices, I mean what else could they be, > syscon is an MFD after all (and lives in drivers/mfd/). No, syscon is not an MFD. Syscon means system controller and alone it does not have children. > > "syscon" often just says, others can use the registers within this node, so as a > different option, make "syscon" a property of "simple-mfd" nodes. I'm seeing all > these examples of devices that should have been children of the "syscon" device, > but instead use > > regmap = <&x>; > syscon = <&x>; > > or similar and put the device node out somewhere random. And in those cases, > wouldn't it have been more correct to use the normal "reg" and "regions" to > define the registers belonging to the child node/device?.. Sorry, I do not follow. How this is even related to your patch? Your bindings say A, DTS say B. A != B. This needs fixing. Unless you are asking me what your device is in general. This I don't really know, but if you want to use it as regmap provider for system registers and as a parent of syscon-based reboot device, then your device is syscon and simple-mfd. With a specific compatible. Was this your question? Best regards, Krzysztof
On 10/27/22 4:27 PM, Krzysztof Kozlowski wrote: > On 27/10/2022 17:07, Andrew Davis wrote: >> On 10/27/22 2:33 PM, Krzysztof Kozlowski wrote: >>> On 27/10/2022 14:13, Andrew Davis wrote: >>>> Writing this bit can be handled by the syscon-reboot driver. >>>> Add this node to DT. >>>> >>>> Signed-off-by: Andrew Davis <afd@ti.com> >>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >>>> Tested-by: Fabian Vogt <fabian@ritter-vogt.de> >>>> Reviewed-by: Fabian Vogt <fabian@ritter-vogt.de> >>>> --- >>>> arch/arm/boot/dts/nspire.dtsi | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/nspire.dtsi b/arch/arm/boot/dts/nspire.dtsi >>>> index bb240e6a3a6f..48fbc9d533c3 100644 >>>> --- a/arch/arm/boot/dts/nspire.dtsi >>>> +++ b/arch/arm/boot/dts/nspire.dtsi >>>> @@ -172,7 +172,14 @@ rtc: rtc@90090000 { >>>> }; >>>> >>>> misc: misc@900a0000 { >>>> + compatible = "ti,nspire-misc", "syscon", "simple-mfd"; >>> >>> You have syscon and simple-mfd, but bindings in patch #1 say only syscon. >>> >> >> I'm not following, are you just saying my wording in the patch message just >> wasn't complete? > > Your binding patch adds nspire compatible to the list of two items, so > you have two items in total - nspire followed by syscon. > > What you implemented here is different. > Is there a list of three items I can add this compatible? If instead you mean I should go make a new binding, just say so :) >> >> Or are you saying something more about nodes that are both syscon and simple-mfd? >> In that case, having both syscon and simple-mfd seems rather common, looks like >> you added the rule for it[0]. >> >> Thinking on this, they almost represent the same thing. simple-mfd says "my child >> nodes should be considered devices", why do we need that? Couldn't we simply state >> that "syscon" node's children are always devices, I mean what else could they be, >> syscon is an MFD after all (and lives in drivers/mfd/). > > No, syscon is not an MFD. Syscon means system controller and alone it > does not have children. > The binding lives in devicetree/bindings/*mfd*/, it is mentioned as one in devicetree/bindings/mfd/mfd.txt. If it is not an MFD then the bindings are giving out mixed signals here.. >> >> "syscon" often just says, others can use the registers within this node, so as a >> different option, make "syscon" a property of "simple-mfd" nodes. I'm seeing all >> these examples of devices that should have been children of the "syscon" device, >> but instead use >> >> regmap = <&x>; >> syscon = <&x>; >> >> or similar and put the device node out somewhere random. And in those cases, >> wouldn't it have been more correct to use the normal "reg" and "regions" to >> define the registers belonging to the child node/device?.. > > Sorry, I do not follow. How this is even related to your patch? > > Your bindings say A, DTS say B. A != B. This needs fixing. > I said it was compatible with "syscon", not that it is incompatible with "simple-mfd" devices. What I've done here gives no dtbs_check warnings and "devicetree/bindings/mfd/mfd.txt" explicitly allows what I am doing. Unless we do not consider the old bindings valid? If so, would you like me to convert mfd.txt to yaml, just let me know. > Unless you are asking me what your device is in general. This I don't > really know, but if you want to use it as regmap provider for system > registers and as a parent of syscon-based reboot device, then your > device is syscon and simple-mfd. With a specific compatible. Was this > your question? > Yes, I would like to use it as a regmap provider, my question here is a much more general one: why do I need to specify that in device tree? That is not a hardware description, my hardware is not "regmap" hardware. This "syscon" stuff feels like a bodge to make the Linux drivers and bus frameworks interact the way we want. I know at this point this has little to do with this series, but I'd like to just think this out for a moment. The latest Devicetree Specification talks about "simple-bus" as a special compatible that communicates that child nodes with compatible strings need probed also. ("simple-mfd" seems to be used the same way but without needing a "ranges" property..) Both of these are properties of a node, not something a device is "compatible" with. "compatibles" are also supposed to be listed "from most specific to most general", so which is more specific, "simple-mfd" or "syscon", etc.. Seems like Rob might agree[0], these are not really compatibles. We cant fix history, but for new nodes, instead of growing the problem and forcing these to be overloaded compatibles, we allow these to become new standard node properties. For instance: main_conf: syscon@43000000 { compatible = "ti,j721e-system-controller"; reg = <0x0 0x43000000 0x0 0x20000>; simple-bus; syscon; ... }; Thoughts? Thanks, Andrew [0] https://lore.kernel.org/all/CAL_JsqKiUcO76bo1GoepWM1TusJWoty_BRy2hFSgtEVMqtrvvQ@mail.gmail.com/ > Best regards, > Krzysztof >
On Mon, Oct 31, 2022 at 09:30:45AM -0500, Andrew Davis wrote: > On 10/27/22 4:27 PM, Krzysztof Kozlowski wrote: > > On 27/10/2022 17:07, Andrew Davis wrote: > > > On 10/27/22 2:33 PM, Krzysztof Kozlowski wrote: > > > > On 27/10/2022 14:13, Andrew Davis wrote: > > > > > Writing this bit can be handled by the syscon-reboot driver. > > > > > Add this node to DT. > > > > > > > > > > Signed-off-by: Andrew Davis <afd@ti.com> > > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > > Tested-by: Fabian Vogt <fabian@ritter-vogt.de> > > > > > Reviewed-by: Fabian Vogt <fabian@ritter-vogt.de> > > > > > --- > > > > > arch/arm/boot/dts/nspire.dtsi | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/arch/arm/boot/dts/nspire.dtsi b/arch/arm/boot/dts/nspire.dtsi > > > > > index bb240e6a3a6f..48fbc9d533c3 100644 > > > > > --- a/arch/arm/boot/dts/nspire.dtsi > > > > > +++ b/arch/arm/boot/dts/nspire.dtsi > > > > > @@ -172,7 +172,14 @@ rtc: rtc@90090000 { > > > > > }; > > > > > misc: misc@900a0000 { > > > > > + compatible = "ti,nspire-misc", "syscon", "simple-mfd"; > > > > > > > > You have syscon and simple-mfd, but bindings in patch #1 say only syscon. > > > > > > > > > > I'm not following, are you just saying my wording in the patch message just > > > wasn't complete? > > > > Your binding patch adds nspire compatible to the list of two items, so > > you have two items in total - nspire followed by syscon. > > > > What you implemented here is different. > > > > Is there a list of three items I can add this compatible? If instead you > mean I should go make a new binding, just say so :) An MFD should define its own schema file. The original intent of syscon.yaml was for just single nodes with 'syscon' (and a specific compatible). Adding in simple-mfd was probably a mistake. Certainly we need to rework the schema as you should get a warning in this case. > > > Or are you saying something more about nodes that are both syscon and simple-mfd? > > > In that case, having both syscon and simple-mfd seems rather common, looks like > > > you added the rule for it[0]. > > > > > > Thinking on this, they almost represent the same thing. simple-mfd says "my child > > > nodes should be considered devices", why do we need that? Couldn't we simply state > > > that "syscon" node's children are always devices, I mean what else could they be, > > > syscon is an MFD after all (and lives in drivers/mfd/). > > > > No, syscon is not an MFD. Syscon means system controller and alone it > > does not have children. > > > > The binding lives in devicetree/bindings/*mfd*/, it is mentioned as one > in devicetree/bindings/mfd/mfd.txt. If it is not an MFD then the bindings > are giving out mixed signals here.. > > > > > > > "syscon" often just says, others can use the registers within this node, so as a > > > different option, make "syscon" a property of "simple-mfd" nodes. I'm seeing all > > > these examples of devices that should have been children of the "syscon" device, > > > but instead use > > > > > > regmap = <&x>; > > > syscon = <&x>; > > > > > > or similar and put the device node out somewhere random. And in those cases, > > > wouldn't it have been more correct to use the normal "reg" and "regions" to > > > define the registers belonging to the child node/device?.. > > > > Sorry, I do not follow. How this is even related to your patch? > > > > Your bindings say A, DTS say B. A != B. This needs fixing. > > > > I said it was compatible with "syscon", not that it is incompatible > with "simple-mfd" devices. > > What I've done here gives no dtbs_check warnings and > "devicetree/bindings/mfd/mfd.txt" explicitly allows what I am doing. > Unless we do not consider the old bindings valid? Only that the example is not because it doesn't have a specific compatible. What needs to be clarified is that MFDs must define all the child nodes whether they are 'simple' or not. > If so, would you > like me to convert mfd.txt to yaml, just let me know. No, because I don't think there is anything to define as a schema. > > Unless you are asking me what your device is in general. This I don't > > really know, but if you want to use it as regmap provider for system > > registers and as a parent of syscon-based reboot device, then your > > device is syscon and simple-mfd. With a specific compatible. Was this > > your question? > > > > Yes, I would like to use it as a regmap provider, my question here is > a much more general one: why do I need to specify that in device tree? > That is not a hardware description, my hardware is not "regmap" hardware. > This "syscon" stuff feels like a bodge to make the Linux drivers and bus > frameworks interact the way we want. Bingo! It's a hint for create a regmap. We could just have a compatible list in the kernel for compatibles needing a regmap. Maybe that list would be too long though. So call it h/w description for this h/w is referenced by other places. > I know at this point this has little to do with this series, but I'd like > to just think this out for a moment. The latest Devicetree Specification > talks about "simple-bus" as a special compatible that communicates that > child nodes with compatible strings need probed also. ("simple-mfd" seems > to be used the same way but without needing a "ranges" property..) Yes, both cases are saying there is no dependency or setup of the parent needs. If the child nodes depend on the regmap, then it's not a 'simple-mfd' IMO. Therefore 'syscon' together with 'simple-mfd' is wrong unless it's other nodes that need the regmap. The schema can't really check that. > Both of these are properties of a node, not something a device is "compatible" > with. "compatibles" are also supposed to be listed "from most specific to > most general", so which is more specific, "simple-mfd" or "syscon", etc.. I would say 'syscon' is more specific if I have to pick. It implies some registers exist. 'simple-mfd' should mean there are no parent resources (...the children depend on). We've probably got enough of a mixture of the order, it wouldn't be worth the effort to try to enforce the order here. > Seems like Rob might agree[0], these are not really compatibles. We cant fix > history, but for new nodes, instead of growing the problem and forcing these to > be overloaded compatibles, we allow these to become new standard node properties. > > For instance: > > main_conf: syscon@43000000 { > compatible = "ti,j721e-system-controller"; > reg = <0x0 0x43000000 0x0 0x20000>; > > simple-bus; > syscon; Umm, no. This ship already sailed and we don't need a 2nd way to do things. Rob
On 10/31/22 12:14 PM, Rob Herring wrote: > On Mon, Oct 31, 2022 at 09:30:45AM -0500, Andrew Davis wrote: >> On 10/27/22 4:27 PM, Krzysztof Kozlowski wrote: >>> On 27/10/2022 17:07, Andrew Davis wrote: >>>> On 10/27/22 2:33 PM, Krzysztof Kozlowski wrote: >>>>> On 27/10/2022 14:13, Andrew Davis wrote: >>>>>> Writing this bit can be handled by the syscon-reboot driver. >>>>>> Add this node to DT. >>>>>> >>>>>> Signed-off-by: Andrew Davis <afd@ti.com> >>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >>>>>> Tested-by: Fabian Vogt <fabian@ritter-vogt.de> >>>>>> Reviewed-by: Fabian Vogt <fabian@ritter-vogt.de> >>>>>> --- >>>>>> arch/arm/boot/dts/nspire.dtsi | 7 +++++++ >>>>>> 1 file changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/nspire.dtsi b/arch/arm/boot/dts/nspire.dtsi >>>>>> index bb240e6a3a6f..48fbc9d533c3 100644 >>>>>> --- a/arch/arm/boot/dts/nspire.dtsi >>>>>> +++ b/arch/arm/boot/dts/nspire.dtsi >>>>>> @@ -172,7 +172,14 @@ rtc: rtc@90090000 { >>>>>> }; >>>>>> misc: misc@900a0000 { >>>>>> + compatible = "ti,nspire-misc", "syscon", "simple-mfd"; >>>>> >>>>> You have syscon and simple-mfd, but bindings in patch #1 say only syscon. >>>>> >>>> >>>> I'm not following, are you just saying my wording in the patch message just >>>> wasn't complete? >>> >>> Your binding patch adds nspire compatible to the list of two items, so >>> you have two items in total - nspire followed by syscon. >>> >>> What you implemented here is different. >>> >> >> Is there a list of three items I can add this compatible? If instead you >> mean I should go make a new binding, just say so :) > > An MFD should define its own schema file. > > The original intent of syscon.yaml was for just single nodes with > 'syscon' (and a specific compatible). Adding in simple-mfd was probably > a mistake. Certainly we need to rework the schema as you should get a > warning in this case. > >>>> Or are you saying something more about nodes that are both syscon and simple-mfd? >>>> In that case, having both syscon and simple-mfd seems rather common, looks like >>>> you added the rule for it[0]. >>>> >>>> Thinking on this, they almost represent the same thing. simple-mfd says "my child >>>> nodes should be considered devices", why do we need that? Couldn't we simply state >>>> that "syscon" node's children are always devices, I mean what else could they be, >>>> syscon is an MFD after all (and lives in drivers/mfd/). >>> >>> No, syscon is not an MFD. Syscon means system controller and alone it >>> does not have children. >>> >> >> The binding lives in devicetree/bindings/*mfd*/, it is mentioned as one >> in devicetree/bindings/mfd/mfd.txt. If it is not an MFD then the bindings >> are giving out mixed signals here.. >> >>>> >>>> "syscon" often just says, others can use the registers within this node, so as a >>>> different option, make "syscon" a property of "simple-mfd" nodes. I'm seeing all >>>> these examples of devices that should have been children of the "syscon" device, >>>> but instead use >>>> >>>> regmap = <&x>; >>>> syscon = <&x>; >>>> >>>> or similar and put the device node out somewhere random. And in those cases, >>>> wouldn't it have been more correct to use the normal "reg" and "regions" to >>>> define the registers belonging to the child node/device?.. >>> >>> Sorry, I do not follow. How this is even related to your patch? >>> >>> Your bindings say A, DTS say B. A != B. This needs fixing. >>> >> >> I said it was compatible with "syscon", not that it is incompatible >> with "simple-mfd" devices. >> >> What I've done here gives no dtbs_check warnings and >> "devicetree/bindings/mfd/mfd.txt" explicitly allows what I am doing. >> Unless we do not consider the old bindings valid? > > Only that the example is not because it doesn't have a specific > compatible. > > What needs to be clarified is that MFDs must define all the child nodes > whether they are 'simple' or not. > >> If so, would you >> like me to convert mfd.txt to yaml, just let me know. > > No, because I don't think there is anything to define as a schema. > It would allow for simple register regions to be 'simple-mfd' without needing a whole new binding document for each. Same as we already have with 'syscon.yaml'. Making every simple MMIO space create a new binding document is not reasonable. Neither is defining all nodes up front in that binding, we don't expect that for top level nodes or 'simple-bus', why should we for 'simple-mfd'? My point with mfd.txt is that this *was allowed*, and there are already a large number of users of the existing style. > >>> Unless you are asking me what your device is in general. This I don't >>> really know, but if you want to use it as regmap provider for system >>> registers and as a parent of syscon-based reboot device, then your >>> device is syscon and simple-mfd. With a specific compatible. Was this >>> your question? >>> >> >> Yes, I would like to use it as a regmap provider, my question here is >> a much more general one: why do I need to specify that in device tree? >> That is not a hardware description, my hardware is not "regmap" hardware. >> This "syscon" stuff feels like a bodge to make the Linux drivers and bus >> frameworks interact the way we want. > > Bingo! It's a hint for create a regmap. We could just have a compatible > list in the kernel for compatibles needing a regmap. Maybe that list > would be too long though. So call it h/w description for this h/w is > referenced by other places. > > >> I know at this point this has little to do with this series, but I'd like >> to just think this out for a moment. The latest Devicetree Specification >> talks about "simple-bus" as a special compatible that communicates that >> child nodes with compatible strings need probed also. ("simple-mfd" seems >> to be used the same way but without needing a "ranges" property..) > > Yes, both cases are saying there is no dependency or setup of the parent > needs. If the child nodes depend on the regmap, then it's not a > 'simple-mfd' IMO. Therefore 'syscon' together with 'simple-mfd' is wrong > unless it's other nodes that need the regmap. The schema can't really > check that. > 'syscon' also provides for reusing the same single register by multiple users, such as bit-mapped registers. It also allows re-using the existing simple syscon device compatibles. Again because people do not like writing bindings for simple nodes. Andrew >> Both of these are properties of a node, not something a device is "compatible" >> with. "compatibles" are also supposed to be listed "from most specific to >> most general", so which is more specific, "simple-mfd" or "syscon", etc.. > > I would say 'syscon' is more specific if I have to pick. It implies some > registers exist. 'simple-mfd' should mean there are no parent resources > (...the children depend on). > > We've probably got enough of a mixture of the order, it wouldn't be > worth the effort to try to enforce the order here. > >> Seems like Rob might agree[0], these are not really compatibles. We cant fix >> history, but for new nodes, instead of growing the problem and forcing these to >> be overloaded compatibles, we allow these to become new standard node properties. >> >> For instance: >> >> main_conf: syscon@43000000 { >> compatible = "ti,j721e-system-controller"; >> reg = <0x0 0x43000000 0x0 0x20000>; >> >> simple-bus; >> syscon; > > Umm, no. This ship already sailed and we don't need a 2nd way to do > things. > > Rob
diff --git a/arch/arm/boot/dts/nspire.dtsi b/arch/arm/boot/dts/nspire.dtsi index bb240e6a3a6f..48fbc9d533c3 100644 --- a/arch/arm/boot/dts/nspire.dtsi +++ b/arch/arm/boot/dts/nspire.dtsi @@ -172,7 +172,14 @@ rtc: rtc@90090000 { }; misc: misc@900a0000 { + compatible = "ti,nspire-misc", "syscon", "simple-mfd"; reg = <0x900a0000 0x1000>; + + reboot { + compatible = "syscon-reboot"; + offset = <0x08>; + value = <0x02>; + }; }; pwr: pwr@900b0000 {