[PATCH 0/2] drivers: soc: xilinx: Add support for ZynqMP power domain driver

[PATCH 0/2] drivers: soc: xilinx: Add support for ZynqMP power domain driver

[Jolly Shah]

The zynqmp power domain driver communicates the usage requirements
for logical power domains / devices to the platform FW.
FW is responsible for choosing appropriate power states,
taking Linux' usage information into account.

This patchset has dependency on below drivers:
Firmware Driver:     https://patchwork.kernel.org/patch/10230773/

Jolly Shah (2):
  dt-bindings: power: Add ZynqMP power domain bindings
  drivers: soc: xilinx: Add ZynqMP power domain driver

 .../devicetree/bindings/power/zynqmp-genpd.txt     |  46 +++
 drivers/soc/xilinx/Kconfig                         |   2 +
 drivers/soc/xilinx/Makefile                        |   2 +
 drivers/soc/xilinx/zynqmp/Kconfig                  |  16 +
 drivers/soc/xilinx/zynqmp/Makefile                 |   4 +
 drivers/soc/xilinx/zynqmp/pm_domains.c             | 339 +++++++++++++++++++++
 6 files changed, 409 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/zynqmp-genpd.txt
 create mode 100644 drivers/soc/xilinx/zynqmp/Kconfig
 create mode 100644 drivers/soc/xilinx/zynqmp/Makefile
 create mode 100644 drivers/soc/xilinx/zynqmp/pm_domains.c

-- 
2.7.4

[PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Jolly Shah]

Add documentation to describe ZynqMP power domain bindings.

Signed-off-by: Jolly Shah <jollys@xilinx.com>
Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
---
 .../devicetree/bindings/power/zynqmp-genpd.txt     | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/zynqmp-genpd.txt

diff --git a/Documentation/devicetree/bindings/power/zynqmp-genpd.txt b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
new file mode 100644
index 0000000..25f9711
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
@@ -0,0 +1,46 @@
+Device Tree bindings for Xilinx Zynq MPSoC PM domains
+
+The binding for zynqmp-genpd follow the common generic PM domain binding[1].
+
+[1] Documentation/devicetree/bindings/power/power_domain.txt
+
+== Zynq MPSoC Generic PM Domain Node ==
+
+Required properties:
+ - compatible: Must be: "xlnx,zynqmp-genpd"
+
+This node contains a number of subnodes, each representing a single PM domain
+that PM domain consumer devices reference.
+
+== PM Domain Nodes ==
+
+Required properties:
+ - #power-domain-cells: Number of cells in a PM domain specifier. Must be 0.
+ - pd-id: List of domain identifiers of as defined by platform firmware. These
+	  identifiers are passed to the PM firmware.
+
+Example:
+	zynqmp-genpd {
+		compatible = "xlnx,zynqmp-genpd";
+
+		pd_usb0: pd-usb0 {
+			pd-id = <22>;
+			#power-domain-cells = <0>;
+		};
+
+		pd_sata: pd-sata {
+			pd-id = <28>;
+			#power-domain-cells = <0>;
+		};
+
+		pd_gpu: pd-gpu {
+			pd-id = <58 20 21>;
+			#power-domain-cells = <0x0>;
+		};
+	};
+
+	sata0: ahci@SATA_AHCI_HBA {
+		...
+		power-domains = <&pd_sata>;
+		...
+	};
-- 
2.7.4

[PATCH 2/2] drivers: soc: xilinx: Add ZynqMP power domain driver

[Jolly Shah]

The zynqmp-genpd driver communicates the usage requirements
for logical power domains / devices to the platform FW.
FW is responsible for choosing appropriate power states,
taking Linux' usage information into account.

Signed-off-by: Jolly Shah <jollys@xilinx.com>
Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
---
 drivers/soc/xilinx/Kconfig             |   2 +
 drivers/soc/xilinx/Makefile            |   2 +
 drivers/soc/xilinx/zynqmp/Kconfig      |  16 ++
 drivers/soc/xilinx/zynqmp/Makefile     |   4 +
 drivers/soc/xilinx/zynqmp/pm_domains.c | 339 +++++++++++++++++++++++++++++++++
 5 files changed, 363 insertions(+)
 create mode 100644 drivers/soc/xilinx/zynqmp/Kconfig
 create mode 100644 drivers/soc/xilinx/zynqmp/Makefile
 create mode 100644 drivers/soc/xilinx/zynqmp/pm_domains.c

diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig
index 687c8f3..1c98d7f 100644
--- a/drivers/soc/xilinx/Kconfig
+++ b/drivers/soc/xilinx/Kconfig
@@ -17,4 +17,6 @@ config XILINX_VCU
 	  To compile this driver as a module, choose M here: the
 	  module will be called xlnx_vcu.
 
+source "drivers/soc/xilinx/zynqmp/Kconfig"
+
 endmenu
