[PATCH V2 0/6] Add regulator support for IPQ9574 SoC

[PATCH V2 0/6] Add regulator support for IPQ9574 SoC

[Devi Priya]

IPQ9574 SoC uses the PMIC MP5496 and SMPA1 regulator for
APSS voltage scaling.
This patch series adds the support for the same and also
enables the RPM communication over the RPMSG framework

DTS patch depends on the below series
https://lore.kernel.org/linux-arm-kernel/20230217134107.13946-1-quic_devipriy@quicinc.com/

[V2]:
	- Reordered the patches to have the bindings and driver changes
	  in place before the device tree support
	- Dropped the 'soc: qcom: smd-rpm: Add IPQ9574 compatible' since
	  it is already part of the linux-next/master
	- Detailed change log is added to the respective patches
[V1]:
	https://lore.kernel.org/linux-arm-msm/20230113150310.29709-1-quic_devipriy@quicinc.com/

Devi Priya (6):
  dt-bindings: soc: qcom: smd-rpm: Add IPQ9574 compatible
  regulator: qcom_smd: Add s1 sub-node to mp5496 regulator
  regulator: qcom_smd: Add MP5496 S1 regulator
  regulator: qcom_smd: Add support to define the bootup voltage
  arm64: dts: qcom: ipq9574: Add RPM related nodes
  arm64: dts: qcom: ipq9574: Add cpufreq support

 .../regulator/qcom,smd-rpm-regulator.yaml     |  2 +-
 .../bindings/soc/qcom/qcom,smd-rpm.yaml       |  1 +
 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts  | 11 +++
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         | 75 +++++++++++++++++++
 drivers/regulator/qcom_smd-regulator.c        | 18 ++++-
 5 files changed, 104 insertions(+), 3 deletions(-)


base-commit: c068f40300a0eaa34f7105d137a5560b86951aa9
-- 
2.17.1

[PATCH V2 1/6] dt-bindings: soc: qcom: smd-rpm: Add IPQ9574 compatible

[Devi Priya]

Document the compatible for RPM communication
over SMD for IPQ9574 SoC

Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 Changes in V2:
	- Dropped the Co-developed-by tag

 Documentation/devicetree/bindings/soc/qcom/qcom,smd-rpm.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smd-rpm.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smd-rpm.yaml
index 16fd67c0bd1f..2a5791ebc8dd 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,smd-rpm.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smd-rpm.yaml
@@ -33,6 +33,7 @@ properties:
     enum:
       - qcom,rpm-apq8084
       - qcom,rpm-ipq6018
+      - qcom,rpm-ipq9574
       - qcom,rpm-msm8226
       - qcom,rpm-msm8909
       - qcom,rpm-msm8916
-- 
2.17.1

[PATCH V2 2/6] regulator: qcom_smd: Add s1 sub-node to mp5496 regulator

[Devi Priya]

Adding s1 sub-node entry to mp5496 regulator node

Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 Changes in V2:
	- Dropped ipq9574 compatible as the existing MP5496 support is
	  extended for S1 regulator

 .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
index 8c45f53212b1..a8ca8e0b27f8 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
@@ -22,7 +22,7 @@ description:
   Each sub-node is identified using the node's name, with valid values listed
   for each of the pmics below.
 
-  For mp5496, s2
+  For mp5496, s1, s2
 
   For pm2250, s1, s2, s3, s4, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11,
   l12, l13, l14, l15, l16, l17, l18, l19, l20, l21, l22
-- 
2.17.1

[PATCH V2 3/6] regulator: qcom_smd: Add MP5496 S1 regulator

[Devi Priya]

Adding support for MP5496 S1 regulator on IPQ9574 SoC

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 Changes in V2:
	- Extended the existing MP5496 support with S1 regulator
	  and dropped the ipq9574 compatible

 drivers/regulator/qcom_smd-regulator.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index 9f2b58458841..a40e66cea7e7 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -749,6 +749,15 @@ static const struct regulator_desc pms405_pldo600 = {
 	.ops = &rpm_smps_ldo_ops,
 };
 
+static const struct regulator_desc mp5496_smpa1 = {
+	.linear_ranges = (struct linear_range[]) {
+		REGULATOR_LINEAR_RANGE(600000, 0, 127, 12500),
+	},
+	.n_linear_ranges = 1,
+	.n_voltages = 128,
+	.ops = &rpm_mp5496_ops,
+};
+
 static const struct regulator_desc mp5496_smpa2 = {
 	.linear_ranges = (struct linear_range[]) {
 		REGULATOR_LINEAR_RANGE(600000, 0, 127, 12500),
@@ -794,6 +803,7 @@ struct rpm_regulator_data {
 };
 
 static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
+	{ "s1", QCOM_SMD_RPM_SMPA, 1, &mp5496_smpa1, "s1" },
 	{ "s2", QCOM_SMD_RPM_SMPA, 2, &mp5496_smpa2, "s2" },
 	{ "l2", QCOM_SMD_RPM_LDOA, 2, &mp5496_ldoa2, "l2" },
 	{}
-- 
2.17.1

[PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Devi Priya]

Kernel does not know the initial voltage set by the bootloaders.
During regulator registration, the voltage variable is just declared
and it is zero. Based on that, the regulator framework considers current
the voltage as zero and tries to bring up each regulator to minimum
the supported voltage.

This introduces a dip in the voltage during kernel boot and gets
stabilized once the voltage scaling comes into picture.

To avoid the voltage dip, adding support to define the
bootup voltage set by the boodloaders and based on it, regulator
framework understands that proper voltage is already set

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 Changes in V2:
	- Added the bootup voltages to s2 and l2 regulators

 drivers/regulator/qcom_smd-regulator.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index a40e66cea7e7..5f9fe6b9d368 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -800,12 +800,13 @@ struct rpm_regulator_data {
 	u32 id;
 	const struct regulator_desc *desc;
 	const char *supply;
+	int boot_uV; /* To store the bootup voltage set by bootloaders */
 };
 
 static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
-	{ "s1", QCOM_SMD_RPM_SMPA, 1, &mp5496_smpa1, "s1" },
-	{ "s2", QCOM_SMD_RPM_SMPA, 2, &mp5496_smpa2, "s2" },
-	{ "l2", QCOM_SMD_RPM_LDOA, 2, &mp5496_ldoa2, "l2" },
+	{ "s1", QCOM_SMD_RPM_SMPA, 1, &mp5496_smpa1, "s1", 875000  },
+	{ "s2", QCOM_SMD_RPM_SMPA, 2, &mp5496_smpa2, "s2", 875000  },
+	{ "l2", QCOM_SMD_RPM_LDOA, 2, &mp5496_ldoa2, "l2", 2950000 },
 	{}
 };
 
@@ -1388,6 +1389,9 @@ static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev
 	vreg->type	= rpm_data->type;
 	vreg->id	= rpm_data->id;
 
+	if (rpm_data->boot_uV)
+		vreg->uV = rpm_data->boot_uV;
+
 	memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc));
 	vreg->desc.name = rpm_data->name;
 	vreg->desc.supply_name = rpm_data->supply;
-- 
2.17.1

[PATCH V2 5/6] arm64: dts: qcom: ipq9574: Add RPM related nodes

[Devi Priya]

Add RPM Glink, RPM message RAM and SMPA1 regulator
nodes to support frequency scaling on IPQ9574

Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 Changes in V2:
	- Splitted the RPM and CPU Freq changes to individual patches
	- Moved the regulators node to Board DT
	- Dropped the regulator-always-on property
	- Updated the compatible in regulators node with the existing
	  mp5496 compatible

 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts | 11 +++++++++++
 arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 17 +++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
index 21b53f34ce84..8a6caaeb0c4b 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
+++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
@@ -57,6 +57,17 @@
 	status = "okay";
 };
 
+&rpm_requests {
+	regulators {
+		compatible = "qcom,rpm-mp5496-regulators";
+
+		ipq9574_s1: s1 {
+			regulator-min-microvolt = <587500>;
+			regulator-max-microvolt = <1075000>;
+		};
+	};
+};
+
 &sdhc_1 {
 	pinctrl-0 = <&sdc_default_state>;
 	pinctrl-names = "default";
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index d20f3c7383f5..2f300cbab93e 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -133,6 +133,11 @@
 		#size-cells = <2>;
 		ranges;
 
+		rpm_msg_ram: rpm@60000 {
+			reg = <0x0 0x00060000 0x0 0x6000>;
+			no-map;
+		};
+
 		tz_region: tz@4a600000 {
 			reg = <0x0 0x4a600000 0x0 0x400000>;
 			no-map;
@@ -768,6 +773,18 @@
 		};
 	};
 
