[RESEND PATCH 0/3] Add power domain driver support for imx8m family

[RESEND PATCH 0/3] Add power domain driver support for imx8m family

[Jacky Bai]

I just resend this patchset again to let us rethink & find out a quick way enable
the power domain support in mainline to help other modules upstream.

The GPC module is used for system power management for CPU core & peripheral's
power domain. For the whole i.MX8M family, different SOC has different power
domain design. Some power domains need special on/off flow(need to access the
register out of the GPC module).

It makes us hard to reuse the GPCv2 driver to cover all the different power
up sequence. Each time a new SOC is added, we must modify the GPCv2 power domain
driver to make it resuable for it. We need to add a lot of code for each new chip.
We need to access the SRC & SS's GPR in GPCv2 power domain driver, it is
burden to maintain the GPCv2 power domain driver. For example, in the future
i.MX8MP, there are ~20 power domains, some of the power domain need some special
handling only for this specific chip, same situation on i.MX8MM & i.MX8MN.

THis patchset add a more generic power domain driver that give us the possibility
to use one driver to cover the whole i.MX8M family power domain in kernel side.
kernel side driver don't need to handle the power domain difference time to time.
All the power domain on/off sequence can be abstracted & handled in TF-A side.
it can simplify the power domain handling in kernel side. All the power domain
details can be hiden to TF-A side. TF-A image is SOC specific, we don't need
to care more about the one image principle.

I know some guys suggest to use SCMI to implement the power domains, but it is
a long way, need more time to investigate. especially, for the current SCMI power
domain, it can not meet all our requirement for power domain management. On i.MX8M,
some of the power domain on/off need to handle clock and external regulators, it is
not a generic handling for other SOC vendors, I think.

Additonally, the SiP Service Calls provide interfaces to SoC implementation specific
services on this platform. For example, Secure platform initialization, configuration
and some power control. I don't think it can not be used for specific SOC.

Jacky Bai (3):
  dt-bindings: power: Add power domain binding for i.mx8m family
  soc: imx: Add power domain driver support for i.mx8m family
  arm64: dts: freescale: Add power domain nodes for i.mx8mm

 .../bindings/power/fsl,imx8m-genpd.txt        |  46 ++++
 arch/arm64/boot/dts/freescale/imx8mm.dtsi     | 103 ++++++++
 drivers/soc/imx/Kconfig                       |   6 +
 drivers/soc/imx/Makefile                      |   1 +
 drivers/soc/imx/imx8m_pm_domains.c            | 224 ++++++++++++++++++
 include/soc/imx/imx_sip.h                     |  12 +
 6 files changed, 392 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt
 create mode 100644 drivers/soc/imx/imx8m_pm_domains.c
 create mode 100644 include/soc/imx/imx_sip.h

-- 
2.21.0

[RESEND PATCH 1/3] dt-bindings: power: Add power domain binding for i.mx8m family

[Jacky Bai]

Add the binding doc of power domain for i.MX8M SOC family.

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
 .../bindings/power/fsl,imx8m-genpd.txt        | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt

diff --git a/Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt b/Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt
new file mode 100644
index 000000000000..a92a7103de38
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt
@@ -0,0 +1,46 @@
+Device Tree Bindings for Freescale i.MX8M Generic Power Domain
+==============================================================
+The binding for the i.MX8M Generic power Domain[1].
+
+[1] Documentation/devicetree/bindings/power/power_domain.txt
+
+Required properties:
+
+ - compatible: should be of:
+	- "fsl,imx8m-power-domain"
+ - #power-domain-cells: Number of cells in a PM domain Specifier, must be 0
+ - domain-index: should be the domain index number need to pass to TF-A
+ - domain-name: the name of this pm domain
+
+Optional properties:
+ - clocks: a number of phandles to clocks that need to be enabled during
+   domain power-up sequence to ensure reset propagation into devices
+   located inside this power domain
+ - power-supply: Power supply used to power the domain
+ - parent-domains: the phandle to the parent power domain
+
+example:
+	vpu_g1_pd: vpug1-pd {
+		compatible = "fsl,imx8mm-pm-domain";
+		#power-domain-cells = <0>;
+		domain-index = <6>;
+		domain-name = "vpu_g1";
+		parent-domains = <&vpumix_pd>;
+		clocks = <&clk IMX8MM_CLK_VPU_G1_ROOT>;
+	};
+
+
+Specifying Power domain for IP modules
+======================================
+
+IP cores belonging to a power domain should contain a 'power-domains'
+property that is a phandle for PGC node representing the domain.
+
+Example of a device that is part of the vpu_g1 power domain:
+	vpu_g1: vpu_g1@38300000 {
+		/* ... */
+		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "irq_hantro";
+		/* ... */
+		power-domains = <&vpu_g1_pd>;
+	};
-- 
2.21.0