diff --git a/drivers/soc/xilinx/Makefile b/drivers/soc/xilinx/Makefile
index dee8fd5..e132897 100644
--- a/drivers/soc/xilinx/Makefile
+++ b/drivers/soc/xilinx/Makefile
@@ -1,2 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_XILINX_VCU)	+= xlnx_vcu.o
+
+obj-$(CONFIG_ARCH_ZYNQMP)	+= zynqmp/
diff --git a/drivers/soc/xilinx/zynqmp/Kconfig b/drivers/soc/xilinx/zynqmp/Kconfig
new file mode 100644
index 0000000..95d8665
--- /dev/null
+++ b/drivers/soc/xilinx/zynqmp/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Kconfig for Xilinx zynqmp SoC
+
+menu "Zynq MPSoC SoC Drivers"
+	depends on ARCH_ZYNQMP
+
+config ZYNQMP_PM_DOMAINS
+	bool "Enable Zynq MPSoC generic PM domains"
+	default y
+	depends on PM
+	select PM_GENERIC_DOMAINS
+	help
+	  Say yes to enable device power management through PM domains
+	  In doubt, say N
+
+endmenu
diff --git a/drivers/soc/xilinx/zynqmp/Makefile b/drivers/soc/xilinx/zynqmp/Makefile
new file mode 100644
index 0000000..0bd522b
--- /dev/null
+++ b/drivers/soc/xilinx/zynqmp/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Makefile for Xilinx zynqmp SoC
+
+obj-$(CONFIG_ZYNQMP_PM_DOMAINS) += pm_domains.o
diff --git a/drivers/soc/xilinx/zynqmp/pm_domains.c b/drivers/soc/xilinx/zynqmp/pm_domains.c
new file mode 100644
index 0000000..7ddca8e
--- /dev/null
+++ b/drivers/soc/xilinx/zynqmp/pm_domains.c
@@ -0,0 +1,339 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ZynqMP Generic PM domain support
+ *
+ *  Copyright (C) 2015-2018 Xilinx, Inc.
+ *
+ *  Davorin Mista <davorin.mista@aggios.com>
+ *  Jolly Shah <jollys@xilinx.com>
+ *  Rajan Vaja <rajanv@xilinx.com>
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/firmware/xilinx/zynqmp/firmware.h>
+
+/* Flag stating if PM nodes mapped to the PM domain has been requested */
+#define ZYNQMP_PM_DOMAIN_REQUESTED	BIT(0)
+
+/**
+ * struct zynqmp_pm_domain - Wrapper around struct generic_pm_domain
+ * @gpd:		Generic power domain
+ * @dev_list:		List of devices belong to power domain
+ * @node_ids:		PM node IDs corresponding to device(s) inside PM domain
+ * @node_id_num:	Number of PM node IDs
+ * @flags:		ZynqMP PM domain flags
+ */
+struct zynqmp_pm_domain {
+	struct generic_pm_domain gpd;
+	struct list_head dev_list;
+	u32 *node_ids;
+	int node_id_num;
+	u8 flags;
+};
+
+/*
+ * struct zynqmp_domain_device - Device node present in power domain
+ * @dev: Device
+ * &list: List member for the devices in domain list
+ */
+struct zynqmp_domain_device {
+	struct device *dev;
+	struct list_head list;
+};
+
+/**
+ * zynqmp_gpd_is_active_wakeup_path - Check if device is in wakeup source path
+ * @dev: Device to check for wakeup source path
+ * @not_used: Data member (not required)
+ *
+ * This function is checks device's child hierarchy and checks if any device is
+ * set as wakeup source.
+ *
+ * Return:	1 if device is in wakeup source path else 0.
+ */
+static int zynqmp_gpd_is_active_wakeup_path(struct device *dev, void *not_used)
+{
+	int may_wakeup;
+
+	may_wakeup = device_may_wakeup(dev);
+	if (may_wakeup)
+		return may_wakeup;
+
+	return device_for_each_child(dev, NULL,
+			zynqmp_gpd_is_active_wakeup_path);
+}
+
+/**
+ * zynqmp_gpd_power_on - Power on PM domain
+ * @domain:	Generic PM domain
+ *
+ * This function is called before devices inside a PM domain are resumed, to
+ * power on PM domain.
+ *
+ * Return:	0 on success, error code otherwise.
+ */
+static int zynqmp_gpd_power_on(struct generic_pm_domain *domain)
+{
+	int i, status = 0;
+	struct zynqmp_pm_domain *pd;
+	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+	if (!eemi_ops || !eemi_ops->set_requirement)
+		return status;
+
+	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
+	for (i = 0; i < pd->node_id_num; i++) {
+		status = eemi_ops->set_requirement(pd->node_ids[i],
+					ZYNQMP_PM_CAPABILITY_ACCESS,
+					ZYNQMP_PM_MAX_QOS,
+					ZYNQMP_PM_REQUEST_ACK_BLOCKING);
+		if (status)
+			break;
+	}
+	return status;
+}
+
+/**
+ * zynqmp_gpd_power_off - Power off PM domain
+ * @domain:	Generic PM domain
+ *
+ * This function is called after devices inside a PM domain are suspended, to
+ * power off PM domain.
+ *
+ * Return:	0 on success, error code otherwise.
+ */
+static int zynqmp_gpd_power_off(struct generic_pm_domain *domain)
+{
+	int i, status = 0;
+	struct zynqmp_pm_domain *pd;
+	struct zynqmp_domain_device *zdev, *tmp;
+	u32 capabilities = 0;
+	bool may_wakeup = 0;
+	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+	if (!eemi_ops || !eemi_ops->set_requirement)
+		return status;
+
+	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
+
+	/* If domain is already released there is nothing to be done */
+	if (!(pd->flags & ZYNQMP_PM_DOMAIN_REQUESTED))
+		return 0;
+
+	list_for_each_entry_safe(zdev, tmp, &pd->dev_list, list) {
+		/* If device is in wakeup path, set capability to WAKEUP */
+		may_wakeup = zynqmp_gpd_is_active_wakeup_path(zdev->dev, NULL);
+		if (may_wakeup) {
+			dev_dbg(zdev->dev, "device is in wakeup path in %s\n",
+				domain->name);
+			capabilities = ZYNQMP_PM_CAPABILITY_WAKEUP;
+			break;
+		}
+	}
+
+	for (i = pd->node_id_num - 1; i >= 0; i--) {
+		status = eemi_ops->set_requirement(pd->node_ids[i],
+						   capabilities, 0,
+						   ZYNQMP_PM_REQUEST_ACK_NO);
+		/**
+		 * If powering down of any node inside this domain fails,
+		 * report and return the error
+		 */
+		if (status) {
+			pr_err("%s error %d, node %u\n", __func__, status,
+			       pd->node_ids[i]);
+			return status;
+		}
+	}
+
+	return status;
+}
+
+/**
+ * zynqmp_gpd_attach_dev - Attach device to the PM domain
+ * @domain:	Generic PM domain
+ * @dev:	Device to attach
+ *
+ * Return:	0 on success, error code otherwise.
+ */
+static int zynqmp_gpd_attach_dev(struct generic_pm_domain *domain,
+				 struct device *dev)
+{
+	int i, status;
+	struct zynqmp_pm_domain *pd;
+	struct zynqmp_domain_device *zdev;
+	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+	if (!eemi_ops || !eemi_ops->request_node)
+		return -ENXIO;
+
+	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
+
+	zdev = devm_kzalloc(dev, sizeof(*zdev), GFP_KERNEL);
+	if (!zdev)
+		return -ENOMEM;
+
+	zdev->dev = dev;
+	list_add(&zdev->list, &pd->dev_list);
+
+	/* If this is not the first device to attach there is nothing to do */
+	if (domain->device_count)
+		return 0;
+
+	for (i = 0; i < pd->node_id_num; i++) {
+		status = eemi_ops->request_node(pd->node_ids[i], 0, 0,
+						ZYNQMP_PM_REQUEST_ACK_BLOCKING);
+		/* If requesting a node fails print and return the error */
+		if (status) {
+			pr_err("%s error %d, node %u\n", __func__,
+			       status, pd->node_ids[i]);
+			list_del(&zdev->list);
+			zdev->dev = NULL;
+			devm_kfree(dev, zdev);
+			return status;
+		}
+	}
+
+	pd->flags |= ZYNQMP_PM_DOMAIN_REQUESTED;
+
+	return 0;
+}
+
+/**
+ * zynqmp_gpd_detach_dev - Detach device from the PM domain
+ * @domain:	Generic PM domain
+ * @dev:	Device to detach
+ */
+static void zynqmp_gpd_detach_dev(struct generic_pm_domain *domain,
+				  struct device *dev)
+{
+	int i, status;
+	struct zynqmp_pm_domain *pd;
+	struct zynqmp_domain_device *zdev, *tmp;
+	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+	if (!eemi_ops || !eemi_ops->release_node)
+		return;
+
+	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
+
+	list_for_each_entry_safe(zdev, tmp, &pd->dev_list, list)
+		if (zdev->dev == dev) {
+			list_del(&zdev->list);
+			zdev->dev = NULL;
+			devm_kfree(dev, zdev);
+		}
+
+	/* If this is not the last device to detach there is nothing to do */
+	if (domain->device_count)
+		return;
+
+	for (i = 0; i < pd->node_id_num; i++) {
+		status = eemi_ops->release_node(pd->node_ids[i]);
+		/* If releasing a node fails print the error and return */
+		if (status) {
+			pr_err("%s error %d, node %u\n", __func__, status,
+			       pd->node_ids[i]);
+			return;
+		}
+	}
+
+	pd->flags &= ~ZYNQMP_PM_DOMAIN_REQUESTED;
+}
+
+/**
+ * zynqmp_gpd_probe - Initialize ZynqMP specific PM domains
+ * @pdev:	Platform device pointer
+ *
+ * Description:	This function populates struct zynqmp_pm_domain for each PM
+ * domain and initalizes generic PM domain. If the "pd-id" DT property
+ * of a certain domain is missing or invalid, that domain will be skipped.
+ *
+ * Return:	0 on success, error code otherwise.
+ */
+static int __init zynqmp_gpd_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device_node *child_err, *child, *np = pdev->dev.of_node;
+
+	for_each_child_of_node(np, child) {
+		struct zynqmp_pm_domain *pd;
+
+		pd = devm_kzalloc(&pdev->dev, sizeof(*pd), GFP_KERNEL);
+		if (!pd) {
+			ret = -ENOMEM;
+			goto err_cleanup;
+		}
+
+		ret = of_property_count_u32_elems(child, "pd-id");
+		if (ret <= 0)
+			goto err_cleanup;
+
+		pd->node_id_num = ret;
+		pd->node_ids = devm_kcalloc(&pdev->dev, ret,
+					    sizeof(*pd->node_ids), GFP_KERNEL);
+		if (!pd->node_ids) {
+			ret = -ENOMEM;
+			goto err_cleanup;
+		}
+
+		ret = of_property_read_u32_array(child, "pd-id", pd->node_ids,
+						 pd->node_id_num);
+		if (ret)
+			goto err_cleanup;
+
+		pd->gpd.name = kstrdup(child->name, GFP_KERNEL);
+		pd->gpd.power_off = zynqmp_gpd_power_off;
+		pd->gpd.power_on = zynqmp_gpd_power_on;
+		pd->gpd.attach_dev = zynqmp_gpd_attach_dev;
+		pd->gpd.detach_dev = zynqmp_gpd_detach_dev;
+
+		/* Mark all PM domains as initially powered off */
+		pm_genpd_init(&pd->gpd, NULL, true);
+
+		ret = of_genpd_add_provider_simple(child, &pd->gpd);
+		if (ret)
+			goto err_cleanup;
+
+		INIT_LIST_HEAD(&pd->dev_list);
+	}
+
+	return 0;
+
+err_cleanup:
+	child_err = child;
+	for_each_child_of_node(np, child) {
+		if (child == child_err)
+			break;
+		of_genpd_del_provider(child);
+	}
+
+	return ret;
+}
+
+static const struct of_device_id zynqmp_gpd_of_match[] = {
+	{ .compatible = "xlnx,zynqmp-genpd" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, zynqmp_gpd_of_match);
+
+static struct platform_driver zynqmp_gpd_platform_driver = {
+	.driver	= {
+		.name = "zynqmp_gpd",
+		.of_match_table = zynqmp_gpd_of_match,
+	},
+};
+
+static __init int zynqmp_gpd_init(void)
+{
+	return platform_driver_probe(&zynqmp_gpd_platform_driver,
+				     zynqmp_gpd_probe);
+}
+subsys_initcall(zynqmp_gpd_init);
-- 
2.7.4

Re: [PATCH 2/2] drivers: soc: xilinx: Add ZynqMP power domain driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

Hi Jolly,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.16-rc3 next-20180302]
[cannot apply to xlnx/master mediatek/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jolly-Shah/drivers-soc-xilinx-Add-support-for-ZynqMP-power-domain-driver/20180302-213151
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/soc/xilinx/zynqmp/pm_domains.c:19:10: fatal error: linux/firmware/xilinx/zynqmp/firmware.h: No such file or directory
    #include <linux/firmware/xilinx/zynqmp/firmware.h>
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   compilation terminated.

vim +19 drivers/soc/xilinx/zynqmp/pm_domains.c

  > 19	#include <linux/firmware/xilinx/zynqmp/firmware.h>
    20	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37892 bytes --]

Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Rob Herring]