+	rpm-glink {
+		compatible = "qcom,glink-rpm";
+		interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
+		qcom,rpm-msg-ram = <&rpm_msg_ram>;
+		mboxes = <&apcs_glb 0>;
+
+		rpm_requests: glink-channel {
+			compatible = "qcom,rpm-ipq9574";
+			qcom,glink-channels = "rpm_requests";
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
-- 
2.17.1

[PATCH V2 6/6] arm64: dts: qcom: ipq9574: Add cpufreq support

[Devi Priya]

Add cpu freq nodes in the device tree to bump
cpu frequency above 800MHz

Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 Changes in V2:
	- Created a separate patch for CPU freq related nodes
	- Updated cpu0-supply to cpu-supply in CPU@0 node
	- Moved the opp-table-cpu node after memory to have it sorted
	  alphabetically
	- Added a new line between the subnodes in opp-table-cpu

 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 58 +++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 2f300cbab93e..2bb4053641da 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -6,6 +6,7 @@
  * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
+#include <dt-bindings/clock/qcom,apss-ipq.h>
 #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/reset/qcom,ipq9574-gcc.h>
@@ -73,6 +74,10 @@
 			reg = <0x0>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			clocks = <&apcs_glb APCS_ALIAS0_CORE_CLK>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu_opp_table>;
+			cpu-supply = <&ipq9574_s1>;
 		};
 
 		CPU1: cpu@1 {
@@ -81,6 +86,10 @@
 			reg = <0x1>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			clocks = <&apcs_glb APCS_ALIAS0_CORE_CLK>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu_opp_table>;
+			cpu-supply = <&ipq9574_s1>;
 		};
 
 		CPU2: cpu@2 {
@@ -89,6 +98,10 @@
 			reg = <0x2>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			clocks = <&apcs_glb APCS_ALIAS0_CORE_CLK>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu_opp_table>;
+			cpu-supply = <&ipq9574_s1>;
 		};
 
 		CPU3: cpu@3 {
@@ -97,6 +110,10 @@
 			reg = <0x3>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			clocks = <&apcs_glb APCS_ALIAS0_CORE_CLK>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu_opp_table>;
+			cpu-supply = <&ipq9574_s1>;
 		};
 
 		L2_0: l2-cache {
@@ -111,6 +128,47 @@
 		reg = <0x0 0x40000000 0x0 0x0>;
 	};
 
+	cpu_opp_table: opp-table-cpu {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp-936000000 {
+			opp-hz = /bits/ 64 <936000000>;
+			opp-microvolt = <725000>;
+			clock-latency-ns = <200000>;
+		};
+
+		opp-1104000000 {
+			opp-hz = /bits/ 64 <1104000000>;
+			opp-microvolt = <787500>;
+			clock-latency-ns = <200000>;
+		};
+
+		opp-1416000000 {
+			opp-hz = /bits/ 64 <1416000000>;
+			opp-microvolt = <862500>;
+			clock-latency-ns = <200000>;
+		};
+
+		opp-1488000000 {
+			opp-hz = /bits/ 64 <1488000000>;
+			opp-microvolt = <925000>;
+			clock-latency-ns = <200000>;
+		};
+
+		opp-1800000000 {
+			opp-hz = /bits/ 64 <1800000000>;
+			opp-microvolt = <987500>;
+			clock-latency-ns = <200000>;
+		};
+
+		opp-2208000000 {
+			opp-hz = /bits/ 64 <2208000000>;
+			opp-microvolt = <1062500>;
+			clock-latency-ns = <200000>;
+		};
+	};
+
 	firmware {
 		scm {
 			compatible = "qcom,scm-ipq9574", "qcom,scm";
-- 
2.17.1

Re: [PATCH V2 5/6] arm64: dts: qcom: ipq9574: Add RPM related nodes

[Konrad Dybcio]

On 17.02.2023 15:20, Devi Priya wrote:
> Add RPM Glink, RPM message RAM and SMPA1 regulator
> nodes to support frequency scaling on IPQ9574
> 
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---
>  Changes in V2:
> 	- Splitted the RPM and CPU Freq changes to individual patches
> 	- Moved the regulators node to Board DT
> 	- Dropped the regulator-always-on property
> 	- Updated the compatible in regulators node with the existing
> 	  mp5496 compatible
> 
>  arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts | 11 +++++++++++
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 17 +++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> index 21b53f34ce84..8a6caaeb0c4b 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> @@ -57,6 +57,17 @@
>  	status = "okay";
>  };
>  
> +&rpm_requests {
> +	regulators {
> +		compatible = "qcom,rpm-mp5496-regulators";
> +
> +		ipq9574_s1: s1 {
> +			regulator-min-microvolt = <587500>;
> +			regulator-max-microvolt = <1075000>;
> +		};
> +	};
> +};
This belongs in a separate patch.

> +
>  &sdhc_1 {
>  	pinctrl-0 = <&sdc_default_state>;
>  	pinctrl-names = "default";
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index d20f3c7383f5..2f300cbab93e 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -133,6 +133,11 @@
>  		#size-cells = <2>;
>  		ranges;
>  
> +		rpm_msg_ram: rpm@60000 {
Since this is a part of the MMIO region and not a part of DRAM,
we generally put this node under /soc with the compatible of
qcom,rpm-msg-ram and without no-map.

And the node name then should be sram@.

> +			reg = <0x0 0x00060000 0x0 0x6000>;
> +			no-map;
> +		};
> +
>  		tz_region: tz@4a600000 {
>  			reg = <0x0 0x4a600000 0x0 0x400000>;
>  			no-map;
> @@ -768,6 +773,18 @@
>  		};
>  	};
>  
> +	rpm-glink {
Alphabetically this should come before /soc.

Konrad
> +		compatible = "qcom,glink-rpm";
> +		interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
> +		qcom,rpm-msg-ram = <&rpm_msg_ram>;
> +		mboxes = <&apcs_glb 0>;
> +
> +		rpm_requests: glink-channel {
> +			compatible = "qcom,rpm-ipq9574";
> +			qcom,glink-channels = "rpm_requests";
> +		};
> +	};
> +
>  	timer {
>  		compatible = "arm,armv8-timer";
>  		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,

Re: [PATCH V2 1/6] dt-bindings: soc: qcom: smd-rpm: Add IPQ9574 compatible

[Krzysztof Kozlowski]

On 17/02/2023 15:20, Devi Priya wrote:
> Document the compatible for RPM communication
> over SMD for IPQ9574 SoC
> 
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH V2 5/6] arm64: dts: qcom: ipq9574: Add RPM related nodes

[Devi Priya]

Hi Konrad,

Thanks for taking time to review the patch!

On 2/17/2023 8:20 PM, Konrad Dybcio wrote:
> 
> 
> On 17.02.2023 15:20, Devi Priya wrote:
>> Add RPM Glink, RPM message RAM and SMPA1 regulator
>> nodes to support frequency scaling on IPQ9574
>>
>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>> ---
>>   Changes in V2:
>> 	- Splitted the RPM and CPU Freq changes to individual patches
>> 	- Moved the regulators node to Board DT
>> 	- Dropped the regulator-always-on property
>> 	- Updated the compatible in regulators node with the existing
>> 	  mp5496 compatible
>>
>>   arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts | 11 +++++++++++
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 17 +++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> index 21b53f34ce84..8a6caaeb0c4b 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> @@ -57,6 +57,17 @@
>>   	status = "okay";
>>   };
>>   
>> +&rpm_requests {
>> +	regulators {
>> +		compatible = "qcom,rpm-mp5496-regulators";
>> +
>> +		ipq9574_s1: s1 {
>> +			regulator-min-microvolt = <587500>;
>> +			regulator-max-microvolt = <1075000>;
>> +		};
>> +	};
>> +};
> This belongs in a separate patch.
> 
Do you recommend to move this change to the below patch in the next spin?
[PATCH V2 6/6]arm64: dts: qcom: ipq9574: Add cpufreq support
>> +
>>   &sdhc_1 {
>>   	pinctrl-0 = <&sdc_default_state>;
>>   	pinctrl-names = "default";
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> index d20f3c7383f5..2f300cbab93e 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -133,6 +133,11 @@
>>   		#size-cells = <2>;
>>   		ranges;
>>   
>> +		rpm_msg_ram: rpm@60000 {
> Since this is a part of the MMIO region and not a part of DRAM,
> we generally put this node under /soc with the compatible of
> qcom,rpm-msg-ram and without no-map.
> 
> And the node name then should be sram@.
Sure, okay. Will update this in V3
> 
>> +			reg = <0x0 0x00060000 0x0 0x6000>;
>> +			no-map;
>> +		};
>> +
>>   		tz_region: tz@4a600000 {
>>   			reg = <0x0 0x4a600000 0x0 0x400000>;
>>   			no-map;
>> @@ -768,6 +773,18 @@
>>   		};
>>   	};
>>   
>> +	rpm-glink {
> Alphabetically this should come before /soc.
Okay
> 
> Konrad
>> +		compatible = "qcom,glink-rpm";
>> +		interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
>> +		qcom,rpm-msg-ram = <&rpm_msg_ram>;
>> +		mboxes = <&apcs_glb 0>;
>> +
>> +		rpm_requests: glink-channel {
>> +			compatible = "qcom,rpm-ipq9574";
>> +			qcom,glink-channels = "rpm_requests";
>> +		};
>> +	};
>> +
>>   	timer {
>>   		compatible = "arm,armv8-timer";
>>   		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
Best Regards,
Devi Priya

Re: [PATCH V2 5/6] arm64: dts: qcom: ipq9574: Add RPM related nodes

[Konrad Dybcio]

On 20.02.2023 14:53, Devi Priya wrote:
> Hi Konrad,
> 
> Thanks for taking time to review the patch!
I appreciate your gratitude, but please don't toppost (a.k.a
don't reply in the first lines of the email), that's rather
frowned upon on LKML.

> 
> On 2/17/2023 8:20 PM, Konrad Dybcio wrote:
>>
>>
>> On 17.02.2023 15:20, Devi Priya wrote:
>>> Add RPM Glink, RPM message RAM and SMPA1 regulator
>>> nodes to support frequency scaling on IPQ9574
>>>
>>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>>> ---
>>>   Changes in V2:
>>>     - Splitted the RPM and CPU Freq changes to individual patches
>>>     - Moved the regulators node to Board DT
>>>     - Dropped the regulator-always-on property
>>>     - Updated the compatible in regulators node with the existing
>>>       mp5496 compatible
>>>
>>>   arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts | 11 +++++++++++
>>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 17 +++++++++++++++++
>>>   2 files changed, 28 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>> index 21b53f34ce84..8a6caaeb0c4b 100644
>>> --- a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>> @@ -57,6 +57,17 @@
>>>       status = "okay";
>>>   };
>>>   +&rpm_requests {
>>> +    regulators {
>>> +        compatible = "qcom,rpm-mp5496-regulators";
>>> +
>>> +        ipq9574_s1: s1 {
>>> +            regulator-min-microvolt = <587500>;
>>> +            regulator-max-microvolt = <1075000>;
>>> +        };
>>> +    };
>>> +};
>> This belongs in a separate patch.
>>
> Do you recommend to move this change to the below patch in the next spin?
> [PATCH V2 6/6]arm64: dts: qcom: ipq9574: Add cpufreq support
Sounds good

Also, I think you missed a newline before &rpm_requests now that
I look at it.

Konrad
>>> +
>>>   &sdhc_1 {
>>>       pinctrl-0 = <&sdc_default_state>;
>>>       pinctrl-names = "default";
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>> index d20f3c7383f5..2f300cbab93e 100644
>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>> @@ -133,6 +133,11 @@
>>>           #size-cells = <2>;
>>>           ranges;
>>>   +        rpm_msg_ram: rpm@60000 {
>> Since this is a part of the MMIO region and not a part of DRAM,
>> we generally put this node under /soc with the compatible of
>> qcom,rpm-msg-ram and without no-map.
>>
>> And the node name then should be sram@.
> Sure, okay. Will update this in V3
>>
>>> +            reg = <0x0 0x00060000 0x0 0x6000>;
>>> +            no-map;
>>> +        };
>>> +
>>>           tz_region: tz@4a600000 {
>>>               reg = <0x0 0x4a600000 0x0 0x400000>;
>>>               no-map;
>>> @@ -768,6 +773,18 @@
>>>           };
>>>       };
>>>   +    rpm-glink {
>> Alphabetically this should come before /soc.
> Okay
>>
>> Konrad
>>> +        compatible = "qcom,glink-rpm";
>>> +        interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
>>> +        qcom,rpm-msg-ram = <&rpm_msg_ram>;
>>> +        mboxes = <&apcs_glb 0>;
>>> +
>>> +        rpm_requests: glink-channel {
>>> +            compatible = "qcom,rpm-ipq9574";
>>> +            qcom,glink-channels = "rpm_requests";
>>> +        };
>>> +    };
>>> +
>>>       timer {
>>>           compatible = "arm,armv8-timer";
>>>           interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> Best Regards,
> Devi Priya

Re: [PATCH V2 5/6] arm64: dts: qcom: ipq9574: Add RPM related nodes

[Devi Priya]

On 2/20/2023 8:06 PM, Konrad Dybcio wrote:
> 
> 
> On 20.02.2023 14:53, Devi Priya wrote:
>> Hi Konrad,
>>
>> Thanks for taking time to review the patch!
> I appreciate your gratitude, but please don't toppost (a.k.a
> don't reply in the first lines of the email), that's rather
> frowned upon on LKML.
> 
Sure, understood!
>>
>> On 2/17/2023 8:20 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 17.02.2023 15:20, Devi Priya wrote:
>>>> Add RPM Glink, RPM message RAM and SMPA1 regulator
>>>> nodes to support frequency scaling on IPQ9574
>>>>
>>>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>>>> ---
>>>>    Changes in V2:
>>>>      - Splitted the RPM and CPU Freq changes to individual patches
>>>>      - Moved the regulators node to Board DT
>>>>      - Dropped the regulator-always-on property
>>>>      - Updated the compatible in regulators node with the existing
>>>>        mp5496 compatible
>>>>
>>>>    arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts | 11 +++++++++++
>>>>    arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 17 +++++++++++++++++
>>>>    2 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>>> index 21b53f34ce84..8a6caaeb0c4b 100644
>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>>> @@ -57,6 +57,17 @@
>>>>        status = "okay";
>>>>    };
>>>>    +&rpm_requests {
>>>> +    regulators {
>>>> +        compatible = "qcom,rpm-mp5496-regulators";
>>>> +
>>>> +        ipq9574_s1: s1 {
>>>> +            regulator-min-microvolt = <587500>;
>>>> +            regulator-max-microvolt = <1075000>;
>>>> +        };
>>>> +    };
>>>> +};
>>> This belongs in a separate patch.
>>>
>> Do you recommend to move this change to the below patch in the next spin?
>> [PATCH V2 6/6]arm64: dts: qcom: ipq9574: Add cpufreq support
> Sounds good
> 
> Also, I think you missed a newline before &rpm_requests now that
> I look at it.
Sure, will take care of that in V3
> 
> Konrad
>>>> +
>>>>    &sdhc_1 {
>>>>        pinctrl-0 = <&sdc_default_state>;
>>>>        pinctrl-names = "default";
>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>> index d20f3c7383f5..2f300cbab93e 100644
>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>> @@ -133,6 +133,11 @@
>>>>            #size-cells = <2>;
>>>>            ranges;
>>>>    +        rpm_msg_ram: rpm@60000 {
>>> Since this is a part of the MMIO region and not a part of DRAM,
>>> we generally put this node under /soc with the compatible of
>>> qcom,rpm-msg-ram and without no-map.
>>>
>>> And the node name then should be sram@.
>> Sure, okay. Will update this in V3
>>>
>>>> +            reg = <0x0 0x00060000 0x0 0x6000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>>            tz_region: tz@4a600000 {
>>>>                reg = <0x0 0x4a600000 0x0 0x400000>;
>>>>                no-map;
>>>> @@ -768,6 +773,18 @@
>>>>            };
>>>>        };
>>>>    +    rpm-glink {
>>> Alphabetically this should come before /soc.
>> Okay
>>>
>>> Konrad
>>>> +        compatible = "qcom,glink-rpm";
>>>> +        interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
>>>> +        qcom,rpm-msg-ram = <&rpm_msg_ram>;
>>>> +        mboxes = <&apcs_glb 0>;
>>>> +
>>>> +        rpm_requests: glink-channel {
>>>> +            compatible = "qcom,rpm-ipq9574";
>>>> +            qcom,glink-channels = "rpm_requests";
>>>> +        };
>>>> +    };
>>>> +
>>>>        timer {
>>>>            compatible = "arm,armv8-timer";
>>>>            interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> Best Regards,
>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Konrad Dybcio]

On 17.02.2023 15:20, Devi Priya wrote:
> Kernel does not know the initial voltage set by the bootloaders.
> During regulator registration, the voltage variable is just declared
> and it is zero. Based on that, the regulator framework considers current
> the voltage as zero and tries to bring up each regulator to minimum
> the supported voltage.
> 
> This introduces a dip in the voltage during kernel boot and gets
> stabilized once the voltage scaling comes into picture.
> 
> To avoid the voltage dip, adding support to define the
> bootup voltage set by the boodloaders and based on it, regulator
> framework understands that proper voltage is already set
> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---
Thinking about it again, this seems like something that could be
generalized and introduced into regulator core.. Hardcoding this
will not end well.. Not to mention it'll affect all mp5496-using
boards that are already upstream.

WDYT about regulator-init-microvolts Mark?

Konrad
>  Changes in V2:
> 	- Added the bootup voltages to s2 and l2 regulators
> 
>  drivers/regulator/qcom_smd-regulator.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> index a40e66cea7e7..5f9fe6b9d368 100644
> --- a/drivers/regulator/qcom_smd-regulator.c
> +++ b/drivers/regulator/qcom_smd-regulator.c
> @@ -800,12 +800,13 @@ struct rpm_regulator_data {
>  	u32 id;
>  	const struct regulator_desc *desc;
>  	const char *supply;
> +	int boot_uV; /* To store the bootup voltage set by bootloaders */
>  };
>  
>  static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
> -	{ "s1", QCOM_SMD_RPM_SMPA, 1, &mp5496_smpa1, "s1" },
> -	{ "s2", QCOM_SMD_RPM_SMPA, 2, &mp5496_smpa2, "s2" },
> -	{ "l2", QCOM_SMD_RPM_LDOA, 2, &mp5496_ldoa2, "l2" },
> +	{ "s1", QCOM_SMD_RPM_SMPA, 1, &mp5496_smpa1, "s1", 875000  },
> +	{ "s2", QCOM_SMD_RPM_SMPA, 2, &mp5496_smpa2, "s2", 875000  },
> +	{ "l2", QCOM_SMD_RPM_LDOA, 2, &mp5496_ldoa2, "l2", 2950000 },
>  	{}
>  };
>  
> @@ -1388,6 +1389,9 @@ static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev
>  	vreg->type	= rpm_data->type;
>  	vreg->id	= rpm_data->id;
>  
> +	if (rpm_data->boot_uV)
> +		vreg->uV = rpm_data->boot_uV;
> +
>  	memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc));
>  	vreg->desc.name = rpm_data->name;
>  	vreg->desc.supply_name = rpm_data->supply;

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:

> Thinking about it again, this seems like something that could be
> generalized and introduced into regulator core.. Hardcoding this
> will not end well.. Not to mention it'll affect all mp5496-using
> boards that are already upstream.

> WDYT about regulator-init-microvolts Mark?

The overwhelming majority of devices that have variable voltages
support readback, these Qualcomm firmware devices are pretty much
unique in this regard.  We don't want a general property to set a
specific voltage since normally we should be using the
constraints and don't normally need to adjust things immediately
since we can tell what the current voltage is. 

This is pretty much just going to be a device specific bodge,
ideally something that does know what the voltage is would be
able to tell us at runtime but if that's not possible then
there's no good options.  If the initial voltage might vary based
on board then a device specific DT property might be less
terrible, if it's determined by the regulator the current code
seems fine.  Or just leave the current behavour, if the
constraints are accurate then hopefully a temporary dip in
voltage is just inelegant rather than an issue.  Indeed the
current behaviour might well save power if you've got a voltage
range configured and nothing actually ever gets round to setting
the voltage (which is depressingly common, people seem keen on
setting voltage ranges even when the voltage is never varied in
practice).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V2 2/6] regulator: qcom_smd: Add s1 sub-node to mp5496 regulator

[Rob Herring]

On Fri, 17 Feb 2023 19:50:26 +0530, Devi Priya wrote:
> Adding s1 sub-node entry to mp5496 regulator node
> 
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---
>  Changes in V2:
> 	- Dropped ipq9574 compatible as the existing MP5496 support is
> 	  extended for S1 regulator
> 
>  .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml   | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Devi Priya]

