* [PATCH V4 2/2] firmware: imx: add SCU power domain driver
@ 2018-10-07 13:10 ` Dong Aisheng
0 siblings, 0 replies; 22+ messages in thread
From: Dong Aisheng @ 2018-10-07 13:10 UTC (permalink / raw)
To: linux-arm-kernel
Some i.MX SoCs contain a system controller that is responsible for
controlling the state of the IPs that are present. Communication
between the host processor running an OS and the system controller
happens through a SCU protocol. This patch adds SCU protocol based
power domains drivers.
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: linux-pm at vger.kernel.org
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v3->v4:
* update firmware headfile patch from include/soc/imx to
include/linux/firmware/imx
v2->v3:
* name of structures/enums updated with imx_sc prefix
v1->v2:
* move into drivers/firmware/imx
* Implement sc_pm_set_resource_power_mode() API in the driver instead
of call it via SCU API according to Sascha's suggestion
---
drivers/firmware/imx/Kconfig | 6 ++
drivers/firmware/imx/Makefile | 3 +-
drivers/firmware/imx/scu-pd.c | 167 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 175 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/imx/scu-pd.c
diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
index b170c28..6a7a7c2 100644
--- a/drivers/firmware/imx/Kconfig
+++ b/drivers/firmware/imx/Kconfig
@@ -9,3 +9,9 @@ config IMX_SCU
This driver manages the IPC interface between host CPU and the
SCU firmware running on M4.
+
+config IMX_SCU_PD
+ bool "IMX SCU Power Domain driver"
+ depends on IMX_SCU
+ help
+ The System Controller Firmware (SCFW) based power domain driver.
diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
index 0ac04df..1b2e15b 100644
--- a/drivers/firmware/imx/Makefile
+++ b/drivers/firmware/imx/Makefile
@@ -1,2 +1,3 @@
# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
+obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
+obj-$(CONFIG_IMX_SCU_PD) += scu-pd.o
diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
new file mode 100644
index 0000000..af5b9e4
--- /dev/null
+++ b/drivers/firmware/imx/scu-pd.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ * Dong Aisheng <aisheng.dong@nxp.com>
+ *
+ * Implementation of the SCU based Power Domains
+ */
+
+#define pr_fmt(fmt) "scu_pd: " fmt
+
+#include <linux/firmware/imx/sci.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+
+struct imx_sc_pm_domain {
+ struct generic_pm_domain pd;
+ u32 rsrc_id;
+};
+
+static struct imx_sc_ipc *pm_ipc_handle;
+
+/* SCU Power Mode Protocol definition */
+struct imx_sc_msg_req_set_resource_power_mode {
+ struct imx_sc_rpc_msg hdr;
+ u16 resource;
+ u8 mode;
+} __packed;
+
+static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
+{
+ struct imx_sc_msg_req_set_resource_power_mode msg;
+ struct imx_sc_rpc_msg *hdr = &msg.hdr;
+ struct imx_sc_pm_domain *pd;
+ int ret;
+
+ pd = container_of(domain, struct imx_sc_pm_domain, pd);
+
+ hdr->ver = IMX_SC_RPC_VERSION;
+ hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
+ hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE;
+ hdr->size = 2;
+
+ msg.resource = pd->rsrc_id;
+ msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
+
+ ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
+ if (ret)
+ pr_err("failed to power %s resource %d ret %d\n",
+ power_on ? "up" : "off", pd->rsrc_id, ret);
+
+ return ret;
+}
+
+static int imx_sc_pd_power_on(struct generic_pm_domain *domain)
+{
+ return imx_sc_pd_power(domain, true);
+}
+
+static int imx_sc_pd_power_off(struct generic_pm_domain *domain)
+{
+ return imx_sc_pd_power(domain, false);
+}
+
+static struct generic_pm_domain *imx_sc_pm_add_one_domain(
+ struct device_node *np,
+ struct generic_pm_domain *genpd_parent)
+{
+ struct imx_sc_pm_domain *imx_sc_pd;
+ u32 rsrc_id;
+ int ret;
+
+ imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
+ if (!imx_sc_pd)
+ return ERR_PTR(-ENOMEM);
+
+ if (!of_property_read_u32(np, "reg", &rsrc_id)) {
+ if (rsrc_id > IMX_SC_R_LAST) {
+ pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
+ ret = -EINVAL;
+ goto err;
+ }
+ imx_sc_pd->rsrc_id = rsrc_id;
+ } else {
+ imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
+ }
+
+ if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
+ imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
+ imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
+ }
+
+ imx_sc_pd->pd.name = np->name;
+
+ ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
+ if (ret < 0)
+ goto err;
+
+ if (genpd_parent) {
+ ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
+ if (ret)
+ goto err;
+ }
+
+ ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
+ if (!ret)
+ return &imx_sc_pd->pd;
+
+ pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
+
+err:
+ pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
+ kfree(imx_sc_pd);
+ return ERR_PTR(ret);
+}
+
+static void imx_sc_pm_add_pm_domains(struct device_node *parent,
+ struct generic_pm_domain *genpd_parent)
+{
+ struct generic_pm_domain *pd;
+ struct device_node *np;
+
+ for_each_child_of_node(parent, np) {
+ pd = imx_sc_pm_add_one_domain(np, genpd_parent);
+ if (!IS_ERR(pd))
+ imx_sc_pm_add_pm_domains(np, pd);
+ }
+}
+
+static int imx_sc_pd_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+
+ ret = imx_scu_get_handle(&pm_ipc_handle);
+ if (ret)
+ return ret;
+
+ imx_sc_pm_add_pm_domains(np, NULL);
+
+ return 0;
+}
+
+static const struct of_device_id imx_sc_pd_match[] = {
+ { .compatible = "fsl,scu-pd", },
+ { /* sentinel */ }
+};
+
+static struct platform_driver imx_sc_pd_driver = {
+ .driver = {
+ .name = "imx-scu-pd",
+ .of_match_table = imx_sc_pd_match,
+ },
+ .probe = imx_sc_pd_probe,
+};
+builtin_platform_driver(imx_sc_pd_driver);
+
+MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
+MODULE_DESCRIPTION("IMX SCU Power Domain driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
2018-10-07 13:10 ` Dong Aisheng
@ 2018-10-09 0:53 ` Shawn Guo
-1 siblings, 0 replies; 22+ messages in thread
From: Shawn Guo @ 2018-10-09 0:53 UTC (permalink / raw)
To: Dong Aisheng
Cc: ulf.hansson, dongas86, linux-pm, Sascha Hauer, khilman, rjw,
linux-imx, kernel, fabio.estevam, linux-arm-kernel
On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
> Some i.MX SoCs contain a system controller that is responsible for
> controlling the state of the IPs that are present. Communication
> between the host processor running an OS and the system controller
> happens through a SCU protocol. This patch adds SCU protocol based
> power domains drivers.
>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: linux-pm@vger.kernel.org
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
@Sascha, are you okay with this?
> ---
> ChangeLog:
> v3->v4:
> * update firmware headfile patch from include/soc/imx to
> include/linux/firmware/imx
> v2->v3:
> * name of structures/enums updated with imx_sc prefix
> v1->v2:
> * move into drivers/firmware/imx
> * Implement sc_pm_set_resource_power_mode() API in the driver instead
> of call it via SCU API according to Sascha's suggestion
> ---
> drivers/firmware/imx/Kconfig | 6 ++
> drivers/firmware/imx/Makefile | 3 +-
> drivers/firmware/imx/scu-pd.c | 167 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 175 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/imx/scu-pd.c
>
> diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> index b170c28..6a7a7c2 100644
> --- a/drivers/firmware/imx/Kconfig
> +++ b/drivers/firmware/imx/Kconfig
> @@ -9,3 +9,9 @@ config IMX_SCU
>
> This driver manages the IPC interface between host CPU and the
> SCU firmware running on M4.
> +
> +config IMX_SCU_PD
> + bool "IMX SCU Power Domain driver"
> + depends on IMX_SCU
> + help
> + The System Controller Firmware (SCFW) based power domain driver.
> diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
> index 0ac04df..1b2e15b 100644
> --- a/drivers/firmware/imx/Makefile
> +++ b/drivers/firmware/imx/Makefile
> @@ -1,2 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
> +obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
> +obj-$(CONFIG_IMX_SCU_PD) += scu-pd.o
> diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
> new file mode 100644
> index 0000000..af5b9e4
> --- /dev/null
> +++ b/drivers/firmware/imx/scu-pd.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Copyright 2017~2018 NXP
> + * Dong Aisheng <aisheng.dong@nxp.com>
> + *
> + * Implementation of the SCU based Power Domains
> + */
> +
> +#define pr_fmt(fmt) "scu_pd: " fmt
> +
> +#include <linux/firmware/imx/sci.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +
> +struct imx_sc_pm_domain {
> + struct generic_pm_domain pd;
> + u32 rsrc_id;
> +};
> +
> +static struct imx_sc_ipc *pm_ipc_handle;
> +
> +/* SCU Power Mode Protocol definition */
> +struct imx_sc_msg_req_set_resource_power_mode {
> + struct imx_sc_rpc_msg hdr;
> + u16 resource;
> + u8 mode;
> +} __packed;
> +
> +static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
> +{
> + struct imx_sc_msg_req_set_resource_power_mode msg;
> + struct imx_sc_rpc_msg *hdr = &msg.hdr;
> + struct imx_sc_pm_domain *pd;
> + int ret;
> +
> + pd = container_of(domain, struct imx_sc_pm_domain, pd);
> +
> + hdr->ver = IMX_SC_RPC_VERSION;
> + hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
> + hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE;
> + hdr->size = 2;
> +
> + msg.resource = pd->rsrc_id;
> + msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
> +
> + ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
> + if (ret)
> + pr_err("failed to power %s resource %d ret %d\n",
> + power_on ? "up" : "off", pd->rsrc_id, ret);
> +
> + return ret;
> +}
> +
> +static int imx_sc_pd_power_on(struct generic_pm_domain *domain)
> +{
> + return imx_sc_pd_power(domain, true);
> +}
> +
> +static int imx_sc_pd_power_off(struct generic_pm_domain *domain)
> +{
> + return imx_sc_pd_power(domain, false);
> +}
> +
> +static struct generic_pm_domain *imx_sc_pm_add_one_domain(
> + struct device_node *np,
> + struct generic_pm_domain *genpd_parent)
> +{
> + struct imx_sc_pm_domain *imx_sc_pd;
> + u32 rsrc_id;
> + int ret;
> +
> + imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
> + if (!imx_sc_pd)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!of_property_read_u32(np, "reg", &rsrc_id)) {
> + if (rsrc_id > IMX_SC_R_LAST) {
> + pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
> + ret = -EINVAL;
> + goto err;
> + }
> + imx_sc_pd->rsrc_id = rsrc_id;
> + } else {
> + imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
> + }
> +
> + if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
> + imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
> + imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
> + }
> +
> + imx_sc_pd->pd.name = np->name;
> +
> + ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
> + if (ret < 0)
> + goto err;
> +
> + if (genpd_parent) {
> + ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
> + if (ret)
> + goto err;
> + }
> +
> + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> + if (!ret)
> + return &imx_sc_pd->pd;
> +
> + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> +
> +err:
> + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
> + kfree(imx_sc_pd);
> + return ERR_PTR(ret);
> +}
> +
> +static void imx_sc_pm_add_pm_domains(struct device_node *parent,
> + struct generic_pm_domain *genpd_parent)
> +{
> + struct generic_pm_domain *pd;
> + struct device_node *np;
> +
> + for_each_child_of_node(parent, np) {
> + pd = imx_sc_pm_add_one_domain(np, genpd_parent);
> + if (!IS_ERR(pd))
> + imx_sc_pm_add_pm_domains(np, pd);
> + }
> +}
> +
> +static int imx_sc_pd_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + int ret;
> +
> + ret = imx_scu_get_handle(&pm_ipc_handle);
> + if (ret)
> + return ret;
> +
> + imx_sc_pm_add_pm_domains(np, NULL);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id imx_sc_pd_match[] = {
> + { .compatible = "fsl,scu-pd", },
Has the compatible been documented? We used to encode a soc name in the
compatible to specify the programming model of particular hardware
block. Is this power domain block generic enough with no need of doing
so?
Shawn
> + { /* sentinel */ }
> +};
> +
> +static struct platform_driver imx_sc_pd_driver = {
> + .driver = {
> + .name = "imx-scu-pd",
> + .of_match_table = imx_sc_pd_match,
> + },
> + .probe = imx_sc_pd_probe,
> +};
> +builtin_platform_driver(imx_sc_pd_driver);
> +
> +MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
> +MODULE_DESCRIPTION("IMX SCU Power Domain driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH V4 2/2] firmware: imx: add SCU power domain driver
@ 2018-10-09 0:53 ` Shawn Guo
0 siblings, 0 replies; 22+ messages in thread
From: Shawn Guo @ 2018-10-09 0:53 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
> Some i.MX SoCs contain a system controller that is responsible for
> controlling the state of the IPs that are present. Communication
> between the host processor running an OS and the system controller
> happens through a SCU protocol. This patch adds SCU protocol based
> power domains drivers.
>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: linux-pm at vger.kernel.org
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
@Sascha, are you okay with this?
> ---
> ChangeLog:
> v3->v4:
> * update firmware headfile patch from include/soc/imx to
> include/linux/firmware/imx
> v2->v3:
> * name of structures/enums updated with imx_sc prefix
> v1->v2:
> * move into drivers/firmware/imx
> * Implement sc_pm_set_resource_power_mode() API in the driver instead
> of call it via SCU API according to Sascha's suggestion
> ---
> drivers/firmware/imx/Kconfig | 6 ++
> drivers/firmware/imx/Makefile | 3 +-
> drivers/firmware/imx/scu-pd.c | 167 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 175 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/imx/scu-pd.c
>
> diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> index b170c28..6a7a7c2 100644
> --- a/drivers/firmware/imx/Kconfig
> +++ b/drivers/firmware/imx/Kconfig
> @@ -9,3 +9,9 @@ config IMX_SCU
>
> This driver manages the IPC interface between host CPU and the
> SCU firmware running on M4.
> +
> +config IMX_SCU_PD
> + bool "IMX SCU Power Domain driver"
> + depends on IMX_SCU
> + help
> + The System Controller Firmware (SCFW) based power domain driver.
> diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
> index 0ac04df..1b2e15b 100644
> --- a/drivers/firmware/imx/Makefile
> +++ b/drivers/firmware/imx/Makefile
> @@ -1,2 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
> +obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
> +obj-$(CONFIG_IMX_SCU_PD) += scu-pd.o
> diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
> new file mode 100644
> index 0000000..af5b9e4
> --- /dev/null
> +++ b/drivers/firmware/imx/scu-pd.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Copyright 2017~2018 NXP
> + * Dong Aisheng <aisheng.dong@nxp.com>
> + *
> + * Implementation of the SCU based Power Domains
> + */
> +
> +#define pr_fmt(fmt) "scu_pd: " fmt
> +
> +#include <linux/firmware/imx/sci.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +
> +struct imx_sc_pm_domain {
> + struct generic_pm_domain pd;
> + u32 rsrc_id;
> +};
> +
> +static struct imx_sc_ipc *pm_ipc_handle;
> +
> +/* SCU Power Mode Protocol definition */
> +struct imx_sc_msg_req_set_resource_power_mode {
> + struct imx_sc_rpc_msg hdr;
> + u16 resource;
> + u8 mode;
> +} __packed;
> +
> +static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
> +{
> + struct imx_sc_msg_req_set_resource_power_mode msg;
> + struct imx_sc_rpc_msg *hdr = &msg.hdr;
> + struct imx_sc_pm_domain *pd;
> + int ret;
> +
> + pd = container_of(domain, struct imx_sc_pm_domain, pd);
> +
> + hdr->ver = IMX_SC_RPC_VERSION;
> + hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
> + hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE;
> + hdr->size = 2;
> +
> + msg.resource = pd->rsrc_id;
> + msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
> +
> + ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
> + if (ret)
> + pr_err("failed to power %s resource %d ret %d\n",
> + power_on ? "up" : "off", pd->rsrc_id, ret);
> +
> + return ret;
> +}
> +
> +static int imx_sc_pd_power_on(struct generic_pm_domain *domain)
> +{
> + return imx_sc_pd_power(domain, true);
> +}
> +
> +static int imx_sc_pd_power_off(struct generic_pm_domain *domain)
> +{
> + return imx_sc_pd_power(domain, false);
> +}
> +
> +static struct generic_pm_domain *imx_sc_pm_add_one_domain(
> + struct device_node *np,
> + struct generic_pm_domain *genpd_parent)
> +{
> + struct imx_sc_pm_domain *imx_sc_pd;
> + u32 rsrc_id;
> + int ret;
> +
> + imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
> + if (!imx_sc_pd)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!of_property_read_u32(np, "reg", &rsrc_id)) {
> + if (rsrc_id > IMX_SC_R_LAST) {
> + pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
> + ret = -EINVAL;
> + goto err;
> + }
> + imx_sc_pd->rsrc_id = rsrc_id;
> + } else {
> + imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
> + }
> +
> + if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
> + imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
> + imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
> + }
> +
> + imx_sc_pd->pd.name = np->name;
> +
> + ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
> + if (ret < 0)
> + goto err;
> +
> + if (genpd_parent) {
> + ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
> + if (ret)
> + goto err;
> + }
> +
> + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> + if (!ret)
> + return &imx_sc_pd->pd;
> +
> + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> +
> +err:
> + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
> + kfree(imx_sc_pd);
> + return ERR_PTR(ret);
> +}
> +
> +static void imx_sc_pm_add_pm_domains(struct device_node *parent,
> + struct generic_pm_domain *genpd_parent)
> +{
> + struct generic_pm_domain *pd;
> + struct device_node *np;
> +
> + for_each_child_of_node(parent, np) {
> + pd = imx_sc_pm_add_one_domain(np, genpd_parent);
> + if (!IS_ERR(pd))
> + imx_sc_pm_add_pm_domains(np, pd);
> + }
> +}
> +
> +static int imx_sc_pd_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + int ret;
> +
> + ret = imx_scu_get_handle(&pm_ipc_handle);
> + if (ret)
> + return ret;
> +
> + imx_sc_pm_add_pm_domains(np, NULL);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id imx_sc_pd_match[] = {
> + { .compatible = "fsl,scu-pd", },
Has the compatible been documented? We used to encode a soc name in the
compatible to specify the programming model of particular hardware
block. Is this power domain block generic enough with no need of doing
so?
Shawn
> + { /* sentinel */ }
> +};
> +
> +static struct platform_driver imx_sc_pd_driver = {
> + .driver = {
> + .name = "imx-scu-pd",
> + .of_match_table = imx_sc_pd_match,
> + },
> + .probe = imx_sc_pd_probe,
> +};
> +builtin_platform_driver(imx_sc_pd_driver);
> +
> +MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
> +MODULE_DESCRIPTION("IMX SCU Power Domain driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
2018-10-09 0:53 ` Shawn Guo
@ 2018-10-09 2:48 ` A.s. Dong
-1 siblings, 0 replies; 22+ messages in thread
From: A.s. Dong @ 2018-10-09 2:48 UTC (permalink / raw)
To: Shawn Guo
Cc: ulf.hansson, dongas86, linux-pm, Sascha Hauer, khilman, rjw,
dl-linux-imx, kernel, Fabio Estevam, linux-arm-kernel
> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo@kernel.org]
> Sent: Tuesday, October 9, 2018 8:54 AM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; ulf.hansson@linaro.org;
> dongas86@gmail.com; khilman@kernel.org; linux-pm@vger.kernel.org;
> rjw@rjwysocki.net; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>; Sascha
> Hauer <s.hauer@pengutronix.de>
> Subject: Re: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
>
> On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
> > Some i.MX SoCs contain a system controller that is responsible for
> > controlling the state of the IPs that are present. Communication
> > between the host processor running an OS and the system controller
> > happens through a SCU protocol. This patch adds SCU protocol based
> > power domains drivers.
> >
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Kevin Hilman <khilman@kernel.org>
> > Cc: linux-pm@vger.kernel.org
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>
> @Sascha, are you okay with this?
>
Yes, it would be good if we can have Sasha's sign offs.
> > ---
> > ChangeLog:
> > v3->v4:
> > * update firmware headfile patch from include/soc/imx to
> > include/linux/firmware/imx
> > v2->v3:
> > * name of structures/enums updated with imx_sc prefix
> > v1->v2:
> > * move into drivers/firmware/imx
> > * Implement sc_pm_set_resource_power_mode() API in the driver instead
> > of call it via SCU API according to Sascha's suggestion
> > ---
> > drivers/firmware/imx/Kconfig | 6 ++
> > drivers/firmware/imx/Makefile | 3 +-
> > drivers/firmware/imx/scu-pd.c | 167
> > ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 175 insertions(+), 1 deletion(-) create mode 100644
> > drivers/firmware/imx/scu-pd.c
> >
> > diff --git a/drivers/firmware/imx/Kconfig
> > b/drivers/firmware/imx/Kconfig index b170c28..6a7a7c2 100644
> > --- a/drivers/firmware/imx/Kconfig
> > +++ b/drivers/firmware/imx/Kconfig
> > @@ -9,3 +9,9 @@ config IMX_SCU
> >
> > This driver manages the IPC interface between host CPU and the
> > SCU firmware running on M4.
> > +
> > +config IMX_SCU_PD
> > + bool "IMX SCU Power Domain driver"
> > + depends on IMX_SCU
> > + help
> > + The System Controller Firmware (SCFW) based power domain driver.
> > diff --git a/drivers/firmware/imx/Makefile
> > b/drivers/firmware/imx/Makefile index 0ac04df..1b2e15b 100644
> > --- a/drivers/firmware/imx/Makefile
> > +++ b/drivers/firmware/imx/Makefile
> > @@ -1,2 +1,3 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
> > +obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
> > +obj-$(CONFIG_IMX_SCU_PD) += scu-pd.o
> > diff --git a/drivers/firmware/imx/scu-pd.c
> > b/drivers/firmware/imx/scu-pd.c new file mode 100644 index
> > 0000000..af5b9e4
> > --- /dev/null
> > +++ b/drivers/firmware/imx/scu-pd.c
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + * Dong Aisheng <aisheng.dong@nxp.com>
> > + *
> > + * Implementation of the SCU based Power Domains */
> > +
> > +#define pr_fmt(fmt) "scu_pd: " fmt
> > +
> > +#include <linux/firmware/imx/sci.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/slab.h>
> > +
> > +struct imx_sc_pm_domain {
> > + struct generic_pm_domain pd;
> > + u32 rsrc_id;
> > +};
> > +
> > +static struct imx_sc_ipc *pm_ipc_handle;
> > +
> > +/* SCU Power Mode Protocol definition */ struct
> > +imx_sc_msg_req_set_resource_power_mode {
> > + struct imx_sc_rpc_msg hdr;
> > + u16 resource;
> > + u8 mode;
> > +} __packed;
> > +
> > +static int imx_sc_pd_power(struct generic_pm_domain *domain, bool
> > +power_on) {
> > + struct imx_sc_msg_req_set_resource_power_mode msg;
> > + struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > + struct imx_sc_pm_domain *pd;
> > + int ret;
> > +
> > + pd = container_of(domain, struct imx_sc_pm_domain, pd);
> > +
> > + hdr->ver = IMX_SC_RPC_VERSION;
> > + hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
> > + hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE;
> > + hdr->size = 2;
> > +
> > + msg.resource = pd->rsrc_id;
> > + msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON :
> IMX_SC_PM_PW_MODE_LP;
> > +
> > + ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
> > + if (ret)
> > + pr_err("failed to power %s resource %d ret %d\n",
> > + power_on ? "up" : "off", pd->rsrc_id, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int imx_sc_pd_power_on(struct generic_pm_domain *domain) {
> > + return imx_sc_pd_power(domain, true); }
> > +
> > +static int imx_sc_pd_power_off(struct generic_pm_domain *domain) {
> > + return imx_sc_pd_power(domain, false); }
> > +
> > +static struct generic_pm_domain *imx_sc_pm_add_one_domain(
> > + struct device_node *np,
> > + struct generic_pm_domain *genpd_parent) {
> > + struct imx_sc_pm_domain *imx_sc_pd;
> > + u32 rsrc_id;
> > + int ret;
> > +
> > + imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
> > + if (!imx_sc_pd)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (!of_property_read_u32(np, "reg", &rsrc_id)) {
> > + if (rsrc_id > IMX_SC_R_LAST) {
> > + pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > + imx_sc_pd->rsrc_id = rsrc_id;
> > + } else {
> > + imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
> > + }
> > +
> > + if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
> > + imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
> > + imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
> > + }
> > +
> > + imx_sc_pd->pd.name = np->name;
> > +
> > + ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (genpd_parent) {
> > + ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
> > + if (ret)
> > + goto err;
> > + }
> > +
> > + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> > + if (!ret)
> > + return &imx_sc_pd->pd;
> > +
> > + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> > +
> > +err:
> > + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
> > + kfree(imx_sc_pd);
> > + return ERR_PTR(ret);
> > +}
> > +
> > +static void imx_sc_pm_add_pm_domains(struct device_node *parent,
> > + struct generic_pm_domain *genpd_parent) {
> > + struct generic_pm_domain *pd;
> > + struct device_node *np;
> > +
> > + for_each_child_of_node(parent, np) {
> > + pd = imx_sc_pm_add_one_domain(np, genpd_parent);
> > + if (!IS_ERR(pd))
> > + imx_sc_pm_add_pm_domains(np, pd);
> > + }
> > +}
> > +
> > +static int imx_sc_pd_probe(struct platform_device *pdev) {
> > + struct device_node *np = pdev->dev.of_node;
> > + int ret;
> > +
> > + ret = imx_scu_get_handle(&pm_ipc_handle);
> > + if (ret)
> > + return ret;
> > +
> > + imx_sc_pm_add_pm_domains(np, NULL);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id imx_sc_pd_match[] = {
> > + { .compatible = "fsl,scu-pd", },
>
> Has the compatible been documented? We used to encode a soc name in the
> compatible to specify the programming model of particular hardware block.
> Is this power domain block generic enough with no need of doing so?
>
Yes, it's a generic binding and has been documented in SCU binding doc.
See:
https://www.spinics.net/lists/arm-kernel/msg680738.html
Regards
Dong Aisheng
> Shawn
>
> > + { /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver imx_sc_pd_driver = {
> > + .driver = {
> > + .name = "imx-scu-pd",
> > + .of_match_table = imx_sc_pd_match,
> > + },
> > + .probe = imx_sc_pd_probe,
> > +};
> > +builtin_platform_driver(imx_sc_pd_driver);
> > +
> > +MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
> > +MODULE_DESCRIPTION("IMX SCU Power Domain driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flist
> >
> s.infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=02%7C
> >
> 01%7Caisheng.dong%40nxp.com%7Ce7cc7fda44ad4f62bc3d08d62d81c3e2%
> 7C686ea
> >
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636746432705451877&
> sdata=30j
> > 5I%2Fek2U5vujNu2Yzj1Wbsh0BvbxriRFcXUjDOeg4%3D&reserved=0
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH V4 2/2] firmware: imx: add SCU power domain driver
@ 2018-10-09 2:48 ` A.s. Dong
0 siblings, 0 replies; 22+ messages in thread
From: A.s. Dong @ 2018-10-09 2:48 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo at kernel.org]
> Sent: Tuesday, October 9, 2018 8:54 AM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel at lists.infradead.org; ulf.hansson at linaro.org;
> dongas86 at gmail.com; khilman at kernel.org; linux-pm at vger.kernel.org;
> rjw at rjwysocki.net; dl-linux-imx <linux-imx@nxp.com>;
> kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>; Sascha
> Hauer <s.hauer@pengutronix.de>
> Subject: Re: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
>
> On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
> > Some i.MX SoCs contain a system controller that is responsible for
> > controlling the state of the IPs that are present. Communication
> > between the host processor running an OS and the system controller
> > happens through a SCU protocol. This patch adds SCU protocol based
> > power domains drivers.
> >
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Kevin Hilman <khilman@kernel.org>
> > Cc: linux-pm at vger.kernel.org
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>
> @Sascha, are you okay with this?
>
Yes, it would be good if we can have Sasha's sign offs.
> > ---
> > ChangeLog:
> > v3->v4:
> > * update firmware headfile patch from include/soc/imx to
> > include/linux/firmware/imx
> > v2->v3:
> > * name of structures/enums updated with imx_sc prefix
> > v1->v2:
> > * move into drivers/firmware/imx
> > * Implement sc_pm_set_resource_power_mode() API in the driver instead
> > of call it via SCU API according to Sascha's suggestion
> > ---
> > drivers/firmware/imx/Kconfig | 6 ++
> > drivers/firmware/imx/Makefile | 3 +-
> > drivers/firmware/imx/scu-pd.c | 167
> > ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 175 insertions(+), 1 deletion(-) create mode 100644
> > drivers/firmware/imx/scu-pd.c
> >
> > diff --git a/drivers/firmware/imx/Kconfig
> > b/drivers/firmware/imx/Kconfig index b170c28..6a7a7c2 100644
> > --- a/drivers/firmware/imx/Kconfig
> > +++ b/drivers/firmware/imx/Kconfig
> > @@ -9,3 +9,9 @@ config IMX_SCU
> >
> > This driver manages the IPC interface between host CPU and the
> > SCU firmware running on M4.
> > +
> > +config IMX_SCU_PD
> > + bool "IMX SCU Power Domain driver"
> > + depends on IMX_SCU
> > + help
> > + The System Controller Firmware (SCFW) based power domain driver.
> > diff --git a/drivers/firmware/imx/Makefile
> > b/drivers/firmware/imx/Makefile index 0ac04df..1b2e15b 100644
> > --- a/drivers/firmware/imx/Makefile
> > +++ b/drivers/firmware/imx/Makefile
> > @@ -1,2 +1,3 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
> > +obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o
> > +obj-$(CONFIG_IMX_SCU_PD) += scu-pd.o
> > diff --git a/drivers/firmware/imx/scu-pd.c
> > b/drivers/firmware/imx/scu-pd.c new file mode 100644 index
> > 0000000..af5b9e4
> > --- /dev/null
> > +++ b/drivers/firmware/imx/scu-pd.c
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + * Dong Aisheng <aisheng.dong@nxp.com>
> > + *
> > + * Implementation of the SCU based Power Domains */
> > +
> > +#define pr_fmt(fmt) "scu_pd: " fmt
> > +
> > +#include <linux/firmware/imx/sci.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/slab.h>
> > +
> > +struct imx_sc_pm_domain {
> > + struct generic_pm_domain pd;
> > + u32 rsrc_id;
> > +};
> > +
> > +static struct imx_sc_ipc *pm_ipc_handle;
> > +
> > +/* SCU Power Mode Protocol definition */ struct
> > +imx_sc_msg_req_set_resource_power_mode {
> > + struct imx_sc_rpc_msg hdr;
> > + u16 resource;
> > + u8 mode;
> > +} __packed;
> > +
> > +static int imx_sc_pd_power(struct generic_pm_domain *domain, bool
> > +power_on) {
> > + struct imx_sc_msg_req_set_resource_power_mode msg;
> > + struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > + struct imx_sc_pm_domain *pd;
> > + int ret;
> > +
> > + pd = container_of(domain, struct imx_sc_pm_domain, pd);
> > +
> > + hdr->ver = IMX_SC_RPC_VERSION;
> > + hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
> > + hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE;
> > + hdr->size = 2;
> > +
> > + msg.resource = pd->rsrc_id;
> > + msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON :
> IMX_SC_PM_PW_MODE_LP;
> > +
> > + ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
> > + if (ret)
> > + pr_err("failed to power %s resource %d ret %d\n",
> > + power_on ? "up" : "off", pd->rsrc_id, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int imx_sc_pd_power_on(struct generic_pm_domain *domain) {
> > + return imx_sc_pd_power(domain, true); }
> > +
> > +static int imx_sc_pd_power_off(struct generic_pm_domain *domain) {
> > + return imx_sc_pd_power(domain, false); }
> > +
> > +static struct generic_pm_domain *imx_sc_pm_add_one_domain(
> > + struct device_node *np,
> > + struct generic_pm_domain *genpd_parent) {
> > + struct imx_sc_pm_domain *imx_sc_pd;
> > + u32 rsrc_id;
> > + int ret;
> > +
> > + imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
> > + if (!imx_sc_pd)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (!of_property_read_u32(np, "reg", &rsrc_id)) {
> > + if (rsrc_id > IMX_SC_R_LAST) {
> > + pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > + imx_sc_pd->rsrc_id = rsrc_id;
> > + } else {
> > + imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
> > + }
> > +
> > + if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
> > + imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
> > + imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
> > + }
> > +
> > + imx_sc_pd->pd.name = np->name;
> > +
> > + ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (genpd_parent) {
> > + ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
> > + if (ret)
> > + goto err;
> > + }
> > +
> > + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> > + if (!ret)
> > + return &imx_sc_pd->pd;
> > +
> > + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> > +
> > +err:
> > + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
> > + kfree(imx_sc_pd);
> > + return ERR_PTR(ret);
> > +}
> > +
> > +static void imx_sc_pm_add_pm_domains(struct device_node *parent,
> > + struct generic_pm_domain *genpd_parent) {
> > + struct generic_pm_domain *pd;
> > + struct device_node *np;
> > +
> > + for_each_child_of_node(parent, np) {
> > + pd = imx_sc_pm_add_one_domain(np, genpd_parent);
> > + if (!IS_ERR(pd))
> > + imx_sc_pm_add_pm_domains(np, pd);
> > + }
> > +}
> > +
> > +static int imx_sc_pd_probe(struct platform_device *pdev) {
> > + struct device_node *np = pdev->dev.of_node;
> > + int ret;
> > +
> > + ret = imx_scu_get_handle(&pm_ipc_handle);
> > + if (ret)
> > + return ret;
> > +
> > + imx_sc_pm_add_pm_domains(np, NULL);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id imx_sc_pd_match[] = {
> > + { .compatible = "fsl,scu-pd", },
>
> Has the compatible been documented? We used to encode a soc name in the
> compatible to specify the programming model of particular hardware block.
> Is this power domain block generic enough with no need of doing so?
>
Yes, it's a generic binding and has been documented in SCU binding doc.
See:
https://www.spinics.net/lists/arm-kernel/msg680738.html
Regards
Dong Aisheng
> Shawn
>
> > + { /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver imx_sc_pd_driver = {
> > + .driver = {
> > + .name = "imx-scu-pd",
> > + .of_match_table = imx_sc_pd_match,
> > + },
> > + .probe = imx_sc_pd_probe,
> > +};
> > +builtin_platform_driver(imx_sc_pd_driver);
> > +
> > +MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
> > +MODULE_DESCRIPTION("IMX SCU Power Domain driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flist
> >
> s.infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=02%7C
> >
> 01%7Caisheng.dong%40nxp.com%7Ce7cc7fda44ad4f62bc3d08d62d81c3e2%
> 7C686ea
> >
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636746432705451877&
> sdata=30j
> > 5I%2Fek2U5vujNu2Yzj1Wbsh0BvbxriRFcXUjDOeg4%3D&reserved=0
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
2018-10-07 13:10 ` Dong Aisheng
@ 2018-10-09 10:35 ` Sascha Hauer
-1 siblings, 0 replies; 22+ messages in thread
From: Sascha Hauer @ 2018-10-09 10:35 UTC (permalink / raw)
To: Dong Aisheng
Cc: ulf.hansson, dongas86, linux-pm, khilman, rjw, linux-imx, kernel,
fabio.estevam, shawnguo, linux-arm-kernel
Hi Dong,
Looks mostly ok to me, some minor comments inside.
On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
> Some i.MX SoCs contain a system controller that is responsible for
> controlling the state of the IPs that are present. Communication
> between the host processor running an OS and the system controller
> happens through a SCU protocol. This patch adds SCU protocol based
> power domains drivers.
>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: linux-pm@vger.kernel.org
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> +static struct generic_pm_domain *imx_sc_pm_add_one_domain(
> + struct device_node *np,
> + struct generic_pm_domain *genpd_parent)
> +{
> + struct imx_sc_pm_domain *imx_sc_pd;
> + u32 rsrc_id;
> + int ret;
> +
> + imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
> + if (!imx_sc_pd)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!of_property_read_u32(np, "reg", &rsrc_id)) {
> + if (rsrc_id > IMX_SC_R_LAST) {
Should this be >= instead?
> + pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
> + ret = -EINVAL;
> + goto err;
> + }
> + imx_sc_pd->rsrc_id = rsrc_id;
> + } else {
> + imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
> + }
> +
> + if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
> + imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
> + imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
> + }
Can be moved into the property exists test above.
> +
> + imx_sc_pd->pd.name = np->name;
> +
> + ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
> + if (ret < 0)
> + goto err;
> +
> + if (genpd_parent) {
> + ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
> + if (ret)
> + goto err;
> + }
> +
> + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> + if (!ret)
> + return &imx_sc_pd->pd;
> +
> + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> +
> +err:
> + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
You have a struct device *, you should use dev_* functions.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH V4 2/2] firmware: imx: add SCU power domain driver
@ 2018-10-09 10:35 ` Sascha Hauer
0 siblings, 0 replies; 22+ messages in thread
From: Sascha Hauer @ 2018-10-09 10:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi Dong,
Looks mostly ok to me, some minor comments inside.
On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
> Some i.MX SoCs contain a system controller that is responsible for
> controlling the state of the IPs that are present. Communication
> between the host processor running an OS and the system controller
> happens through a SCU protocol. This patch adds SCU protocol based
> power domains drivers.
>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: linux-pm at vger.kernel.org
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> +static struct generic_pm_domain *imx_sc_pm_add_one_domain(
> + struct device_node *np,
> + struct generic_pm_domain *genpd_parent)
> +{
> + struct imx_sc_pm_domain *imx_sc_pd;
> + u32 rsrc_id;
> + int ret;
> +
> + imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
> + if (!imx_sc_pd)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!of_property_read_u32(np, "reg", &rsrc_id)) {
> + if (rsrc_id > IMX_SC_R_LAST) {
Should this be >= instead?
> + pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
> + ret = -EINVAL;
> + goto err;
> + }
> + imx_sc_pd->rsrc_id = rsrc_id;
> + } else {
> + imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
> + }
> +
> + if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
> + imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
> + imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
> + }
Can be moved into the property exists test above.
> +
> + imx_sc_pd->pd.name = np->name;
> +
> + ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
> + if (ret < 0)
> + goto err;
> +
> + if (genpd_parent) {
> + ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
> + if (ret)
> + goto err;
> + }
> +
> + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> + if (!ret)
> + return &imx_sc_pd->pd;
> +
> + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> +
> +err:
> + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
You have a struct device *, you should use dev_* functions.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
2018-10-09 10:35 ` Sascha Hauer
@ 2018-10-09 10:48 ` A.s. Dong
-1 siblings, 0 replies; 22+ messages in thread
From: A.s. Dong @ 2018-10-09 10:48 UTC (permalink / raw)
To: Sascha Hauer
Cc: ulf.hansson, dongas86, linux-pm, khilman, rjw, dl-linux-imx,
kernel, Fabio Estevam, shawnguo, linux-arm-kernel
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: Tuesday, October 9, 2018 6:35 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; ulf.hansson@linaro.org;
> dongas86@gmail.com; khilman@kernel.org; linux-pm@vger.kernel.org;
> rjw@rjwysocki.net; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo@kernel.org
> Subject: Re: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
>
> Hi Dong,
>
> Looks mostly ok to me, some minor comments inside.
>
> On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
> > Some i.MX SoCs contain a system controller that is responsible for
> > controlling the state of the IPs that are present. Communication
> > between the host processor running an OS and the system controller
> > happens through a SCU protocol. This patch adds SCU protocol based
> > power domains drivers.
> >
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Kevin Hilman <khilman@kernel.org>
> > Cc: linux-pm@vger.kernel.org
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > +static struct generic_pm_domain *imx_sc_pm_add_one_domain(
> > + struct device_node *np,
> > + struct generic_pm_domain *genpd_parent) {
> > + struct imx_sc_pm_domain *imx_sc_pd;
> > + u32 rsrc_id;
> > + int ret;
> > +
> > + imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
> > + if (!imx_sc_pd)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (!of_property_read_u32(np, "reg", &rsrc_id)) {
> > + if (rsrc_id > IMX_SC_R_LAST) {
>
> Should this be >= instead?
>
You're right.
> > + pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > + imx_sc_pd->rsrc_id = rsrc_id;
> > + } else {
> > + imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
> > + }
> > +
> > + if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
> > + imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
> > + imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
> > + }
>
> Can be moved into the property exists test above.
>
Good suggestion
> > +
> > + imx_sc_pd->pd.name = np->name;
> > +
> > + ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (genpd_parent) {
> > + ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
> > + if (ret)
> > + goto err;
> > + }
> > +
> > + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> > + if (!ret)
> > + return &imx_sc_pd->pd;
> > +
> > + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> > +
> > +err:
> > + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
>
> You have a struct device *, you should use dev_* functions.
Good suggestion.
Will update and resend a new version.
Thanks for the carefully review.
Regards
Dong Aisheng
>
> Sascha
>
> --
> Pengutronix e.K. |
> |
> Industrial Linux Solutions |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C49
> 9fbc118b8948ed075908d62dd2e6b1%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636746781171494898&sdata=MwU9%2BAzpX8Tmwd
> pYch7ztNZXRo4aqifxbbtALjuW9lo%3D&reserved=0 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH V4 2/2] firmware: imx: add SCU power domain driver
@ 2018-10-09 10:48 ` A.s. Dong
0 siblings, 0 replies; 22+ messages in thread
From: A.s. Dong @ 2018-10-09 10:48 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, October 9, 2018 6:35 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel at lists.infradead.org; ulf.hansson at linaro.org;
> dongas86 at gmail.com; khilman at kernel.org; linux-pm at vger.kernel.org;
> rjw at rjwysocki.net; dl-linux-imx <linux-imx@nxp.com>;
> kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo at kernel.org
> Subject: Re: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
>
> Hi Dong,
>
> Looks mostly ok to me, some minor comments inside.
>
> On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
> > Some i.MX SoCs contain a system controller that is responsible for
> > controlling the state of the IPs that are present. Communication
> > between the host processor running an OS and the system controller
> > happens through a SCU protocol. This patch adds SCU protocol based
> > power domains drivers.
> >
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Kevin Hilman <khilman@kernel.org>
> > Cc: linux-pm at vger.kernel.org
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > +static struct generic_pm_domain *imx_sc_pm_add_one_domain(
> > + struct device_node *np,
> > + struct generic_pm_domain *genpd_parent) {
> > + struct imx_sc_pm_domain *imx_sc_pd;
> > + u32 rsrc_id;
> > + int ret;
> > +
> > + imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
> > + if (!imx_sc_pd)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (!of_property_read_u32(np, "reg", &rsrc_id)) {
> > + if (rsrc_id > IMX_SC_R_LAST) {
>
> Should this be >= instead?
>
You're right.
> > + pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > + imx_sc_pd->rsrc_id = rsrc_id;
> > + } else {
> > + imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
> > + }
> > +
> > + if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
> > + imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
> > + imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
> > + }
>
> Can be moved into the property exists test above.
>
Good suggestion
> > +
> > + imx_sc_pd->pd.name = np->name;
> > +
> > + ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (genpd_parent) {
> > + ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
> > + if (ret)
> > + goto err;
> > + }
> > +
> > + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> > + if (!ret)
> > + return &imx_sc_pd->pd;
> > +
> > + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> > +
> > +err:
> > + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
>
> You have a struct device *, you should use dev_* functions.
Good suggestion.
Will update and resend a new version.
Thanks for the carefully review.
Regards
Dong Aisheng
>
> Sascha
>
> --
> Pengutronix e.K. |
> |
> Industrial Linux Solutions |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C49
> 9fbc118b8948ed075908d62dd2e6b1%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636746781171494898&sdata=MwU9%2BAzpX8Tmwd
> pYch7ztNZXRo4aqifxbbtALjuW9lo%3D&reserved=0 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
2018-10-09 10:48 ` A.s. Dong
@ 2018-10-09 11:19 ` A.s. Dong
-1 siblings, 0 replies; 22+ messages in thread
From: A.s. Dong @ 2018-10-09 11:19 UTC (permalink / raw)
To: Sascha Hauer
Cc: ulf.hansson, dongas86, linux-pm, khilman, rjw, dl-linux-imx,
kernel, Fabio Estevam, shawnguo, linux-arm-kernel
Hi Sascha,
> > > +
> > > + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> > > + if (!ret)
> > > + return &imx_sc_pd->pd;
> > > +
> > > + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> > > +
> > > +err:
> > > + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
> >
> > You have a struct device *, you should use dev_* functions.
>
> Good suggestion.
>
A bit more thinking of it.
It may be not quite suitable to use dev_* here for print functions
as we need pass in a pointer of struct device for debug purpose only.
And we need change other two places as well which are in the same situation.
In current patch, we already define the print format at the top of the file.
#define pr_fmt(fmt) "scu_pd: " fmt
As SCU PD has only one device, it may does not help too much to use dev_* functions.
Do you think it's reasonable?
If you're okay with this, I will drop this change and resend with other things updated
according to your suggestions.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH V4 2/2] firmware: imx: add SCU power domain driver
@ 2018-10-09 11:19 ` A.s. Dong
0 siblings, 0 replies; 22+ messages in thread
From: A.s. Dong @ 2018-10-09 11:19 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sascha,
> > > +
> > > + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> > > + if (!ret)
> > > + return &imx_sc_pd->pd;
> > > +
> > > + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> > > +
> > > +err:
> > > + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
> >
> > You have a struct device *, you should use dev_* functions.
>
> Good suggestion.
>
A bit more thinking of it.
It may be not quite suitable to use dev_* here for print functions
as we need pass in a pointer of struct device for debug purpose only.
And we need change other two places as well which are in the same situation.
In current patch, we already define the print format at the top of the file.
#define pr_fmt(fmt) "scu_pd: " fmt
As SCU PD has only one device, it may does not help too much to use dev_* functions.
Do you think it's reasonable?
If you're okay with this, I will drop this change and resend with other things updated
according to your suggestions.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH V4 2/2] firmware: imx: add SCU power domain driver
2018-10-09 11:19 ` A.s. Dong
@ 2018-10-10 8:42 ` A.s. Dong
-1 siblings, 0 replies; 22+ messages in thread
From: A.s. Dong @ 2018-10-10 8:42 UTC (permalink / raw)
To: Sascha Hauer
Cc: ulf.hansson, dongas86, linux-pm, khilman, rjw, dl-linux-imx,
kernel, Fabio Estevam, shawnguo, linux-arm-kernel
Hi Sascha,
> Hi Sascha,
>
> > > > +
> > > > + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> > > > + if (!ret)
> > > > + return &imx_sc_pd->pd;
> > > > +
> > > > + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> > > > +
> > > > +err:
> > > > + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
> > >
> > > You have a struct device *, you should use dev_* functions.
> >
> > Good suggestion.
> >
>
> A bit more thinking of it.
>
> It may be not quite suitable to use dev_* here for print functions as we need
> pass in a pointer of struct device for debug purpose only.
> And we need change other two places as well which are in the same situation.
>
> In current patch, we already define the print format at the top of the file.
> #define pr_fmt(fmt) "scu_pd: " fmt
> As SCU PD has only one device, it may does not help too much to use dev_*
> functions.
>
> Do you think it's reasonable?
>
> If you're okay with this, I will drop this change and resend with other things
> updated according to your suggestions.
>
Are you okay with this?
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH V4 2/2] firmware: imx: add SCU power domain driver
@ 2018-10-10 8:42 ` A.s. Dong
0 siblings, 0 replies; 22+ messages in thread
From: A.s. Dong @ 2018-10-10 8:42 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sascha,
> Hi Sascha,
>
> > > > +
> > > > + ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
> > > > + if (!ret)
> > > > + return &imx_sc_pd->pd;
> > > > +
> > > > + pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
> > > > +
> > > > +err:
> > > > + pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
> > >
> > > You have a struct device *, you should use dev_* functions.
> >
> > Good suggestion.
> >
>
> A bit more thinking of it.
>
> It may be not quite suitable to use dev_* here for print functions as we need
> pass in a pointer of struct device for debug purpose only.
> And we need change other two places as well which are in the same situation.
>
> In current patch, we already define the print format at the top of the file.
> #define pr_fmt(fmt) "scu_pd: " fmt
> As SCU PD has only one device, it may does not help too much to use dev_*
> functions.
>
> Do you think it's reasonable?
>
> If you're okay with this, I will drop this change and resend with other things
> updated according to your suggestions.
>
Are you okay with this?
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 22+ messages in thread