On Tue, Feb 27, 2018 at 03:55:49PM -0800, Jolly Shah wrote:
> Add documentation to describe ZynqMP power domain bindings.
> 
> Signed-off-by: Jolly Shah <jollys@xilinx.com>
> Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> ---
>  .../devicetree/bindings/power/zynqmp-genpd.txt     | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/zynqmp-genpd.txt b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> new file mode 100644
> index 0000000..25f9711
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> @@ -0,0 +1,46 @@
> +Device Tree bindings for Xilinx Zynq MPSoC PM domains
> +
> +The binding for zynqmp-genpd follow the common generic PM domain binding[1].
> +
> +[1] Documentation/devicetree/bindings/power/power_domain.txt
> +
> +== Zynq MPSoC Generic PM Domain Node ==
> +
> +Required properties:
> + - compatible: Must be: "xlnx,zynqmp-genpd"

genpd is a Linux term. The DT should contain a power controller that 
controls physical power islands on a chip.

> +
> +This node contains a number of subnodes, each representing a single PM domain
> +that PM domain consumer devices reference.
> +
> +== PM Domain Nodes ==
> +
> +Required properties:
> + - #power-domain-cells: Number of cells in a PM domain specifier. Must be 0.
> + - pd-id: List of domain identifiers of as defined by platform firmware. These
> +	  identifiers are passed to the PM firmware.
> +
> +Example:
> +	zynqmp-genpd {
> +		compatible = "xlnx,zynqmp-genpd";

What's the control interface for controlling the domains?
> +
> +		pd_usb0: pd-usb0 {
> +			pd-id = <22>;
> +			#power-domain-cells = <0>;

There's no need for all these sub nodes. Make #power-domain-cells 1 and 
put the id in the cell value.

> +		};
> +
> +		pd_sata: pd-sata {
> +			pd-id = <28>;
> +			#power-domain-cells = <0>;
> +		};
> +
> +		pd_gpu: pd-gpu {
> +			pd-id = <58 20 21>;
> +			#power-domain-cells = <0x0>;
> +		};
> +	};
> +
> +	sata0: ahci@SATA_AHCI_HBA {
> +		...
> +		power-domains = <&pd_sata>;
> +		...
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Geert Uytterhoeven]