On 2/23/2023 4:31 AM, Mark Brown wrote:
> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
> 
>> Thinking about it again, this seems like something that could be
>> generalized and introduced into regulator core.. Hardcoding this
>> will not end well.. Not to mention it'll affect all mp5496-using
>> boards that are already upstream.
> 
>> WDYT about regulator-init-microvolts Mark?
> 
> The overwhelming majority of devices that have variable voltages
> support readback, these Qualcomm firmware devices are pretty much
> unique in this regard.  We don't want a general property to set a
> specific voltage since normally we should be using the
> constraints and don't normally need to adjust things immediately
> since we can tell what the current voltage is.
> 
> This is pretty much just going to be a device specific bodge,
> ideally something that does know what the voltage is would be
> able to tell us at runtime but if that's not possible then
> there's no good options.  If the initial voltage might vary based
> on board then a device specific DT property might be less
> terrible, if it's determined by the regulator the current code
> seems fine.  Or just leave the current behavour, if the
> constraints are accurate then hopefully a temporary dip in
> voltage is just inelegant rather than an issue.  Indeed the
> current behaviour might well save power if you've got a voltage
> range configured and nothing actually ever gets round to setting
> the voltage (which is depressingly common, people seem keen on
> setting voltage ranges even when the voltage is never varied in
> practice).

Hi Mark, The initial bootup voltage is actually blown into the OTP 
register of the PMIC and it remains the same across boards for IPQ9574 
SoC. Initially the SoC runs at 800MHz with a voltage of 875mV set by the 
bootloaders. As kernel does not know the initial voltage, during 
regulator registration the framework considers the current voltage to be 
zero and tries to bring up the regulator to minimum supported voltage of 
600mV. This causes the dip which might be of concern in SS parts where 
the voltage might be insufficient leading to silent reboots.

Best Regards,
Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Konrad Dybcio]

On 3.03.2023 14:21, Devi Priya wrote:
> 
> 
> On 2/23/2023 4:31 AM, Mark Brown wrote:
>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>
>>> Thinking about it again, this seems like something that could be
>>> generalized and introduced into regulator core.. Hardcoding this
>>> will not end well.. Not to mention it'll affect all mp5496-using
>>> boards that are already upstream.
>>
>>> WDYT about regulator-init-microvolts Mark?
>>
>> The overwhelming majority of devices that have variable voltages
>> support readback, these Qualcomm firmware devices are pretty much
>> unique in this regard.  We don't want a general property to set a
>> specific voltage since normally we should be using the
>> constraints and don't normally need to adjust things immediately
>> since we can tell what the current voltage is.
>>
>> This is pretty much just going to be a device specific bodge,
>> ideally something that does know what the voltage is would be
>> able to tell us at runtime but if that's not possible then
>> there's no good options.  If the initial voltage might vary based
>> on board then a device specific DT property might be less
>> terrible, if it's determined by the regulator the current code
>> seems fine.  Or just leave the current behavour, if the
>> constraints are accurate then hopefully a temporary dip in
>> voltage is just inelegant rather than an issue.  Indeed the
>> current behaviour might well save power if you've got a voltage
>> range configured and nothing actually ever gets round to setting
>> the voltage (which is depressingly common, people seem keen on
>> setting voltage ranges even when the voltage is never varied in
>> practice).
> 
> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
But what about IPQ6018 which also uses MP5496? That's also gonna
set the voltage on there, it may be too high/low..

 Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
That's an SoC-specific thing, the same regulator can be used with
many different ones. We can't just assume it'll always be like this.
I see the problem, but I believe this is not the correct solution.

Konrad
> 
> Best Regards,
> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Devi Priya]

On 3/6/2023 6:39 PM, Devi Priya wrote:
> 
> 
> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>
>>
>> On 3.03.2023 14:21, Devi Priya wrote:
>>>
>>>
>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>
>>>>> Thinking about it again, this seems like something that could be
>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>> boards that are already upstream.
>>>>
>>>>> WDYT about regulator-init-microvolts Mark?
>>>>
>>>> The overwhelming majority of devices that have variable voltages
>>>> support readback, these Qualcomm firmware devices are pretty much
>>>> unique in this regard.  We don't want a general property to set a
>>>> specific voltage since normally we should be using the
>>>> constraints and don't normally need to adjust things immediately
>>>> since we can tell what the current voltage is.
>>>>
>>>> This is pretty much just going to be a device specific bodge,
>>>> ideally something that does know what the voltage is would be
>>>> able to tell us at runtime but if that's not possible then
>>>> there's no good options.  If the initial voltage might vary based
>>>> on board then a device specific DT property might be less
>>>> terrible, if it's determined by the regulator the current code
>>>> seems fine.  Or just leave the current behavour, if the
>>>> constraints are accurate then hopefully a temporary dip in
>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>> current behaviour might well save power if you've got a voltage
>>>> range configured and nothing actually ever gets round to setting
>>>> the voltage (which is depressingly common, people seem keen on
>>>> setting voltage ranges even when the voltage is never varied in
>>>> practice).
>>>
>>> Hi Mark, The initial bootup voltage is actually blown into the OTP 
>>> register of the PMIC and it remains the same across boards for 
>>> IPQ9574 SoC.
>> But what about IPQ6018 which also uses MP5496? That's also gonna
>> set the voltage on there, it may be too high/low..
For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
875mV
>>
>>   Initially the SoC runs at 800MHz with a voltage of 875mV set by the 
>> bootloaders. As kernel does not know the initial voltage, during 
>> regulator registration the framework considers the current voltage to 
>> be zero and tries to bring up the regulator to minimum supported 
>> voltage of 600mV. This causes the dip which might be of concern in SS 
>> parts where the voltage might be insufficient leading to silent reboots.
>> That's an SoC-specific thing, the same regulator can be used with
>> many different ones. We can't just assume it'll always be like this.
>> I see the problem, but I believe this is not the correct solution
Okay, As we had discussions on reading back the voltage & the generic
DT property, do you suggest any other possible solutions here?
>>
>> Konrad
>>>
>>> Best Regards,
>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Konrad Dybcio]

On 7.03.2023 07:55, Devi Priya wrote:
> 
> 
> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>
>>
>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>
>>>>
>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>
>>>>>> Thinking about it again, this seems like something that could be
>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>> boards that are already upstream.
>>>>>
>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>
>>>>> The overwhelming majority of devices that have variable voltages
>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>> unique in this regard.  We don't want a general property to set a
>>>>> specific voltage since normally we should be using the
>>>>> constraints and don't normally need to adjust things immediately
>>>>> since we can tell what the current voltage is.
>>>>>
>>>>> This is pretty much just going to be a device specific bodge,
>>>>> ideally something that does know what the voltage is would be
>>>>> able to tell us at runtime but if that's not possible then
>>>>> there's no good options.  If the initial voltage might vary based
>>>>> on board then a device specific DT property might be less
>>>>> terrible, if it's determined by the regulator the current code
>>>>> seems fine.  Or just leave the current behavour, if the
>>>>> constraints are accurate then hopefully a temporary dip in
>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>> current behaviour might well save power if you've got a voltage
>>>>> range configured and nothing actually ever gets round to setting
>>>>> the voltage (which is depressingly common, people seem keen on
>>>>> setting voltage ranges even when the voltage is never varied in
>>>>> practice).
>>>>
>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>> set the voltage on there, it may be too high/low..
> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
> 875mV
Okay, but what about any other design that employs or may employ
MP5496 in the future?

>>>
>>>   Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>> That's an SoC-specific thing, the same regulator can be used with
>>> many different ones. We can't just assume it'll always be like this.
>>> I see the problem, but I believe this is not the correct solution
> Okay, As we had discussions on reading back the voltage & the generic
> DT property, do you suggest any other possible solutions here?
Due to the sudden influx of various IPQ SoCs on the mailing list lately
I have no idea if it concerned this one too, but at least one of them
was said not to use RPM for controlling the clocks. If that's the case,
I see no reason at all to use it for scaling the regulators, the PMIC
could be addressed directly over I2C as a normal device. You'd probably
want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
simply not registering the CX/MX registers as children of the I2C
regulator IC.

Konrad
>>>
>>> Konrad
>>>>
>>>> Best Regards,
>>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Devi Priya]

On 3/8/2023 3:57 PM, Konrad Dybcio wrote:
> 
> 
> On 7.03.2023 07:55, Devi Priya wrote:
>>
>>
>> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>>
>>>
>>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>>
>>>>>
>>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>>
>>>>>>> Thinking about it again, this seems like something that could be
>>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>>> boards that are already upstream.
>>>>>>
>>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>>
>>>>>> The overwhelming majority of devices that have variable voltages
>>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>>> unique in this regard.  We don't want a general property to set a
>>>>>> specific voltage since normally we should be using the
>>>>>> constraints and don't normally need to adjust things immediately
>>>>>> since we can tell what the current voltage is.
>>>>>>
>>>>>> This is pretty much just going to be a device specific bodge,
>>>>>> ideally something that does know what the voltage is would be
>>>>>> able to tell us at runtime but if that's not possible then
>>>>>> there's no good options.  If the initial voltage might vary based
>>>>>> on board then a device specific DT property might be less
>>>>>> terrible, if it's determined by the regulator the current code
>>>>>> seems fine.  Or just leave the current behavour, if the
>>>>>> constraints are accurate then hopefully a temporary dip in
>>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>>> current behaviour might well save power if you've got a voltage
>>>>>> range configured and nothing actually ever gets round to setting
>>>>>> the voltage (which is depressingly common, people seem keen on
>>>>>> setting voltage ranges even when the voltage is never varied in
>>>>>> practice).
>>>>>
>>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>>> set the voltage on there, it may be too high/low..
>> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
>> 875mV
> Okay, but what about any other design that employs or may employ
> MP5496 in the future?
> 
>>>>
>>>>    Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>>> That's an SoC-specific thing, the same regulator can be used with
>>>> many different ones. We can't just assume it'll always be like this.
>>>> I see the problem, but I believe this is not the correct solution
>> Okay, As we had discussions on reading back the voltage & the generic
>> DT property, do you suggest any other possible solutions here?
> Due to the sudden influx of various IPQ SoCs on the mailing list lately
> I have no idea if it concerned this one too, but at least one of them
> was said not to use RPM for controlling the clocks. If that's the case,
> I see no reason at all to use it for scaling the regulators, the PMIC
> could be addressed directly over I2C as a normal device. You'd probably
> want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
> simply not registering the CX/MX registers as children of the I2C
> regulator IC.

