Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse

[Rob Herring]

On Mon, Jun 15, 2020 at 4:44 AM Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
>
>
>
> On 12/06/2020 22:59, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx> wrote:
> >>
> >> This patch adds dt-bindings document for qfprom-efuse controller.
> >>
> >> Signed-off-by: Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx>
> >> ---
> >>   .../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@xxxxxxxxxxxxxxx" 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@xxxxxxxxxxxxxx>
> >> +
> >> +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
>
>
> Above is not required for now in this patchset, as we can attach data at
> runtime specific to version of the qfprom.
>
> This can be added when required!
>
> >        - 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.
> >
> >> +
> >
> > You missed a bunch of things that you should document:
> >
> >    # Clocks must be provided if QFPROM is writable from the OS image.
> >    clocks:
> >      maxItems: 1
> >    clock-names:
> >      const: sec
> >
> >    # Supply reference must be provided if QFPROM is writable from the OS image.
> >    vcc-supply:
> >      description: Our power supply.
> >
> >    # Needed if any child nodes are present.
> >    "#address-cells":
> >      const: 1
> >    "#size-cells":
> >      const: 1
> >
> >> +required:
> >> +   - compatible
> >> +   - reg
> >> +   - reg-names
> >
> > reg-names is discouraged.  Please remove.  I always point people here
> > as a reference:
> >
> > https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@xxxxxxxxxxxxxx/
> >
> > You can just figure out whether there are 3 register fields or 1 register field.
>
> Am not sure if I understand this correctly, reg-names are very useful in
> this particular case as we are dealing with multiple memory ranges with
> holes. I agree with not having this for cases where we have only one
> resource.

1 or 3 doesn't sound like a case with holes.

> But having the ordering in DT without proper names associated with it
> seems fragile to me! And it makes very difficult to debug issues with
> wrong resource ordering in DT.
>
> Rob, Is this the guidance for new bindings?

This has *always* been the guidance since *-names were added.

> I have not seen any strong suggestion or guidance either in
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/resource-names.txt?h=v5.8-rc1

The key word is 'supplemental'. Perhaps that could be clearer, but DT
always required a defined order and the supplemental information
doesn't throw that out.

>   Or in ./drivers/of/address.c

How could it? Order is defined by the specific binding.

>
> Am I missing anything here?