From: Janne Grunau <[email protected]>
Multiple power domains need to be handled explicitly in each driver. The
driver core can not handle it automatically since it is not aware of
power sequencing requirements the hardware might have. This is not a
problem for simpledrm since everything is expected to be powered on by
the bootloader. simpledrm has just ensure it remains powered on during
its lifetime.
This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
systems. The HDMI output initialized by the bootloader requires keeping
the display controller and a DP phy power domain on.
Signed-off-by: Janne Grunau <[email protected]>
---
Changes in v2:
- removed broken drm_err() log statement only ment for debugging
- removed commented cast
- use correct format spcifier for 'int' in log statement
- add 'continue;' after failure to get device for power_domain
- use drm_warn() in non fatal error cases
- removed duplicate PTR_ERR conversion
- Link to v1: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685@jannau.net
---
drivers/gpu/drm/tiny/simpledrm.c | 105 +++++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index ff86ba1ae1b8..9c597461d1e2 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -6,6 +6,7 @@
#include <linux/of_address.h>
#include <linux/platform_data/simplefb.h>
#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
#include <linux/regulator/consumer.h>
#include <drm/drm_aperture.h>
@@ -227,6 +228,12 @@ struct simpledrm_device {
unsigned int regulator_count;
struct regulator **regulators;
#endif
+ /* power-domains */
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+ int pwr_dom_count;
+ struct device **pwr_dom_devs;
+ struct device_link **pwr_dom_links;
+#endif
/* simplefb settings */
struct drm_display_mode mode;
@@ -468,6 +475,101 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
}
#endif
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+/*
+ * Generic power domain handling code.
+ *
+ * Here we handle the power-domains properties of our "simple-framebuffer"
+ * dt node. This is only necessary if there is more than one power-domain.
+ * A single power-domains is handled automatically by the driver core. Multiple
+ * power-domains have to be handled by drivers since the driver core can't know
+ * the correct power sequencing. Power sequencing is not an issue for simpledrm
+ * since the bootloader has put the power domains already in the correct state.
+ * simpledrm has only to ensure they remain active for its lifetime.
+ *
+ * When the driver unloads, we detach from the power-domains.
+ *
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up the "simple-framebuffer" dt node, and the PM domain providers in the
+ * device tree. Chances are that there are no adverse effects, and if there are,
+ * a clean teardown of the fb probe will not help us much either. So just
+ * complain and carry on, and hope that the user actually gets a working fb at
+ * the end of things.
+ */
+static void simpledrm_device_detach_genpd(void *res)
+{
+ int i;
+ struct simpledrm_device *sdev = res;
+
+ if (sdev->pwr_dom_count <= 1)
+ return;
+
+ for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
+ if (!sdev->pwr_dom_links[i])
+ device_link_del(sdev->pwr_dom_links[i]);
+ if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
+ dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
+ }
+}
+
+static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
+{
+ struct device *dev = sdev->dev.dev;
+ int i;
+
+ sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
+ "#power-domain-cells");
+ /*
+ * Single power-domain devices are handled by driver core nothing to do
+ * here. The same for device nodes without "power-domains" property.
+ */
+ if (sdev->pwr_dom_count <= 1)
+ return 0;
+
+ sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
+ sizeof(*sdev->pwr_dom_devs),
+ GFP_KERNEL);
+ if (!sdev->pwr_dom_devs)
+ return -ENOMEM;
+
+ sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
+ sizeof(*sdev->pwr_dom_links),
+ GFP_KERNEL);
+ if (!sdev->pwr_dom_links)
+ return -ENOMEM;
+
+ for (i = 0; i < sdev->pwr_dom_count; i++) {
+ sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
+ if (IS_ERR(sdev->pwr_dom_devs[i])) {
+ int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
+ if (ret == -EPROBE_DEFER) {
+ simpledrm_device_detach_genpd(sdev);
+ return ret;
+ }
+ drm_warn(&sdev->dev,
+ "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
+ continue;
+ }
+
+ sdev->pwr_dom_links[i] = device_link_add(dev,
+ sdev->pwr_dom_devs[i],
+ DL_FLAG_STATELESS |
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_RPM_ACTIVE);
+ if (!sdev->pwr_dom_links[i])
+ drm_warn(&sdev->dev, "failed to link power-domain %d\n", i);
+ }
+
+ return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
+}
+#else
+static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
+{
+ return 0;
+}
+#endif
+
/*
* Modesetting
*/
@@ -651,6 +753,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
if (ret)
return ERR_PTR(ret);
ret = simpledrm_device_init_regulators(sdev);
+ if (ret)
+ return ERR_PTR(ret);
+ ret = simpledrm_device_attach_genpd(sdev);
if (ret)
return ERR_PTR(ret);
---
base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
Best regards,
--
Janne Grunau <[email protected]>
On Tue, 12 Sept 2023 at 21:30, Janne Grunau via B4 Relay
<[email protected]> wrote:
>
> From: Janne Grunau <[email protected]>
>
> Multiple power domains need to be handled explicitly in each driver. The
> driver core can not handle it automatically since it is not aware of
> power sequencing requirements the hardware might have. This is not a
> problem for simpledrm since everything is expected to be powered on by
> the bootloader. simpledrm has just ensure it remains powered on during
> its lifetime.
> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> systems. The HDMI output initialized by the bootloader requires keeping
> the display controller and a DP phy power domain on.
>
> Signed-off-by: Janne Grunau <[email protected]>
Reviewed-by: Eric Curtin <[email protected]>
Is mise le meas/Regards,
Eric Curtin
> ---
> Changes in v2:
> - removed broken drm_err() log statement only ment for debugging
> - removed commented cast
> - use correct format spcifier for 'int' in log statement
> - add 'continue;' after failure to get device for power_domain
> - use drm_warn() in non fatal error cases
> - removed duplicate PTR_ERR conversion
> - Link to v1: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685@jannau.net
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 105 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index ff86ba1ae1b8..9c597461d1e2 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -6,6 +6,7 @@
> #include <linux/of_address.h>
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
>
> #include <drm/drm_aperture.h>
> @@ -227,6 +228,12 @@ struct simpledrm_device {
> unsigned int regulator_count;
> struct regulator **regulators;
> #endif
> + /* power-domains */
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + int pwr_dom_count;
> + struct device **pwr_dom_devs;
> + struct device_link **pwr_dom_links;
> +#endif
>
> /* simplefb settings */
> struct drm_display_mode mode;
> @@ -468,6 +475,101 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +/*
> + * Generic power domain handling code.
> + *
> + * Here we handle the power-domains properties of our "simple-framebuffer"
> + * dt node. This is only necessary if there is more than one power-domain.
> + * A single power-domains is handled automatically by the driver core. Multiple
> + * power-domains have to be handled by drivers since the driver core can't know
> + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> + * since the bootloader has put the power domains already in the correct state.
> + * simpledrm has only to ensure they remain active for its lifetime.
> + *
> + * When the driver unloads, we detach from the power-domains.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> + * device tree. Chances are that there are no adverse effects, and if there are,
> + * a clean teardown of the fb probe will not help us much either. So just
> + * complain and carry on, and hope that the user actually gets a working fb at
> + * the end of things.
> + */
> +static void simpledrm_device_detach_genpd(void *res)
> +{
> + int i;
> + struct simpledrm_device *sdev = res;
> +
> + if (sdev->pwr_dom_count <= 1)
> + return;
> +
> + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> + if (!sdev->pwr_dom_links[i])
> + device_link_del(sdev->pwr_dom_links[i]);
> + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> + }
> +}
> +
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->dev.dev;
> + int i;
> +
> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by driver core nothing to do
> + * here. The same for device nodes without "power-domains" property.
> + */
> + if (sdev->pwr_dom_count <= 1)
> + return 0;
> +
> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_devs),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_devs)
> + return -ENOMEM;
> +
> + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_links),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_links)
> + return -ENOMEM;
> +
> + for (i = 0; i < sdev->pwr_dom_count; i++) {
> + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> + if (ret == -EPROBE_DEFER) {
> + simpledrm_device_detach_genpd(sdev);
> + return ret;
> + }
> + drm_warn(&sdev->dev,
> + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> + continue;
> + }
> +
> + sdev->pwr_dom_links[i] = device_link_add(dev,
> + sdev->pwr_dom_devs[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!sdev->pwr_dom_links[i])
> + drm_warn(&sdev->dev, "failed to link power-domain %d\n", i);
> + }
> +
> + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
> +}
> +#else
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * Modesetting
> */
> @@ -651,6 +753,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> if (ret)
> return ERR_PTR(ret);
> ret = simpledrm_device_init_regulators(sdev);
> + if (ret)
> + return ERR_PTR(ret);
> + ret = simpledrm_device_attach_genpd(sdev);
> if (ret)
> return ERR_PTR(ret);
>
>
> ---
> base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
> change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
>
> Best regards,
> --
> Janne Grunau <[email protected]>
>
>
On Tue, Sep 12, 2023 at 4:22 PM Janne Grunau via B4 Relay
<[email protected]> wrote:
>
> From: Janne Grunau <[email protected]>
>
> Multiple power domains need to be handled explicitly in each driver. The
> driver core can not handle it automatically since it is not aware of
> power sequencing requirements the hardware might have. This is not a
> problem for simpledrm since everything is expected to be powered on by
> the bootloader. simpledrm has just ensure it remains powered on during
> its lifetime.
> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> systems. The HDMI output initialized by the bootloader requires keeping
> the display controller and a DP phy power domain on.
>
> Signed-off-by: Janne Grunau <[email protected]>
> ---
> Changes in v2:
> - removed broken drm_err() log statement only ment for debugging
> - removed commented cast
> - use correct format spcifier for 'int' in log statement
> - add 'continue;' after failure to get device for power_domain
> - use drm_warn() in non fatal error cases
> - removed duplicate PTR_ERR conversion
> - Link to v1: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685@jannau.net
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 105 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index ff86ba1ae1b8..9c597461d1e2 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -6,6 +6,7 @@
> #include <linux/of_address.h>
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
>
> #include <drm/drm_aperture.h>
> @@ -227,6 +228,12 @@ struct simpledrm_device {
> unsigned int regulator_count;
> struct regulator **regulators;
> #endif
> + /* power-domains */
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + int pwr_dom_count;
> + struct device **pwr_dom_devs;
> + struct device_link **pwr_dom_links;
> +#endif
>
> /* simplefb settings */
> struct drm_display_mode mode;
> @@ -468,6 +475,101 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +/*
> + * Generic power domain handling code.
> + *
> + * Here we handle the power-domains properties of our "simple-framebuffer"
> + * dt node. This is only necessary if there is more than one power-domain.
> + * A single power-domains is handled automatically by the driver core. Multiple
> + * power-domains have to be handled by drivers since the driver core can't know
> + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> + * since the bootloader has put the power domains already in the correct state.
> + * simpledrm has only to ensure they remain active for its lifetime.
> + *
> + * When the driver unloads, we detach from the power-domains.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> + * device tree. Chances are that there are no adverse effects, and if there are,
> + * a clean teardown of the fb probe will not help us much either. So just
> + * complain and carry on, and hope that the user actually gets a working fb at
> + * the end of things.
> + */
> +static void simpledrm_device_detach_genpd(void *res)
> +{
> + int i;
> + struct simpledrm_device *sdev = res;
> +
> + if (sdev->pwr_dom_count <= 1)
> + return;
> +
> + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> + if (!sdev->pwr_dom_links[i])
> + device_link_del(sdev->pwr_dom_links[i]);
> + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> + }
> +}
> +
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->dev.dev;
> + int i;
> +
> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by driver core nothing to do
> + * here. The same for device nodes without "power-domains" property.
> + */
> + if (sdev->pwr_dom_count <= 1)
> + return 0;
> +
> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_devs),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_devs)
> + return -ENOMEM;
> +
> + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_links),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_links)
> + return -ENOMEM;
> +
> + for (i = 0; i < sdev->pwr_dom_count; i++) {
> + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> + if (ret == -EPROBE_DEFER) {
> + simpledrm_device_detach_genpd(sdev);
> + return ret;
> + }
> + drm_warn(&sdev->dev,
> + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> + continue;
> + }
> +
> + sdev->pwr_dom_links[i] = device_link_add(dev,
> + sdev->pwr_dom_devs[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!sdev->pwr_dom_links[i])
> + drm_warn(&sdev->dev, "failed to link power-domain %d\n", i);
> + }
> +
> + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
> +}
> +#else
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * Modesetting
> */
> @@ -651,6 +753,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> if (ret)
> return ERR_PTR(ret);
> ret = simpledrm_device_init_regulators(sdev);
> + if (ret)
> + return ERR_PTR(ret);
> + ret = simpledrm_device_attach_genpd(sdev);
> if (ret)
> return ERR_PTR(ret);
>
>
> ---
> base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
> change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
>
> Best regards,
> --
> Janne Grunau <[email protected]>
>
>
Reviewed-by: Neal Gompa <[email protected]>
--
真実はいつも一つ！/ Always, there's only one truth!
Hi
Am 12.09.23 um 22:22 schrieb Janne Grunau via B4 Relay:
> From: Janne Grunau <[email protected]>
>
> Multiple power domains need to be handled explicitly in each driver. The
> driver core can not handle it automatically since it is not aware of
> power sequencing requirements the hardware might have. This is not a
> problem for simpledrm since everything is expected to be powered on by
> the bootloader. simpledrm has just ensure it remains powered on during
> its lifetime.
> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> systems. The HDMI output initialized by the bootloader requires keeping
> the display controller and a DP phy power domain on.
>
> Signed-off-by: Janne Grunau <[email protected]>
As a simpledrm patch:
Reviewed-by: Thomas Zimmermann <[email protected]>
Do you want to wait for another review from someone with
power-management expertise?
Do we need a similar patch for ofdrm?
Best regards
Thomas
> ---
> Changes in v2:
> - removed broken drm_err() log statement only ment for debugging
> - removed commented cast
> - use correct format spcifier for 'int' in log statement
> - add 'continue;' after failure to get device for power_domain
> - use drm_warn() in non fatal error cases
> - removed duplicate PTR_ERR conversion
> - Link to v1: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685@jannau.net
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 105 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index ff86ba1ae1b8..9c597461d1e2 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -6,6 +6,7 @@
> #include <linux/of_address.h>
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
>
> #include <drm/drm_aperture.h>
> @@ -227,6 +228,12 @@ struct simpledrm_device {
> unsigned int regulator_count;
> struct regulator **regulators;
> #endif
> + /* power-domains */
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + int pwr_dom_count;
> + struct device **pwr_dom_devs;
> + struct device_link **pwr_dom_links;
> +#endif
>
> /* simplefb settings */
> struct drm_display_mode mode;
> @@ -468,6 +475,101 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +/*
> + * Generic power domain handling code.
> + *
> + * Here we handle the power-domains properties of our "simple-framebuffer"
> + * dt node. This is only necessary if there is more than one power-domain.
> + * A single power-domains is handled automatically by the driver core. Multiple
> + * power-domains have to be handled by drivers since the driver core can't know
> + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> + * since the bootloader has put the power domains already in the correct state.
> + * simpledrm has only to ensure they remain active for its lifetime.
> + *
> + * When the driver unloads, we detach from the power-domains.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> + * device tree. Chances are that there are no adverse effects, and if there are,
> + * a clean teardown of the fb probe will not help us much either. So just
> + * complain and carry on, and hope that the user actually gets a working fb at
> + * the end of things.
> + */
> +static void simpledrm_device_detach_genpd(void *res)
> +{
> + int i;
> + struct simpledrm_device *sdev = res;
> +
> + if (sdev->pwr_dom_count <= 1)
> + return;
> +
> + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> + if (!sdev->pwr_dom_links[i])
> + device_link_del(sdev->pwr_dom_links[i]);
> + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> + }
> +}
> +
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->dev.dev;
> + int i;
> +
> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by driver core nothing to do
> + * here. The same for device nodes without "power-domains" property.
> + */
> + if (sdev->pwr_dom_count <= 1)
> + return 0;
> +
> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_devs),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_devs)
> + return -ENOMEM;
> +
> + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_links),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_links)
> + return -ENOMEM;
> +
> + for (i = 0; i < sdev->pwr_dom_count; i++) {
> + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> + if (ret == -EPROBE_DEFER) {
> + simpledrm_device_detach_genpd(sdev);
> + return ret;
> + }
> + drm_warn(&sdev->dev,
> + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> + continue;
> + }
> +
> + sdev->pwr_dom_links[i] = device_link_add(dev,
> + sdev->pwr_dom_devs[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!sdev->pwr_dom_links[i])
> + drm_warn(&sdev->dev, "failed to link power-domain %d\n", i);
> + }
> +
> + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
> +}
> +#else
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * Modesetting
> */
> @@ -651,6 +753,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> if (ret)
> return ERR_PTR(ret);
> ret = simpledrm_device_init_regulators(sdev);
> + if (ret)
> + return ERR_PTR(ret);
> + ret = simpledrm_device_attach_genpd(sdev);
> if (ret)
> return ERR_PTR(ret);
>
>
> ---
> base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
> change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
>
> Best regards,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
"Sven Peter" <[email protected]> writes:
> Hi,
>
>
> On Mon, Sep 18, 2023, at 09:11, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.09.23 um 22:22 schrieb Janne Grunau via B4 Relay:
>>> From: Janne Grunau <[email protected]>
>>>
>>> Multiple power domains need to be handled explicitly in each driver. The
>>> driver core can not handle it automatically since it is not aware of
>>> power sequencing requirements the hardware might have. This is not a
>>> problem for simpledrm since everything is expected to be powered on by
>>> the bootloader. simpledrm has just ensure it remains powered on during
>>> its lifetime.
>>> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
>>> systems. The HDMI output initialized by the bootloader requires keeping
>>> the display controller and a DP phy power domain on.
>>>
>>> Signed-off-by: Janne Grunau <[email protected]>
>>
>> As a simpledrm patch:
>>
>> Reviewed-by: Thomas Zimmermann <[email protected]>
>>
>> Do you want to wait for another review from someone with
>> power-management expertise?
>
> I can't claim to have a lot of genpd experience but we use very similar
> code in a few other M1/M2 drivers that also require multiple power domains
> to be up without any sequencing constraints. So for whatever it's worth:
>
> Reviewed-by: Sven Peter <[email protected]>
>
Can't claim to be an expert on genpd either but it looks good to me as well.
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi,
On Mon, Sep 18, 2023, at 09:11, Thomas Zimmermann wrote:
> Hi
>
> Am 12.09.23 um 22:22 schrieb Janne Grunau via B4 Relay:
>> From: Janne Grunau <[email protected]>
>>
>> Multiple power domains need to be handled explicitly in each driver. The
>> driver core can not handle it automatically since it is not aware of
>> power sequencing requirements the hardware might have. This is not a
>> problem for simpledrm since everything is expected to be powered on by
>> the bootloader. simpledrm has just ensure it remains powered on during
>> its lifetime.
>> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
>> systems. The HDMI output initialized by the bootloader requires keeping
>> the display controller and a DP phy power domain on.
>>
>> Signed-off-by: Janne Grunau <[email protected]>
>
> As a simpledrm patch:
>
> Reviewed-by: Thomas Zimmermann <[email protected]>
>
> Do you want to wait for another review from someone with
> power-management expertise?
I can't claim to have a lot of genpd experience but we use very similar
code in a few other M1/M2 drivers that also require multiple power domains
to be up without any sequencing constraints. So for whatever it's worth:
Reviewed-by: Sven Peter <[email protected]>
Best,
Sven
added to drm-misc-next
Am 12.09.23 um 22:22 schrieb Janne Grunau via B4 Relay:
> From: Janne Grunau <[email protected]>
>
> Multiple power domains need to be handled explicitly in each driver. The
> driver core can not handle it automatically since it is not aware of
> power sequencing requirements the hardware might have. This is not a
> problem for simpledrm since everything is expected to be powered on by
> the bootloader. simpledrm has just ensure it remains powered on during
> its lifetime.
> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> systems. The HDMI output initialized by the bootloader requires keeping
> the display controller and a DP phy power domain on.
>
> Signed-off-by: Janne Grunau <[email protected]>
> ---
> Changes in v2:
> - removed broken drm_err() log statement only ment for debugging
> - removed commented cast
> - use correct format spcifier for 'int' in log statement
> - add 'continue;' after failure to get device for power_domain
> - use drm_warn() in non fatal error cases
> - removed duplicate PTR_ERR conversion
> - Link to v1: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685@jannau.net
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 105 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index ff86ba1ae1b8..9c597461d1e2 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -6,6 +6,7 @@
> #include <linux/of_address.h>
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
>
> #include <drm/drm_aperture.h>
> @@ -227,6 +228,12 @@ struct simpledrm_device {
> unsigned int regulator_count;
> struct regulator **regulators;
> #endif
> + /* power-domains */
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + int pwr_dom_count;
> + struct device **pwr_dom_devs;
> + struct device_link **pwr_dom_links;
> +#endif
>
> /* simplefb settings */
> struct drm_display_mode mode;
> @@ -468,6 +475,101 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +/*
> + * Generic power domain handling code.
> + *
> + * Here we handle the power-domains properties of our "simple-framebuffer"
> + * dt node. This is only necessary if there is more than one power-domain.
> + * A single power-domains is handled automatically by the driver core. Multiple
> + * power-domains have to be handled by drivers since the driver core can't know
> + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> + * since the bootloader has put the power domains already in the correct state.
> + * simpledrm has only to ensure they remain active for its lifetime.
> + *
> + * When the driver unloads, we detach from the power-domains.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> + * device tree. Chances are that there are no adverse effects, and if there are,
> + * a clean teardown of the fb probe will not help us much either. So just
> + * complain and carry on, and hope that the user actually gets a working fb at
> + * the end of things.
> + */
> +static void simpledrm_device_detach_genpd(void *res)
> +{
> + int i;
> + struct simpledrm_device *sdev = res;
> +
> + if (sdev->pwr_dom_count <= 1)
> + return;
> +
> + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> + if (!sdev->pwr_dom_links[i])
> + device_link_del(sdev->pwr_dom_links[i]);
> + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> + }
> +}
> +
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->dev.dev;
> + int i;
> +
> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by driver core nothing to do
> + * here. The same for device nodes without "power-domains" property.
> + */
> + if (sdev->pwr_dom_count <= 1)
> + return 0;
> +
> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_devs),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_devs)
> + return -ENOMEM;
> +
> + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_links),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_links)
> + return -ENOMEM;
> +
> + for (i = 0; i < sdev->pwr_dom_count; i++) {
> + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> + if (ret == -EPROBE_DEFER) {
> + simpledrm_device_detach_genpd(sdev);
> + return ret;
> + }
> + drm_warn(&sdev->dev,
> + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> + continue;
> + }
> +
> + sdev->pwr_dom_links[i] = device_link_add(dev,
> + sdev->pwr_dom_devs[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!sdev->pwr_dom_links[i])
> + drm_warn(&sdev->dev, "failed to link power-domain %d\n", i);
> + }
> +
> + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
> +}
> +#else
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * Modesetting
> */
> @@ -651,6 +753,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> if (ret)
> return ERR_PTR(ret);
> ret = simpledrm_device_init_regulators(sdev);
> + if (ret)
> + return ERR_PTR(ret);
> + ret = simpledrm_device_attach_genpd(sdev);
> if (ret)
> return ERR_PTR(ret);
>
>
> ---
> base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
> change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
>
> Best regards,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
CC genpd
On Mon, Sep 18, 2023 at 10:24 AM Thomas Zimmermann <[email protected]> wrote:
> Am 12.09.23 um 22:22 schrieb Janne Grunau via B4 Relay:
> > From: Janne Grunau <[email protected]>
> >
> > Multiple power domains need to be handled explicitly in each driver. The
> > driver core can not handle it automatically since it is not aware of
> > power sequencing requirements the hardware might have. This is not a
> > problem for simpledrm since everything is expected to be powered on by
> > the bootloader. simpledrm has just ensure it remains powered on during
> > its lifetime.
> > This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> > systems. The HDMI output initialized by the bootloader requires keeping
> > the display controller and a DP phy power domain on.
> >
> > Signed-off-by: Janne Grunau <[email protected]>
Thanks for your patch, which is now commit 61df9ca231075e70
("drm/simpledrm: Add support for multiple "power-domains"") in
drm-misc/for-linux-next.
> As a simpledrm patch:
>
> Reviewed-by: Thomas Zimmermann <[email protected]>
>
> Do you want to wait for another review from someone with
> power-management expertise?
Yeah, why not? Let's CC them, so they become aware...
> Do we need a similar patch for ofdrm?
>
> Best regards
> Thomas
>
> > ---
> > Changes in v2:
> > - removed broken drm_err() log statement only ment for debugging
> > - removed commented cast
> > - use correct format spcifier for 'int' in log statement
> > - add 'continue;' after failure to get device for power_domain
> > - use drm_warn() in non fatal error cases
> > - removed duplicate PTR_ERR conversion
> > - Link to v1: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685@jannau.net
> > ---
> > drivers/gpu/drm/tiny/simpledrm.c | 105 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 105 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index ff86ba1ae1b8..9c597461d1e2 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -6,6 +6,7 @@
> > #include <linux/of_address.h>
> > #include <linux/platform_data/simplefb.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > #include <linux/regulator/consumer.h>
> >
> > #include <drm/drm_aperture.h>
> > @@ -227,6 +228,12 @@ struct simpledrm_device {
> > unsigned int regulator_count;
> > struct regulator **regulators;
> > #endif
> > + /* power-domains */
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > + int pwr_dom_count;
> > + struct device **pwr_dom_devs;
> > + struct device_link **pwr_dom_links;
> > +#endif
> >
> > /* simplefb settings */
> > struct drm_display_mode mode;
> > @@ -468,6 +475,101 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> > }
> > #endif
> >
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > +/*
> > + * Generic power domain handling code.
> > + *
> > + * Here we handle the power-domains properties of our "simple-framebuffer"
> > + * dt node. This is only necessary if there is more than one power-domain.
> > + * A single power-domains is handled automatically by the driver core. Multiple
> > + * power-domains have to be handled by drivers since the driver core can't know
> > + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> > + * since the bootloader has put the power domains already in the correct state.
> > + * simpledrm has only to ensure they remain active for its lifetime.
> > + *
> > + * When the driver unloads, we detach from the power-domains.
> > + *
> > + * We only complain about errors here, no action is taken as the most likely
> > + * error can only happen due to a mismatch between the bootloader which set
> > + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> > + * device tree. Chances are that there are no adverse effects, and if there are,
> > + * a clean teardown of the fb probe will not help us much either. So just
> > + * complain and carry on, and hope that the user actually gets a working fb at
> > + * the end of things.
> > + */
> > +static void simpledrm_device_detach_genpd(void *res)
> > +{
> > + int i;
> > + struct simpledrm_device *sdev = res;
> > +
> > + if (sdev->pwr_dom_count <= 1)
> > + return;
> > +
> > + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> > + if (!sdev->pwr_dom_links[i])
> > + device_link_del(sdev->pwr_dom_links[i]);
> > + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> > + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> > + }
> > +}
> > +
> > +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> > +{
> > + struct device *dev = sdev->dev.dev;
> > + int i;
> > +
> > + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> > + "#power-domain-cells");
> > + /*
> > + * Single power-domain devices are handled by driver core nothing to do
> > + * here. The same for device nodes without "power-domains" property.
> > + */
> > + if (sdev->pwr_dom_count <= 1)
> > + return 0;
> > +
> > + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> > + sizeof(*sdev->pwr_dom_devs),
> > + GFP_KERNEL);
> > + if (!sdev->pwr_dom_devs)
> > + return -ENOMEM;
> > +
> > + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> > + sizeof(*sdev->pwr_dom_links),
> > + GFP_KERNEL);
> > + if (!sdev->pwr_dom_links)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < sdev->pwr_dom_count; i++) {
> > + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> > + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> > + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> > + if (ret == -EPROBE_DEFER) {
> > + simpledrm_device_detach_genpd(sdev);
> > + return ret;
> > + }
> > + drm_warn(&sdev->dev,
> > + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> > + continue;
> > + }
> > +
> > + sdev->pwr_dom_links[i] = device_link_add(dev,
> > + sdev->pwr_dom_devs[i],
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > + if (!sdev->pwr_dom_links[i])
> > + drm_warn(&sdev->dev, "failed to link power-domain %d\n", i);
> > + }
> > +
> > + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
> > +}
> > +#else
> > +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > /*
> > * Modesetting
> > */
> > @@ -651,6 +753,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > if (ret)
> > return ERR_PTR(ret);
> > ret = simpledrm_device_init_regulators(sdev);
> > + if (ret)
> > + return ERR_PTR(ret);
> > + ret = simpledrm_device_attach_genpd(sdev);
> > if (ret)
> > return ERR_PTR(ret);
> >
> >
> > ---
> > base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
> > change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 Tue, 26 Sept 2023 at 09:32, Geert Uytterhoeven <[email protected]> wrote:
>
> CC genpd
>
> On Mon, Sep 18, 2023 at 10:24 AM Thomas Zimmermann <[email protected]> wrote:
> > Am 12.09.23 um 22:22 schrieb Janne Grunau via B4 Relay:
> > > From: Janne Grunau <[email protected]>
> > >
> > > Multiple power domains need to be handled explicitly in each driver. The
> > > driver core can not handle it automatically since it is not aware of
> > > power sequencing requirements the hardware might have. This is not a
> > > problem for simpledrm since everything is expected to be powered on by
> > > the bootloader. simpledrm has just ensure it remains powered on during
> > > its lifetime.
> > > This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> > > systems. The HDMI output initialized by the bootloader requires keeping
> > > the display controller and a DP phy power domain on.
> > >
> > > Signed-off-by: Janne Grunau <[email protected]>
>
> Thanks for your patch, which is now commit 61df9ca231075e70
> ("drm/simpledrm: Add support for multiple "power-domains"") in
> drm-misc/for-linux-next.
>
> > As a simpledrm patch:
> >
> > Reviewed-by: Thomas Zimmermann <[email protected]>
> >
> > Do you want to wait for another review from someone with
> > power-management expertise?
>
> Yeah, why not? Let's CC them, so they become aware...
>
> > Do we need a similar patch for ofdrm?
Drivers/subsystems for devices that are attached to multiple PM
domains are getting more and more common. Clearly there is boilerplate
code in drivers that we could get rid of, by using some common helper
functions to deal with attach/detach.
I believe there have been some attempts to add such helpers too, but
we didn't quite reach a point where those were ready to be applied. It
sounds like someone (me? :-)) needs to find some time to revisit this.
Until then, there is no other way than having each driver to deal with
this.
Kind regards
Uffe
> >
> > Best regards
> > Thomas
> >
> > > ---
> > > Changes in v2:
> > > - removed broken drm_err() log statement only ment for debugging
> > > - removed commented cast
> > > - use correct format spcifier for 'int' in log statement
> > > - add 'continue;' after failure to get device for power_domain
> > > - use drm_warn() in non fatal error cases
> > > - removed duplicate PTR_ERR conversion
> > > - Link to v1: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685@jannau.net
> > > ---
> > > drivers/gpu/drm/tiny/simpledrm.c | 105 +++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 105 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > > index ff86ba1ae1b8..9c597461d1e2 100644
> > > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > > @@ -6,6 +6,7 @@
> > > #include <linux/of_address.h>
> > > #include <linux/platform_data/simplefb.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/pm_domain.h>
> > > #include <linux/regulator/consumer.h>
> > >
> > > #include <drm/drm_aperture.h>
> > > @@ -227,6 +228,12 @@ struct simpledrm_device {
> > > unsigned int regulator_count;
> > > struct regulator **regulators;
> > > #endif
> > > + /* power-domains */
> > > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > > + int pwr_dom_count;
> > > + struct device **pwr_dom_devs;
> > > + struct device_link **pwr_dom_links;
> > > +#endif
> > >
> > > /* simplefb settings */
> > > struct drm_display_mode mode;
> > > @@ -468,6 +475,101 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> > > }
> > > #endif
> > >
> > > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > > +/*
> > > + * Generic power domain handling code.
> > > + *
> > > + * Here we handle the power-domains properties of our "simple-framebuffer"
> > > + * dt node. This is only necessary if there is more than one power-domain.
> > > + * A single power-domains is handled automatically by the driver core. Multiple
> > > + * power-domains have to be handled by drivers since the driver core can't know
> > > + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> > > + * since the bootloader has put the power domains already in the correct state.
> > > + * simpledrm has only to ensure they remain active for its lifetime.
> > > + *
> > > + * When the driver unloads, we detach from the power-domains.
> > > + *
> > > + * We only complain about errors here, no action is taken as the most likely
> > > + * error can only happen due to a mismatch between the bootloader which set
> > > + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> > > + * device tree. Chances are that there are no adverse effects, and if there are,
> > > + * a clean teardown of the fb probe will not help us much either. So just
> > > + * complain and carry on, and hope that the user actually gets a working fb at
> > > + * the end of things.
> > > + */
> > > +static void simpledrm_device_detach_genpd(void *res)
> > > +{
> > > + int i;
> > > + struct simpledrm_device *sdev = res;
> > > +
> > > + if (sdev->pwr_dom_count <= 1)
> > > + return;
> > > +
> > > + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> > > + if (!sdev->pwr_dom_links[i])
> > > + device_link_del(sdev->pwr_dom_links[i]);
> > > + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> > > + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> > > + }
> > > +}
> > > +
> > > +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> > > +{
> > > + struct device *dev = sdev->dev.dev;
> > > + int i;
> > > +
> > > + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> > > + "#power-domain-cells");
> > > + /*
> > > + * Single power-domain devices are handled by driver core nothing to do
> > > + * here. The same for device nodes without "power-domains" property.
> > > + */
> > > + if (sdev->pwr_dom_count <= 1)
> > > + return 0;
> > > +
> > > + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> > > + sizeof(*sdev->pwr_dom_devs),
> > > + GFP_KERNEL);
> > > + if (!sdev->pwr_dom_devs)
> > > + return -ENOMEM;
> > > +
> > > + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> > > + sizeof(*sdev->pwr_dom_links),
> > > + GFP_KERNEL);
> > > + if (!sdev->pwr_dom_links)
> > > + return -ENOMEM;
> > > +
> > > + for (i = 0; i < sdev->pwr_dom_count; i++) {
> > > + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> > > + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> > > + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> > > + if (ret == -EPROBE_DEFER) {
> > > + simpledrm_device_detach_genpd(sdev);
> > > + return ret;
> > > + }
> > > + drm_warn(&sdev->dev,
> > > + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> > > + continue;
> > > + }
> > > +
> > > + sdev->pwr_dom_links[i] = device_link_add(dev,
> > > + sdev->pwr_dom_devs[i],
> > > + DL_FLAG_STATELESS |
> > > + DL_FLAG_PM_RUNTIME |
> > > + DL_FLAG_RPM_ACTIVE);
> > > + if (!sdev->pwr_dom_links[i])
> > > + drm_warn(&sdev->dev, "failed to link power-domain %d\n", i);
> > > + }
> > > +
> > > + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
> > > +}
> > > +#else
> > > +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> > > +{
> > > + return 0;
> > > +}
> > > +#endif
> > > +
> > > /*
> > > * Modesetting
> > > */
> > > @@ -651,6 +753,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > > if (ret)
> > > return ERR_PTR(ret);
> > > ret = simpledrm_device_init_regulators(sdev);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > + ret = simpledrm_device_attach_genpd(sdev);
> > > if (ret)
> > > return ERR_PTR(ret);
> > >
> > >
> > > ---
> > > base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
> > > change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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