Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1284110 > unrolled thread

[PATCH v2 0/5] Raspberry Pi power domains v2

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/5] Raspberry Pi power domains v2 Eric Anholt <eric@anholt.net> - 2015-12-04 18:50 +0100
    [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver Eric Anholt <eric@anholt.net> - 2015-12-04 18:50 +0100
      Re: [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver Kevin Hilman <khilman@kernel.org> - 2015-12-08 00:40 +0100
        Re: [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver Eric Anholt <eric@anholt.net> - 2015-12-08 02:10 +0100
          Re: [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver Kevin Hilman <khilman@kernel.org> - 2015-12-08 19:20 +0100
      Re: [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver Stefan Wahren <info@lategoodbye.de> - 2015-12-11 19:20 +0100
    [PATCH v2 2/5] ARM: bcm2835: Define two new packets from the latest firmware. Eric Anholt <eric@anholt.net> - 2015-12-04 18:50 +0100
    [PATCH v2 1/5] power: domain: add pm_genpd_exit Eric Anholt <eric@anholt.net> - 2015-12-04 19:00 +0100
      Re: [PATCH v2 1/5] power: domain: add pm_genpd_exit Jon Hunter <jonathanh@nvidia.com> - 2015-12-07 11:10 +0100
        Re: [PATCH v2 1/5] power: domain: add pm_genpd_exit Kevin Hilman <khilman@kernel.org> - 2015-12-08 20:00 +0100
          Re: [PATCH v2 1/5] power: domain: add pm_genpd_exit Alexander Aring <alex.aring@gmail.com> - 2015-12-09 11:50 +0100
            Re: [PATCH v2 1/5] power: domain: add pm_genpd_exit Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-12-09 12:00 +0100

#1284110 — [PATCH v2 0/5] Raspberry Pi power domains v2

This is a series to replace Alexander Aring's power domains
submission.  It uses a new firmware interface to add support for the
other power domains in the system, which I needed for the 3D support,
while still supporting the USB domain on older firmwares.  It also
drops the static declarations, so that the driver instance is entirely
contained inside of the device.

Alexander Aring (4):
  power: domain: add pm_genpd_exit
  ARM: bcm2835: add rpi power domain driver
  dt-bindings: add rpi power domain driver bindings
  ARM: bcm2835: Add the Raspberry Pi power domain driver to the DT.

Eric Anholt (1):
  ARM: bcm2835: Define two new packets from the latest firmware.

 .../bindings/arm/bcm/raspberrypi,bcm2835-power.txt |  47 ++++
 arch/arm/boot/dts/bcm2835-rpi.dtsi                 |  11 +
 arch/arm/boot/dts/bcm2835.dtsi                     |   2 +-
 arch/arm/mach-bcm/Kconfig                          |  10 +
 arch/arm/mach-bcm/Makefile                         |   1 +
 arch/arm/mach-bcm/raspberrypi-power.c              | 269 +++++++++++++++++++++
 drivers/base/power/domain.c                        |  22 ++
 include/dt-bindings/arm/raspberrypi-power.h        |  41 ++++
 include/linux/pm_domain.h                          |   4 +
 include/soc/bcm2835/raspberrypi-firmware.h         |   2 +
 10 files changed, 408 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-power.txt
 create mode 100644 arch/arm/mach-bcm/raspberrypi-power.c
 create mode 100644 include/dt-bindings/arm/raspberrypi-power.h

-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1284114 — [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver

From: Alexander Aring <alex.aring@gmail.com>

This patch adds support for several power domains on Raspberry Pi,
including USB (so it can be enabled even if the bootloader didn't do
it), and graphics.

This patch is the combined work of Eric Anholt (who wrote USB support
inside of the Raspberry Pi firmware driver, and wrote the non-USB
domain support) and Alexander Aring (who separated the original USB
work out from the firmware driver).

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
Signed-off-by: Eric Anholt <eric@anholt.net>
---

v2: Add support for power domains other than USB, using the new
    firmware interface, reword commit message (changes by Eric)

 arch/arm/mach-bcm/Kconfig                   |  10 ++
 arch/arm/mach-bcm/Makefile                  |   1 +
 arch/arm/mach-bcm/raspberrypi-power.c       | 269 ++++++++++++++++++++++++++++
 include/dt-bindings/arm/raspberrypi-power.h |  41 +++++
 4 files changed, 321 insertions(+)
 create mode 100644 arch/arm/mach-bcm/raspberrypi-power.c
 create mode 100644 include/dt-bindings/arm/raspberrypi-power.h

diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 8c53c55..0f23bad 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -134,6 +134,16 @@ config ARCH_BCM2835
 	  This enables support for the Broadcom BCM2835 SoC. This SoC is
 	  used in the Raspberry Pi and Roku 2 devices.
 
+config RASPBERRYPI_POWER
+	bool "Raspberry Pi power domain driver"
+	depends on ARCH_BCM2835 || COMPILE_TEST
+	depends on RASPBERRYPI_FIRMWARE
+	select PM_GENERIC_DOMAINS if PM
+	select PM_GENERIC_DOMAINS_OF if PM
+	help
+	  This enables support for the RPi power domains which can be enabled
+	  or disabled via the RPi firmware.
+
 config ARCH_BCM_63XX
 	bool "Broadcom BCM63xx DSL SoC" if ARCH_MULTI_V7
 	depends on MMU
diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
index 892261f..fec2d6b 100644
--- a/arch/arm/mach-bcm/Makefile
+++ b/arch/arm/mach-bcm/Makefile
@@ -36,6 +36,7 @@ endif
 
 # BCM2835
 obj-$(CONFIG_ARCH_BCM2835)	+= board_bcm2835.o
+obj-$(CONFIG_RASPBERRYPI_POWER)	+= raspberrypi-power.o
 
 # BCM5301X
 obj-$(CONFIG_ARCH_BCM_5301X)	+= bcm_5301x.o
diff --git a/arch/arm/mach-bcm/raspberrypi-power.c b/arch/arm/mach-bcm/raspberrypi-power.c
new file mode 100644
index 0000000..3444301
--- /dev/null
+++ b/arch/arm/mach-bcm/raspberrypi-power.c
@@ -0,0 +1,269 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Authors:
+ * (C) 2015 Pengutronix, Alexander Aring <aar@pengutronix.de>
+ * Eric Anholt <eric@anholt.net>
+ */
+
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <dt-bindings/arm/raspberrypi-power.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+/*
+ * Firmware indices for the old power domains interface.  Only a few
+ * of them were actually implemented.
+ */
+#define RPI_OLD_POWER_DOMAIN_USB		3
+#define RPI_OLD_POWER_DOMAIN_V3D		10
+
+struct rpi_power_domain {
+	u32 domain;
+	bool enabled;
+	bool old_interface;
+	struct generic_pm_domain base;
+	struct rpi_firmware *fw;
+};
+
+struct rpi_power_domains {
+	bool has_new_interface;
+	struct genpd_onecell_data xlate;
+	struct rpi_firmware *fw;
+	struct rpi_power_domain domains[RPI_POWER_DOMAIN_COUNT];
+};
+
+/*
+ * Packet definition used by RPI_FIRMWARE_SET_POWER_STATE and
+ * RPI_FIRMWARE_SET_DOMAIN_STATE
+ */
+struct rpi_power_domain_packet {
+	u32 domain;
+	u32 on;
+} __packet;
+
+/*
+ * Asks the firmware to enable or disable power on a specific power
+ * domain.
+ */
+static int rpi_firmware_set_power(struct rpi_power_domain *rpi_domain, bool on)
+{
+	struct rpi_power_domain_packet packet;
+
+	packet.domain = rpi_domain->domain;
+	packet.on = on;
+	return rpi_firmware_property(rpi_domain->fw,
+				     rpi_domain->old_interface ?
+				     RPI_FIRMWARE_SET_POWER_STATE :
+				     RPI_FIRMWARE_SET_DOMAIN_STATE,
+				     &packet, sizeof(packet));
+}
+
+static int rpi_domain_off(struct generic_pm_domain *domain)
+{
+	struct rpi_power_domain *rpi_domain =
+		container_of(domain, struct rpi_power_domain, base);
+
+	return rpi_firmware_set_power(rpi_domain, false);
+}
+
+static int rpi_domain_on(struct generic_pm_domain *domain)
+{
+	struct rpi_power_domain *rpi_domain =
+		container_of(domain, struct rpi_power_domain, base);
+
+	return rpi_firmware_set_power(rpi_domain, true);
+}
+
+static void rpi_common_init_power_domain(struct rpi_power_domains *rpi_domains,
+					 int xlate_index, const char *name)
+{
+	struct rpi_power_domain *dom = &rpi_domains->domains[xlate_index];
+
+	dom->fw = rpi_domains->fw;
+
+	dom->base.name = name;
+	dom->base.power_on = rpi_domain_on;
+	dom->base.power_off = rpi_domain_off;
+
+	/*
+	 * Treat all power domains as off at boot.
+	 *
+	 * The firmware itself may be keeping some domains on, but
+	 * from Linux's perspective all we control is the refcounts
+	 * that we give to the firmware, and we can't ask the firmware
+	 * to turn off something that we haven't ourselves turned on.
+	 */
+	pm_genpd_init(&dom->base, NULL, true);
+
+	rpi_domains->xlate.domains[xlate_index] = &dom->base;
+}
+
+static void rpi_init_power_domain(struct rpi_power_domains *rpi_domains,
+				  int xlate_index, const char *name)
+{
+	struct rpi_power_domain *dom = &rpi_domains->domains[xlate_index];
+
+	if (!rpi_domains->has_new_interface)
+		return;
+
+	/* The DT binding index is the firmware's domain index minus one. */
+	dom->domain = xlate_index + 1;
+
+	rpi_common_init_power_domain(rpi_domains, xlate_index, name);
+}
+
+static void rpi_init_old_power_domain(struct rpi_power_domains *rpi_domains,
+				      int xlate_index, int domain,
+				      const char *name)
+{
+	struct rpi_power_domain *dom = &rpi_domains->domains[xlate_index];
+
+	dom->old_interface = true;
+	dom->domain = domain;
+
+	rpi_common_init_power_domain(rpi_domains, xlate_index, name);
+}
+
+/*
+ * Detects whether the firmware supports the new power domains interface.
+ *
+ * The firmware doesn't actually return an error on an unknown tag,
+ * and just skips over it, so we do the detection by putting an
+ * unexpected value in the return field and checking if it was
+ * unchanged.
+ */
+static bool
+rpi_has_new_domain_support(struct rpi_power_domains *rpi_domains)
+{
+	struct rpi_power_domain_packet packet;
+	int ret;
+
+	packet.domain = RPI_POWER_DOMAIN_ARM;
+	packet.on = ~0;
+
+	ret = rpi_firmware_property(rpi_domains->fw,
+				    RPI_FIRMWARE_GET_DOMAIN_STATE,
+				    &packet, sizeof(packet));
+
+	return ret == 0 && packet.on != ~0;
+}
+
+static int rpi_power_probe(struct platform_device *pdev)
+{
+	struct device_node *fw_np;
+	struct device *dev = &pdev->dev;
+	struct rpi_power_domains *rpi_domains;
+	int ret, i;
+
+	rpi_domains = devm_kzalloc(dev, sizeof(*rpi_domains), GFP_KERNEL);
+	if (!rpi_domains)
+		return -ENOMEM;
+
+	rpi_domains->xlate.domains =
+		devm_kzalloc(dev, sizeof(*rpi_domains->xlate.domains) *
+			     RPI_POWER_DOMAIN_COUNT, GFP_KERNEL);
+	if (!rpi_domains->xlate.domains)
+		return -ENOMEM;
+
+	rpi_domains->xlate.num_domains = RPI_POWER_DOMAIN_COUNT;
+
+	fw_np = of_parse_phandle(pdev->dev.of_node, "firmware", 0);
+	if (!fw_np) {
+		dev_err(&pdev->dev, "no firmware node\n");
+		return -ENODEV;
+	}
+
+	rpi_domains->fw = rpi_firmware_get(fw_np);
+	of_node_put(fw_np);
+	if (!rpi_domains->fw)
+		return -EPROBE_DEFER;
+
+	rpi_domains->has_new_interface =
+		rpi_has_new_domain_support(rpi_domains);
+
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_I2C0, "I2C0");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_I2C1, "I2C1");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_I2C2, "I2C2");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_VIDEO_SCALER,
+			      "VIDEO_SCALER");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_VPU1, "VPU1");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_HDMI, "HDMI");
+
+	/*
+	 * Use the old firmware interface for USB power, so that we
+	 * can turn it on even if the firmware hasn't been updated.
+	 */
+	rpi_init_old_power_domain(rpi_domains, RPI_POWER_DOMAIN_USB,
+				  RPI_OLD_POWER_DOMAIN_USB, "USB");
+
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_VEC, "VEC");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_JPEG, "JPEG");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_H264, "H264");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_V3D, "V3D");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_UNICAM0, "UNICAM0");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_UNICAM1, "UNICAM1");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CCP2RX, "CCP2RX");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CSI2, "CSI2");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CPI, "CPI");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_DSI0, "DSI0");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_DSI1, "DSI1");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_TRANSPOSER,
+			      "TRANPOSER");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CCP2TX, "CCP2TX");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CDP, "CDP");
+	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_ARM, "ARM");
+
+	ret = of_genpd_add_provider_onecell(dev->of_node, &rpi_domains->xlate);
+	if (ret < 0)
+		goto exit_pm;
+
+	platform_set_drvdata(pdev, rpi_domains);
+
+	return 0;
+
+exit_pm:
+	for (i = 0; i < rpi_domains->xlate.num_domains; i++)
+		pm_genpd_exit(rpi_domains->xlate.domains[i]);
+
+	return ret;
+}
+
+static int rpi_power_remove(struct platform_device *pdev)
+{
+	struct rpi_power_domains *rpi_domains = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int i;
+
+	for (i = 0; i < rpi_domains->xlate.num_domains; i++)
+		pm_genpd_exit(rpi_domains->xlate.domains[i]);
+
+	of_genpd_del_provider(dev->of_node);
+
+	return 0;
+}
+
+static const struct of_device_id rpi_power_of_match[] = {
+	{ .compatible = "raspberrypi,bcm2835-power", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rpi_power_of_match);
+
+static struct platform_driver rpi_power_driver = {
+	.driver = {
+		.name = "raspberrypi-power",
+		.of_match_table = rpi_power_of_match,
+	},
+	.probe		= rpi_power_probe,
+	.remove		= rpi_power_remove,
+};
+module_platform_driver(rpi_power_driver);
+
+MODULE_AUTHOR("Alexander Aring <aar@pengutronix.de>");
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi power domain driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/dt-bindings/arm/raspberrypi-power.h b/include/dt-bindings/arm/raspberrypi-power.h
new file mode 100644
index 0000000..b3ff8e0
--- /dev/null
+++ b/include/dt-bindings/arm/raspberrypi-power.h
@@ -0,0 +1,41 @@
+/*
+ *  Copyright © 2015 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _DT_BINDINGS_ARM_BCM2835_RPI_POWER_H
+#define _DT_BINDINGS_ARM_BCM2835_RPI_POWER_H
+
+/* These power domain indices are the firmware interface's indices
+ * minus one.
+ */
+#define RPI_POWER_DOMAIN_I2C0		0
+#define RPI_POWER_DOMAIN_I2C1		1
+#define RPI_POWER_DOMAIN_I2C2		2
+#define RPI_POWER_DOMAIN_VIDEO_SCALER	3
+#define RPI_POWER_DOMAIN_VPU1		4
+#define RPI_POWER_DOMAIN_HDMI		5
+#define RPI_POWER_DOMAIN_USB		6
+#define RPI_POWER_DOMAIN_VEC		7
+#define RPI_POWER_DOMAIN_JPEG		8
+#define RPI_POWER_DOMAIN_H264		9
+#define RPI_POWER_DOMAIN_V3D		10
+#define RPI_POWER_DOMAIN_ISP		11
+#define RPI_POWER_DOMAIN_UNICAM0	12
+#define RPI_POWER_DOMAIN_UNICAM1	13
+#define RPI_POWER_DOMAIN_CCP2RX		14
+#define RPI_POWER_DOMAIN_CSI2		15
+#define RPI_POWER_DOMAIN_CPI		16
+#define RPI_POWER_DOMAIN_DSI0		17
+#define RPI_POWER_DOMAIN_DSI1		18
+#define RPI_POWER_DOMAIN_TRANSPOSER	19
+#define RPI_POWER_DOMAIN_CCP2TX		20
+#define RPI_POWER_DOMAIN_CDP		21
+#define RPI_POWER_DOMAIN_ARM		22
+
+#define RPI_POWER_DOMAIN_COUNT		23
+
+#endif /* _DT_BINDINGS_ARM_BCM2835_RPI_POWER_H */
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286108 — Re: [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver

Eric Anholt <eric@anholt.net> writes:

> From: Alexander Aring <alex.aring@gmail.com>
>
> This patch adds support for several power domains on Raspberry Pi,
> including USB (so it can be enabled even if the bootloader didn't do
> it), and graphics.
>
> This patch is the combined work of Eric Anholt (who wrote USB support
> inside of the Raspberry Pi firmware driver, and wrote the non-USB
> domain support) and Alexander Aring (who separated the original USB
> work out from the firmware driver).
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>
> v2: Add support for power domains other than USB, using the new
>     firmware interface, reword commit message (changes by Eric)

[...]

> +/*
> + * Firmware indices for the old power domains interface.  Only a few
> + * of them were actually implemented.
> + */
> +#define RPI_OLD_POWER_DOMAIN_USB		3
> +#define RPI_OLD_POWER_DOMAIN_V3D		10
> +

Is "old" the right word here?  Are there firmware versions that could be
used instead?  What happens when the firwmware is updated next time?

[...]

> +	/*
> +	 * Use the old firmware interface for USB power, so that we
> +	 * can turn it on even if the firmware hasn't been updated.
> +	 */
> +	rpi_init_old_power_domain(rpi_domains, RPI_POWER_DOMAIN_USB,
> +				  RPI_OLD_POWER_DOMAIN_USB, "USB");

This seems a bit restrictive.

To me, it seems that determining "old" or "new" (or revision of fw
interface to use) should be described in DT, not hard-coded in the power
domain driver.

What about an additional DT property to describe that? or possibly
another cell in the domain which could be used to optionally set
old/legacy.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286135 — Re: [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver

[Multipart message — attachments visible in raw view] — view raw

Kevin Hilman <khilman@kernel.org> writes:

> Eric Anholt <eric@anholt.net> writes:
>
>> From: Alexander Aring <alex.aring@gmail.com>
>>
>> This patch adds support for several power domains on Raspberry Pi,
>> including USB (so it can be enabled even if the bootloader didn't do
>> it), and graphics.
>>
>> This patch is the combined work of Eric Anholt (who wrote USB support
>> inside of the Raspberry Pi firmware driver, and wrote the non-USB
>> domain support) and Alexander Aring (who separated the original USB
>> work out from the firmware driver).
>>
>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>
>> v2: Add support for power domains other than USB, using the new
>>     firmware interface, reword commit message (changes by Eric)
>
> [...]
>
>> +/*
>> + * Firmware indices for the old power domains interface.  Only a few
>> + * of them were actually implemented.
>> + */
>> +#define RPI_OLD_POWER_DOMAIN_USB		3
>> +#define RPI_OLD_POWER_DOMAIN_V3D		10
>> +
>
> Is "old" the right word here?  Are there firmware versions that could be
> used instead?  What happens when the firwmware is updated next time?

Old is a good word.  It's the old interface.

As for what happens when the firmware is updated: Nothing.  The firmware
is updated all the time, and it maintains backwards compatibility.
Unless you mean "what happens when a newer, fancier power domain
interface is created and we need a name for the newer one" and the
answer is "this is a define entirely within the driver, and we can just
rename it when we want to."

> [...]
>
>> +	/*
>> +	 * Use the old firmware interface for USB power, so that we
>> +	 * can turn it on even if the firmware hasn't been updated.
>> +	 */
>> +	rpi_init_old_power_domain(rpi_domains, RPI_POWER_DOMAIN_USB,
>> +				  RPI_OLD_POWER_DOMAIN_USB, "USB");
>
> This seems a bit restrictive.
>
> To me, it seems that determining "old" or "new" (or revision of fw
> interface to use) should be described in DT, not hard-coded in the power
> domain driver.
>
> What about an additional DT property to describe that? or possibly
> another cell in the domain which could be used to optionally set
> old/legacy.

As the author and maintainer of the code, I don't feel it's restrictive.
The firmware protocol is defined and is guaranteed to continue to exist,
it's only useful for this platform, and defining a new set of custom
devicetree properties for it would only obfuscate the implementation.
DT is a useful tool for separating out the between-board differences for
the same piece of hardware across multiple implementations at different
addresses, while this is neither hardware nor in multiple
implementations at different addresses.

[toc] | [prev] | [next] | [standalone]

#1286734 — Re: [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver

Hi Eric,

Eric Anholt <eric@anholt.net> writes:

> Kevin Hilman <khilman@kernel.org> writes:
>
>> Eric Anholt <eric@anholt.net> writes:
>>
>>> From: Alexander Aring <alex.aring@gmail.com>
>>>
>>> This patch adds support for several power domains on Raspberry Pi,
>>> including USB (so it can be enabled even if the bootloader didn't do
>>> it), and graphics.
>>>
>>> This patch is the combined work of Eric Anholt (who wrote USB support
>>> inside of the Raspberry Pi firmware driver, and wrote the non-USB
>>> domain support) and Alexander Aring (who separated the original USB
>>> work out from the firmware driver).
>>>
>>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>> ---
>>>
>>> v2: Add support for power domains other than USB, using the new
>>>     firmware interface, reword commit message (changes by Eric)
>>
>> [...]
>>
>>> +/*
>>> + * Firmware indices for the old power domains interface.  Only a few
>>> + * of them were actually implemented.
>>> + */
>>> +#define RPI_OLD_POWER_DOMAIN_USB		3
>>> +#define RPI_OLD_POWER_DOMAIN_V3D		10
>>> +
>>
>> Is "old" the right word here?  Are there firmware versions that could be
>> used instead?  What happens when the firwmware is updated next time?
>
> Old is a good word.  It's the old interface.

Sure, but "old" is relative and based on experience, folks come to
regret those kinds of names.

> As for what happens when the firmware is updated: Nothing.  The firmware
> is updated all the time, and it maintains backwards compatibility.
> Unless you mean "what happens when a newer, fancier power domain
> interface is created and we need a name for the newer one" and the
> answer is "this is a define entirely within the driver, and we can just
> rename it when we want to."

Sure, it's very contained in this driver, so it's ultimately up to you.
It's not something worth blocking this about, I just wanted to be sure
since I'm not very familiar with how the rpi firmware evolves.

>> [...]
>>
>>> +	/*
>>> +	 * Use the old firmware interface for USB power, so that we
>>> +	 * can turn it on even if the firmware hasn't been updated.
>>> +	 */
>>> +	rpi_init_old_power_domain(rpi_domains, RPI_POWER_DOMAIN_USB,
>>> +				  RPI_OLD_POWER_DOMAIN_USB, "USB");
>>
>> This seems a bit restrictive.
>>
>> To me, it seems that determining "old" or "new" (or revision of fw
>> interface to use) should be described in DT, not hard-coded in the power
>> domain driver.
>>
>> What about an additional DT property to describe that? or possibly
>> another cell in the domain which could be used to optionally set
>> old/legacy.
>
> As the author and maintainer of the code, I don't feel it's restrictive.
> The firmware protocol is defined and is guaranteed to continue to exist,
> it's only useful for this platform, and defining a new set of custom
> devicetree properties for it would only obfuscate the implementation.
> DT is a useful tool for separating out the between-board differences for
> the same piece of hardware across multiple implementations at different
> addresses, while this is neither hardware nor in multiple
> implementations at different addresses.

That being said, firmware revisions are also very often something that
qualifies as a difference between boards.

Anyways, as I said above, I think this is a potential future problem,
but it's not a big deal to me since it's very self contained.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1289809 — Re: [PATCH v2 3/5] ARM: bcm2835: add rpi power domain driver

Hi Eric,

Am 04.12.2015 um 18:45 schrieb Eric Anholt:
> From: Alexander Aring <alex.aring@gmail.com>
>
> This patch adds support for several power domains on Raspberry Pi,
> including USB (so it can be enabled even if the bootloader didn't do
> it), and graphics.
>
> This patch is the combined work of Eric Anholt (who wrote USB support
> inside of the Raspberry Pi firmware driver, and wrote the non-USB
> domain support) and Alexander Aring (who separated the original USB
> work out from the firmware driver).
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>
> v2: Add support for power domains other than USB, using the new
>      firmware interface, reword commit message (changes by Eric)
>
>   arch/arm/mach-bcm/Kconfig                   |  10 ++
>   arch/arm/mach-bcm/Makefile                  |   1 +
>   arch/arm/mach-bcm/raspberrypi-power.c       | 269 ++++++++++++++++++++++++++++
>   include/dt-bindings/arm/raspberrypi-power.h |  41 +++++
>   4 files changed, 321 insertions(+)
>   create mode 100644 arch/arm/mach-bcm/raspberrypi-power.c
>   create mode 100644 include/dt-bindings/arm/raspberrypi-power.h
>
> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
> index 8c53c55..0f23bad 100644
> --- a/arch/arm/mach-bcm/Kconfig
> +++ b/arch/arm/mach-bcm/Kconfig
> @@ -134,6 +134,16 @@ config ARCH_BCM2835
>   	  This enables support for the Broadcom BCM2835 SoC. This SoC is
>   	  used in the Raspberry Pi and Roku 2 devices.
>
> +config RASPBERRYPI_POWER
> +	bool "Raspberry Pi power domain driver"
> +	depends on ARCH_BCM2835 || COMPILE_TEST
> +	depends on RASPBERRYPI_FIRMWARE
> +	select PM_GENERIC_DOMAINS if PM
> +	select PM_GENERIC_DOMAINS_OF if PM
> +	help
> +	  This enables support for the RPi power domains which can be enabled
> +	  or disabled via the RPi firmware.
> +
>   config ARCH_BCM_63XX
>   	bool "Broadcom BCM63xx DSL SoC" if ARCH_MULTI_V7
>   	depends on MMU
> diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
> index 892261f..fec2d6b 100644
> --- a/arch/arm/mach-bcm/Makefile
> +++ b/arch/arm/mach-bcm/Makefile
> @@ -36,6 +36,7 @@ endif
>
>   # BCM2835
>   obj-$(CONFIG_ARCH_BCM2835)	+= board_bcm2835.o
> +obj-$(CONFIG_RASPBERRYPI_POWER)	+= raspberrypi-power.o
>
>   # BCM5301X
>   obj-$(CONFIG_ARCH_BCM_5301X)	+= bcm_5301x.o
> diff --git a/arch/arm/mach-bcm/raspberrypi-power.c b/arch/arm/mach-bcm/raspberrypi-power.c
> new file mode 100644
> index 0000000..3444301
> --- /dev/null
> +++ b/arch/arm/mach-bcm/raspberrypi-power.c
> @@ -0,0 +1,269 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Authors:
> + * (C) 2015 Pengutronix, Alexander Aring <aar@pengutronix.de>
> + * Eric Anholt <eric@anholt.net>

shouldn't be the copyright before license?

> + */
> + [...]
> +
> +static int rpi_power_probe(struct platform_device *pdev)
> +{
> +	struct device_node *fw_np;
> +	struct device *dev = &pdev->dev;
> +	struct rpi_power_domains *rpi_domains;
> +	int ret, i;
> +
> +	rpi_domains = devm_kzalloc(dev, sizeof(*rpi_domains), GFP_KERNEL);
> +	if (!rpi_domains)
> +		return -ENOMEM;
> +
> +	rpi_domains->xlate.domains =
> +		devm_kzalloc(dev, sizeof(*rpi_domains->xlate.domains) *
> +			     RPI_POWER_DOMAIN_COUNT, GFP_KERNEL);
> +	if (!rpi_domains->xlate.domains)
> +		return -ENOMEM;
> +
> +	rpi_domains->xlate.num_domains = RPI_POWER_DOMAIN_COUNT;
> +
> +	fw_np = of_parse_phandle(pdev->dev.of_node, "firmware", 0);
> +	if (!fw_np) {
> +		dev_err(&pdev->dev, "no firmware node\n");
> +		return -ENODEV;
> +	}
> +
> +	rpi_domains->fw = rpi_firmware_get(fw_np);
> +	of_node_put(fw_np);
> +	if (!rpi_domains->fw)
> +		return -EPROBE_DEFER;
> +
> +	rpi_domains->has_new_interface =
> +		rpi_has_new_domain_support(rpi_domains);
> +
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_I2C0, "I2C0");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_I2C1, "I2C1");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_I2C2, "I2C2");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_VIDEO_SCALER,
> +			      "VIDEO_SCALER");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_VPU1, "VPU1");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_HDMI, "HDMI");
> +
> +	/*
> +	 * Use the old firmware interface for USB power, so that we
> +	 * can turn it on even if the firmware hasn't been updated.
> +	 */
> +	rpi_init_old_power_domain(rpi_domains, RPI_POWER_DOMAIN_USB,
> +				  RPI_OLD_POWER_DOMAIN_USB, "USB");
> +
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_VEC, "VEC");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_JPEG, "JPEG");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_H264, "H264");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_V3D, "V3D");

