| Message ID | 1591868882-16553-2-git-send-email-rbokka@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RFC,v2,1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On 6/11/2020 3:18 PM, Ravi Kumar Bokka wrote: > This patch adds dt-bindings document for qfprom-efuse controller. there is an existing bindings doc (in .txt) which you should convert to yaml, and then add another patch to extend it. > > Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org> > --- > .../devicetree/bindings/nvmem/qfprom.yaml | 52 ++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > 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 > + > + reg: > + maxItems: 3 > + > +required: > + - compatible > + - reg > + - reg-names > + - clocks > + - clock-names > + > +examples: > + - | > + #include <dt-bindings/clock/qcom,gcc-sc7180.h> > + > + qfprom@780000 { > + compatible = "qcom,qfprom"; > + reg = <0 0x00780000 0 0x7a0>, > + <0 0x00782000 0 0x100>, > + <0 0x00784000 0 0x8ff>; > + reg-names = "raw", "conf", "corrected"; > + clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>; > + clock-names = "secclk"; > + > + qusb2p_hstx_trim: hstx-trim-primary@25b { > + reg = <0x25b 0x1>; > + bits = <1 3>; > + }; > + }; > + > + &qfprom { > + vcc-supply = <&vreg_l11a_1p8>; > + }; > + >
On Thu, 11 Jun 2020 15:18:00 +0530, Ravi Kumar Bokka 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(+) > create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/nvmem/qfprom.yaml: while scanning a block scalar in "<unicode string>", line 31, column 5 found a tab character where an indentation space is expected in "<unicode string>", line 35, column 1 Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/nvmem/qfprom.example.dts' failed make[1]: *** [Documentation/devicetree/bindings/nvmem/qfprom.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qfprom.yaml: ignoring, error parsing file warning: no schema found in file: ./Documentation/devicetree/bindings/nvmem/qfprom.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qfprom.yaml: ignoring, error parsing file warning: no schema found in file: ./Documentation/devicetree/bindings/nvmem/qfprom.yaml Makefile:1300: recipe for target 'dt_binding_check' failed make: *** [dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1307399 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit.
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. > + 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@mail.gmail.com/ You can just figure out whether there are 3 register fields or 1 register field. > + - clocks > + - clock-names You can't retroactively make things required. In read-only mode I believe we don't require clocks/clock-names. Presumably the clock is only needed if we're writing? > +examples: > + - | > + #include <dt-bindings/clock/qcom,gcc-sc7180.h> > + > + qfprom@780000 { > + compatible = "qcom,qfprom"; As pointed out by Rob H in v1, you have a whole bunch of tabs in here that "dt_binding_check" yells about. Please fix and confirm that you ran "dt_binding_check" on your bindings and they passed with no errors. This is another case where someone took the time to respond to your v1 but you didn't address their comments in sending v2. > + reg = <0 0x00780000 0 0x7a0>, > + <0 0x00782000 0 0x100>, > + <0 0x00784000 0 0x8ff>; > + reg-names = "raw", "conf", "corrected"; You are missing #address-cells and #size-cells, which are required because you have a sub-node. > + clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>; > + clock-names = "secclk"; As pointed out in v1, people don't like clock names to end in "clk". Just call this "sec". > + qusb2p_hstx_trim: hstx-trim-primary@25b { > + reg = <0x25b 0x1>; > + bits = <1 3>; > + }; > + }; > + > + &qfprom { > + vcc-supply = <&vreg_l11a_1p8>; > + }; Just fold the vcc-supply into the above node. Note that your current example refers to a phandle that doesn't exist. > + > -- > Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. For your edification, I have placed a patch that fixes all my own review feedback (and passes dt_binding_check) at: https://crrev.com/c/2243853 Please either fold this into your next patch or provide comments about why you aren't taking one of my suggestions. -Doug
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
On 12/06/2020 22:59, Doug Anderson 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 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@mail.gmail.com/ > > 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. 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? 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 Or in ./drivers/of/address.c Am I missing anything here?
On Mon, Jun 15, 2020 at 4:44 AM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > > On 12/06/2020 22:59, Doug Anderson 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 > > > 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@mail.gmail.com/ > > > > 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?
Hi, On Mon, Jun 15, 2020 at 3:44 AM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > >> +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! I'm OK with leaving off for this patchset. After we get everything landed for blowing fuses then I can send out a separate patch where we can debate the merits of adding the SoC-specific compatible strings. :-) Sound good? -Doug
On 15/06/2020 15:27, Doug Anderson wrote: > Hi, > > On Mon, Jun 15, 2020 at 3:44 AM Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: >> >>>> +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! > > I'm OK with leaving off for this patchset. After we get everything > landed for blowing fuses then I can send out a separate patch where we > can debate the merits of adding the SoC-specific compatible strings. > :-) Sound good? Sounds good! --srini > > -Doug >
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 + + reg: + maxItems: 3 + +required: + - compatible + - reg + - reg-names + - clocks + - clock-names + +examples: + - | + #include <dt-bindings/clock/qcom,gcc-sc7180.h> + + qfprom@780000 { + compatible = "qcom,qfprom"; + reg = <0 0x00780000 0 0x7a0>, + <0 0x00782000 0 0x100>, + <0 0x00784000 0 0x8ff>; + reg-names = "raw", "conf", "corrected"; + clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>; + clock-names = "secclk"; + + qusb2p_hstx_trim: hstx-trim-primary@25b { + reg = <0x25b 0x1>; + bits = <1 3>; + }; + }; + + &qfprom { + vcc-supply = <&vreg_l11a_1p8>; + }; +
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(+) create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml