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

[PATCH v2 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

v1[1]->v2:
	- Fixed the endianness handling in scpi_device_get_power_state
	  as spotted by Tixy
	- Renamed scpi_pd.c to scpi_pm_domain.c as suggested by Ulf
	- Added Mark's ack on DT binding

[1] https://marc.info/?l=linux-kernel&m=146522850003483&w=2

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                           |   9 ++
 drivers/firmware/Makefile                          |   1 +
 drivers/firmware/arm_scpi.c                        |  30 ++++
 drivers/firmware/scpi_pm_domain.c                  | 152 +++++++++++++++++++++
 include/linux/scpi_protocol.h                      |   2 +
 6 files changed, 228 insertions(+)
 create mode 100644 drivers/firmware/scpi_pm_domain.c

--
2.7.4

[PATCH v2 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.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scpi.c   | 30 ++++++++++++++++++++++++++++++
 include/linux/scpi_protocol.h |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 51c6db0774cc..438893762076 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,29 @@ 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;
+	__le16 id = cpu_to_le16(dev_id);
+
+	ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &id,
+				sizeof(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 +576,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 v2 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>
Acked-by: Mark Rutland <mark.rutland@arm.com>
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..faa4b44572e3 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 at 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 v2 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          |   9 +++
 drivers/firmware/Makefile         |   1 +
 drivers/firmware/scpi_pm_domain.c | 152 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+)
 create mode 100644 drivers/firmware/scpi_pm_domain.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 41abdc54815e..ac22094b1e32 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -27,6 +27,15 @@ 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 || COMPILE_TEST
+	select PM_GENERIC_DOMAINS if PM
+	select PM_GENERIC_DOMAINS_OF 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..44a59dcfc398 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_pm_domain.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_pm_domain.c b/drivers/firmware/scpi_pm_domain.c
new file mode 100644
index 000000000000..e523d29e12fa
--- /dev/null
+++ b/drivers/firmware/scpi_pm_domain.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[30];
+};
+
+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 v2 0/3] firmware: scpi: add device power domain support

[Jon Medhurst (Tixy)]

On Thu, 2016-06-16 at 11:37 +0100, Sudeep Holla wrote:
> This series add support for SCPI based device device power state
> management using genpd.
> 
> Regards,
> Sudeep
> 
> v1[1]->v2:
> 	- Fixed the endianness handling in scpi_device_get_power_state
> 	  as spotted by Tixy
> 	- Renamed scpi_pd.c to scpi_pm_domain.c as suggested by Ulf

You seemed to also have made some changes to the contents of the file,
is that deliberate? This is the diff...

--- a/drivers/firmware/scpi_pm_domain.c
+++ b/drivers/firmware/scpi_pd.c
@@ -27,7 +27,7 @@ struct scpi_pm_domain {
        struct generic_pm_domain genpd;
        struct scpi_ops *ops;
        u32 domain;
-       char name[20];
+       char name[30];
 };
 
 enum scpi_power_domain_state {
@@ -51,7 +51,7 @@ static int scpi_pd_power(struct scpi_pm_domain *pd,
bool power_on)
        if (ret)
                return ret;
 
-       return (state == pd->ops->device_get_power_state(pd->domain));
+       return !(state == pd->ops->device_get_power_state(pd->domain));
 }
 
 static int scpi_pd_power_on(struct generic_pm_domain *domain)

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

[Sudeep Holla]

On 16/06/16 14:14, Jon Medhurst (Tixy) wrote:
> On Thu, 2016-06-16 at 11:37 +0100, Sudeep Holla wrote:
>> This series add support for SCPI based device device power state
>> management using genpd.
>>
>> Regards,
>> Sudeep
>>
>> v1[1]->v2:
>> 	- Fixed the endianness handling in scpi_device_get_power_state
>> 	  as spotted by Tixy
>> 	- Renamed scpi_pd.c to scpi_pm_domain.c as suggested by Ulf
>
> You seemed to also have made some changes to the contents of the file,
> is that deliberate? This is the diff...
>