[RESEND PATCH 2/3] soc: imx: Add power domain driver support for i.mx8m family

[Jacky Bai]

The i.MX8M family is a set of NXP product focus on delivering
the latest and greatest video and audio experience combining
state-of-the-art media-specific features with high-performance
processing while optimized for lowest power consumption.

i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all
belong to this family. A GPC module is used to manage all the
PU power domain on/off. But the situation is that the number of
power domains & the power up sequence has significate difference
on those SoCs. Even on the same SoC. The power up sequence still
has big difference. It makes us hard to reuse the GPCv2 driver to
cover the whole i.MX8M family. Each time a new SoC is supported in
the mainline kernel, we need to modify the GPCv2 driver to support
it. We need to add or modify hundred lines of code in worst case.
It is a bad practice for the driver maintainability.

This driver add a more generic power domain driver that the actual
power on/off is done by TF-A code. the abstraction give us the
possibility that using one driver to cover the whole i.MX8M family
in kernel side.

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
 drivers/soc/imx/Kconfig            |   6 +
 drivers/soc/imx/Makefile           |   1 +
 drivers/soc/imx/imx8m_pm_domains.c | 224 +++++++++++++++++++++++++++++
 include/soc/imx/imx_sip.h          |  12 ++
 4 files changed, 243 insertions(+)
 create mode 100644 drivers/soc/imx/imx8m_pm_domains.c
 create mode 100644 include/soc/imx/imx_sip.h

diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig
index 8aaebf13e2e6..2f3be81aabea 100644
--- a/drivers/soc/imx/Kconfig
+++ b/drivers/soc/imx/Kconfig
@@ -17,4 +17,10 @@ config IMX_SCU_SOC
 	  Controller Unit SoC info module, it will provide the SoC info
 	  like SoC family, ID and revision etc.
 
+config IMX8M_PM_DOMAINS
+	bool "i.MX8M PM domains"
+	depends on ARCH_MXC || (COMPILE_TEST && OF)
+	depends on PM
+	select PM_GENERIC_DOMAINS
+
 endmenu
diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index cf9ca42ff739..469e721b479e 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
 obj-$(CONFIG_ARCH_MXC) += soc-imx8.o
 obj-$(CONFIG_IMX_SCU_SOC) += soc-imx-scu.o