IPQ9574 SoC has RPM and uses it for controlling the regulators.
Currently, the RPM firmware does not have read support implemented
and so, we were not able to read the bootup voltage.
As we randomly saw silent reboots when the kernel boots up,
do you think we could proceed with this change for time being
and revisit the same when any SoC in the future employs MP5496?
> 
> Konrad
>>>>
>>>> Konrad
>>>>>
>>>>> Best Regards,
>>>>> Devi Priya

Re: (subset) [PATCH V2 0/6] Add regulator support for IPQ9574 SoC

[Bjorn Andersson]

On Fri, 17 Feb 2023 19:50:24 +0530, Devi Priya wrote:
> IPQ9574 SoC uses the PMIC MP5496 and SMPA1 regulator for
> APSS voltage scaling.
> This patch series adds the support for the same and also
> enables the RPM communication over the RPMSG framework
> 
> DTS patch depends on the below series
> https://lore.kernel.org/linux-arm-kernel/20230217134107.13946-1-quic_devipriy@quicinc.com/
> 
> [...]

Applied, thanks!

[1/6] dt-bindings: soc: qcom: smd-rpm: Add IPQ9574 compatible
      commit: 56d2156e7c298ecad5a113a81e5550631bcf7fd5

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Konrad Dybcio]

On 14.03.2023 18:15, Devi Priya wrote:
> 
> 
> On 3/8/2023 3:57 PM, Konrad Dybcio wrote:
>>
>>
>> On 7.03.2023 07:55, Devi Priya wrote:
>>>
>>>
>>> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>>>
>>>>
>>>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>>>
>>>>>
>>>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>>>
>>>>>>
>>>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>>>
>>>>>>>> Thinking about it again, this seems like something that could be
>>>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>>>> boards that are already upstream.
>>>>>>>
>>>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>>>
>>>>>>> The overwhelming majority of devices that have variable voltages
>>>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>>>> unique in this regard.  We don't want a general property to set a
>>>>>>> specific voltage since normally we should be using the
>>>>>>> constraints and don't normally need to adjust things immediately
>>>>>>> since we can tell what the current voltage is.
>>>>>>>
>>>>>>> This is pretty much just going to be a device specific bodge,
>>>>>>> ideally something that does know what the voltage is would be
>>>>>>> able to tell us at runtime but if that's not possible then
>>>>>>> there's no good options.  If the initial voltage might vary based
>>>>>>> on board then a device specific DT property might be less
>>>>>>> terrible, if it's determined by the regulator the current code
>>>>>>> seems fine.  Or just leave the current behavour, if the
>>>>>>> constraints are accurate then hopefully a temporary dip in
>>>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>>>> current behaviour might well save power if you've got a voltage
>>>>>>> range configured and nothing actually ever gets round to setting
>>>>>>> the voltage (which is depressingly common, people seem keen on
>>>>>>> setting voltage ranges even when the voltage is never varied in
>>>>>>> practice).
>>>>>>
>>>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>>>> set the voltage on there, it may be too high/low..
>>> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
>>> 875mV
>> Okay, but what about any other design that employs or may employ
>> MP5496 in the future?
>>
>>>>>
>>>>>    Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>>>> That's an SoC-specific thing, the same regulator can be used with
>>>>> many different ones. We can't just assume it'll always be like this.
>>>>> I see the problem, but I believe this is not the correct solution
>>> Okay, As we had discussions on reading back the voltage & the generic
>>> DT property, do you suggest any other possible solutions here?
>> Due to the sudden influx of various IPQ SoCs on the mailing list lately
>> I have no idea if it concerned this one too, but at least one of them
>> was said not to use RPM for controlling the clocks. If that's the case,
>> I see no reason at all to use it for scaling the regulators, the PMIC
>> could be addressed directly over I2C as a normal device. You'd probably
>> want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
>> simply not registering the CX/MX registers as children of the I2C
>> regulator IC.
> 
> IPQ9574 SoC has RPM and uses it for controlling the regulators.
> Currently, the RPM firmware does not have read support implemented
> and so, we were not able to read the bootup voltage.
> As we randomly saw silent reboots when the kernel boots up,
> do you think we could proceed with this change for time being
> and revisit the same when any SoC in the future employs MP5496?
I'm still thinking about a cleaner fix because hardcoding voltages
in kernel is just dangerous. Could you check whether attaching a CPU
supply and adding an OPP table where each level has an opp-microvolt
property would resolve your issue?

Konrad
>>
>> Konrad
>>>>>
>>>>> Konrad
>>>>>>
>>>>>> Best Regards,
>>>>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Devi Priya]

On 3/18/2023 7:38 PM, Konrad Dybcio wrote:
> 
> 
> On 14.03.2023 18:15, Devi Priya wrote:
>>
>>
>> On 3/8/2023 3:57 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 7.03.2023 07:55, Devi Priya wrote:
>>>>
>>>>
>>>> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>>>>
>>>>>
>>>>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>>>>
>>>>>>
>>>>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>>> Thinking about it again, this seems like something that could be
>>>>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>>>>> boards that are already upstream.
>>>>>>>>
>>>>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>>>>
>>>>>>>> The overwhelming majority of devices that have variable voltages
>>>>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>>>>> unique in this regard.  We don't want a general property to set a
>>>>>>>> specific voltage since normally we should be using the
>>>>>>>> constraints and don't normally need to adjust things immediately
>>>>>>>> since we can tell what the current voltage is.
>>>>>>>>
>>>>>>>> This is pretty much just going to be a device specific bodge,
>>>>>>>> ideally something that does know what the voltage is would be
>>>>>>>> able to tell us at runtime but if that's not possible then
>>>>>>>> there's no good options.  If the initial voltage might vary based
>>>>>>>> on board then a device specific DT property might be less
>>>>>>>> terrible, if it's determined by the regulator the current code
>>>>>>>> seems fine.  Or just leave the current behavour, if the
>>>>>>>> constraints are accurate then hopefully a temporary dip in
>>>>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>>>>> current behaviour might well save power if you've got a voltage
>>>>>>>> range configured and nothing actually ever gets round to setting
>>>>>>>> the voltage (which is depressingly common, people seem keen on
>>>>>>>> setting voltage ranges even when the voltage is never varied in
>>>>>>>> practice).
>>>>>>>
>>>>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>>>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>>>>> set the voltage on there, it may be too high/low..
>>>> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
>>>> 875mV
>>> Okay, but what about any other design that employs or may employ
>>> MP5496 in the future?
>>>
>>>>>>
>>>>>>     Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>>>>> That's an SoC-specific thing, the same regulator can be used with
>>>>>> many different ones. We can't just assume it'll always be like this.
>>>>>> I see the problem, but I believe this is not the correct solution
>>>> Okay, As we had discussions on reading back the voltage & the generic
>>>> DT property, do you suggest any other possible solutions here?
>>> Due to the sudden influx of various IPQ SoCs on the mailing list lately
>>> I have no idea if it concerned this one too, but at least one of them
>>> was said not to use RPM for controlling the clocks. If that's the case,
>>> I see no reason at all to use it for scaling the regulators, the PMIC
>>> could be addressed directly over I2C as a normal device. You'd probably
>>> want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
>>> simply not registering the CX/MX registers as children of the I2C
>>> regulator IC.
>>
>> IPQ9574 SoC has RPM and uses it for controlling the regulators.
>> Currently, the RPM firmware does not have read support implemented
>> and so, we were not able to read the bootup voltage.
>> As we randomly saw silent reboots when the kernel boots up,
>> do you think we could proceed with this change for time being
>> and revisit the same when any SoC in the future employs MP5496?
> I'm still thinking about a cleaner fix because hardcoding voltages
> in kernel is just dangerous. Could you check whether attaching a CPU
> supply and adding an OPP table where each level has an opp-microvolt
> property would resolve your issue?
> 
> Konrad
Yes, Understood! We already have the CPU supply and OPP table where
each level has an opp-microvolt property changes in place and it did
not help in our case.
We have now planned to update the regulator-min-microvolt property
with the SVS voltage (725000uV) in the board DT such that the regulators
are brought up with the nominal voltage which would be sufficient
for all the corner parts operating at 800MHz.

That way, we would update the DT and drop this patch in the next spin.

Thanks,
Devi Priya
>>>
>>> Konrad
>>>>>>
>>>>>> Konrad
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Konrad Dybcio]

