Re: [PATCH v7 1/5] dt-bindings: power: avs: Add support for CPR (Core Power Reduction)

[Niklas Cassel]

On Fri, Dec 20, 2019 at 10:31:53AM +0100, Rafael J. Wysocki wrote:
> On Friday, November 29, 2019 10:39:11 PM CET Niklas Cassel wrote:
> > Add DT bindings to describe the CPR HW found on certain Qualcomm SoCs.
> > 
> > Co-developed-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@xxxxxxxxxx>
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@xxxxxxxxxx>
> > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxxxxx>
> > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > ---
> > Changes since v6:
> > -Picked up Bjorn's and Ulf's Reviewed-by.
> > 
> >  .../bindings/power/avs/qcom,cpr.txt           | 130 ++++++++++++++++++
> >  1 file changed, 130 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/avs/qcom,cpr.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/power/avs/qcom,cpr.txt b/Documentation/devicetree/bindings/power/avs/qcom,cpr.txt
> > new file mode 100644
> > index 000000000000..ab0d5ebbad4e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/avs/qcom,cpr.txt
> > @@ -0,0 +1,130 @@
> > +QCOM CPR (Core Power Reduction)
> > +
> > +CPR (Core Power Reduction) is a technology to reduce core power on a CPU
> > +or other device. Each OPP of a device corresponds to a "corner" that has
> > +a range of valid voltages for a particular frequency. While the device is
> > +running at a particular frequency, CPR monitors dynamic factors such as
> > +temperature, etc. and suggests adjustments to the voltage to save power
> > +and meet silicon characteristic requirements.
> > +
> > +- compatible:
> > +	Usage: required
> > +	Value type: <string>
> > +	Definition: should be "qcom,qcs404-cpr", "qcom,cpr" for qcs404
> > +
> > +- reg:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: base address and size of the rbcpr register region
> > +
> > +- interrupts:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: should specify the CPR interrupt
> > +
> > +- clocks:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: phandle to the reference clock
> > +
> > +- clock-names:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: must be "ref"
> > +
> > +- vdd-apc-supply:
> > +	Usage: required
> > +	Value type: <phandle>
> > +	Definition: phandle to the vdd-apc-supply regulator
> > +
> > +- #power-domain-cells:
> > +	Usage: required
> > +	Value type: <u32>
> > +	Definition: should be 0
> > +
> > +- operating-points-v2:
> > +	Usage: required
> > +	Value type: <phandle>
> > +	Definition: A phandle to the OPP table containing the
> > +		    performance states supported by the CPR
> > +		    power domain
> > +
> > +- acc-syscon:
> > +	Usage: optional
> > +	Value type: <phandle>
> > +	Definition: phandle to syscon for writing ACC settings
> > +
> > +- nvmem-cells:
> > +	Usage: required
> > +	Value type: <phandle>
> > +	Definition: phandle to nvmem cells containing the data
> > +		    that makes up a fuse corner, for each fuse corner.
> > +		    As well as the CPR fuse revision.
> > +
> > +- nvmem-cell-names:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: should be "cpr_quotient_offset1", "cpr_quotient_offset2",
> > +		    "cpr_quotient_offset3", "cpr_init_voltage1",
> > +		    "cpr_init_voltage2", "cpr_init_voltage3", "cpr_quotient1",
> > +		    "cpr_quotient2", "cpr_quotient3", "cpr_ring_osc1",
> > +		    "cpr_ring_osc2", "cpr_ring_osc3", "cpr_fuse_revision"
> > +		    for qcs404.
> > +
> > +Example:
> > +
> > +	cpr_opp_table: cpr-opp-table {
> > +		compatible = "operating-points-v2-qcom-level";
> > +
> > +		cpr_opp1: opp1 {
> > +			opp-level = <1>;
> > +			qcom,opp-fuse-level = <1>;
> > +		};
> > +		cpr_opp2: opp2 {
> > +			opp-level = <2>;
> > +			qcom,opp-fuse-level = <2>;
> > +		};
> > +		cpr_opp3: opp3 {
> > +			opp-level = <3>;
> > +			qcom,opp-fuse-level = <3>;
> > +		};
> > +	};
> > +
> > +	power-controller@b018000 {
> > +		compatible = "qcom,qcs404-cpr", "qcom,cpr";
> > +		reg = <0x0b018000 0x1000>;
> > +		interrupts = <0 15 IRQ_TYPE_EDGE_RISING>;
> > +		clocks = <&xo_board>;
> > +		clock-names = "ref";
> > +		vdd-apc-supply = <&pms405_s3>;
> > +		#power-domain-cells = <0>;
> > +		operating-points-v2 = <&cpr_opp_table>;
> > +		acc-syscon = <&tcsr>;
> > +
> > +		nvmem-cells = <&cpr_efuse_quot_offset1>,
> > +			<&cpr_efuse_quot_offset2>,
> > +			<&cpr_efuse_quot_offset3>,
> > +			<&cpr_efuse_init_voltage1>,
> > +			<&cpr_efuse_init_voltage2>,
> > +			<&cpr_efuse_init_voltage3>,
> > +			<&cpr_efuse_quot1>,
> > +			<&cpr_efuse_quot2>,
> > +			<&cpr_efuse_quot3>,
> > +			<&cpr_efuse_ring1>,
> > +			<&cpr_efuse_ring2>,
> > +			<&cpr_efuse_ring3>,
> > +			<&cpr_efuse_revision>;
> > +		nvmem-cell-names = "cpr_quotient_offset1",
> > +			"cpr_quotient_offset2",
> > +			"cpr_quotient_offset3",
> > +			"cpr_init_voltage1",
> > +			"cpr_init_voltage2",
> > +			"cpr_init_voltage3",
> > +			"cpr_quotient1",
> > +			"cpr_quotient2",
> > +			"cpr_quotient3",
> > +			"cpr_ring_osc1",
> > +			"cpr_ring_osc2",
> > +			"cpr_ring_osc3",
> > +			"cpr_fuse_revision";
> > +	};
> > 
> 
> I have queued up this one and the [2/5] for 5.6, but if you'd rather want them
> to go in via a different patch, please let me know and I'll drop them.
> 

Thanks a lot Rafael!

I would very much prefer them to go via your tree.

Unfortunately it seems like kbuild test robot
found an incorrect printk format specifier in
one of the debug prints.

Line 838
dev_dbg(dev, "efuse read(%s) = %x, bytes %ld\n", cname, *data, len);

should be
dev_dbg(dev, "efuse read(%s) = %x, bytes %zd\n", cname, *data, len);

So %zd rather than %ld.

This was obviously an error, but didn't show when
compiling on arm64 or x86_64.

Sorry for this inconvenience.

Could you fix up the commit or do I need to do a respin?


Kind regards,
Niklas