diff mbox

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

Message ID 1466073481-697-4-git-send-email-sudeep.holla@arm.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Sudeep Holla June 16, 2016, 10:38 a.m. UTC
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

--
2.7.4

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

Comments

Jon Medhurst (Tixy) June 16, 2016, 5:47 p.m. UTC | #1
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?
Sudeep Holla June 16, 2016, 5:59 p.m. UTC | #2
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.
Jon Medhurst (Tixy) June 17, 2016, 8:19 a.m. UTC | #3
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.
Sudeep Holla June 17, 2016, 8:38 a.m. UTC | #4
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...
Sudeep Holla June 17, 2016, 8:55 a.m. UTC | #5
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.
Sudeep Holla June 17, 2016, 8:55 a.m. UTC | #6
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.
Kevin Hilman June 20, 2016, 5:50 p.m. UTC | #7
"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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman June 20, 2016, 5:53 p.m. UTC | #8
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla June 20, 2016, 5:57 p.m. UTC | #9
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.
Sudeep Holla June 20, 2016, 6:06 p.m. UTC | #10
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.
diff mbox

Patch

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