On 27.03.2023 10:40, Devi Priya wrote:
> 
> 
> On 3/18/2023 7:38 PM, Konrad Dybcio wrote:
>>
>>
>> On 14.03.2023 18:15, Devi Priya wrote:
>>>
>>>
>>> On 3/8/2023 3:57 PM, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 7.03.2023 07:55, Devi Priya wrote:
>>>>>
>>>>>
>>>>> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>>>>>
>>>>>>
>>>>>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>>>>>
>>>>>>>>>> Thinking about it again, this seems like something that could be
>>>>>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>>>>>> boards that are already upstream.
>>>>>>>>>
>>>>>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>>>>>
>>>>>>>>> The overwhelming majority of devices that have variable voltages
>>>>>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>>>>>> unique in this regard.  We don't want a general property to set a
>>>>>>>>> specific voltage since normally we should be using the
>>>>>>>>> constraints and don't normally need to adjust things immediately
>>>>>>>>> since we can tell what the current voltage is.
>>>>>>>>>
>>>>>>>>> This is pretty much just going to be a device specific bodge,
>>>>>>>>> ideally something that does know what the voltage is would be
>>>>>>>>> able to tell us at runtime but if that's not possible then
>>>>>>>>> there's no good options.  If the initial voltage might vary based
>>>>>>>>> on board then a device specific DT property might be less
>>>>>>>>> terrible, if it's determined by the regulator the current code
>>>>>>>>> seems fine.  Or just leave the current behavour, if the
>>>>>>>>> constraints are accurate then hopefully a temporary dip in
>>>>>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>>>>>> current behaviour might well save power if you've got a voltage
>>>>>>>>> range configured and nothing actually ever gets round to setting
>>>>>>>>> the voltage (which is depressingly common, people seem keen on
>>>>>>>>> setting voltage ranges even when the voltage is never varied in
>>>>>>>>> practice).
>>>>>>>>
>>>>>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>>>>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>>>>>> set the voltage on there, it may be too high/low..
>>>>> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
>>>>> 875mV
>>>> Okay, but what about any other design that employs or may employ
>>>> MP5496 in the future?
>>>>
>>>>>>>
>>>>>>>     Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>>>>>> That's an SoC-specific thing, the same regulator can be used with
>>>>>>> many different ones. We can't just assume it'll always be like this.
>>>>>>> I see the problem, but I believe this is not the correct solution
>>>>> Okay, As we had discussions on reading back the voltage & the generic
>>>>> DT property, do you suggest any other possible solutions here?
>>>> Due to the sudden influx of various IPQ SoCs on the mailing list lately
>>>> I have no idea if it concerned this one too, but at least one of them
>>>> was said not to use RPM for controlling the clocks. If that's the case,
>>>> I see no reason at all to use it for scaling the regulators, the PMIC
>>>> could be addressed directly over I2C as a normal device. You'd probably
>>>> want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
>>>> simply not registering the CX/MX registers as children of the I2C
>>>> regulator IC.
>>>
>>> IPQ9574 SoC has RPM and uses it for controlling the regulators.
>>> Currently, the RPM firmware does not have read support implemented
>>> and so, we were not able to read the bootup voltage.
>>> As we randomly saw silent reboots when the kernel boots up,
>>> do you think we could proceed with this change for time being
>>> and revisit the same when any SoC in the future employs MP5496?
>> I'm still thinking about a cleaner fix because hardcoding voltages
>> in kernel is just dangerous. Could you check whether attaching a CPU
>> supply and adding an OPP table where each level has an opp-microvolt
>> property would resolve your issue?
>>
>> Konrad
> Yes, Understood! We already have the CPU supply and OPP table where
> each level has an opp-microvolt property changes in place and it did
> not help in our case.
Ouch. That totally sounds like a bug.. Would you be willing to dig
a bit further into why this happens?

Konrad
> We have now planned to update the regulator-min-microvolt property
> with the SVS voltage (725000uV) in the board DT such that the regulators
> are brought up with the nominal voltage which would be sufficient
> for all the corner parts operating at 800MHz.
> 
> That way, we would update the DT and drop this patch in the next spin.
> 
> Thanks,
> Devi Priya
>>>>
>>>> Konrad
>>>>>>>
>>>>>>> Konrad
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Devi Priya]

On 3/27/2023 2:56 PM, Konrad Dybcio wrote:
> 
> 
> On 27.03.2023 10:40, Devi Priya wrote:
>>
>>
>> On 3/18/2023 7:38 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 14.03.2023 18:15, Devi Priya wrote:
>>>>
>>>>
>>>> On 3/8/2023 3:57 PM, Konrad Dybcio wrote:
>>>>>
>>>>>
>>>>> On 7.03.2023 07:55, Devi Priya wrote:
>>>>>>
>>>>>>
>>>>>> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>>>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>>>>>>
>>>>>>>>>>> Thinking about it again, this seems like something that could be
>>>>>>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>>>>>>> boards that are already upstream.
>>>>>>>>>>
>>>>>>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>>>>>>
>>>>>>>>>> The overwhelming majority of devices that have variable voltages
>>>>>>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>>>>>>> unique in this regard.  We don't want a general property to set a
>>>>>>>>>> specific voltage since normally we should be using the
>>>>>>>>>> constraints and don't normally need to adjust things immediately
>>>>>>>>>> since we can tell what the current voltage is.
>>>>>>>>>>
>>>>>>>>>> This is pretty much just going to be a device specific bodge,
>>>>>>>>>> ideally something that does know what the voltage is would be
>>>>>>>>>> able to tell us at runtime but if that's not possible then
>>>>>>>>>> there's no good options.  If the initial voltage might vary based
>>>>>>>>>> on board then a device specific DT property might be less
>>>>>>>>>> terrible, if it's determined by the regulator the current code
>>>>>>>>>> seems fine.  Or just leave the current behavour, if the
>>>>>>>>>> constraints are accurate then hopefully a temporary dip in
>>>>>>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>>>>>>> current behaviour might well save power if you've got a voltage
>>>>>>>>>> range configured and nothing actually ever gets round to setting
>>>>>>>>>> the voltage (which is depressingly common, people seem keen on
>>>>>>>>>> setting voltage ranges even when the voltage is never varied in
>>>>>>>>>> practice).
>>>>>>>>>
>>>>>>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>>>>>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>>>>>>> set the voltage on there, it may be too high/low..
>>>>>> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
>>>>>> 875mV
>>>>> Okay, but what about any other design that employs or may employ
>>>>> MP5496 in the future?
>>>>>
>>>>>>>>
>>>>>>>>      Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>>>>>>> That's an SoC-specific thing, the same regulator can be used with
>>>>>>>> many different ones. We can't just assume it'll always be like this.
>>>>>>>> I see the problem, but I believe this is not the correct solution
>>>>>> Okay, As we had discussions on reading back the voltage & the generic
>>>>>> DT property, do you suggest any other possible solutions here?
>>>>> Due to the sudden influx of various IPQ SoCs on the mailing list lately
>>>>> I have no idea if it concerned this one too, but at least one of them
>>>>> was said not to use RPM for controlling the clocks. If that's the case,
>>>>> I see no reason at all to use it for scaling the regulators, the PMIC
>>>>> could be addressed directly over I2C as a normal device. You'd probably
>>>>> want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
>>>>> simply not registering the CX/MX registers as children of the I2C
>>>>> regulator IC.
>>>>
>>>> IPQ9574 SoC has RPM and uses it for controlling the regulators.
>>>> Currently, the RPM firmware does not have read support implemented
>>>> and so, we were not able to read the bootup voltage.
>>>> As we randomly saw silent reboots when the kernel boots up,
>>>> do you think we could proceed with this change for time being
>>>> and revisit the same when any SoC in the future employs MP5496?
>>> I'm still thinking about a cleaner fix because hardcoding voltages
>>> in kernel is just dangerous. Could you check whether attaching a CPU
>>> supply and adding an OPP table where each level has an opp-microvolt
>>> property would resolve your issue?
>>>
>>> Konrad
>> Yes, Understood! We already have the CPU supply and OPP table where
>> each level has an opp-microvolt property changes in place and it did
>> not help in our case.
> Ouch. That totally sounds like a bug.. Would you be willing to dig
> a bit further into why this happens?
As the regulator registration happens before the cpu freq scaling comes
into picture, having an OPP table did not help to set the bootup voltage
> 
> Konrad
>> We have now planned to update the regulator-min-microvolt property
>> with the SVS voltage (725000uV) in the board DT such that the regulators
>> are brought up with the nominal voltage which would be sufficient
>> for all the corner parts operating at 800MHz.
>>
>> That way, we would update the DT and drop this patch in the next spin.
>>
>> Thanks,
>> Devi Priya
>>>>>
>>>>> Konrad
>>>>>>>>
>>>>>>>> Konrad
>>>>>>>>>
>>>>>>>>> Best Regards,
>>>>>>>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Konrad Dybcio]

