| Message ID | 20231004-reg-smd-unused-v1-2-5d682493d555@kernkonzept.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | regulator: qcom_smd: Disable unused regulators | expand |
On 4.10.2023 16:17, Stephan Gerhold wrote: > The RPM firmware on Qualcomm platforms does not provide a way to check > if a regulator is on during boot using the SMD interface. If the > regulators are already on during boot and Linux does not make use of > them they will currently stay enabled forever. The regulator core does > not know these regulators are on and cannot clean them up together with > the other unused regulators. > > Fix this by setting the initial enable state to -EINVAL similar to > qcom-rpmh-regulator.c. The regulator core will then also explicitly > disable all unused regulators with unknown status. > > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > --- > NOTE: This has a slight potential of breaking boards that rely on having > unused regulators permanently enabled (without regulator-always-on). > However, this is always a mistake in the device tree so it's probably > better to risk some breakage now, add the missing regulators and avoid > this problem for all future boards. > --- > drivers/regulator/qcom_smd-regulator.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c > index f53ada076252..0bbfba2e17ff 100644 > --- a/drivers/regulator/qcom_smd-regulator.c > +++ b/drivers/regulator/qcom_smd-regulator.c > @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg) > reqlen++; > } > > - if (vreg->uv_updated && vreg->is_enabled) { > + if (vreg->uv_updated && vreg->is_enabled > 0) { At a quick glance, are there any states for this value, other than 0 and 1? This is not the regulator_ops->is_enabled, but qcom_rpm_reg->is_enabled. Konrad
On Fri, Oct 06, 2023 at 11:15:40PM +0200, Konrad Dybcio wrote: > On 4.10.2023 16:17, Stephan Gerhold wrote: > > The RPM firmware on Qualcomm platforms does not provide a way to check > > if a regulator is on during boot using the SMD interface. If the > > regulators are already on during boot and Linux does not make use of > > them they will currently stay enabled forever. The regulator core does > > not know these regulators are on and cannot clean them up together with > > the other unused regulators. > > > > Fix this by setting the initial enable state to -EINVAL similar to > > qcom-rpmh-regulator.c. The regulator core will then also explicitly > > disable all unused regulators with unknown status. > > > > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > > --- > > NOTE: This has a slight potential of breaking boards that rely on having > > unused regulators permanently enabled (without regulator-always-on). > > However, this is always a mistake in the device tree so it's probably > > better to risk some breakage now, add the missing regulators and avoid > > this problem for all future boards. > > --- > > drivers/regulator/qcom_smd-regulator.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c > > index f53ada076252..0bbfba2e17ff 100644 > > --- a/drivers/regulator/qcom_smd-regulator.c > > +++ b/drivers/regulator/qcom_smd-regulator.c > > @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg) > > reqlen++; > > } > > > > - if (vreg->uv_updated && vreg->is_enabled) { > > + if (vreg->uv_updated && vreg->is_enabled > 0) { > At a quick glance, are there any states for this value, other > than 0 and 1? This is not the regulator_ops->is_enabled, but > qcom_rpm_reg->is_enabled. > Yes, I initially assign vreg->is_enabled = -EINVAL (for use with PATCH 1/2). It's in the part of the patch that you trimmed in your reply. :D Thanks, Stephan @@ -1377,6 +1377,7 @@ static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev vreg->rpm = rpm; vreg->type = rpm_data->type; vreg->id = rpm_data->id; + vreg->is_enabled = -EINVAL; memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc)); vreg->desc.name = rpm_data->name;
On 10/9/23 22:23, Stephan Gerhold wrote: > On Fri, Oct 06, 2023 at 11:15:40PM +0200, Konrad Dybcio wrote: >> On 4.10.2023 16:17, Stephan Gerhold wrote: >>> The RPM firmware on Qualcomm platforms does not provide a way to check >>> if a regulator is on during boot using the SMD interface. If the >>> regulators are already on during boot and Linux does not make use of >>> them they will currently stay enabled forever. The regulator core does >>> not know these regulators are on and cannot clean them up together with >>> the other unused regulators. >>> >>> Fix this by setting the initial enable state to -EINVAL similar to >>> qcom-rpmh-regulator.c. The regulator core will then also explicitly >>> disable all unused regulators with unknown status. >>> >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> >>> --- >>> NOTE: This has a slight potential of breaking boards that rely on having >>> unused regulators permanently enabled (without regulator-always-on). >>> However, this is always a mistake in the device tree so it's probably >>> better to risk some breakage now, add the missing regulators and avoid >>> this problem for all future boards. >>> --- >>> drivers/regulator/qcom_smd-regulator.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c >>> index f53ada076252..0bbfba2e17ff 100644 >>> --- a/drivers/regulator/qcom_smd-regulator.c >>> +++ b/drivers/regulator/qcom_smd-regulator.c >>> @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg) >>> reqlen++; >>> } >>> >>> - if (vreg->uv_updated && vreg->is_enabled) { >>> + if (vreg->uv_updated && vreg->is_enabled > 0) { >> At a quick glance, are there any states for this value, other >> than 0 and 1? This is not the regulator_ops->is_enabled, but >> qcom_rpm_reg->is_enabled. >> > > Yes, I initially assign vreg->is_enabled = -EINVAL (for use with PATCH > 1/2). It's in the part of the patch that you trimmed in your reply. :D > > Thanks, > Stephan Oh, right ^^ Konrad
diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c index f53ada076252..0bbfba2e17ff 100644 --- a/drivers/regulator/qcom_smd-regulator.c +++ b/drivers/regulator/qcom_smd-regulator.c @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg) reqlen++; } - if (vreg->uv_updated && vreg->is_enabled) { + if (vreg->uv_updated && vreg->is_enabled > 0) { 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) { + if (vreg->load_updated && vreg->is_enabled > 0) { 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); @@ -1377,6 +1377,7 @@ static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev vreg->rpm = rpm; vreg->type = rpm_data->type; vreg->id = rpm_data->id; + vreg->is_enabled = -EINVAL; memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc)); vreg->desc.name = rpm_data->name;
The RPM firmware on Qualcomm platforms does not provide a way to check if a regulator is on during boot using the SMD interface. If the regulators are already on during boot and Linux does not make use of them they will currently stay enabled forever. The regulator core does not know these regulators are on and cannot clean them up together with the other unused regulators. Fix this by setting the initial enable state to -EINVAL similar to qcom-rpmh-regulator.c. The regulator core will then also explicitly disable all unused regulators with unknown status. Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> --- NOTE: This has a slight potential of breaking boards that rely on having unused regulators permanently enabled (without regulator-always-on). However, this is always a mistake in the device tree so it's probably better to risk some breakage now, add the missing regulators and avoid this problem for all future boards. --- drivers/regulator/qcom_smd-regulator.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)