After this line i would expect the following:

rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_ISP, "ISP");

> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_UNICAM0, "UNICAM0");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_UNICAM1, "UNICAM1");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CCP2RX, "CCP2RX");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CSI2, "CSI2");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CPI, "CPI");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_DSI0, "DSI0");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_DSI1, "DSI1");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_TRANSPOSER,
> +			      "TRANPOSER");

s/TRANPOSER/TRANSPOSER ?

> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CCP2TX, "CCP2TX");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_CDP, "CDP");
> +	rpi_init_power_domain(rpi_domains, RPI_POWER_DOMAIN_ARM, "ARM");
> +
> +	ret = of_genpd_add_provider_onecell(dev->of_node, &rpi_domains->xlate);
> +	if (ret < 0)
> +		goto exit_pm;
> +
> +	platform_set_drvdata(pdev, rpi_domains);
> +
> +	return 0;
> +
> +exit_pm:
> +	for (i = 0; i < rpi_domains->xlate.num_domains; i++)
> +		pm_genpd_exit(rpi_domains->xlate.domains[i]);
> +
> +	return ret;
> +}
> +

Best regards
Stefan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1284116 — [PATCH v2 2/5] ARM: bcm2835: Define two new packets from the latest firmware.

These packets give us direct access to the firmware's power management
code, as opposed to GET/SET_POWER_STATE packets that only had a couple
of domains implemented.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 include/soc/bcm2835/raspberrypi-firmware.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
index c07d74a..3fb3571 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -72,10 +72,12 @@ enum rpi_firmware_property_tag {
 	RPI_FIRMWARE_SET_ENABLE_QPU =                         0x00030012,
 	RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
 	RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
+	RPI_FIRMWARE_GET_DOMAIN_STATE =                       0x00030030,
 	RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
 	RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
 	RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
 	RPI_FIRMWARE_SET_TURBO =                              0x00038009,
+	RPI_FIRMWARE_SET_DOMAIN_STATE =                       0x00038030,
 
 	/* Dispmanx TAGS */
 	RPI_FIRMWARE_FRAMEBUFFER_ALLOCATE =                   0x00040001,
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1284125 — [PATCH v2 1/5] power: domain: add pm_genpd_exit

From: Alexander Aring <alex.aring@gmail.com>

This patch adds function pm_genpd_exit for undo a pm_genpd_init. This
is useful for multiple power domains while probing. If the probing fails
after one pm_genpd_init was called we need to undo all previous
registrations of generic pm domains inside the gpd_list list.

There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again
registered power domains and not registered domains, the driver can use
this mechanism to have an array with registered and non-registered power
domains, where non-registered power domains are NULL.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Aring <alex.aring@gmail.com>
Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 22 ++++++++++++++++++++++
 include/linux/pm_domain.h   |  4 ++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 167418e..e7aca27 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1509,6 +1509,28 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 
+/**
+ * pm_genpd_exit - Uninitialize a generic I/O PM domain object.
+ * @genpd: PM domain object to uninitialize.
+ */
+void pm_genpd_exit(struct generic_pm_domain *genpd)
+{
+	if (IS_ERR_OR_NULL(genpd))
+		return;
+
+	/* check if domain is still in registered inside the pm subsystem */
+	WARN_ON_ONCE(!list_empty(&genpd->master_links) ||
+		     !list_empty(&genpd->slave_links) ||
+		     !list_empty(&genpd->dev_list));
+
+	mutex_lock(&gpd_list_lock);
+	list_del(&genpd->gpd_list_node);
+	mutex_unlock(&gpd_list_lock);
+
+	mutex_destroy(&genpd->lock);
+}
+EXPORT_SYMBOL_GPL(pm_genpd_exit);
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 /*
  * Device Tree based PM domain providers.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ba4ced3..5020b36 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -123,6 +123,7 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 				     struct generic_pm_domain *target);
 extern void pm_genpd_init(struct generic_pm_domain *genpd,
 			  struct dev_power_governor *gov, bool is_off);
+extern void pm_genpd_exit(struct generic_pm_domain *genpd);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -161,6 +162,9 @@ static inline void pm_genpd_init(struct generic_pm_domain *genpd,
 				 struct dev_power_governor *gov, bool is_off)
 {
 }
+static inline void pm_genpd_exit(struct generic_pm_domain *genpd)
+{
+}
 #endif
 
 static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285170 — Re: [PATCH v2 1/5] power: domain: add pm_genpd_exit

On 04/12/15 17:45, Eric Anholt wrote:
> From: Alexander Aring <alex.aring@gmail.com>
> 
> This patch adds function pm_genpd_exit for undo a pm_genpd_init. This
> is useful for multiple power domains while probing. If the probing fails
> after one pm_genpd_init was called we need to undo all previous
> registrations of generic pm domains inside the gpd_list list.
> 
> There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again
> registered power domains and not registered domains, the driver can use
> this mechanism to have an array with registered and non-registered power
> domains, where non-registered power domains are NULL.
> 
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/domain.c | 22 ++++++++++++++++++++++
>  include/linux/pm_domain.h   |  4 ++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 167418e..e7aca27 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1509,6 +1509,28 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>  }
>  EXPORT_SYMBOL_GPL(pm_genpd_init);
>  
> +/**
> + * pm_genpd_exit - Uninitialize a generic I/O PM domain object.
> + * @genpd: PM domain object to uninitialize.
> + */
> +void pm_genpd_exit(struct generic_pm_domain *genpd)
> +{
> +	if (IS_ERR_OR_NULL(genpd))
> +		return;
> +
> +	/* check if domain is still in registered inside the pm subsystem */
> +	WARN_ON_ONCE(!list_empty(&genpd->master_links) ||
> +		     !list_empty(&genpd->slave_links) ||
> +		     !list_empty(&genpd->dev_list));
> +

Why not return an error here? Seems bad to remove it, if it could still
be referenced by other domains.

Also not sure if you need to lock around the above test and removing the
domain.

> +	mutex_lock(&gpd_list_lock);
> +	list_del(&genpd->gpd_list_node);
> +	mutex_unlock(&gpd_list_lock);
> +
> +	mutex_destroy(&genpd->lock);
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_exit);
> +

BTW, I had just submitted a similar patch here [0]. So I would also like
to see such an API added.

Cheers
Jon

[0] http://marc.info/?l=devicetree&m=144924138932726&w=2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286775 — Re: [PATCH v2 1/5] power: domain: add pm_genpd_exit

Jon Hunter <jonathanh@nvidia.com> writes:

> On 04/12/15 17:45, Eric Anholt wrote:
>> From: Alexander Aring <alex.aring@gmail.com>
>> 
>> This patch adds function pm_genpd_exit for undo a pm_genpd_init. This
>> is useful for multiple power domains while probing. If the probing fails
>> after one pm_genpd_init was called we need to undo all previous
>> registrations of generic pm domains inside the gpd_list list.
>> 
>> There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again
>> registered power domains and not registered domains, the driver can use
>> this mechanism to have an array with registered and non-registered power
>> domains, where non-registered power domains are NULL.
>> 
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Kevin Hilman <khilman@kernel.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Pavel Machek <pavel@ucw.cz>
>> Cc: Len Brown <len.brown@intel.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 22 ++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  4 ++++
>>  2 files changed, 26 insertions(+)
>> 
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 167418e..e7aca27 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1509,6 +1509,28 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>>  }
>>  EXPORT_SYMBOL_GPL(pm_genpd_init);
>>  
>> +/**
>> + * pm_genpd_exit - Uninitialize a generic I/O PM domain object.
>> + * @genpd: PM domain object to uninitialize.
>> + */
>> +void pm_genpd_exit(struct generic_pm_domain *genpd)
>> +{
>> +	if (IS_ERR_OR_NULL(genpd))
>> +		return;
>> +
>> +	/* check if domain is still in registered inside the pm subsystem */
>> +	WARN_ON_ONCE(!list_empty(&genpd->master_links) ||
>> +		     !list_empty(&genpd->slave_links) ||
>> +		     !list_empty(&genpd->dev_list));
>> +
>
> Why not return an error here? Seems bad to remove it, if it could still
> be referenced by other domains.

I had pointed this out as well in an earlier review.

> Also not sure if you need to lock around the above test and removing the
> domain.
>
>> +	mutex_lock(&gpd_list_lock);
>> +	list_del(&genpd->gpd_list_node);
>> +	mutex_unlock(&gpd_list_lock);
>> +
>> +	mutex_destroy(&genpd->lock);
>> +}
>> +EXPORT_SYMBOL_GPL(pm_genpd_exit);
>> +
>
> BTW, I had just submitted a similar patch here [0]. So I would also like
> to see such an API added.