Hi Rob, Jolly,

On Mon, Mar 5, 2018 at 11:39 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Feb 27, 2018 at 03:55:49PM -0800, Jolly Shah wrote:
>> Add documentation to describe ZynqMP power domain bindings.
>>
>> Signed-off-by: Jolly Shah <jollys@xilinx.com>
>> Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
>> ---
>>  .../devicetree/bindings/power/zynqmp-genpd.txt     | 46 ++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/zynqmp-genpd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/zynqmp-genpd.txt b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
>> new file mode 100644
>> index 0000000..25f9711
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt

>> +This node contains a number of subnodes, each representing a single PM domain
>> +that PM domain consumer devices reference.
>> +
>> +== PM Domain Nodes ==
>> +
>> +Required properties:
>> + - #power-domain-cells: Number of cells in a PM domain specifier. Must be 0.
>> + - pd-id: List of domain identifiers of as defined by platform firmware. These
>> +       identifiers are passed to the PM firmware.
>> +
>> +Example:
>> +     zynqmp-genpd {
>> +             compatible = "xlnx,zynqmp-genpd";
>
> What's the control interface for controlling the domains?
>> +
>> +             pd_usb0: pd-usb0 {
>> +                     pd-id = <22>;
>> +                     #power-domain-cells = <0>;
>
> There's no need for all these sub nodes. Make #power-domain-cells 1 and
> put the id in the cell value.

That was my first reaction, too...
>
>> +             };
>> +
>> +             pd_sata: pd-sata {
>> +                     pd-id = <28>;
>> +                     #power-domain-cells = <0>;
>> +             };
>> +
>> +             pd_gpu: pd-gpu {
>> +                     pd-id = <58 20 21>;

... until I saw the above.
Controlling the GPU power area requires controlling 3 physical areas?

However, doing it this way may bite you in the future, if a need arises to
control a subset. And what about power up/down order?

>> +                     #power-domain-cells = <0x0>;
>> +             };
>> +     };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Marek Szyprowski]

