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

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

[devi priya]

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

This series depends on the below patchset
https://lore.kernel.org/linux-arm-msm/20230113143647.14961-1-quic_devipriy@quicinc.com/

devi priya (6):
  soc: qcom: smd-rpm: Add IPQ9574 compatible
  dt-bindings: soc: qcom: smd-rpm: Add IPQ9574 compatible string
  regulator: qcom_smd: Add MP5496 regulators
  regulator: qcom_smd: Add PMIC compatible for IPQ9574
  arm64: dts: qcom: ipq9574: Add cpufreq & RPM related nodes
  regulator: qcom_smd: Add support to define the bootup voltage

 .../regulator/qcom,smd-rpm-regulator.yaml     |  3 +-
 .../bindings/soc/qcom/qcom,smd-rpm.yaml       |  1 +
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         | 80 +++++++++++++++++++
 drivers/regulator/qcom_smd-regulator.c        | 20 +++++
 drivers/soc/qcom/smd-rpm.c                    |  1 +
 5 files changed, 104 insertions(+), 1 deletion(-)


base-commit: 1fe4fd6f5cad346e598593af36caeadc4f5d4fa9
-- 
2.17.1

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

[devi priya]

Adding compatible string to support RPM communication over SMD for
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>
---
 drivers/soc/qcom/smd-rpm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/qcom/smd-rpm.c b/drivers/soc/qcom/smd-rpm.c
index 7e3b6a7ea34c..523627d5d398 100644
--- a/drivers/soc/qcom/smd-rpm.c
+++ b/drivers/soc/qcom/smd-rpm.c
@@ -233,6 +233,7 @@ static void qcom_smd_rpm_remove(struct rpmsg_device *rpdev)
 static const struct of_device_id qcom_smd_rpm_of_match[] = {
 	{ .compatible = "qcom,rpm-apq8084" },
 	{ .compatible = "qcom,rpm-ipq6018" },
+	{ .compatible = "qcom,rpm-ipq9574" },
 	{ .compatible = "qcom,rpm-msm8226" },
 	{ .compatible = "qcom,rpm-msm8909" },
 	{ .compatible = "qcom,rpm-msm8916" },
-- 
2.17.1

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

[devi priya]

Add the dt-bindings for the RPM communication over SMD for 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>
---
 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 11c0f4dd797c..2e8c9e937424 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 3/6] regulator: qcom_smd: Add MP5496 regulators

[devi priya]

Adding support for PMIC MP5496 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>
---
 drivers/regulator/qcom_smd-regulator.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index 9f2b58458841..1eb17d378897 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -767,6 +767,15 @@ static const struct regulator_desc mp5496_ldoa2 = {
 	.ops = &rpm_mp5496_ops,
 };
 