name array was accidental. I added it as I was not sure if I want to
make it dynamic, just found I might be hitting the max while testing and
added it.

The other change was something that I found recently after I tried to
get rid of juno coresight primecell override after I realized AMBA
framework should be able to cope up with power domains. I will reply
on that thread. Sorry for missing out this in changelog. I assume in my
previous testings only the paths that ignored the return were executed.

-- 
Regards,
Sudeep

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

[Jon Medhurst (Tixy)]

On Thu, 2016-06-16 at 11:38 +0100, Sudeep Holla wrote:
[...]
> +enum scpi_power_domain_state {
> +	SCPI_PD_STATE_ON = 0,
> +	SCPI_PD_STATE_OFF = 3,
> +};

The SCPI doc defines the meaning of these numbers (0 and 3) in the 'Juno
specifics' chapter. So does these values need to come from device-tree
to allow for other hardware or SCP implementations?

-- 
Tixy

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

[Sudeep Holla]

On 16/06/16 18:47, Jon Medhurst (Tixy) wrote:
> On Thu, 2016-06-16 at 11:38 +0100, Sudeep Holla wrote:
> [...]
>> +enum scpi_power_domain_state {
>> +	SCPI_PD_STATE_ON = 0,
>> +	SCPI_PD_STATE_OFF = 3,
>> +};
>
> The SCPI doc defines the meaning of these numbers (0 and 3) in the 'Juno
> specifics' chapter. So does these values need to come from device-tree
> to allow for other hardware or SCP implementations?
>