Hi All,

On 2018-03-06 08:59, Geert Uytterhoeven wrote:
> Hi Rob, Jolly,
>
> On Mon, Mar 5, 2018 at 11:39 PM, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Feb 27, 2018 at 03:55:49PM -0800, Jolly Shah wrote:
>>> Add documentation to describe ZynqMP power domain bindings.
>>>
>>> Signed-off-by: Jolly Shah <jollys@xilinx.com>
>>> Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
>>> ---
>>>   .../devicetree/bindings/power/zynqmp-genpd.txt     | 46 ++++++++++++++++++++++
>>>   1 file changed, 46 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/power/zynqmp-genpd.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/zynqmp-genpd.txt b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
>>> new file mode 100644
>>> index 0000000..25f9711
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
>>> +This node contains a number of subnodes, each representing a single PM domain
>>> +that PM domain consumer devices reference.
>>> +
>>> +== PM Domain Nodes ==
>>> +
>>> +Required properties:
>>> + - #power-domain-cells: Number of cells in a PM domain specifier. Must be 0.
>>> + - pd-id: List of domain identifiers of as defined by platform firmware. These
>>> +       identifiers are passed to the PM firmware.
>>> +
>>> +Example:
>>> +     zynqmp-genpd {
>>> +             compatible = "xlnx,zynqmp-genpd";
>> What's the control interface for controlling the domains?
>>> +
>>> +             pd_usb0: pd-usb0 {
>>> +                     pd-id = <22>;
>>> +                     #power-domain-cells = <0>;
>> There's no need for all these sub nodes. Make #power-domain-cells 1 and
>> put the id in the cell value.
> That was my first reaction, too...
>>> +             };
>>> +
>>> +             pd_sata: pd-sata {
>>> +                     pd-id = <28>;
>>> +                     #power-domain-cells = <0>;
>>> +             };
>>> +
>>> +             pd_gpu: pd-gpu {
>>> +                     pd-id = <58 20 21>;
> ... until I saw the above.
> Controlling the GPU power area requires controlling 3 physical areas?
>
> However, doing it this way may bite you in the future, if a need arises to
> control a subset. And what about power up/down order?

What about defining 3 separate domains and arranging them in parent-child
relationship? generic power domains already supports that and this allows
to nicely define the power on/off order.

>>> +                     #power-domain-cells = <0x0>;
>>> +             };
>>> +     };

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

RE: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Jolly Shah]

Hi Rob, Geert, Marek,

> -----Original Message-----
> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Tuesday, March 06, 2018 12:06 AM
> To: Geert Uytterhoeven <geert@linux-m68k.org>; Rob Herring
> <robh@kernel.org>
> Cc: Jolly Shah <JOLLYS@xilinx.com>; Matthias Brugger
> <matthias.bgg@gmail.com>; Andy Gross <andy.gross@linaro.org>; Shawn Guo
> <shawnguo@kernel.org>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Björn Andersson <bjorn.andersson@linaro.org>; sean.wang@mediatek.com;
> Michal Simek <michal.simek@xilinx.com>; Mark Rutland
> <mark.rutland@arm.com>; Rajan Vaja <RAJANV@xilinx.com>; open list:OPEN
> FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> <devicetree@vger.kernel.org>; Linux ARM <linux-arm-
> kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Jolly Shah <JOLLYS@xilinx.com>
> Subject: Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain
> bindings
> 
> Hi All,
> 
> On 2018-03-06 08:59, Geert Uytterhoeven wrote:
> > Hi Rob, Jolly,
> >
> > On Mon, Mar 5, 2018 at 11:39 PM, Rob Herring <robh@kernel.org> wrote:
> >> On Tue, Feb 27, 2018 at 03:55:49PM -0800, Jolly Shah wrote:
> >>> Add documentation to describe ZynqMP power domain bindings.
> >>>
> >>> Signed-off-by: Jolly Shah <jollys@xilinx.com>
> >>> Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> >>> ---
> >>>   .../devicetree/bindings/power/zynqmp-genpd.txt     | 46
> ++++++++++++++++++++++
> >>>   1 file changed, 46 insertions(+)
> >>>   create mode 100644
> >>> Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> >>> b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> >>> new file mode 100644
> >>> index 0000000..25f9711
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> >>> +This node contains a number of subnodes, each representing a single
> >>> +PM domain that PM domain consumer devices reference.
> >>> +
> >>> +== PM Domain Nodes ==
> >>> +
> >>> +Required properties:
> >>> + - #power-domain-cells: Number of cells in a PM domain specifier. Must be
> 0.
> >>> + - pd-id: List of domain identifiers of as defined by platform firmware.
> These
> >>> +       identifiers are passed to the PM firmware.
> >>> +
> >>> +Example:
> >>> +     zynqmp-genpd {
> >>> +             compatible = "xlnx,zynqmp-genpd";
> >> What's the control interface for controlling the domains?
> >>> +
> >>> +             pd_usb0: pd-usb0 {
> >>> +                     pd-id = <22>;
> >>> +                     #power-domain-cells = <0>;
> >> There's no need for all these sub nodes. Make #power-domain-cells 1
> >> and put the id in the cell value.
> > That was my first reaction, too...
> >>> +             };
> >>> +
> >>> +             pd_sata: pd-sata {
> >>> +                     pd-id = <28>;
> >>> +                     #power-domain-cells = <0>;
> >>> +             };
> >>> +
> >>> +             pd_gpu: pd-gpu {
> >>> +                     pd-id = <58 20 21>;
> > ... until I saw the above.
> > Controlling the GPU power area requires controlling 3 physical areas?
> >
> > However, doing it this way may bite you in the future, if a need
> > arises to control a subset. And what about power up/down order?
> 
> What about defining 3 separate domains and arranging them in parent-child
> relationship? generic power domains already supports that and this allows to
> nicely define the power on/off order.
> 
> >>> +                     #power-domain-cells = <0x0>;
> >>> +             };
> >>> +     };
> 

