| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 17 Jan 2021 19:13:19 +0100
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...ainline.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: linux-arm-msm@...r.kernel.org, agross@...nel.org,
lgirdwood@...il.com, broonie@...nel.org, robh+dt@...nel.org,
sumit.semwal@...aro.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, phone-devel@...r.kernel.org,
konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
martin.botka@...ainline.org
Subject: Re: [PATCH v2 2/7] regulator: qcom-labibb: Implement current limiting
Il 15/01/21 05:37, Bjorn Andersson ha scritto:
> On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
>
>> LAB and IBB regulators can be current-limited by setting the
>> appropriate registers, but this operation is granted only after
>> sending an unlock code for secure access.
>>
>> Besides the secure access, it would be possible to use the
>> regmap helper for get_current_limit, as there is no security
>> blocking reads, but I chose not to as to avoid having a very
>> big array containing current limits, especially for IBB.
>>
>> That said, these regulators support current limiting for:
>> - LAB (pos): 200-1600mA, with 200mA per step (8 steps),
>> - IBB (neg): 0-1550mA, with 50mA per step (32 steps).
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
>> ---
>> drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
>> 1 file changed, 92 insertions(+)
>>
>> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
>> index 9f51c96f16fb..d364f54ad294 100644
>> --- a/drivers/regulator/qcom-labibb-regulator.c
>> +++ b/drivers/regulator/qcom-labibb-regulator.c
>> @@ -29,6 +29,15 @@
>> #define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
>> #define LABIBB_CONTROL_ENABLE BIT(7)
>>
>> +#define REG_LABIBB_CURRENT_LIMIT 0x4b
>> + #define LAB_CURRENT_LIMIT_MASK GENMASK(2, 0)
>> + #define IBB_CURRENT_LIMIT_MASK GENMASK(4, 0)
>> + #define LAB_CURRENT_LIMIT_OVERRIDE_EN BIT(3)
>> + #define LABIBB_CURRENT_LIMIT_EN BIT(7)
>> +
>> +#define REG_LABIBB_SEC_ACCESS 0xd0
>> + #define LABIBB_SEC_UNLOCK_CODE 0xa5
>> +
>> #define LAB_ENABLE_CTL_MASK BIT(7)
>> #define IBB_ENABLE_CTL_MASK (BIT(7) | BIT(6))
>>
>> @@ -37,11 +46,18 @@
>> #define IBB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 10)
>> #define LABIBB_POLL_ENABLED_TIME 1000
>>
>> +struct labibb_current_limits {
>> + u32 uA_min;
>> + u32 uA_step;
>> + u8 ovr_val;
>> +};
>> +
>> struct labibb_regulator {
>> struct regulator_desc desc;
>> struct device *dev;
>> struct regmap *regmap;
>> struct regulator_dev *rdev;
>> + struct labibb_current_limits uA_limits;
>> u16 base;
>> u8 type;
>> };
>> @@ -53,6 +69,57 @@ struct labibb_regulator_data {
>> const struct regulator_desc *desc;
>> };
>>
>> +static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
>> + int min_uA, int max_uA)
>
> I was under the impression that a regulator driver should either
> implement set_voltage_* or set_current_limit, depending on which type of
> regulator it is - i.e. this API isn't supposed to be setting the current
> limit. Perhaps I'm wrong though?
>
As far as I've understood, these are two entirely different things - set
voltage and set current limits - that's how I'm using them, exactly, and
I'm really sure that I'm right on this.
Besides what I think, though, obviously Mark has the last word on this.
>> +{
>> + struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
>> + struct regulator_desc *desc = &vreg->desc;
>> + struct labibb_current_limits *lim = &vreg->uA_limits;
>> + u32 mask, val;
>> + int i, ret, sel = -1;
>> +
>> + if (min_uA < lim->uA_min || max_uA < lim->uA_min)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < desc->n_current_limits; i++) {
>> + int uA_limit = (lim->uA_step * i) + lim->uA_min;
>> +
>> + if (max_uA >= uA_limit && min_uA <= uA_limit)
>
> I presume here you rely on the client passing something like min_uA = 0
> and max_uA 500? Because if the client where to
> regulator_set_current_limit(475, 475) you will pass through this loop
> without finding a match, but 450 would probably be a really good
> pick...
This regulator does not support setting a minimum limit, but only a
ceiling limit, and that's used to raise an interrupt for over-current
protection.
As far as I've understood, the Portable Batch System does its job only
in the specific case of *short circuit* detection, but the OCP is
totally left to the driver.
The reason why I am restricting the match in a sort of paranoid way is
that this regulator can set the current limit in steps, but what I
wanted to avoid was a scenario like:
- My display hardware draws a maximum of 475mA
- I set the current limit on LAB/IBB to 475mA
- The driver picks 450mA because it likes that value more
- I get Over Current Protection interrupts and I think that my hardware
will get fried if I keep going on
- This points me to fix the display driver
- Wrong! It was the regulator driver doing something different from
what I asked it to.
So that's what I am avoiding here. I don't want developers to go crazy
over their eventually new driver design for their new HW for "no reason".
>
> But what does it even mean to pass min/max uA for a current limit?
>
As I explained above, in this specific case, it means setting a limit to
trigger the over current interrupt in order to protect the hardware that
is attached to this regulator.
In other cases, meanings may be *slightly* different (at least, from my
understanding of it).
> That said, I think this loop would be better expressed as a single
> subtract uA_min and then divide by uA_step.
>
>
Yes I can write it shorter... and even more... but I wanted to improve
human readability of this function (and this entire driver) because
regulators may be dangerous, if badly understood and/or badly set.
I just wanted two things:
1. Whoever reviewed my patches couldn't misunderstand what I wrote
as much as possible;
2. Any other developer reading this driver (which may not be really
familiar with this HW) gets the meaning of what I'm doing in less
time, without doing too much time-expensive research.
After all, sometimes, writing shorter code decreases human readability
without improving performance in any way, and I think that this would
be one of these times... so... :))
> Apart from that, this patch looks good to me.
>
Thank you!
> Regards,
> Bjorn
>
>> + sel = i;
>> + }
>> + if (sel < 0)
>> + return -EINVAL;
>> +
>> + /* Current limit setting needs secure access */
>> + ret = regmap_write(vreg->regmap, vreg->base + REG_LABIBB_SEC_ACCESS,
>> + LABIBB_SEC_UNLOCK_CODE);
>> + if (ret)
>> + return ret;
>> +
>> + mask = desc->csel_mask | lim->ovr_val;
>> + mask |= LABIBB_CURRENT_LIMIT_EN;
>> + val = (u32)sel | lim->ovr_val;
>> + val |= LABIBB_CURRENT_LIMIT_EN;
>> +
>> + return regmap_update_bits(vreg->regmap, desc->csel_reg, mask, val);
>> +}
>> +
>> +static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)
>> +{
>> + struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
>> + struct regulator_desc *desc = &vreg->desc;
>> + struct labibb_current_limits *lim = &vreg->uA_limits;
>> + unsigned int cur_step;
>> + int ret;
>> +
>> + ret = regmap_read(vreg->regmap, desc->csel_reg, &cur_step);
>> + if (ret)
>> + return ret;
>> + cur_step &= desc->csel_mask;
>> +
>> + return (cur_step * lim->uA_step) + lim->uA_min;
>> +}
>> +
>> static const struct regulator_ops qcom_labibb_ops = {
>> .enable = regulator_enable_regmap,
>> .disable = regulator_disable_regmap,
>> @@ -61,6 +128,8 @@ static const struct regulator_ops qcom_labibb_ops = {
>> .get_voltage_sel = regulator_get_voltage_sel_regmap,
>> .list_voltage = regulator_list_voltage_linear_range,
>> .map_voltage = regulator_map_voltage_linear_range,
>> + .set_current_limit = qcom_labibb_set_current_limit,
>> + .get_current_limit = qcom_labibb_get_current_limit,
>> };
>>
>> static const struct regulator_desc pmi8998_lab_desc = {
>> @@ -73,6 +142,9 @@ static const struct regulator_desc pmi8998_lab_desc = {
>> .vsel_mask = LAB_VOLTAGE_SET_MASK,
>> .apply_reg = (PMI8998_LAB_REG_BASE + REG_LABIBB_VOLTAGE),
>> .apply_bit = LABIBB_VOLTAGE_OVERRIDE_EN,
>> + .csel_reg = (PMI8998_LAB_REG_BASE + REG_LABIBB_CURRENT_LIMIT),
>> + .csel_mask = LAB_CURRENT_LIMIT_MASK,
>> + .n_current_limits = 8,
>> .off_on_delay = LABIBB_OFF_ON_DELAY,
>> .owner = THIS_MODULE,
>> .type = REGULATOR_VOLTAGE,
>> @@ -94,6 +166,9 @@ static const struct regulator_desc pmi8998_ibb_desc = {
>> .vsel_mask = IBB_VOLTAGE_SET_MASK,
>> .apply_reg = (PMI8998_IBB_REG_BASE + REG_LABIBB_VOLTAGE),
>> .apply_bit = LABIBB_VOLTAGE_OVERRIDE_EN,
>> + .csel_reg = (PMI8998_IBB_REG_BASE + REG_LABIBB_CURRENT_LIMIT),
>> + .csel_mask = IBB_CURRENT_LIMIT_MASK,
>> + .n_current_limits = 32,
>> .off_on_delay = LABIBB_OFF_ON_DELAY,
>> .owner = THIS_MODULE,
>> .type = REGULATOR_VOLTAGE,
>> @@ -167,6 +242,23 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)
>> vreg->base = reg_data->base;
>> vreg->type = reg_data->type;
>>
>> + switch (vreg->type) {
>> + case QCOM_LAB_TYPE:
>> + /* LAB Limits: 200-1600mA */
>> + vreg->uA_limits.uA_min = 200000;
>> + vreg->uA_limits.uA_step = 200000;
>> + vreg->uA_limits.ovr_val = LAB_CURRENT_LIMIT_OVERRIDE_EN;
>> + break;
>> + case QCOM_IBB_TYPE:
>> + /* IBB Limits: 0-1550mA */
>> + vreg->uA_limits.uA_min = 0;
>> + vreg->uA_limits.uA_step = 50000;
>> + vreg->uA_limits.ovr_val = 0; /* No override bit */
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> memcpy(&vreg->desc, reg_data->desc, sizeof(vreg->desc));
>> vreg->desc.of_match = reg_data->name;
>> vreg->desc.name = reg_data->name;
>> --
>> 2.29.2
>>
Powered by blists - more mailing lists