| Message ID | 20200212075529.156756-1-drinkcat@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | PM / OPP: Add support for multiple regulators | expand |
On 12-02-20, 15:55, Nicolas Boichat wrote: > The OPP table can contain multiple voltages and regulators, and > implementing that logic does not add a lot of code or complexity, > so let's modify _generic_set_opp_regulator to support that use > case. > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> This is already supported in a different way. See how following driver does this (hint dev_pm_opp_register_set_opp_helper()). drivers/opp/ti-opp-supply.c The problem with a generic solution is that we can't assume an order for programming of the regulators, as this can be complex on few platforms.
On Wed, Feb 12, 2020 at 4:13 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 12-02-20, 15:55, Nicolas Boichat wrote: > > The OPP table can contain multiple voltages and regulators, and > > implementing that logic does not add a lot of code or complexity, > > so let's modify _generic_set_opp_regulator to support that use > > case. > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > This is already supported in a different way. See how following driver > does this (hint dev_pm_opp_register_set_opp_helper()). > > drivers/opp/ti-opp-supply.c > > The problem with a generic solution is that we can't assume an order > for programming of the regulators, as this can be complex on few > platforms. I see... And you're right that it's probably best to change the voltages in a specific order (I just ignored that problem ,-P). I do wonder if there's something we could do in the core/DT to specify that order (if it's a simple order?), it's not really ideal to have to copy paste code around... > -- > viresh
On 12-02-20, 16:57, Nicolas Boichat wrote: > I see... And you're right that it's probably best to change the > voltages in a specific order (I just ignored that problem ,-P). I do > wonder if there's something we could do in the core/DT to specify that > order (if it's a simple order?), it's not really ideal to have to copy > paste code around... I will suggest adding your own version (like TI) for now, if we later feel that there is too much duplicate code, we can look for an alternative. But as of now, there aren't a lot of platforms using multiple regulators anyway.
On Wed, Feb 12, 2020 at 5:04 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 12-02-20, 16:57, Nicolas Boichat wrote: > > I see... And you're right that it's probably best to change the > > voltages in a specific order (I just ignored that problem ,-P). I do > > wonder if there's something we could do in the core/DT to specify that > > order (if it's a simple order?), it's not really ideal to have to copy > > paste code around... > > I will suggest adding your own version (like TI) for now, if we later > feel that there is too much duplicate code, we can look for an > alternative. But as of now, there aren't a lot of platforms using > multiple regulators anyway. Will do. Thanks! > -- > viresh
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index ba43e6a3dc0aeed..ea4a12a8932014f 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -650,6 +650,24 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg, return ret; } +/* + * Set multiple voltages. The caller is responsible for restoring all the + * voltages if the function fails. + */ +static int _set_opp_voltages(struct device *dev, struct regulator **regs, + struct dev_pm_opp_supply *supplies, int count) +{ + int i, ret; + + for (i = 0; i < count; i++) { + ret = _set_opp_voltage(dev, regs[i], &supplies[i]); + if (ret) + return ret; + } + + return 0; +} + static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk, unsigned long freq) { @@ -671,18 +689,13 @@ static int _generic_set_opp_regulator(const struct opp_table *opp_table, struct dev_pm_opp_supply *old_supply, struct dev_pm_opp_supply *new_supply) { - struct regulator *reg = opp_table->regulators[0]; + struct regulator **regs = opp_table->regulators; + int count = opp_table->regulator_count; int ret; - /* This function only supports single regulator per device */ - if (WARN_ON(opp_table->regulator_count > 1)) { - dev_err(dev, "multiple regulators are not supported\n"); - return -EINVAL; - } - /* Scaling up? Scale voltage before frequency */ if (freq >= old_freq) { - ret = _set_opp_voltage(dev, reg, new_supply); + ret = _set_opp_voltages(dev, regs, new_supply, count); if (ret) goto restore_voltage; } @@ -694,7 +707,7 @@ static int _generic_set_opp_regulator(const struct opp_table *opp_table, /* Scaling down? Scale voltage after frequency */ if (freq < old_freq) { - ret = _set_opp_voltage(dev, reg, new_supply); + ret = _set_opp_voltages(dev, regs, new_supply, count); if (ret) goto restore_freq; } @@ -708,7 +721,7 @@ static int _generic_set_opp_regulator(const struct opp_table *opp_table, restore_voltage: /* This shouldn't harm even if the voltages weren't updated earlier */ if (old_supply) - _set_opp_voltage(dev, reg, old_supply); + _set_opp_voltages(dev, regs, old_supply, count); return ret; }
The OPP table can contain multiple voltages and regulators, and implementing that logic does not add a lot of code or complexity, so let's modify _generic_set_opp_regulator to support that use case. Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> --- This is required to add panfrost support for the Bifrost GPU in MT8183, see this patch: https://patchwork.kernel.org/patch/11369821/ drivers/opp/core.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)