+obj-$(CONFIG_IMX8M_PM_DOMAINS) += imx8m_pm_domains.o
diff --git a/drivers/soc/imx/imx8m_pm_domains.c b/drivers/soc/imx/imx8m_pm_domains.c
new file mode 100644
index 000000000000..ce06a059eaaa
--- /dev/null
+++ b/drivers/soc/imx/imx8m_pm_domains.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/regulator/consumer.h>
+
+#include <soc/imx/imx_sip.h>
+
+#define MAX_CLK_NUM	6
+#define to_imx8m_pm_domain(_genpd) container_of(_genpd, struct imx8m_pm_domain, pd)
+
+
+struct imx8m_pm_domain {
+	struct device *dev;
+	struct generic_pm_domain pd;
+	u32 domain_index;
+	struct clk *clk[MAX_CLK_NUM];
+	unsigned int num_clks;
+	struct regulator *reg;
+};
+
+enum imx8m_pm_domain_state {
+	PD_STATE_OFF,
+	PD_STATE_ON,
+};
+
+static DEFINE_MUTEX(gpc_pd_mutex);
+
+static int imx8m_pd_power_on(struct generic_pm_domain *genpd)
+{
+	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
+	struct arm_smccc_res res;
+	int index, ret = 0;
+
+	/* power on the external supply */
+	if (!IS_ERR(domain->reg)) {
+		ret = regulator_enable(domain->reg);
+		if (ret) {
+			dev_warn(domain->dev, "failed to power up the reg%d\n", ret);
+			return ret;
+		}
+	}
+
+	/* enable the necessary clks needed by the power domain */
+	if (domain->num_clks) {
+		for (index = 0; index < domain->num_clks; index++)
+			clk_prepare_enable(domain->clk[index]);
+	}
+
+	mutex_lock(&gpc_pd_mutex);
+	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN, domain->domain_index,
+		      PD_STATE_ON, 0, 0, 0, 0, &res);
+	mutex_unlock(&gpc_pd_mutex);
+
+	return 0;
+}
+
+static int imx8m_pd_power_off(struct generic_pm_domain *genpd)
+{
+	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
+	struct arm_smccc_res res;
+	int index, ret = 0;
+
+	mutex_lock(&gpc_pd_mutex);
+	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN, domain->domain_index,
+		      PD_STATE_OFF, 0, 0, 0, 0, &res);
+	mutex_unlock(&gpc_pd_mutex);
+
+	/* power off the external supply */
+	if (!IS_ERR(domain->reg)) {
+		ret = regulator_disable(domain->reg);
+		if (ret) {
+			dev_warn(domain->dev, "failed to power off the reg%d\n", ret);
+			return ret;
+		}
+	}
+
+	/* disable clks when power domain is off */
+	if (domain->num_clks) {
+		for (index = 0; index < domain->num_clks; index++)
+			clk_disable_unprepare(domain->clk[index]);
+	}
+
+	return ret;
+};
+
+static int imx8m_pd_get_clocks(struct imx8m_pm_domain *domain)
+{
+	int i, ret;
+
+	for (i = 0; ; i++) {
+		struct clk *clk = of_clk_get(domain->dev->of_node, i);
+		if (IS_ERR(clk))
+			break;
+		if (i >= MAX_CLK_NUM) {
+			dev_err(domain->dev, "more than %d clocks\n",
+				MAX_CLK_NUM);
+			ret = -EINVAL;
+			goto clk_err;
+		}
+		domain->clk[i] = clk;
+	}
+	domain->num_clks = i;
+
+	return 0;
+
+clk_err:
+	while (i--)
+		clk_put(domain->clk[i]);
+
+	return ret;
+}
+
+static void imx8m_pd_put_clocks(struct imx8m_pm_domain *domain)
+{
+	int i;
+
+	for (i = domain->num_clks - 1; i >= 0; i--)
+		clk_put(domain->clk[i]);
+}
+
+static const struct of_device_id imx8m_pm_domain_ids[] = {
+	{.compatible = "fsl,imx8m-pm-domain"},
+	{},
+};
+
+static int imx8m_pm_domain_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct imx8m_pm_domain *domain;
+	struct of_phandle_args parent, child;
+	int ret;
+
+	domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
+	if (!domain)
+		return -ENOMEM;
+
+	child.np = np;
+	domain->dev = dev;
+
+	ret = of_property_read_string(np, "domain-name", &domain->pd.name);
+	if (ret) {
+		dev_err(dev, "failed to get the domain name\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(np, "domain-index", &domain->domain_index);
+	if (ret) {
+		dev_err(dev, "failed to get the domain index\n");
+		return -EINVAL;
+	}
+
+	domain->reg = devm_regulator_get_optional(dev, "power");
+	if (IS_ERR(domain->reg)) {
+		if (PTR_ERR(domain->reg) != -ENODEV) {
+			if (PTR_ERR(domain->reg) != -EPROBE_DEFER)
+				dev_err(dev, "failed to get domain's regulator\n");
+			return PTR_ERR(domain->reg);
+		}
+	}
+
+	ret = imx8m_pd_get_clocks(domain);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get domain's clocks\n");
+		return ret;
+	}
+
+	domain->pd.power_off = imx8m_pd_power_off;
+	domain->pd.power_on = imx8m_pd_power_on;
+
+	pm_genpd_init(&domain->pd, NULL, true);
+
+	ret = of_genpd_add_provider_simple(np, &domain->pd);
+	if (ret) {
+		dev_err(dev, "failed to add the domain provider\n");
+		pm_genpd_remove(&domain->pd);
+		imx8m_pd_put_clocks(domain);
+		return ret;
+	}
+
+	/* add it as subdomain if necessary */
+	if (!of_parse_phandle_with_args(np, "parent-domains",
+			"#power-domain-cells", 0, &parent)) {
+		ret = of_genpd_add_subdomain(&parent, &child);
+		of_node_put(parent.np);
+
+		if (ret < 0) {
+			dev_dbg(dev, "failed to add the subdomain: %s: %d",
+				domain->pd.name, ret);
+			of_genpd_del_provider(np);
+			pm_genpd_remove(&domain->pd);
+			imx8m_pd_put_clocks(domain);
+			return driver_deferred_probe_check_state(dev);
+		}
+	}
+
+	return 0;
+}
+
+static struct platform_driver imx8m_pm_domain_driver = {
+	.driver = {
+		.name	= "imx8m_pm_domain",
+		.owner	= THIS_MODULE,
+		.of_match_table = imx8m_pm_domain_ids,
+	},
+	.probe = imx8m_pm_domain_probe,
+};
+module_platform_driver(imx8m_pm_domain_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP i.MX8M power domain driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/soc/imx/imx_sip.h b/include/soc/imx/imx_sip.h
new file mode 100644
index 000000000000..6b96b33c870e
--- /dev/null
+++ b/include/soc/imx/imx_sip.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2019 NXP
+ */
+
+#ifndef __IMX_SIP_H__
+#define __IMX_SIP_H__
+
+#define IMX_SIP_GPC			0xC2000000
+#define IMX_SIP_CONFIG_GPC_PM_DOMAIN	0x03
+
+#endif /* __IMX_SIP_H__ */
-- 
2.21.0

[RESEND PATCH 3/3] arm64: dts: freescale: Add power domain nodes for i.mx8mm

[Jacky Bai]

Add the power domain nodes for i.MX8MM.

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 103 ++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 232a7412755a..850ca6a7ac66 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -181,6 +181,109 @@
 		interrupt-affinity = <&A53_0>, <&A53_1>, <&A53_2>, <&A53_3>;
 	};
 