Ah unfortunately true :(. I had not noticed that. But I would like to
check if this can be made as part of the standard protocol. Adding such
details to DT seems overkill and defeat of the whole purpose of the
standard protocol.

-- 
Regards,
Sudeep

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

[Jon Medhurst (Tixy)]

On Thu, 2016-06-16 at 11:37 +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.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Jon Medhurst <tixy@linaro.org>
Tested-by: Jon Medhurst <tixy@linaro.org>

I've also tested this series on Juno r2 together with the coresight
changes [1], to the extent that enabling various kernel configs gets the
kernel log showing several lines like:

coresight-etm4x 22040000.etm: ETM 4.0 initialized

and if I omit the power domain entries from device-tree then boot hangs
as you indicated it would be expected due to coresight hardware being
powered off.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/433769.html


> ---
>  drivers/firmware/arm_scpi.c   | 30 ++++++++++++++++++++++++++++++
>  include/linux/scpi_protocol.h |  2 ++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 51c6db0774cc..438893762076 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,29 @@ 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;
> +	__le16 id = cpu_to_le16(dev_id);
> +
> +	ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &id,
> +				sizeof(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 +576,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
> 

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

[Jon Medhurst (Tixy)]

On Thu, 2016-06-16 at 18:59 +0100, Sudeep Holla wrote:
> 
> On 16/06/16 18:47, Jon Medhurst (Tixy) wrote:
> > On Thu, 2016-06-16 at 11:38 +0100, Sudeep Holla wrote:
> > [...]
> >> +enum scpi_power_domain_state {
> >> +	SCPI_PD_STATE_ON = 0,
> >> +	SCPI_PD_STATE_OFF = 3,
> >> +};
> >
> > The SCPI doc defines the meaning of these numbers (0 and 3) in the 'Juno
> > specifics' chapter. So does these values need to come from device-tree
> > to allow for other hardware or SCP implementations?
> >
> 
> Ah unfortunately true :(. I had not noticed that. But I would like to
> check if this can be made as part of the standard protocol. Adding such
> details to DT seems overkill and defeat of the whole purpose of the
> standard protocol.

Well. it seems to me the 'standard protocol' is whatever the current
implementation of ARM's closed source SCP firmware is. It also seems to
me that people are making things up as they go along, without a clue as
to how to make things generic, robust and future proof. Basically,
Status Normal ARM Fucked Up.

-- 
Tixy

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

[Sudeep Holla]

On 17/06/16 09:19, Jon Medhurst (Tixy) wrote:
> On Thu, 2016-06-16 at 18:59 +0100, Sudeep Holla wrote:
>>
>> On 16/06/16 18:47, Jon Medhurst (Tixy) wrote:
>>> On Thu, 2016-06-16 at 11:38 +0100, Sudeep Holla wrote:
>>> [...]
>>>> +enum scpi_power_domain_state {
>>>> +	SCPI_PD_STATE_ON = 0,
>>>> +	SCPI_PD_STATE_OFF = 3,
>>>> +};
>>>
>>> The SCPI doc defines the meaning of these numbers (0 and 3) in the 'Juno
>>> specifics' chapter. So does these values need to come from device-tree
>>> to allow for other hardware or SCP implementations?
>>>
>>
>> Ah unfortunately true :(. I had not noticed that. But I would like to
>> check if this can be made as part of the standard protocol. Adding such
>> details to DT seems overkill and defeat of the whole purpose of the
>> standard protocol.
>
> Well. it seems to me the 'standard protocol' is whatever the current
> implementation of ARM's closed source SCP firmware is. It also seems to
> me that people are making things up as they go along, without a clue as
> to how to make things generic, robust and future proof.

Totally agree. There's an effort to come up with more standard version
of this involving partners/users soon. It's still under discussion and
the aim is to make it as good as PSCI. Let's see where it goes...

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 16/06/16 19:03, Jon Medhurst (Tixy) wrote:
> On Thu, 2016-06-16 at 11:37 +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.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Reviewed-by: Jon Medhurst <tixy@linaro.org>
> Tested-by: Jon Medhurst <tixy@linaro.org>
>

Thanks a lot.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

Hi Ulf,

On 16/06/16 11:38, 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>

Can you review this ? I have cc-ed you on the entire series.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 17/06/16 09:55, Sudeep Holla wrote:
> Hi Ulf,
>
> On 16/06/16 11:38, 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>
>
> Can you review this ? I have cc-ed you on the entire series.
>

Sorry we crossed over the email, thanks for the review.

-- 
Regards,
Sudeep

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

[Ulf Hansson]

On 16 June 2016 at 15:26, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 16/06/16 14:14, Jon Medhurst (Tixy) wrote:
>>
>> On Thu, 2016-06-16 at 11:37 +0100, Sudeep Holla wrote:
>>>
>>> This series add support for SCPI based device device power state
>>> management using genpd.
>>>
>>> Regards,
>>> Sudeep
>>>
>>> v1[1]->v2:
>>>         - Fixed the endianness handling in scpi_device_get_power_state
>>>           as spotted by Tixy
>>>         - Renamed scpi_pd.c to scpi_pm_domain.c as suggested by Ulf
>>
>>
>> You seemed to also have made some changes to the contents of the file,
>> is that deliberate? This is the diff...
>>
>
> name array was accidental. I added it as I was not sure if I want to
> make it dynamic, just found I might be hitting the max while testing and
> added it.
>
> The other change was something that I found recently after I tried to
> get rid of juno coresight primecell override after I realized AMBA
> framework should be able to cope up with power domains. I will reply
> on that thread. Sorry for missing out this in changelog. I assume in my
> previous testings only the paths that ignored the return were executed.
>
> --
> Regards,
> Sudeep

For the series,

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

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

[Mathieu Poirier]

On 16 June 2016 at 04:37, Sudeep Holla <sudeep.holla@arm.com> wrote:
> This series add support for SCPI based device device power state
> management using genpd.
>
> Regards,
> Sudeep
>
> v1[1]->v2:
>         - Fixed the endianness handling in scpi_device_get_power_state
>           as spotted by Tixy
>         - Renamed scpi_pd.c to scpi_pm_domain.c as suggested by Ulf
>         - Added Mark's ack on DT binding
>
> [1] https://marc.info/?l=linux-kernel&m=146522850003483&w=2
>
> 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                           |   9 ++
>  drivers/firmware/Makefile                          |   1 +
>  drivers/firmware/arm_scpi.c                        |  30 ++++
>  drivers/firmware/scpi_pm_domain.c                  | 152 +++++++++++++++++++++
>  include/linux/scpi_protocol.h                      |   2 +
>  6 files changed, 228 insertions(+)
>  create mode 100644 drivers/firmware/scpi_pm_domain.c
>
> --
> 2.7.4
>

For the set:

Tested-by: Mathieu Poirier <mathieu.poirier@linaro.org>

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

[Sudeep Holla]

On 17/06/16 16:22, Mathieu Poirier wrote:
> On 16 June 2016 at 04:37, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> This series add support for SCPI based device device power state
>> management using genpd.
>>
>> Regards,
>> Sudeep
>>
>> v1[1]->v2:
>>          - Fixed the endianness handling in scpi_device_get_power_state
>>            as spotted by Tixy
>>          - Renamed scpi_pd.c to scpi_pm_domain.c as suggested by Ulf
>>          - Added Mark's ack on DT binding
>>
>> [1] https://marc.info/?l=linux-kernel&m=146522850003483&w=2
>>
>> 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                           |   9 ++
>>   drivers/firmware/Makefile                          |   1 +
>>   drivers/firmware/arm_scpi.c                        |  30 ++++
>>   drivers/firmware/scpi_pm_domain.c                  | 152 +++++++++++++++++++++
>>   include/linux/scpi_protocol.h                      |   2 +
>>   6 files changed, 228 insertions(+)
>>   create mode 100644 drivers/firmware/scpi_pm_domain.c
>>
>> --
>> 2.7.4
>>
>
> For the set:
>
> Tested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>

Thanks for testing.

-- 
Regards,
Sudeep

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

[Kevin Hilman]

"Jon Medhurst (Tixy)" <tixy@linaro.org> writes:

> On Thu, 2016-06-16 at 18:59 +0100, Sudeep Holla wrote:
>> 
>> On 16/06/16 18:47, Jon Medhurst (Tixy) wrote:
>> > On Thu, 2016-06-16 at 11:38 +0100, Sudeep Holla wrote:
>> > [...]
>> >> +enum scpi_power_domain_state {
>> >> +	SCPI_PD_STATE_ON = 0,
>> >> +	SCPI_PD_STATE_OFF = 3,
>> >> +};
>> >
>> > The SCPI doc defines the meaning of these numbers (0 and 3) in the 'Juno
>> > specifics' chapter. So does these values need to come from device-tree
>> > to allow for other hardware or SCP implementations?
>> >
>> 
>> Ah unfortunately true :(. I had not noticed that. But I would like to
>> check if this can be made as part of the standard protocol. Adding such
>> details to DT seems overkill and defeat of the whole purpose of the
>> standard protocol.
>
> Well. it seems to me the 'standard protocol' is whatever the current
> implementation of ARM's closed source SCP firmware is. It also seems to
> me that people are making things up as they go along, without a clue as
> to how to make things generic, robust and future proof. Basically,
> Status Normal ARM Fucked Up.

Fully agree here.  Just because ARM calls it a "standard" does not make
it so.  As we've already seen[1], vendors are using initial/older/whatever
versions of SCPI, so pushing the Juno version as standard just becuase
it's in ARM's closed firmware is not the right way forward either.

Kevin

[1] http://marc.info/?l=linux-arm-kernel&m=146425562931515&w=2

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

[Kevin Hilman]

Sudeep Holla <sudeep.holla@arm.com> writes:

> 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>

[...]

> +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);

There should probably be some sanity checks here that these function
pointers are non-NULL.

> +	if (ret)
> +		return ret;
> +
> +	return !(state == pd->ops->device_get_power_state(pd->domain));
> +}

Kevin

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

[Kevin Hilman]

Sudeep Holla <sudeep.holla@arm.com> writes:

> This series add support for SCPI based device device power state
> management using genpd.
>
> Regards,
> Sudeep
>
> v1[1]->v2:
> 	- Fixed the endianness handling in scpi_device_get_power_state
> 	  as spotted by Tixy
> 	- Renamed scpi_pd.c to scpi_pm_domain.c as suggested by Ulf
> 	- Added Mark's ack on DT binding
>
> [1] https://marc.info/?l=linux-kernel&m=146522850003483&w=2
>
> 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

Other than the couple nits on specific patches,

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

And just a[nother] reminder to be sure to keep things that are Juno
specific well described since we know other vendors will be (and are
being) "creative" in their implementations of SCPI.

Kevin

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

[Sudeep Holla]

On 20/06/16 18:50, Kevin Hilman wrote:
> "Jon Medhurst (Tixy)" <tixy@linaro.org> writes:
>
>> On Thu, 2016-06-16 at 18:59 +0100, Sudeep Holla wrote:
>>>
>>> On 16/06/16 18:47, Jon Medhurst (Tixy) wrote:
>>>> On Thu, 2016-06-16 at 11:38 +0100, Sudeep Holla wrote:
>>>> [...]
>>>>> +enum scpi_power_domain_state {
>>>>> +	SCPI_PD_STATE_ON = 0,
>>>>> +	SCPI_PD_STATE_OFF = 3,
>>>>> +};
>>>>
>>>> The SCPI doc defines the meaning of these numbers (0 and 3) in the 'Juno
>>>> specifics' chapter. So does these values need to come from device-tree
>>>> to allow for other hardware or SCP implementations?
>>>>
>>>
>>> Ah unfortunately true :(. I had not noticed that. But I would like to
>>> check if this can be made as part of the standard protocol. Adding such
>>> details to DT seems overkill and defeat of the whole purpose of the
>>> standard protocol.
>>
>> Well. it seems to me the 'standard protocol' is whatever the current
>> implementation of ARM's closed source SCP firmware is. It also seems to
>> me that people are making things up as they go along, without a clue as
>> to how to make things generic, robust and future proof. Basically,
>> Status Normal ARM Fucked Up.
>
> Fully agree here.  Just because ARM calls it a "standard" does not make
> it so.  As we've already seen[1], vendors are using initial/older/whatever
> versions of SCPI, so pushing the Juno version as standard just becuase
> it's in ARM's closed firmware is not the right way forward either.
>

I am not disagreeing on that. There's an on going effort to make it as
standard as PSCI. But that's still work in progress. We need to deal
with the existing variety of SCPI until then. No argument on that.

The only argument I had on the other thread is not to make the changes
too flexible that enabled the vendors to make up their own deviation of
SCPI even for their future platforms. All I am asking is to keep is less
flexible just to support existing platforms out there and not to help
the vendors to deviate further.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 20/06/16 18:56, Kevin Hilman wrote:
> Sudeep Holla <sudeep.holla@arm.com> writes:
>
>> This series add support for SCPI based device device power state
>> management using genpd.
>>
>> Regards,
>> Sudeep
>>
>> v1[1]->v2:
>> 	- Fixed the endianness handling in scpi_device_get_power_state
>> 	  as spotted by Tixy
>> 	- Renamed scpi_pd.c to scpi_pm_domain.c as suggested by Ulf
>> 	- Added Mark's ack on DT binding
>>
>> [1] https://marc.info/?l=linux-kernel&m=146522850003483&w=2
>>
>> 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
>
> Other than the couple nits on specific patches,
>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>
> And just a[nother] reminder to be sure to keep things that are Juno
> specific well described since we know other vendors will be (and are
> being) "creative" in their implementations of SCPI.
>

Agreed as mentioned in the other thread.

I just sent a PR to arm-soc this afternoon. Do you think it's better to
recall it and incorporate this ?

Extremely sorry for rushing, I thought it would get too late. Ulf
reviewed them so I rushed a bit.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 20/06/16 18:53, Kevin Hilman wrote:
> Sudeep Holla <sudeep.holla@arm.com> writes:
>
>> 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>
>
> [...]
>
>> +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);
>
> There should probably be some sanity checks here that these function
> pointers are non-NULL.
>

Yes I agree. Since with the current scpi driver, if scpi_ops is
populated, it's all non-NULL I skipped the sanity check as the probe
will check for non-NULL scpi_ops.

However, with extension work by Neil I agree, we may have to revisit all
such callers.

-- 
Regards,
Sudeep