On 28.03.2023 08:12, Devi Priya wrote:
> 
> 
> On 3/27/2023 2:56 PM, Konrad Dybcio wrote:
>>
>>
>> On 27.03.2023 10:40, Devi Priya wrote:
>>>
>>>
>>> On 3/18/2023 7:38 PM, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 14.03.2023 18:15, Devi Priya wrote:
>>>>>
>>>>>
>>>>> On 3/8/2023 3:57 PM, Konrad Dybcio wrote:
>>>>>>
>>>>>>
>>>>>> On 7.03.2023 07:55, Devi Priya wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>>>>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Thinking about it again, this seems like something that could be
>>>>>>>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>>>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>>>>>>>> boards that are already upstream.
>>>>>>>>>>>
>>>>>>>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>>>>>>>
>>>>>>>>>>> The overwhelming majority of devices that have variable voltages
>>>>>>>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>>>>>>>> unique in this regard.  We don't want a general property to set a
>>>>>>>>>>> specific voltage since normally we should be using the
>>>>>>>>>>> constraints and don't normally need to adjust things immediately
>>>>>>>>>>> since we can tell what the current voltage is.
>>>>>>>>>>>
>>>>>>>>>>> This is pretty much just going to be a device specific bodge,
>>>>>>>>>>> ideally something that does know what the voltage is would be
>>>>>>>>>>> able to tell us at runtime but if that's not possible then
>>>>>>>>>>> there's no good options.  If the initial voltage might vary based
>>>>>>>>>>> on board then a device specific DT property might be less
>>>>>>>>>>> terrible, if it's determined by the regulator the current code
>>>>>>>>>>> seems fine.  Or just leave the current behavour, if the
>>>>>>>>>>> constraints are accurate then hopefully a temporary dip in
>>>>>>>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>>>>>>>> current behaviour might well save power if you've got a voltage
>>>>>>>>>>> range configured and nothing actually ever gets round to setting
>>>>>>>>>>> the voltage (which is depressingly common, people seem keen on
>>>>>>>>>>> setting voltage ranges even when the voltage is never varied in
>>>>>>>>>>> practice).
>>>>>>>>>>
>>>>>>>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>>>>>>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>>>>>>>> set the voltage on there, it may be too high/low..
>>>>>>> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
>>>>>>> 875mV
>>>>>> Okay, but what about any other design that employs or may employ
>>>>>> MP5496 in the future?
>>>>>>
>>>>>>>>>
>>>>>>>>>      Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>>>>>>>> That's an SoC-specific thing, the same regulator can be used with
>>>>>>>>> many different ones. We can't just assume it'll always be like this.
>>>>>>>>> I see the problem, but I believe this is not the correct solution
>>>>>>> Okay, As we had discussions on reading back the voltage & the generic
>>>>>>> DT property, do you suggest any other possible solutions here?
>>>>>> Due to the sudden influx of various IPQ SoCs on the mailing list lately
>>>>>> I have no idea if it concerned this one too, but at least one of them
>>>>>> was said not to use RPM for controlling the clocks. If that's the case,
>>>>>> I see no reason at all to use it for scaling the regulators, the PMIC
>>>>>> could be addressed directly over I2C as a normal device. You'd probably
>>>>>> want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
>>>>>> simply not registering the CX/MX registers as children of the I2C
>>>>>> regulator IC.
>>>>>
>>>>> IPQ9574 SoC has RPM and uses it for controlling the regulators.
>>>>> Currently, the RPM firmware does not have read support implemented
>>>>> and so, we were not able to read the bootup voltage.
>>>>> As we randomly saw silent reboots when the kernel boots up,
>>>>> do you think we could proceed with this change for time being
>>>>> and revisit the same when any SoC in the future employs MP5496?
>>>> I'm still thinking about a cleaner fix because hardcoding voltages
>>>> in kernel is just dangerous. Could you check whether attaching a CPU
>>>> supply and adding an OPP table where each level has an opp-microvolt
>>>> property would resolve your issue?
>>>>
>>>> Konrad
>>> Yes, Understood! We already have the CPU supply and OPP table where
>>> each level has an opp-microvolt property changes in place and it did
>>> not help in our case.
>> Ouch. That totally sounds like a bug.. Would you be willing to dig
>> a bit further into why this happens?
> As the regulator registration happens before the cpu freq scaling comes
> into picture, having an OPP table did not help to set the bootup voltage
I see, but the order should not affect it. If your regulator driver
returns -EPROBE_DEFER, so should the cpufreq one.

Konrad
>>
>> Konrad
>>> We have now planned to update the regulator-min-microvolt property
>>> with the SVS voltage (725000uV) in the board DT such that the regulators
>>> are brought up with the nominal voltage which would be sufficient
>>> for all the corner parts operating at 800MHz.
>>>
>>> That way, we would update the DT and drop this patch in the next spin.
>>>
>>> Thanks,
>>> Devi Priya
>>>>>>
>>>>>> Konrad
>>>>>>>>>
>>>>>>>>> Konrad
>>>>>>>>>>
>>>>>>>>>> Best Regards,
>>>>>>>>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Devi Priya]

On 3/28/2023 1:58 PM, Konrad Dybcio wrote:
> 
> 
> On 28.03.2023 08:12, Devi Priya wrote:
>>
>>
>> On 3/27/2023 2:56 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 27.03.2023 10:40, Devi Priya wrote:
>>>>
>>>>
>>>> On 3/18/2023 7:38 PM, Konrad Dybcio wrote:
>>>>>
>>>>>
>>>>> On 14.03.2023 18:15, Devi Priya wrote:
>>>>>>
>>>>>>
>>>>>> On 3/8/2023 3:57 PM, Konrad Dybcio wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 7.03.2023 07:55, Devi Priya wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>>>>>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Thinking about it again, this seems like something that could be
>>>>>>>>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>>>>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>>>>>>>>> boards that are already upstream.
>>>>>>>>>>>>
>>>>>>>>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>>>>>>>>
>>>>>>>>>>>> The overwhelming majority of devices that have variable voltages
>>>>>>>>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>>>>>>>>> unique in this regard.  We don't want a general property to set a
>>>>>>>>>>>> specific voltage since normally we should be using the
>>>>>>>>>>>> constraints and don't normally need to adjust things immediately
>>>>>>>>>>>> since we can tell what the current voltage is.
>>>>>>>>>>>>
>>>>>>>>>>>> This is pretty much just going to be a device specific bodge,
>>>>>>>>>>>> ideally something that does know what the voltage is would be
>>>>>>>>>>>> able to tell us at runtime but if that's not possible then
>>>>>>>>>>>> there's no good options.  If the initial voltage might vary based
>>>>>>>>>>>> on board then a device specific DT property might be less
>>>>>>>>>>>> terrible, if it's determined by the regulator the current code
>>>>>>>>>>>> seems fine.  Or just leave the current behavour, if the
>>>>>>>>>>>> constraints are accurate then hopefully a temporary dip in
>>>>>>>>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>>>>>>>>> current behaviour might well save power if you've got a voltage
>>>>>>>>>>>> range configured and nothing actually ever gets round to setting
>>>>>>>>>>>> the voltage (which is depressingly common, people seem keen on
>>>>>>>>>>>> setting voltage ranges even when the voltage is never varied in
>>>>>>>>>>>> practice).
>>>>>>>>>>>
>>>>>>>>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>>>>>>>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>>>>>>>>> set the voltage on there, it may be too high/low..
>>>>>>>> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
>>>>>>>> 875mV
>>>>>>> Okay, but what about any other design that employs or may employ
>>>>>>> MP5496 in the future?
>>>>>>>
>>>>>>>>>>
>>>>>>>>>>       Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>>>>>>>>> That's an SoC-specific thing, the same regulator can be used with
>>>>>>>>>> many different ones. We can't just assume it'll always be like this.
>>>>>>>>>> I see the problem, but I believe this is not the correct solution
>>>>>>>> Okay, As we had discussions on reading back the voltage & the generic
>>>>>>>> DT property, do you suggest any other possible solutions here?
>>>>>>> Due to the sudden influx of various IPQ SoCs on the mailing list lately
>>>>>>> I have no idea if it concerned this one too, but at least one of them
>>>>>>> was said not to use RPM for controlling the clocks. If that's the case,
>>>>>>> I see no reason at all to use it for scaling the regulators, the PMIC
>>>>>>> could be addressed directly over I2C as a normal device. You'd probably
>>>>>>> want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
>>>>>>> simply not registering the CX/MX registers as children of the I2C
>>>>>>> regulator IC.
>>>>>>
>>>>>> IPQ9574 SoC has RPM and uses it for controlling the regulators.
>>>>>> Currently, the RPM firmware does not have read support implemented
>>>>>> and so, we were not able to read the bootup voltage.
>>>>>> As we randomly saw silent reboots when the kernel boots up,
>>>>>> do you think we could proceed with this change for time being
>>>>>> and revisit the same when any SoC in the future employs MP5496?
>>>>> I'm still thinking about a cleaner fix because hardcoding voltages
>>>>> in kernel is just dangerous. Could you check whether attaching a CPU
>>>>> supply and adding an OPP table where each level has an opp-microvolt
>>>>> property would resolve your issue?
>>>>>
>>>>> Konrad
>>>> Yes, Understood! We already have the CPU supply and OPP table where
>>>> each level has an opp-microvolt property changes in place and it did
>>>> not help in our case.
>>> Ouch. That totally sounds like a bug.. Would you be willing to dig
>>> a bit further into why this happens?
>> As the regulator registration happens before the cpu freq scaling comes
>> into picture, having an OPP table did not help to set the bootup voltage
> I see, but the order should not affect it. If your regulator driver
> returns -EPROBE_DEFER, so should the cpufreq one.
> 
> Konrad
Sorry konrad, don't exactly get your point here.
The cpufreq driver depends on the regulator driver to be probed as
the regulator serves as the cpu-supply.
But, when the regulator driver comes up, it tries to bring up the
regulators to the minimum supported voltage provided with the
regulator-min-microvolt property in the DT.

The regulator being the CPU only supply, updating the
regulator-min-microvolt with SVS voltage (725000uV) would ideally help
us with the issue. That way, we could update the DT and drop this patch.

This approach seems quite ideal to us. Shall we proceed with it if we 
don't foresee any issues?

Thanks,
Devi Priya

>>>
>>> Konrad
>>>> We have now planned to update the regulator-min-microvolt property
>>>> with the SVS voltage (725000uV) in the board DT such that the regulators
>>>> are brought up with the nominal voltage which would be sufficient
>>>> for all the corner parts operating at 800MHz.
>>>>
>>>> That way, we would update the DT and drop this patch in the next spin.
>>>>
>>>> Thanks,
>>>> Devi Priya
>>>>>>>
>>>>>>> Konrad
>>>>>>>>>>
>>>>>>>>>> Konrad
>>>>>>>>>>>
>>>>>>>>>>> Best Regards,
>>>>>>>>>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Konrad Dybcio]

