[PATCH 0/3] firmware: scpi: add device power domain support

[PATCH 0/3] firmware: scpi: add device power domain support

[Sudeep Holla]

This series add support for SCPI based device device power state
management using genpd.

Regards,
Sudeep

Sudeep Holla (3):
  firmware: arm_scpi: add support for device power state management
  Documentation: add DT bindings for ARM SCPI power domains
  firmware: scpi: add device power domain support using genpd

 Documentation/devicetree/bindings/arm/arm,scpi.txt |  34 +++++
 drivers/firmware/Kconfig                           |   8 ++
 drivers/firmware/Makefile                          |   1 +
 drivers/firmware/arm_scpi.c                        |  29 ++++
 drivers/firmware/scpi_pd.c                         | 152 +++++++++++++++++++++
 include/linux/scpi_protocol.h                      |   2 +
 6 files changed, 226 insertions(+)
 create mode 100644 drivers/firmware/scpi_pd.c

-- 
2.7.4

[PATCH 1/3] firmware: arm_scpi: add support for device power state management

[Sudeep Holla]

SCPI protocol supports device power state management. This deals with
power states of various peripheral devices in the system other than the
core compute subsystem.

This patch adds support for the power state management of those
peripheral devices.

Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scpi.c   | 29 +++++++++++++++++++++++++++++
 include/linux/scpi_protocol.h |  2 ++
 2 files changed, 31 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 51c6db0774cc..ca6afd2217a8 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -231,6 +231,11 @@ struct sensor_value {
 	__le32 hi_val;
 } __packed;

+struct dev_pstate_set {
+	u16 dev_id;
+	u8 pstate;
+} __packed;
+
 static struct scpi_drvinfo *scpi_info;

 static int scpi_linux_errmap[SCPI_ERR_MAX] = {
@@ -537,6 +542,28 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
 	return ret;
 }

