| Message ID | 20200319053902.3415984-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | clk: qcom: gdsc: Handle supply regulators | expand |
Quoting Bjorn Andersson (2020-03-18 22:38:59) > Certain GDSCs, such as the GPU_GX on MSM8996, requires that the upstream > regulator supply is powered in order to be turned on. > > It's not guaranteed that the bootloader will leave these supplies on and > the driver core will attempt to enable any GDSCs before allowing the > individual drivers to probe defer on the PMIC regulator driver not yet > being present. > > So the gdsc driver needs to be made aware of supplying regulators and > probe defer on their absence, and it needs to enable and disable the > regulator accordingly. > > Voltage adjustments of the supplying regulator are deferred to the > client drivers themselves. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- Ok. Looks mostly fine to me. > drivers/clk/qcom/gdsc.c | 24 ++++++++++++++++++++++++ > drivers/clk/qcom/gdsc.h | 4 ++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index a250f59708d8..3528789cc9d0 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -13,6 +13,7 @@ > #include <linux/regmap.h> > #include <linux/reset-controller.h> > #include <linux/slab.h> > +#include <linux/regulator/consumer.h> Alphabetical? > #include "gdsc.h" > > #define PWR_ON_MASK BIT(31) > @@ -371,6 +385,16 @@ int gdsc_register(struct gdsc_desc *desc, > if (!data->domains) > return -ENOMEM; > > + /* Resolve any regulator supplies */ Drop the comment please, it's just saying what the code is doing. > + for (i = 0; i < num; i++) { > + if (!scs[i] || !scs[i]->supply) > + continue; > + > + scs[i]->rsupply = devm_regulator_get(dev, scs[i]->supply); > + if (IS_ERR(scs[i]->rsupply)) > + return PTR_ERR(scs[i]->rsupply); > + } > + > data->num_domains = num; > for (i = 0; i < num; i++) { > if (!scs[i])
Quoting Bjorn Andersson (2020-03-20 22:16:12) > On Fri 20 Mar 16:31 PDT 2020, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2020-03-18 22:39:00) > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml > > > index 85518494ce43..65d9aa790581 100644 > > > --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml > > > +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml > > > @@ -67,6 +67,10 @@ properties: > > > description: > > > Protected clock specifier list as per common clock binding > > > > > > + vdd_gfx-supply: > > > > Why not vdd-gfx-supply? What's with the underscore? > > > > The pad is named "VDD_GFX" in the datasheet and the schematics. I see > that we've started y/_/-/ in some of the newly added bindings, would you > prefer I follow this? If the datasheet has this then I guess it's fine. I'll wait for Rob to ack.
On Sat, Mar 21, 2020 at 11:43:12AM -0700, Stephen Boyd wrote: > Quoting Bjorn Andersson (2020-03-20 22:16:12) > > On Fri 20 Mar 16:31 PDT 2020, Stephen Boyd wrote: > > > > > Quoting Bjorn Andersson (2020-03-18 22:39:00) > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml > > > > index 85518494ce43..65d9aa790581 100644 > > > > --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml > > > > +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml > > > > @@ -67,6 +67,10 @@ properties: > > > > description: > > > > Protected clock specifier list as per common clock binding > > > > > > > > + vdd_gfx-supply: > > > > > > Why not vdd-gfx-supply? What's with the underscore? > > > > > > > The pad is named "VDD_GFX" in the datasheet and the schematics. I see > > that we've started y/_/-/ in some of the newly added bindings, would you > > prefer I follow this? > > If the datasheet has this then I guess it's fine. I'll wait for Rob to > ack. vddgfx or vdd-gfx. Rob
Hi Stephen, I think the upstream design always wanted the client/consumer to enable the GPU Rail and then turn ON the GDSC? Why are we going ahead with adding the support of regulator in the GDSC driver? On 3/19/2020 11:08 AM, Bjorn Andersson wrote: > Certain GDSCs, such as the GPU_GX on MSM8996, requires that the upstream > regulator supply is powered in order to be turned on. > > It's not guaranteed that the bootloader will leave these supplies on and > the driver core will attempt to enable any GDSCs before allowing the > individual drivers to probe defer on the PMIC regulator driver not yet > being present. > > So the gdsc driver needs to be made aware of supplying regulators and > probe defer on their absence, and it needs to enable and disable the > regulator accordingly. > > Voltage adjustments of the supplying regulator are deferred to the > client drivers themselves. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/clk/qcom/gdsc.c | 24 ++++++++++++++++++++++++ > drivers/clk/qcom/gdsc.h | 4 ++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index a250f59708d8..3528789cc9d0 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -13,6 +13,7 @@ > #include <linux/regmap.h> > #include <linux/reset-controller.h> > #include <linux/slab.h> > +#include <linux/regulator/consumer.h> > #include "gdsc.h" > > #define PWR_ON_MASK BIT(31) > @@ -112,6 +113,12 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status) > int ret; > u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK; > > + if (status == GDSC_ON && sc->rsupply) { > + ret = regulator_enable(sc->rsupply); > + if (ret < 0) > + return ret; > + } > + > ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val); > if (ret) > return ret; > @@ -143,6 +150,13 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status) > > ret = gdsc_poll_status(sc, status); > WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n"); > + > + if (!ret && status == GDSC_OFF && sc->rsupply) { > + ret = regulator_disable(sc->rsupply); > + if (ret < 0) > + return ret; > + } > + > return ret; > } > > @@ -371,6 +385,16 @@ int gdsc_register(struct gdsc_desc *desc, > if (!data->domains) > return -ENOMEM; > > + /* Resolve any regulator supplies */ > + for (i = 0; i < num; i++) { > + if (!scs[i] || !scs[i]->supply) > + continue; > + > + scs[i]->rsupply = devm_regulator_get(dev, scs[i]->supply); > + if (IS_ERR(scs[i]->rsupply)) > + return PTR_ERR(scs[i]->rsupply); > + } > + > data->num_domains = num; > for (i = 0; i < num; i++) { > if (!scs[i]) > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h > index 64cdc8cf0d4d..c36fc26dcdff 100644 > --- a/drivers/clk/qcom/gdsc.h > +++ b/drivers/clk/qcom/gdsc.h > @@ -10,6 +10,7 @@ > #include <linux/pm_domain.h> > > struct regmap; > +struct regulator; > struct reset_controller_dev; > > /** > @@ -52,6 +53,9 @@ struct gdsc { > struct reset_controller_dev *rcdev; > unsigned int *resets; > unsigned int reset_count; > + > + const char *supply; > + struct regulator *rsupply; > }; > > struct gdsc_desc { >