+static const struct regulator_desc ipq9574_mp5496_smpa1 = {
+	.linear_ranges = (struct linear_range[]) {
+		REGULATOR_LINEAR_RANGE(600000, 0, 37, 12500),
+	},
+	.n_linear_ranges = 1,
+	.n_voltages = 38,
+	.ops = &rpm_mp5496_ops,
+};
+
 static const struct regulator_desc pm2250_lvftsmps = {
 	.linear_ranges = (struct linear_range[]) {
 		REGULATOR_LINEAR_RANGE(320000, 0, 269, 4000),
@@ -799,6 +808,11 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
 	{}
 };
 
+static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
+	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
+	{}
+};
+
 static const struct rpm_regulator_data rpm_pm2250_regulators[] = {
 	{ "s1", QCOM_SMD_RPM_SMPA, 1, &pm2250_lvftsmps, "vdd_s1" },
 	{ "s2", QCOM_SMD_RPM_SMPA, 2, &pm2250_lvftsmps, "vdd_s2" },
@@ -1320,6 +1334,8 @@ static const struct rpm_regulator_data rpm_pms405_regulators[] = {
 };
 
 static const struct of_device_id rpm_of_match[] = {
+	{ .compatible = "qcom,rpm-ipq9574-mp5496-regulators",
+		.data = &rpm_ipq9574_mp5496_regulators },
 	{ .compatible = "qcom,rpm-mp5496-regulators", .data = &rpm_mp5496_regulators },
 	{ .compatible = "qcom,rpm-pm2250-regulators", .data = &rpm_pm2250_regulators },
 	{ .compatible = "qcom,rpm-pm6125-regulators", .data = &rpm_pm6125_regulators },
-- 
2.17.1

[PATCH 4/6] regulator: qcom_smd: Add PMIC compatible for IPQ9574

[devi priya]

Add mp5496 PMIC compatible string for 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>
---
 .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml  | 3 ++-
 1 file changed, 2 insertions(+), 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..7907d9385583 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
@@ -84,6 +84,7 @@ properties:
   compatible:
     enum:
       - qcom,rpm-mp5496-regulators
+      - qcom,rpm-ipq9574-mp5496-regulators
       - qcom,rpm-pm2250-regulators
       - qcom,rpm-pm6125-regulators
       - qcom,rpm-pm660-regulators
-- 
2.17.1

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

[devi priya]

Add CPU Freq and RPM related nodes in the device tree

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>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 80 +++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 5a2244b437ed..79fa5d91882c 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -9,6 +9,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/qcom,gcc-ipq9574.h>
 #include <dt-bindings/reset/qcom,gcc-ipq9574.h>
+#include <dt-bindings/clock/qcom,apss-ipq.h>
 
 / {
 	interrupt-parent = <&intc>;
@@ -75,6 +76,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>;
+			cpu0-supply = <&ipq9574_s1>;
 		};
 
 		CPU1: cpu@1 {
@@ -83,6 +88,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 {
@@ -91,6 +100,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 {
@@ -99,6 +112,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 {
@@ -107,6 +124,42 @@
 		};
 	};
 
+	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>;
+		};
+	};
+
 	memory@40000000 {
 		device_type = "memory";
 		/* We expect the bootloader to fill in the size */
@@ -128,6 +181,11 @@
 		#size-cells = <2>;
 		ranges;
 
+		rpm_msg_ram: memory@60000 {
+			reg = <0x0 0x00060000 0x0 0x6000>;
+			no-map;
+		};
+
 		tz_region: memory@4a600000 {
 			reg = <0x0 0x4a600000 0x0 0x400000>;
 			no-map;
@@ -324,6 +382,28 @@
 		};
 	};
 
+	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";
+
+			regulators {
+				compatible = "qcom,rpm-ipq9574-mp5496-regulators";
+
+				ipq9574_s1: s1 {
+					regulator-min-microvolt = <587500>;
+					regulator-max-microvolt = <1075000>;
+					regulator-always-on;
+				};
+			};
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
-- 
2.17.1

[PATCH 6/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>
---
 drivers/regulator/qcom_smd-regulator.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index 1eb17d378897..49a36b07397c 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -800,6 +800,7 @@ 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[] = {
@@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
 };
 
 static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
-	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
+	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
 	{}
 };
 
@@ -1394,6 +1395,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

Re: [PATCH 3/6] regulator: qcom_smd: Add MP5496 regulators

[Konrad Dybcio]

On 13.01.2023 16:03, devi priya wrote:
> Adding support for PMIC MP5496 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>
> ---
Please simply extend the existing MP5496 support with this
S1 regulator. If you don't explicitly define and set voltages
for the other vregs, they will not be probed.

Konrad
>  drivers/regulator/qcom_smd-regulator.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> index 9f2b58458841..1eb17d378897 100644
> --- a/drivers/regulator/qcom_smd-regulator.c
> +++ b/drivers/regulator/qcom_smd-regulator.c
> @@ -767,6 +767,15 @@ static const struct regulator_desc mp5496_ldoa2 = {
>  	.ops = &rpm_mp5496_ops,
>  };
>  
> +static const struct regulator_desc ipq9574_mp5496_smpa1 = {
> +	.linear_ranges = (struct linear_range[]) {
> +		REGULATOR_LINEAR_RANGE(600000, 0, 37, 12500),
> +	},
> +	.n_linear_ranges = 1,
> +	.n_voltages = 38,
> +	.ops = &rpm_mp5496_ops,
> +};
> +
>  static const struct regulator_desc pm2250_lvftsmps = {
>  	.linear_ranges = (struct linear_range[]) {
>  		REGULATOR_LINEAR_RANGE(320000, 0, 269, 4000),
> @@ -799,6 +808,11 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>  	{}
>  };
>  
> +static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
> +	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
> +	{}
> +};
> +
>  static const struct rpm_regulator_data rpm_pm2250_regulators[] = {
>  	{ "s1", QCOM_SMD_RPM_SMPA, 1, &pm2250_lvftsmps, "vdd_s1" },
>  	{ "s2", QCOM_SMD_RPM_SMPA, 2, &pm2250_lvftsmps, "vdd_s2" },
> @@ -1320,6 +1334,8 @@ static const struct rpm_regulator_data rpm_pms405_regulators[] = {
>  };
>  
>  static const struct of_device_id rpm_of_match[] = {
> +	{ .compatible = "qcom,rpm-ipq9574-mp5496-regulators",
> +		.data = &rpm_ipq9574_mp5496_regulators },
>  	{ .compatible = "qcom,rpm-mp5496-regulators", .data = &rpm_mp5496_regulators },
>  	{ .compatible = "qcom,rpm-pm2250-regulators", .data = &rpm_pm2250_regulators },
>  	{ .compatible = "qcom,rpm-pm6125-regulators", .data = &rpm_pm6125_regulators },

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

[Konrad Dybcio]

On 13.01.2023 16:03, devi priya wrote:
> Add CPU Freq and RPM related nodes in the device tree
These two are wildly different things, barely related to one
another and can very well be introduced in separate patches.
Please do so.

> 
> 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>
> ---
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 80 +++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 5a2244b437ed..79fa5d91882c 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -9,6 +9,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-ipq9574.h>
>  #include <dt-bindings/reset/qcom,gcc-ipq9574.h>
> +#include <dt-bindings/clock/qcom,apss-ipq.h>
Please sort the includes alphabetically.

>  
>  / {
>  	interrupt-parent = <&intc>;
> @@ -75,6 +76,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>;
> +			cpu0-supply = <&ipq9574_s1>;
Why is this cpu0-supply and the rest are cpu-supply? Neither of them
seem particularly documented, by the way..


>  		};
>  
>  		CPU1: cpu@1 {
> @@ -83,6 +88,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 {
> @@ -91,6 +100,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 {
> @@ -99,6 +112,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 {
> @@ -107,6 +124,42 @@
>  		};
>  	};
>  
> +	cpu_opp_table: opp-table-cpu {
Alphabetically this goes after memory

> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		opp-936000000 {
> +			opp-hz = /bits/ 64 <936000000>;
> +			opp-microvolt = <725000>;
> +			clock-latency-ns = <200000>;
> +		};
Please add a newline between each subnode.

> +		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>;
> +		};
> +	};
> +
>  	memory@40000000 {
>  		device_type = "memory";
>  		/* We expect the bootloader to fill in the size */
> @@ -128,6 +181,11 @@
>  		#size-cells = <2>;
>  		ranges;
>  
> +		rpm_msg_ram: memory@60000 {
> +			reg = <0x0 0x00060000 0x0 0x6000>;
> +			no-map;
> +		};
> +
>  		tz_region: memory@4a600000 {
>  			reg = <0x0 0x4a600000 0x0 0x400000>;
>  			no-map;
> @@ -324,6 +382,28 @@
>  		};
>  	};
>  
> +	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";
> +
> +			regulators {
> +				compatible = "qcom,rpm-ipq9574-mp5496-regulators";
The regulators are board-specific and should not be included in the
SoC DTSI. If this is a very common configuration, you may split that
into ipq9574-mp5496.dtsi, for example. Or ipq9574-pmics.dtsi if it's
coupled with more PMICs.

> +
> +				ipq9574_s1: s1 {
> +					regulator-min-microvolt = <587500>;
> +					regulator-max-microvolt = <1075000>;
> +					regulator-always-on;
Won't this break CPU retention?

You're holding a vote on it from the CPU devices, so it should be
always enabled when the CPUs are oneline (as far as Linux is
concerned).


Or maybe Linux will think it's enabled and RPM will quietly park
it when it decides it's good to do so.. but will it with an active
request.. not sure, really.. just something to consider..

Konrad
> +				};
> +			};
> +		};
> +	};
> +
>  	timer {
>  		compatible = "arm,armv8-timer";
>  		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,

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

[Konrad Dybcio]

On 13.01.2023 16:03, 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>
> ---
Or maybe hook it up to the spmi_regulator_common_get_voltage()
from the SPMI regulator driver and read the real voltage instead
of relying on hardcoded values thay may differ between boards?

Konrad
>  drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> index 1eb17d378897..49a36b07397c 100644
> --- a/drivers/regulator/qcom_smd-regulator.c
> +++ b/drivers/regulator/qcom_smd-regulator.c
> @@ -800,6 +800,7 @@ 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[] = {
> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>  };
>  
>  static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
> -	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
> +	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
>  	{}
>  };
>  
> @@ -1394,6 +1395,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 1/6] soc: qcom: smd-rpm: Add IPQ9574 compatible

[Krzysztof Kozlowski]

On 13/01/2023 16:03, devi priya wrote:
> Adding compatible string to support RPM communication over SMD for
> IPQ9574 SoC
> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>

What exactly was developed here but the other author?

> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---
>  drivers/soc/qcom/smd-rpm.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 13/01/2023 16:03, devi priya wrote:
> Add the dt-bindings for the RPM communication over SMD for IPQ9574 SoC
> 
> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>

That's just one line, one compatible. Which part of the patch is
authored by other person?

> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> ---

Best regards,
Krzysztof

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

[Mark Brown]

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

On Fri, Jan 13, 2023 at 05:42:36PM +0100, Krzysztof Kozlowski wrote:
> On 13/01/2023 16:03, devi priya wrote:
> > Adding compatible string to support RPM communication over SMD for
> > IPQ9574 SoC
> > 
> > Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> > Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> 
> What exactly was developed here but the other author?

It's fairly clear looking at this in the context of the series
that the same tags have been applied to every patch in the
series.  Probably a patch like this was actually written by just
one person but there's a decent chance that it's just been
forgotten who it was and fundamentally it just doesn't matter
that much.

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

Re: [PATCH 4/6] regulator: qcom_smd: Add PMIC compatible for IPQ9574

[Rob Herring]

On Fri, Jan 13, 2023 at 08:33:08PM +0530, devi priya wrote:
> Add mp5496 PMIC compatible string for 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>
> ---
>  .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml  | 3 ++-
>  1 file changed, 2 insertions(+), 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..7907d9385583 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
> @@ -84,6 +84,7 @@ properties:
>    compatible:
>      enum:
>        - qcom,rpm-mp5496-regulators
> +      - qcom,rpm-ipq9574-mp5496-regulators

Is this a different part than just mp5496? Or used in a different, 
incompatible way?

>        - qcom,rpm-pm2250-regulators
>        - qcom,rpm-pm6125-regulators
>        - qcom,rpm-pm660-regulators
> -- 
> 2.17.1
> 

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

[Devi Priya]

On 1/13/2023 10:12 PM, Krzysztof Kozlowski wrote:
> On 13/01/2023 16:03, devi priya wrote:
>> Adding compatible string to support RPM communication over SMD for
>> IPQ9574 SoC
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> 
> What exactly was developed here but the other author?
> 
The intention was to mention folks who supported with the patch series 
development using the co-developed-by tag on the individual patches.
But yeah, for patches with minimal changes, will trim down the tag as 
suggested.


>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
>>   drivers/soc/qcom/smd-rpm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
> 
> Best regards,
> Krzysztof
> 
Best Regards,
Devi Priya

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

[Devi Priya]

On 1/16/2023 4:42 PM, Mark Brown wrote:
> On Fri, Jan 13, 2023 at 05:42:36PM +0100, Krzysztof Kozlowski wrote:
>> On 13/01/2023 16:03, devi priya wrote:
>>> Adding compatible string to support RPM communication over SMD for
>>> IPQ9574 SoC
>>>
>>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>
>> What exactly was developed here but the other author?
> 
> It's fairly clear looking at this in the context of the series
> that the same tags have been applied to every patch in the
> series.  Probably a patch like this was actually written by just
> one person but there's a decent chance that it's just been
> forgotten who it was and fundamentally it just doesn't matter
> that much.

Yeah!
Will drop the tag


Best Regards,
Devi Priya

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

[Devi Priya]

On 1/13/2023 10:13 PM, Krzysztof Kozlowski wrote:
> On 13/01/2023 16:03, devi priya wrote:
>> Add the dt-bindings for the RPM communication over SMD for IPQ9574 SoC
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> 
> That's just one line, one compatible. Which part of the patch is
> authored by other person?
> 
Okay, will drop the tag
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
> 
> Best regards,
> Krzysztof
> 
Best Regards,
Devi Priya

Re: [PATCH 3/6] regulator: qcom_smd: Add MP5496 regulators

[Devi Priya]

On 1/13/2023 8:54 PM, Konrad Dybcio wrote:
> 
> 
> On 13.01.2023 16:03, devi priya wrote:
>> Adding support for PMIC MP5496 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>
>> ---
> Please simply extend the existing MP5496 support with this
> S1 regulator. If you don't explicitly define and set voltages
> for the other vregs, they will not be probed.
> 
> Konrad
IPQ6018 and IPQ9574 platforms use the same PMIC MP5496 but they have a 
different power layout. IPQ9574 has S2 regulator which will be used for 
NSS scaling but S2 in IPQ6018 serves a different purpose. Hence it would 
not be possible to extend the existing MP5496 support for IPQ9574
>>   drivers/regulator/qcom_smd-regulator.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>> index 9f2b58458841..1eb17d378897 100644
>> --- a/drivers/regulator/qcom_smd-regulator.c
>> +++ b/drivers/regulator/qcom_smd-regulator.c
>> @@ -767,6 +767,15 @@ static const struct regulator_desc mp5496_ldoa2 = {
>>   	.ops = &rpm_mp5496_ops,
>>   };
>>   
>> +static const struct regulator_desc ipq9574_mp5496_smpa1 = {
>> +	.linear_ranges = (struct linear_range[]) {
>> +		REGULATOR_LINEAR_RANGE(600000, 0, 37, 12500),
>> +	},
>> +	.n_linear_ranges = 1,
>> +	.n_voltages = 38,
>> +	.ops = &rpm_mp5496_ops,
>> +};
>> +
>>   static const struct regulator_desc pm2250_lvftsmps = {
>>   	.linear_ranges = (struct linear_range[]) {
>>   		REGULATOR_LINEAR_RANGE(320000, 0, 269, 4000),
>> @@ -799,6 +808,11 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>   	{}
>>   };
>>   
>> +static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>> +	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>> +	{}
>> +};
>> +
>>   static const struct rpm_regulator_data rpm_pm2250_regulators[] = {
>>   	{ "s1", QCOM_SMD_RPM_SMPA, 1, &pm2250_lvftsmps, "vdd_s1" },
>>   	{ "s2", QCOM_SMD_RPM_SMPA, 2, &pm2250_lvftsmps, "vdd_s2" },
>> @@ -1320,6 +1334,8 @@ static const struct rpm_regulator_data rpm_pms405_regulators[] = {
>>   };
>>   
>>   static const struct of_device_id rpm_of_match[] = {
>> +	{ .compatible = "qcom,rpm-ipq9574-mp5496-regulators",
>> +		.data = &rpm_ipq9574_mp5496_regulators },
>>   	{ .compatible = "qcom,rpm-mp5496-regulators", .data = &rpm_mp5496_regulators },
>>   	{ .compatible = "qcom,rpm-pm2250-regulators", .data = &rpm_pm2250_regulators },
>>   	{ .compatible = "qcom,rpm-pm6125-regulators", .data = &rpm_pm6125_regulators },
Best Regards,
Devi Priya

Re: [PATCH 3/6] regulator: qcom_smd: Add MP5496 regulators

[Konrad Dybcio]

On 27.01.2023 17:01, Devi Priya wrote:
> 
> 
> On 1/13/2023 8:54 PM, Konrad Dybcio wrote:
>>
>>
>> On 13.01.2023 16:03, devi priya wrote:
>>> Adding support for PMIC MP5496 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>
>>> ---
>> Please simply extend the existing MP5496 support with this
>> S1 regulator. If you don't explicitly define and set voltages
>> for the other vregs, they will not be probed.
>>
>> Konrad
> IPQ6018 and IPQ9574 platforms use the same PMIC MP5496 but they have a different power layout. IPQ9574 has S2 regulator which will be used for NSS scaling but S2 in IPQ6018 serves a different purpose. Hence it would not be possible to extend the existing MP5496 support for IPQ9574
Does the s2 on IPQ9574 have a different voltage range than
the one on IPQ6018? No? Then there's nothing blocking you
from using the setup for both SoCs. As I've mentioned,
regulators that you don't add to the device tree will
not even be probed.

Konrad
>>>   drivers/regulator/qcom_smd-regulator.c | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>> index 9f2b58458841..1eb17d378897 100644
>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>> @@ -767,6 +767,15 @@ static const struct regulator_desc mp5496_ldoa2 = {
>>>       .ops = &rpm_mp5496_ops,
>>>   };
>>>   +static const struct regulator_desc ipq9574_mp5496_smpa1 = {
>>> +    .linear_ranges = (struct linear_range[]) {
>>> +        REGULATOR_LINEAR_RANGE(600000, 0, 37, 12500),
>>> +    },
>>> +    .n_linear_ranges = 1,
>>> +    .n_voltages = 38,
>>> +    .ops = &rpm_mp5496_ops,
>>> +};
>>> +
>>>   static const struct regulator_desc pm2250_lvftsmps = {
>>>       .linear_ranges = (struct linear_range[]) {
>>>           REGULATOR_LINEAR_RANGE(320000, 0, 269, 4000),
>>> @@ -799,6 +808,11 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>>       {}
>>>   };
>>>   +static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>>> +    {}
>>> +};
>>> +
>>>   static const struct rpm_regulator_data rpm_pm2250_regulators[] = {
>>>       { "s1", QCOM_SMD_RPM_SMPA, 1, &pm2250_lvftsmps, "vdd_s1" },
>>>       { "s2", QCOM_SMD_RPM_SMPA, 2, &pm2250_lvftsmps, "vdd_s2" },
>>> @@ -1320,6 +1334,8 @@ static const struct rpm_regulator_data rpm_pms405_regulators[] = {
>>>   };
>>>     static const struct of_device_id rpm_of_match[] = {
>>> +    { .compatible = "qcom,rpm-ipq9574-mp5496-regulators",
>>> +        .data = &rpm_ipq9574_mp5496_regulators },
>>>       { .compatible = "qcom,rpm-mp5496-regulators", .data = &rpm_mp5496_regulators },
>>>       { .compatible = "qcom,rpm-pm2250-regulators", .data = &rpm_pm2250_regulators },
>>>       { .compatible = "qcom,rpm-pm6125-regulators", .data = &rpm_pm6125_regulators },
> Best Regards,
> Devi Priya

Re: [PATCH 4/6] regulator: qcom_smd: Add PMIC compatible for IPQ9574

[Devi Priya]

On 1/18/2023 12:08 AM, Rob Herring wrote:
> On Fri, Jan 13, 2023 at 08:33:08PM +0530, devi priya wrote:
>> Add mp5496 PMIC compatible string for 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>
>> ---
>>   .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml  | 3 ++-
>>   1 file changed, 2 insertions(+), 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..7907d9385583 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
>> @@ -84,6 +84,7 @@ properties:
>>     compatible:
>>       enum:
>>         - qcom,rpm-mp5496-regulators
>> +      - qcom,rpm-ipq9574-mp5496-regulators
> 
> Is this a different part than just mp5496? Or used in a different,
> incompatible way?
IPQ6018 and IPQ9574 platforms use the same PMIC MP5496 but they have a 
different power layout.So, we plan to update the compatible: 
qcom,rpm-mp5496-regulators to 
qcom,rpm-ipq6018-mp5496-regulators(target-specific) in the next patchset 
as the regulators serve different purposes
> 
>>         - qcom,rpm-pm2250-regulators
>>         - qcom,rpm-pm6125-regulators
>>         - qcom,rpm-pm660-regulators
>> -- 
>> 2.17.1
>>
Best Regards,
Devi Priya

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

[Devi Priya]

On 1/13/2023 9:07 PM, Konrad Dybcio wrote:
> 
> 
> On 13.01.2023 16:03, 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>
>> ---
> Or maybe hook it up to the spmi_regulator_common_get_voltage()
> from the SPMI regulator driver and read the real voltage instead
> of relying on hardcoded values thay may differ between boards?
> 
> Konrad
In IPQ9574, SPMI regulator is not used. We are using RPM-Glink 
communication and the regulators are controlled by RPM.
In this case, we don't have an option to readback the bootup voltage and 
so, we have hardcoded the values

>>   drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>> index 1eb17d378897..49a36b07397c 100644
>> --- a/drivers/regulator/qcom_smd-regulator.c
>> +++ b/drivers/regulator/qcom_smd-regulator.c
>> @@ -800,6 +800,7 @@ 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[] = {
>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>   };
>>   
>>   static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>> -	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>> +	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
>>   	{}
>>   };
>>   
>> @@ -1394,6 +1395,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;
Best Regards,
Devi Priya

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

[Konrad Dybcio]

On 27.01.2023 17:07, Devi Priya wrote:
> 
> 
> On 1/13/2023 9:07 PM, Konrad Dybcio wrote:
>>
>>
>> On 13.01.2023 16:03, 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>
>>> ---
>> Or maybe hook it up to the spmi_regulator_common_get_voltage()
>> from the SPMI regulator driver and read the real voltage instead
>> of relying on hardcoded values thay may differ between boards?
>>
>> Konrad
> In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM.
> In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values
Unless something changed, RPM regulator framework is simply a
fancy front-end for communicating with the PMIC over SPMI, AFAIK..

Konrad
> 
>>>   drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>> index 1eb17d378897..49a36b07397c 100644
>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>> @@ -800,6 +800,7 @@ 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[] = {
>>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>>   };
>>>     static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>>> -    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
>>>       {}
>>>   };
>>>   @@ -1394,6 +1395,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;
> Best Regards,
> Devi Priya

Re: [PATCH 4/6] regulator: qcom_smd: Add PMIC compatible for IPQ9574

[Rob Herring]

On Fri, Jan 27, 2023 at 10:05 AM Devi Priya <quic_devipriy@quicinc.com> wrote:
>
>
>
> On 1/18/2023 12:08 AM, Rob Herring wrote:
> > On Fri, Jan 13, 2023 at 08:33:08PM +0530, devi priya wrote:
> >> Add mp5496 PMIC compatible string for 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>
> >> ---
> >>   .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml  | 3 ++-
> >>   1 file changed, 2 insertions(+), 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..7907d9385583 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
> >> @@ -84,6 +84,7 @@ properties:
> >>     compatible:
> >>       enum:
> >>         - qcom,rpm-mp5496-regulators
> >> +      - qcom,rpm-ipq9574-mp5496-regulators
> >
> > Is this a different part than just mp5496? Or used in a different,
> > incompatible way?
> IPQ6018 and IPQ9574 platforms use the same PMIC MP5496 but they have a
> different power layout.So, we plan to update the compatible:
> qcom,rpm-mp5496-regulators to
> qcom,rpm-ipq6018-mp5496-regulators(target-specific) in the next patchset
> as the regulators serve different purposes

You can't just change compatibles. It is an ABI.

This still doesn't make sense to me. The PMIC hasn't changed, so the
binding shouldn't. It should be flexible enough to be hooked up to
different platforms. That's why we have all the per regulator
configuration. What are you missing?

Rob

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

[Devi Priya]

On 1/27/2023 9:40 PM, Konrad Dybcio wrote:
> 
> 
> On 27.01.2023 17:07, Devi Priya wrote:
>>
>>
>> On 1/13/2023 9:07 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 13.01.2023 16:03, 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>
>>>> ---
>>> Or maybe hook it up to the spmi_regulator_common_get_voltage()
>>> from the SPMI regulator driver and read the real voltage instead
>>> of relying on hardcoded values thay may differ between boards?
>>>
>>> Konrad
>> In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM.
>> In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values
> Unless something changed, RPM regulator framework is simply a
> fancy front-end for communicating with the PMIC over SPMI, AFAIK..
> 
> Konrad
Currently in our driver, the voltage write request will be sent to RPM 
via GLINK which then writes it to the PMIC over I2C using the below APIs
qcom_rpm_smd_write -> rpmsg_send
In IPQ9574, we do not have SPMI support or the support to readback voltage.

>>
>>>>    drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>>> index 1eb17d378897..49a36b07397c 100644
>>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>>> @@ -800,6 +800,7 @@ 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[] = {
>>>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>>>    };
>>>>      static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>>>> -    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>>>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
>>>>        {}
>>>>    };
>>>>    @@ -1394,6 +1395,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;
>> Best Regards,
>> Devi Priya
Best Regards,
Devi Priya

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

[Dmitry Baryshkov]

On 13/01/2023 17:03, 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>
> ---
>   drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> index 1eb17d378897..49a36b07397c 100644
> --- a/drivers/regulator/qcom_smd-regulator.c
> +++ b/drivers/regulator/qcom_smd-regulator.c
> @@ -800,6 +800,7 @@ 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[] = {
> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>   };
>   
>   static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
> -	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
> +	{ "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },

I think this is a peculiarity of the particular board that than a 
property of the PMIC. Please describe this in the board or SoC DTS if 
the value can not be read using the software .

>   	{}
>   };
>   
> @@ -1394,6 +1395,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;

-- 
With best wishes
Dmitry

Re: [PATCH 3/6] regulator: qcom_smd: Add MP5496 regulators

[Devi Priya]

On 1/27/2023 9:33 PM, Konrad Dybcio wrote:
> 
> 
> On 27.01.2023 17:01, Devi Priya wrote:
>>
>>
>> On 1/13/2023 8:54 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 13.01.2023 16:03, devi priya wrote:
>>>> Adding support for PMIC MP5496 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>
>>>> ---
>>> Please simply extend the existing MP5496 support with this
>>> S1 regulator. If you don't explicitly define and set voltages
>>> for the other vregs, they will not be probed.
>>>
>>> Konrad
>> IPQ6018 and IPQ9574 platforms use the same PMIC MP5496 but they have a different power layout. IPQ9574 has S2 regulator which will be used for NSS scaling but S2 in IPQ6018 serves a different purpose. Hence it would not be possible to extend the existing MP5496 support for IPQ9574
> Does the s2 on IPQ9574 have a different voltage range than
> the one on IPQ6018? No? Then there's nothing blocking you
> from using the setup for both SoCs. As I've mentioned,
> regulators that you don't add to the device tree will
> not even be probed.
> 
Yeah, understood! will update this in V2
> Konrad
>>>>    drivers/regulator/qcom_smd-regulator.c | 16 ++++++++++++++++
>>>>    1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>>> index 9f2b58458841..1eb17d378897 100644
>>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>>> @@ -767,6 +767,15 @@ static const struct regulator_desc mp5496_ldoa2 = {
>>>>        .ops = &rpm_mp5496_ops,
>>>>    };
>>>>    +static const struct regulator_desc ipq9574_mp5496_smpa1 = {
>>>> +    .linear_ranges = (struct linear_range[]) {
>>>> +        REGULATOR_LINEAR_RANGE(600000, 0, 37, 12500),
>>>> +    },
>>>> +    .n_linear_ranges = 1,
>>>> +    .n_voltages = 38,
>>>> +    .ops = &rpm_mp5496_ops,
>>>> +};
>>>> +
>>>>    static const struct regulator_desc pm2250_lvftsmps = {
>>>>        .linear_ranges = (struct linear_range[]) {
>>>>            REGULATOR_LINEAR_RANGE(320000, 0, 269, 4000),
>>>> @@ -799,6 +808,11 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>>>        {}
>>>>    };
>>>>    +static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>>>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>>>> +    {}
>>>> +};
>>>> +
>>>>    static const struct rpm_regulator_data rpm_pm2250_regulators[] = {
>>>>        { "s1", QCOM_SMD_RPM_SMPA, 1, &pm2250_lvftsmps, "vdd_s1" },
>>>>        { "s2", QCOM_SMD_RPM_SMPA, 2, &pm2250_lvftsmps, "vdd_s2" },
>>>> @@ -1320,6 +1334,8 @@ static const struct rpm_regulator_data rpm_pms405_regulators[] = {
>>>>    };
>>>>      static const struct of_device_id rpm_of_match[] = {
>>>> +    { .compatible = "qcom,rpm-ipq9574-mp5496-regulators",
>>>> +        .data = &rpm_ipq9574_mp5496_regulators },
>>>>        { .compatible = "qcom,rpm-mp5496-regulators", .data = &rpm_mp5496_regulators },
>>>>        { .compatible = "qcom,rpm-pm2250-regulators", .data = &rpm_pm2250_regulators },
>>>>        { .compatible = "qcom,rpm-pm6125-regulators", .data = &rpm_pm6125_regulators },
>> Best Regards,
>> Devi Priya
Best Regards,
Devi Priya

Re: [PATCH 4/6] regulator: qcom_smd: Add PMIC compatible for IPQ9574

[Devi Priya]

On 1/28/2023 1:42 AM, Rob Herring wrote:
> On Fri, Jan 27, 2023 at 10:05 AM Devi Priya <quic_devipriy@quicinc.com> wrote:
>>
>>
>>
>> On 1/18/2023 12:08 AM, Rob Herring wrote:
>>> On Fri, Jan 13, 2023 at 08:33:08PM +0530, devi priya wrote:
>>>> Add mp5496 PMIC compatible string for 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>
>>>> ---
>>>>    .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml  | 3 ++-
>>>>    1 file changed, 2 insertions(+), 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..7907d9385583 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
>>>> @@ -84,6 +84,7 @@ properties:
>>>>      compatible:
>>>>        enum:
>>>>          - qcom,rpm-mp5496-regulators
>>>> +      - qcom,rpm-ipq9574-mp5496-regulators
>>>
>>> Is this a different part than just mp5496? Or used in a different,
>>> incompatible way?
>> IPQ6018 and IPQ9574 platforms use the same PMIC MP5496 but they have a
>> different power layout.So, we plan to update the compatible:
>> qcom,rpm-mp5496-regulators to
>> qcom,rpm-ipq6018-mp5496-regulators(target-specific) in the next patchset
>> as the regulators serve different purposes
> 
> You can't just change compatibles. It is an ABI.
> 
> This still doesn't make sense to me. The PMIC hasn't changed, so the
> binding shouldn't. It should be flexible enough to be hooked up to
> different platforms. That's why we have all the per regulator
> configuration. What are you missing?
> 
> Rob
Sure got it!
It should be fine to use the existing mp5496 compatible for IPQ9574 too.
Will update this in V2

Best Regards,
Devi Priya

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

[Konrad Dybcio]

On 31.01.2023 10:28, Devi Priya wrote:
> 
> 
> On 1/27/2023 9:40 PM, Konrad Dybcio wrote:
>>
>>
>> On 27.01.2023 17:07, Devi Priya wrote:
>>>
>>>
>>> On 1/13/2023 9:07 PM, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 13.01.2023 16:03, 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>
>>>>> ---
>>>> Or maybe hook it up to the spmi_regulator_common_get_voltage()
>>>> from the SPMI regulator driver and read the real voltage instead
>>>> of relying on hardcoded values thay may differ between boards?
>>>>
>>>> Konrad
>>> In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM.
>>> In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values
>> Unless something changed, RPM regulator framework is simply a
>> fancy front-end for communicating with the PMIC over SPMI, AFAIK..
>>
>> Konrad
> Currently in our driver, the voltage write request will be sent to RPM via GLINK which then writes it to the PMIC over I2C using the below APIs
> qcom_rpm_smd_write -> rpmsg_send
> In IPQ9574, we do not have SPMI support or the support to readback voltage.
Okay, I didn't quite catch that there's *only* an i2c PMIC on this
platform.. Looking at the MP5496 datasheet though, reading back
the voltage should be possible via simply reading the fields that
are used to set it.

Konrad
> 
>>>
>>>>>    drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>>>> index 1eb17d378897..49a36b07397c 100644
>>>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>>>> @@ -800,6 +800,7 @@ 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[] = {
>>>>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>>>>    };
>>>>>      static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>>>>> -    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>>>>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
>>>>>        {}
>>>>>    };
>>>>>    @@ -1394,6 +1395,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;
>>> Best Regards,
>>> Devi Priya
> Best Regards,
> Devi Priya

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

[Devi Priya]

Thanks konrad for taking time to review the patch!

On 1/13/2023 9:02 PM, Konrad Dybcio wrote:
> 
> 
> On 13.01.2023 16:03, devi priya wrote:
>> Add CPU Freq and RPM related nodes in the device tree
> These two are wildly different things, barely related to one
> another and can very well be introduced in separate patches.
> Please do so.
> 
>>
>> 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>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi | 80 +++++++++++++++++++++++++++
>>   1 file changed, 80 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> index 5a2244b437ed..79fa5d91882c 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -9,6 +9,7 @@
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>   #include <dt-bindings/clock/qcom,gcc-ipq9574.h>
>>   #include <dt-bindings/reset/qcom,gcc-ipq9574.h>
>> +#include <dt-bindings/clock/qcom,apss-ipq.h>
> Please sort the includes alphabetically.
Sure, okay
> 
>>   
>>   / {
>>   	interrupt-parent = <&intc>;
>> @@ -75,6 +76,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>;
>> +			cpu0-supply = <&ipq9574_s1>;
> Why is this cpu0-supply and the rest are cpu-supply? Neither of them
> seem particularly documented, by the way..
Sure, will check and update this in V2
> 
> 
>>   		};
>>   
>>   		CPU1: cpu@1 {
>> @@ -83,6 +88,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 {
>> @@ -91,6 +100,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 {
>> @@ -99,6 +112,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 {
>> @@ -107,6 +124,42 @@
>>   		};
>>   	};
>>   
>> +	cpu_opp_table: opp-table-cpu {
> Alphabetically this goes after memory
Okay
> 
>> +		compatible = "operating-points-v2";
>> +		opp-shared;
>> +
>> +		opp-936000000 {
>> +			opp-hz = /bits/ 64 <936000000>;
>> +			opp-microvolt = <725000>;
>> +			clock-latency-ns = <200000>;
>> +		};
> Please add a newline between each subnode.
Okay
> 
>> +		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>;
>> +		};
>> +	};
>> +
>>   	memory@40000000 {
>>   		device_type = "memory";
>>   		/* We expect the bootloader to fill in the size */
>> @@ -128,6 +181,11 @@
>>   		#size-cells = <2>;
>>   		ranges;
>>   
>> +		rpm_msg_ram: memory@60000 {
>> +			reg = <0x0 0x00060000 0x0 0x6000>;
>> +			no-map;
>> +		};
>> +
>>   		tz_region: memory@4a600000 {
>>   			reg = <0x0 0x4a600000 0x0 0x400000>;
>>   			no-map;
>> @@ -324,6 +382,28 @@
>>   		};
>>   	};
>>   
>> +	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";
>> +
>> +			regulators {
>> +				compatible = "qcom,rpm-ipq9574-mp5496-regulators";
> The regulators are board-specific and should not be included in the
> SoC DTSI. If this is a very common configuration, you may split that
> into ipq9574-mp5496.dtsi, for example. Or ipq9574-pmics.dtsi if it's
> coupled with more PMICs.
Got it. Will move this to the board DT
> 
>> +
>> +				ipq9574_s1: s1 {
>> +					regulator-min-microvolt = <587500>;
>> +					regulator-max-microvolt = <1075000>;
>> +					regulator-always-on;
> Won't this break CPU retention?
> 
> You're holding a vote on it from the CPU devices, so it should be
> always enabled when the CPUs are oneline (as far as Linux is
> concerned).
> 
> 
> Or maybe Linux will think it's enabled and RPM will quietly park
> it when it decides it's good to do so.. but will it with an active
> request.. not sure, really.. just something to consider..
> 
Sure, will check this and update accordingly in V2
> Konrad
>> +				};
>> +			};
>> +		};
>> +	};
>> +
>>   	timer {
>>   		compatible = "arm,armv8-timer";
>>   		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
Best Regards,
Devi Priya

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

[Devi Priya]

On 1/31/2023 6:14 PM, Konrad Dybcio wrote:
> 
> 
> On 31.01.2023 10:28, Devi Priya wrote:
>>
>>
>> On 1/27/2023 9:40 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 27.01.2023 17:07, Devi Priya wrote:
>>>>
>>>>
>>>> On 1/13/2023 9:07 PM, Konrad Dybcio wrote:
>>>>>
>>>>>
>>>>> On 13.01.2023 16:03, 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>
>>>>>> ---
>>>>> Or maybe hook it up to the spmi_regulator_common_get_voltage()
>>>>> from the SPMI regulator driver and read the real voltage instead
>>>>> of relying on hardcoded values thay may differ between boards?
>>>>>
>>>>> Konrad
>>>> In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM.
>>>> In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values
>>> Unless something changed, RPM regulator framework is simply a
>>> fancy front-end for communicating with the PMIC over SPMI, AFAIK..
>>>
>>> Konrad
>> Currently in our driver, the voltage write request will be sent to RPM via GLINK which then writes it to the PMIC over I2C using the below APIs
>> qcom_rpm_smd_write -> rpmsg_send
>> In IPQ9574, we do not have SPMI support or the support to readback voltage.
> Okay, I didn't quite catch that there's *only* an i2c PMIC on this
> platform.. Looking at the MP5496 datasheet though, reading back
> the voltage should be possible via simply reading the fields that
> are used to set it.
> 
> Konrad
The CPR regulator operates in closed loop mode and the RPM can 
independently update the PMIC voltage.
So, Performing an i2c read to the PMIC would introduce conflicts when 
RPM uses the i2c for any of the voltage write or read operations.
>>
>>>>
>>>>>>     drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>>>>>>     1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>>>>> index 1eb17d378897..49a36b07397c 100644
>>>>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>>>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>>>>> @@ -800,6 +800,7 @@ 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[] = {
>>>>>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>>>>>     };
>>>>>>       static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>>>>>> -    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>>>>>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
>>>>>>         {}
>>>>>>     };
>>>>>>     @@ -1394,6 +1395,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;
>>>> Best Regards,
>>>> Devi Priya
>> Best Regards,
>> Devi Priya

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

[Devi Priya]

On 1/31/2023 3:07 PM, Dmitry Baryshkov wrote:
> On 13/01/2023 17:03, 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>
>> ---
>>   drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/regulator/qcom_smd-regulator.c 
>> b/drivers/regulator/qcom_smd-regulator.c
>> index 1eb17d378897..49a36b07397c 100644
>> --- a/drivers/regulator/qcom_smd-regulator.c
>> +++ b/drivers/regulator/qcom_smd-regulator.c
>> @@ -800,6 +800,7 @@ 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[] = {
>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data 
>> rpm_mp5496_regulators[] = {
>>   };
>>   static const struct rpm_regulator_data 
>> rpm_ipq9574_mp5496_regulators[] = {
>> -    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
> 
> I think this is a peculiarity of the particular board that than a 
> property of the PMIC. Please describe this in the board or SoC DTS if 
> the value can not be read using the software .
> 
The bootup voltage is actually blown into the OTP register of the PMIC 
and so, it remains the same across boards for IPQ9574 SoC
>>       {}
>>   };
>> @@ -1394,6 +1395,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;
> 
Best Regards,
Devi Priya

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

[Konrad Dybcio]

On 2.02.2023 12:09, Devi Priya wrote:
> 
> 
> On 1/31/2023 6:14 PM, Konrad Dybcio wrote:
>>
>>
>> On 31.01.2023 10:28, Devi Priya wrote:
>>>
>>>
>>> On 1/27/2023 9:40 PM, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 27.01.2023 17:07, Devi Priya wrote:
>>>>>
>>>>>
>>>>> On 1/13/2023 9:07 PM, Konrad Dybcio wrote:
>>>>>>
>>>>>>
>>>>>> On 13.01.2023 16:03, 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>
>>>>>>> ---
>>>>>> Or maybe hook it up to the spmi_regulator_common_get_voltage()
>>>>>> from the SPMI regulator driver and read the real voltage instead
>>>>>> of relying on hardcoded values thay may differ between boards?
>>>>>>
>>>>>> Konrad
>>>>> In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM.
>>>>> In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values
>>>> Unless something changed, RPM regulator framework is simply a
>>>> fancy front-end for communicating with the PMIC over SPMI, AFAIK..
>>>>
>>>> Konrad
>>> Currently in our driver, the voltage write request will be sent to RPM via GLINK which then writes it to the PMIC over I2C using the below APIs
>>> qcom_rpm_smd_write -> rpmsg_send
>>> In IPQ9574, we do not have SPMI support or the support to readback voltage.
>> Okay, I didn't quite catch that there's *only* an i2c PMIC on this
>> platform.. Looking at the MP5496 datasheet though, reading back
>> the voltage should be possible via simply reading the fields that
>> are used to set it.
>>
>> Konrad
> The CPR regulator operates in closed loop mode and the RPM can independently update the PMIC voltage.
> So, Performing an i2c read to the PMIC would introduce conflicts when RPM uses the i2c for any of the voltage write or read operations.
So.. are we even going to set voltage from Linux at all, for example
for DCVS? If not, maybe we can simply not register the regulator and
let the non-APSS parts handle it themselves?

Konrad
>>>
>>>>>
>>>>>>>     drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>>>>>>>     1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>>>>>> index 1eb17d378897..49a36b07397c 100644
>>>>>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>>>>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>>>>>> @@ -800,6 +800,7 @@ 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[] = {
>>>>>>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>>>>>>     };
>>>>>>>       static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>>>>>>> -    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>>>>>>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
>>>>>>>         {}
>>>>>>>     };
>>>>>>>     @@ -1394,6 +1395,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;
>>>>> Best Regards,
>>>>> Devi Priya
>>> Best Regards,
>>> Devi Priya

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

[Devi Priya]

On 2/2/2023 5:13 PM, Konrad Dybcio wrote:
> 
> 
> On 2.02.2023 12:09, Devi Priya wrote:
>>
>>
>> On 1/31/2023 6:14 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 31.01.2023 10:28, Devi Priya wrote:
>>>>
>>>>
>>>> On 1/27/2023 9:40 PM, Konrad Dybcio wrote:
>>>>>
>>>>>
>>>>> On 27.01.2023 17:07, Devi Priya wrote:
>>>>>>
>>>>>>
>>>>>> On 1/13/2023 9:07 PM, Konrad Dybcio wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 13.01.2023 16:03, 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>
>>>>>>>> ---
>>>>>>> Or maybe hook it up to the spmi_regulator_common_get_voltage()
>>>>>>> from the SPMI regulator driver and read the real voltage instead
>>>>>>> of relying on hardcoded values thay may differ between boards?
>>>>>>>
>>>>>>> Konrad
>>>>>> In IPQ9574, SPMI regulator is not used. We are using RPM-Glink communication and the regulators are controlled by RPM.
>>>>>> In this case, we don't have an option to readback the bootup voltage and so, we have hardcoded the values
>>>>> Unless something changed, RPM regulator framework is simply a
>>>>> fancy front-end for communicating with the PMIC over SPMI, AFAIK..
>>>>>
>>>>> Konrad
>>>> Currently in our driver, the voltage write request will be sent to RPM via GLINK which then writes it to the PMIC over I2C using the below APIs
>>>> qcom_rpm_smd_write -> rpmsg_send
>>>> In IPQ9574, we do not have SPMI support or the support to readback voltage.
>>> Okay, I didn't quite catch that there's *only* an i2c PMIC on this
>>> platform.. Looking at the MP5496 datasheet though, reading back
>>> the voltage should be possible via simply reading the fields that
>>> are used to set it.
>>>
>>> Konrad
>> The CPR regulator operates in closed loop mode and the RPM can independently update the PMIC voltage.
>> So, Performing an i2c read to the PMIC would introduce conflicts when RPM uses the i2c for any of the voltage write or read operations.
> So.. are we even going to set voltage from Linux at all, for example
> for DCVS? If not, maybe we can simply not register the regulator and
> let the non-APSS parts handle it themselves?
> 
In IPQ9574, PMIC basically controls three rails. In that, RPM has 
control over two rails (MX and CX) & APSS has control over the APC rail. 
RPM controls the MX and CX rails independently. For APC rail, APSS sends 
the voltage request to RPM via GLINK and RPM takes care of accessing the 
PMIC via I2C for APSS voltage requests & its own requests. This approach 
helps us to avoid arbitration. In this case, if we directly use the I2C 
to read the PMIC we would end up having issues, if RPM is accessing the 
PMIC.
> Konrad
>>>>
>>>>>>
>>>>>>>>      drivers/regulator/qcom_smd-regulator.c | 6 +++++-
>>>>>>>>      1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>>>>>>> index 1eb17d378897..49a36b07397c 100644
>>>>>>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>>>>>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>>>>>>> @@ -800,6 +800,7 @@ 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[] = {
>>>>>>>> @@ -809,7 +810,7 @@ static const struct rpm_regulator_data rpm_mp5496_regulators[] = {
>>>>>>>>      };
>>>>>>>>        static const struct rpm_regulator_data rpm_ipq9574_mp5496_regulators[] = {
>>>>>>>> -    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1" },
>>>>>>>> +    { "s1", QCOM_SMD_RPM_SMPA, 1, &ipq9574_mp5496_smpa1, "s1", 875000 },
>>>>>>>>          {}
>>>>>>>>      };
>>>>>>>>      @@ -1394,6 +1395,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;
>>>>>> Best Regards,
>>>>>> Devi Priya
>>>> Best Regards,
>>>> Devi Priya

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

[Bjorn Andersson]

On Fri, 13 Jan 2023 20:33:04 +0530, devi priya wrote:
> IPQ9574 SoC uses the PMIC MP5496 and SMPA1 regulator is used for
> APSS voltage scaling.
> This patch series adds the support for the same.
> Also enables the RPM communication over the RPMSG framework
> 
> This series depends on the below patchset
> https://lore.kernel.org/linux-arm-msm/20230113143647.14961-1-quic_devipriy@quicinc.com/
> 
> [...]

Applied, thanks!

[1/6] soc: qcom: smd-rpm: Add IPQ9574 compatible
      commit: 64dc69f3f36a71a95bfed8054d49600a7872415e

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

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

[Devi Priya]

On 2/7/2023 4:00 AM, Bjorn Andersson wrote:
> On Fri, 13 Jan 2023 20:33:04 +0530, devi priya wrote:
>> IPQ9574 SoC uses the PMIC MP5496 and SMPA1 regulator is used for
>> APSS voltage scaling.
>> This patch series adds the support for the same.
>> Also enables the RPM communication over the RPMSG framework
>>
>> This series depends on the below patchset
>> https://lore.kernel.org/linux-arm-msm/20230113143647.14961-1-quic_devipriy@quicinc.com/
>>
>> [...]
> 
> Applied, thanks!
> 
Thanks Bjorn!

> [1/6] soc: qcom: smd-rpm: Add IPQ9574 compatible
>        commit: 64dc69f3f36a71a95bfed8054d49600a7872415e
> 
> Best regards,
Regards,
Devi Priya