+	power-domains {
+		compatible = "simple-bus";
+		/* HSIO SS */
+		hsiomix_pd: hsiomix-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <0>;
+			domain-name = "hsiomix";
+			clocks = <&clk IMX8MM_CLK_USB1_CTRL_ROOT>;
+		};
+
+		pcie_pd: pcie-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <1>;
+			domain-name = "pcie";
+			parent-domains = <&hsiomix_pd>;
+		};
+
+		usb_otg1_pd: usbotg1-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <2>;
+			domain-name = "usb_otg1";
+			parent-domains = <&hsiomix_pd>;
+		};
+
+		usb_otg2_pd: usbotg2-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <3>;
+			domain-name = "usb_otg2";
+			parent-domains = <&hsiomix_pd>;
+		};
+
+		/* GPU SS */
+		gpumix_pd: gpumix-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <4>;
+			domain-name = "gpumix";
+			clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
+				 <&clk IMX8MM_CLK_GPU_AHB>,
+				 <&clk IMX8MM_CLK_GPU2D_ROOT>,
+				 <&clk IMX8MM_CLK_GPU3D_ROOT>;
+		};
+
+		/* VPU SS */
+		vpumix_pd: vpumix-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <5>;
+			domain-name = "vpumix";
+			clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>;
+		};
+
+		vpu_g1_pd: vpug1-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <6>;
+			domain-name = "vpu_g1";
+			parent-domains = <&vpumix_pd>;
+			clocks = <&clk IMX8MM_CLK_VPU_G1_ROOT>;
+		};
+
+		vpu_g2_pd: vpug2-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <7>;
+			domain-name = "vpu_g2";
+			parent-domains = <&vpumix_pd>;
+			clocks = <&clk IMX8MM_CLK_VPU_G2_ROOT>;
+		};
+
+		vpu_h1_pd: vpuh1-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <8>;
+			domain-name = "vpu_h1";
+			parent-domains = <&vpumix_pd>;
+			clocks = <&clk IMX8MM_CLK_VPU_H1_ROOT>;
+		};
+
+		/* DISP SS */
+		dispmix_pd: dispmix-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <9>;
+			domain-name = "dispmix";
+			clocks = <&clk IMX8MM_CLK_DISP_ROOT>,
+				 <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
+				 <&clk IMX8MM_CLK_DISP_APB_ROOT>;
+		};
+
+		mipi_pd: mipi-pd {
+			compatible = "fsl,imx8m-pm-domain";
+			#power-domain-cells = <0>;
+			domain-index = <10>;
+			domain-name = "mipi";
+			parent-domains = <&dispmix_pd>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>, /* Physical Secure */
-- 
2.21.0

Re: [RESEND PATCH 2/3] soc: imx: Add power domain driver support for i.mx8m family

[Sudeep Holla]

On Tue, Jul 02, 2019 at 10:03:46AM +0000, Jacky Bai wrote:
> The i.MX8M family is a set of NXP product focus on delivering
> the latest and greatest video and audio experience combining
> state-of-the-art media-specific features with high-performance
> processing while optimized for lowest power consumption.
> 
> i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all
> belong to this family. A GPC module is used to manage all the
> PU power domain on/off. But the situation is that the number of
> power domains & the power up sequence has significate difference
> on those SoCs. Even on the same SoC. The power up sequence still
> has big difference. It makes us hard to reuse the GPCv2 driver to
> cover the whole i.MX8M family. Each time a new SoC is supported in
> the mainline kernel, we need to modify the GPCv2 driver to support
> it. We need to add or modify hundred lines of code in worst case.
> It is a bad practice for the driver maintainability.
> 
> This driver add a more generic power domain driver that the actual
> power on/off is done by TF-A code. the abstraction give us the
> possibility that using one driver to cover the whole i.MX8M family
> in kernel side.
>

TF-A has SCMI support, why can't that be used as abstraction instead
of inventing new. Peng Fan has been working to get SMC mailbox.

Unless you give me strong reasons for not able to pursue that path,
NACK for this patch. I have told this in the recent past.

> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> ---
>  drivers/soc/imx/Kconfig            |   6 +
>  drivers/soc/imx/Makefile           |   1 +
>  drivers/soc/imx/imx8m_pm_domains.c | 224 +++++++++++++++++++++++++++++
>  include/soc/imx/imx_sip.h          |  12 ++
>  4 files changed, 243 insertions(+)
>  create mode 100644 drivers/soc/imx/imx8m_pm_domains.c
>  create mode 100644 include/soc/imx/imx_sip.h

[...]

> +
> +	mutex_lock(&gpc_pd_mutex);
> +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN, domain->domain_index,
> +		      PD_STATE_ON, 0, 0, 0, 0, &res);
> +	mutex_unlock(&gpc_pd_mutex);
> +
> +	return 0;
> +}
> +
> +static int imx8m_pd_power_off(struct generic_pm_domain *genpd)
> +{
> +	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
> +	struct arm_smccc_res res;
> +	int index, ret = 0;
> +
> +	mutex_lock(&gpc_pd_mutex);
> +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN, domain->domain_index,
> +		      PD_STATE_OFF, 0, 0, 0, 0, &res);

