From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4E8F6C4361B for ; Wed, 16 Dec 2020 06:26:28 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D4B6F2313C for ; Wed, 16 Dec 2020 06:26:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D4B6F2313C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=baikalelectronics.ru Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=FOxzBtzb0NdVpyGVk6OmQ/Vp4Xb3yg6IBSC5h1CfExA=; b=hsVdeYiGHIMTRs4KE4/S/OIpJ jiW6mBEMexIoSv2CHk4g/a81/PyLt1fgvrK4dUMD7RRGY756KMh3AmRBw5X5VsAWPB9rIyrcV9k7N 9BHvNLkvceWyDzmc4rQjNJhcu7mFp9Vvvs0RbMhzXmBiiv6g+YKeISTi04+fConCuAZdWz8UYiF1j 5W1FvsKnvGFlzviIUOqVbsxHPbziPbeD0kyKUcfqNuh02ehWwLDSltrl6QpKGWXxXNKm2+0fBu9b1 TEkvsrjccklFGEF77HBqi2zUKT6yuymq0fbdPQ/NS2u+1xXKT+c8v0fX4KHLxwCT5tNX2vgXnoB3w XuZAymO8g==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kpQFE-0003VM-SZ; Wed, 16 Dec 2020 06:25:04 +0000 Received: from mail.baikalelectronics.com ([87.245.175.226] helo=mail.baikalelectronics.ru) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kpQFB-0003Uo-H0 for linux-arm-kernel@lists.infradead.org; Wed, 16 Dec 2020 06:25:02 +0000 Date: Wed, 16 Dec 2020 09:24:58 +0300 From: Serge Semin To: Rob Herring Subject: Re: [PATCH 04/25] dt-bindings: net: dwmac: Refactor snps,*-config properties Message-ID: <20201216062458.7oiccf6fc5lvabdc@mobilestation> References: <20201214091616.13545-1-Sergey.Semin@baikalelectronics.ru> <20201214091616.13545-5-Sergey.Semin@baikalelectronics.ru> <20201214143006.GA1864564@robh.at.kernel.org> <20201215085421.v5aepprkk2iyimaw@mobilestation> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-ClientProxiedBy: MAIL.baikal.int (192.168.51.25) To mail (192.168.51.25) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201216_012501_856657_A56EC054 X-CRM114-Status: GOOD ( 43.38 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, "linux-kernel@vger.kernel.org" , Alexandre Torgue , Joao Pinto , netdev , Lars Persson , "moderated list:ARM/STM32 ARCHITECTURE" , Johan Hovold , Serge Semin , Alexey Malahov , Jose Abreu , Maxime Ripard , Maxime Coquelin , Jakub Kicinski , Giuseppe Cavallaro , Vyacheslav Mitrofanov , "David S. Miller" , linux-arm-kernel , Pavel Parkhomenko Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Dec 15, 2020 at 08:08:35AM -0600, Rob Herring wrote: > On Tue, Dec 15, 2020 at 2:54 AM Serge Semin > wrote: > > > > Hello Rob, > > > > On Mon, Dec 14, 2020 at 08:30:06AM -0600, Rob Herring wrote: > > > On Mon, Dec 14, 2020 at 12:15:54PM +0300, Serge Semin wrote: > > > > Currently the "snps,axi-config", "snps,mtl-rx-config" and > > > > "snps,mtl-tx-config" properties are declared as a single phandle reference > > > > to a node with corresponding parameters defined. That's not good for > > > > several reasons. First of all scattering around a device tree some > > > > particular device-specific configs with no visual relation to that device > > > > isn't suitable from maintainability point of view. That leads to a > > > > disturbed representation of the actual device tree mixing actual device > > > > nodes and some vendor-specific configs. Secondly using the same configs > > > > set for several device nodes doesn't represent well the devices structure, > > > > since the interfaces these configs describe in hardware belong to > > > > different devices and may actually differ. In the later case having the > > > > configs node separated from the corresponding device nodes gets to be > > > > even unjustified. > > > > > > > > So instead of having a separate DW *MAC configs nodes we suggest to > > > > define them as sub-nodes of the device nodes, which interfaces they > > > > actually describe. By doing so we'll make the DW *MAC nodes visually > > > > correct describing all the aspects of the IP-core configuration. Thus > > > > we'll be able to describe the configs sub-nodes bindings right in the > > > > snps,dwmac.yaml file. > > > > > > > > Note the former "snps,axi-config", "snps,mtl-rx-config" and > > > > "snps,mtl-tx-config" bindings have been marked as deprecated. > > > > > > > > Signed-off-by: Serge Semin > > > > > > > > --- > > > > > > > > Note the current DT schema tool requires the vendor-specific properties to be > > > > defined in accordance with the schema: dtschema/meta-schemas/vendor-props.yaml > > > > It means the property can be; > > > > - boolean, > > > > - string, > > > > - defined with $ref and additional constraints, > > > > - defined with allOf: [ $ref ] and additional constraints. > > > > > > > > The modification provided by this commit needs to extend that definition to > > > > make the DT schema tool correctly parse this schema. That is we need to let > > > > the vendors-specific properties to also accept the oneOf-based combined > > > > sub-schema. Like this: > > > > > > > > --- a/dtschema/meta-schemas/vendor-props.yaml > > > > +++ b/dtschema/meta-schemas/vendor-props.yaml > > > > @@ -48,15 +48,24 @@ > > > > - properties: # A property with a type and additional constraints > > > > $ref: > > > > pattern: "types.yaml#[\/]{0,1}definitions\/.*" > > > > - allOf: > > > > - items: > > > > - - properties: > > > > + > > > > + if: > > > > + not: > > > > + required: > > > > + - $ref > > > > + then: > > > > + patternProperties: > > > > + "^(all|one)Of$": > > > > + contains: > > > > + properties: > > > > $ref: > > > > pattern: "types.yaml#[\/]{0,1}definitions\/.*" > > > > required: > > > > - $ref > > > > - oneOf: > > > > + > > > > + anyOf: > > > > - required: [ $ref ] > > > > - required: [ allOf ] > > > > + - required: [ oneOf ] > > > > > > > > ... > > > > --- > > > > .../devicetree/bindings/net/snps,dwmac.yaml | 380 +++++++++++++----- > > > > 1 file changed, 288 insertions(+), 92 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > > index 0dd543c6c08e..44aa88151cba 100644 > > > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > > @@ -150,69 +150,251 @@ properties: > > > > in a different mode than the PHY in order to function. > > > > > > > > snps,axi-config: > > > > - $ref: /schemas/types.yaml#definitions/phandle > > > > - description: > > > > - AXI BUS Mode parameters. Phandle to a node that can contain the > > > > - following properties > > > > - * snps,lpi_en, enable Low Power Interface > > > > - * snps,xit_frm, unlock on WoL > > > > - * snps,wr_osr_lmt, max write outstanding req. limit > > > > - * snps,rd_osr_lmt, max read outstanding req. limit > > > > - * snps,kbbe, do not cross 1KiB boundary. > > > > - * snps,blen, this is a vector of supported burst length. > > > > - * snps,fb, fixed-burst > > > > - * snps,mb, mixed-burst > > > > - * snps,rb, rebuild INCRx Burst > > > > + description: AXI BUS Mode parameters > > > > + oneOf: > > > > + - deprecated: true > > > > + $ref: /schemas/types.yaml#definitions/phandle > > > > + - type: object > > > > + properties: > > > > > > > > Anywhere have have the same node/property string meaning 2 different > > > things is a pain, let's not create another one. > > > > IIUC you meant that having a node and property with the same name > > isn't ok. Right? If so could you explain why not? especially seeing > > the property is expected to be set with phandle reference to that > > node. That seemed like a perfect solution to me. We wouldn't need to > > introduce a new property/node name, but just deprecate the > > corresponding name to be a property. > > Right. It's also a property or node name having 2 different meanings. > I think your schema above demonstrates the problem in that it > unnecessarily complicates the schema. It's not such a problem here as > it is self contained. The worst example is 'ports'. That's a container > of graph port nodes, ethernet switch nodes or a property pointing to > DRM graphics pipelines. If there's multiple meanings, then we can't > apply a schema unconditionally. Or we can only check it matches one of > the 3 definitions. It turned out in case of this change having different meaning also luckily fit with having different types (property vs node). Right, as you called it's self contained. But in general, of course, having different meaning of the same node indeed may cause problems with different schema validation. So ok. Thanks for clarification. I'll just introduce a new sub-node with the same name but no vendor-prefix. > > > > Just define a new node > > > 'axi-config'. Or just put all the properties into the node directly. > > > Grouping them has little purpose. > > > > Hm, you suggest to remove the vendor prefix, right? > > Yes, we don't do vendor prefixes on node names either. Ok. > > > If so what about > > the rest of the changes introduced here in this patch? They concern > > "snps,mtl-tx-config" and "snps,mtl-rx-config" properties (please note > > these changes are a bit more complicated than once connected with > > "snps,axi-config"). Should I remove the vendor-prefix from them too? > > Yes. Ok. -Sergey > > > Anyway that seems a bit questionable, because all the "snps,*-config" > > properties/nodes seems more vendor-specific than generic. Am I wrong > > in that matter? > > > > If you think they are generic, then the "{axi,mtl-rx,mtl-tx}-config" > > nodes most likely should be described in the dedicated DT schema... > > > > -Sergey > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel