| Message ID | 20180316040824.21472-1-rnayak@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add powerdomain driver for corners on msm8996 | expand |
On 16-03-18, 09:38, Rajendra Nayak wrote: > With genpd now expecting powerdomain drivers supporting performance > state to support get/set performance state callbacks, add support for it > in the rpmpd driver. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/soc/qcom/rpmpd.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c > index c8754d867c33..4058c5b450c6 100644 > --- a/drivers/soc/qcom/rpmpd.c > +++ b/drivers/soc/qcom/rpmpd.c > @@ -14,6 +14,7 @@ > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > +#include <linux/pm_opp.h> > #include <linux/soc/qcom/smd-rpm.h> > > #include <dt-bindings/mfd/qcom-rpm.h> > @@ -29,6 +30,8 @@ > #define KEY_ENABLE 0x6e657773 /* swen */ > #define KEY_FLOOR_CORNER 0x636676 /* vfc */ > > +#define MAX_RPMPD_STATE 6 > + > #define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id) \ > static struct rpmpd _platform##_##_active; \ > static struct rpmpd _platform##_##_name = { \ > @@ -222,6 +225,43 @@ static int rpmpd_power_off(struct generic_pm_domain *domain) > return ret; > } > > +static int rpmpd_set_performance(struct generic_pm_domain *domain, > + unsigned int state) > +{ > + int ret = 0; > + struct rpmpd *pd = domain_to_rpmpd(domain); > + > + mutex_lock(&rpmpd_lock); > + > + if (state > MAX_RPMPD_STATE) > + goto out; > + > + pd->corner = state; > + > + if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER)) > + goto out; > + > + ret = rpmpd_aggregate_corner(pd); > + > +out: > + mutex_unlock(&rpmpd_lock); > + > + return ret; > +} > + > +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd, > + struct dev_pm_opp *opp) > +{ > + struct device_node *np; > + unsigned int corner = 0; > + > + np = dev_pm_opp_get_of_node(opp); > + of_property_read_u32(np, "qcom,level", &corner); Don't we want to error out or do something else in case of an error ? > + of_node_put(np); > + > + return corner; > +} > + > static int rpmpd_probe(struct platform_device *pdev) > { > int i; > @@ -259,6 +299,8 @@ static int rpmpd_probe(struct platform_device *pdev) > rpmpds[i]->rpm = rpm; > rpmpds[i]->pd.power_off = rpmpd_power_off; > rpmpds[i]->pd.power_on = rpmpd_power_on; > + rpmpds[i]->pd.set_performance_state = rpmpd_set_performance; > + rpmpds[i]->pd.get_performance_state = rpmpd_get_performance; > pm_genpd_init(&rpmpds[i]->pd, NULL, true); > > data->domains[i] = &rpmpds[i]->pd; Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On 16-03-18, 09:38, Rajendra Nayak wrote: > Add rpmpd device node and its OPP table > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 46 +++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index 0a6f7952bbb1..43757a078146 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -297,6 +297,52 @@ > #clock-cells = <1>; > }; > > + rpmpd: qcom,rpmpd { > + compatible = "qcom,rpmpd-msm8996"; > + #power-domain-cells = <1>; > + operating-points-v2 = <&rpmpd_opp_table>, /* cx */ > + <&rpmpd_opp_table>, /* cx_ao */ > + <&rpmpd_opp_table>, /* cx_vfc */ > + <&rpmpd_opp_table>, /* mx */ > + <&rpmpd_opp_table>, /* mx_ao */ > + <&rpmpd_opp_table>, /* sscx */ > + <&rpmpd_opp_table>; /* sscx_vfc */ > + }; > + > + rpmpd_opp_table: opp-table { > + compatible = "operating-points-v2", "operating-points-v2-qcom"; > + > + rpmpd_opp1: opp@1 { > + opp-hz = /bits/ 64 <1>; Actually this (magic values) isn't allowed right now. I will send a patchset to make this property optional later on separately (once the other series is accepted). > + qcom,level = <1>; > + }; > + > + rpmpd_opp2: opp@2 { > + opp-hz = /bits/ 64 <2>; > + qcom,level = <2>; > + }; > + > + rpmpd_opp3: opp@3 { > + opp-hz = /bits/ 64 <3>; > + qcom,level = <3>; > + }; > + > + rpmpd_opp4: opp@4 { > + opp-hz = /bits/ 64 <4>; > + qcom,level = <4>; > + }; > + > + rpmpd_opp5: opp@5 { > + opp-hz = /bits/ 64 <5>; > + qcom,level = <5>; > + }; > + > + rpmpd_opp6: opp@6 { > + opp-hz = /bits/ 64 <6>; > + qcom,level = <6>; > + }; > + }; > + > pm8994-regulators { > compatible = "qcom,rpm-pm8994-regulators"; > Everything else is fine. After you remove the opp-hz property from this table: Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On 16-03-18, 09:38, Rajendra Nayak wrote: > @@ -1540,6 +1571,9 @@ static int sdhci_msm_probe(struct platform_device *pdev) > pm_runtime_disable(&pdev->dev); > pm_runtime_set_suspended(&pdev->dev); > pm_runtime_put_noidle(&pdev->dev); > + dev_pm_opp_of_remove_table(&pdev->dev); You can't do this if there is no OPP table. Probably you should just make it part of the below if block. > + if (msm_host->opp_table) > + dev_pm_opp_put_clkname(msm_host->opp_table); > clk_disable: > clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks), > msm_host->bulk_clks); > @@ -1564,6 +1598,9 @@ static int sdhci_msm_remove(struct platform_device *pdev) > pm_runtime_get_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > pm_runtime_put_noidle(&pdev->dev); > + dev_pm_opp_of_remove_table(&pdev->dev); And this too. > + if (msm_host->opp_table) > + dev_pm_opp_put_clkname(msm_host->opp_table); > > clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks), > msm_host->bulk_clks); Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On 16-03-18, 09:38, Rajendra Nayak wrote: > As we move from no clients/consumers in kernel voting on corners, > to *some* voting and some not voting, we might end up in a situation > where the clients which remove votes can adversly impact others > who still don't have a way to vote. > > To avoid this situation, have a max vote on all corners at init. > This should/can be removed once we have all clients moved to > be able to vote/unvote for themselves. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > drivers/soc/qcom/rpmpd.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c > index 4058c5b450c6..ebdcf9398441 100644 > --- a/drivers/soc/qcom/rpmpd.c > +++ b/drivers/soc/qcom/rpmpd.c > @@ -304,6 +304,15 @@ static int rpmpd_probe(struct platform_device *pdev) > pm_genpd_init(&rpmpds[i]->pd, NULL, true); > > data->domains[i] = &rpmpds[i]->pd; > + > + /* > + * Until we have all consumers voting on corners > + * just vote the max corner on all PDs > + * This should ideally be *removed* once we have > + * all (most) consumers being able to vote > + */ > + rpmpd_set_performance(&rpmpds[i]->pd, MAX_RPMPD_STATE); > + rpmpd_power_on(&rpmpds[i]->pd); > } > > return of_genpd_add_provider_onecell(pdev->dev.of_node, data); Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On 03/16/2018 10:05 AM, Viresh Kumar wrote: > On 16-03-18, 09:38, Rajendra Nayak wrote: >> With genpd now expecting powerdomain drivers supporting performance >> state to support get/set performance state callbacks, add support for it >> in the rpmpd driver. >> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> drivers/soc/qcom/rpmpd.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 42 insertions(+) >> >> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c >> index c8754d867c33..4058c5b450c6 100644 >> --- a/drivers/soc/qcom/rpmpd.c >> +++ b/drivers/soc/qcom/rpmpd.c >> @@ -14,6 +14,7 @@ >> #include <linux/of.h> >> #include <linux/of_device.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_opp.h> >> #include <linux/soc/qcom/smd-rpm.h> >> >> #include <dt-bindings/mfd/qcom-rpm.h> >> @@ -29,6 +30,8 @@ >> #define KEY_ENABLE 0x6e657773 /* swen */ >> #define KEY_FLOOR_CORNER 0x636676 /* vfc */ >> >> +#define MAX_RPMPD_STATE 6 >> + >> #define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id) \ >> static struct rpmpd _platform##_##_active; \ >> static struct rpmpd _platform##_##_name = { \ >> @@ -222,6 +225,43 @@ static int rpmpd_power_off(struct generic_pm_domain *domain) >> return ret; >> } >> >> +static int rpmpd_set_performance(struct generic_pm_domain *domain, >> + unsigned int state) >> +{ >> + int ret = 0; >> + struct rpmpd *pd = domain_to_rpmpd(domain); >> + >> + mutex_lock(&rpmpd_lock); >> + >> + if (state > MAX_RPMPD_STATE) >> + goto out; >> + >> + pd->corner = state; >> + >> + if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER)) >> + goto out; >> + >> + ret = rpmpd_aggregate_corner(pd); >> + >> +out: >> + mutex_unlock(&rpmpd_lock); >> + >> + return ret; >> +} >> + >> +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd, >> + struct dev_pm_opp *opp) >> +{ >> + struct device_node *np; >> + unsigned int corner = 0; >> + >> + np = dev_pm_opp_get_of_node(opp); >> + of_property_read_u32(np, "qcom,level", &corner); > > Don't we want to error out or do something else in case of an error ? yes, I missed the error checks, will add.
Quoting Rajendra Nayak (2018-03-15 21:08:18) > From: Viresh Kumar <viresh.kumar@linaro.org> > > This adds a new helper to let the power domain drivers to access > opp->np, so that they can read platform specific properties from the > node. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- Are there two versions of this patch floating around? > drivers/opp/of.c | 19 +++++++++++++++++++ > include/linux/pm_opp.h | 5 +++++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 21265af55117..b17715bc3c0a 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -736,3 +736,22 @@ struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, > return opp; > } > EXPORT_SYMBOL_GPL(of_dev_pm_opp_find_required_opp); > + > +/** > + * dev_pm_opp_get_of_node() - Gets the DT node corresponding to an opp > + * @opp: opp for which DT node has to be returned for > + * > + * Return: DT node corresponding to the opp, else 0 on success. > + * > + * The caller needs to put the node of_node_put() after using it. put the node with of_node_put() -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07-04-18, 06:00, Stephen Boyd wrote: > Quoting Rajendra Nayak (2018-03-15 21:08:18) > > From: Viresh Kumar <viresh.kumar@linaro.org> > > > > This adds a new helper to let the power domain drivers to access > > opp->np, so that they can read platform specific properties from the > > node. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > > --- > > Are there two versions of this patch floating around? Yeah, I will carry this patch in my series now going forward. > > drivers/opp/of.c | 19 +++++++++++++++++++ > > include/linux/pm_opp.h | 5 +++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > > index 21265af55117..b17715bc3c0a 100644 > > --- a/drivers/opp/of.c > > +++ b/drivers/opp/of.c > > @@ -736,3 +736,22 @@ struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, > > return opp; > > } > > EXPORT_SYMBOL_GPL(of_dev_pm_opp_find_required_opp); > > + > > +/** > > + * dev_pm_opp_get_of_node() - Gets the DT node corresponding to an opp > > + * @opp: opp for which DT node has to be returned for > > + * > > + * Return: DT node corresponding to the opp, else 0 on success. > > + * > > + * The caller needs to put the node of_node_put() after using it. > > put the node with of_node_put() Fixed it up, thanks.
On Sat, Apr 07, 2018 at 06:00:29AM -0700, Stephen Boyd wrote: > Quoting Rajendra Nayak (2018-03-15 21:08:18) > > From: Viresh Kumar <viresh.kumar@linaro.org> > > > > This adds a new helper to let the power domain drivers to access > > opp->np, so that they can read platform specific properties from the > > node. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > > --- > > Are there two versions of this patch floating around? The other one was mine, but it is identical to this. Jordan
Quoting Rajendra Nayak (2018-03-15 21:08:22) > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index 0a6f7952bbb1..43757a078146 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -297,6 +297,52 @@ > #clock-cells = <1>; > }; > > + rpmpd: qcom,rpmpd { power-controller? power-domain-controller? power-domains? Or something like that. > + compatible = "qcom,rpmpd-msm8996"; > + #power-domain-cells = <1>; > + operating-points-v2 = <&rpmpd_opp_table>, /* cx */ > + <&rpmpd_opp_table>, /* cx_ao */ > + <&rpmpd_opp_table>, /* cx_vfc */ > + <&rpmpd_opp_table>, /* mx */ > + <&rpmpd_opp_table>, /* mx_ao */ > + <&rpmpd_opp_table>, /* sscx */ > + <&rpmpd_opp_table>; /* sscx_vfc */ > + }; > + > + rpmpd_opp_table: opp-table { This should go into the root of the tree? Otherwise it may be populated by the RPMh platform populate code which would be odd. We should go and update the platform populate code to always ignore operating-points-v2 compatible nodes too. > + compatible = "operating-points-v2", "operating-points-v2-qcom"; This is backwards? I thought more specific compatible went first. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09-04-18, 09:03, Stephen Boyd wrote: > We should go and > update the platform populate code to always ignore operating-points-v2 > compatible nodes too. Will do that.
On 04/09/2018 09:33 PM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2018-03-15 21:08:22) >> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi >> index 0a6f7952bbb1..43757a078146 100644 >> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi >> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi >> @@ -297,6 +297,52 @@ >> #clock-cells = <1>; >> }; >> >> + rpmpd: qcom,rpmpd { > > power-controller? power-domain-controller? power-domains? Or something > like that. > >> + compatible = "qcom,rpmpd-msm8996"; >> + #power-domain-cells = <1>; >> + operating-points-v2 = <&rpmpd_opp_table>, /* cx */ >> + <&rpmpd_opp_table>, /* cx_ao */ >> + <&rpmpd_opp_table>, /* cx_vfc */ >> + <&rpmpd_opp_table>, /* mx */ >> + <&rpmpd_opp_table>, /* mx_ao */ >> + <&rpmpd_opp_table>, /* sscx */ >> + <&rpmpd_opp_table>; /* sscx_vfc */ >> + }; >> + >> + rpmpd_opp_table: opp-table { > > This should go into the root of the tree? Otherwise it may be populated > by the RPMh platform populate code which would be odd. We should go and > update the platform populate code to always ignore operating-points-v2 > compatible nodes too. > >> + compatible = "operating-points-v2", "operating-points-v2-qcom"; > > This is backwards? I thought more specific compatible went first. thanks for the review, will fixup all of these when I respin.
With performance state support for genpd merged, and with some more patches to add support for them in the OPP layer [1] under discussion, this is an effort to model a powerdomain driver to communicate corner/level values for qualcomm platforms to RPM (Remote Power Manager) This series adds data specific to msm8996 and is tested on the db820c. We also modify mmc as one of the first devices to move to using an OPP table and vote on corners using the performance state infrastructure. [1] https://lwn.net/Articles/742136/ Rajendra Nayak (6): soc: qcom: rpmpd: Add a powerdomain driver to model corners dt-bindings: opp: Introduce qcom-opp bindings soc: qcom: rpmpd: Add support for get/set performance state arm64: dts: msm8996: Add rpmpd device node mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance state soc: qcom: rpmpd: Add a max vote on all corners at init Viresh Kumar (1): PM / OPP: Add dev_pm_opp_get_of_node() Documentation/devicetree/bindings/opp/qcom-opp.txt | 25 ++ .../devicetree/bindings/power/qcom,rpmpd.txt | 14 + arch/arm64/boot/dts/qcom/msm8996.dtsi | 87 +++++ drivers/clk/qcom/gcc-msm8996.c | 8 +- drivers/mmc/host/sdhci-msm.c | 57 +++- drivers/opp/of.c | 19 ++ drivers/soc/qcom/Kconfig | 9 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/rpmpd.c | 350 +++++++++++++++++++++ include/linux/pm_opp.h | 5 + 10 files changed, 561 insertions(+), 14 deletions(-) create mode 100644 Documentation/devicetree/bindings/opp/qcom-opp.txt create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt create mode 100644 drivers/soc/qcom/rpmpd.c