How big is this IMX SMC SIP ? I keep seeing that it's ever growing.
I don't want to see this for any future products as they seem to be
designed "ON THE GO" as and when needed rather than completely thought
through.

--
Regards,
Sudeep

RE: [RESEND PATCH 2/3] soc: imx: Add power domain driver support for i.mx8m family

[Jacky Bai]

> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Tuesday, July 2, 2019 7:07 PM
> To: Jacky Bai <ping.bai@nxp.com>
> Cc: robh+dt@kernel.org; l.stach@pengutronix.de; shawnguo@kernel.org;
> festevam@gmail.com; Leonard Crestez <leonard.crestez@nxp.com>; Aisheng
> Dong <aisheng.dong@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Sudeep Holla <sudeep.holla@arm.com>
> Subject: Re: [RESEND PATCH 2/3] soc: imx: Add power domain driver support
> for i.mx8m family
> 
> On Tue, Jul 02, 2019 at 10:03:46AM +0000, Jacky Bai wrote:
> > The i.MX8M family is a set of NXP product focus on delivering the
> > latest and greatest video and audio experience combining
> > state-of-the-art media-specific features with high-performance
> > processing while optimized for lowest power consumption.
> >
> > i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all belong to
> > this family. A GPC module is used to manage all the PU power domain
> > on/off. But the situation is that the number of power domains & the
> > power up sequence has significate difference on those SoCs. Even on
> > the same SoC. The power up sequence still has big difference. It makes
> > us hard to reuse the GPCv2 driver to cover the whole i.MX8M family.
> > Each time a new SoC is supported in the mainline kernel, we need to
> > modify the GPCv2 driver to support it. We need to add or modify
> > hundred lines of code in worst case.
> > It is a bad practice for the driver maintainability.
> >
> > This driver add a more generic power domain driver that the actual
> > power on/off is done by TF-A code. the abstraction give us the
> > possibility that using one driver to cover the whole i.MX8M family in
> > kernel side.
> >
> 
> TF-A has SCMI support, why can't that be used as abstraction instead of
> inventing new. Peng Fan has been working to get SMC mailbox.
> 
> Unless you give me strong reasons for not able to pursue that path, NACK for
> this patch. I have told this in the recent past.
> 

For some of the power domains, we need to handle the external regulator.
In current SCMI power domain driver, there is not such support. If we use
the SCMI power domain driver, how to handle regulator on/off, in TF-A?
currently, all the regulator is managed by kernel side. most of the regulator
is controlled by I2C bus. Accessing the I2C from kernel & TF-A both is not feasible.
if regulator need to be handled in TF-A, I am not sure if it is necessary to extend the
SCMI spec to include a regulator protocol.

