Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 6495D6034E
Subject: Re: [PATCH] regulator: qcom-smd: Batch up requests for disabled
 regulators
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org
References: <20190122190147.26735-1-bjorn.andersson@linaro.org>
 <fee88300-dbda-344a-81e3-4d81b6ba2234@codeaurora.org>
 <20190122201148.GD31919@minitux>
From:   Jeffrey Hugo <jhugo@codeaurora.org>
Message-ID: <68f0a261-ba4b-8411-82f1-37eed4eccf24@codeaurora.org>
Date:   Tue, 22 Jan 2019 13:32:30 -0700
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <20190122201148.GD31919@minitux>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.