| Message ID | 1479122155-13393-2-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
[+cc Geert, Kevin, Simon] On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote: > This patch adds support to pm clocks via device tree, so that the clocks > can be turned on and off during runtime pm. This patch is required for > Qualcomm msm8996 pcie controller which sits on a bus with its own > power-domain and clocks. > > Without this patch the clock associated with the bus are never turned on. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd like an ack or at least a review from Geert or Simon. > --- > drivers/bus/simple-pm-bus.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c > index c5eb46c..63b7e8c 100644 > --- a/drivers/bus/simple-pm-bus.c > +++ b/drivers/bus/simple-pm-bus.c > @@ -11,6 +11,7 @@ > #include <linux/module.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/pm_clock.h> > #include <linux/pm_runtime.h> > > > @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > pm_runtime_enable(&pdev->dev); > > - if (np) > + if (np) { > + of_pm_clk_add_clks(&pdev->dev); > of_platform_populate(np, NULL, NULL, &pdev->dev); > + } > > return 0; > } > > +static const struct dev_pm_ops simple_pm_bus_pm_ops = { > + SET_RUNTIME_PM_OPS(pm_clk_suspend, > + pm_clk_resume, NULL) > +}; > + > static int simple_pm_bus_remove(struct platform_device *pdev) > { > dev_dbg(&pdev->dev, "%s\n", __func__); > > pm_runtime_disable(&pdev->dev); > + pm_clk_destroy(&pdev->dev); > + > return 0; > } > > @@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = { > .driver = { > .name = "simple-pm-bus", > .of_match_table = simple_pm_bus_of_match, > + .pm = &simple_pm_bus_pm_ops, > }, > }; > > -- > 2.10.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+cc linux-pm On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Geert, Kevin, Simon] > > On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote: >> This patch adds support to pm clocks via device tree, so that the clocks >> can be turned on and off during runtime pm. This patch is required for >> Qualcomm msm8996 pcie controller which sits on a bus with its own >> power-domain and clocks. >> >> Without this patch the clock associated with the bus are never turned on. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd > like an ack or at least a review from Geert or Simon. Thanks for letting me know! >> --- >> drivers/bus/simple-pm-bus.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c >> index c5eb46c..63b7e8c 100644 >> --- a/drivers/bus/simple-pm-bus.c >> +++ b/drivers/bus/simple-pm-bus.c >> @@ -11,6 +11,7 @@ >> #include <linux/module.h> >> #include <linux/of_platform.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_clock.h> >> #include <linux/pm_runtime.h> >> >> >> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev) >> >> pm_runtime_enable(&pdev->dev); >> >> - if (np) >> + if (np) { >> + of_pm_clk_add_clks(&pdev->dev); This should work out-of-the-box (that's the actual purpose of this driver), if the platform code that registers your PM Domain would take care of registering the clocks needed for PM management of the bus. Adding of_pm_clk_add_clks() here will start managing all clocks of the bus, which may not be wanted on all platforms. >> of_platform_populate(np, NULL, NULL, &pdev->dev); >> + } >> >> return 0; >> } >> >> +static const struct dev_pm_ops simple_pm_bus_pm_ops = { >> + SET_RUNTIME_PM_OPS(pm_clk_suspend, >> + pm_clk_resume, NULL) >> +}; >> + >> static int simple_pm_bus_remove(struct platform_device *pdev) >> { >> dev_dbg(&pdev->dev, "%s\n", __func__); >> >> pm_runtime_disable(&pdev->dev); >> + pm_clk_destroy(&pdev->dev); >> + >> return 0; >> } >> >> @@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = { >> .driver = { >> .name = "simple-pm-bus", >> .of_match_table = simple_pm_bus_of_match, >> + .pm = &simple_pm_bus_pm_ops, >> }, >> }; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+ Rajendra (qcom,gdsc author) On 15/11/16 08:23, Geert Uytterhoeven wrote: > +cc linux-pm > > On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> [+cc Geert, Kevin, Simon] >> >> On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote: >>> This patch adds support to pm clocks via device tree, so that the clocks >>> can be turned on and off during runtime pm. This patch is required for >>> Qualcomm msm8996 pcie controller which sits on a bus with its own >>> power-domain and clocks. >>> >>> Without this patch the clock associated with the bus are never turned on. >>> >>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd >> like an ack or at least a review from Geert or Simon. > > Thanks for letting me know! > >>> --- >>> drivers/bus/simple-pm-bus.c | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c >>> index c5eb46c..63b7e8c 100644 >>> --- a/drivers/bus/simple-pm-bus.c >>> +++ b/drivers/bus/simple-pm-bus.c >>> @@ -11,6 +11,7 @@ >>> #include <linux/module.h> >>> #include <linux/of_platform.h> >>> #include <linux/platform_device.h> >>> +#include <linux/pm_clock.h> >>> #include <linux/pm_runtime.h> >>> >>> >>> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev) >>> >>> pm_runtime_enable(&pdev->dev); >>> >>> - if (np) >>> + if (np) { >>> + of_pm_clk_add_clks(&pdev->dev); > > This should work out-of-the-box (that's the actual purpose of this driver), > if the platform code that registers your PM Domain would take care > of registering the clocks needed for PM management of the bus. Yep, if the pm domain provider takes care of the bus clks, then it would work. Am guessing that the clocks property in the DT node would be read by the PM domain provider and enable/disable during attach/detach callbacks. If that is true, then any device tree nodes which are not children of "simple-pm-bus" and consumers of power-domain provider would enable all (including non-bus clks) clks twice. Once in the power-domain provider and once in the actual driver. Is this expected behavior from power-domains in general? > > Adding of_pm_clk_add_clks() here will start managing all clocks of the bus, > which may not be wanted on all platforms. > That was the purpose. Rajendra, Looks like qcom gdsc pm domain provider driver does not handle bus clks along with power-domain, Is this something we should do? Or the bus driver take care of it? Thanks, srini >>> of_platform_populate(np, NULL, NULL, &pdev->dev); >>> + } >>> >>> return 0; >>> } >>> >>> +static const struct dev_pm_ops simple_pm_bus_pm_ops = { >>> + SET_RUNTIME_PM_OPS(pm_clk_suspend, >>> + pm_clk_resume, NULL) >>> +}; >>> + >>> static int simple_pm_bus_remove(struct platform_device *pdev) >>> { >>> dev_dbg(&pdev->dev, "%s\n", __func__); >>> >>> pm_runtime_disable(&pdev->dev); >>> + pm_clk_destroy(&pdev->dev); >>> + >>> return 0; >>> } >>> >>> @@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = { >>> .driver = { >>> .name = "simple-pm-bus", >>> .of_match_table = simple_pm_bus_of_match, >>> + .pm = &simple_pm_bus_pm_ops, >>> }, >>> }; > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Srini, On 11/15/2016 4:55 PM, Srinivas Kandagatla wrote: > + Rajendra (qcom,gdsc author) [].. >> >>>> --- >>>> drivers/bus/simple-pm-bus.c | 13 ++++++++++++- >>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c >>>> index c5eb46c..63b7e8c 100644 >>>> --- a/drivers/bus/simple-pm-bus.c >>>> +++ b/drivers/bus/simple-pm-bus.c >>>> @@ -11,6 +11,7 @@ >>>> #include <linux/module.h> >>>> #include <linux/of_platform.h> >>>> #include <linux/platform_device.h> >>>> +#include <linux/pm_clock.h> >>>> #include <linux/pm_runtime.h> >>>> >>>> >>>> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct >>>> platform_device *pdev) >>>> >>>> pm_runtime_enable(&pdev->dev); >>>> >>>> - if (np) >>>> + if (np) { >>>> + of_pm_clk_add_clks(&pdev->dev); >> >> This should work out-of-the-box (that's the actual purpose of this >> driver), >> if the platform code that registers your PM Domain would take care >> of registering the clocks needed for PM management of the bus. > > Yep, if the pm domain provider takes care of the bus clks, then it would > work. > > Am guessing that the clocks property in the DT node would be read by the > PM domain provider and enable/disable during attach/detach callbacks. > If that is true, then any device tree nodes which are not children of > "simple-pm-bus" and consumers of power-domain provider would enable all > (including non-bus clks) clks twice. Once in the power-domain provider > and once in the actual driver. Is this expected behavior from > power-domains in general? > >> >> Adding of_pm_clk_add_clks() here will start managing all clocks of the >> bus, >> which may not be wanted on all platforms. >> > That was the purpose. > > > Rajendra, > Looks like qcom gdsc pm domain provider driver does not handle bus clks > along with power-domain, Is this something we should do? Or the bus > driver take care of it? I did post some patches to support handling of clocks associated with gdscs [1], but it got dropped at that point since there wasn't a real user, besides there were some open issues wrt the handling of !CONFIG_PM cases etc. I will revive and repost those patches again now based on the discussions last time around. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/362492.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Rajendra for pointing to the patch On 16/11/16 15:50, Nayak, Rajendra wrote: >> >> >> Rajendra, >> Looks like qcom gdsc pm domain provider driver does not handle bus clks >> along with power-domain, Is this something we should do? Or the bus >> driver take care of it? > > I did post some patches to support handling of clocks associated with > gdscs [1], but it got dropped at that point since there wasn't a > real user, besides there were some open issues wrt the handling of > !CONFIG_PM cases etc. > I will revive and repost those patches again now based on the > discussions last time around. > > [1] > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/362492.html This patch looks exactly like the one useful in pcie case, i would be interesting to see the final patch on how we handle clocks which are both related and not related to power domain. thanks, srini > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 15 Nov 00:23 PST 2016, Geert Uytterhoeven wrote: > +cc linux-pm > > On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Geert, Kevin, Simon] > > > > On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote: > >> This patch adds support to pm clocks via device tree, so that the clocks > >> can be turned on and off during runtime pm. This patch is required for > >> Qualcomm msm8996 pcie controller which sits on a bus with its own > >> power-domain and clocks. > >> > >> Without this patch the clock associated with the bus are never turned on. > >> > >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > > > I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd > > like an ack or at least a review from Geert or Simon. > > Thanks for letting me know! > > >> --- > >> drivers/bus/simple-pm-bus.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c > >> index c5eb46c..63b7e8c 100644 > >> --- a/drivers/bus/simple-pm-bus.c > >> +++ b/drivers/bus/simple-pm-bus.c > >> @@ -11,6 +11,7 @@ > >> #include <linux/module.h> > >> #include <linux/of_platform.h> > >> #include <linux/platform_device.h> > >> +#include <linux/pm_clock.h> > >> #include <linux/pm_runtime.h> > >> > >> > >> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > >> > >> pm_runtime_enable(&pdev->dev); > >> > >> - if (np) > >> + if (np) { > >> + of_pm_clk_add_clks(&pdev->dev); > > This should work out-of-the-box (that's the actual purpose of this driver), > if the platform code that registers your PM Domain would take care > of registering the clocks needed for PM management of the bus. > Hi Geert, I'm having problems finding any code that would make this work "out-of-the-box". The DT binding documents a clocks property but I can't find any code referencing this in the kernel. I see that Srinivas interpreted your response as that we should fold the clocks in behind the power-domain, rather than referencing them from the bus - but this seems awkward and would indicate the DT binding being wrong. Perhaps I'm just misunderstanding the design here? Which "platform code" do you refer to, can you help me by pointing me to the code that handles the zb_clk in the Renesas case? > Adding of_pm_clk_add_clks() here will start managing all clocks of the bus, > which may not be wanted on all platforms. > It would not be strange to do so in the "simple" implementation for the bus, allowing custom behavior to be implemented in a more specific driver for a platform with custom needs. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On Thu, Dec 15, 2016 at 11:26 PM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Tue 15 Nov 00:23 PST 2016, Geert Uytterhoeven wrote: >> On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote: >> >> This patch adds support to pm clocks via device tree, so that the clocks >> >> can be turned on and off during runtime pm. This patch is required for >> >> Qualcomm msm8996 pcie controller which sits on a bus with its own >> >> power-domain and clocks. >> >> >> >> Without this patch the clock associated with the bus are never turned on. >> >> >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> > >> > I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd >> > like an ack or at least a review from Geert or Simon. >> >> Thanks for letting me know! >> >> >> --- >> >> drivers/bus/simple-pm-bus.c | 13 ++++++++++++- >> >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c >> >> index c5eb46c..63b7e8c 100644 >> >> --- a/drivers/bus/simple-pm-bus.c >> >> +++ b/drivers/bus/simple-pm-bus.c >> >> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev) >> >> >> >> pm_runtime_enable(&pdev->dev); >> >> >> >> - if (np) >> >> + if (np) { >> >> + of_pm_clk_add_clks(&pdev->dev); >> >> This should work out-of-the-box (that's the actual purpose of this driver), >> if the platform code that registers your PM Domain would take care >> of registering the clocks needed for PM management of the bus. > I'm having problems finding any code that would make this work > "out-of-the-box". The DT binding documents a clocks property but I > can't find any code referencing this in the kernel. > > I see that Srinivas interpreted your response as that we should fold the > clocks in behind the power-domain, rather than referencing them from the > bus - but this seems awkward and would indicate the DT binding being > wrong. Perhaps I'm just misunderstanding the design here? Platform-wide PM depends heavily on the platform. Instead of adding code to handle all relevant platforms to all drivers, the generic PM Domain code is used. The "power-domains" and corresponding PM "clocks" properties may be added to any device name, depending on the platform. > Which "platform code" do you refer to, can you help me by pointing me to > the code that handles the zb_clk in the Renesas case? See drivers/clk/renesas/clk-mstp.c:cpg_mstp_attach_dev(), which registers the clocks that are used for PM. If a PM Domain sets the GENPD_FLAG_PM_CLK flag, these clocks are enabled resp. disabled by the genpd code when the device is resumed resp. suspend. >> Adding of_pm_clk_add_clks() here will start managing all clocks of the bus, >> which may not be wanted on all platforms. > > It would not be strange to do so in the "simple" implementation for the > bus, allowing custom behavior to be implemented in a more specific > driver for a platform with custom needs. Doing that means every platform that doesn't want all clocks to be used for PM need to add custom drivers, while we already handle this in genpd. Moreover, the same functionality (some clocks are used for PM and/or the device may be part of a power domain) is needed for non-bus devices, and thus handled by genpd. BTW, actually bus drivers are the case that's currently handled special: I'd rather seen the pm_runtime_*() handling being added to plain simple-bus. For non-bus drivers, we just add the calls to any driver that may be used on platforms with clock and/or power domains, but for bus drivers, people wanted a separate driver (with its own DT binding). Sigh... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c index c5eb46c..63b7e8c 100644 --- a/drivers/bus/simple-pm-bus.c +++ b/drivers/bus/simple-pm-bus.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/pm_clock.h> #include <linux/pm_runtime.h> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev) pm_runtime_enable(&pdev->dev); - if (np) + if (np) { + of_pm_clk_add_clks(&pdev->dev); of_platform_populate(np, NULL, NULL, &pdev->dev); + } return 0; } +static const struct dev_pm_ops simple_pm_bus_pm_ops = { + SET_RUNTIME_PM_OPS(pm_clk_suspend, + pm_clk_resume, NULL) +}; + static int simple_pm_bus_remove(struct platform_device *pdev) { dev_dbg(&pdev->dev, "%s\n", __func__); pm_runtime_disable(&pdev->dev); + pm_clk_destroy(&pdev->dev); + return 0; } @@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = { .driver = { .name = "simple-pm-bus", .of_match_table = simple_pm_bus_of_match, + .pm = &simple_pm_bus_pm_ops, }, };
This patch adds support to pm clocks via device tree, so that the clocks can be turned on and off during runtime pm. This patch is required for Qualcomm msm8996 pcie controller which sits on a bus with its own power-domain and clocks. Without this patch the clock associated with the bus are never turned on. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/bus/simple-pm-bus.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)