I agree it should be arranged in as parent child order to control subset or control order. Will incorporate those changes in next version.

> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland

RE: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Jolly Shah]

Hi Marek,

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Jolly Shah
> Sent: Thursday, March 15, 2018 10:47 AM
> To: Marek Szyprowski <m.szyprowski@samsung.com>; Geert Uytterhoeven
> <geert@linux-m68k.org>; Rob Herring <robh@kernel.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>; Andy Gross
> <andy.gross@linaro.org>; Shawn Guo <shawnguo@kernel.org>; Geert
> Uytterhoeven <geert+renesas@glider.be>; Björn Andersson
> <bjorn.andersson@linaro.org>; sean.wang@mediatek.com; Michal Simek
> <michal.simek@xilinx.com>; Mark Rutland <mark.rutland@arm.com>; Rajan
> Vaja <RAJANV@xilinx.com>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; Linux ARM <linux-arm-
> kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>
> Subject: RE: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain
> bindings
> 
> [This sender failed our fraud detection checks and may not be who they appear
> to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
> 
> Hi Rob, Geert, Marek,
> 
> > -----Original Message-----
> > From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> > Sent: Tuesday, March 06, 2018 12:06 AM
> > To: Geert Uytterhoeven <geert@linux-m68k.org>; Rob Herring
> > <robh@kernel.org>
> > Cc: Jolly Shah <JOLLYS@xilinx.com>; Matthias Brugger
> > <matthias.bgg@gmail.com>; Andy Gross <andy.gross@linaro.org>; Shawn
> > Guo <shawnguo@kernel.org>; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Björn Andersson
> > <bjorn.andersson@linaro.org>; sean.wang@mediatek.com; Michal Simek
> > <michal.simek@xilinx.com>; Mark Rutland <mark.rutland@arm.com>; Rajan
> > Vaja <RAJANV@xilinx.com>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE
> > TREE BINDINGS <devicetree@vger.kernel.org>; Linux ARM <linux-arm-
> > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; Jolly Shah <JOLLYS@xilinx.com>
> > Subject: Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain
> > bindings
> >
> > Hi All,
> >
> > On 2018-03-06 08:59, Geert Uytterhoeven wrote:
> > > Hi Rob, Jolly,
> > >
> > > On Mon, Mar 5, 2018 at 11:39 PM, Rob Herring <robh@kernel.org> wrote:
> > >> On Tue, Feb 27, 2018 at 03:55:49PM -0800, Jolly Shah wrote:
> > >>> Add documentation to describe ZynqMP power domain bindings.
> > >>>
> > >>> Signed-off-by: Jolly Shah <jollys@xilinx.com>
> > >>> Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> > >>> ---
> > >>>   .../devicetree/bindings/power/zynqmp-genpd.txt     | 46
> > ++++++++++++++++++++++
> > >>>   1 file changed, 46 insertions(+)
> > >>>   create mode 100644
> > >>> Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> > >>>
> > >>> diff --git
> > >>> a/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> > >>> b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> > >>> new file mode 100644
> > >>> index 0000000..25f9711
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> > >>> +This node contains a number of subnodes, each representing a
> > >>> +single PM domain that PM domain consumer devices reference.
> > >>> +
> > >>> +== PM Domain Nodes ==
> > >>> +
> > >>> +Required properties:
> > >>> + - #power-domain-cells: Number of cells in a PM domain specifier.
> > >>> +Must be
> > 0.
> > >>> + - pd-id: List of domain identifiers of as defined by platform firmware.
> > These
> > >>> +       identifiers are passed to the PM firmware.
> > >>> +
> > >>> +Example:
> > >>> +     zynqmp-genpd {
> > >>> +             compatible = "xlnx,zynqmp-genpd";
> > >> What's the control interface for controlling the domains?
> > >>> +
> > >>> +             pd_usb0: pd-usb0 {
> > >>> +                     pd-id = <22>;
> > >>> +                     #power-domain-cells = <0>;
> > >> There's no need for all these sub nodes. Make #power-domain-cells 1
> > >> and put the id in the cell value.
> > > That was my first reaction, too...
> > >>> +             };
> > >>> +
> > >>> +             pd_sata: pd-sata {
> > >>> +                     pd-id = <28>;
> > >>> +                     #power-domain-cells = <0>;
> > >>> +             };
> > >>> +
> > >>> +             pd_gpu: pd-gpu {
> > >>> +                     pd-id = <58 20 21>;
> > > ... until I saw the above.
> > > Controlling the GPU power area requires controlling 3 physical areas?
> > >
> > > However, doing it this way may bite you in the future, if a need
> > > arises to control a subset. And what about power up/down order?
> >
> > What about defining 3 separate domains and arranging them in
> > parent-child relationship? generic power domains already supports that
> > and this allows to nicely define the power on/off order.
> >
> > >>> +                     #power-domain-cells = <0x0>;
> > >>> +             };
> > >>> +     };
> >
> 
> I agree it should be arranged in as parent child order to control subset or control
> order. Will incorporate those changes in next version.
> 
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R&D Institute Poland


As suggested, I tried out parent, child approach. However what I found is Genpd core takes care of parent child dependencies for power on off routines only. In our case, We need them in attach-detach routines too. In that case, we need to handle dependencies manually for those routines. Please suggest better approach, if any.

Thanks,
Jolly Shah

Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Marek Szyprowski]

