[PATCH] regulator: qcom-smd: Batch up requests for disabled regulators

[PATCH] regulator: qcom-smd: Batch up requests for disabled regulators

[Bjorn Andersson]

In some scenarios the early stages of the boot chain has configured
regulators to be in a required state, but the later stages has skipped
to inform the RPM about it's requirements.

But as the SMD RPM regulators are being initialized voltage change
requests will be issued to align the voltage with the valid ranges. The
RPM aggregates all parameters for the specific regulator, the voltage
will be adjusted and the "enabled" state will be "off" - and the
regulator is turned off.

This patch addresses this problem by caching the requested enable state,
voltage and load and send the parameters in a batch, depending on the
enable state - effectively delaying the voltage request for disabled
regulators.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/regulator/qcom_smd-regulator.c | 104 ++++++++++++++++---------
 1 file changed, 69 insertions(+), 35 deletions(-)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index f5bca77d67c1..dfdc67da5cec 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -31,6 +31,11 @@ struct qcom_rpm_reg {
 
 	int is_enabled;
 	int uV;
+	u32 load;
+
+	unsigned int enabled_updated:1;
+	unsigned int uv_updated:1;
+	unsigned int load_updated:1;
 };
 
 struct rpm_regulator_req {
@@ -43,30 +48,59 @@ struct rpm_regulator_req {
 #define RPM_KEY_UV	0x00007675 /* "uv" */
 #define RPM_KEY_MA	0x0000616d /* "ma" */
 
-static int rpm_reg_write_active(struct qcom_rpm_reg *vreg,
-				struct rpm_regulator_req *req,
-				size_t size)
+static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
 {
-	return qcom_rpm_smd_write(vreg->rpm,
-				  QCOM_SMD_RPM_ACTIVE_STATE,
-				  vreg->type,
-				  vreg->id,
-				  req, size);
+	struct rpm_regulator_req req[3];
+	int reqlen = 0;
+	int ret;
+
+	if (vreg->enabled_updated) {
+		req[reqlen].key = cpu_to_le32(RPM_KEY_SWEN);
+		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
+		req[reqlen].value = cpu_to_le32(vreg->is_enabled);
+		reqlen++;
+	}
+
+	if (vreg->uv_updated && vreg->is_enabled) {
+		req[reqlen].key = cpu_to_le32(RPM_KEY_UV);
+		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
+		req[reqlen].value = cpu_to_le32(vreg->uV);
+		reqlen++;
+	}
+
+	if (vreg->load_updated && vreg->is_enabled) {
+		req[reqlen].key = cpu_to_le32(RPM_KEY_MA);
+		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
+		req[reqlen].value = cpu_to_le32(vreg->load / 1000);
+		reqlen++;
+	}
+
+	if (!reqlen)
+		return 0;
+
+	ret = qcom_rpm_smd_write(vreg->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
+				 vreg->type, vreg->id,
+				 req, sizeof(req[0]) * reqlen);
+	if (!ret) {
+		vreg->enabled_updated = 0;
+		vreg->uv_updated = 0;
+		vreg->load_updated = 0;
+	}
+
+	return ret;
 }
 
 static int rpm_reg_enable(struct regulator_dev *rdev)
 {
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
-	struct rpm_regulator_req req;
 	int ret;
 
-	req.key = cpu_to_le32(RPM_KEY_SWEN);
-	req.nbytes = cpu_to_le32(sizeof(u32));
-	req.value = cpu_to_le32(1);
+	vreg->is_enabled = 1;
+	vreg->enabled_updated = 1;
 
-	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
-	if (!ret)
-		vreg->is_enabled = 1;
+	ret = rpm_reg_write_active(vreg);
+	if (ret)
+		vreg->is_enabled = 0;
 
 	return ret;
 }
@@ -81,16 +115,14 @@ static int rpm_reg_is_enabled(struct regulator_dev *rdev)
 static int rpm_reg_disable(struct regulator_dev *rdev)
 {
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
-	struct rpm_regulator_req req;
 	int ret;
 
-	req.key = cpu_to_le32(RPM_KEY_SWEN);
-	req.nbytes = cpu_to_le32(sizeof(u32));
-	req.value = 0;
+	vreg->is_enabled = 0;
+	vreg->enabled_updated = 1;
 
-	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
-	if (!ret)
-		vreg->is_enabled = 0;
+	ret = rpm_reg_write_active(vreg);
+	if (ret)
+		vreg->is_enabled = 1;
 
 	return ret;
 }
@@ -108,16 +140,15 @@ static int rpm_reg_set_voltage(struct regulator_dev *rdev,
 			       unsigned *selector)
 {
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
-	struct rpm_regulator_req req;
-	int ret = 0;
+	int ret;
+	int old_uV = vreg->uV;
 
-	req.key = cpu_to_le32(RPM_KEY_UV);
-	req.nbytes = cpu_to_le32(sizeof(u32));
-	req.value = cpu_to_le32(min_uV);
+	vreg->uV = min_uV;
+	vreg->uv_updated = 1;
 
-	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
-	if (!ret)
-		vreg->uV = min_uV;
+	ret = rpm_reg_write_active(vreg);
+	if (ret)
+		vreg->uV = old_uV;
 
 	return ret;
 }
@@ -125,13 +156,16 @@ static int rpm_reg_set_voltage(struct regulator_dev *rdev,
 static int rpm_reg_set_load(struct regulator_dev *rdev, int load_uA)
 {
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
-	struct rpm_regulator_req req;
+	u32 old_load = vreg->load;
+	int ret;
 
-	req.key = cpu_to_le32(RPM_KEY_MA);
-	req.nbytes = cpu_to_le32(sizeof(u32));
-	req.value = cpu_to_le32(load_uA / 1000);
+	vreg->load = load_uA;
+	vreg->load_updated = 1;
+	ret = rpm_reg_write_active(vreg);
+	if (ret)
+		vreg->load = old_load;
 
-	return rpm_reg_write_active(vreg, &req, sizeof(req));
+	return ret;
 }
 
 static const struct regulator_ops rpm_smps_ldo_ops = {
-- 
2.18.0

Re: [PATCH] regulator: qcom-smd: Batch up requests for disabled regulators

[Jeffrey Hugo]

Hi Bjorn,

This seems intresting, but I'm not sure I fully understand it yet.

On 1/22/2019 12:01 PM, Bjorn Andersson wrote:
> In some scenarios the early stages of the boot chain has configured
> regulators to be in a required state, but the later stages has skipped
> to inform the RPM about it's requirements.
So if I understand this correctly, the boot chain turned on and 
configured the regulator, but didn't give the RPM its vote, so the RPM 
doesn't know anything and just assumes the default state - 
off/unconfigured.  Right?

If we are in Linux, is that boot chain "vote" valid anymore?
> 
> But as the SMD RPM regulators are being initialized voltage change
> requests will be issued to align the voltage with the valid ranges. The
> RPM aggregates all parameters for the specific regulator, the voltage
> will be adjusted and the "enabled" state will be "off" - and the
> regulator is turned off.

So, this would happen when Linux is processing the constraints from DT 
for example, but is still initing the devices, so there are no consumers 
and thus the device is not enabled, but the voltage configuration is 
sent to the RPM to ensure the constraints are met?

> 
> This patch addresses this problem by caching the requested enable state,
> voltage and load and send the parameters in a batch, depending on the
> enable state - effectively delaying the voltage request for disabled
> regulators.

I'm curious, would any sort of additional latency be expected because 
the RPM has delayed work to the enable "stage"?

While I suspect there are benefits to this change regardless, it seems 
like what you are describing above should be addressed by 
"regulator-boot-on" in the DT.  Why is that not a valid solution for 
this scenario?

> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/regulator/qcom_smd-regulator.c | 104 ++++++++++++++++---------
>   1 file changed, 69 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> index f5bca77d67c1..dfdc67da5cec 100644
> --- a/drivers/regulator/qcom_smd-regulator.c
> +++ b/drivers/regulator/qcom_smd-regulator.c
> @@ -31,6 +31,11 @@ struct qcom_rpm_reg {
>   
>   	int is_enabled;
>   	int uV;
> +	u32 load;
> +
> +	unsigned int enabled_updated:1;
> +	unsigned int uv_updated:1;
> +	unsigned int load_updated:1;
>   };
>   
>   struct rpm_regulator_req {
> @@ -43,30 +48,59 @@ struct rpm_regulator_req {
>   #define RPM_KEY_UV	0x00007675 /* "uv" */
>   #define RPM_KEY_MA	0x0000616d /* "ma" */
>   
> -static int rpm_reg_write_active(struct qcom_rpm_reg *vreg,
> -				struct rpm_regulator_req *req,
> -				size_t size)
> +static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
>   {
> -	return qcom_rpm_smd_write(vreg->rpm,
> -				  QCOM_SMD_RPM_ACTIVE_STATE,
> -				  vreg->type,
> -				  vreg->id,
> -				  req, size);
> +	struct rpm_regulator_req req[3];
> +	int reqlen = 0;
> +	int ret;
> +
> +	if (vreg->enabled_updated) {
> +		req[reqlen].key = cpu_to_le32(RPM_KEY_SWEN);
> +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
> +		req[reqlen].value = cpu_to_le32(vreg->is_enabled);
> +		reqlen++;
> +	}
> +
> +	if (vreg->uv_updated && vreg->is_enabled) {
> +		req[reqlen].key = cpu_to_le32(RPM_KEY_UV);
> +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
> +		req[reqlen].value = cpu_to_le32(vreg->uV);
> +		reqlen++;
> +	}
> +
> +	if (vreg->load_updated && vreg->is_enabled) {
> +		req[reqlen].key = cpu_to_le32(RPM_KEY_MA);
> +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
> +		req[reqlen].value = cpu_to_le32(vreg->load / 1000);
> +		reqlen++;
> +	}
> +
> +	if (!reqlen)
> +		return 0;
> +
> +	ret = qcom_rpm_smd_write(vreg->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
> +				 vreg->type, vreg->id,
> +				 req, sizeof(req[0]) * reqlen);
> +	if (!ret) {
> +		vreg->enabled_updated = 0;
> +		vreg->uv_updated = 0;
> +		vreg->load_updated = 0;
> +	}
> +
> +	return ret;
>   }
>   
>   static int rpm_reg_enable(struct regulator_dev *rdev)
>   {
>   	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> -	struct rpm_regulator_req req;
>   	int ret;
>   
> -	req.key = cpu_to_le32(RPM_KEY_SWEN);
> -	req.nbytes = cpu_to_le32(sizeof(u32));
> -	req.value = cpu_to_le32(1);
> +	vreg->is_enabled = 1;
> +	vreg->enabled_updated = 1;
>   
> -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
> -	if (!ret)
> -		vreg->is_enabled = 1;
> +	ret = rpm_reg_write_active(vreg);
> +	if (ret)
> +		vreg->is_enabled = 0;
>   
>   	return ret;
>   }
> @@ -81,16 +115,14 @@ static int rpm_reg_is_enabled(struct regulator_dev *rdev)
>   static int rpm_reg_disable(struct regulator_dev *rdev)
>   {
>   	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> -	struct rpm_regulator_req req;
>   	int ret;
>   
> -	req.key = cpu_to_le32(RPM_KEY_SWEN);
> -	req.nbytes = cpu_to_le32(sizeof(u32));
> -	req.value = 0;
> +	vreg->is_enabled = 0;
> +	vreg->enabled_updated = 1;
>   
> -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
> -	if (!ret)
> -		vreg->is_enabled = 0;
> +	ret = rpm_reg_write_active(vreg);
> +	if (ret)
> +		vreg->is_enabled = 1;
>   
>   	return ret;
>   }
> @@ -108,16 +140,15 @@ static int rpm_reg_set_voltage(struct regulator_dev *rdev,
>   			       unsigned *selector)
>   {
>   	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> -	struct rpm_regulator_req req;
> -	int ret = 0;
> +	int ret;
> +	int old_uV = vreg->uV;
>   
> -	req.key = cpu_to_le32(RPM_KEY_UV);
> -	req.nbytes = cpu_to_le32(sizeof(u32));
> -	req.value = cpu_to_le32(min_uV);
> +	vreg->uV = min_uV;
> +	vreg->uv_updated = 1;
>   
> -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
> -	if (!ret)
> -		vreg->uV = min_uV;
> +	ret = rpm_reg_write_active(vreg);
> +	if (ret)
> +		vreg->uV = old_uV;
>   
>   	return ret;
>   }
> @@ -125,13 +156,16 @@ static int rpm_reg_set_voltage(struct regulator_dev *rdev,
>   static int rpm_reg_set_load(struct regulator_dev *rdev, int load_uA)
>   {
>   	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> -	struct rpm_regulator_req req;
> +	u32 old_load = vreg->load;
> +	int ret;
>   
> -	req.key = cpu_to_le32(RPM_KEY_MA);
> -	req.nbytes = cpu_to_le32(sizeof(u32));
> -	req.value = cpu_to_le32(load_uA / 1000);
> +	vreg->load = load_uA;
> +	vreg->load_updated = 1;
> +	ret = rpm_reg_write_active(vreg);
> +	if (ret)
> +		vreg->load = old_load;
>   
> -	return rpm_reg_write_active(vreg, &req, sizeof(req));
> +	return ret;
>   }
>   
>   static const struct regulator_ops rpm_smps_ldo_ops = {
> 


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] regulator: qcom-smd: Batch up requests for disabled regulators

[Bjorn Andersson]

On Tue 22 Jan 11:25 PST 2019, Jeffrey Hugo wrote:

> Hi Bjorn,
> 
> This seems intresting, but I'm not sure I fully understand it yet.
> 
> On 1/22/2019 12:01 PM, Bjorn Andersson wrote:
> > In some scenarios the early stages of the boot chain has configured
> > regulators to be in a required state, but the later stages has skipped
> > to inform the RPM about it's requirements.
> So if I understand this correctly, the boot chain turned on and configured
> the regulator, but didn't give the RPM its vote, so the RPM doesn't know
> anything and just assumes the default state - off/unconfigured.  Right?
> 

In our particular case we see that if ABL decides not to enter fastboot
it will not request the RPM to power the USB block. But the particular
regulator is part of the power-on sequence of the PMIC, so it's on.

So there's no votes in the RPM to have this on, but it is on and
disabling it causes the system to reboot.

> If we are in Linux, is that boot chain "vote" valid anymore?

I've never seen this code, but I assume it treats votes from "unsecure
apps" the same. The problem here is that there's no votes, from anyone,
to keep this regulator enabled.

> > 
> > But as the SMD RPM regulators are being initialized voltage change
> > requests will be issued to align the voltage with the valid ranges. The
> > RPM aggregates all parameters for the specific regulator, the voltage
> > will be adjusted and the "enabled" state will be "off" - and the
> > regulator is turned off.
> 
> So, this would happen when Linux is processing the constraints from DT for
> example, but is still initing the devices, so there are no consumers and
> thus the device is not enabled, but the voltage configuration is sent to the
> RPM to ensure the constraints are met?
> 

Correct.

> > 
> > This patch addresses this problem by caching the requested enable state,
> > voltage and load and send the parameters in a batch, depending on the
> > enable state - effectively delaying the voltage request for disabled
> > regulators.
> 
> I'm curious, would any sort of additional latency be expected because the
> RPM has delayed work to the enable "stage"?
> 

Afaict the PMIC itself is enabled/disabled by setting the voltage, so
there should be no negative latency difference in postponing the voltage
change until enable time. There's quite likely a positive gain though,
by removing the extra round trip over SMD.

> While I suspect there are benefits to this change regardless, it seems like
> what you are describing above should be addressed by "regulator-boot-on" in
> the DT.  Why is that not a valid solution for this scenario?
> 

Because set_machine_constraints() first calls
machine_constraints_voltage() then _regulator_do_enable() (if boot_on is
set). So without this patch the RPM will disable the regulator in the
beginning of the function and not reach the end of it.

Regards,
Bjorn

> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >   drivers/regulator/qcom_smd-regulator.c | 104 ++++++++++++++++---------
> >   1 file changed, 69 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> > index f5bca77d67c1..dfdc67da5cec 100644
> > --- a/drivers/regulator/qcom_smd-regulator.c
> > +++ b/drivers/regulator/qcom_smd-regulator.c
> > @@ -31,6 +31,11 @@ struct qcom_rpm_reg {
> >   	int is_enabled;
> >   	int uV;
> > +	u32 load;
> > +
> > +	unsigned int enabled_updated:1;
> > +	unsigned int uv_updated:1;
> > +	unsigned int load_updated:1;
> >   };
> >   struct rpm_regulator_req {
> > @@ -43,30 +48,59 @@ struct rpm_regulator_req {
> >   #define RPM_KEY_UV	0x00007675 /* "uv" */
> >   #define RPM_KEY_MA	0x0000616d /* "ma" */
> > -static int rpm_reg_write_active(struct qcom_rpm_reg *vreg,
> > -				struct rpm_regulator_req *req,
> > -				size_t size)
> > +static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
> >   {
> > -	return qcom_rpm_smd_write(vreg->rpm,
> > -				  QCOM_SMD_RPM_ACTIVE_STATE,
> > -				  vreg->type,
> > -				  vreg->id,
> > -				  req, size);
> > +	struct rpm_regulator_req req[3];
> > +	int reqlen = 0;
> > +	int ret;
> > +
> > +	if (vreg->enabled_updated) {
> > +		req[reqlen].key = cpu_to_le32(RPM_KEY_SWEN);
> > +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
> > +		req[reqlen].value = cpu_to_le32(vreg->is_enabled);
> > +		reqlen++;
> > +	}
> > +
> > +	if (vreg->uv_updated && vreg->is_enabled) {
> > +		req[reqlen].key = cpu_to_le32(RPM_KEY_UV);
> > +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
> > +		req[reqlen].value = cpu_to_le32(vreg->uV);
> > +		reqlen++;
> > +	}
> > +
> > +	if (vreg->load_updated && vreg->is_enabled) {
> > +		req[reqlen].key = cpu_to_le32(RPM_KEY_MA);
> > +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
> > +		req[reqlen].value = cpu_to_le32(vreg->load / 1000);
> > +		reqlen++;
> > +	}
> > +
> > +	if (!reqlen)
> > +		return 0;
> > +
> > +	ret = qcom_rpm_smd_write(vreg->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
> > +				 vreg->type, vreg->id,
> > +				 req, sizeof(req[0]) * reqlen);
> > +	if (!ret) {
> > +		vreg->enabled_updated = 0;
> > +		vreg->uv_updated = 0;
> > +		vreg->load_updated = 0;
> > +	}
> > +
> > +	return ret;
> >   }
> >   static int rpm_reg_enable(struct regulator_dev *rdev)
> >   {
> >   	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> > -	struct rpm_regulator_req req;
> >   	int ret;
> > -	req.key = cpu_to_le32(RPM_KEY_SWEN);
> > -	req.nbytes = cpu_to_le32(sizeof(u32));
> > -	req.value = cpu_to_le32(1);
> > +	vreg->is_enabled = 1;
> > +	vreg->enabled_updated = 1;
> > -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
> > -	if (!ret)
> > -		vreg->is_enabled = 1;
> > +	ret = rpm_reg_write_active(vreg);
> > +	if (ret)
> > +		vreg->is_enabled = 0;
> >   	return ret;
> >   }
> > @@ -81,16 +115,14 @@ static int rpm_reg_is_enabled(struct regulator_dev *rdev)
> >   static int rpm_reg_disable(struct regulator_dev *rdev)
> >   {
> >   	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> > -	struct rpm_regulator_req req;
> >   	int ret;
> > -	req.key = cpu_to_le32(RPM_KEY_SWEN);
> > -	req.nbytes = cpu_to_le32(sizeof(u32));
> > -	req.value = 0;
> > +	vreg->is_enabled = 0;
> > +	vreg->enabled_updated = 1;
> > -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
> > -	if (!ret)
> > -		vreg->is_enabled = 0;
> > +	ret = rpm_reg_write_active(vreg);
> > +	if (ret)
> > +		vreg->is_enabled = 1;
> >   	return ret;
> >   }
> > @@ -108,16 +140,15 @@ static int rpm_reg_set_voltage(struct regulator_dev *rdev,
> >   			       unsigned *selector)
> >   {
> >   	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> > -	struct rpm_regulator_req req;
> > -	int ret = 0;
> > +	int ret;
> > +	int old_uV = vreg->uV;
> > -	req.key = cpu_to_le32(RPM_KEY_UV);
> > -	req.nbytes = cpu_to_le32(sizeof(u32));
> > -	req.value = cpu_to_le32(min_uV);
> > +	vreg->uV = min_uV;
> > +	vreg->uv_updated = 1;
> > -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
> > -	if (!ret)
> > -		vreg->uV = min_uV;
> > +	ret = rpm_reg_write_active(vreg);
> > +	if (ret)
> > +		vreg->uV = old_uV;
> >   	return ret;
> >   }
> > @@ -125,13 +156,16 @@ static int rpm_reg_set_voltage(struct regulator_dev *rdev,
> >   static int rpm_reg_set_load(struct regulator_dev *rdev, int load_uA)
> >   {
> >   	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> > -	struct rpm_regulator_req req;
> > +	u32 old_load = vreg->load;
> > +	int ret;
> > -	req.key = cpu_to_le32(RPM_KEY_MA);
> > -	req.nbytes = cpu_to_le32(sizeof(u32));
> > -	req.value = cpu_to_le32(load_uA / 1000);
> > +	vreg->load = load_uA;
> > +	vreg->load_updated = 1;
> > +	ret = rpm_reg_write_active(vreg);
> > +	if (ret)
> > +		vreg->load = old_load;
> > -	return rpm_reg_write_active(vreg, &req, sizeof(req));
> > +	return ret;
> >   }
> >   static const struct regulator_ops rpm_smps_ldo_ops = {
> > 
> 
> 
> -- 
> Jeffrey Hugo
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
> Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] regulator: qcom-smd: Batch up requests for disabled regulators

[Jeffrey Hugo]

On 1/22/2019 1:11 PM, Bjorn Andersson wrote:
> On Tue 22 Jan 11:25 PST 2019, Jeffrey Hugo wrote:
> 
>> Hi Bjorn,
>>
>> This seems intresting, but I'm not sure I fully understand it yet.
>>
>> On 1/22/2019 12:01 PM, Bjorn Andersson wrote:
>>> In some scenarios the early stages of the boot chain has configured
>>> regulators to be in a required state, but the later stages has skipped
>>> to inform the RPM about it's requirements.
>> So if I understand this correctly, the boot chain turned on and configured
>> the regulator, but didn't give the RPM its vote, so the RPM doesn't know
>> anything and just assumes the default state - off/unconfigured.  Right?
>>
> 
> In our particular case we see that if ABL decides not to enter fastboot
> it will not request the RPM to power the USB block. But the particular
> regulator is part of the power-on sequence of the PMIC, so it's on.
> 
> So there's no votes in the RPM to have this on, but it is on and
> disabling it causes the system to reboot.
> 
>> If we are in Linux, is that boot chain "vote" valid anymore?
> 
> I've never seen this code, but I assume it treats votes from "unsecure
> apps" the same. The problem here is that there's no votes, from anyone,
> to keep this regulator enabled.

As far as I am aware, yes "unsecured apps" has one vote, be it Linux or 
the boot chain.  I was thinking conceptually, the boot chain is done, 
why do we care about what hardware they were using, but you pointed out 
above that you get a board reset, so I guess we do.

> 
>>>
>>> But as the SMD RPM regulators are being initialized voltage change
>>> requests will be issued to align the voltage with the valid ranges. The
>>> RPM aggregates all parameters for the specific regulator, the voltage
>>> will be adjusted and the "enabled" state will be "off" - and the
>>> regulator is turned off.
>>
>> So, this would happen when Linux is processing the constraints from DT for
>> example, but is still initing the devices, so there are no consumers and
>> thus the device is not enabled, but the voltage configuration is sent to the
>> RPM to ensure the constraints are met?
>>
> 
> Correct.
> 
>>>
>>> This patch addresses this problem by caching the requested enable state,
>>> voltage and load and send the parameters in a batch, depending on the
>>> enable state - effectively delaying the voltage request for disabled
>>> regulators.
>>
>> I'm curious, would any sort of additional latency be expected because the
>> RPM has delayed work to the enable "stage"?
>>
> 
> Afaict the PMIC itself is enabled/disabled by setting the voltage, so
> there should be no negative latency difference in postponing the voltage
> change until enable time. There's quite likely a positive gain though,
> by removing the extra round trip over SMD.

Sounds good.  That's what I thought, but I was wondering if I was wrong.

> 
>> While I suspect there are benefits to this change regardless, it seems like
>> what you are describing above should be addressed by "regulator-boot-on" in
>> the DT.  Why is that not a valid solution for this scenario?
>>
> 
> Because set_machine_constraints() first calls
> machine_constraints_voltage() then _regulator_do_enable() (if boot_on is
> set). So without this patch the RPM will disable the regulator in the
> beginning of the function and not reach the end of it.

Makes sense.

> 
> Regards,
> Bjorn
> 
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> ---
>>>    drivers/regulator/qcom_smd-regulator.c | 104 ++++++++++++++++---------
>>>    1 file changed, 69 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>> index f5bca77d67c1..dfdc67da5cec 100644
>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>> @@ -31,6 +31,11 @@ struct qcom_rpm_reg {
>>>    	int is_enabled;
>>>    	int uV;
>>> +	u32 load;
>>> +
>>> +	unsigned int enabled_updated:1;
>>> +	unsigned int uv_updated:1;
>>> +	unsigned int load_updated:1;
>>>    };
>>>    struct rpm_regulator_req {
>>> @@ -43,30 +48,59 @@ struct rpm_regulator_req {
>>>    #define RPM_KEY_UV	0x00007675 /* "uv" */
>>>    #define RPM_KEY_MA	0x0000616d /* "ma" */
>>> -static int rpm_reg_write_active(struct qcom_rpm_reg *vreg,
>>> -				struct rpm_regulator_req *req,
>>> -				size_t size)
>>> +static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
>>>    {
>>> -	return qcom_rpm_smd_write(vreg->rpm,
>>> -				  QCOM_SMD_RPM_ACTIVE_STATE,
>>> -				  vreg->type,
>>> -				  vreg->id,
>>> -				  req, size);
>>> +	struct rpm_regulator_req req[3];
>>> +	int reqlen = 0;
>>> +	int ret;
>>> +
>>> +	if (vreg->enabled_updated) {
>>> +		req[reqlen].key = cpu_to_le32(RPM_KEY_SWEN);
>>> +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
>>> +		req[reqlen].value = cpu_to_le32(vreg->is_enabled);
>>> +		reqlen++;
>>> +	}
>>> +
>>> +	if (vreg->uv_updated && vreg->is_enabled) {
>>> +		req[reqlen].key = cpu_to_le32(RPM_KEY_UV);
>>> +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
>>> +		req[reqlen].value = cpu_to_le32(vreg->uV);
>>> +		reqlen++;
>>> +	}
>>> +
>>> +	if (vreg->load_updated && vreg->is_enabled) {
>>> +		req[reqlen].key = cpu_to_le32(RPM_KEY_MA);
>>> +		req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
>>> +		req[reqlen].value = cpu_to_le32(vreg->load / 1000);
>>> +		reqlen++;
>>> +	}
>>> +
>>> +	if (!reqlen)
>>> +		return 0;
>>> +
>>> +	ret = qcom_rpm_smd_write(vreg->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
>>> +				 vreg->type, vreg->id,
>>> +				 req, sizeof(req[0]) * reqlen);
>>> +	if (!ret) {
>>> +		vreg->enabled_updated = 0;
>>> +		vreg->uv_updated = 0;
>>> +		vreg->load_updated = 0;
>>> +	}
>>> +
>>> +	return ret;
>>>    }
>>>    static int rpm_reg_enable(struct regulator_dev *rdev)
>>>    {
>>>    	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
>>> -	struct rpm_regulator_req req;
>>>    	int ret;
>>> -	req.key = cpu_to_le32(RPM_KEY_SWEN);
>>> -	req.nbytes = cpu_to_le32(sizeof(u32));
>>> -	req.value = cpu_to_le32(1);
>>> +	vreg->is_enabled = 1;
>>> +	vreg->enabled_updated = 1;
>>> -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
>>> -	if (!ret)
>>> -		vreg->is_enabled = 1;
>>> +	ret = rpm_reg_write_active(vreg);
>>> +	if (ret)
>>> +		vreg->is_enabled = 0;
>>>    	return ret;
>>>    }
>>> @@ -81,16 +115,14 @@ static int rpm_reg_is_enabled(struct regulator_dev *rdev)
>>>    static int rpm_reg_disable(struct regulator_dev *rdev)
>>>    {
>>>    	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
>>> -	struct rpm_regulator_req req;
>>>    	int ret;
>>> -	req.key = cpu_to_le32(RPM_KEY_SWEN);
>>> -	req.nbytes = cpu_to_le32(sizeof(u32));
>>> -	req.value = 0;
>>> +	vreg->is_enabled = 0;
>>> +	vreg->enabled_updated = 1;
>>> -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
>>> -	if (!ret)
>>> -		vreg->is_enabled = 0;
>>> +	ret = rpm_reg_write_active(vreg);
>>> +	if (ret)
>>> +		vreg->is_enabled = 1;
>>>    	return ret;
>>>    }
>>> @@ -108,16 +140,15 @@ static int rpm_reg_set_voltage(struct regulator_dev *rdev,
>>>    			       unsigned *selector)
>>>    {
>>>    	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
>>> -	struct rpm_regulator_req req;
>>> -	int ret = 0;
>>> +	int ret;
>>> +	int old_uV = vreg->uV;
>>> -	req.key = cpu_to_le32(RPM_KEY_UV);
>>> -	req.nbytes = cpu_to_le32(sizeof(u32));
>>> -	req.value = cpu_to_le32(min_uV);
>>> +	vreg->uV = min_uV;
>>> +	vreg->uv_updated = 1;
>>> -	ret = rpm_reg_write_active(vreg, &req, sizeof(req));
>>> -	if (!ret)
>>> -		vreg->uV = min_uV;
>>> +	ret = rpm_reg_write_active(vreg);
>>> +	if (ret)
>>> +		vreg->uV = old_uV;
>>>    	return ret;
>>>    }
>>> @@ -125,13 +156,16 @@ static int rpm_reg_set_voltage(struct regulator_dev *rdev,
>>>    static int rpm_reg_set_load(struct regulator_dev *rdev, int load_uA)
>>>    {
>>>    	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
>>> -	struct rpm_regulator_req req;
>>> +	u32 old_load = vreg->load;
>>> +	int ret;
>>> -	req.key = cpu_to_le32(RPM_KEY_MA);
>>> -	req.nbytes = cpu_to_le32(sizeof(u32));
>>> -	req.value = cpu_to_le32(load_uA / 1000);
>>> +	vreg->load = load_uA;
>>> +	vreg->load_updated = 1;
>>> +	ret = rpm_reg_write_active(vreg);
>>> +	if (ret)
>>> +		vreg->load = old_load;
>>> -	return rpm_reg_write_active(vreg, &req, sizeof(req));
>>> +	return ret;
>>>    }
>>>    static const struct regulator_ops rpm_smps_ldo_ops = {
>>>
>>
>>
>> -- 
>> Jeffrey Hugo
>> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
>> Inc.
>> Qualcomm Technologies, Inc. is a member of the
>> Code Aurora Forum, a Linux Foundation Collaborative Project.


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] regulator: qcom-smd: Batch up requests for disabled regulators

[Jeffrey Hugo]

On 1/22/2019 12:01 PM, Bjorn Andersson wrote:
> In some scenarios the early stages of the boot chain has configured
> regulators to be in a required state, but the later stages has skipped
> to inform the RPM about it's requirements.
> 
> But as the SMD RPM regulators are being initialized voltage change
> requests will be issued to align the voltage with the valid ranges. The
> RPM aggregates all parameters for the specific regulator, the voltage
> will be adjusted and the "enabled" state will be "off" - and the
> regulator is turned off.
> 
> This patch addresses this problem by caching the requested enable state,
> voltage and load and send the parameters in a batch, depending on the
> enable state - effectively delaying the voltage request for disabled
> regulators.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

This happens to fix an annoyance I've been avoiding on 8998.

Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.