Another concern is that moving all currently implementation to SCMI compatible is
a huge work, waiting for SCMI implementation ready will block other peripherals
upstream for a very long time.

Anyway, if current implementation can NOT be accepted, we can try to switch SCMI
implementation.

> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > ---
> >  drivers/soc/imx/Kconfig            |   6 +
> >  drivers/soc/imx/Makefile           |   1 +
> >  drivers/soc/imx/imx8m_pm_domains.c | 224
> +++++++++++++++++++++++++++++
> >  include/soc/imx/imx_sip.h          |  12 ++
> >  4 files changed, 243 insertions(+)
> >  create mode 100644 drivers/soc/imx/imx8m_pm_domains.c
> >  create mode 100644 include/soc/imx/imx_sip.h
> 
> [...]
> 
> > +
> > +	mutex_lock(&gpc_pd_mutex);
> > +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN,
> domain->domain_index,
> > +		      PD_STATE_ON, 0, 0, 0, 0, &res);
> > +	mutex_unlock(&gpc_pd_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx8m_pd_power_off(struct generic_pm_domain *genpd) {
> > +	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
> > +	struct arm_smccc_res res;
> > +	int index, ret = 0;
> > +
> > +	mutex_lock(&gpc_pd_mutex);
> > +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN,
> domain->domain_index,
> > +		      PD_STATE_OFF, 0, 0, 0, 0, &res);
> 
> How big is this IMX SMC SIP ? I keep seeing that it's ever growing.
> I don't want to see this for any future products as they seem to be designed
> "ON THE GO" as and when needed rather than completely thought through.
> 
> --
> Regards,
> Sudeep

Re: [RESEND PATCH 1/3] dt-bindings: power: Add power domain binding for i.mx8m family

[Rob Herring]

On Tue, Jul 02, 2019 at 10:03:41AM +0000, Jacky Bai wrote:
> Add the binding doc of power domain for i.MX8M SOC family.
> 
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> ---
>  .../bindings/power/fsl,imx8m-genpd.txt        | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt b/Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt
> new file mode 100644
> index 000000000000..a92a7103de38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/fsl,imx8m-genpd.txt
> @@ -0,0 +1,46 @@
> +Device Tree Bindings for Freescale i.MX8M Generic Power Domain
> +==============================================================
> +The binding for the i.MX8M Generic power Domain[1].
> +
> +[1] Documentation/devicetree/bindings/power/power_domain.txt
> +
> +Required properties:
> +
> + - compatible: should be of:
> +	- "fsl,imx8m-power-domain"
> + - #power-domain-cells: Number of cells in a PM domain Specifier, must be 0
> + - domain-index: should be the domain index number need to pass to TF-A
> + - domain-name: the name of this pm domain
> +
> +Optional properties:
> + - clocks: a number of phandles to clocks that need to be enabled during
> +   domain power-up sequence to ensure reset propagation into devices
> +   located inside this power domain

Isn't that firmware's problem?

> + - power-supply: Power supply used to power the domain
> + - parent-domains: the phandle to the parent power domain
> +
> +example:
> +	vpu_g1_pd: vpug1-pd {

What's this a child of?

Use generic node names.

> +		compatible = "fsl,imx8mm-pm-domain";
> +		#power-domain-cells = <0>;

I'm usually not a fan when I see a single domain per provider. Why can't 
you have firmware be a single provider with multiple domains?

> +		domain-index = <6>;

Don't create your own index properties.

> +		domain-name = "vpu_g1";

Not a standard name. Why do you need this? (Hint: we'd already have a 
standard name if you did.)

> +		parent-domains = <&vpumix_pd>;

We already have a standard way to do this.

> +		clocks = <&clk IMX8MM_CLK_VPU_G1_ROOT>;
> +	};
> +
> +
> +Specifying Power domain for IP modules
> +======================================
> +
> +IP cores belonging to a power domain should contain a 'power-domains'
> +property that is a phandle for PGC node representing the domain.
> +
> +Example of a device that is part of the vpu_g1 power domain:
> +	vpu_g1: vpu_g1@38300000 {

video-codec@...

> +		/* ... */
> +		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "irq_hantro";
> +		/* ... */
> +		power-domains = <&vpu_g1_pd>;
> +	};
> -- 
> 2.21.0
>