Between the two of you, maybe come up with an agreed upon patch and
re-submit.

Kevin

> Cheers
> Jon
>
> [0] http://marc.info/?l=devicetree&m=144924138932726&w=2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287400 — Re: [PATCH v2 1/5] power: domain: add pm_genpd_exit

Hi,

On Tue, Dec 08, 2015 at 10:59:00AM -0800, Kevin Hilman wrote:
> Jon Hunter <jonathanh@nvidia.com> writes:
> 
> > On 04/12/15 17:45, Eric Anholt wrote:
> >> From: Alexander Aring <alex.aring@gmail.com>
> >> 
> >> This patch adds function pm_genpd_exit for undo a pm_genpd_init. This
> >> is useful for multiple power domains while probing. If the probing fails
> >> after one pm_genpd_init was called we need to undo all previous
> >> registrations of generic pm domains inside the gpd_list list.
> >> 
> >> There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again
> >> registered power domains and not registered domains, the driver can use
> >> this mechanism to have an array with registered and non-registered power
> >> domains, where non-registered power domains are NULL.
> >> 
> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> >> Cc: Kevin Hilman <khilman@kernel.org>
> >> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >> Cc: Pavel Machek <pavel@ucw.cz>
> >> Cc: Len Brown <len.brown@intel.com>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >> Signed-off-by: Eric Anholt <eric@anholt.net>
> >> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> ---
> >>  drivers/base/power/domain.c | 22 ++++++++++++++++++++++
> >>  include/linux/pm_domain.h   |  4 ++++
> >>  2 files changed, 26 insertions(+)
> >> 
> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >> index 167418e..e7aca27 100644
> >> --- a/drivers/base/power/domain.c
> >> +++ b/drivers/base/power/domain.c
> >> @@ -1509,6 +1509,28 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
> >>  }
> >>  EXPORT_SYMBOL_GPL(pm_genpd_init);
> >>  
> >> +/**
> >> + * pm_genpd_exit - Uninitialize a generic I/O PM domain object.
> >> + * @genpd: PM domain object to uninitialize.
> >> + */
> >> +void pm_genpd_exit(struct generic_pm_domain *genpd)
> >> +{
> >> +	if (IS_ERR_OR_NULL(genpd))
> >> +		return;
> >> +
> >> +	/* check if domain is still in registered inside the pm subsystem */
> >> +	WARN_ON_ONCE(!list_empty(&genpd->master_links) ||
> >> +		     !list_empty(&genpd->slave_links) ||
> >> +		     !list_empty(&genpd->dev_list));
> >> +
> >
> > Why not return an error here? Seems bad to remove it, if it could still
> > be referenced by other domains.
> 
> I had pointed this out as well in an earlier review.
> 

I talked with Ulf Hansson about such handling and there exists currently
no handling to remove the pm_genpd on error handling (what our use case
is for RPi domains).

The real solution would be a "pm_genpd_exit_recursive" functionality to
remove subdomains, etc -> simple everything. Anway I am not a expert into
power domain functionality and this simple approach was enough to him to
add "something" which we have actually a lack of support.


Now "returning an errno" here:

I don't know how it should be handled in an "error handling" case. The
WARN_ON_ONCE should say "somebody use this API wrong" which is a very
unlikely case.
These lists should be empty when calling pm_genpd_exit before in any case.


Example: the error case is while probing, how we react on a -EBUSY there
"in an error case" -> simple ignore it? But then nobody see that the use
of this function is wrong.

Should it be something like that?

err_probe:
	WARN_ON_ONCE(pm_genpd_exit(foo) < 0);

This function should be "void" here, I never saw some unregistration
functionality which returns some "int" for error handling. Which brings
us back to the "real" solution, a "pm_genpd_exit_recursive" functionality
which unregister everything which belongs to a "generic_pm_domain".

To have a "pm_genpd_exit" is only the first step. That we can improve error
handling for pm_genpd_init. (Which RPi power domains use and doesn't
register any subdomains, etc. for probing).

> > Also not sure if you need to lock around the above test and removing the
> > domain.
> >
> >> +	mutex_lock(&gpd_list_lock);
> >> +	list_del(&genpd->gpd_list_node);
> >> +	mutex_unlock(&gpd_list_lock);
> >> +
> >> +	mutex_destroy(&genpd->lock);
> >> +}
> >> +EXPORT_SYMBOL_GPL(pm_genpd_exit);
> >> +
> >
> > BTW, I had just submitted a similar patch here [0]. So I would also like
> > to see such an API added.
> 

:-)

> Between the two of you, maybe come up with an agreed upon patch and
> re-submit.
> 
> Kevin
> 
> > Cheers
> > Jon
> >
> > [0] http://marc.info/?l=devicetree&m=144924138932726&w=2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287414 — Re: [PATCH v2 1/5] power: domain: add pm_genpd_exit

On Wed, Dec 09, 2015 at 11:47:58AM +0100, Alexander Aring wrote:
> Example: the error case is while probing, how we react on a -EBUSY there
> "in an error case" -> simple ignore it? But then nobody see that the use
> of this function is wrong.

The proper way to deal with functionality that can only be registered
but never removed is to report the error, but never fail during probing,
and never allow removal (empty removal function.)

If you return a failure code during probe, you end up in an inconsistent
situation where you have facilities registered, but resources that those
facilities require will be undone when probe() returns a failure, and
that can potentially lead to kernel oops or scribbling over someone
elses device or memory.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web