+static int scpi_device_get_power_state(u16 dev_id)
+{
+	int ret;
+	u8 pstate;
+
+	ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &dev_id,
+				sizeof(dev_id), &pstate, sizeof(pstate));
+	return ret ? ret : pstate;
+}
+
+static int scpi_device_set_power_state(u16 dev_id, u8 pstate)
+{
+	int stat;
+	struct dev_pstate_set dev_set = {
+		.dev_id = cpu_to_le16(dev_id),
+		.pstate = pstate,
+	};
+
+	return scpi_send_message(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set,
+				 sizeof(dev_set), &stat, sizeof(stat));
+}
+
 static struct scpi_ops scpi_ops = {
 	.get_version = scpi_get_version,
 	.clk_get_range = scpi_clk_get_range,
@@ -548,6 +575,8 @@ static struct scpi_ops scpi_ops = {
 	.sensor_get_capability = scpi_sensor_get_capability,
 	.sensor_get_info = scpi_sensor_get_info,
 	.sensor_get_value = scpi_sensor_get_value,
+	.device_get_power_state = scpi_device_get_power_state,
+	.device_set_power_state = scpi_device_set_power_state,
 };

 struct scpi_ops *get_scpi_ops(void)
diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
index 35de50a65665..dc5f989be226 100644
--- a/include/linux/scpi_protocol.h
+++ b/include/linux/scpi_protocol.h
@@ -70,6 +70,8 @@ struct scpi_ops {
 	int (*sensor_get_capability)(u16 *sensors);
 	int (*sensor_get_info)(u16 sensor_id, struct scpi_sensor_info *);
 	int (*sensor_get_value)(u16, u64 *);
+	int (*device_get_power_state)(u16);
+	int (*device_set_power_state)(u16, u8);
 };

 #if IS_REACHABLE(CONFIG_ARM_SCPI_PROTOCOL)
--
2.7.4

[PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains

[Sudeep Holla]

The System Control Processor (SCP) provides peripheral devices with
power domains that can be enabled and disabled viathe System Control
and Power Interface (SCPI) Message Protocol. Add bindings to allow
probing of these device power domians.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
index 313dabdc14f9..7141670d649b 100644
--- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
@@ -87,10 +87,33 @@ SCPI provides an API to access the various sensors on the SoC.
 			 implementation for the IDs to use. For Juno
 			 R0 and Juno R1 refer to [3].

+Power domain bindings for the power domains based on SCPI Message Protocol
+------------------------------------------------------------
+
+This binding uses the generic power domain binding[4].
+
+PM domain providers
+===================
+
+Required properties:
+ - #power-domain-cells : Should be 1. Contains the device or the power
+			 domain ID value used by SCPI commands.
+ - num-domains: Total number of power domains provided by SCPI. This is
+		needed as the SCPI message protocol lacks a mechanism to
+		query this information runtime.
+
+PM domain consumers
+===================
+
+Required properties:
+ - power-domains : A phandle and PM domain specifier as defined by bindings of
+                   the power controller specified by phandle.
+
 [0] http://infocenter.arm.com/help/topic/com.arm.doc.dui0922b/index.html
 [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
 [2] Documentation/devicetree/bindings/thermal/thermal.txt
 [3] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922b/apas03s22.html
+[4] Documentation/devicetree/bindings/power/power_domain.txt

 Example:

@@ -144,6 +167,12 @@ scpi_protocol: scpi@2e000000 {
 		compatible = "arm,scpi-sensors";
 		#thermal-sensor-cells = <1>;
 	};
+
+	scpi_devpd: scpi-power-domains {
+		compatible = "arm,scpi-power-domains";
+		num-domains = <2>;
+		#power-domain-cells = <1>;
+	};
 };

 cpu@0 {
@@ -156,6 +185,7 @@ hdlcd@7ff60000 {
 	...
 	reg = <0 0x7ff60000 0 0x1000>;
 	clocks = <&scpi_clk 4>;
+	power-domains = <&scpi_devpd 1>;
 };

 thermal-zones {
@@ -186,3 +216,7 @@ The thermal-sensors property in the soc_thermal node uses the
 temperature sensor provided by SCP firmware to setup a thermal
 zone. The ID "3" is the sensor identifier for the temperature sensor
 as used by the firmware.
+
+The num-domains property in scpi-power-domains domain specifies that
+SCPI provides 2 power domains. The hdlcd node uses the power domain with
+domain ID 1.
--
2.7.4

[PATCH 3/3] firmware: scpi: add device power domain support using genpd

[Sudeep Holla]

This patch hooks up the support for device power domain provided by
SCPI using the Linux generic power domain infrastructure.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/Kconfig   |   8 +++
 drivers/firmware/Makefile  |   1 +
 drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+)
 create mode 100644 drivers/firmware/scpi_pd.c

Hi,

Since most of the power controller drivers are place in drivers/soc/<soc_name>,
I am not sure where to put this SCPI power domain code as it can be used
on multiple SoC. I have placed it in drivers/firmware temporarily for
review. Please suggest the most apt place to put this driver.

Regards,
Sudeep

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 41abdc54815e..80c963c60f13 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL
 	  This protocol library provides interface for all the client drivers
 	  making use of the features offered by the SCP.

+config ARM_SCPI_POWER_DOMAIN
+	tristate "SCPI power domain driver"
+	depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST
+	select PM_GENERIC_DOMAINS_OF
+	help
+	  This enables support for the SCPI power domains which can be
+	  enabled or disabled via the SCP firmware
+
 config EDD
 	tristate "BIOS Enhanced Disk Drive calls determine boot disk"
 	depends on X86
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 474bada56fcd..24f7fe8e3fc3 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -3,6 +3,7 @@
 #
 obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
 obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
+obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pd.o
 obj-$(CONFIG_DMI)		+= dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)		+= dmi-sysfs.o
 obj-$(CONFIG_EDD)		+= edd.o
diff --git a/drivers/firmware/scpi_pd.c b/drivers/firmware/scpi_pd.c
new file mode 100644
index 000000000000..fb24631d2b2e
--- /dev/null
+++ b/drivers/firmware/scpi_pd.c
@@ -0,0 +1,152 @@
+/*
+ * SCPI Generic power domain support.
+ *
+ * Copyright (C) 2016 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pm_domain.h>
+#include <linux/scpi_protocol.h>
+
+struct scpi_pm_domain {
+	struct generic_pm_domain genpd;
+	struct scpi_ops *ops;
+	u32 domain;
+	char name[20];
+};
+
+enum scpi_power_domain_state {
+	SCPI_PD_STATE_ON = 0,
+	SCPI_PD_STATE_OFF = 3,
+};
+
+#define to_scpi_pd(gpd) container_of(gpd, struct scpi_pm_domain, genpd)
+
+static int scpi_pd_power(struct scpi_pm_domain *pd, bool power_on)
+{
+	int ret;
+	enum scpi_power_domain_state state;
+
+	if (power_on)
+		state = SCPI_PD_STATE_ON;
+	else
+		state = SCPI_PD_STATE_OFF;
+
+	ret = pd->ops->device_set_power_state(pd->domain, state);
+	if (ret)
+		return ret;
+
+	return (state == pd->ops->device_get_power_state(pd->domain));
+}
+
+static int scpi_pd_power_on(struct generic_pm_domain *domain)
+{
+	struct scpi_pm_domain *pd = to_scpi_pd(domain);
+
+	return scpi_pd_power(pd, true);
+}
+
+static int scpi_pd_power_off(struct generic_pm_domain *domain)
+{
+	struct scpi_pm_domain *pd = to_scpi_pd(domain);
+
+	return scpi_pd_power(pd, false);
+}
+
+static int scpi_pm_domain_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct scpi_pm_domain *scpi_pd;
+	struct genpd_onecell_data *scpi_pd_data;
+	struct generic_pm_domain **domains;
+	struct scpi_ops *scpi_ops;
+	int ret, num_domains, i;
+
+	scpi_ops = get_scpi_ops();
+	if (!scpi_ops)
+		return -EPROBE_DEFER;
+
+	if (!np) {
+		dev_err(dev, "device tree node not found\n");
+		return -ENODEV;
+	}
+
+	ret = of_property_read_u32(np, "num-domains", &num_domains);
+	if (ret) {
+		dev_err(dev, "number of domains not found\n");
+		return -EINVAL;
+	}
+
+	scpi_pd = devm_kcalloc(dev, num_domains, sizeof(*scpi_pd), GFP_KERNEL);
+	if (!scpi_pd)
+		return -ENOMEM;
+
+	scpi_pd_data = devm_kzalloc(dev, sizeof(*scpi_pd_data), GFP_KERNEL);
+	if (!scpi_pd_data)
+		return -ENOMEM;
+
+	domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL);
+	if (!domains)
+		return -ENOMEM;
+
+	for (i = 0; i < num_domains; i++, scpi_pd++) {
+		domains[i] = &scpi_pd->genpd;
+
+		scpi_pd->domain = i;
+		scpi_pd->ops = scpi_ops;
+		sprintf(scpi_pd->name, "%s.%d", np->name, i);
+		scpi_pd->genpd.name = scpi_pd->name;
+		scpi_pd->genpd.power_off = scpi_pd_power_off;
+		scpi_pd->genpd.power_on = scpi_pd_power_on;
+
+		/*
+		 * Treat all power domains as off at boot.
+		 *
+		 * The SCP firmware itself may have switched on some domains,
+		 * but for reference counting purpose, keep it this way.
+		 */
+		pm_genpd_init(&scpi_pd->genpd, NULL, true);
+	}
+
+	scpi_pd_data->domains = domains;
+	scpi_pd_data->num_domains = num_domains;
+
+	of_genpd_add_provider_onecell(np, scpi_pd_data);
+
+	return 0;
+}
+
+static const struct of_device_id scpi_power_domain_ids[] = {
+	{ .compatible = "arm,scpi-power-domains", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, scpi_power_domain_ids);
+
+static struct platform_driver scpi_power_domain_driver = {
+	.driver	= {
+		.name = "scpi_power_domain",
+		.of_match_table = scpi_power_domain_ids,
+	},
+	.probe = scpi_pm_domain_probe,
+};
+module_platform_driver(scpi_power_domain_driver);
+
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_DESCRIPTION("ARM SCPI power domain driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4

Re: [PATCH 1/3] firmware: arm_scpi: add support for device power state management

[Jon Medhurst (Tixy)]

On Mon, 2016-06-06 at 16:53 +0100, Sudeep Holla wrote:
> SCPI protocol supports device power state management. This deals with
> power states of various peripheral devices in the system other than the
> core compute subsystem.
> 
> This patch adds support for the power state management of those
> peripheral devices.
> 
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scpi.c   | 29 +++++++++++++++++++++++++++++
>  include/linux/scpi_protocol.h |  2 ++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 51c6db0774cc..ca6afd2217a8 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -231,6 +231,11 @@ struct sensor_value {
>  	__le32 hi_val;
>  } __packed;
> 
> +struct dev_pstate_set {
> +	u16 dev_id;
> +	u8 pstate;
> +} __packed;
> +
>  static struct scpi_drvinfo *scpi_info;
> 
>  static int scpi_linux_errmap[SCPI_ERR_MAX] = {
> @@ -537,6 +542,28 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>  	return ret;
>  }
> 
> +static int scpi_device_get_power_state(u16 dev_id)
> +{
> +	int ret;
> +	u8 pstate;
> +
> +	ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &dev_id,
> +				sizeof(dev_id), &pstate, sizeof(pstate));

Don't you need to run translate dev_id to little-endian before sending
it? You remembered to do that next function...

> +	return ret ? ret : pstate;
> +}
> +
> +static int scpi_device_set_power_state(u16 dev_id, u8 pstate)
> +{
> +	int stat;
> +	struct dev_pstate_set dev_set = {
> +		.dev_id = cpu_to_le16(dev_id),
> +		.pstate = pstate,
> +	};
> +
> +	return scpi_send_message(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set,
> +				 sizeof(dev_set), &stat, sizeof(stat));
> +}
> +

[...]

-- 
Tixy

Re: [PATCH 1/3] firmware: arm_scpi: add support for device power state management

[Sudeep Holla]

On 07/06/16 13:58, Jon Medhurst (Tixy) wrote:
> On Mon, 2016-06-06 at 16:53 +0100, Sudeep Holla wrote:
>> SCPI protocol supports device power state management. This deals with
>> power states of various peripheral devices in the system other than the
>> core compute subsystem.
>>
>> This patch adds support for the power state management of those
>> peripheral devices.
>>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   drivers/firmware/arm_scpi.c   | 29 +++++++++++++++++++++++++++++
>>   include/linux/scpi_protocol.h |  2 ++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 51c6db0774cc..ca6afd2217a8 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -231,6 +231,11 @@ struct sensor_value {
>>   	__le32 hi_val;
>>   } __packed;
>>
>> +struct dev_pstate_set {
>> +	u16 dev_id;
>> +	u8 pstate;
>> +} __packed;
>> +
>>   static struct scpi_drvinfo *scpi_info;
>>
>>   static int scpi_linux_errmap[SCPI_ERR_MAX] = {
>> @@ -537,6 +542,28 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>>   	return ret;
>>   }
>>
>> +static int scpi_device_get_power_state(u16 dev_id)
>> +{
>> +	int ret;
>> +	u8 pstate;
>> +
>> +	ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &dev_id,
>> +				sizeof(dev_id), &pstate, sizeof(pstate));
>
> Don't you need to run translate dev_id to little-endian before sending
> it? You remembered to do that next function...
>

Good catch, that's the reason I added the get_power_state call in
scpi_genpd code to cover testing but still I failed.

Thanks for catching this.

-- 
Regards,
Sudeep

Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd

[Jon Medhurst (Tixy)]

On Mon, 2016-06-06 at 16:53 +0100, Sudeep Holla wrote:
> This patch hooks up the support for device power domain provided by
> SCPI using the Linux generic power domain infrastructure.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/Kconfig   |   8 +++
>  drivers/firmware/Makefile  |   1 +
>  drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 161 insertions(+)
>  create mode 100644 drivers/firmware/scpi_pd.c
> 
> Hi,
> 
> Since most of the power controller drivers are place in drivers/soc/<soc_name>,
> I am not sure where to put this SCPI power domain code as it can be used
> on multiple SoC. I have placed it in drivers/firmware temporarily for
> review. Please suggest the most apt place to put this driver.
> 
> Regards,
> Sudeep
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 41abdc54815e..80c963c60f13 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL
>  	  This protocol library provides interface for all the client drivers
>  	  making use of the features offered by the SCP.
> 
> +config ARM_SCPI_POWER_DOMAIN
> +	tristate "SCPI power domain driver"
> +	depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST
> +	select PM_GENERIC_DOMAINS_OF

That select doesn't work for me and gives:

warning: (ARM_SCPI_POWER_DOMAIN) selects PM_GENERIC_DOMAINS_OF which has unmet direct dependencies (PM_GENERIC_DOMAINS && OF)

Followed by link errors due to missing symbols.

I think you need to select PM_GENERIC_DOMAINS as well. Or perhaps just
instead of, as PM_GENERIC_DOMAINS_OF defaults 'y' and isn't user
selectable. From kernel/power/Kconfig ...

config PM_GENERIC_DOMAINS_OF
	def_bool y
	depends on PM_GENERIC_DOMAINS && OF

[...]

-- 
Tixy

Re: [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains

[Mark Rutland]

On Mon, Jun 06, 2016 at 04:53:58PM +0100, Sudeep Holla wrote:
> The System Control Processor (SCP) provides peripheral devices with
> power domains that can be enabled and disabled viathe System Control
> and Power Interface (SCPI) Message Protocol. Add bindings to allow
> probing of these device power domians.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> index 313dabdc14f9..7141670d649b 100644
> --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
> +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> @@ -87,10 +87,33 @@ SCPI provides an API to access the various sensors on the SoC.
>  			 implementation for the IDs to use. For Juno
>  			 R0 and Juno R1 refer to [3].
> 
> +Power domain bindings for the power domains based on SCPI Message Protocol
> +------------------------------------------------------------
> +
> +This binding uses the generic power domain binding[4].
> +
> +PM domain providers
> +===================
> +
> +Required properties:
> + - #power-domain-cells : Should be 1. Contains the device or the power
> +			 domain ID value used by SCPI commands.
> + - num-domains: Total number of power domains provided by SCPI. This is
> +		needed as the SCPI message protocol lacks a mechanism to
> +		query this information runtime.
                                      ^
I guess there should be an 'at' here.

Are domain IDs zero-based and definitely non-sparse?

What exactly does this matter for? Just for validation at parsing time,
or is this strictly required for correctness?

If we send a command with an invalid domain ID, would the FW reliably
report an error that we can recover from?

Otherwise, this looks ok. I'd just like to make sure I've understood
correctly.

Thanks,
Mark.

> +
> +PM domain consumers
> +===================
> +
> +Required properties:
> + - power-domains : A phandle and PM domain specifier as defined by bindings of
> +                   the power controller specified by phandle.
> +
>  [0] http://infocenter.arm.com/help/topic/com.arm.doc.dui0922b/index.html
>  [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>  [2] Documentation/devicetree/bindings/thermal/thermal.txt
>  [3] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922b/apas03s22.html
> +[4] Documentation/devicetree/bindings/power/power_domain.txt
> 
>  Example:
> 
> @@ -144,6 +167,12 @@ scpi_protocol: scpi@2e000000 {
>  		compatible = "arm,scpi-sensors";
>  		#thermal-sensor-cells = <1>;
>  	};
> +
> +	scpi_devpd: scpi-power-domains {
> +		compatible = "arm,scpi-power-domains";
> +		num-domains = <2>;
> +		#power-domain-cells = <1>;
> +	};
>  };
> 
>  cpu@0 {
> @@ -156,6 +185,7 @@ hdlcd@7ff60000 {
>  	...
>  	reg = <0 0x7ff60000 0 0x1000>;
>  	clocks = <&scpi_clk 4>;
> +	power-domains = <&scpi_devpd 1>;
>  };
> 
>  thermal-zones {
> @@ -186,3 +216,7 @@ The thermal-sensors property in the soc_thermal node uses the
>  temperature sensor provided by SCP firmware to setup a thermal
>  zone. The ID "3" is the sensor identifier for the temperature sensor
>  as used by the firmware.
> +
> +The num-domains property in scpi-power-domains domain specifies that
> +SCPI provides 2 power domains. The hdlcd node uses the power domain with
> +domain ID 1.
> --
> 2.7.4
> 

Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd

[Sudeep Holla]

On 07/06/16 14:18, Jon Medhurst (Tixy) wrote:
> On Mon, 2016-06-06 at 16:53 +0100, Sudeep Holla wrote:
>> This patch hooks up the support for device power domain provided by
>> SCPI using the Linux generic power domain infrastructure.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Kevin Hilman <khilman@kernel.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: linux-pm@vger.kernel.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   drivers/firmware/Kconfig   |   8 +++
>>   drivers/firmware/Makefile  |   1 +
>>   drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 161 insertions(+)
>>   create mode 100644 drivers/firmware/scpi_pd.c
>>
>> Hi,
>>
>> Since most of the power controller drivers are place in drivers/soc/<soc_name>,
>> I am not sure where to put this SCPI power domain code as it can be used
>> on multiple SoC. I have placed it in drivers/firmware temporarily for
>> review. Please suggest the most apt place to put this driver.
>>
>> Regards,
>> Sudeep
>>
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index 41abdc54815e..80c963c60f13 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL
>>   	  This protocol library provides interface for all the client drivers
>>   	  making use of the features offered by the SCP.
>>
>> +config ARM_SCPI_POWER_DOMAIN
>> +	tristate "SCPI power domain driver"
>> +	depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST
>> +	select PM_GENERIC_DOMAINS_OF
>

Actually I had something like below before and changed it before posting.

config ARM_SCPI_POWER_DOMAIN
	tristate "SCPI power domain driver"
	depends on ARM_SCPI_PROTOCOL || COMPILE_TEST
	select PM_GENERIC_DOMAINS if PM
	select PM_GENERIC_DOMAINS_OF if PM

The idea was to allow compilation of this even if PM was disabled.


> That select doesn't work for me and gives:
>
> warning: (ARM_SCPI_POWER_DOMAIN) selects PM_GENERIC_DOMAINS_OF which has unmet direct dependencies (PM_GENERIC_DOMAINS && OF)
>
> Followed by link errors due to missing symbols.
>
> I think you need to select PM_GENERIC_DOMAINS as well.

I agree, that's exactly what I had before.

> Or perhaps just instead of, as PM_GENERIC_DOMAINS_OF defaults 'y' and isn't user
> selectable. From kernel/power/Kconfig ...
>

Makes sense, I am fine with that too.

-- 
Regards,
Sudeep

Re: [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains

[Sudeep Holla]

On 07/06/16 14:22, Mark Rutland wrote:
> On Mon, Jun 06, 2016 at 04:53:58PM +0100, Sudeep Holla wrote:
>> The System Control Processor (SCP) provides peripheral devices with
>> power domains that can be enabled and disabled viathe System Control
>> and Power Interface (SCPI) Message Protocol. Add bindings to allow
>> probing of these device power domians.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 ++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
>> index 313dabdc14f9..7141670d649b 100644
>> --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
>> +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
>> @@ -87,10 +87,33 @@ SCPI provides an API to access the various sensors on the SoC.
>>   			 implementation for the IDs to use. For Juno
>>   			 R0 and Juno R1 refer to [3].
>>
>> +Power domain bindings for the power domains based on SCPI Message Protocol
>> +------------------------------------------------------------
>> +
>> +This binding uses the generic power domain binding[4].
>> +
>> +PM domain providers
>> +===================
>> +
>> +Required properties:
>> + - #power-domain-cells : Should be 1. Contains the device or the power
>> +			 domain ID value used by SCPI commands.
>> + - num-domains: Total number of power domains provided by SCPI. This is
>> +		needed as the SCPI message protocol lacks a mechanism to
>> +		query this information runtime.
>                                        ^
> I guess there should be an 'at' here.
>

Will fix.

> Are domain IDs zero-based and definitely non-sparse?
>

Yes

> What exactly does this matter for? Just for validation at parsing time,
> or is this strictly required for correctness?
>

This is mainly to know the maximum number of power domains that firmware
supports. This will help the software handling the provider part to
setup the information in advance before any consumer request for the
service.

> If we send a command with an invalid domain ID, would the FW reliably
> report an error that we can recover from?
>

Yes for anything above this value, firmware returns invalid parameter
error. It's would be good to have that as a separate command instead
of getting it via DT. We already have that for OPPs and clocks. Just
this lacks that feature.

> Otherwise, this looks ok. I'd just like to make sure I've understood
> correctly.
>

Sure, thanks for the review.

-- 
Regards,
Sudeep

Re: [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains

[Mark Rutland]

On Tue, Jun 07, 2016 at 02:39:25PM +0100, Sudeep Holla wrote:
> 
> 
> On 07/06/16 14:22, Mark Rutland wrote:
> >On Mon, Jun 06, 2016 at 04:53:58PM +0100, Sudeep Holla wrote:
> >>The System Control Processor (SCP) provides peripheral devices with
> >>power domains that can be enabled and disabled viathe System Control
> >>and Power Interface (SCPI) Message Protocol. Add bindings to allow
> >>probing of these device power domians.
> >>
> >>Cc: Rob Herring <robh+dt@kernel.org>
> >>Cc: Mark Rutland <mark.rutland@arm.com>
> >>Cc: linux-arm-kernel@lists.infradead.org
> >>Cc: devicetree@vger.kernel.org
> >>Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>---
> >>  Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 ++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >>diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> >>index 313dabdc14f9..7141670d649b 100644
> >>--- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
> >>+++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> >>@@ -87,10 +87,33 @@ SCPI provides an API to access the various sensors on the SoC.
> >>  			 implementation for the IDs to use. For Juno
> >>  			 R0 and Juno R1 refer to [3].
> >>
> >>+Power domain bindings for the power domains based on SCPI Message Protocol
> >>+------------------------------------------------------------
> >>+
> >>+This binding uses the generic power domain binding[4].
> >>+
> >>+PM domain providers
> >>+===================
> >>+
> >>+Required properties:
> >>+ - #power-domain-cells : Should be 1. Contains the device or the power
> >>+			 domain ID value used by SCPI commands.
> >>+ - num-domains: Total number of power domains provided by SCPI. This is
> >>+		needed as the SCPI message protocol lacks a mechanism to
> >>+		query this information runtime.
> >                                       ^
> >I guess there should be an 'at' here.
> >
> 
> Will fix.
> 
> >Are domain IDs zero-based and definitely non-sparse?
> >
> 
> Yes

Ok. So FWIW, with the fix above:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd

[Sudeep Holla]

Hi,

On 06/06/16 16:53, Sudeep Holla wrote:
> This patch hooks up the support for device power domain provided by
> SCPI using the Linux generic power domain infrastructure.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/firmware/Kconfig   |   8 +++
>   drivers/firmware/Makefile  |   1 +
>   drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 161 insertions(+)
>   create mode 100644 drivers/firmware/scpi_pd.c
>
> Hi,
>
> Since most of the power controller drivers are place in drivers/soc/<soc_name>,
> I am not sure where to put this SCPI power domain code as it can be used
> on multiple SoC. I have placed it in drivers/firmware temporarily for
> review. Please suggest the most apt place to put this driver.
>

Any suggestions ? Or drivers/firmware is OK for now ?

-- 
Regards,
Sudeep

Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd

[Ulf Hansson]

On 6 June 2016 at 17:53, Sudeep Holla <sudeep.holla@arm.com> wrote:
> This patch hooks up the support for device power domain provided by
> SCPI using the Linux generic power domain infrastructure.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

For following versions, please keep me in the loop for the entire
series. Including the cover-letter which I am unable to find.

> ---
>  drivers/firmware/Kconfig   |   8 +++
>  drivers/firmware/Makefile  |   1 +
>  drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 161 insertions(+)
>  create mode 100644 drivers/firmware/scpi_pd.c
>
> Hi,
>
> Since most of the power controller drivers are place in drivers/soc/<soc_name>,
> I am not sure where to put this SCPI power domain code as it can be used
> on multiple SoC. I have placed it in drivers/firmware temporarily for
> review. Please suggest the most apt place to put this driver.

To me, I think it makes sense to put this in the suggested directory,
as it's not SoC specific code.

>
> Regards,
> Sudeep
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 41abdc54815e..80c963c60f13 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL
>           This protocol library provides interface for all the client drivers
>           making use of the features offered by the SCP.
>
> +config ARM_SCPI_POWER_DOMAIN
> +       tristate "SCPI power domain driver"
> +       depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST
> +       select PM_GENERIC_DOMAINS_OF

I think this is better:
depends on (ARM_SCPI_PROTOCOL) || COMPILE_TEST
select PM_GENERIC_DOMAINS if PM

> +       help
> +         This enables support for the SCPI power domains which can be
> +         enabled or disabled via the SCP firmware
> +
>  config EDD
>         tristate "BIOS Enhanced Disk Drive calls determine boot disk"
>         depends on X86
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 474bada56fcd..24f7fe8e3fc3 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -3,6 +3,7 @@
>  #
>  obj-$(CONFIG_ARM_PSCI_FW)      += psci.o
>  obj-$(CONFIG_ARM_SCPI_PROTOCOL)        += arm_scpi.o
> +obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pd.o
>  obj-$(CONFIG_DMI)              += dmi_scan.o
>  obj-$(CONFIG_DMI_SYSFS)                += dmi-sysfs.o
>  obj-$(CONFIG_EDD)              += edd.o
> diff --git a/drivers/firmware/scpi_pd.c b/drivers/firmware/scpi_pd.c

Perhaps name it scpi_pm_domain.c instead as it gives a better
description of its purpose.

> new file mode 100644
> index 000000000000..fb24631d2b2e
> --- /dev/null
> +++ b/drivers/firmware/scpi_pd.c
> @@ -0,0 +1,152 @@
> +/*
> + * SCPI Generic power domain support.
> + *
> + * Copyright (C) 2016 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_domain.h>
> +#include <linux/scpi_protocol.h>
> +
> +struct scpi_pm_domain {
> +       struct generic_pm_domain genpd;
> +       struct scpi_ops *ops;
> +       u32 domain;
> +       char name[20];
> +};
> +
> +enum scpi_power_domain_state {
> +       SCPI_PD_STATE_ON = 0,
> +       SCPI_PD_STATE_OFF = 3,
> +};
> +
> +#define to_scpi_pd(gpd) container_of(gpd, struct scpi_pm_domain, genpd)
> +
> +static int scpi_pd_power(struct scpi_pm_domain *pd, bool power_on)
> +{
> +       int ret;
> +       enum scpi_power_domain_state state;
> +
> +       if (power_on)
> +               state = SCPI_PD_STATE_ON;
> +       else
> +               state = SCPI_PD_STATE_OFF;
> +
> +       ret = pd->ops->device_set_power_state(pd->domain, state);
> +       if (ret)
> +               return ret;
> +
> +       return (state == pd->ops->device_get_power_state(pd->domain));
> +}
> +
> +static int scpi_pd_power_on(struct generic_pm_domain *domain)
> +{
> +       struct scpi_pm_domain *pd = to_scpi_pd(domain);
> +
> +       return scpi_pd_power(pd, true);
> +}
> +
> +static int scpi_pd_power_off(struct generic_pm_domain *domain)
> +{
> +       struct scpi_pm_domain *pd = to_scpi_pd(domain);
> +
> +       return scpi_pd_power(pd, false);
> +}
> +
> +static int scpi_pm_domain_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct scpi_pm_domain *scpi_pd;
> +       struct genpd_onecell_data *scpi_pd_data;
> +       struct generic_pm_domain **domains;
> +       struct scpi_ops *scpi_ops;
> +       int ret, num_domains, i;
> +
> +       scpi_ops = get_scpi_ops();
> +       if (!scpi_ops)
> +               return -EPROBE_DEFER;
> +
> +       if (!np) {
> +               dev_err(dev, "device tree node not found\n");
> +               return -ENODEV;
> +       }
> +
> +       ret = of_property_read_u32(np, "num-domains", &num_domains);
> +       if (ret) {
> +               dev_err(dev, "number of domains not found\n");
> +               return -EINVAL;
> +       }
> +
> +       scpi_pd = devm_kcalloc(dev, num_domains, sizeof(*scpi_pd), GFP_KERNEL);
> +       if (!scpi_pd)
> +               return -ENOMEM;
> +
> +       scpi_pd_data = devm_kzalloc(dev, sizeof(*scpi_pd_data), GFP_KERNEL);
> +       if (!scpi_pd_data)
> +               return -ENOMEM;
> +
> +       domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL);
> +       if (!domains)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < num_domains; i++, scpi_pd++) {
> +               domains[i] = &scpi_pd->genpd;
> +
> +               scpi_pd->domain = i;
> +               scpi_pd->ops = scpi_ops;
> +               sprintf(scpi_pd->name, "%s.%d", np->name, i);
> +               scpi_pd->genpd.name = scpi_pd->name;
> +               scpi_pd->genpd.power_off = scpi_pd_power_off;
> +               scpi_pd->genpd.power_on = scpi_pd_power_on;
> +
> +               /*
> +                * Treat all power domains as off at boot.
> +                *
> +                * The SCP firmware itself may have switched on some domains,
> +                * but for reference counting purpose, keep it this way.
> +                */
> +               pm_genpd_init(&scpi_pd->genpd, NULL, true);
> +       }
> +
> +       scpi_pd_data->domains = domains;
> +       scpi_pd_data->num_domains = num_domains;
> +
> +       of_genpd_add_provider_onecell(np, scpi_pd_data);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id scpi_power_domain_ids[] = {
> +       { .compatible = "arm,scpi-power-domains", },
> +       { /* sentinel */ }
> +};

Actually I think you shouldn't implement this a standalone driver and
thus you can remove this compatible.

Instead, I think it's better if you let the arm_scpi driver to also
initialize the PM domain.

If you still want the PM domain code to be maintained in a separate
file, just provide a header file which declares an
"scpi_pm_domain_init()" function (and a stub when not supported),
which the arm_scpi driver should call during ->probe().

> +MODULE_DEVICE_TABLE(of, scpi_power_domain_ids);
> +
> +static struct platform_driver scpi_power_domain_driver = {
> +       .driver = {
> +               .name = "scpi_power_domain",
> +               .of_match_table = scpi_power_domain_ids,
> +       },
> +       .probe = scpi_pm_domain_probe,
> +};
> +module_platform_driver(scpi_power_domain_driver);
> +
> +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> +MODULE_DESCRIPTION("ARM SCPI power domain driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

Kind regards
Uffe

Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd

[Sudeep Holla]

On 15/06/16 14:05, Ulf Hansson wrote:
> On 6 June 2016 at 17:53, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> This patch hooks up the support for device power domain provided by
>> SCPI using the Linux generic power domain infrastructure.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Kevin Hilman <khilman@kernel.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: linux-pm@vger.kernel.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> For following versions, please keep me in the loop for the entire
> series. Including the cover-letter which I am unable to find.
>

Ok, will do.

>> ---
>>   drivers/firmware/Kconfig   |   8 +++
>>   drivers/firmware/Makefile  |   1 +
>>   drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 161 insertions(+)
>>   create mode 100644 drivers/firmware/scpi_pd.c
>>
>> Hi,
>>
>> Since most of the power controller drivers are place in drivers/soc/<soc_name>,
>> I am not sure where to put this SCPI power domain code as it can be used
>> on multiple SoC. I have placed it in drivers/firmware temporarily for
>> review. Please suggest the most apt place to put this driver.
>
> To me, I think it makes sense to put this in the suggested directory,
> as it's not SoC specific code.
>

Sure

>>
>> Regards,
>> Sudeep
>>
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index 41abdc54815e..80c963c60f13 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL
>>            This protocol library provides interface for all the client drivers
>>            making use of the features offered by the SCP.
>>
>> +config ARM_SCPI_POWER_DOMAIN
>> +       tristate "SCPI power domain driver"
>> +       depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST
>> +       select PM_GENERIC_DOMAINS_OF
>
> I think this is better:
> depends on (ARM_SCPI_PROTOCOL) || COMPILE_TEST
> select PM_GENERIC_DOMAINS if PM
>

Yes it's changed already like this after Tixy reported an issue.

>> +       help
>> +         This enables support for the SCPI power domains which can be
>> +         enabled or disabled via the SCP firmware
>> +
>>   config EDD
>>          tristate "BIOS Enhanced Disk Drive calls determine boot disk"
>>          depends on X86
>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>> index 474bada56fcd..24f7fe8e3fc3 100644
>> --- a/drivers/firmware/Makefile
>> +++ b/drivers/firmware/Makefile
>> @@ -3,6 +3,7 @@
>>   #
>>   obj-$(CONFIG_ARM_PSCI_FW)      += psci.o
>>   obj-$(CONFIG_ARM_SCPI_PROTOCOL)        += arm_scpi.o
>> +obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pd.o
>>   obj-$(CONFIG_DMI)              += dmi_scan.o
>>   obj-$(CONFIG_DMI_SYSFS)                += dmi-sysfs.o
>>   obj-$(CONFIG_EDD)              += edd.o
>> diff --git a/drivers/firmware/scpi_pd.c b/drivers/firmware/scpi_pd.c
>
> Perhaps name it scpi_pm_domain.c instead as it gives a better
> description of its purpose.
>

Agreed.

[...]

>> +static const struct of_device_id scpi_power_domain_ids[] = {
>> +       { .compatible = "arm,scpi-power-domains", },
>> +       { /* sentinel */ }
>> +};
>
> Actually I think you shouldn't implement this a standalone driver and
> thus you can remove this compatible.
>

While I tend to agree, I did this to keep it aligned with other SCPI
users(clocks, sensors,.. for example).

I assume remove compatible just from driver ? IMO, it doesn't make sense
to add power domain provider without a compatible.

> Instead, I think it's better if you let the arm_scpi driver to also
> initialize the PM domain.
>

OK, I can do that.

> If you still want the PM domain code to be maintained in a separate
> file, just provide a header file which declares an
> "scpi_pm_domain_init()" function (and a stub when not supported),
> which the arm_scpi driver should call during ->probe().
>

I am fine with that, just that it deviates from the approach taken in
other subsystems as I mentioned above.

-- 
Regards,
Sudeep

Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd

[Ulf Hansson]

[...]

>
>>> +static const struct of_device_id scpi_power_domain_ids[] = {
>>> +       { .compatible = "arm,scpi-power-domains", },
>>> +       { /* sentinel */ }
>>> +};
>>
>>
>> Actually I think you shouldn't implement this a standalone driver and
>> thus you can remove this compatible.
>>
>
> While I tend to agree, I did this to keep it aligned with other SCPI
> users(clocks, sensors,.. for example).
>
> I assume remove compatible just from driver ? IMO, it doesn't make sense
> to add power domain provider without a compatible.
>
>> Instead, I think it's better if you let the arm_scpi driver to also
>> initialize the PM domain.
>>
>
> OK, I can do that.
>
>> If you still want the PM domain code to be maintained in a separate
>> file, just provide a header file which declares an
>> "scpi_pm_domain_init()" function (and a stub when not supported),
>> which the arm_scpi driver should call during ->probe().
>>
>
> I am fine with that, just that it deviates from the approach taken in
> other subsystems as I mentioned above.

If DT maintainers are happy with you adding a compatible for this,
don't let me stop you from implementing this as standalone driver.

I have no strong opinions about it, so perhaps it's then better to not
deviate from other cases!?

Kind regards
Uffe

Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd

[Sudeep Holla]

On 15/06/16 14:29, Ulf Hansson wrote:
> [...]
>
>>
>>>> +static const struct of_device_id scpi_power_domain_ids[] = {
>>>> +       { .compatible = "arm,scpi-power-domains", },
>>>> +       { /* sentinel */ }
>>>> +};
>>>
>>>
>>> Actually I think you shouldn't implement this a standalone driver and
>>> thus you can remove this compatible.
>>>
>>
>> While I tend to agree, I did this to keep it aligned with other SCPI
>> users(clocks, sensors,.. for example).
>>
>> I assume remove compatible just from driver ? IMO, it doesn't make sense
>> to add power domain provider without a compatible.
>>
>>> Instead, I think it's better if you let the arm_scpi driver to also
>>> initialize the PM domain.
>>>
>>
>> OK, I can do that.
>>
>>> If you still want the PM domain code to be maintained in a separate
>>> file, just provide a header file which declares an
>>> "scpi_pm_domain_init()" function (and a stub when not supported),
>>> which the arm_scpi driver should call during ->probe().
>>>
>>
>> I am fine with that, just that it deviates from the approach taken in
>> other subsystems as I mentioned above.
>
> If DT maintainers are happy with you adding a compatible for this,
> don't let me stop you from implementing this as standalone driver.
>

I assume compatibles are always preferred even if they are not used to
make it future proof and may be that's why the binding was accepted. We
need to have a node to specify phandle in the consumers anyways, it's
always better to have separate node for each of the SCPI users/provider
(like clock, sensors, power domains) instead of pointing all to the one
SCPI node. Again that's just my view.

> I have no strong opinions about it, so perhaps it's then better to not
> deviate from other cases!?
>

OK, thanks. I will respin with Kconfig changes and retain the file in
drivers/firmware for now.

-- 
Regards,
Sudeep