Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1591868882-16553-1-git-send-email-rbokka@codeaurora.org>
 <1591868882-16553-2-git-send-email-rbokka@codeaurora.org> <CAD=FV=WjvAWVmq3fTh=_f2p1Dv+sXg1RV-CqZr8KRgHe8_wT0w@mail.gmail.com>
In-Reply-To: <CAD=FV=WjvAWVmq3fTh=_f2p1Dv+sXg1RV-CqZr8KRgHe8_wT0w@mail.gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 12 Jun 2020 16:15:29 -0700
Message-ID: <CAD=FV=W4QqjOOfh_pJgg3P1PCEGOv8cubFKFKeYOBC0VVk2mZg@mail.gmail.com>
Subject: Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse
To:     Ravi Kumar Bokka <rbokka@codeaurora.org>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
        dhavalp@codeaurora.org, mturney@codeaurora.org,
        sparate@codeaurora.org, mkurumel@codeaurora.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Fri, Jun 12, 2020 at 2:59 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
> >
> > This patch adds dt-bindings document for qfprom-efuse controller.
> >
> > Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> > ---
> >  .../devicetree/bindings/nvmem/qfprom.yaml          | 52 ++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
>
> Overall comment: I reviewed your v1 series and so I'm obviously
> interested in your series.  Please CC me on future versions.
>
> I would also note that, since this is relevant to Qualcomm SoCs that
> you probably should be CCing "linux-arm-msm@vger.kernel.org" on your
> series.
>
>
> >  create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> > new file mode 100644
> > index 0000000..7c8fc31
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/nvmem/qfprom.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Technologies Inc, QFPROM Efuse bindings
> > +
> > +maintainers:
> > +  - Ravi Kumar Bokka <rbokka@codeaurora.org>
> > +
> > +allOf:
> > +  - $ref: "nvmem.yaml#"
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,qfprom
>
> As per discussion in patch #1, I believe SoC compatible should be here
> too in case it is ever needed.  This is standard practice for dts
> files for IP blocks embedded in an SoC.  AKA, this should be:
>
>     items:
>       - enum:
>           - qcom,apq8064-qfprom
>           - qcom,apq8084-qfprom
>           - qcom,msm8974-qfprom
>           - qcom,msm8916-qfprom
>           - qcom,msm8996-qfprom
>           - qcom,msm8998-qfprom
>           - qcom,qcs404-qfprom
>           - qcom,sc7180-qfprom
>           - qcom,sdm845-qfprom
>       - const: qcom,qfprom
>
> NOTE: old SoCs won't have both of these and thus they will get flagged
> with "dtbs_check", but I believe that's fine (Rob can correct me if
> I'm wrong).  The code should still work OK if the SoC isn't there but
> it would be good to fix old dts files to have the SoC specific string
> too.
>
>
> > +
> > +  reg:
> > +    maxItems: 3
>
> Please address feedback feedback on v1.  If you disagree with my
> feedback it's OK to say so (I make no claims of being always right),
> but silently ignoring my feedback and sending the next version doesn't
> make me feel like it's a good use of my time to keep reviewing your
> series.  Specifically I suggested that you actually add descriptions
> rather than just putting "maxItems: 3".
>
> With all that has been discussed, I think the current best thing to
> put there is:
>
>     # If the QFPROM is read-only OS image then only the corrected region
>     # needs to be provided.  If the QFPROM is writable then all 3 regions
>     # must be provided.
>     oneOf:
>       - items:
>           - description: The start of the corrected region.
>       - items:
>           - description: The start of the raw region.
>           - description: The start of the config region.
>           - description: The start of the corrected region.

To address some of Srinivas's comments, I think you should actually
add the 4th range in here too:

- description: The start of the security control region.

That allows you to read 0x6000 and get the major/minor/step properly.


I will also note that as I've started reviewing the driver code it'll
probably make everyone's life easiest if you keep the "corrected"
region first, even if it's not numerically first.


I've updated my FIXUP patch again.  I might notice more comments as I
review the driver more, but we'll see...

-Doug