| Message ID | 20220804061133.4110734-3-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: bus: Add Freescale i.MX8qxp pixel link MSI bus support | expand |
Hi Liu, On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <victor.liu@nxp.com> wrote: > Simple Power-Managed bus controller may need functional clock(s) > to be enabled before child devices connected to the bus can be > accessed. Get the clock(s) as a bulk and enable/disable the > clock(s) when the bus is being power managed. > > One example is that Freescale i.MX8qxp pixel link MSI bus controller > needs MSI clock and AHB clock to be enabled before accessing child > devices. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> Thanks for your patch! > --- a/drivers/bus/simple-pm-bus.c > +++ b/drivers/bus/simple-pm-bus.c > @@ -8,11 +8,17 @@ > * for more details. > */ > > +#include <linux/clk.h> > #include <linux/module.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > > +struct simple_pm_bus { > + struct clk_bulk_data *clks; > + int num_clks; > +}; > + > static const struct of_device_id simple_pm_bus_child_matches[] = { > { .compatible = "simple-mfd", }, > {} > @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > struct device_node *np = dev->of_node; > const struct of_device_id *match; > + struct simple_pm_bus *bus; > > /* > * Allow user to use driver_override to bind this driver to a > @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > return -ENODEV; > } > > + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL); > + if (!bus) > + return -ENOMEM; > + > + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus->clks); > + if (bus->num_clks < 0) > + return dev_err_probe(&pdev->dev, bus->num_clks, "failed to get clocks\n"); > + > + dev_set_drvdata(&pdev->dev, bus); > + > dev_dbg(&pdev->dev, "%s\n", __func__); > > pm_runtime_enable(&pdev->dev); While I agree this patch has merits on its own[*], I am wondering if you really need it for your use case. In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel link MSI bus binding", I see your bus has both "clocks" and "power-domains" properties. Perhaps your PM Domain can be a clock domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing generic_pm_domain.{at,de}tach_dev() callbacks), thus handing clock handling over to Runtime PM? [*] The simple-pm-bus DT bindings state: clocks: true # Functional clocks # Required if power-domains is absent, optional otherwise power-domains: # Required if clocks is absent, optional otherwise minItems: 1 While "power-domains" (+ "clocks" in case of a clock domain) is handled through Runtime PM, the current driver indeed does not handle "clocks" in the absence of the "power-domains" property. It looks like all existing users that use "clocks" rely on the PM Domain being a clock domain. With your patch, the clocks might be controlled twice: once explicitly, through clk_bulk_*(), and a second time implicitly, through Runtime PM. While this works fine to do clock enable counters, it looks suboptimal to me. This could be avoided by making the new explicit clock code depend on the absence of the "power-domains" property, but that would break users that have both a PM Domain which is not a clock domain, and clocks. So we may have no other option. 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
On Thu, 2022-08-11 at 14:34 +0200, Geert Uytterhoeven wrote: > Hi Liu, Hi Geert, > > On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <victor.liu@nxp.com> wrote: > > Simple Power-Managed bus controller may need functional clock(s) > > to be enabled before child devices connected to the bus can be > > accessed. Get the clock(s) as a bulk and enable/disable the > > clock(s) when the bus is being power managed. > > > > One example is that Freescale i.MX8qxp pixel link MSI bus > > controller > > needs MSI clock and AHB clock to be enabled before accessing child > > devices. > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > Thanks for your patch! Thanks for the review. > > > --- a/drivers/bus/simple-pm-bus.c > > +++ b/drivers/bus/simple-pm-bus.c > > @@ -8,11 +8,17 @@ > > * for more details. > > */ > > > > +#include <linux/clk.h> > > #include <linux/module.h> > > #include <linux/of_platform.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > > > +struct simple_pm_bus { > > + struct clk_bulk_data *clks; > > + int num_clks; > > +}; > > + > > static const struct of_device_id simple_pm_bus_child_matches[] = { > > { .compatible = "simple-mfd", }, > > {} > > @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct > > platform_device *pdev) > > const struct of_dev_auxdata *lookup = > > dev_get_platdata(dev); > > struct device_node *np = dev->of_node; > > const struct of_device_id *match; > > + struct simple_pm_bus *bus; > > > > /* > > * Allow user to use driver_override to bind this driver to > > a > > @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct > > platform_device *pdev) > > return -ENODEV; > > } > > > > + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL); > > + if (!bus) > > + return -ENOMEM; > > + > > + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus- > > > clks); > > > > + if (bus->num_clks < 0) > > + return dev_err_probe(&pdev->dev, bus->num_clks, > > "failed to get clocks\n"); > > + > > + dev_set_drvdata(&pdev->dev, bus); > > + > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > pm_runtime_enable(&pdev->dev); > > While I agree this patch has merits on its own[*], I am wondering > if you really need it for your use case. > > In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel > link MSI bus binding", I see your bus has both "clocks" and > "power-domains" properties. Perhaps your PM Domain can be a clock > domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing > generic_pm_domain.{at,de}tach_dev() callbacks), thus handing clock > handling over to Runtime PM? It looks like most(if not all) PM domains can be clock domains with GENPD_FLAG_PM_CLK and generic_pm_domain.{at,de}tach_dev() callbacks set. So, technically, my PM domain(scu-pd.c) can be a clock domain with all those set and a special check for "simple-pm-bus" in the {at,de}tach_dev callbacks. But, I'm not sure if it is appropriate to do that. How do we determine clocks should be managed by PM domains or device drivers? Technically, both would work... > > [*] The simple-pm-bus DT bindings state: > > clocks: true > # Functional clocks > # Required if power-domains is absent, optional otherwise > > power-domains: > # Required if clocks is absent, optional otherwise > minItems: 1 > > While "power-domains" (+ "clocks" in case of a clock domain) is > handled through Runtime PM, the current driver indeed does not handle > "clocks" in the absence of the "power-domains" property. It looks Right. > like all existing users that use "clocks" rely on the PM Domain being > a clock domain. "renesas,bsc" seems to be one of the users. > > With your patch, the clocks might be controlled twice: once > explicitly, > through clk_bulk_*(), and a second time implicitly, through Runtime > PM. > While this works fine to do clock enable counters, it looks > suboptimal > to me. This could be avoided by making the new explicit clock code > depend on the absence of the "power-domains" property, but that would > break users that have both a PM Domain which is not a clock domain, > and clocks. So we may have no other option. Hmm, I'm not sure if there are really users that have both a PM domain which is not a clock domain and clocks, given that a PM domain can sort of always be a clock domain by setting that GENPD flag and those callbacks. So, what do you suggest? Keep the patch as-is? Or, maybe, make my PM domain additionally be a clock domain? Regards, Liu Ying
Hi Liu, On Fri, Aug 12, 2022 at 10:13 AM Liu Ying <victor.liu@nxp.com> wrote: > On Thu, 2022-08-11 at 14:34 +0200, Geert Uytterhoeven wrote: > > On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <victor.liu@nxp.com> wrote: > > > Simple Power-Managed bus controller may need functional clock(s) > > > to be enabled before child devices connected to the bus can be > > > accessed. Get the clock(s) as a bulk and enable/disable the > > > clock(s) when the bus is being power managed. > > > > > > One example is that Freescale i.MX8qxp pixel link MSI bus > > > controller > > > needs MSI clock and AHB clock to be enabled before accessing child > > > devices. > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > Thanks for your patch! > > Thanks for the review. > > > > > > --- a/drivers/bus/simple-pm-bus.c > > > +++ b/drivers/bus/simple-pm-bus.c > > > @@ -8,11 +8,17 @@ > > > * for more details. > > > */ > > > > > > +#include <linux/clk.h> > > > #include <linux/module.h> > > > #include <linux/of_platform.h> > > > #include <linux/platform_device.h> > > > #include <linux/pm_runtime.h> > > > > > > +struct simple_pm_bus { > > > + struct clk_bulk_data *clks; > > > + int num_clks; > > > +}; > > > + > > > static const struct of_device_id simple_pm_bus_child_matches[] = { > > > { .compatible = "simple-mfd", }, > > > {} > > > @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct > > > platform_device *pdev) > > > const struct of_dev_auxdata *lookup = > > > dev_get_platdata(dev); > > > struct device_node *np = dev->of_node; > > > const struct of_device_id *match; > > > + struct simple_pm_bus *bus; > > > > > > /* > > > * Allow user to use driver_override to bind this driver to > > > a > > > @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct > > > platform_device *pdev) > > > return -ENODEV; > > > } > > > > > > + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL); > > > + if (!bus) > > > + return -ENOMEM; > > > + > > > + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus- > > > > clks); > > > > > > + if (bus->num_clks < 0) > > > + return dev_err_probe(&pdev->dev, bus->num_clks, > > > "failed to get clocks\n"); > > > + > > > + dev_set_drvdata(&pdev->dev, bus); > > > + > > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > > > pm_runtime_enable(&pdev->dev); > > > > While I agree this patch has merits on its own[*], I am wondering > > if you really need it for your use case. > > > > In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel > > link MSI bus binding", I see your bus has both "clocks" and > > "power-domains" properties. Perhaps your PM Domain can be a clock > > domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing > > generic_pm_domain.{at,de}tach_dev() callbacks), thus handing clock > > handling over to Runtime PM? > > It looks like most(if not all) PM domains can be clock domains with > GENPD_FLAG_PM_CLK and generic_pm_domain.{at,de}tach_dev() callbacks > set. So, technically, my PM domain(scu-pd.c) can be a clock domain with > all those set and a special check for "simple-pm-bus" in the > {at,de}tach_dev callbacks. But, I'm not sure if it is appropriate to > do that. How do we determine clocks should be managed by PM domains or > device drivers? Technically, both would work... It depends on the hardware topology: is there really a clock domain (i.e. lots of consumer modules are clocked by a single clock controller and can be power-managed that way), or is it just a coincidence that your bus has clocks. E.g. drivers/clk/renesas/renesas-cpg-mssr.c:cpg_mssr_attach_dev() looks for clocks from the right clock provider and of the right type. > > [*] The simple-pm-bus DT bindings state: > > > > clocks: true > > # Functional clocks > > # Required if power-domains is absent, optional otherwise > > > > power-domains: > > # Required if clocks is absent, optional otherwise > > minItems: 1 > > > > While "power-domains" (+ "clocks" in case of a clock domain) is > > handled through Runtime PM, the current driver indeed does not handle > > "clocks" in the absence of the "power-domains" property. It looks > > Right. > > > like all existing users that use "clocks" rely on the PM Domain being > > a clock domain. > > "renesas,bsc" seems to be one of the users. Yes it is. And it doesn't need a special driver, as it just relies on Runtime PM controlling both the power area and the clocks through the PM Domain. > > With your patch, the clocks might be controlled twice: once > > explicitly, > > through clk_bulk_*(), and a second time implicitly, through Runtime > > PM. > > While this works fine to do clock enable counters, it looks > > suboptimal > > to me. This could be avoided by making the new explicit clock code > > depend on the absence of the "power-domains" property, but that would > > break users that have both a PM Domain which is not a clock domain, > > and clocks. So we may have no other option. > > Hmm, I'm not sure if there are really users that have both a PM domain > which is not a clock domain and clocks, given that a PM domain can sort > of always be a clock domain by setting that GENPD flag and those > callbacks. So, what do you suggest? Keep the patch as-is? Or, maybe, > make my PM domain additionally be a clock domain? It depends. Is "dc0_disp_ctrl_link_mst0_lpcg" a clock controller that controls the clock inputs to multiple modules? Based on the name, it seems to be used only for display-related clocks, while "pd" controls power to various modules, not limited to display? Hence you may want to keep your patch as-is. Renesas R-Car SoCs have separate power area and clock controllers, too, but they apply to most/all devices. Hence we moduled this as a single PM Domain (also because "power-domains" (used to) support only a single provider), through a close integration of the power area and clock drivers. 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
Hi Geert, On Fri, 2022-08-12 at 10:58 +0200, Geert Uytterhoeven wrote: > On Fri, Aug 12, 2022 at 10:13 AM Liu Ying <victor.liu@nxp.com> wrote: > > On Thu, 2022-08-11 at 14:34 +0200, Geert Uytterhoeven wrote: > > > On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <victor.liu@nxp.com> > > > wrote: > > > > Simple Power-Managed bus controller may need functional > > > > clock(s) > > > > to be enabled before child devices connected to the bus can be > > > > accessed. Get the clock(s) as a bulk and enable/disable the > > > > clock(s) when the bus is being power managed. > > > > > > > > One example is that Freescale i.MX8qxp pixel link MSI bus > > > > controller > > > > needs MSI clock and AHB clock to be enabled before accessing > > > > child > > > > devices. > > > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > > > Thanks for your patch! > > > > Thanks for the review. > > > > > > > > > --- a/drivers/bus/simple-pm-bus.c > > > > +++ b/drivers/bus/simple-pm-bus.c > > > > @@ -8,11 +8,17 @@ > > > > * for more details. > > > > */ > > > > > > > > +#include <linux/clk.h> > > > > #include <linux/module.h> > > > > #include <linux/of_platform.h> > > > > #include <linux/platform_device.h> > > > > #include <linux/pm_runtime.h> > > > > > > > > +struct simple_pm_bus { > > > > + struct clk_bulk_data *clks; > > > > + int num_clks; > > > > +}; > > > > + > > > > static const struct of_device_id simple_pm_bus_child_matches[] > > > > = { > > > > { .compatible = "simple-mfd", }, > > > > {} > > > > @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct > > > > platform_device *pdev) > > > > const struct of_dev_auxdata *lookup = > > > > dev_get_platdata(dev); > > > > struct device_node *np = dev->of_node; > > > > const struct of_device_id *match; > > > > + struct simple_pm_bus *bus; > > > > > > > > /* > > > > * Allow user to use driver_override to bind this > > > > driver to > > > > a > > > > @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct > > > > platform_device *pdev) > > > > return -ENODEV; > > > > } > > > > > > > > + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), > > > > GFP_KERNEL); > > > > + if (!bus) > > > > + return -ENOMEM; > > > > + > > > > + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus- > > > > > clks); > > > > > > > > + if (bus->num_clks < 0) > > > > + return dev_err_probe(&pdev->dev, bus->num_clks, > > > > "failed to get clocks\n"); > > > > + > > > > + dev_set_drvdata(&pdev->dev, bus); > > > > + > > > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > > > > > pm_runtime_enable(&pdev->dev); > > > > > > While I agree this patch has merits on its own[*], I am wondering > > > if you really need it for your use case. > > > > > > In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel > > > link MSI bus binding", I see your bus has both "clocks" and > > > "power-domains" properties. Perhaps your PM Domain can be a > > > clock > > > domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing > > > generic_pm_domain.{at,de}tach_dev() callbacks), thus handing > > > clock > > > handling over to Runtime PM? > > > > It looks like most(if not all) PM domains can be clock domains with > > GENPD_FLAG_PM_CLK and generic_pm_domain.{at,de}tach_dev() callbacks > > set. So, technically, my PM domain(scu-pd.c) can be a clock domain > > with > > all those set and a special check for "simple-pm-bus" in the > > {at,de}tach_dev callbacks. But, I'm not sure if it is appropriate > > to > > do that. How do we determine clocks should be managed by PM domains > > or > > device drivers? Technically, both would work... > > It depends on the hardware topology: is there really a clock domain > (i.e. lots of consumer modules are clocked by a single clock > controller and can be power-managed that way), or is it just a > coincidence that your bus has clocks. It's just a coincidence that my bus has the two clocks. > > E.g. drivers/clk/renesas/renesas-cpg-mssr.c:cpg_mssr_attach_dev() > looks for clocks from the right clock provider and of the right type. Ok, I see the function. > > > > [*] The simple-pm-bus DT bindings state: > > > > > > clocks: true > > > # Functional clocks > > > # Required if power-domains is absent, optional otherwise > > > > > > power-domains: > > > # Required if clocks is absent, optional otherwise > > > minItems: 1 > > > > > > While "power-domains" (+ "clocks" in case of a clock domain) is > > > handled through Runtime PM, the current driver indeed does not > > > handle > > > "clocks" in the absence of the "power-domains" property. It > > > looks > > > > Right. > > > > > like all existing users that use "clocks" rely on the PM Domain > > > being > > > a clock domain. > > > > "renesas,bsc" seems to be one of the users. > > Yes it is. And it doesn't need a special driver, as it just relies > on Runtime PM controlling both the power area and the clocks through > the PM Domain. Yes, I see. > > > > With your patch, the clocks might be controlled twice: once > > > explicitly, > > > through clk_bulk_*(), and a second time implicitly, through > > > Runtime > > > PM. > > > While this works fine to do clock enable counters, it looks > > > suboptimal > > > to me. This could be avoided by making the new explicit clock > > > code > > > depend on the absence of the "power-domains" property, but that > > > would > > > break users that have both a PM Domain which is not a clock > > > domain, > > > and clocks. So we may have no other option. > > > > Hmm, I'm not sure if there are really users that have both a PM > > domain > > which is not a clock domain and clocks, given that a PM domain can > > sort > > of always be a clock domain by setting that GENPD flag and those > > callbacks. So, what do you suggest? Keep the patch as-is? Or, > > maybe, > > make my PM domain additionally be a clock domain? > > It depends. Is "dc0_disp_ctrl_link_mst0_lpcg" a clock controller that > controls the clock inputs to multiple modules? Based on the name, it > seems to be used only for display-related clocks, while "pd" controls > power to various modules, not limited to display? > Hence you may want to keep your patch as-is. "dc0_disp_ctrl_link_mst0_lpcg" only controls the clock inputs to my bus, not multiple modules. "<&pd IMX_SC_R_DC_0>" controls power to all modules in Display Controller Subsystem, including display controller engines, irq controller, clock controllers and this MSI bus. Hmm, based on your information, it looks like I need to keep the patch as-is, as the clocks are only for the bus. If that's the case, may I add your 'Reviewed-by' tag or sth like that on this patch that when I send v4? > > Renesas R-Car SoCs have separate power area and clock controllers, > too, but they apply to most/all devices. Hence we moduled this as > a single PM Domain (also because "power-domains" (used to) support > only a single provider), through a close integration of the power > area and clock drivers. Thanks for the information. Liu Ying
Hi Liu, On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <victor.liu@nxp.com> wrote: > Simple Power-Managed bus controller may need functional clock(s) > to be enabled before child devices connected to the bus can be > accessed. Get the clock(s) as a bulk and enable/disable the > clock(s) when the bus is being power managed. > > One example is that Freescale i.MX8qxp pixel link MSI bus controller > needs MSI clock and AHB clock to be enabled before accessing child > devices. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v1->v3: > * No change. Thanks for your patch! > --- a/drivers/bus/simple-pm-bus.c > +++ b/drivers/bus/simple-pm-bus.c > @@ -72,6 +89,42 @@ static int simple_pm_bus_remove(struct platform_device *pdev) > return 0; > } > > +static int simple_pm_bus_runtime_suspend(struct device *dev) > +{ > + struct simple_pm_bus *bus = dev_get_drvdata(dev); > + > + if (!bus) > + return 0; Can this really happen? > + > + clk_bulk_disable_unprepare(bus->num_clks, bus->clks); > + > + return 0; > +} > + > +static int simple_pm_bus_runtime_resume(struct device *dev) > +{ > + struct simple_pm_bus *bus = dev_get_drvdata(dev); > + int ret; > + > + if (!bus) > + return 0; Likewise. > + > + ret = clk_bulk_prepare_enable(bus->num_clks, bus->clks); > + if (ret) { > + dev_err(dev, "failed to enable clocks: %d\n", ret); > + return ret; > + } > + > + return 0; > +} The rest LGTM, so Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> 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
Hi Geert, On Fri, 2022-08-12 at 11:49 +0200, Geert Uytterhoeven wrote: > On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <victor.liu@nxp.com> wrote: > > Simple Power-Managed bus controller may need functional clock(s) > > to be enabled before child devices connected to the bus can be > > accessed. Get the clock(s) as a bulk and enable/disable the > > clock(s) when the bus is being power managed. > > > > One example is that Freescale i.MX8qxp pixel link MSI bus > > controller > > needs MSI clock and AHB clock to be enabled before accessing child > > devices. > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > v1->v3: > > * No change. > > Thanks for your patch! > > > --- a/drivers/bus/simple-pm-bus.c > > +++ b/drivers/bus/simple-pm-bus.c > > @@ -72,6 +89,42 @@ static int simple_pm_bus_remove(struct > > platform_device *pdev) > > return 0; > > } > > > > +static int simple_pm_bus_runtime_suspend(struct device *dev) > > +{ > > + struct simple_pm_bus *bus = dev_get_drvdata(dev); > > + > > + if (!bus) > > + return 0; > > Can this really happen? Good question. Should not happen in any cases. pm_runtime_init() sets runtime_status to RPM_SUSPENDED and needs_force_resume to 0, so simple_pm_bus_runtime_{suspend,resume} are only called for simple-pm-bus, not the others. Unless I miss something, I would remove the check here and ... > > > + > > + clk_bulk_disable_unprepare(bus->num_clks, bus->clks); > > + > > + return 0; > > +} > > + > > +static int simple_pm_bus_runtime_resume(struct device *dev) > > +{ > > + struct simple_pm_bus *bus = dev_get_drvdata(dev); > > + int ret; > > + > > + if (!bus) > > + return 0; > > Likewise. ... here. > > > + > > + ret = clk_bulk_prepare_enable(bus->num_clks, bus->clks); > > + if (ret) { > > + dev_err(dev, "failed to enable clocks: %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > The rest LGTM, so > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks, Liu Ying
diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c index ff5f8ca5c024..876a906724b3 100644 --- a/drivers/bus/simple-pm-bus.c +++ b/drivers/bus/simple-pm-bus.c @@ -8,11 +8,17 @@ * for more details. */ +#include <linux/clk.h> #include <linux/module.h> #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +struct simple_pm_bus { + struct clk_bulk_data *clks; + int num_clks; +}; + static const struct of_device_id simple_pm_bus_child_matches[] = { { .compatible = "simple-mfd", }, {} @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct platform_device *pdev) const struct of_dev_auxdata *lookup = dev_get_platdata(dev); struct device_node *np = dev->of_node; const struct of_device_id *match; + struct simple_pm_bus *bus; /* * Allow user to use driver_override to bind this driver to a @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct platform_device *pdev) return -ENODEV; } + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL); + if (!bus) + return -ENOMEM; + + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus->clks); + if (bus->num_clks < 0) + return dev_err_probe(&pdev->dev, bus->num_clks, "failed to get clocks\n"); + + dev_set_drvdata(&pdev->dev, bus); + dev_dbg(&pdev->dev, "%s\n", __func__); pm_runtime_enable(&pdev->dev); @@ -72,6 +89,42 @@ static int simple_pm_bus_remove(struct platform_device *pdev) return 0; } +static int simple_pm_bus_runtime_suspend(struct device *dev) +{ + struct simple_pm_bus *bus = dev_get_drvdata(dev); + + if (!bus) + return 0; + + clk_bulk_disable_unprepare(bus->num_clks, bus->clks); + + return 0; +} + +static int simple_pm_bus_runtime_resume(struct device *dev) +{ + struct simple_pm_bus *bus = dev_get_drvdata(dev); + int ret; + + if (!bus) + return 0; + + ret = clk_bulk_prepare_enable(bus->num_clks, bus->clks); + if (ret) { + dev_err(dev, "failed to enable clocks: %d\n", ret); + return ret; + } + + return 0; +} + +static const struct dev_pm_ops simple_pm_bus_pm_ops = { + SET_RUNTIME_PM_OPS(simple_pm_bus_runtime_suspend, + simple_pm_bus_runtime_resume, NULL) + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) +}; + #define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ static const struct of_device_id simple_pm_bus_of_match[] = { @@ -90,6 +143,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, }, };
Simple Power-Managed bus controller may need functional clock(s) to be enabled before child devices connected to the bus can be accessed. Get the clock(s) as a bulk and enable/disable the clock(s) when the bus is being power managed. One example is that Freescale i.MX8qxp pixel link MSI bus controller needs MSI clock and AHB clock to be enabled before accessing child devices. Signed-off-by: Liu Ying <victor.liu@nxp.com> --- v1->v3: * No change. drivers/bus/simple-pm-bus.c | 54 +++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)