Hi Jolly,

On 2018-05-17 23:10, Jolly Shah wrote:

>>>>>> +Example:
>>>>>> +     zynqmp-genpd {
>>>>>> +             compatible = "xlnx,zynqmp-genpd";
>>>>> What's the control interface for controlling the domains?
>>>>>> +
>>>>>> +             pd_usb0: pd-usb0 {
>>>>>> +                     pd-id = <22>;
>>>>>> +                     #power-domain-cells = <0>;
>>>>> There's no need for all these sub nodes. Make #power-domain-cells 1
>>>>> and put the id in the cell value.
>>>> That was my first reaction, too...
>>>>>> +             };
>>>>>> +
>>>>>> +             pd_sata: pd-sata {
>>>>>> +                     pd-id = <28>;
>>>>>> +                     #power-domain-cells = <0>;
>>>>>> +             };
>>>>>> +
>>>>>> +             pd_gpu: pd-gpu {
>>>>>> +                     pd-id = <58 20 21>;
>>>> ... until I saw the above.
>>>> Controlling the GPU power area requires controlling 3 physical areas?
>>>>
>>>> However, doing it this way may bite you in the future, if a need
>>>> arises to control a subset. And what about power up/down order?
>>> What about defining 3 separate domains and arranging them in
>>> parent-child relationship? generic power domains already supports that
>>> and this allows to nicely define the power on/off order.
>>>
>>>>>> +                     #power-domain-cells = <0x0>;
>>>>>> +             };
>>>>>> +     };
>> I agree it should be arranged in as parent child order to control subset or control
>> order. Will incorporate those changes in next version.
>
> As suggested, I tried out parent, child approach. However what I found is Genpd core takes care of parent child dependencies for power on off routines only. In our case, We need them in attach-detach routines too. In that case, we need to handle dependencies manually for those routines. Please suggest better approach, if any.

What do you mean to handle attach-detach?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

RE: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Jolly Shah]

Hi Marek,

> -----Original Message-----
> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> Sent: Thursday, May 17, 2018 11:31 PM
> To: Jolly Shah <JOLLYS@xilinx.com>; Geert Uytterhoeven <geert@linux-
> m68k.org>; Rob Herring <robh@kernel.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>; Andy Gross
> <andy.gross@linaro.org>; Shawn Guo <shawnguo@kernel.org>; Geert
> Uytterhoeven <geert+renesas@glider.be>; Björn Andersson
> <bjorn.andersson@linaro.org>; sean.wang@mediatek.com; Michal Simek
> <michal.simek@xilinx.com>; Mark Rutland <mark.rutland@arm.com>; Rajan
> Vaja <RAJANV@xilinx.com>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; Linux ARM <linux-arm-
> kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>
> Subject: Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain
> bindings
> 
> Hi Jolly,
> 
> On 2018-05-17 23:10, Jolly Shah wrote:
> 
> >>>>>> +Example:
> >>>>>> +     zynqmp-genpd {
> >>>>>> +             compatible = "xlnx,zynqmp-genpd";
> >>>>> What's the control interface for controlling the domains?
> >>>>>> +
> >>>>>> +             pd_usb0: pd-usb0 {
> >>>>>> +                     pd-id = <22>;
> >>>>>> +                     #power-domain-cells = <0>;
> >>>>> There's no need for all these sub nodes. Make #power-domain-cells
> >>>>> 1 and put the id in the cell value.
> >>>> That was my first reaction, too...
> >>>>>> +             };
> >>>>>> +
> >>>>>> +             pd_sata: pd-sata {
> >>>>>> +                     pd-id = <28>;
> >>>>>> +                     #power-domain-cells = <0>;
> >>>>>> +             };
> >>>>>> +
> >>>>>> +             pd_gpu: pd-gpu {
> >>>>>> +                     pd-id = <58 20 21>;
> >>>> ... until I saw the above.
> >>>> Controlling the GPU power area requires controlling 3 physical areas?
> >>>>
> >>>> However, doing it this way may bite you in the future, if a need
> >>>> arises to control a subset. And what about power up/down order?
> >>> What about defining 3 separate domains and arranging them in
> >>> parent-child relationship? generic power domains already supports
> >>> that and this allows to nicely define the power on/off order.
> >>>
> >>>>>> +                     #power-domain-cells = <0x0>;
> >>>>>> +             };
> >>>>>> +     };
> >> I agree it should be arranged in as parent child order to control
> >> subset or control order. Will incorporate those changes in next version.
> >
> > As suggested, I tried out parent, child approach. However what I found is
> Genpd core takes care of parent child dependencies for power on off routines
> only. In our case, We need them in attach-detach routines too. In that case, we
> need to handle dependencies manually for those routines. Please suggest better
> approach, if any.
> 
> What do you mean to handle attach-detach?
> 
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland

