Date:   Thu, 18 Nov 2021 17:57:35 -0600
From:   Rob Herring <robh@kernel.org>
To:     Bhupesh Sharma <bhupesh.sharma@linaro.org>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-crypto@vger.kernel.org,
        bhupesh.linux@gmail.com, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, agross@kernel.org,
        herbert@gondor.apana.org.au, davem@davemloft.net,
        stephan@gerhold.net, Thara Gopinath <thara.gopinath@linaro.org>
Subject: Re: [PATCH v5 09/22] dt-bindings: qcom-qce: Move 'clocks' to
 optional properties
Message-ID: <YZbobzlBNWIRNn/e@robh.at.kernel.org>
References: <20211110105922.217895-1-bhupesh.sharma@linaro.org>
 <20211110105922.217895-10-bhupesh.sharma@linaro.org>
 <YZAZxmsp5WLeOBuF@builder.lan>
 <CAH=2NtwGM0==3etkG6seV=3+xO347VNEoKghpyBs9DjZPU4xNA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAH=2NtwGM0==3etkG6seV=3+xO347VNEoKghpyBs9DjZPU4xNA@mail.gmail.com>
Precedence: bulk

On Mon, Nov 15, 2021 at 11:04:31AM +0530, Bhupesh Sharma wrote:
> Hi Bjorn,
> 
> On Sun, 14 Nov 2021 at 01:32, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 10 Nov 04:59 CST 2021, Bhupesh Sharma wrote:
> >
> > > QCom QCE block on some SoCs like ipq6018 don't
> > > require clock as the required property, so the properties
> > > 'clocks' and 'clock-names' can be moved instead in the dt-bindings
> > > to the 'optional' properties section.
> > >
> > > Otherwise, running 'make dtbs_check' leads to the following
> > > errors:
> > >
> > > dma-controller@7984000: clock-names:0: 'bam_clk' was expected
> > >       arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dt.yaml
> > >
> > > dma-controller@7984000: clock-names: Additional items are not allowed ('bam_clk' was unexpected)
> > >       arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dt.yaml
> > >
> > > dma-controller@7984000: clock-names: ['iface_clk', 'bam_clk'] is too long
> > >       arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dt.yaml
> > >
> > > dma-controller@7984000: clocks: [[9, 138], [9, 137]] is too long
> > >       arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dt.yaml
> > >
> > > Cc: Thara Gopinath <thara.gopinath@linaro.org>
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/crypto/qcom-qce.yaml | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/crypto/qcom-qce.yaml b/Documentation/devicetree/bindings/crypto/qcom-qce.yaml
> > > index 30deaa0fa93d..f35bdb9ee7a8 100644
> > > --- a/Documentation/devicetree/bindings/crypto/qcom-qce.yaml
> > > +++ b/Documentation/devicetree/bindings/crypto/qcom-qce.yaml
> > > @@ -53,8 +53,6 @@ properties:
> > >  required:
> > >    - compatible
> > >    - reg
> > > -  - clocks
> > > -  - clock-names
> >
> > I would prefer that we make this conditional on the compatible. That
> > said, if this only applies to ipq6018 I think we should double check the
> > fact that there's no clock there...
> >
> > For the sake of making progress on the series, I think you should omit
> > this patch from the next version.
> 
> Without this patch, 'make dtbs_check' fails with the following error:
> dma-controller@7984000: clock-names:0: 'bam_clk' was expected
>         arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dt.yaml
> 
> dma-controller@7984000: clock-names: Additional items are not allowed
> ('bam_clk' was unexpected)
>         arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dt.yaml

Those errors do not correspond to the change here. Adding something to 
'required' would never solve any error (other than a driver requires a 
property to function).


> which I think is making Rob bot-check fail.

dtbs_check don't have to be fixed as the message says.

> So, I think instead of dropping the patch, let's try and understand
> from the 'ipq6018 qce' documentation if the clocks are really
> 'optional' there for the qce block (as clock properties are not
> mentioned in the dts from the very first upstream version). If not, we
> can try and fix the 'ipq6018 qce' dts node itself.
> 
> Regards,
> Bhupesh
>