On 3.04.2023 16:07, Devi Priya wrote:
> 
> 
> On 3/28/2023 1:58 PM, Konrad Dybcio wrote:
>>
>>
>> On 28.03.2023 08:12, Devi Priya wrote:
>>>
>>>
>>> On 3/27/2023 2:56 PM, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 27.03.2023 10:40, Devi Priya wrote:
>>>>>
>>>>>
>>>>> On 3/18/2023 7:38 PM, Konrad Dybcio wrote:
>>>>>>
>>>>>>
>>>>>> On 14.03.2023 18:15, Devi Priya wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 3/8/2023 3:57 PM, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 7.03.2023 07:55, Devi Priya wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 3/6/2023 6:39 PM, Devi Priya wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 3/3/2023 6:57 PM, Konrad Dybcio wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 3.03.2023 14:21, Devi Priya wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2/23/2023 4:31 AM, Mark Brown wrote:
>>>>>>>>>>>>> On Wed, Feb 22, 2023 at 11:11:42PM +0100, Konrad Dybcio wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thinking about it again, this seems like something that could be
>>>>>>>>>>>>>> generalized and introduced into regulator core.. Hardcoding this
>>>>>>>>>>>>>> will not end well.. Not to mention it'll affect all mp5496-using
>>>>>>>>>>>>>> boards that are already upstream.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> WDYT about regulator-init-microvolts Mark?
>>>>>>>>>>>>>
>>>>>>>>>>>>> The overwhelming majority of devices that have variable voltages
>>>>>>>>>>>>> support readback, these Qualcomm firmware devices are pretty much
>>>>>>>>>>>>> unique in this regard.  We don't want a general property to set a
>>>>>>>>>>>>> specific voltage since normally we should be using the
>>>>>>>>>>>>> constraints and don't normally need to adjust things immediately
>>>>>>>>>>>>> since we can tell what the current voltage is.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is pretty much just going to be a device specific bodge,
>>>>>>>>>>>>> ideally something that does know what the voltage is would be
>>>>>>>>>>>>> able to tell us at runtime but if that's not possible then
>>>>>>>>>>>>> there's no good options.  If the initial voltage might vary based
>>>>>>>>>>>>> on board then a device specific DT property might be less
>>>>>>>>>>>>> terrible, if it's determined by the regulator the current code
>>>>>>>>>>>>> seems fine.  Or just leave the current behavour, if the
>>>>>>>>>>>>> constraints are accurate then hopefully a temporary dip in
>>>>>>>>>>>>> voltage is just inelegant rather than an issue.  Indeed the
>>>>>>>>>>>>> current behaviour might well save power if you've got a voltage
>>>>>>>>>>>>> range configured and nothing actually ever gets round to setting
>>>>>>>>>>>>> the voltage (which is depressingly common, people seem keen on
>>>>>>>>>>>>> setting voltage ranges even when the voltage is never varied in
>>>>>>>>>>>>> practice).
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Mark, The initial bootup voltage is actually blown into the OTP register of the PMIC and it remains the same across boards for IPQ9574 SoC.
>>>>>>>>>>> But what about IPQ6018 which also uses MP5496? That's also gonna
>>>>>>>>>>> set the voltage on there, it may be too high/low..
>>>>>>>>> For IPQ6018, the bootup voltage is the same as that of IPQ9574 which is
>>>>>>>>> 875mV
>>>>>>>> Okay, but what about any other design that employs or may employ
>>>>>>>> MP5496 in the future?
>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>       Initially the SoC runs at 800MHz with a voltage of 875mV set by the bootloaders. As kernel does not know the initial voltage, during regulator registration the framework considers the current voltage to be zero and tries to bring up the regulator to minimum supported voltage of 600mV. This causes the dip which might be of concern in SS parts where the voltage might be insufficient leading to silent reboots.
>>>>>>>>>>> That's an SoC-specific thing, the same regulator can be used with
>>>>>>>>>>> many different ones. We can't just assume it'll always be like this.
>>>>>>>>>>> I see the problem, but I believe this is not the correct solution
>>>>>>>>> Okay, As we had discussions on reading back the voltage & the generic
>>>>>>>>> DT property, do you suggest any other possible solutions here?
>>>>>>>> Due to the sudden influx of various IPQ SoCs on the mailing list lately
>>>>>>>> I have no idea if it concerned this one too, but at least one of them
>>>>>>>> was said not to use RPM for controlling the clocks. If that's the case,
>>>>>>>> I see no reason at all to use it for scaling the regulators, the PMIC
>>>>>>>> could be addressed directly over I2C as a normal device. You'd probably
>>>>>>>> want to keep VDD_[CM]X scaling through rpmpd, but it's easily done by
>>>>>>>> simply not registering the CX/MX registers as children of the I2C
>>>>>>>> regulator IC.
>>>>>>>
>>>>>>> IPQ9574 SoC has RPM and uses it for controlling the regulators.
>>>>>>> Currently, the RPM firmware does not have read support implemented
>>>>>>> and so, we were not able to read the bootup voltage.
>>>>>>> As we randomly saw silent reboots when the kernel boots up,
>>>>>>> do you think we could proceed with this change for time being
>>>>>>> and revisit the same when any SoC in the future employs MP5496?
>>>>>> I'm still thinking about a cleaner fix because hardcoding voltages
>>>>>> in kernel is just dangerous. Could you check whether attaching a CPU
>>>>>> supply and adding an OPP table where each level has an opp-microvolt
>>>>>> property would resolve your issue?
>>>>>>
>>>>>> Konrad
>>>>> Yes, Understood! We already have the CPU supply and OPP table where
>>>>> each level has an opp-microvolt property changes in place and it did
>>>>> not help in our case.
>>>> Ouch. That totally sounds like a bug.. Would you be willing to dig
>>>> a bit further into why this happens?
>>> As the regulator registration happens before the cpu freq scaling comes
>>> into picture, having an OPP table did not help to set the bootup voltage
>> I see, but the order should not affect it. If your regulator driver
>> returns -EPROBE_DEFER, so should the cpufreq one.
>>
>> Konrad
> Sorry konrad, don't exactly get your point here.
> The cpufreq driver depends on the regulator driver to be probed as
> the regulator serves as the cpu-supply.
> But, when the regulator driver comes up, it tries to bring up the
> regulators to the minimum supported voltage provided with the
> regulator-min-microvolt property in the DT.
Right, that exists.. 

Mark, do you think it should be updated such that the requests are
aggregated before assuming min_uV is "just fine"?

> 
> The regulator being the CPU only supply, updating the
> regulator-min-microvolt with SVS voltage (725000uV) would ideally help
> us with the issue. That way, we could update the DT and drop this patch.
> 
> This approach seems quite ideal to us. Shall we proceed with it if we don't foresee any issues?
Yes, please do so as a temporary measure and leave a comment about
it in the device tree so that it's not forgotten about. Ideally
both scaling up and down should be supported..

Konrad
> 
> Thanks,
> Devi Priya
> 
>>>>
>>>> Konrad
>>>>> We have now planned to update the regulator-min-microvolt property
>>>>> with the SVS voltage (725000uV) in the board DT such that the regulators
>>>>> are brought up with the nominal voltage which would be sufficient
>>>>> for all the corner parts operating at 800MHz.
>>>>>
>>>>> That way, we would update the DT and drop this patch in the next spin.
>>>>>
>>>>> Thanks,
>>>>> Devi Priya
>>>>>>>>
>>>>>>>> Konrad
>>>>>>>>>>>
>>>>>>>>>>> Konrad
>>>>>>>>>>>>
>>>>>>>>>>>> Best Regards,
>>>>>>>>>>>> Devi Priya

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

On Mon, Apr 03, 2023 at 07:53:48PM +0200, Konrad Dybcio wrote:
> On 3.04.2023 16:07, Devi Priya wrote:

> > But, when the regulator driver comes up, it tries to bring up the
> > regulators to the minimum supported voltage provided with the
> > regulator-min-microvolt property in the DT.

> Right, that exists.. 

> Mark, do you think it should be updated such that the requests are
> aggregated before assuming min_uV is "just fine"?

We can't tell if any consumers are ever going to appear, and the
regulator having a voltage outside of the constraints is an urgent
problem we need to fix quickly.  Since we try to bring the voltage to
the nearest end of the constraint the driver could always change the
bogus voltage it reports to one that is excessively high, this would
mean the core will try to bring the voltage down to the maximum rather
than up to the minimum.  The driver could also look at the constraints
when guessing at the hardware configuration rather than claiming an out
of spec voltage, this would mean we wouldn't need to correct anything.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Konrad Dybcio]

On 3.04.2023 20:14, Mark Brown wrote:
> On Mon, Apr 03, 2023 at 07:53:48PM +0200, Konrad Dybcio wrote:
>> On 3.04.2023 16:07, Devi Priya wrote:
> 
>>> But, when the regulator driver comes up, it tries to bring up the
>>> regulators to the minimum supported voltage provided with the
>>> regulator-min-microvolt property in the DT.
> 
>> Right, that exists.. 
> 
>> Mark, do you think it should be updated such that the requests are
>> aggregated before assuming min_uV is "just fine"?
> 
> We can't tell if any consumers are ever going to appear, and the
> regulator having a voltage outside of the constraints is an urgent
> problem we need to fix quickly.  Since we try to bring the voltage to
> the nearest end of the constraint the driver could always change the
> bogus voltage it reports to one that is excessively high, this would
> mean the core will try to bring the voltage down to the maximum rather
> than up to the minimum.  The driver could also look at the constraints
> when guessing at the hardware configuration rather than claiming an out
> of spec voltage, this would mean we wouldn't need to correct anything.
Hm, all of what you said sounds like a valid concern.. And then we
probably shouldn't shoot up to max by default, as going too low is
not going to cause as much potential irreversible damage as going
too high.. Especially with programmer error..

Too bad Qualcomm's firmware architecture doesn't allow for reading
back the voltage..

Konrad

Re: [PATCH V2 4/6] regulator: qcom_smd: Add support to define the bootup voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Mon, Apr 03, 2023 at 08:21:25PM +0200, Konrad Dybcio wrote:
> On 3.04.2023 20:14, Mark Brown wrote:

> > than up to the minimum.  The driver could also look at the constraints
> > when guessing at the hardware configuration rather than claiming an out
> > of spec voltage, this would mean we wouldn't need to correct anything.

> Hm, all of what you said sounds like a valid concern.. And then we
> probably shouldn't shoot up to max by default, as going too low is
> not going to cause as much potential irreversible damage as going
> too high.. Especially with programmer error..

It sounds like the driver should just be reporting a value which is at
least within the constraints.

> Too bad Qualcomm's firmware architecture doesn't allow for reading
> back the voltage..

Right, their interfaces here have some serious drawbacks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]