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;
MIME-Version: 1.0
References: <20220610165940.2326777-1-jiaqing.zhao@linux.intel.com>
 <20220610165940.2326777-6-jiaqing.zhao@linux.intel.com> <1654903146.313095.2450355.nullmailer@robh.at.kernel.org>
 <21c9ba6b-e84e-4545-44d2-5ffe5fea9581@linux.intel.com>
In-Reply-To: <21c9ba6b-e84e-4545-44d2-5ffe5fea9581@linux.intel.com>
From:   Rob Herring <robh@kernel.org>
Date:   Mon, 13 Jun 2022 09:28:26 -0600
Message-ID: <CAL_Jsq+y3tkfLV8UpUe6jw7Fq7YDrzwoq3FKK4jeeZEBOxhM4g@mail.gmail.com>
Subject: Re: [PATCH v2 5/6] dt-bindings: net: Add NCSI bindings
To:     Jiaqing Zhao <jiaqing.zhao@linux.intel.com>
Cc:     OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        netdev <netdev@vger.kernel.org>, devicetree@vger.kernel.org,
        "David S . Miller" <davem@davemloft.net>,
        Samuel Mendoza-Jonas <sam@mendozajonas.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 10, 2022 at 9:09 PM Jiaqing Zhao
<jiaqing.zhao@linux.intel.com> wrote:
>
> On 2022-06-11 07:19, Rob Herring wrote:
> > On Sat, 11 Jun 2022 00:59:39 +0800, Jiaqing Zhao wrote:
> >> Add devicetree bindings for NCSI VLAN modes. This allows VLAN mode to
> >> be configured in devicetree.
> >>
> >> Signed-off-by: Jiaqing Zhao <jiaqing.zhao@linux.intel.com>
> >> ---
> >>  .../devicetree/bindings/net/ncsi.yaml         | 34 ++++++++++++++++++=
+
> >>  MAINTAINERS                                   |  2 ++
> >>  include/dt-bindings/net/ncsi.h                | 15 ++++++++
> >>  3 files changed, 51 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/net/ncsi.yaml
> >>  create mode 100644 include/dt-bindings/net/ncsi.h
> >>
> >
> > My bot found errors running 'make DT_CHECKER_FLAGS=3D-m dt_binding_chec=
k'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/ne=
t/ncsi.yaml: 'oneOf' conditional failed, one must be fixed:
> >       'unevaluatedProperties' is a required property
> >       'additionalProperties' is a required property
> >       hint: Either unevaluatedProperties or additionalProperties must b=
e present
> >       from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/ne=
t/ncsi.yaml: ignoring, error in schema:
> > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/ne=
t/ncsi.example.dtb: ethernet@1e660000: 'ncsi,vlan-mode' does not match any =
of the regexes
> >       From schema: /builds/robherring/linux-dt-review/Documentation/dev=
icetree/bindings/vendor-prefixes.yaml
>
> I saw vendor-prefix.yaml says do not add non-vendor prefixes to the list.=
 Since "ncsi" is not a vendor, may I ask what is the suggested replacement =
for 'ncsi,vlan-mode'? Will 'ncsi-vlan-mode' be fine?

I don't know. What is NCSI? Is it specific to certain MACs? Why do you
need to set this up in DT? Network configuration is typically done in
userspace, so putting VLAN config in DT doesn't seem right. All
questions your commit message should answer.

>
> > Documentation/devicetree/bindings/net/ncsi.example.dtb:0:0: /example-0/=
ethernet@1e660000: failed to match any schema with compatible: ['aspeed,ast=
2600-mac', 'faraday,ftgmac100']
> > Documentation/devicetree/bindings/net/ncsi.example.dtb:0:0: /example-0/=
ethernet@1e660000: failed to match any schema with compatible: ['aspeed,ast=
2600-mac', 'faraday,ftgmac100']
>
> The ftgmac100 it depends on uses a txt document instead of an yaml schema=
. And I see there is other schemas having the same error, can this be ignor=
ed?

No. Don't add to the list. Once all the existing warnings (~40) are
fixed, then this will be turned on by default.

>
> And I've got one more question. The ncsi driver does not has its own comp=
atible field, instead, it is enabled by setting the "use-ncsi" property of =
some specific mac drivers. Though currently only ftgmac100 supports ncsi in=
 upstream kernel, it may be used by other mac drivers in the future. What d=
o you think is a proper way for defining the ncsi schema? Having it in a se=
parate yaml like this patch or add the properties to all the mac yamls that=
 supports yaml? If the former way is preferred, how should the schema be de=
fined without "compatible"?

If it is a function of driver support or not, then it doesn't belong in DT.

Rob