For our power domain driver,  we request usage of these nodes in attach routines and power them on in power on routine. So for below specific case, when attach_dev is called, all 3 nodes need to be requested.

> >>>>>> +             pd_gpu: pd-gpu {
> >>>>>> +                     pd-id = <58 20 21>;

Thanks,
Jolly Shah

RE: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings

[Jolly Shah]

Hi Marek,

> -----Original Message-----
> From: Jolly Shah
> Sent: Friday, May 18, 2018 2:19 PM
> To: 'Marek Szyprowski' <m.szyprowski@samsung.com>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>; Andy Gross
> <andy.gross@linaro.org>; Shawn Guo <shawnguo@kernel.org>; Geert
> Uytterhoeven <geert+renesas@glider.be>; Björn Andersson
> <bjorn.andersson@linaro.org>; sean.wang@mediatek.com; Michal Simek
> <michal.simek@xilinx.com>; Mark Rutland <mark.rutland@arm.com>; Rajan
> Vaja <RAJANV@xilinx.com>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; Linux ARM <linux-arm-
> kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Geert Uytterhoeven <geert@linux-m68k.org>; Rob
> Herring <robh@kernel.org>
> Subject: RE: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain
> bindings
> 
> Hi Marek,
> 
> > -----Original Message-----
> > From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> > Sent: Thursday, May 17, 2018 11:31 PM
> > To: Jolly Shah <JOLLYS@xilinx.com>; Geert Uytterhoeven <geert@linux-
> > m68k.org>; Rob Herring <robh@kernel.org>
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>; Andy Gross
> > <andy.gross@linaro.org>; Shawn Guo <shawnguo@kernel.org>; Geert
> > Uytterhoeven <geert+renesas@glider.be>; Björn Andersson
> > <bjorn.andersson@linaro.org>; sean.wang@mediatek.com; Michal Simek
> > <michal.simek@xilinx.com>; Mark Rutland <mark.rutland@arm.com>; Rajan
> > Vaja <RAJANV@xilinx.com>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE
> > TREE BINDINGS <devicetree@vger.kernel.org>; Linux ARM <linux-arm-
> > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>
> > Subject: Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain
> > bindings
> >
> > Hi Jolly,
> >
> > On 2018-05-17 23:10, Jolly Shah wrote:
> >
> > >>>>>> +Example:
> > >>>>>> +     zynqmp-genpd {
> > >>>>>> +             compatible = "xlnx,zynqmp-genpd";
> > >>>>> What's the control interface for controlling the domains?
> > >>>>>> +
> > >>>>>> +             pd_usb0: pd-usb0 {
> > >>>>>> +                     pd-id = <22>;
> > >>>>>> +                     #power-domain-cells = <0>;
> > >>>>> There's no need for all these sub nodes. Make
> > >>>>> #power-domain-cells
> > >>>>> 1 and put the id in the cell value.
> > >>>> That was my first reaction, too...
> > >>>>>> +             };
> > >>>>>> +
> > >>>>>> +             pd_sata: pd-sata {
> > >>>>>> +                     pd-id = <28>;
> > >>>>>> +                     #power-domain-cells = <0>;
> > >>>>>> +             };
> > >>>>>> +
> > >>>>>> +             pd_gpu: pd-gpu {
> > >>>>>> +                     pd-id = <58 20 21>;
> > >>>> ... until I saw the above.
> > >>>> Controlling the GPU power area requires controlling 3 physical areas?
> > >>>>
> > >>>> However, doing it this way may bite you in the future, if a need
> > >>>> arises to control a subset. And what about power up/down order?
> > >>> What about defining 3 separate domains and arranging them in
> > >>> parent-child relationship? generic power domains already supports
> > >>> that and this allows to nicely define the power on/off order.
> > >>>
> > >>>>>> +                     #power-domain-cells = <0x0>;
> > >>>>>> +             };
> > >>>>>> +     };
> > >> I agree it should be arranged in as parent child order to control
> > >> subset or control order. Will incorporate those changes in next version.
> > >
> > > As suggested, I tried out parent, child approach. However what I
> > > found is
> > Genpd core takes care of parent child dependencies for power on off
> > routines only. In our case, We need them in attach-detach routines
> > too. In that case, we need to handle dependencies manually for those
> > routines. Please suggest better approach, if any.
> >
> > What do you mean to handle attach-detach?
> >
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R&D Institute Poland
> 
> For our power domain driver,  we request usage of these nodes in attach
> routines and power them on in power on routine. So for below specific case,
> when attach_dev is called, all 3 nodes need to be requested.
> 
> > >>>>>> +             pd_gpu: pd-gpu {
> > >>>>>> +                     pd-id = <58 20 21>;
> 
> Thanks,
> Jolly Shah
> 

Please let me know your opinion.

Thanks,
Jolly Shah