* [RFC v2 0/3] Add QTI QFPROM-Efuse driver support @ 2020-06-11 9:47 Ravi Kumar Bokka 2020-06-11 9:48 ` [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse Ravi Kumar Bokka ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Ravi Kumar Bokka @ 2020-06-11 9:47 UTC (permalink / raw) To: Srinivas Kandagatla, Rob Herring Cc: linux-kernel, devicetree, rnayak, saiprakash.ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel, Ravi Kumar Bokka This patch series adds qfprom-efuse controller driver support. This driver can access the raw qfprom regions for fuse blowing. The current existed qfprom driver is only supports for cpufreq, thermal sensors drivers by read out calibration data, speed bins..etc which is stored by qfprom efuses. Ravi Kumar Bokka (3): dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse drivers: nvmem: Add QTI qfprom-efuse support arm64: dts: qcom: sc7180: Add qfprom-efuse .../devicetree/bindings/nvmem/qfprom.yaml | 52 +++ arch/arm64/boot/dts/qcom/sc7180-idp.dts | 4 + arch/arm64/boot/dts/qcom/sc7180.dtsi | 10 +- drivers/nvmem/Kconfig | 1 + drivers/nvmem/qfprom.c | 405 +++++++++++++++++++-- 5 files changed, 449 insertions(+), 23 deletions(-) create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml -- Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-11 9:47 [RFC v2 0/3] Add QTI QFPROM-Efuse driver support Ravi Kumar Bokka @ 2020-06-11 9:48 ` Ravi Kumar Bokka 2020-06-11 12:18 ` Rajendra Nayak ` (2 more replies) 2020-06-11 9:48 ` [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support Ravi Kumar Bokka 2020-06-11 9:48 ` [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse Ravi Kumar Bokka 2 siblings, 3 replies; 18+ messages in thread From: Ravi Kumar Bokka @ 2020-06-11 9:48 UTC (permalink / raw) To: Srinivas Kandagatla, Rob Herring Cc: linux-kernel, devicetree, rnayak, saiprakash.ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel, Ravi Kumar Bokka 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 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>; + }; + -- Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-11 9:48 ` [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse Ravi Kumar Bokka @ 2020-06-11 12:18 ` Rajendra Nayak 2020-06-12 21:58 ` Rob Herring 2020-06-12 21:59 ` Doug Anderson 2 siblings, 0 replies; 18+ messages in thread From: Rajendra Nayak @ 2020-06-11 12:18 UTC (permalink / raw) To: Ravi Kumar Bokka, Srinivas Kandagatla, Rob Herring Cc: linux-kernel, devicetree, saiprakash.ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel 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>; > + }; > + > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-11 9:48 ` [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse Ravi Kumar Bokka 2020-06-11 12:18 ` Rajendra Nayak @ 2020-06-12 21:58 ` Rob Herring 2020-06-12 21:59 ` Doug Anderson 2 siblings, 0 replies; 18+ messages in thread From: Rob Herring @ 2020-06-12 21:58 UTC (permalink / raw) To: Ravi Kumar Bokka Cc: mkurumel, saiprakash.ranjan, linux-kernel, sparate, devicetree, dhavalp, mturney, Rob Herring, Srinivas Kandagatla, rnayak, c_rbokka 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-11 9:48 ` [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse Ravi Kumar Bokka 2020-06-11 12:18 ` Rajendra Nayak 2020-06-12 21:58 ` Rob Herring @ 2020-06-12 21:59 ` Doug Anderson 2020-06-12 23:15 ` Doug Anderson 2020-06-15 10:44 ` Srinivas Kandagatla 2 siblings, 2 replies; 18+ messages in thread From: Doug Anderson @ 2020-06-12 21:59 UTC (permalink / raw) To: Ravi Kumar Bokka Cc: Srinivas Kandagatla, Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-12 21:59 ` Doug Anderson @ 2020-06-12 23:15 ` Doug Anderson 2020-06-15 10:44 ` Srinivas Kandagatla 1 sibling, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-06-12 23:15 UTC (permalink / raw) To: Ravi Kumar Bokka Cc: Srinivas Kandagatla, Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, mkurumel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-12 21:59 ` Doug Anderson 2020-06-12 23:15 ` Doug Anderson @ 2020-06-15 10:44 ` Srinivas Kandagatla 2020-06-15 14:17 ` Rob Herring 2020-06-15 14:27 ` Doug Anderson 1 sibling, 2 replies; 18+ messages in thread From: Srinivas Kandagatla @ 2020-06-15 10:44 UTC (permalink / raw) To: Doug Anderson, Ravi Kumar Bokka, Rob Herring Cc: LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel 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? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-15 10:44 ` Srinivas Kandagatla @ 2020-06-15 14:17 ` Rob Herring 2020-06-15 14:27 ` Doug Anderson 1 sibling, 0 replies; 18+ messages in thread From: Rob Herring @ 2020-06-15 14:17 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Doug Anderson, Ravi Kumar Bokka, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel 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? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-15 10:44 ` Srinivas Kandagatla 2020-06-15 14:17 ` Rob Herring @ 2020-06-15 14:27 ` Doug Anderson 2020-06-15 14:28 ` Srinivas Kandagatla 1 sibling, 1 reply; 18+ messages in thread From: Doug Anderson @ 2020-06-15 14:27 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Ravi Kumar Bokka, Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse 2020-06-15 14:27 ` Doug Anderson @ 2020-06-15 14:28 ` Srinivas Kandagatla 0 siblings, 0 replies; 18+ messages in thread From: Srinivas Kandagatla @ 2020-06-15 14:28 UTC (permalink / raw) To: Doug Anderson Cc: Ravi Kumar Bokka, Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support 2020-06-11 9:47 [RFC v2 0/3] Add QTI QFPROM-Efuse driver support Ravi Kumar Bokka 2020-06-11 9:48 ` [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse Ravi Kumar Bokka @ 2020-06-11 9:48 ` Ravi Kumar Bokka 2020-06-13 20:33 ` Doug Anderson 2020-06-15 9:56 ` Srinivas Kandagatla 2020-06-11 9:48 ` [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse Ravi Kumar Bokka 2 siblings, 2 replies; 18+ messages in thread From: Ravi Kumar Bokka @ 2020-06-11 9:48 UTC (permalink / raw) To: Srinivas Kandagatla, Rob Herring Cc: linux-kernel, devicetree, rnayak, saiprakash.ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel, Ravi Kumar Bokka This patch adds support for QTI qfprom-efuse controller. This driver can access the raw qfprom regions for fuse blowing. The current existed qfprom driver is only supports for cpufreq, thermal sensors drivers by read out calibration data, speed bins..etc which is stored by qfprom efuses. Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org> --- drivers/nvmem/Kconfig | 1 + drivers/nvmem/qfprom.c | 405 ++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 385 insertions(+), 21 deletions(-) diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig index d7b7f6d..623d59e 100644 --- a/drivers/nvmem/Kconfig +++ b/drivers/nvmem/Kconfig @@ -117,6 +117,7 @@ config QCOM_QFPROM help Say y here to enable QFPROM support. The QFPROM provides access functions for QFPROM data to rest of the drivers via nvmem interface. + And this driver provides access QTI qfprom efuse via nvmem interface. This driver can also be built as a module. If so, the module will be called nvmem_qfprom. diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c index 8a91717..312318c 100644 --- a/drivers/nvmem/qfprom.c +++ b/drivers/nvmem/qfprom.c @@ -3,17 +3,266 @@ * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> */ +#include <linux/clk.h> #include <linux/device.h> +#include <linux/io.h> +#include <linux/iopoll.h> +#include <linux/kernel.h> #include <linux/module.h> #include <linux/mod_devicetable.h> -#include <linux/io.h> #include <linux/nvmem-provider.h> +#include <linux/of_device.h> #include <linux/platform_device.h> +#include <linux/regulator/consumer.h> + +#define QFPROM_BLOW_STATUS_BUSY 0x1 +#define QFPROM_BLOW_STATUS_READY 0x0 + +/* Blow timer clock frequency in Mhz for 10nm LPe technology */ +#define QFPROM_BLOW_TIMER_OFFSET 0x03c +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0 + +/* Amount of time required to hold charge to blow fuse in micro-seconds */ +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100 +#define QFPROM_BLOW_STATUS_OFFSET 0x048 + +#define QFPROM_ACCEL_OFFSET 0x044 + +/** + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse + * platform data + * + * @name: qfprom-efuse compatible name + * @fuse_blow_time_in_us: Should contain the wait time when doing the fuse blow + * @accel_value: Should contain qfprom accel value + * @accel_reset_value: The reset value of qfprom accel value + * @qfprom_blow_timer_value: The timer value of qfprom when doing efuse blow + * @qfprom_blow_reset_freq: The frequency required to set when fuse blowing + * is done + * @qfprom_blow_set_freq: The frequency required to set when we start the + * fuse blowing + * @qfprom_max_vol: max voltage required to set fuse blow + * @qfprom_min_vol: min voltage required to set fuse blow + */ +struct qfprom_efuse_platform_data { + const char *name; + u8 fuse_blow_time_in_us; + u32 accel_value; + u32 accel_reset_value; + u32 qfprom_blow_timer_value; + u32 qfprom_blow_reset_freq; + u32 qfprom_blow_set_freq; + u32 qfprom_max_vol; + u32 qfprom_min_vol; +}; + +/** + * struct qfprom_priv - structure holding qfprom attributes + * + * @qfpraw: iomapped memory space for qfprom-efuse raw address space + * @qfpconf: iomapped memory space for qfprom-efuse configuration address space + * @qfpcorrected: iomapped memory space for qfprom corrected address space + + * @dev: qfprom device structure + * @secclk: clock supply + * @vcc: regulator supply + * @qfprom_efuse_platform_data: qfprom platform data + */ struct qfprom_priv { - void __iomem *base; + void __iomem *qfpraw; + void __iomem *qfpconf; + void __iomem *qfpcorrected; + struct device *dev; + struct clk *secclk; + struct regulator *vcc; + struct qfprom_efuse_platform_data efuse; }; +/* + * restore the gcc_sec_ctrl_clk frequency to default value(19.2 MHz) + */ +static int qfprom_reset_clock_settings(const struct qfprom_priv *priv) +{ + int ret; + + ret = clk_set_rate(priv->secclk, priv->efuse.qfprom_blow_reset_freq); + if (ret) { + dev_err(priv->dev, "clk_set_rate() failed to enable secclk\n"); + return ret; + } + + return 0; +} + +/* + * set the gcc_sec_ctrl_clk to 4.8 MHz + */ +static int qfprom_set_clock_settings(const struct qfprom_priv *priv) +{ + int ret; + + ret = clk_set_rate(priv->secclk, priv->efuse.qfprom_blow_set_freq); + if (ret) { + dev_err(priv->dev, "clk_set_rate() failed to enable secclk\n"); + return ret; + } + + return 0; +} + +/* + * set and reset the voltage for 1.8V and OFF(0V) on VDD_QFPROM (LDO11) + */ +static int qfprom_set_voltage_settings(const struct qfprom_priv *priv, + int min_uV, int max_uV) +{ + int ret; + + ret = regulator_set_voltage(priv->vcc, min_uV, max_uV); + if (ret) { + dev_err(priv->dev, "regulator_set_voltage() failed!\n"); + return ret; + } + + ret = regulator_enable(priv->vcc); + if (ret) { + dev_err(priv->dev, "failed to enable regulator\n"); + return ret; + } + + return 0; +} + +/* + * resets the value of the blow timer, accel register and the clock + * and voltage settings + */ +static int qfprom_disable_fuse_blowing(const struct qfprom_priv *priv) +{ + int ret; + + ret = qfprom_set_voltage_settings(priv, 0, priv->efuse.qfprom_max_vol); + if (ret) { + dev_err(priv->dev, "qfprom_set_voltage_settings failed\n"); + return ret; + } + + ret = qfprom_reset_clock_settings(priv); + if (ret) { + dev_err(priv->dev, "qfprom_reset_clock_settings failed\n"); + return ret; + } + + writel(QFPROM_BLOW_TIMER_RESET_VALUE, priv->qfpconf + + QFPROM_BLOW_TIMER_OFFSET); + writel(priv->efuse.accel_reset_value, + priv->qfpconf + QFPROM_ACCEL_OFFSET); + + return 0; +} + +/* + * sets the value of the blow timer, accel register and the clock + * and voltage settings + */ +static int qfprom_enable_fuse_blowing(const struct qfprom_priv *priv) +{ + int ret; + + ret = qfprom_set_clock_settings(priv); + if (ret) { + dev_err(priv->dev, "qpfrom_set_clock_settings()\n"); + return ret; + } + + ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol, + priv->efuse.qfprom_max_vol); + + if (ret) { + dev_err(priv->dev, "qfprom_set_voltage_settings()\n"); + return ret; + } + + writel(priv->efuse.qfprom_blow_timer_value, priv->qfpconf + + QFPROM_BLOW_TIMER_OFFSET); + writel(priv->efuse.accel_value, priv->qfpconf + QFPROM_ACCEL_OFFSET); + + return 0; +} + +/* + * API for writing to raw qfprom region - fuse blowing + * returns success or failure code as per the conditions + */ +static int qfprom_efuse_reg_write(void *context, unsigned int reg, void *_val, + size_t bytes) +{ + struct qfprom_priv *priv = context; + u32 *value = _val; + u32 align_check; + u32 blow_status = QFPROM_BLOW_STATUS_BUSY; + int ret; + int i = 0, words = bytes / 4; + + dev_info(priv->dev, + "writing to raw qfprom region : 0x%08x of size: %zd\n", + reg, bytes); + + if (bytes % 4 != 0x00) { + dev_err(priv->dev, "Bytes: %zd should be word align\n", bytes); + return -EINVAL; + } + + align_check = (reg & 0xF); + if (value && ((align_check & ~3) == align_check)) { + ret = qfprom_enable_fuse_blowing(priv); + if (ret) { + dev_err(priv->dev, "qfprom_enable_fuse_blowing\n"); + return ret; + } + + ret = readl_relaxed_poll_timeout(priv->qfpconf + + QFPROM_BLOW_STATUS_OFFSET, blow_status, + (blow_status != QFPROM_BLOW_STATUS_BUSY), + QFPROM_FUSE_BLOW_POLL_PERIOD, + priv->efuse.fuse_blow_time_in_us); + + if (ret) { + dev_err(priv->dev, "Timeout blow status ready\n"); + return ret; + } + + if (blow_status == QFPROM_BLOW_STATUS_READY) { + while (words--) + writel(*value++, + priv->qfpraw + reg + (i++ * 4)); + + ret = readl_relaxed_poll_timeout(priv->qfpconf + + QFPROM_BLOW_STATUS_OFFSET, blow_status, + (blow_status != QFPROM_BLOW_STATUS_BUSY), + QFPROM_FUSE_BLOW_POLL_PERIOD, + priv->efuse.fuse_blow_time_in_us); + + if (ret) { + dev_err(priv->dev, "Timeout blow-status ready\n"); + return ret; + } + } + + ret = qfprom_disable_fuse_blowing(priv); + if (ret) + return ret; + } else { + dev_err(priv->dev, "Invalid input parameter fuse blow address"); + return -EINVAL; + } + + dev_info(priv->dev, "written successfully raw qfprom region\n"); + + return 0; +} + static int qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes) { @@ -22,47 +271,161 @@ static int qfprom_reg_read(void *context, int i = 0, words = bytes; while (words--) - *val++ = readb(priv->base + reg + i++); + *val++ = readb(priv->qfpraw + reg + i++); return 0; } -static struct nvmem_config econfig = { - .name = "qfprom", - .stride = 1, - .word_size = 1, - .reg_read = qfprom_reg_read, -}; +static int qfprom_reg_write(void *context, + unsigned int reg, void *_val, size_t bytes) +{ + struct qfprom_priv *priv = context; + u8 *val = _val; + int i = 0, words = bytes; + + while (words--) + writeb(*val++, priv->qfpraw + reg + i++); + + return 0; +} static int qfprom_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct resource *res; - struct nvmem_device *nvmem; + struct resource *qfpraw, *qfpconf, *qfpcorrected; + struct nvmem_device *nvmem, *nvmem_efuse; + struct nvmem_config *econfig, *econfig_efuse; struct qfprom_priv *priv; + const struct qfprom_efuse_platform_data *drvdata; + int ret; + + drvdata = of_device_get_match_data(&pdev->dev); + if (!drvdata) + return -EINVAL; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - priv->base = devm_ioremap_resource(dev, res); - if (IS_ERR(priv->base)) - return PTR_ERR(priv->base); + priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us; + priv->efuse.accel_value = drvdata->accel_value; + priv->efuse.accel_reset_value = drvdata->accel_reset_value; + priv->efuse.qfprom_blow_timer_value = drvdata->qfprom_blow_timer_value; + priv->efuse.qfprom_blow_reset_freq = drvdata->qfprom_blow_reset_freq; + priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq; + priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol; + priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol; + priv->dev = dev; + + qfpraw = platform_get_resource_byname(pdev, IORESOURCE_MEM, "raw"); + + priv->qfpraw = devm_ioremap_resource(dev, qfpraw); + if (IS_ERR(priv->qfpraw)) { + ret = PTR_ERR(priv->qfpraw); + goto err; + } + + qfpconf = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf"); + + priv->qfpconf = devm_ioremap_resource(dev, qfpconf); + if (IS_ERR(priv->qfpconf)) { + ret = PTR_ERR(priv->qfpconf); + goto err; + } + + qfpcorrected = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "corrected"); + + priv->qfpcorrected = devm_ioremap_resource(dev, qfpcorrected); + if (IS_ERR(priv->qfpcorrected)) { + ret = PTR_ERR(priv->qfpcorrected); + goto err; + } + + priv->vcc = devm_regulator_get(&pdev->dev, "vcc"); + if (IS_ERR(priv->vcc)) { + ret = PTR_ERR(priv->vcc); + if (ret == -ENODEV) + ret = -EPROBE_DEFER; + goto err; + } - econfig.size = resource_size(res); - econfig.dev = dev; - econfig.priv = priv; + priv->secclk = devm_clk_get(dev, "secclk"); + if (IS_ERR(priv->secclk)) { + ret = PTR_ERR(priv->secclk); + if (ret != -EPROBE_DEFER) + dev_err(dev, "secclk error getting : %d\n", ret); + goto err; + } - nvmem = devm_nvmem_register(dev, &econfig); + ret = clk_prepare_enable(priv->secclk); + if (ret) { + dev_err(dev, "clk_prepare_enable() failed\n"); + goto err; + } - return PTR_ERR_OR_ZERO(nvmem); + econfig_efuse = devm_kzalloc(dev, sizeof(*econfig_efuse), GFP_KERNEL); + if (!econfig_efuse) + return -ENOMEM; + + econfig_efuse->dev = dev; + econfig_efuse->name = "qfprom-efuse"; + econfig_efuse->stride = 1; + econfig_efuse->word_size = 1; + econfig_efuse->reg_read = qfprom_reg_read; + econfig_efuse->reg_write = qfprom_efuse_reg_write; + econfig_efuse->size = resource_size(qfpraw); + econfig_efuse->priv = priv; + + nvmem_efuse = devm_nvmem_register(dev, econfig_efuse); + if (IS_ERR(nvmem_efuse)) { + dev_err(&pdev->dev, "failed to register nvmem config\n"); + return PTR_ERR(nvmem_efuse); + } + + econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL); + if (!econfig) + return -ENOMEM; + + econfig->name = "qfprom", + econfig->stride = 1, + econfig->word_size = 1, + econfig->reg_read = qfprom_reg_read, + econfig->reg_write = qfprom_reg_write, + econfig->size = resource_size(qfpcorrected); + econfig->dev = dev; + econfig->priv = priv; + + nvmem = devm_nvmem_register(dev, econfig); + if (IS_ERR(nvmem)) { + dev_err(&pdev->dev, "failed to register nvmem config\n"); + return PTR_ERR(nvmem); + } + +err: + clk_disable_unprepare(priv->secclk); + return ret; } +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = { + .name = "qfprom", + .fuse_blow_time_in_us = 10, + .accel_value = 0xD10, + .accel_reset_value = 0x800, + .qfprom_blow_timer_value = 25, + .qfprom_blow_reset_freq = 19200000, + .qfprom_blow_set_freq = 4800000, + .qfprom_max_vol = 1904000, + .qfprom_min_vol = 1800000, +}; + static const struct of_device_id qfprom_of_match[] = { - { .compatible = "qcom,qfprom",}, + { .compatible = "qcom,qfprom", + .data = &sc7180_qfp_efuse_data + }, {/* sentinel */}, }; + MODULE_DEVICE_TABLE(of, qfprom_of_match); static struct platform_driver qfprom_driver = { -- Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support 2020-06-11 9:48 ` [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support Ravi Kumar Bokka @ 2020-06-13 20:33 ` Doug Anderson 2020-06-15 10:44 ` Srinivas Kandagatla 2020-06-17 14:54 ` Doug Anderson 2020-06-15 9:56 ` Srinivas Kandagatla 1 sibling, 2 replies; 18+ messages in thread From: Doug Anderson @ 2020-06-13 20:33 UTC (permalink / raw) To: Ravi Kumar Bokka Cc: Srinivas Kandagatla, Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel Hi, On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote: > > This patch adds support for QTI qfprom-efuse controller. This driver can > access the raw qfprom regions for fuse blowing. > > The current existed qfprom driver is only supports for cpufreq, thermal sensors > drivers by read out calibration data, speed bins..etc which is stored > by qfprom efuses. > > Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org> > --- It makes all your code reviewers much happier (and much more likely to take the time to review your patches) if you include a changelog with what changed from one version to the next. If you would like some help maintaining such a thing, might I suggest "patman": https://gitlab.denx.de/u-boot/u-boot/-/blob/master/tools/patman/README ...pay no mind that it's hosted in the U-Boot repo--it's really quite a great tool. > drivers/nvmem/Kconfig | 1 + > drivers/nvmem/qfprom.c | 405 ++++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 385 insertions(+), 21 deletions(-) > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig > index d7b7f6d..623d59e 100644 > --- a/drivers/nvmem/Kconfig > +++ b/drivers/nvmem/Kconfig > @@ -117,6 +117,7 @@ config QCOM_QFPROM > help > Say y here to enable QFPROM support. The QFPROM provides access > functions for QFPROM data to rest of the drivers via nvmem interface. > + And this driver provides access QTI qfprom efuse via nvmem interface. I'm not sure it was necessary to add that line, but I won't object if you/others really like it. > This driver can also be built as a module. If so, the module > will be called nvmem_qfprom. > diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c > index 8a91717..312318c 100644 > --- a/drivers/nvmem/qfprom.c > +++ b/drivers/nvmem/qfprom.c You've still mostly not addressed most of the review feedback I've now given you 3 times. Rather than repeating comments, I have simply provided a patch that makes the driver into a state that I'm happy with: https://crrev.com/c/2244932 Rough summary: * There should be no reason to provide "reset" values for things. For anything that you change for fuse blowing, just save and restore after. * Use the major/minor version read from 0x6000 to pick the parameters to use. Please double-check that I got this right. * Reading should still read "corrected", not "raw". Added a sysfs knob to allow you to read "raw", though. * Simplified the SoC data structure. * No need for quite so many levels of abstraction for setting clocks / regulator. * Don't set regulator voltage. Rely on device tree to make sure it's right. * Properly undo things in the case of failure. * Don't just keep enabling the regulator over and over again. * Enable / disable the clock each time; now we don't need a .remove function and yet we still don't leave the clock enabled/prepared. * Polling every 100 us but timing out in 10 us didn't make sense. Swap those. Also no reason for 100 us to be SoC specific. * No need for reg-names. * We shouldn't be creating two separate nvmem devices. In general I'm happy to post my series to the list myself to get review feedback. For now I'm expecting that you can squash my changes in and send the next version. -Doug ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support 2020-06-13 20:33 ` Doug Anderson @ 2020-06-15 10:44 ` Srinivas Kandagatla 2020-06-15 17:13 ` Doug Anderson 2020-06-17 14:54 ` Doug Anderson 1 sibling, 1 reply; 18+ messages in thread From: Srinivas Kandagatla @ 2020-06-15 10:44 UTC (permalink / raw) To: Doug Anderson, Ravi Kumar Bokka Cc: Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel On 13/06/2020 21:33, Doug Anderson wrote: > Hi, > > On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote: >> >> This patch adds support for QTI qfprom-efuse controller. This driver can >> access the raw qfprom regions for fuse blowing. >> >> The current existed qfprom driver is only supports for cpufreq, thermal sensors >> drivers by read out calibration data, speed bins..etc which is stored >> by qfprom efuses. >> >> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org> >> --- > > It makes all your code reviewers much happier (and much more likely to > take the time to review your patches) if you include a changelog with > what changed from one version to the next. If you would like some > help maintaining such a thing, might I suggest "patman": > > https://gitlab.denx.de/u-boot/u-boot/-/blob/master/tools/patman/README > > ...pay no mind that it's hosted in the U-Boot repo--it's really quite > a great tool. > > >> drivers/nvmem/Kconfig | 1 + >> drivers/nvmem/qfprom.c | 405 ++++++++++++++++++++++++++++++++++++++++++++++--- >> 2 files changed, 385 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig >> index d7b7f6d..623d59e 100644 >> --- a/drivers/nvmem/Kconfig >> +++ b/drivers/nvmem/Kconfig >> @@ -117,6 +117,7 @@ config QCOM_QFPROM >> help >> Say y here to enable QFPROM support. The QFPROM provides access >> functions for QFPROM data to rest of the drivers via nvmem interface. >> + And this driver provides access QTI qfprom efuse via nvmem interface. > > I'm not sure it was necessary to add that line, but I won't object if > you/others really like it. > NAK from my side! > >> This driver can also be built as a module. If so, the module >> will be called nvmem_qfprom. >> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c >> index 8a91717..312318c 100644 >> --- a/drivers/nvmem/qfprom.c >> +++ b/drivers/nvmem/qfprom.c > > You've still mostly not addressed most of the review feedback I've now > given you 3 times. Rather than repeating comments, I have simply > provided a patch that makes the driver into a state that I'm happy > with: > > https://crrev.com/c/2244932 > > Rough summary: > > * There should be no reason to provide "reset" values for things. For > anything that you change for fuse blowing, just save and restore > after. > > * Use the major/minor version read from 0x6000 to pick the parameters > to use. Please double-check that I got this right. > > * Reading should still read "corrected", not "raw". Added a sysfs > knob to allow you to read "raw", though. We could create an additional nvmem read-only provider in future if required to read raw!. > > * Simplified the SoC data structure. > > * No need for quite so many levels of abstraction for setting clocks / > regulator. > > * Don't set regulator voltage. Rely on device tree to make sure it's right. > > * Properly undo things in the case of failure. > > * Don't just keep enabling the regulator over and over again. > > * Enable / disable the clock each time; now we don't need a .remove > function and yet we still don't leave the clock enabled/prepared. > > * Polling every 100 us but timing out in 10 us didn't make sense. > Swap those. Also no reason for 100 us to be SoC specific. > > * No need for reg-names. > > * We shouldn't be creating two separate nvmem devices. > > > In general I'm happy to post my series to the list myself to get > review feedback. For now I'm expecting that you can squash my changes > in and send the next version. > > > -Doug > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support 2020-06-15 10:44 ` Srinivas Kandagatla @ 2020-06-15 17:13 ` Doug Anderson 0 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-06-15 17:13 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Ravi Kumar Bokka, Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel Hi, On Mon, Jun 15, 2020 at 3:44 AM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > * Reading should still read "corrected", not "raw". Added a sysfs > > knob to allow you to read "raw", though. > > We could create an additional nvmem read-only provider in future if > required to read raw!. Makes sense. For now module parameter (which can be tweaked from sysfs) seemed like at least an easy to get access to the raw values for testing. Other than for checking that the driver works OK, though, I'm under the impression that you should never read the raw values back. -Doug ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support 2020-06-13 20:33 ` Doug Anderson 2020-06-15 10:44 ` Srinivas Kandagatla @ 2020-06-17 14:54 ` Doug Anderson 1 sibling, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-06-17 14:54 UTC (permalink / raw) To: Ravi Kumar Bokka Cc: Srinivas Kandagatla, Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel Hi, On Sat, Jun 13, 2020 at 1:33 PM Doug Anderson <dianders@chromium.org> wrote: > > In general I'm happy to post my series to the list myself to get > review feedback. For now I'm expecting that you can squash my changes > in and send the next version. I talked with Ravi offline and he suggested that I send out the v3, so it's been posted. For the cover letter, see: https://lore.kernel.org/r/20200617145116.247432-1-dianders@chromium.org -Doug ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support 2020-06-11 9:48 ` [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support Ravi Kumar Bokka 2020-06-13 20:33 ` Doug Anderson @ 2020-06-15 9:56 ` Srinivas Kandagatla 1 sibling, 0 replies; 18+ messages in thread From: Srinivas Kandagatla @ 2020-06-15 9:56 UTC (permalink / raw) To: Ravi Kumar Bokka, Rob Herring Cc: linux-kernel, devicetree, rnayak, saiprakash.ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel I think Doug already covered most of the comments and his fixes seems be in right direction. On 11/06/2020 10:48, Ravi Kumar Bokka wrote: > + qfpraw = platform_get_resource_byname(pdev, IORESOURCE_MEM, "raw"); > + > + priv->qfpraw = devm_ioremap_resource(dev, qfpraw); > + if (IS_ERR(priv->qfpraw)) { General comment for up-streaming is that your patch should not break whats in mainline, Your patch is totally ignoring!! Please be mindful while doing changes to drivers which are used by other platforms. --srini > + ret = PTR_ERR(priv->qfpraw); > + goto err; > + } > + > + qfpconf = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf"); > + > + priv->qfpconf = devm_ioremap_resource(dev, qfpconf); > + if (IS_ERR(priv->qfpconf)) { > + ret = PTR_ERR(priv->qfpconf); > + goto err; > + } > + > + qfpcorrected = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "corrected"); > + > + priv->qfpcorrected = devm_ioremap_resource(dev, qfpcorrected); > + if (IS_ERR(priv->qfpcorrected)) { > + ret = PTR_ERR(priv->qfpcorrected); > + goto err; > + } ^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse 2020-06-11 9:47 [RFC v2 0/3] Add QTI QFPROM-Efuse driver support Ravi Kumar Bokka 2020-06-11 9:48 ` [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse Ravi Kumar Bokka 2020-06-11 9:48 ` [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support Ravi Kumar Bokka @ 2020-06-11 9:48 ` Ravi Kumar Bokka 2020-06-13 20:23 ` Doug Anderson 2 siblings, 1 reply; 18+ messages in thread From: Ravi Kumar Bokka @ 2020-06-11 9:48 UTC (permalink / raw) To: Srinivas Kandagatla, Rob Herring Cc: linux-kernel, devicetree, rnayak, saiprakash.ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel, Ravi Kumar Bokka This patch adds device tree node for qfprom-efuse controller. Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org> --- arch/arm64/boot/dts/qcom/sc7180-idp.dts | 4 ++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 10 ++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts index 4e9149d..2a9224e 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts @@ -287,6 +287,10 @@ }; }; +&qfprom { + vcc-supply = <&vreg_l11a_1p8>; +}; + &qspi { status = "okay"; pinctrl-names = "default"; diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index 31b9217..20f3480 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -498,9 +498,15 @@ #power-domain-cells = <1>; }; - qfprom@784000 { + qfprom: qfprom@780000 { compatible = "qcom,qfprom"; - reg = <0 0x00784000 0 0x8ff>; + 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"; #address-cells = <1>; #size-cells = <1>; -- Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation. ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse 2020-06-11 9:48 ` [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse Ravi Kumar Bokka @ 2020-06-13 20:23 ` Doug Anderson 0 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-06-13 20:23 UTC (permalink / raw) To: Ravi Kumar Bokka Cc: Srinivas Kandagatla, Rob Herring, LKML, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate, c_rbokka, mkurumel Hi, On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote: > > This patch adds device tree node for qfprom-efuse controller. > > Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 4 ++++ > arch/arm64/boot/dts/qcom/sc7180.dtsi | 10 ++++++++-- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > index 4e9149d..2a9224e 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > @@ -287,6 +287,10 @@ > }; > }; > > +&qfprom { > + vcc-supply = <&vreg_l11a_1p8>; > +}; > + > &qspi { > status = "okay"; > pinctrl-names = "default"; > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi > index 31b9217..20f3480 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -498,9 +498,15 @@ > #power-domain-cells = <1>; > }; > > - qfprom@784000 { > + qfprom: qfprom@780000 { The dt schema checker claims that your node should actually be called: eeprom|efuse|nvram So you probably want the above to actually be: qfprom: efuse@780000 The label to the side doesn't matter and so it can stay qfprom--just the node name itself is what the checker cares about. > compatible = "qcom,qfprom"; As per my response in the bindings, this should be: "qcom,sc7180-qfprom", "qcom,qfprom" ...even if the driver only ever makes use of "qcom,qfprom" this future-proofs us a bit. > - reg = <0 0x00784000 0 0x8ff>; > + reg = <0 0x00780000 0 0x7a0>, > + <0 0x00782000 0 0x100>, > + <0 0x00784000 0 0x8ff>; > + reg-names = "raw", "conf", "corrected"; As per my response in the bindings, reg-names is discouraged so you should remove and make it so that the driver doesn't need. > + It's hard to tell in email, but checkpatch yells above the above line: ERROR: trailing whitespace #53: FILE: arch/arm64/boot/dts/qcom/sc7180.dtsi:512: +^I^I^I$ > + clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>; > + clock-names = "secclk"; As per the binding spec, clock name shouldn't end in "clk". For your edification, I provided a patch to fix all my review feedback at: https://crrev.com/c/2244933 Feel free to squash it into your next version. -Doug ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-06-17 14:55 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-11 9:47 [RFC v2 0/3] Add QTI QFPROM-Efuse driver support Ravi Kumar Bokka 2020-06-11 9:48 ` [RFC v2 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse Ravi Kumar Bokka 2020-06-11 12:18 ` Rajendra Nayak 2020-06-12 21:58 ` Rob Herring 2020-06-12 21:59 ` Doug Anderson 2020-06-12 23:15 ` Doug Anderson 2020-06-15 10:44 ` Srinivas Kandagatla 2020-06-15 14:17 ` Rob Herring 2020-06-15 14:27 ` Doug Anderson 2020-06-15 14:28 ` Srinivas Kandagatla 2020-06-11 9:48 ` [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support Ravi Kumar Bokka 2020-06-13 20:33 ` Doug Anderson 2020-06-15 10:44 ` Srinivas Kandagatla 2020-06-15 17:13 ` Doug Anderson 2020-06-17 14:54 ` Doug Anderson 2020-06-15 9:56 ` Srinivas Kandagatla 2020-06-11 9:48 ` [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse Ravi Kumar Bokka 2020-06-13 20:23 ` Doug Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).