[PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

V8:
Revert one change in v7, force goto disable_clk for handshake when power on in patch 3
One minor update to use if{} else {}, not if{}; if{}; in patch 3
Typo Hankshake->Handshake

V7:
 patch 2: update patch title per Shawn 
 Patch 3: Addressed several comments from Shawn

V6:
 Thanks for Adam's report on V5.
 Resolve the error message dump, it is the child device reuse
 the parent device node and matches the parent driver.
 Filled the remove function for child device.
 A diff dts file for upstream:
 https://gist.github.com/MrVan/d73888d8273c43ea4a3b28fa668ca1d0

V5:
 Rework the blk-ctl driver to let sub-PGC use blk-ctl as parent power
 domain to fix the potential handshake issue.
 I still keep R-b/A-b tag for Patch 1,2,4, since very minor changes
 I only drop R-b tag for Patch 3, since it has big change.
 An example, the pgc_mipi not take pgc_dispmix as parent:

	pgc_dispmix: power-domain@10 {
		#power-domain-cells = <0>;
		reg = <IMX8MM_POWER_DOMAIN_DISPMIX>;
		clocks = <&clk IMX8MM_CLK_DISP_ROOT>,
			 <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
			 <&clk IMX8MM_CLK_DISP_APB_ROOT>;
	};

	pgc_mipi: power-domain@11 {
		#power-domain-cells = <0>;
		reg = <IMX8MM_POWER_DOMAIN_MIPI>;
		power-domains = <&dispmix_blk_ctl IMX8MM_BLK_CTL_PD_DISPMIX_BUS>;
	};

	dispmix_blk_ctl: clock-controller@32e28000 {
		compatible = "fsl,imx8mm-dispmix-blk-ctl", "syscon";
		reg = <0x32e28000 0x100>;
		#power-domain-cells = <1>;
		power-domains = <&pgc_dispmix>, <&pgc_mipi>;
		power-domain-names = "dispmix", "mipi";
		clocks = <&clk IMX8MM_CLK_DISP_ROOT>, <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
			 <&clk IMX8MM_CLK_DISP_APB_ROOT>;
	};

V4:
 Add R-b tag
 Typo fix
 Update the power domain macro names Per Abel and Frieder

V3:
 Add explaination for not listing items in patch 2 commit log Per Rob.
 Addressed comments from Lucas and Frieder on patch [3,4].
 A few comments from Jacky was ignored, because following gpcv2
 coding style.

V2:
 Fix yaml check failure.

Previously there is an effort from Abel that take BLK-CTL as clock
provider, but it turns out that there is A/B lock issue and we are
not able resolve that.

Per discuss with Lucas and Jacky, we made an agreement that take BLK-CTL
as a power domain provider and use GPC's domain as parent, the consumer
node take BLK-CTL as power domain input.

This patchset has been tested on i.MX8MM EVK board, but one hack
is not included in the patchset is that the DISPMIX BLK-CTL
MIPI_M/S_RESET not implemented. Per Lucas, we will finally have a MIPI
DPHY driver, so fine to leave it.

Thanks for Lucas's suggestion, Frieder Schrempf for collecting
all the patches, Abel's previous BLK-CTL work, Jacky Bai on help
debug issues.


Peng Fan (4):
  dt-bindings: power: Add defines for i.MX8MM BLK-CTL power domains
  dt-bindings: soc: imx: Add bindings for i.MX BLK_CTL
  soc: imx: Add generic blk-ctl driver
  soc: imx: Add blk-ctl driver for i.MX8MM

 .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     |  66 ++++
 drivers/soc/imx/Makefile                      |   2 +-
 drivers/soc/imx/blk-ctl-imx8mm.c              | 139 ++++++++
 drivers/soc/imx/blk-ctl.c                     | 324 ++++++++++++++++++
 drivers/soc/imx/blk-ctl.h                     |  85 +++++
 include/dt-bindings/power/imx8mm-power.h      |  13 +
 6 files changed, 628 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
 create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c
 create mode 100644 drivers/soc/imx/blk-ctl.c
 create mode 100644 drivers/soc/imx/blk-ctl.h

-- 
2.30.0

[PATCH V8 1/4] dt-bindings: power: Add defines for i.MX8MM BLK-CTL power domains

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Adding the defines for i.MX8MM BLK-CTL power domains.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 include/dt-bindings/power/imx8mm-power.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/dt-bindings/power/imx8mm-power.h b/include/dt-bindings/power/imx8mm-power.h
index fc9c2e16aadc..982ca2939cdc 100644
--- a/include/dt-bindings/power/imx8mm-power.h
+++ b/include/dt-bindings/power/imx8mm-power.h
@@ -19,4 +19,17 @@
 #define IMX8MM_POWER_DOMAIN_DISPMIX	10
 #define IMX8MM_POWER_DOMAIN_MIPI	11
 
+#define IMX8MM_BLK_CTL_PD_VPU_G2		0
+#define IMX8MM_BLK_CTL_PD_VPU_G1		1
+#define IMX8MM_BLK_CTL_PD_VPU_H1		2
+#define IMX8MM_BLK_CTL_PD_VPU_BUS		3
+#define IMX8MM_BLK_CTL_PD_VPU_MAX		4
+
+#define IMX8MM_BLK_CTL_PD_DISPMIX_CSI_BRIDGE	0
+#define IMX8MM_BLK_CTL_PD_DISPMIX_LCDIF		1
+#define IMX8MM_BLK_CTL_PD_DISPMIX_MIPI_DSI	2
+#define IMX8MM_BLK_CTL_PD_DISPMIX_MIPI_CSI	3
+#define IMX8MM_BLK_CTL_PD_DISPMIX_BUS		4
+#define IMX8MM_BLK_CTL_PD_DISPMIX_MAX		5
+
 #endif
-- 
2.30.0

[PATCH V8 2/4] dt-bindings: soc: imx: Add bindings for i.MX BLK_CTL

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Document the i.MX BLK_CTL with its devicetree properties.

Each BLK CTL have different power domain inputs and they have different
names, so we are not able to list all the power domain names for each
BLK CTL here.

For example:
i.MX8MM dispmix BLK CTL, it has
	power-domains = <&pgc_dispmix>, <&pgc_mipi>;
	power-domain-names = "dispmix", "mipi";

vpumix BLK CTL, it has
	power-domains = <&vpumix_pd>, <&vpu_g1_pd>, <&vpu_g2_pd>,
			<&vpu_h1_pd>;
	power-domain-names = "vpumix", "g1", "g2", "h1";

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml

diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
new file mode 100644
index 000000000000..a66f11acc6b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/imx/fsl,imx-blk-ctl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX BLK_CTL
+
+maintainers:
+  - Peng Fan <peng.fan@nxp.com>
+
+description:
+  i.MX BLK_CTL is a conglomerate of different GPRs that are
+  dedicated to a specific subsystem. It usually contains
+  clocks and resets amongst other things. Here we take the clocks
+  and resets as virtual PDs, the reason we could not take it as
+  clock provider is there is A/B lock issue between power domain
+  and clock.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - fsl,imx8mm-dispmix-blk-ctl
+          - fsl,imx8mm-vpumix-blk-ctl
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+  "#power-domain-cells":
+    const: 1
+
+  power-domains:
+    minItems: 1
+    maxItems: 32
+
+  power-domain-names:
+    minItems: 1
+    maxItems: 32
+
+  clocks:
+    minItems: 1
+    maxItems: 32
+
+required:
+  - compatible
+  - reg
+  - power-domains
+  - power-domain-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+
+    dispmix_blk_ctl: blk-ctl@32e28000 {
+      compatible = "fsl,imx8mm-dispmix-blk-ctl", "syscon";
+      reg = <0x32e28000 0x100>;
+      #power-domain-cells = <1>;
+      power-domains = <&pgc_dispmix>, <&pgc_mipi>;
+      power-domain-names = "dispmix", "mipi";
+      clocks = <&clk IMX8MM_CLK_DISP_ROOT>, <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
+               <&clk IMX8MM_CLK_DISP_APB_ROOT>;
+    };
-- 
2.30.0

[PATCH V8 3/4] soc: imx: Add generic blk-ctl driver

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
some GPRs.

The GPRs has some clock bits and reset bits, but here we take it
as virtual PDs, because of the clock and power domain A/B lock issue
when taking it as a clock controller.

For some bits, it might be good to also make it as a reset controller,
but to i.MX8MM, we not add that support for now.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/soc/imx/Makefile  |   2 +-
 drivers/soc/imx/blk-ctl.c | 324 ++++++++++++++++++++++++++++++++++++++
 drivers/soc/imx/blk-ctl.h |  85 ++++++++++
 3 files changed, 410 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/imx/blk-ctl.c
 create mode 100644 drivers/soc/imx/blk-ctl.h

diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index 078dc918f4f3..d3d2b49a386c 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile
@@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
 endif
 obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
-obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
+obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c
new file mode 100644
index 000000000000..cec1884202e0
--- /dev/null
+++ b/drivers/soc/imx/blk-ctl.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2021 NXP.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "blk-ctl.h"
+
+static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct generic_pm_domain *genpd)
+{
+	return container_of(genpd, struct imx_blk_ctl_domain, genpd);
+}
+
+static int imx_blk_ctl_enable_hsk(struct device *dev)
+{
+	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
+	const struct imx_blk_ctl_hw *hw = blk_ctl->dev_data->hw_hsk;
+	struct regmap *regmap = blk_ctl->regmap;
+	int ret;
+
+	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
+		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
+
+	/* Wait for handshake */
+	udelay(5);
+
+	return ret;
+}
+
+static int imx_blk_ctl_power_on(struct generic_pm_domain *domain)
+{
+	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
+	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
+	struct regmap *regmap = blk_ctl->regmap;
+	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->pds[pd->id];
+	int ret;
+
+	mutex_lock(&blk_ctl->lock);
+
+	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
+	if (ret) {
+		mutex_unlock(&blk_ctl->lock);
+		return ret;
+	}
+
+	if (hw->flags & IMX_BLK_CTL_PD_HANDSHAKE) {
+		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
+		if (ret)
+			dev_err(blk_ctl->dev, "Handshake failed when power on\n");
+
+		/* Expected, handshake already handle reset*/
+		goto disable_clk;
+	}
+
+	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
+		ret = regmap_clear_bits(regmap, hw->rst_offset, hw->rst_mask);
+		if (ret)
+			goto disable_clk;
+
+		/* Wait for reset propagate */
+		udelay(5);
+
+		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
+		if (ret)
+			goto disable_clk;
+	}
+
+	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
+
+disable_clk:
+	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
+
+	mutex_unlock(&blk_ctl->lock);
+
+	return ret;
+}
+
+static int imx_blk_ctl_power_off(struct generic_pm_domain *domain)
+{
+	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
+	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
+	struct regmap *regmap = blk_ctl->regmap;
+	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->pds[pd->id];
+	int ret = 0;
+
+	mutex_lock(&blk_ctl->lock);
+
+	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
+	if (ret) {
+		mutex_unlock(&blk_ctl->lock);
+		return ret;
+	}
+
+	if (!(hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)) {
+		ret = regmap_clear_bits(regmap, hw->offset, hw->mask);
+		if (ret)
+			goto disable_clk;
+
+		if (hw->flags & IMX_BLK_CTL_PD_RESET) {
+			ret = regmap_clear_bits(regmap, hw->rst_offset, hw->rst_mask);
+			if (ret)
+				goto disable_clk;
+		}
+	} else {
+		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
+		if (ret)
+			dev_err(blk_ctl->dev, "Handshake failed when power off\n");
+	}
+
+disable_clk:
+	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
+
+	mutex_unlock(&blk_ctl->lock);
+
+	return ret;
+}
+
+static int imx_blk_ctl_probe(struct platform_device *pdev)
+{
+	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
+	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
+	struct generic_pm_domain *parent_genpd;
+	struct device *dev = &pdev->dev;
+	struct device *active_pd;
+	int ret;
+
+	pdev->dev.of_node = blk_ctl->dev->of_node;
+
+	if (domain->hw->active_pd_name) {
+		active_pd = dev_pm_domain_attach_by_name(dev, domain->hw->active_pd_name);
+		if (IS_ERR_OR_NULL(active_pd)) {
+			ret = PTR_ERR(active_pd) ? : -ENODATA;
+			pdev->dev.of_node = NULL;
+			return ret;
+		}
+
+		domain->active_pd = active_pd;
+	} else {
+		if (!blk_ctl->bus_domain) {
+			pdev->dev.of_node = NULL;
+			return -EPROBE_DEFER;
+		}
+	}
+
+	if (domain->hw->active_pd_name)
+		parent_genpd = pd_to_genpd(active_pd->pm_domain);
+	else
+		parent_genpd = blk_ctl->bus_domain;
+
+	if (pm_genpd_add_subdomain(parent_genpd, &domain->genpd)) {
+		dev_warn(dev, "failed to add subdomain: %s\n", domain->genpd.name);
+	} else {
+		mutex_lock(&blk_ctl->lock);
+		domain->hooked = true;
+		mutex_unlock(&blk_ctl->lock);
+	}
+
+	return 0;
+}
+
+static int imx_blk_ctl_remove(struct platform_device *pdev)
+{
+	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
+	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
+	struct generic_pm_domain *parent_genpd;
+	struct device *active_pd;
+
+	if (domain->hw->active_pd_name)
+		parent_genpd = pd_to_genpd(active_pd->pm_domain);
+	else
+		parent_genpd = blk_ctl->bus_domain;
+
+	pm_genpd_remove_subdomain(parent_genpd, &domain->genpd);
+
+	mutex_lock(&blk_ctl->lock);
+	domain->hooked = false;
+	mutex_unlock(&blk_ctl->lock);
+
+	if (domain->hw->active_pd_name)
+		dev_pm_domain_detach(domain->active_pd, false);
+
+	return 0;
+}
+
+static const struct platform_device_id imx_blk_ctl_id[] = {
+	{ "imx-vpumix-blk-ctl", },
+	{ "imx-dispmix-blk-ctl", },
+	{ },
+};
+
+static struct platform_driver imx_blk_ctl_driver = {
+	.driver = {
+		.name = "imx-blk-ctl",
+	},
+	.probe    = imx_blk_ctl_probe,
+	.remove   = imx_blk_ctl_remove,
+	.id_table = imx_blk_ctl_id,
+};
+builtin_platform_driver(imx_blk_ctl_driver)
+
+static struct generic_pm_domain *imx_blk_ctl_genpd_xlate(struct of_phandle_args *genpdspec,
+							 void *data)
+{
+	struct genpd_onecell_data *genpd_data = data;
+	unsigned int idx = genpdspec->args[0];
+	struct imx_blk_ctl_domain *domain;
+	struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
+
+	if (genpdspec->args_count != 1)
+		return ERR_PTR(-EINVAL);
+
+	if (idx >= genpd_data->num_domains)
+		return ERR_PTR(-EINVAL);
+
+	if (!genpd_data->domains[idx])
+		return ERR_PTR(-ENOENT);
+
+	domain = to_imx_blk_ctl_pd(genpd_data->domains[idx]);
+
+	mutex_lock(&domain->blk_ctl->lock);
+	if (domain->hooked)
+		genpd = genpd_data->domains[idx];
+	mutex_unlock(&domain->blk_ctl->lock);
+
+	return genpd;
+}
+
+int imx_blk_ctl_register(struct device *dev)
+{
+	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
+	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
+	int num = dev_data->pds_num;
+	struct imx_blk_ctl_domain *domain;
+	struct generic_pm_domain *genpd;
+	struct platform_device *pd_pdev;
+	int domain_index;
+	int i, ret;
+
+	blk_ctl->onecell_data.num_domains = num;
+	blk_ctl->onecell_data.xlate = imx_blk_ctl_genpd_xlate;
+	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num, sizeof(struct generic_pm_domain *),
+						     GFP_KERNEL);
+	if (!blk_ctl->onecell_data.domains)
+		return -ENOMEM;
+
+	for (i = 0; i < num; i++) {
+		domain_index = dev_data->pds[i].id;
+		if (domain_index >= num) {
+			dev_warn(dev, "Domain index %d is out of bounds\n", domain_index);
+			continue;
+		}
+
+		domain = devm_kzalloc(dev, sizeof(struct imx_blk_ctl_domain), GFP_KERNEL);
+		if (!domain)
+			goto error;
+
+		pd_pdev = platform_device_alloc(dev_data->name, domain_index);
+		if (!pd_pdev) {
+			dev_err(dev, "Failed to allocate platform device\n");
+			goto error;
+		}
+
+		pd_pdev->dev.platform_data = domain;
+
+		domain->blk_ctl = blk_ctl;
+		domain->hw = &dev_data->pds[i];
+		domain->id = domain_index;
+		domain->genpd.name = dev_data->pds[i].name;
+		domain->genpd.power_off = imx_blk_ctl_power_off;
+		domain->genpd.power_on = imx_blk_ctl_power_on;
+		domain->dev = &pd_pdev->dev;
+		domain->hooked = false;
+
+		ret = pm_genpd_init(&domain->genpd, NULL, true);
+		pd_pdev->dev.parent = dev;
+
+		if (domain->hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)
+			blk_ctl->bus_domain = &domain->genpd;
+
+		ret = platform_device_add(pd_pdev);
+		if (ret) {
+			platform_device_put(pd_pdev);
+			goto error;
+		}
+		blk_ctl->onecell_data.domains[i] = &domain->genpd;
+	}
+
+	return of_genpd_add_provider_onecell(dev->of_node, &blk_ctl->onecell_data);
+
+error:
+	for (; i >= 0; i--) {
+		genpd = blk_ctl->onecell_data.domains[i];
+		if (!genpd)
+			continue;
+		domain = to_imx_blk_ctl_pd(genpd);
+		if (domain->dev)
+			platform_device_put(to_platform_device(domain->dev));
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(imx_blk_ctl_register);
+
+const struct dev_pm_ops imx_blk_ctl_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+};
+EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);
diff --git a/drivers/soc/imx/blk-ctl.h b/drivers/soc/imx/blk-ctl.h
new file mode 100644
index 000000000000..6780d00ec8c5
--- /dev/null
+++ b/drivers/soc/imx/blk-ctl.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SOC_IMX_BLK_CTL_H
+#define __SOC_IMX_BLK_CTL_H
+
+enum imx_blk_ctl_pd_type {
+	BLK_CTL_PD,
+};
+
+struct imx_blk_ctl_hw {
+	int type;
+	char *name;
+	char *active_pd_name;
+	u32 offset;
+	u32 mask;
+	u32 flags;
+	u32 id;
+	u32 rst_offset;
+	u32 rst_mask;
+	u32 errata;
+};
+
+struct imx_blk_ctl_domain {
+	struct generic_pm_domain genpd;
+	struct device *active_pd;
+	struct imx_blk_ctl *blk_ctl;
+	struct imx_blk_ctl_hw *hw;
+	struct device *dev;
+	bool hooked;
+	u32 id;
+};
+
+struct imx_blk_ctl_dev_data {
+	struct regmap_config config;
+	struct imx_blk_ctl_hw *pds;
+	struct imx_blk_ctl_hw *hw_hsk;
+	u32 pds_num;
+	u32 max_num;
+	char *name;
+};
+
+struct imx_blk_ctl {
+	struct device *dev;
+	struct regmap *regmap;
+	struct genpd_onecell_data onecell_data;
+	const struct imx_blk_ctl_dev_data *dev_data;
+	struct clk_bulk_data *clks;
+	u32 num_clks;
+	struct generic_pm_domain *bus_domain;
+
+	struct mutex lock;
+};
+
+#define IMX_BLK_CTL(_type, _name, _active_pd, _id, _offset, _mask, _rst_offset, _rst_mask,	\
+		    _flags, _errata)								\
+	{											\
+		.type = _type,									\
+		.name = _name,									\
+		.active_pd_name = _active_pd,							\
+		.id = _id,									\
+		.offset = _offset,								\
+		.mask = _mask,									\
+		.flags = _flags,								\
+		.rst_offset = _rst_offset,							\
+		.rst_mask = _rst_mask,								\
+		.errata = _errata,								\
+	}
+
+#define IMX_BLK_CTL_PD(_name, _active_pd, _id, _offset, _mask, _rst_offset, _rst_mask, _flags)	\
+	IMX_BLK_CTL(BLK_CTL_PD, _name, _active_pd, _id, _offset, _mask, _rst_offset,		\
+		    _rst_mask, _flags, 0)
+
+#define IMX_BLK_CTL_PD_ERRATA(_name, _active_pd, _id, _offset, _mask, _rst_offset, _rst_mask,	\
+			      _flags, _errata)							\
+	IMX_BLK_CTL(BLK_CTL_PD, _name, _active_pd, _id, _offset, _mask, _rst_offset,		\
+		    _rst_mask, _flags, _errata)
+
+int imx_blk_ctl_register(struct device *dev);
+
+#define IMX_BLK_CTL_PD_HANDSHAKE	BIT(0)
+#define IMX_BLK_CTL_PD_RESET		BIT(1)
+#define IMX_BLK_CTL_PD_BUS		BIT(2)
+
+const extern struct dev_pm_ops imx_blk_ctl_pm_ops;
+
+#endif
-- 
2.30.0

[PATCH V8 4/4] soc: imx: Add blk-ctl driver for i.MX8MM

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The i.MX8MM SoC has dispmix BLK-CTL and vpumix BLK-CTL, so we add
that support in this driver.

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/soc/imx/Makefile         |   2 +-
 drivers/soc/imx/blk-ctl-imx8mm.c | 139 +++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c

diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index d3d2b49a386c..c260b962f495 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile
@@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
 endif
 obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
-obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
+obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o blk-ctl-imx8mm.o
diff --git a/drivers/soc/imx/blk-ctl-imx8mm.c b/drivers/soc/imx/blk-ctl-imx8mm.c
new file mode 100644
index 000000000000..59443588f892
--- /dev/null
+++ b/drivers/soc/imx/blk-ctl-imx8mm.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2021 NXP
+ */
+
+#include <dt-bindings/clock/imx8mm-clock.h>
+#include <dt-bindings/power/imx8mm-power.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/pm_domain.h>
+#include <linux/regmap.h>
+
+#include "blk-ctl.h"
+
+#define MEDIA_BLK_BUS_RSTN_BLK_SYNC_SFT_EN			BIT(6)
+#define MEDIA_BLK_MIPI_DSI_I_PRESETN_SFT_EN			BIT(5)
+#define MEDIA_BLK_MIPI_CSI_I_PRESETN_SFT_EN			BIT(4)
+#define MEDIA_BLK_CAMERA_PIXEL_RESET_N_SFT_EN			BIT(3)
+#define MEDIA_BLK_CSI_BRIDGE_SFT_EN				GENMASK(2, 0)
+
+#define MEDIA_BLK_BUS_PD_MASK					BIT(12)
+#define MEDIA_BLK_MIPI_CSI_PD_MASK				GENMASK(11, 10)
+#define MEDIA_BLK_MIPI_DSI_PD_MASK				GENMASK(9, 8)
+#define MEDIA_BLK_LCDIF_PD_MASK					GENMASK(7, 6)
+#define MEDIA_BLK_CSI_BRIDGE_PD_MASK				GENMASK(5, 0)
+
+static struct imx_blk_ctl_hw imx8mm_dispmix_blk_ctl_pds[] = {
+	IMX_BLK_CTL_PD("CSI_BRIDGE", NULL, IMX8MM_BLK_CTL_PD_DISPMIX_CSI_BRIDGE, 0x4,
+		       MEDIA_BLK_CSI_BRIDGE_PD_MASK, 0, MEDIA_BLK_CSI_BRIDGE_SFT_EN,
+		       IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("LCDIF", NULL, IMX8MM_BLK_CTL_PD_DISPMIX_LCDIF, 0x4,
+		       MEDIA_BLK_LCDIF_PD_MASK, -1, -1, 0),
+	IMX_BLK_CTL_PD("MIPI_DSI", "mipi", IMX8MM_BLK_CTL_PD_DISPMIX_MIPI_DSI, 0x4,
+		       MEDIA_BLK_MIPI_DSI_PD_MASK, 0, MEDIA_BLK_MIPI_DSI_I_PRESETN_SFT_EN,
+		       IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("MIPI_CSI", "mipi", IMX8MM_BLK_CTL_PD_DISPMIX_MIPI_CSI, 0x4,
+		       MEDIA_BLK_MIPI_CSI_PD_MASK, 0,
+		       MEDIA_BLK_MIPI_CSI_I_PRESETN_SFT_EN | MEDIA_BLK_CAMERA_PIXEL_RESET_N_SFT_EN,
+		       IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("DISPMIX_BUS", "dispmix", IMX8MM_BLK_CTL_PD_DISPMIX_BUS, 0x4,
+		       MEDIA_BLK_BUS_PD_MASK, 0, MEDIA_BLK_BUS_RSTN_BLK_SYNC_SFT_EN,
+		       IMX_BLK_CTL_PD_HANDSHAKE | IMX_BLK_CTL_PD_RESET)
+};
+
+static struct imx_blk_ctl_hw imx8mm_vpumix_blk_ctl_pds[] = {
+	IMX_BLK_CTL_PD("VPU_BLK_CTL_G2", "vpu-g2", IMX8MM_BLK_CTL_PD_VPU_G2, 0x4,
+		       BIT(0), 0, BIT(0), IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("VPU_BLK_CTL_G1", "vpu-g1", IMX8MM_BLK_CTL_PD_VPU_G1, 0x4,
+		       BIT(1), 0, BIT(1), IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("VPU_BLK_CTL_H1", "vpu-h1", IMX8MM_BLK_CTL_PD_VPU_H1, 0x4,
+		       BIT(2), 0, BIT(2), IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("VPU_BLK_CTL_BUS", "vpumix", IMX8MM_BLK_CTL_PD_VPU_BUS, 0x4,
+		       BIT(2), 0, BIT(2), IMX_BLK_CTL_PD_HANDSHAKE | IMX_BLK_CTL_PD_RESET)
+};
+
+static const struct regmap_config imx8mm_blk_ctl_regmap_config = {
+	.reg_bits		= 32,
+	.reg_stride		= 4,
+	.val_bits		= 32,
+	.max_register		= 0x30,
+	.fast_io		= true,
+};
+
+static const struct imx_blk_ctl_dev_data imx8mm_vpumix_blk_ctl_dev_data = {
+	.pds = imx8mm_vpumix_blk_ctl_pds,
+	.pds_num = ARRAY_SIZE(imx8mm_vpumix_blk_ctl_pds),
+	.max_num = IMX8MM_BLK_CTL_PD_VPU_MAX,
+	.hw_hsk = &imx8mm_vpumix_blk_ctl_pds[3],
+	.config = imx8mm_blk_ctl_regmap_config,
+	.name = "imx-vpumix-blk-ctl",
+};
+
+static const struct imx_blk_ctl_dev_data imx8mm_dispmix_blk_ctl_dev_data = {
+	.pds = imx8mm_dispmix_blk_ctl_pds,
+	.pds_num = ARRAY_SIZE(imx8mm_dispmix_blk_ctl_pds),
+	.max_num = IMX8MM_BLK_CTL_PD_DISPMIX_MAX,
+	.hw_hsk = &imx8mm_dispmix_blk_ctl_pds[4],
+	.config = imx8mm_blk_ctl_regmap_config,
+	.name = "imx-dispmix-blk-ctl",
+};
+
+static int imx8mm_blk_ctl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev);
+	struct regmap *regmap;
+	struct imx_blk_ctl *ctl;
+	void __iomem *base;
+
+	ctl = devm_kzalloc(dev, sizeof(*ctl), GFP_KERNEL);
+	if (!ctl)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &dev_data->config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ctl->regmap = regmap;
+	ctl->dev = dev;
+	mutex_init(&ctl->lock);
+
+	ctl->num_clks = devm_clk_bulk_get_all(dev, &ctl->clks);
+	if (ctl->num_clks < 0)
+		return ctl->num_clks;
+
+	dev_set_drvdata(dev, ctl);
+	ctl->dev_data = dev_data;
+
+	return imx_blk_ctl_register(dev);
+}
+
+static const struct of_device_id imx_blk_ctl_of_match[] = {
+	{ .compatible = "fsl,imx8mm-vpumix-blk-ctl", .data = &imx8mm_vpumix_blk_ctl_dev_data },
+	{ .compatible = "fsl,imx8mm-dispmix-blk-ctl", .data = &imx8mm_dispmix_blk_ctl_dev_data },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_blk_ctl_of_match);
+
+static struct platform_driver imx_blk_ctl_driver = {
+	.probe = imx8mm_blk_ctl_probe,
+	.driver = {
+		.name = "imx8mm-blk-ctl",
+		.of_match_table = of_match_ptr(imx_blk_ctl_of_match),
+		.pm = &imx_blk_ctl_pm_ops,
+	},
+};
+module_platform_driver(imx_blk_ctl_driver);
-- 
2.30.0

Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Adam Ford]

On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> V8:
> Revert one change in v7, force goto disable_clk for handshake when power on in patch 3
> One minor update to use if{} else {}, not if{}; if{}; in patch 3
> Typo Hankshake->Handshake
>
I am using ATF, branch lf_v2.4, from the NXP code aurora repo with
U-Boot v2021.07-rc5

I applied this patch against linux-next, I applied the pgc patches
[1], and the suggested power-domains to the otg1 and otg2 nodes.
I am able to boot the device and use USB, but with this applied, I
cannot wake from sleep.  If I revert this, the system wakes from sleep
again.

[1] - https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210604111005.6804-1-peng.fan@oss.nxp.com/

I have not enabled video, GPU, VPU nor CSI.

Just in case there might be a power-domain dependency missing, here is
my power-domain dump:

root@beacon-imx8mm-kit:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain                          status          children
            performance
    /device                                             runtime status
----------------------------------------------------------------------------------------------
vpu-h1                          off-0
            0
                                                VPU_BLK_CTL_H1
    /devices/genpd:3:imx-vpumix-blk-ctl.2               suspended
            0
vpu-g2                          off-0
            0
                                                VPU_BLK_CTL_G2
    /devices/genpd:2:imx-vpumix-blk-ctl.0               suspended
            0
vpu-g1                          off-0
            0
                                                VPU_BLK_CTL_G1
    /devices/genpd:1:imx-vpumix-blk-ctl.1               suspended
            0
mipi                            off-0
            0
                                                MIPI_DSI, MIPI_CSI
    /devices/genpd:1:imx-dispmix-blk-ctl.2              suspended
            0
    /devices/genpd:1:imx-dispmix-blk-ctl.3              suspended
            0
vpumix                          off-0
            0
                                                VPU_BLK_CTL_BUS
    /devices/genpd:0:imx-vpumix-blk-ctl.3               suspended
            0
gpu                             off-0
            0
VPU_BLK_CTL_BUS                 off-0
            0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.7
 suspended                  0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.8
 suspended                  0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.9
 suspended                  0
VPU_BLK_CTL_H1                  off-0
            0
VPU_BLK_CTL_G1                  off-0
            0
VPU_BLK_CTL_G2                  off-0
            0
DISPMIX_BUS                     off-0
            0
                                                CSI_BRIDGE, LCDIF
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.11
 suspended                  0
MIPI_CSI                        off-0
            0
MIPI_DSI                        off-0
            0
LCDIF                           off-0
            0
CSI_BRIDGE                      off-0
            0
dispmix                         off-0
            0
                                                DISPMIX_BUS
    /devices/genpd:0:imx-dispmix-blk-ctl.4              suspended
            0
gpumix                          off-0
            0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.5
 suspended                  0
usb-otg2                        on
            0
    /devices/platform/soc@0/32c00000.bus/32e50000.usb   active
            0
usb-otg1                        on
            0
    /devices/platform/soc@0/32c00000.bus/32e40000.usb   active
            0
pcie                            off-0
            0
hsiomix                         on
            0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.1
 suspended                  0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.2
 active                     0
    /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.3
 active                     0



> V7:
>  patch 2: update patch title per Shawn
>  Patch 3: Addressed several comments from Shawn
>
> V6:
>  Thanks for Adam's report on V5.
>  Resolve the error message dump, it is the child device reuse
>  the parent device node and matches the parent driver.
>  Filled the remove function for child device.
>  A diff dts file for upstream:
>  https://gist.github.com/MrVan/d73888d8273c43ea4a3b28fa668ca1d0
>
> V5:
>  Rework the blk-ctl driver to let sub-PGC use blk-ctl as parent power
>  domain to fix the potential handshake issue.
>  I still keep R-b/A-b tag for Patch 1,2,4, since very minor changes
>  I only drop R-b tag for Patch 3, since it has big change.
>  An example, the pgc_mipi not take pgc_dispmix as parent:
>
>         pgc_dispmix: power-domain@10 {
>                 #power-domain-cells = <0>;
>                 reg = <IMX8MM_POWER_DOMAIN_DISPMIX>;
>                 clocks = <&clk IMX8MM_CLK_DISP_ROOT>,
>                          <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
>                          <&clk IMX8MM_CLK_DISP_APB_ROOT>;
>         };
>
>         pgc_mipi: power-domain@11 {
>                 #power-domain-cells = <0>;
>                 reg = <IMX8MM_POWER_DOMAIN_MIPI>;
>                 power-domains = <&dispmix_blk_ctl IMX8MM_BLK_CTL_PD_DISPMIX_BUS>;
>         };
>
>         dispmix_blk_ctl: clock-controller@32e28000 {
>                 compatible = "fsl,imx8mm-dispmix-blk-ctl", "syscon";
>                 reg = <0x32e28000 0x100>;
>                 #power-domain-cells = <1>;
>                 power-domains = <&pgc_dispmix>, <&pgc_mipi>;
>                 power-domain-names = "dispmix", "mipi";
>                 clocks = <&clk IMX8MM_CLK_DISP_ROOT>, <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
>                          <&clk IMX8MM_CLK_DISP_APB_ROOT>;
>         };
>
> V4:
>  Add R-b tag
>  Typo fix
>  Update the power domain macro names Per Abel and Frieder
>
> V3:
>  Add explaination for not listing items in patch 2 commit log Per Rob.
>  Addressed comments from Lucas and Frieder on patch [3,4].
>  A few comments from Jacky was ignored, because following gpcv2
>  coding style.
>
> V2:
>  Fix yaml check failure.
>
> Previously there is an effort from Abel that take BLK-CTL as clock
> provider, but it turns out that there is A/B lock issue and we are
> not able resolve that.
>
> Per discuss with Lucas and Jacky, we made an agreement that take BLK-CTL
> as a power domain provider and use GPC's domain as parent, the consumer
> node take BLK-CTL as power domain input.
>
> This patchset has been tested on i.MX8MM EVK board, but one hack
> is not included in the patchset is that the DISPMIX BLK-CTL
> MIPI_M/S_RESET not implemented. Per Lucas, we will finally have a MIPI
> DPHY driver, so fine to leave it.
>
> Thanks for Lucas's suggestion, Frieder Schrempf for collecting
> all the patches, Abel's previous BLK-CTL work, Jacky Bai on help
> debug issues.
>
>
> Peng Fan (4):
>   dt-bindings: power: Add defines for i.MX8MM BLK-CTL power domains
>   dt-bindings: soc: imx: Add bindings for i.MX BLK_CTL
>   soc: imx: Add generic blk-ctl driver
>   soc: imx: Add blk-ctl driver for i.MX8MM
>
>  .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     |  66 ++++
>  drivers/soc/imx/Makefile                      |   2 +-
>  drivers/soc/imx/blk-ctl-imx8mm.c              | 139 ++++++++
>  drivers/soc/imx/blk-ctl.c                     | 324 ++++++++++++++++++
>  drivers/soc/imx/blk-ctl.h                     |  85 +++++
>  include/dt-bindings/power/imx8mm-power.h      |  13 +
>  6 files changed, 628 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
>  create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c
>  create mode 100644 drivers/soc/imx/blk-ctl.c
>  create mode 100644 drivers/soc/imx/blk-ctl.h
>
> --
> 2.30.0
>

RE: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Peng Fan (OSS)]

> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
> 
> On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > V8:
> > Revert one change in v7, force goto disable_clk for handshake when
> > power on in patch 3 One minor update to use if{} else {}, not if{};
> > if{}; in patch 3 Typo Hankshake->Handshake
> >
> I am using ATF, branch lf_v2.4, from the NXP code aurora repo with U-Boot
> v2021.07-rc5
> 
> I applied this patch against linux-next, I applied the pgc patches [1], and the
> suggested power-domains to the otg1 and otg2 nodes.
> I am able to boot the device and use USB, but with this applied, I cannot wake
> from sleep.  If I revert this, the system wakes from sleep again.

I just tried linux-next without this patch on iMX8MM EVK, suspend/resume
not work. Per my last test, it works before. Not sure what changed in kernel.

Which kernel are you using, any commit or git repo? I could try on imx8mm
evk and debug the issue you see.

Thanks,
Peng.

> 
> [1] -
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.kernel.org%2Fproject%2Flinux-arm-kernel%2Fpatch%2F202106041110
> 05.6804-1-peng.fan%40oss.nxp.com%2F&amp;data=04%7C01%7Cpeng.fan%
> 40nxp.com%7Caccded5458c049d67fa308d93b0182d9%7C686ea1d3bc2b4c6
> fa92cd99c5c301635%7C0%7C0%7C637605699944911752%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C1000&amp;sdata=Kf6orRXmScWHDTgD2FOV8OBsgG0p
> GVs1byVZTHT0gVI%3D&amp;reserved=0
> 
> I have not enabled video, GPU, VPU nor CSI.
> 
> Just in case there might be a power-domain dependency missing, here is my
> power-domain dump:
> 
> root@beacon-imx8mm-kit:~# cat
> /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain                          status          children
>             performance
>     /device                                             runtime
> status
> ---------------------------------------------------------------------------------------------
> -
> vpu-h1                          off-0
>             0
> 
> VPU_BLK_CTL_H1
>     /devices/genpd:3:imx-vpumix-blk-ctl.2               suspended
>             0
> vpu-g2                          off-0
>             0
> 
> VPU_BLK_CTL_G2
>     /devices/genpd:2:imx-vpumix-blk-ctl.0               suspended
>             0
> vpu-g1                          off-0
>             0
> 
> VPU_BLK_CTL_G1
>     /devices/genpd:1:imx-vpumix-blk-ctl.1               suspended
>             0
> mipi                            off-0
>             0
>                                                 MIPI_DSI,
> MIPI_CSI
>     /devices/genpd:1:imx-dispmix-blk-ctl.2              suspended
>             0
>     /devices/genpd:1:imx-dispmix-blk-ctl.3              suspended
>             0
> vpumix                          off-0
>             0
> 
> VPU_BLK_CTL_BUS
>     /devices/genpd:0:imx-vpumix-blk-ctl.3               suspended
>             0
> gpu                             off-0
>             0
> VPU_BLK_CTL_BUS                 off-0
>             0
> 
> /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.7
>  suspended                  0
> 
> /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.8
>  suspended                  0
> 
> /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.9
>  suspended                  0
> VPU_BLK_CTL_H1                  off-0
>             0
> VPU_BLK_CTL_G1                  off-0
>             0
> VPU_BLK_CTL_G2                  off-0
>             0
> DISPMIX_BUS                     off-0
>             0
>                                                 CSI_BRIDGE,
> LCDIF
> 
> /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.11
>  suspended                  0
> MIPI_CSI                        off-0
>             0
> MIPI_DSI                        off-0
>             0
> LCDIF                           off-0
>             0
> CSI_BRIDGE                      off-0
>             0
> dispmix                         off-0
>             0
>                                                 DISPMIX_BUS
>     /devices/genpd:0:imx-dispmix-blk-ctl.4              suspended
>             0
> gpumix                          off-0
>             0
> 
> /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.5
>  suspended                  0
> usb-otg2                        on
>             0
>     /devices/platform/soc@0/32c00000.bus/32e50000.usb   active
>             0
> usb-otg1                        on
>             0
>     /devices/platform/soc@0/32c00000.bus/32e40000.usb   active
>             0
> pcie                            off-0
>             0
> hsiomix                         on
>             0
> 
> /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.1
>  suspended                  0
> 
> /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.2
>  active                     0
> 
> /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.3
>  active                     0
> 
> 
> 
> > V7:
> >  patch 2: update patch title per Shawn  Patch 3: Addressed several
> > comments from Shawn
> >
> > V6:
> >  Thanks for Adam's report on V5.
> >  Resolve the error message dump, it is the child device reuse  the
> > parent device node and matches the parent driver.
> >  Filled the remove function for child device.
> >  A diff dts file for upstream:
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgist
> > .github.com%2FMrVan%2Fd73888d8273c43ea4a3b28fa668ca1d0&amp;dat
> a=04%7C0
> >
> 1%7Cpeng.fan%40nxp.com%7Caccded5458c049d67fa308d93b0182d9%7C68
> 6ea1d3bc
> >
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C637605699944911752%7CUnknow
> n%7CTWFpbG
> >
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%
> >
> 3D%7C1000&amp;sdata=9FcnPd4nwmdjOqrQCTz0YNgUmVQ9UzTHpfh33LKI
> 7cs%3D&amp
> > ;reserved=0
> >
> > V5:
> >  Rework the blk-ctl driver to let sub-PGC use blk-ctl as parent power
> > domain to fix the potential handshake issue.
> >  I still keep R-b/A-b tag for Patch 1,2,4, since very minor changes  I
> > only drop R-b tag for Patch 3, since it has big change.
> >  An example, the pgc_mipi not take pgc_dispmix as parent:
> >
> >         pgc_dispmix: power-domain@10 {
> >                 #power-domain-cells = <0>;
> >                 reg = <IMX8MM_POWER_DOMAIN_DISPMIX>;
> >                 clocks = <&clk IMX8MM_CLK_DISP_ROOT>,
> >                          <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
> >                          <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> >         };
> >
> >         pgc_mipi: power-domain@11 {
> >                 #power-domain-cells = <0>;
> >                 reg = <IMX8MM_POWER_DOMAIN_MIPI>;
> >                 power-domains = <&dispmix_blk_ctl
> IMX8MM_BLK_CTL_PD_DISPMIX_BUS>;
> >         };
> >
> >         dispmix_blk_ctl: clock-controller@32e28000 {
> >                 compatible = "fsl,imx8mm-dispmix-blk-ctl", "syscon";
> >                 reg = <0x32e28000 0x100>;
> >                 #power-domain-cells = <1>;
> >                 power-domains = <&pgc_dispmix>, <&pgc_mipi>;
> >                 power-domain-names = "dispmix", "mipi";
> >                 clocks = <&clk IMX8MM_CLK_DISP_ROOT>, <&clk
> IMX8MM_CLK_DISP_AXI_ROOT>,
> >                          <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> >         };
> >
> > V4:
> >  Add R-b tag
> >  Typo fix
> >  Update the power domain macro names Per Abel and Frieder
> >
> > V3:
> >  Add explaination for not listing items in patch 2 commit log Per Rob.
> >  Addressed comments from Lucas and Frieder on patch [3,4].
> >  A few comments from Jacky was ignored, because following gpcv2
> > coding style.
> >
> > V2:
> >  Fix yaml check failure.
> >
> > Previously there is an effort from Abel that take BLK-CTL as clock
> > provider, but it turns out that there is A/B lock issue and we are not
> > able resolve that.
> >
> > Per discuss with Lucas and Jacky, we made an agreement that take
> > BLK-CTL as a power domain provider and use GPC's domain as parent, the
> > consumer node take BLK-CTL as power domain input.
> >
> > This patchset has been tested on i.MX8MM EVK board, but one hack is
> > not included in the patchset is that the DISPMIX BLK-CTL
> > MIPI_M/S_RESET not implemented. Per Lucas, we will finally have a MIPI
> > DPHY driver, so fine to leave it.
> >
> > Thanks for Lucas's suggestion, Frieder Schrempf for collecting all the
> > patches, Abel's previous BLK-CTL work, Jacky Bai on help debug issues.
> >
> >
> > Peng Fan (4):
> >   dt-bindings: power: Add defines for i.MX8MM BLK-CTL power domains
> >   dt-bindings: soc: imx: Add bindings for i.MX BLK_CTL
> >   soc: imx: Add generic blk-ctl driver
> >   soc: imx: Add blk-ctl driver for i.MX8MM
> >
> >  .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     |  66 ++++
> >  drivers/soc/imx/Makefile                      |   2 +-
> >  drivers/soc/imx/blk-ctl-imx8mm.c              | 139 ++++++++
> >  drivers/soc/imx/blk-ctl.c                     | 324
> ++++++++++++++++++
> >  drivers/soc/imx/blk-ctl.h                     |  85 +++++
> >  include/dt-bindings/power/imx8mm-power.h      |  13 +
> >  6 files changed, 628 insertions(+), 1 deletion(-)  create mode 100644
> > Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
> >  create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c  create mode
> > 100644 drivers/soc/imx/blk-ctl.c  create mode 100644
> > drivers/soc/imx/blk-ctl.h
> >
> > --
> > 2.30.0
> >

Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Adam Ford]

On Wed, Jun 30, 2021 at 4:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> > Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
> >
> > On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > V8:
> > > Revert one change in v7, force goto disable_clk for handshake when
> > > power on in patch 3 One minor update to use if{} else {}, not if{};
> > > if{}; in patch 3 Typo Hankshake->Handshake
> > >
> > I am using ATF, branch lf_v2.4, from the NXP code aurora repo with U-Boot
> > v2021.07-rc5
> >
> > I applied this patch against linux-next, I applied the pgc patches [1], and the
> > suggested power-domains to the otg1 and otg2 nodes.
> > I am able to boot the device and use USB, but with this applied, I cannot wake
> > from sleep.  If I revert this, the system wakes from sleep again.
>
> I just tried linux-next without this patch on iMX8MM EVK, suspend/resume
> not work. Per my last test, it works before. Not sure what changed in kernel.
>
> Which kernel are you using, any commit or git repo? I could try on imx8mm
> evk and debug the issue you see.

I used kernel-next,
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
commit 889bab4c367a0ef58208fd80fafa74bb6e2dca26 (tag: next-20210621)

I then applied the GPCv2 patch that Marek sent.  You were CC'd on the
e-mail from Marek, but I can forward the patch to you if you can't
find it.
I tested his patch and I was able to suspend-to-RAM and resume.
Once I was comfortable that it worked, I then applied your patch
series for the blk-ctl.
With the blk-ctl series applied, the suspend-resume stopped working.

adam
>
> Thanks,
> Peng.
>
> >
> > [1] -
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> > work.kernel.org%2Fproject%2Flinux-arm-kernel%2Fpatch%2F202106041110
> > 05.6804-1-peng.fan%40oss.nxp.com%2F&amp;data=04%7C01%7Cpeng.fan%
> > 40nxp.com%7Caccded5458c049d67fa308d93b0182d9%7C686ea1d3bc2b4c6
> > fa92cd99c5c301635%7C0%7C0%7C637605699944911752%7CUnknown%7C
> > TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> > CJXVCI6Mn0%3D%7C1000&amp;sdata=Kf6orRXmScWHDTgD2FOV8OBsgG0p
> > GVs1byVZTHT0gVI%3D&amp;reserved=0
> >
> > I have not enabled video, GPU, VPU nor CSI.
> >
> > Just in case there might be a power-domain dependency missing, here is my
> > power-domain dump:
> >
> > root@beacon-imx8mm-kit:~# cat
> > /sys/kernel/debug/pm_genpd/pm_genpd_summary
> > domain                          status          children
> >             performance
> >     /device                                             runtime
> > status
> > ---------------------------------------------------------------------------------------------
> > -
> > vpu-h1                          off-0
> >             0
> >
> > VPU_BLK_CTL_H1
> >     /devices/genpd:3:imx-vpumix-blk-ctl.2               suspended
> >             0
> > vpu-g2                          off-0
> >             0
> >
> > VPU_BLK_CTL_G2
> >     /devices/genpd:2:imx-vpumix-blk-ctl.0               suspended
> >             0
> > vpu-g1                          off-0
> >             0
> >
> > VPU_BLK_CTL_G1
> >     /devices/genpd:1:imx-vpumix-blk-ctl.1               suspended
> >             0
> > mipi                            off-0
> >             0
> >                                                 MIPI_DSI,
> > MIPI_CSI
> >     /devices/genpd:1:imx-dispmix-blk-ctl.2              suspended
> >             0
> >     /devices/genpd:1:imx-dispmix-blk-ctl.3              suspended
> >             0
> > vpumix                          off-0
> >             0
> >
> > VPU_BLK_CTL_BUS
> >     /devices/genpd:0:imx-vpumix-blk-ctl.3               suspended
> >             0
> > gpu                             off-0
> >             0
> > VPU_BLK_CTL_BUS                 off-0
> >             0
> >
> > /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.7
> >  suspended                  0
> >
> > /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.8
> >  suspended                  0
> >
> > /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.9
> >  suspended                  0
> > VPU_BLK_CTL_H1                  off-0
> >             0
> > VPU_BLK_CTL_G1                  off-0
> >             0
> > VPU_BLK_CTL_G2                  off-0
> >             0
> > DISPMIX_BUS                     off-0
> >             0
> >                                                 CSI_BRIDGE,
> > LCDIF
> >
> > /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.11
> >  suspended                  0
> > MIPI_CSI                        off-0
> >             0
> > MIPI_DSI                        off-0
> >             0
> > LCDIF                           off-0
> >             0
> > CSI_BRIDGE                      off-0
> >             0
> > dispmix                         off-0
> >             0
> >                                                 DISPMIX_BUS
> >     /devices/genpd:0:imx-dispmix-blk-ctl.4              suspended
> >             0
> > gpumix                          off-0
> >             0
> >
> > /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.5
> >  suspended                  0
> > usb-otg2                        on
> >             0
> >     /devices/platform/soc@0/32c00000.bus/32e50000.usb   active
> >             0
> > usb-otg1                        on
> >             0
> >     /devices/platform/soc@0/32c00000.bus/32e40000.usb   active
> >             0
> > pcie                            off-0
> >             0
> > hsiomix                         on
> >             0
> >
> > /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.1
> >  suspended                  0
> >
> > /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.2
> >  active                     0
> >
> > /devices/platform/soc@0/30000000.bus/303a0000.gpc/imx-pgc-domain.3
> >  active                     0
> >
> >
> >
> > > V7:
> > >  patch 2: update patch title per Shawn  Patch 3: Addressed several
> > > comments from Shawn
> > >
> > > V6:
> > >  Thanks for Adam's report on V5.
> > >  Resolve the error message dump, it is the child device reuse  the
> > > parent device node and matches the parent driver.
> > >  Filled the remove function for child device.
> > >  A diff dts file for upstream:
> > >
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgist
> > > .github.com%2FMrVan%2Fd73888d8273c43ea4a3b28fa668ca1d0&amp;dat
> > a=04%7C0
> > >
> > 1%7Cpeng.fan%40nxp.com%7Caccded5458c049d67fa308d93b0182d9%7C68
> > 6ea1d3bc
> > >
> > 2b4c6fa92cd99c5c301635%7C0%7C0%7C637605699944911752%7CUnknow
> > n%7CTWFpbG
> > >
> > Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> > Mn0%
> > >
> > 3D%7C1000&amp;sdata=9FcnPd4nwmdjOqrQCTz0YNgUmVQ9UzTHpfh33LKI
> > 7cs%3D&amp
> > > ;reserved=0
> > >
> > > V5:
> > >  Rework the blk-ctl driver to let sub-PGC use blk-ctl as parent power
> > > domain to fix the potential handshake issue.
> > >  I still keep R-b/A-b tag for Patch 1,2,4, since very minor changes  I
> > > only drop R-b tag for Patch 3, since it has big change.
> > >  An example, the pgc_mipi not take pgc_dispmix as parent:
> > >
> > >         pgc_dispmix: power-domain@10 {
> > >                 #power-domain-cells = <0>;
> > >                 reg = <IMX8MM_POWER_DOMAIN_DISPMIX>;
> > >                 clocks = <&clk IMX8MM_CLK_DISP_ROOT>,
> > >                          <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
> > >                          <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> > >         };
> > >
> > >         pgc_mipi: power-domain@11 {
> > >                 #power-domain-cells = <0>;
> > >                 reg = <IMX8MM_POWER_DOMAIN_MIPI>;
> > >                 power-domains = <&dispmix_blk_ctl
> > IMX8MM_BLK_CTL_PD_DISPMIX_BUS>;
> > >         };
> > >
> > >         dispmix_blk_ctl: clock-controller@32e28000 {
> > >                 compatible = "fsl,imx8mm-dispmix-blk-ctl", "syscon";
> > >                 reg = <0x32e28000 0x100>;
> > >                 #power-domain-cells = <1>;
> > >                 power-domains = <&pgc_dispmix>, <&pgc_mipi>;
> > >                 power-domain-names = "dispmix", "mipi";
> > >                 clocks = <&clk IMX8MM_CLK_DISP_ROOT>, <&clk
> > IMX8MM_CLK_DISP_AXI_ROOT>,
> > >                          <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> > >         };
> > >
> > > V4:
> > >  Add R-b tag
> > >  Typo fix
> > >  Update the power domain macro names Per Abel and Frieder
> > >
> > > V3:
> > >  Add explaination for not listing items in patch 2 commit log Per Rob.
> > >  Addressed comments from Lucas and Frieder on patch [3,4].
> > >  A few comments from Jacky was ignored, because following gpcv2
> > > coding style.
> > >
> > > V2:
> > >  Fix yaml check failure.
> > >
> > > Previously there is an effort from Abel that take BLK-CTL as clock
> > > provider, but it turns out that there is A/B lock issue and we are not
> > > able resolve that.
> > >
> > > Per discuss with Lucas and Jacky, we made an agreement that take
> > > BLK-CTL as a power domain provider and use GPC's domain as parent, the
> > > consumer node take BLK-CTL as power domain input.
> > >
> > > This patchset has been tested on i.MX8MM EVK board, but one hack is
> > > not included in the patchset is that the DISPMIX BLK-CTL
> > > MIPI_M/S_RESET not implemented. Per Lucas, we will finally have a MIPI
> > > DPHY driver, so fine to leave it.
> > >
> > > Thanks for Lucas's suggestion, Frieder Schrempf for collecting all the
> > > patches, Abel's previous BLK-CTL work, Jacky Bai on help debug issues.
> > >
> > >
> > > Peng Fan (4):
> > >   dt-bindings: power: Add defines for i.MX8MM BLK-CTL power domains
> > >   dt-bindings: soc: imx: Add bindings for i.MX BLK_CTL
> > >   soc: imx: Add generic blk-ctl driver
> > >   soc: imx: Add blk-ctl driver for i.MX8MM
> > >
> > >  .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     |  66 ++++
> > >  drivers/soc/imx/Makefile                      |   2 +-
> > >  drivers/soc/imx/blk-ctl-imx8mm.c              | 139 ++++++++
> > >  drivers/soc/imx/blk-ctl.c                     | 324
> > ++++++++++++++++++
> > >  drivers/soc/imx/blk-ctl.h                     |  85 +++++
> > >  include/dt-bindings/power/imx8mm-power.h      |  13 +
> > >  6 files changed, 628 insertions(+), 1 deletion(-)  create mode 100644
> > > Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
> > >  create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c  create mode
> > > 100644 drivers/soc/imx/blk-ctl.c  create mode 100644
> > > drivers/soc/imx/blk-ctl.h
> > >
> > > --
> > > 2.30.0
> > >

Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Frieder Schrempf]

On 30.06.21 14:09, Adam Ford wrote:
> On Wed, Jun 30, 2021 at 4:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>>
>>> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
>>>
>>> On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
>>> wrote:
>>>>
>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>
>>>> V8:
>>>> Revert one change in v7, force goto disable_clk for handshake when
>>>> power on in patch 3 One minor update to use if{} else {}, not if{};
>>>> if{}; in patch 3 Typo Hankshake->Handshake
>>>>
>>> I am using ATF, branch lf_v2.4, from the NXP code aurora repo with U-Boot
>>> v2021.07-rc5
>>>
>>> I applied this patch against linux-next, I applied the pgc patches [1], and the
>>> suggested power-domains to the otg1 and otg2 nodes.
>>> I am able to boot the device and use USB, but with this applied, I cannot wake
>>> from sleep.  If I revert this, the system wakes from sleep again.
>>
>> I just tried linux-next without this patch on iMX8MM EVK, suspend/resume
>> not work. Per my last test, it works before. Not sure what changed in kernel.
>>
>> Which kernel are you using, any commit or git repo? I could try on imx8mm
>> evk and debug the issue you see.
> 
> I used kernel-next,
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> commit 889bab4c367a0ef58208fd80fafa74bb6e2dca26 (tag: next-20210621)
> 
> I then applied the GPCv2 patch that Marek sent.  You were CC'd on the
> e-mail from Marek, but I can forward the patch to you if you can't
> find it.
> I tested his patch and I was able to suspend-to-RAM and resume.
> Once I was comfortable that it worked, I then applied your patch
> series for the blk-ctl.
> With the blk-ctl series applied, the suspend-resume stopped working.

Same here. I tested with linux-next-20210629 and as soon as I add the BLK-CTL driver and devicetree nodes, the resume after suspend causes a lockup each time.

Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Marek Vasut]

On 6/30/21 4:46 PM, Frieder Schrempf wrote:
> On 30.06.21 14:09, Adam Ford wrote:
>> On Wed, Jun 30, 2021 at 4:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>>>
>>>> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
>>>>
>>>> On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
>>>> wrote:
>>>>>
>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>
>>>>> V8:
>>>>> Revert one change in v7, force goto disable_clk for handshake when
>>>>> power on in patch 3 One minor update to use if{} else {}, not if{};
>>>>> if{}; in patch 3 Typo Hankshake->Handshake
>>>>>
>>>> I am using ATF, branch lf_v2.4, from the NXP code aurora repo with U-Boot
>>>> v2021.07-rc5
>>>>
>>>> I applied this patch against linux-next, I applied the pgc patches [1], and the
>>>> suggested power-domains to the otg1 and otg2 nodes.
>>>> I am able to boot the device and use USB, but with this applied, I cannot wake
>>>> from sleep.  If I revert this, the system wakes from sleep again.
>>>
>>> I just tried linux-next without this patch on iMX8MM EVK, suspend/resume
>>> not work. Per my last test, it works before. Not sure what changed in kernel.
>>>
>>> Which kernel are you using, any commit or git repo? I could try on imx8mm
>>> evk and debug the issue you see.
>>
>> I used kernel-next,
>> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>> commit 889bab4c367a0ef58208fd80fafa74bb6e2dca26 (tag: next-20210621)
>>
>> I then applied the GPCv2 patch that Marek sent.  You were CC'd on the
>> e-mail from Marek, but I can forward the patch to you if you can't
>> find it.
>> I tested his patch and I was able to suspend-to-RAM and resume.
>> Once I was comfortable that it worked, I then applied your patch
>> series for the blk-ctl.
>> With the blk-ctl series applied, the suspend-resume stopped working.
> 
> Same here. I tested with linux-next-20210629 and as soon as I add the BLK-CTL driver and devicetree nodes, the resume after suspend causes a lockup each time.

btw do you have etnaviv enabled ?

Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Frieder Schrempf]

On 30.06.21 18:28, Marek Vasut wrote:
> On 6/30/21 4:46 PM, Frieder Schrempf wrote:
>> On 30.06.21 14:09, Adam Ford wrote:
>>> On Wed, Jun 30, 2021 at 4:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>>>>
>>>>> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
>>>>>
>>>>> On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
>>>>> wrote:
>>>>>>
>>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>>
>>>>>> V8:
>>>>>> Revert one change in v7, force goto disable_clk for handshake when
>>>>>> power on in patch 3 One minor update to use if{} else {}, not if{};
>>>>>> if{}; in patch 3 Typo Hankshake->Handshake
>>>>>>
>>>>> I am using ATF, branch lf_v2.4, from the NXP code aurora repo with U-Boot
>>>>> v2021.07-rc5
>>>>>
>>>>> I applied this patch against linux-next, I applied the pgc patches [1], and the
>>>>> suggested power-domains to the otg1 and otg2 nodes.
>>>>> I am able to boot the device and use USB, but with this applied, I cannot wake
>>>>> from sleep.  If I revert this, the system wakes from sleep again.
>>>>
>>>> I just tried linux-next without this patch on iMX8MM EVK, suspend/resume
>>>> not work. Per my last test, it works before. Not sure what changed in kernel.
>>>>
>>>> Which kernel are you using, any commit or git repo? I could try on imx8mm
>>>> evk and debug the issue you see.
>>>
>>> I used kernel-next,
>>> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>>> commit 889bab4c367a0ef58208fd80fafa74bb6e2dca26 (tag: next-20210621)
>>>
>>> I then applied the GPCv2 patch that Marek sent.  You were CC'd on the
>>> e-mail from Marek, but I can forward the patch to you if you can't
>>> find it.
>>> I tested his patch and I was able to suspend-to-RAM and resume.
>>> Once I was comfortable that it worked, I then applied your patch
>>> series for the blk-ctl.
>>> With the blk-ctl series applied, the suspend-resume stopped working.
>>
>> Same here. I tested with linux-next-20210629 and as soon as I add the BLK-CTL driver and devicetree nodes, the resume after suspend causes a lockup each time.
> 
> btw do you have etnaviv enabled ?

yes, but I can try without and see if it's related.

Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Frieder Schrempf]

On 30.06.21 19:20, Frieder Schrempf wrote:
> On 30.06.21 18:28, Marek Vasut wrote:
>> On 6/30/21 4:46 PM, Frieder Schrempf wrote:
>>> On 30.06.21 14:09, Adam Ford wrote:
>>>> On Wed, Jun 30, 2021 at 4:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>>>>>
>>>>>> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
>>>>>>
>>>>>> On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>>>
>>>>>>> V8:
>>>>>>> Revert one change in v7, force goto disable_clk for handshake when
>>>>>>> power on in patch 3 One minor update to use if{} else {}, not if{};
>>>>>>> if{}; in patch 3 Typo Hankshake->Handshake
>>>>>>>
>>>>>> I am using ATF, branch lf_v2.4, from the NXP code aurora repo with U-Boot
>>>>>> v2021.07-rc5
>>>>>>
>>>>>> I applied this patch against linux-next, I applied the pgc patches [1], and the
>>>>>> suggested power-domains to the otg1 and otg2 nodes.
>>>>>> I am able to boot the device and use USB, but with this applied, I cannot wake
>>>>>> from sleep.  If I revert this, the system wakes from sleep again.
>>>>>
>>>>> I just tried linux-next without this patch on iMX8MM EVK, suspend/resume
>>>>> not work. Per my last test, it works before. Not sure what changed in kernel.
>>>>>
>>>>> Which kernel are you using, any commit or git repo? I could try on imx8mm
>>>>> evk and debug the issue you see.
>>>>
>>>> I used kernel-next,
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>>>> commit 889bab4c367a0ef58208fd80fafa74bb6e2dca26 (tag: next-20210621)
>>>>
>>>> I then applied the GPCv2 patch that Marek sent.  You were CC'd on the
>>>> e-mail from Marek, but I can forward the patch to you if you can't
>>>> find it.
>>>> I tested his patch and I was able to suspend-to-RAM and resume.
>>>> Once I was comfortable that it worked, I then applied your patch
>>>> series for the blk-ctl.
>>>> With the blk-ctl series applied, the suspend-resume stopped working.
>>>
>>> Same here. I tested with linux-next-20210629 and as soon as I add the BLK-CTL driver and devicetree nodes, the resume after suspend causes a lockup each time.
>>
>> btw do you have etnaviv enabled ?
> 
> yes, but I can try without and see if it's related.

It looks like the issue is not GPU-related. It appears once I add the pgc_dispmix, pgc_mipi and dispmix_blk_ctl nodes to the dt (even without any users, so lcdif and dsim are disabled). Once I remove the three nodes the issue is gone.

RE: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Peng Fan]

All,

> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
> 
> On 30.06.21 19:20, Frieder Schrempf wrote:
> > On 30.06.21 18:28, Marek Vasut wrote:
> >> On 6/30/21 4:46 PM, Frieder Schrempf wrote:
> >>> On 30.06.21 14:09, Adam Ford wrote:
> >>>> On Wed, Jun 30, 2021 at 4:34 AM Peng Fan (OSS)
> <peng.fan@oss.nxp.com> wrote:
> >>>>>
> >>>>>> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
> >>>>>>
> >>>>>> On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS)
> >>>>>> <peng.fan@oss.nxp.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> From: Peng Fan <peng.fan@nxp.com>
> >>>>>>>
> >>>>>>> V8:
> >>>>>>> Revert one change in v7, force goto disable_clk for handshake
> >>>>>>> when power on in patch 3 One minor update to use if{} else {},
> >>>>>>> not if{}; if{}; in patch 3 Typo Hankshake->Handshake
> >>>>>>>
> >>>>>> I am using ATF, branch lf_v2.4, from the NXP code aurora repo
> >>>>>> with U-Boot
> >>>>>> v2021.07-rc5
> >>>>>>
> >>>>>> I applied this patch against linux-next, I applied the pgc
> >>>>>> patches [1], and the suggested power-domains to the otg1 and otg2
> nodes.
> >>>>>> I am able to boot the device and use USB, but with this applied,
> >>>>>> I cannot wake from sleep.  If I revert this, the system wakes from
> sleep again.
> >>>>>
> >>>>> I just tried linux-next without this patch on iMX8MM EVK,
> >>>>> suspend/resume not work. Per my last test, it works before. Not sure
> what changed in kernel.
> >>>>>
> >>>>> Which kernel are you using, any commit or git repo? I could try on
> >>>>> imx8mm evk and debug the issue you see.
> >>>>
> >>>> I used kernel-next,
> >>>> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> >>>> commit 889bab4c367a0ef58208fd80fafa74bb6e2dca26 (tag:
> >>>> next-20210621)
> >>>>
> >>>> I then applied the GPCv2 patch that Marek sent.  You were CC'd on
> >>>> the e-mail from Marek, but I can forward the patch to you if you
> >>>> can't find it.
> >>>> I tested his patch and I was able to suspend-to-RAM and resume.
> >>>> Once I was comfortable that it worked, I then applied your patch
> >>>> series for the blk-ctl.
> >>>> With the blk-ctl series applied, the suspend-resume stopped working.
> >>>
> >>> Same here. I tested with linux-next-20210629 and as soon as I add the
> BLK-CTL driver and devicetree nodes, the resume after suspend causes a
> lockup each time.
> >>
> >> btw do you have etnaviv enabled ?
> >
> > yes, but I can try without and see if it's related.
> 
> It looks like the issue is not GPU-related. It appears once I add the
> pgc_dispmix, pgc_mipi and dispmix_blk_ctl nodes to the dt (even without any
> users, so lcdif and dsim are disabled). Once I remove the three nodes the
> issue is gone.


I have an updated code here:
https://github.com/MrVan/linux/tree/linux-next-master-628-blk-ctl-test

Only have blk-ctl,gpc,pd update. Please help test with applying your
local patches with some peripherals enabled.

I tested the upper code with suspend/resume with uart wakeup,
it not hang anymore.

Thanks,
Peng.

Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Frieder Schrempf]

On 02.07.21 05:24, Peng Fan wrote:
> All,
> 
>> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
>>
>> On 30.06.21 19:20, Frieder Schrempf wrote:
>>> On 30.06.21 18:28, Marek Vasut wrote:
>>>> On 6/30/21 4:46 PM, Frieder Schrempf wrote:
>>>>> On 30.06.21 14:09, Adam Ford wrote:
>>>>>> On Wed, Jun 30, 2021 at 4:34 AM Peng Fan (OSS)
>> <peng.fan@oss.nxp.com> wrote:
>>>>>>>
>>>>>>>> Subject: Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support
>>>>>>>>
>>>>>>>> On Tue, Jun 29, 2021 at 1:56 AM Peng Fan (OSS)
>>>>>>>> <peng.fan@oss.nxp.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>>>>>
>>>>>>>>> V8:
>>>>>>>>> Revert one change in v7, force goto disable_clk for handshake
>>>>>>>>> when power on in patch 3 One minor update to use if{} else {},
>>>>>>>>> not if{}; if{}; in patch 3 Typo Hankshake->Handshake
>>>>>>>>>
>>>>>>>> I am using ATF, branch lf_v2.4, from the NXP code aurora repo
>>>>>>>> with U-Boot
>>>>>>>> v2021.07-rc5
>>>>>>>>
>>>>>>>> I applied this patch against linux-next, I applied the pgc
>>>>>>>> patches [1], and the suggested power-domains to the otg1 and otg2
>> nodes.
>>>>>>>> I am able to boot the device and use USB, but with this applied,
>>>>>>>> I cannot wake from sleep.  If I revert this, the system wakes from
>> sleep again.
>>>>>>>
>>>>>>> I just tried linux-next without this patch on iMX8MM EVK,
>>>>>>> suspend/resume not work. Per my last test, it works before. Not sure
>> what changed in kernel.
>>>>>>>
>>>>>>> Which kernel are you using, any commit or git repo? I could try on
>>>>>>> imx8mm evk and debug the issue you see.
>>>>>>
>>>>>> I used kernel-next,
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>>>>>> commit 889bab4c367a0ef58208fd80fafa74bb6e2dca26 (tag:
>>>>>> next-20210621)
>>>>>>
>>>>>> I then applied the GPCv2 patch that Marek sent.  You were CC'd on
>>>>>> the e-mail from Marek, but I can forward the patch to you if you
>>>>>> can't find it.
>>>>>> I tested his patch and I was able to suspend-to-RAM and resume.
>>>>>> Once I was comfortable that it worked, I then applied your patch
>>>>>> series for the blk-ctl.
>>>>>> With the blk-ctl series applied, the suspend-resume stopped working.
>>>>>
>>>>> Same here. I tested with linux-next-20210629 and as soon as I add the
>> BLK-CTL driver and devicetree nodes, the resume after suspend causes a
>> lockup each time.
>>>>
>>>> btw do you have etnaviv enabled ?
>>>
>>> yes, but I can try without and see if it's related.
>>
>> It looks like the issue is not GPU-related. It appears once I add the
>> pgc_dispmix, pgc_mipi and dispmix_blk_ctl nodes to the dt (even without any
>> users, so lcdif and dsim are disabled). Once I remove the three nodes the
>> issue is gone.
> 
> 
> I have an updated code here:
> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMrVan%2Flinux%2Ftree%2Flinux-next-master-628-blk-ctl-test&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C1ff517847da04f73218f08d93d08effc%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637607930865259413%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=p5xGgid7lnfEpGK5TbljHrn0B1b0oF%2FO39G81UyXhyY%3D&amp;reserved=0
> 
> Only have blk-ctl,gpc,pd update. Please help test with applying your
> local patches with some peripherals enabled.
> 
> I tested the upper code with suspend/resume with uart wakeup,
> it not hang anymore.

Sorry, but in my case I can't boot this version at all. It always locks up at boot.

Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver

[Lucas Stach]

Hi Peng,

Am Dienstag, dem 29.06.2021 um 15:29 +0800 schrieb Peng Fan (OSS):
> From: Peng Fan <peng.fan@nxp.com>
> 
> The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
> some GPRs.
> 
> The GPRs has some clock bits and reset bits, but here we take it
> as virtual PDs, because of the clock and power domain A/B lock issue
> when taking it as a clock controller.
> 
> For some bits, it might be good to also make it as a reset controller,
> but to i.MX8MM, we not add that support for now.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/soc/imx/Makefile  |   2 +-
>  drivers/soc/imx/blk-ctl.c | 324 ++++++++++++++++++++++++++++++++++++++
>  drivers/soc/imx/blk-ctl.h |  85 ++++++++++
>  3 files changed, 410 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soc/imx/blk-ctl.c
>  create mode 100644 drivers/soc/imx/blk-ctl.h
> 
> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> index 078dc918f4f3..d3d2b49a386c 100644
> --- a/drivers/soc/imx/Makefile
> +++ b/drivers/soc/imx/Makefile
> @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
>  endif
>  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
>  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c
> new file mode 100644
> index 000000000000..cec1884202e0
> --- /dev/null
> +++ b/drivers/soc/imx/blk-ctl.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2021 NXP.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include "blk-ctl.h"
> +
> +static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct generic_pm_domain *genpd)
> +{
> +	return container_of(genpd, struct imx_blk_ctl_domain, genpd);
> +}
> +
> +static int imx_blk_ctl_enable_hsk(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_hw *hw = blk_ctl->dev_data->hw_hsk;
> +	struct regmap *regmap = blk_ctl->regmap;
> +	int ret;
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> +
> +	/* Wait for handshake */
> +	udelay(5);
> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_power_on(struct generic_pm_domain *domain)
> +{
> +	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
> +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> +	struct regmap *regmap = blk_ctl->regmap;
> +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->pds[pd->id];
> +	int ret;
> +
> +	mutex_lock(&blk_ctl->lock);
> +
> +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);

I'm still not a fan of enabling all the clocks going into the blk-ctl
to power up/down one specific domain. It's not really a problem with
clocks, where the parents are always on, as the clock gate/ungate is
pretty cheap, but as soon as you get to something like the display
pixel clock, where the parent PLL may be shut down, the clock enable
may easily be the most costly operation of this whole function, even if
this specific clock isn't even needed for the domain in question.

> +	if (ret) {
> +		mutex_unlock(&blk_ctl->lock);
> +		return ret;
> +	}
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_HANDSHAKE) {
> +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> +		if (ret)
> +			dev_err(blk_ctl->dev, "Handshake failed when power on\n");
> +
> +		/* Expected, handshake already handle reset*/
> +		goto disable_clk;
> +	}
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> +		ret = regmap_clear_bits(regmap, hw->rst_offset, hw->rst_mask);
> +		if (ret)
> +			goto disable_clk;
> +
> +		/* Wait for reset propagate */
> +		udelay(5);
> +
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
> +		if (ret)
> +			goto disable_clk;
> +	}
> +
> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> 

I find this very hard to follow and reason about. Why do we even need
those different paths for domains with or without the handshake?

Shouldn't it be enough to just be enough to do the following in all
cases:
1. release sft reset
2. enable sft clock
3. wait a little for reset to propagate or ADB to ack power up

> +disable_clk:
> +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> +
> +	mutex_unlock(&blk_ctl->lock);
> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_power_off(struct generic_pm_domain *domain)
> +{
> +	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
> +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> +	struct regmap *regmap = blk_ctl->regmap;
> +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->pds[pd->id];
> +	int ret = 0;
> +
> +	mutex_lock(&blk_ctl->lock);
> +
> +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> +	if (ret) {
> +		mutex_unlock(&blk_ctl->lock);
> +		return ret;
> +	}
> +
> +	if (!(hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)) {
> +		ret = regmap_clear_bits(regmap, hw->offset, hw->mask);
> +		if (ret)
> +			goto disable_clk;
> +
> +		if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> +			ret = regmap_clear_bits(regmap, hw->rst_offset, hw->rst_mask);
> +			if (ret)
> +				goto disable_clk;
> +		}
> +	} else {
> +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);

Why would we need to enable the handshake again in the power DOWN path?
The clock/reset bits should still be set from the power up. The power
down should probably just be a no-op for the blk-ctl bus domains, to
allow the proper ADB handshake in the PGC domain power-down.

> +		if (ret)
> +			dev_err(blk_ctl->dev, "Handshake failed when power off\n");
> +	}
> +
> +disable_clk:
> +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> +
> +	mutex_unlock(&blk_ctl->lock);
> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_probe(struct platform_device *pdev)
> +{
> +	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
> +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> +	struct generic_pm_domain *parent_genpd;
> +	struct device *dev = &pdev->dev;
> +	struct device *active_pd;
> +	int ret;
> +
> +	pdev->dev.of_node = blk_ctl->dev->of_node;
> +
> +	if (domain->hw->active_pd_name) {
> +		active_pd = dev_pm_domain_attach_by_name(dev, domain->hw->active_pd_name);
> +		if (IS_ERR_OR_NULL(active_pd)) {
> +			ret = PTR_ERR(active_pd) ? : -ENODATA;
> +			pdev->dev.of_node = NULL;

This is extremely ugly. I think we should not even have separate
platform drivers for the blk-ctl domains, there is just no reason for
it. See below for more comments in that direction.
 
> +			return ret;
> +		}
> +
> +		domain->active_pd = active_pd;
> +	} else {
> +		if (!blk_ctl->bus_domain) {
> +			pdev->dev.of_node = NULL;
> +			return -EPROBE_DEFER;
> +		}
> +	}
> +
> +	if (domain->hw->active_pd_name)
> +		parent_genpd = pd_to_genpd(active_pd->pm_domain);
> +	else
> +		parent_genpd = blk_ctl->bus_domain;
> +
> +	if (pm_genpd_add_subdomain(parent_genpd, &domain->genpd)) {
> +		dev_warn(dev, "failed to add subdomain: %s\n", domain->genpd.name);

I don't see where the dispmix_bus domain and clock is kept enabled. I
would guess that the bus domain should be some kind of parent to the
lcdif and mipi domains, as I don't think it would be okay to disable
the bus clock, while any of the peripherals in the dispmix complex are
still active. Am I missing something here?

> +	} else {
> +		mutex_lock(&blk_ctl->lock);
> +		domain->hooked = true;
> +		mutex_unlock(&blk_ctl->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_blk_ctl_remove(struct platform_device *pdev)
> +{
> +	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
> +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> +	struct generic_pm_domain *parent_genpd;
> +	struct device *active_pd;
> +
> +	if (domain->hw->active_pd_name)
> +		parent_genpd = pd_to_genpd(active_pd->pm_domain);

This has probably never been tested. active_pd is undefined at this
point, so will most likely lead to a kernel crash.
> +	else
> +		parent_genpd = blk_ctl->bus_domain;
> +
> +	pm_genpd_remove_subdomain(parent_genpd, &domain->genpd);
> +
> +	mutex_lock(&blk_ctl->lock);
> +	domain->hooked = false;
> +	mutex_unlock(&blk_ctl->lock);
> +
> +	if (domain->hw->active_pd_name)
> +		dev_pm_domain_detach(domain->active_pd, false);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id imx_blk_ctl_id[] = {
> +	{ "imx-vpumix-blk-ctl", },
> +	{ "imx-dispmix-blk-ctl", },
> +	{ },
> +};
> +
> +static struct platform_driver imx_blk_ctl_driver = {
> +	.driver = {
> +		.name = "imx-blk-ctl",
> +	},
> +	.probe    = imx_blk_ctl_probe,
> +	.remove   = imx_blk_ctl_remove,
> +	.id_table = imx_blk_ctl_id,
> +};
> +builtin_platform_driver(imx_blk_ctl_driver)
> +
> +static struct generic_pm_domain *imx_blk_ctl_genpd_xlate(struct of_phandle_args *genpdspec,
> +							 void *data)
> +{
> +	struct genpd_onecell_data *genpd_data = data;
> +	unsigned int idx = genpdspec->args[0];
> +	struct imx_blk_ctl_domain *domain;
> +	struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
> +
> +	if (genpdspec->args_count != 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (idx >= genpd_data->num_domains)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!genpd_data->domains[idx])
> +		return ERR_PTR(-ENOENT);
> +
> +	domain = to_imx_blk_ctl_pd(genpd_data->domains[idx]);
> +
> +	mutex_lock(&domain->blk_ctl->lock);
> +	if (domain->hooked)
> +		genpd = genpd_data->domains[idx];
> +	mutex_unlock(&domain->blk_ctl->lock);
> +
> +	return genpd;
> +}
> +
> +int imx_blk_ctl_register(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> +	int num = dev_data->pds_num;
> +	struct imx_blk_ctl_domain *domain;
> +	struct generic_pm_domain *genpd;
> +	struct platform_device *pd_pdev;
> +	int domain_index;
> +	int i, ret;
> +
> +	blk_ctl->onecell_data.num_domains = num;
> +	blk_ctl->onecell_data.xlate = imx_blk_ctl_genpd_xlate;
> +	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num, sizeof(struct generic_pm_domain *),
> +						     GFP_KERNEL);
> +	if (!blk_ctl->onecell_data.domains)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num; i++) {
> +		domain_index = dev_data->pds[i].id;
> +		if (domain_index >= num) {
> +			dev_warn(dev, "Domain index %d is out of bounds\n", domain_index);
> +			continue;
> +		}
> +
> +		domain = devm_kzalloc(dev, sizeof(struct imx_blk_ctl_domain), GFP_KERNEL);
> +		if (!domain)
> +			goto error;
> +
> +		pd_pdev = platform_device_alloc(dev_data->name, domain_index);
> +		if (!pd_pdev) {
> +			dev_err(dev, "Failed to allocate platform device\n");
> +			goto error;
> +		}

We don't need a full blow platform device and a driver for the
individual domains. The only point where we need the device is to
attach the parent PGC power domains and for this we only need a
device. 

So we could either have a dummy device for this usage in the domain or
we could even reuse the device in the genpd, which is initialized in
pm_genpd_init.

Now while I think about it... genpd_dev_pm_attach_by_name already
allocates the virtual dummy device. I don't think we need to do
anything here on our own. Just get rid of the platform device and
driver.

> +
> +		pd_pdev->dev.platform_data = domain;
> +
> +		domain->blk_ctl = blk_ctl;
> +		domain->hw = &dev_data->pds[i];
> +		domain->id = domain_index;
> +		domain->genpd.name = dev_data->pds[i].name;
> +		domain->genpd.power_off = imx_blk_ctl_power_off;
> +		domain->genpd.power_on = imx_blk_ctl_power_on;
> +		domain->dev = &pd_pdev->dev;
> +		domain->hooked = false;
> +
> +		ret = pm_genpd_init(&domain->genpd, NULL, true);
> +		pd_pdev->dev.parent = dev;
> +
> +		if (domain->hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)
> +			blk_ctl->bus_domain = &domain->genpd;
> +
> +		ret = platform_device_add(pd_pdev);
> +		if (ret) {
> +			platform_device_put(pd_pdev);
> +			goto error;
> +		}

There is really no need for the complexity with the hooked property
(the mutex around the read/write access still doesn't make it properly
thread safe) and the blk-ctl domain probe/remove calls. Just handle
everything within this loop. This would make the driver a whole lot
more easy to follow.

> +		blk_ctl->onecell_data.domains[i] = &domain->genpd;
> +	}
> +
> +	return of_genpd_add_provider_onecell(dev->of_node, &blk_ctl->onecell_data);
> +
> +error:
> +	for (; i >= 0; i--) {
> +		genpd = blk_ctl->onecell_data.domains[i];
> +		if (!genpd)
> +			continue;
> +		domain = to_imx_blk_ctl_pd(genpd);
> +		if (domain->dev)
> +			platform_device_put(to_platform_device(domain->dev));
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_blk_ctl_register);
> +
> +const struct dev_pm_ops imx_blk_ctl_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +};
> +EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);

This code is linked into the same module as the platform driver using
it. So there is no need to export those symbols and expose them to the
whole kernel.

> diff --git a/drivers/soc/imx/blk-ctl.h b/drivers/soc/imx/blk-ctl.h
> new file mode 100644
> index 000000000000..6780d00ec8c5
> --- /dev/null
> +++ b/drivers/soc/imx/blk-ctl.h
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_IMX_BLK_CTL_H
> +#define __SOC_IMX_BLK_CTL_H
> +
> +enum imx_blk_ctl_pd_type {
> +	BLK_CTL_PD,
> +};
> +
> +struct imx_blk_ctl_hw {
> +	int type;

Initialized, but unused.

> +	char *name;
> +	char *active_pd_name;
> +	u32 offset;
> +	u32 mask;

offset and mask are way too generic names. I had to spend some time
reading the driver to find out that those are the clock enable bits.
This should be clear from the naming.

> +	u32 flags; 
> +	u32 id;
> +	u32 rst_offset;
> +	u32 rst_mask;
> +	u32 errata;

Unused.

> +};
> +
> +struct imx_blk_ctl_domain {
> +	struct generic_pm_domain genpd;
> +	struct device *active_pd;
> +	struct imx_blk_ctl *blk_ctl;
> +	struct imx_blk_ctl_hw *hw;
> +	struct device *dev;
> +	bool hooked;
> +	u32 id;

There are already a lot of pointers between the different structures.
Why do we need those id properties? You should be able to get to all
needed information by chasing pointers, instead of  indexing into
arrays. Really the only point where a id->domain mapping is done should
be the xlate function.

> +};
> +
> +struct imx_blk_ctl_dev_data {
> +	struct regmap_config config;
> +	struct imx_blk_ctl_hw *pds;
> +	struct imx_blk_ctl_hw *hw_hsk;
> +	u32 pds_num;
> +	u32 max_num;
> +	char *name;
> +};
> +
> +struct imx_blk_ctl {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct genpd_onecell_data onecell_data;
> +	const struct imx_blk_ctl_dev_data *dev_data;
> +	struct clk_bulk_data *clks;
> +	u32 num_clks;
> +	struct generic_pm_domain *bus_domain;

There should be nothing special about the bus domain at all. Right now
this permeates through the whole driver that the bus domain is somehow
special. It should just be a parent domain of the other domains, or
kept alive via some other appropriate means.

> +
> +	struct mutex lock;

This mutex is only used in the common blk-ctl code, but is initialized
in imx8mm_blk_ctrl_probe. This seems very inconsistent. I would have
expected this mutex to be initialized in imx_blk_ctl_register. However,
once you get rid of the hooked and bus domain magic, this mutex may as
well be per-domain, at which point I think you don't even need the
mutex, as the genpd locking should be enough then.

> +};
> +
> +#define IMX_BLK_CTL(_type, _name, _active_pd, _id, _offset, _mask, _rst_offset, _rst_mask,	\
> +		    _flags, _errata)								\
> +	{											\
> +		.type = _type,									\
> +		.name = _name,									\
> +		.active_pd_name = _active_pd,							\
> +		.id = _id,									\
> +		.offset = _offset,								\
> +		.mask = _mask,									\
> +		.flags = _flags,								\
> +		.rst_offset = _rst_offset,							\
> +		.rst_mask = _rst_mask,								\
> +		.errata = _errata,								\
> +	}
> +
> +#define IMX_BLK_CTL_PD(_name, _active_pd, _id, _offset, _mask, _rst_offset, _rst_mask, _flags)	\
> +	IMX_BLK_CTL(BLK_CTL_PD, _name, _active_pd, _id, _offset, _mask, _rst_offset,		\
> +		    _rst_mask, _flags, 0)
> +
> +#define IMX_BLK_CTL_PD_ERRATA(_name, _active_pd, _id, _offset, _mask, _rst_offset, _rst_mask,	\
> +			      _flags, _errata)							\
> +	IMX_BLK_CTL(BLK_CTL_PD, _name, _active_pd, _id, _offset, _mask, _rst_offset,		\
> +		    _rst_mask, _flags, _errata)
> +
> +int imx_blk_ctl_register(struct device *dev);
> +
> +#define IMX_BLK_CTL_PD_HANDSHAKE	BIT(0)
> +#define IMX_BLK_CTL_PD_RESET		BIT(1)
> +#define IMX_BLK_CTL_PD_BUS		BIT(2)
> +
> +const extern struct dev_pm_ops imx_blk_ctl_pm_ops;
> +
> +#endif

RE: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver

[Peng Fan]

Hi Lucas,

> Subject: Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver

After rethinking about this driver, I think we still need
expose to pgc to touch the blk-ctl hardware bus handshake.

Currently we are assuming the blk-ctl probe will finish
the handshake. But there is another case, saying gpu.

----------------------------------------------------------------------->
GPU on(1)
          VPUMIX on (2)
                        GPU off(3)
        
Between GPU on/off, VPUMIX-BLK-CTL not done, so
vpumix pgc handshake not done. Then GPU off (3) will
trigger failed to command pgc, because the last pgc(vpu power on)
not finished.

I think this could be not avoided if we split the handshake into
blk-ctl driver. How do you think?

BTW: #linux-imx IRC moved to Libre.Chat     

> 
> Hi Peng,
> 
> Am Dienstag, dem 29.06.2021 um 15:29 +0800 schrieb Peng Fan (OSS):
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
> > some GPRs.
> >
> > The GPRs has some clock bits and reset bits, but here we take it as
> > virtual PDs, because of the clock and power domain A/B lock issue when
> > taking it as a clock controller.
> >
> > For some bits, it might be good to also make it as a reset controller,
> > but to i.MX8MM, we not add that support for now.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/soc/imx/Makefile  |   2 +-
> >  drivers/soc/imx/blk-ctl.c | 324
> > ++++++++++++++++++++++++++++++++++++++
> >  drivers/soc/imx/blk-ctl.h |  85 ++++++++++
> >  3 files changed, 410 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/soc/imx/blk-ctl.c  create mode 100644
> > drivers/soc/imx/blk-ctl.h
> >
> > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index
> > 078dc918f4f3..d3d2b49a386c 100644
> > --- a/drivers/soc/imx/Makefile
> > +++ b/drivers/soc/imx/Makefile
> > @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o  endif
> >  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> >  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> > -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> > +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> > diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c new
> > file mode 100644 index 000000000000..cec1884202e0
> > --- /dev/null
> > +++ b/drivers/soc/imx/blk-ctl.c
> > @@ -0,0 +1,324 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2021 NXP.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#include "blk-ctl.h"
> > +
> > +static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct
> > +generic_pm_domain *genpd) {
> > +	return container_of(genpd, struct imx_blk_ctl_domain, genpd); }
> > +
> > +static int imx_blk_ctl_enable_hsk(struct device *dev) {
> > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > +	const struct imx_blk_ctl_hw *hw = blk_ctl->dev_data->hw_hsk;
> > +	struct regmap *regmap = blk_ctl->regmap;
> > +	int ret;
> > +
> > +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask,
> hw->rst_mask);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> > +
> > +	/* Wait for handshake */
> > +	udelay(5);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx_blk_ctl_power_on(struct generic_pm_domain *domain) {
> > +	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
> > +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> > +	struct regmap *regmap = blk_ctl->regmap;
> > +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->pds[pd->id];
> > +	int ret;
> > +
> > +	mutex_lock(&blk_ctl->lock);
> > +
> > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> 
> I'm still not a fan of enabling all the clocks going into the blk-ctl to power
> up/down one specific domain. It's not really a problem with clocks, where the
> parents are always on, as the clock gate/ungate is pretty cheap, but as soon
> as you get to something like the display pixel clock, where the parent PLL may
> be shut down, the clock enable may easily be the most costly operation of this
> whole function, even if this specific clock isn't even needed for the domain in
> question.

We had an agreement to use bulk clks in the beginning :)
But I could look into use dedicated clk, but that requires new logic to map each
pd with needed clks.

> 
> > +	if (ret) {
> > +		mutex_unlock(&blk_ctl->lock);
> > +		return ret;
> > +	}
> > +
> > +	if (hw->flags & IMX_BLK_CTL_PD_HANDSHAKE) {
> > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > +		if (ret)
> > +			dev_err(blk_ctl->dev, "Handshake failed when power on\n");
> > +
> > +		/* Expected, handshake already handle reset*/
> > +		goto disable_clk;
> > +	}
> > +
> > +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > +		ret = regmap_clear_bits(regmap, hw->rst_offset, hw->rst_mask);
> > +		if (ret)
> > +			goto disable_clk;
> > +
> > +		/* Wait for reset propagate */
> > +		udelay(5);
> > +
> > +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask,
> hw->rst_mask);
> > +		if (ret)
> > +			goto disable_clk;
> > +	}
> > +
> > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> >
> 
> I find this very hard to follow and reason about. Why do we even need those
> different paths for domains with or without the handshake?
> 
> Shouldn't it be enough to just be enough to do the following in all
> cases:
> 1. release sft reset
> 2. enable sft clock
> 3. wait a little for reset to propagate or ADB to ack power up

Reset flow is: assert reset, deassert reset, enable clk
Handshake flow: deassert reset, enable clk.

Per my previous test, use reset flow for handshake seems
not work. I could recall clearly.

> 
> > +disable_clk:
> > +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> > +
> > +	mutex_unlock(&blk_ctl->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx_blk_ctl_power_off(struct generic_pm_domain *domain) {
> > +	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
> > +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> > +	struct regmap *regmap = blk_ctl->regmap;
> > +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->pds[pd->id];
> > +	int ret = 0;
> > +
> > +	mutex_lock(&blk_ctl->lock);
> > +
> > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> > +	if (ret) {
> > +		mutex_unlock(&blk_ctl->lock);
> > +		return ret;
> > +	}
> > +
> > +	if (!(hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)) {
> > +		ret = regmap_clear_bits(regmap, hw->offset, hw->mask);
> > +		if (ret)
> > +			goto disable_clk;
> > +
> > +		if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > +			ret = regmap_clear_bits(regmap, hw->rst_offset,
> hw->rst_mask);
> > +			if (ret)
> > +				goto disable_clk;
> > +		}
> > +	} else {
> > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> 
> Why would we need to enable the handshake again in the power DOWN
> path?
> The clock/reset bits should still be set from the power up. The power down
> should probably just be a no-op for the blk-ctl bus domains, to allow the
> proper ADB handshake in the PGC domain power-down.

I exported VPU-H1 as VPU-BUS for handshake, because they share
same bits.  But you raise a valid idea, I think I could drop VPU-BUS,
then just no-op here.

BTW: bus should be enabled when power down others.

> 
> > +		if (ret)
> > +			dev_err(blk_ctl->dev, "Handshake failed when power off\n");
> > +	}
> > +
> > +disable_clk:
> > +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> > +
> > +	mutex_unlock(&blk_ctl->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx_blk_ctl_probe(struct platform_device *pdev) {
> > +	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
> > +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> > +	struct generic_pm_domain *parent_genpd;
> > +	struct device *dev = &pdev->dev;
> > +	struct device *active_pd;
> > +	int ret;
> > +
> > +	pdev->dev.of_node = blk_ctl->dev->of_node;
> > +
> > +	if (domain->hw->active_pd_name) {
> > +		active_pd = dev_pm_domain_attach_by_name(dev,
> domain->hw->active_pd_name);
> > +		if (IS_ERR_OR_NULL(active_pd)) {
> > +			ret = PTR_ERR(active_pd) ? : -ENODATA;
> > +			pdev->dev.of_node = NULL;
> 
> This is extremely ugly. I think we should not even have separate platform
> drivers for the blk-ctl domains, there is just no reason for it. See below for
> more comments in that direction.
> 
> > +			return ret;
> > +		}
> > +
> > +		domain->active_pd = active_pd;
> > +	} else {
> > +		if (!blk_ctl->bus_domain) {
> > +			pdev->dev.of_node = NULL;
> > +			return -EPROBE_DEFER;
> > +		}
> > +	}
> > +
> > +	if (domain->hw->active_pd_name)
> > +		parent_genpd = pd_to_genpd(active_pd->pm_domain);
> > +	else
> > +		parent_genpd = blk_ctl->bus_domain;
> > +
> > +	if (pm_genpd_add_subdomain(parent_genpd, &domain->genpd)) {
> > +		dev_warn(dev, "failed to add subdomain: %s\n",
> domain->genpd.name);
> 
> I don't see where the dispmix_bus domain and clock is kept enabled. I would
> guess that the bus domain should be some kind of parent to the lcdif and mipi
> domains,

Yes. vpumix-blk-ctl bus domain is parent to vpu-g1/g2 and similar to others.

 as I don't think it would be okay to disable the bus clock, while any
> of the peripherals in the dispmix complex are still active. Am I missing
> something here?

You are right, bus clock should be always kept enable if any periphals in
dispmix is alive.

> 
> > +	} else {
> > +		mutex_lock(&blk_ctl->lock);
> > +		domain->hooked = true;
> > +		mutex_unlock(&blk_ctl->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_blk_ctl_remove(struct platform_device *pdev) {
> > +	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
> > +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> > +	struct generic_pm_domain *parent_genpd;
> > +	struct device *active_pd;
> > +
> > +	if (domain->hw->active_pd_name)
> > +		parent_genpd = pd_to_genpd(active_pd->pm_domain);
> 
> This has probably never been tested. active_pd is undefined at this point, so
> will most likely lead to a kernel crash.

My bad. Indeed, remove not tested.

> > +	else
> > +		parent_genpd = blk_ctl->bus_domain;
> > +
> > +	pm_genpd_remove_subdomain(parent_genpd, &domain->genpd);
> > +
> > +	mutex_lock(&blk_ctl->lock);
> > +	domain->hooked = false;
> > +	mutex_unlock(&blk_ctl->lock);
> > +
> > +	if (domain->hw->active_pd_name)
> > +		dev_pm_domain_detach(domain->active_pd, false);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct platform_device_id imx_blk_ctl_id[] = {
> > +	{ "imx-vpumix-blk-ctl", },
> > +	{ "imx-dispmix-blk-ctl", },
> > +	{ },
> > +};
> > +
> > +static struct platform_driver imx_blk_ctl_driver = {
> > +	.driver = {
> > +		.name = "imx-blk-ctl",
> > +	},
> > +	.probe    = imx_blk_ctl_probe,
> > +	.remove   = imx_blk_ctl_remove,
> > +	.id_table = imx_blk_ctl_id,
> > +};
> > +builtin_platform_driver(imx_blk_ctl_driver)
> > +
> > +static struct generic_pm_domain *imx_blk_ctl_genpd_xlate(struct
> of_phandle_args *genpdspec,
> > +							 void *data)
> > +{
> > +	struct genpd_onecell_data *genpd_data = data;
> > +	unsigned int idx = genpdspec->args[0];
> > +	struct imx_blk_ctl_domain *domain;
> > +	struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
> > +
> > +	if (genpdspec->args_count != 1)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (idx >= genpd_data->num_domains)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (!genpd_data->domains[idx])
> > +		return ERR_PTR(-ENOENT);
> > +
> > +	domain = to_imx_blk_ctl_pd(genpd_data->domains[idx]);
> > +
> > +	mutex_lock(&domain->blk_ctl->lock);
> > +	if (domain->hooked)
> > +		genpd = genpd_data->domains[idx];
> > +	mutex_unlock(&domain->blk_ctl->lock);
> > +
> > +	return genpd;
> > +}
> > +
> > +int imx_blk_ctl_register(struct device *dev) {
> > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> > +	int num = dev_data->pds_num;
> > +	struct imx_blk_ctl_domain *domain;
> > +	struct generic_pm_domain *genpd;
> > +	struct platform_device *pd_pdev;
> > +	int domain_index;
> > +	int i, ret;
> > +
> > +	blk_ctl->onecell_data.num_domains = num;
> > +	blk_ctl->onecell_data.xlate = imx_blk_ctl_genpd_xlate;
> > +	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num, sizeof(struct
> generic_pm_domain *),
> > +						     GFP_KERNEL);
> > +	if (!blk_ctl->onecell_data.domains)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		domain_index = dev_data->pds[i].id;
> > +		if (domain_index >= num) {
> > +			dev_warn(dev, "Domain index %d is out of bounds\n",
> domain_index);
> > +			continue;
> > +		}
> > +
> > +		domain = devm_kzalloc(dev, sizeof(struct imx_blk_ctl_domain),
> GFP_KERNEL);
> > +		if (!domain)
> > +			goto error;
> > +
> > +		pd_pdev = platform_device_alloc(dev_data->name, domain_index);
> > +		if (!pd_pdev) {
> > +			dev_err(dev, "Failed to allocate platform device\n");
> > +			goto error;
> > +		}
> 
> We don't need a full blow platform device and a driver for the individual
> domains. The only point where we need the device is to attach the parent
> PGC power domains and for this we only need a device.
> 
> So we could either have a dummy device for this usage in the domain or we
> could even reuse the device in the genpd, which is initialized in
> pm_genpd_init.
> 
> Now while I think about it... genpd_dev_pm_attach_by_name already
> allocates the virtual dummy device. I don't think we need to do anything here
> on our own. Just get rid of the platform device and driver.

ok, let me rethink about it. If you have chance to be on IRC, I could talk
with you later if I come about new implementation.

> 
> > +
> > +		pd_pdev->dev.platform_data = domain;
> > +
> > +		domain->blk_ctl = blk_ctl;
> > +		domain->hw = &dev_data->pds[i];
> > +		domain->id = domain_index;
> > +		domain->genpd.name = dev_data->pds[i].name;
> > +		domain->genpd.power_off = imx_blk_ctl_power_off;
> > +		domain->genpd.power_on = imx_blk_ctl_power_on;
> > +		domain->dev = &pd_pdev->dev;
> > +		domain->hooked = false;
> > +
> > +		ret = pm_genpd_init(&domain->genpd, NULL, true);
> > +		pd_pdev->dev.parent = dev;
> > +
> > +		if (domain->hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)
> > +			blk_ctl->bus_domain = &domain->genpd;
> > +
> > +		ret = platform_device_add(pd_pdev);
> > +		if (ret) {
> > +			platform_device_put(pd_pdev);
> > +			goto error;
> > +		}
> 
> There is really no need for the complexity with the hooked property (the
> mutex around the read/write access still doesn't make it properly thread safe)
> and the blk-ctl domain probe/remove calls. Just handle everything within this
> loop. This would make the driver a whole lot more easy to follow.

I see.

> 
> > +		blk_ctl->onecell_data.domains[i] = &domain->genpd;
> > +	}
> > +
> > +	return of_genpd_add_provider_onecell(dev->of_node,
> > +&blk_ctl->onecell_data);
> > +
> > +error:
> > +	for (; i >= 0; i--) {
> > +		genpd = blk_ctl->onecell_data.domains[i];
> > +		if (!genpd)
> > +			continue;
> > +		domain = to_imx_blk_ctl_pd(genpd);
> > +		if (domain->dev)
> > +			platform_device_put(to_platform_device(domain->dev));
> > +	}
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(imx_blk_ctl_register);
> > +
> > +const struct dev_pm_ops imx_blk_ctl_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +pm_runtime_force_resume) };
> EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);
> 
> This code is linked into the same module as the platform driver using it. So
> there is no need to export those symbols and expose them to the whole
> kernel.

Sure. Fix in next version.

> 
> > diff --git a/drivers/soc/imx/blk-ctl.h b/drivers/soc/imx/blk-ctl.h new
> > file mode 100644 index 000000000000..6780d00ec8c5
> > --- /dev/null
> > +++ b/drivers/soc/imx/blk-ctl.h
> > @@ -0,0 +1,85 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */ #ifndef __SOC_IMX_BLK_CTL_H
> > +#define __SOC_IMX_BLK_CTL_H
> > +
> > +enum imx_blk_ctl_pd_type {
> > +	BLK_CTL_PD,
> > +};
> > +
> > +struct imx_blk_ctl_hw {
> > +	int type;
> 
> Initialized, but unused.
> 
> > +	char *name;
> > +	char *active_pd_name;
> > +	u32 offset;
> > +	u32 mask;
> 
> offset and mask are way too generic names. I had to spend some time reading
> the driver to find out that those are the clock enable bits.
> This should be clear from the naming.
> 
> > +	u32 flags;
> > +	u32 id;
> > +	u32 rst_offset;
> > +	u32 rst_mask;
> > +	u32 errata;
> 
> Unused.
> 
> > +};
> > +
> > +struct imx_blk_ctl_domain {
> > +	struct generic_pm_domain genpd;
> > +	struct device *active_pd;
> > +	struct imx_blk_ctl *blk_ctl;
> > +	struct imx_blk_ctl_hw *hw;
> > +	struct device *dev;
> > +	bool hooked;
> > +	u32 id;
> 
> There are already a lot of pointers between the different structures.
> Why do we need those id properties? You should be able to get to all needed
> information by chasing pointers, instead of  indexing into arrays. Really the
> only point where a id->domain mapping is done should be the xlate function.

Will simplify this.

> 
> > +};
> > +
> > +struct imx_blk_ctl_dev_data {
> > +	struct regmap_config config;
> > +	struct imx_blk_ctl_hw *pds;
> > +	struct imx_blk_ctl_hw *hw_hsk;
> > +	u32 pds_num;
> > +	u32 max_num;
> > +	char *name;
> > +};
> > +
> > +struct imx_blk_ctl {
> > +	struct device *dev;
> > +	struct regmap *regmap;
> > +	struct genpd_onecell_data onecell_data;
> > +	const struct imx_blk_ctl_dev_data *dev_data;
> > +	struct clk_bulk_data *clks;
> > +	u32 num_clks;
> > +	struct generic_pm_domain *bus_domain;
> 
> There should be nothing special about the bus domain at all. Right now this
> permeates through the whole driver that the bus domain is somehow special.
> It should just be a parent domain of the other domains, or kept alive via some
> other appropriate means.

Ok, will try to make it a generic parent domain.

> 
> > +
> > +	struct mutex lock;
> 
> This mutex is only used in the common blk-ctl code, but is initialized in
> imx8mm_blk_ctrl_probe. This seems very inconsistent. I would have expected
> this mutex to be initialized in imx_blk_ctl_register. However, once you get rid
> of the hooked and bus domain magic, this mutex may as well be per-domain,
> at which point I think you don't even need the mutex, as the genpd locking
> should be enough then.

Let me rewrite the code and see how it goes.

Thanks,
Peng.

> 
> > +};
> > +
> > +#define IMX_BLK_CTL(_type, _name, _active_pd, _id, _offset, _mask,
> _rst_offset, _rst_mask,	\
> > +		    _flags, _errata)								\
> > +	{											\
> > +		.type = _type,									\
> > +		.name = _name,									\
> > +		.active_pd_name = _active_pd,							\
> > +		.id = _id,									\
> > +		.offset = _offset,								\
> > +		.mask = _mask,									\
> > +		.flags = _flags,								\
> > +		.rst_offset = _rst_offset,							\
> > +		.rst_mask = _rst_mask,								\
> > +		.errata = _errata,								\
> > +	}
> > +
> > +#define IMX_BLK_CTL_PD(_name, _active_pd, _id, _offset, _mask,
> _rst_offset, _rst_mask, _flags)	\
> > +	IMX_BLK_CTL(BLK_CTL_PD, _name, _active_pd, _id, _offset, _mask,
> _rst_offset,		\
> > +		    _rst_mask, _flags, 0)
> > +
> > +#define IMX_BLK_CTL_PD_ERRATA(_name, _active_pd, _id, _offset, _mask,
> _rst_offset, _rst_mask,	\
> > +			      _flags, _errata)							\
> > +	IMX_BLK_CTL(BLK_CTL_PD, _name, _active_pd, _id, _offset, _mask,
> _rst_offset,		\
> > +		    _rst_mask, _flags, _errata)
> > +
> > +int imx_blk_ctl_register(struct device *dev);
> > +
> > +#define IMX_BLK_CTL_PD_HANDSHAKE	BIT(0)
> > +#define IMX_BLK_CTL_PD_RESET		BIT(1)
> > +#define IMX_BLK_CTL_PD_BUS		BIT(2)
> > +
> > +const extern struct dev_pm_ops imx_blk_ctl_pm_ops;
> > +
> > +#endif
> 

Re: [PATCH V8 0/4] soc: imx: add i.MX BLK-CTL support

[Benjamin Gaignard]

Hi Peng,

I have try to use this series to implement BLK-CTL for IMX8MQ and to test it with G1 and G2 VPUs.
You can find the code here:
https://gitlab.collabora.com/benjamin.gaignard/for-upstream/-/commits/control_block_imx8mq

It seems that IMX8MQ requires to set 'fuses' registers to enable the VPUs, I have added that in your code.

Unfortunately that doesn't work, the VPUs are blocked like if they were never reset.
Note that use syscon to manage the reset is working fine.
Since IMX8MQ control block seems lightly different than for IMX8MM, would you mind to add the support for it in your next series ?

Regards,
Benjamin

Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver

[Lucas Stach]

Hi Peng,

Am Mittwoch, dem 07.07.2021 um 09:56 +0000 schrieb Peng Fan:
> Hi Lucas,
> 
> > Subject: Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver
> 
> After rethinking about this driver, I think we still need
> expose to pgc to touch the blk-ctl hardware bus handshake.
> 
> Currently we are assuming the blk-ctl probe will finish
> the handshake. But there is another case, saying gpu.
> 
> ----------------------------------------------------------------------->
> GPU on(1)
>           VPUMIX on (2)
>                         GPU off(3)
>         
> Between GPU on/off, VPUMIX-BLK-CTL not done, so
> vpumix pgc handshake not done. Then GPU off (3) will
> trigger failed to command pgc, because the last pgc(vpu power on)
> not finished.
> 
> I think this could be not avoided if we split the handshake into
> blk-ctl driver. How do you think?
> 
This is really unfortunate. So the PGC state machine really waits for
the ADB handshake? Until now my understanding was that this isn't
hooked up in the state machine, but sequenced by software through
writing and waiting for the bits in GPC_PU_PWRHSK.

> BTW: #linux-imx IRC moved to Libre.Chat     
> 
> > 
> > Hi Peng,
> > 
> > Am Dienstag, dem 29.06.2021 um 15:29 +0800 schrieb Peng Fan (OSS):
> > > From: Peng Fan <peng.fan@nxp.com>
> > > 
> > > The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
> > > some GPRs.
> > > 
> > > The GPRs has some clock bits and reset bits, but here we take it as
> > > virtual PDs, because of the clock and power domain A/B lock issue when
> > > taking it as a clock controller.
> > > 
> > > For some bits, it might be good to also make it as a reset controller,
> > > but to i.MX8MM, we not add that support for now.
> > > 
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/soc/imx/Makefile  |   2 +-
> > >  drivers/soc/imx/blk-ctl.c | 324
> > > ++++++++++++++++++++++++++++++++++++++
> > >  drivers/soc/imx/blk-ctl.h |  85 ++++++++++
> > >  3 files changed, 410 insertions(+), 1 deletion(-)  create mode 100644
> > > drivers/soc/imx/blk-ctl.c  create mode 100644
> > > drivers/soc/imx/blk-ctl.h
> > > 
> > > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index
> > > 078dc918f4f3..d3d2b49a386c 100644
> > > --- a/drivers/soc/imx/Makefile
> > > +++ b/drivers/soc/imx/Makefile
> > > @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o  endif
> > >  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> > >  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> > > -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> > > +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> > > diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c new
> > > file mode 100644 index 000000000000..cec1884202e0
> > > --- /dev/null
> > > +++ b/drivers/soc/imx/blk-ctl.c
> > > @@ -0,0 +1,324 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2021 NXP.
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/pm_domain.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#include "blk-ctl.h"
> > > +
> > > +static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct
> > > +generic_pm_domain *genpd) {
> > > +	return container_of(genpd, struct imx_blk_ctl_domain, genpd); }
> > > +
> > > +static int imx_blk_ctl_enable_hsk(struct device *dev) {
> > > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > > +	const struct imx_blk_ctl_hw *hw = blk_ctl->dev_data->hw_hsk;
> > > +	struct regmap *regmap = blk_ctl->regmap;
> > > +	int ret;
> > > +
> > > +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask,
> > hw->rst_mask);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> > > +
> > > +	/* Wait for handshake */
> > > +	udelay(5);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int imx_blk_ctl_power_on(struct generic_pm_domain *domain) {
> > > +	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
> > > +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> > > +	struct regmap *regmap = blk_ctl->regmap;
> > > +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->pds[pd->id];
> > > +	int ret;
> > > +
> > > +	mutex_lock(&blk_ctl->lock);
> > > +
> > > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> > 
> > I'm still not a fan of enabling all the clocks going into the blk-ctl to power
> > up/down one specific domain. It's not really a problem with clocks, where the
> > parents are always on, as the clock gate/ungate is pretty cheap, but as soon
> > as you get to something like the display pixel clock, where the parent PLL may
> > be shut down, the clock enable may easily be the most costly operation of this
> > whole function, even if this specific clock isn't even needed for the domain in
> > question.
> 
> We had an agreement to use bulk clks in the beginning :)
> But I could look into use dedicated clk, but that requires new logic to map each
> pd with needed clks.
> 
Yea, I didn't totally reject using bulk clks, but looking at the number
of different clocks going into e.g. the i.MX8MP MEDIAMIX blk-ctl, I
fear that using bulk clocks is actually making the power up/down for
the blk-ctl domains much more costly than it needs to be. I think we
should really think about this now, as using bulk clks vs. individual
clocks for each domain will most likely have an impact on the DT
binding, so will be very hard to change once the driver is accepted
upstream.


> > 
> > > +	if (ret) {
> > > +		mutex_unlock(&blk_ctl->lock);
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (hw->flags & IMX_BLK_CTL_PD_HANDSHAKE) {
> > > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > > +		if (ret)
> > > +			dev_err(blk_ctl->dev, "Handshake failed when power on\n");
> > > +
> > > +		/* Expected, handshake already handle reset*/
> > > +		goto disable_clk;
> > > +	}
> > > +
> > > +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > +		ret = regmap_clear_bits(regmap, hw->rst_offset, hw->rst_mask);
> > > +		if (ret)
> > > +			goto disable_clk;
> > > +
> > > +		/* Wait for reset propagate */
> > > +		udelay(5);
> > > +
> > > +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask,
> > hw->rst_mask);
> > > +		if (ret)
> > > +			goto disable_clk;
> > > +	}
> > > +
> > > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> > > 
> > 
> > I find this very hard to follow and reason about. Why do we even need those
> > different paths for domains with or without the handshake?
> > 
> > Shouldn't it be enough to just be enough to do the following in all
> > cases:
> > 1. release sft reset
> > 2. enable sft clock
> > 3. wait a little for reset to propagate or ADB to ack power up
> 
> Reset flow is: assert reset, deassert reset, enable clk
> Handshake flow: deassert reset, enable clk.
> 
> Per my previous test, use reset flow for handshake seems
> not work. I could recall clearly.
> 
That's surprising to me, given that the blk-ctl resets should be
asserted when we power up the GPC mix domains. I don't dispute your
testing, but I would be very interested to learn what's going on in
hardware that asserting a already asserted reset would break the power-
up flow.

> > 
> > > +disable_clk:
> > > +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> > > +
> > > +	mutex_unlock(&blk_ctl->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int imx_blk_ctl_power_off(struct generic_pm_domain *domain) {
> > > +	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
> > > +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> > > +	struct regmap *regmap = blk_ctl->regmap;
> > > +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->pds[pd->id];
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&blk_ctl->lock);
> > > +
> > > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> > > +	if (ret) {
> > > +		mutex_unlock(&blk_ctl->lock);
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (!(hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)) {
> > > +		ret = regmap_clear_bits(regmap, hw->offset, hw->mask);
> > > +		if (ret)
> > > +			goto disable_clk;
> > > +
> > > +		if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > +			ret = regmap_clear_bits(regmap, hw->rst_offset,
> > hw->rst_mask);
> > > +			if (ret)
> > > +				goto disable_clk;
> > > +		}
> > > +	} else {
> > > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > 
> > Why would we need to enable the handshake again in the power DOWN
> > path?
> > The clock/reset bits should still be set from the power up. The power down
> > should probably just be a no-op for the blk-ctl bus domains, to allow the
> > proper ADB handshake in the PGC domain power-down.
> 
> I exported VPU-H1 as VPU-BUS for handshake, because they share
> same bits.  But you raise a valid idea, I think I could drop VPU-BUS,
> then just no-op here.

By share the same bits, you mean the ADB is connected to the same clock
and reset as H1?

> 
> BTW: bus should be enabled when power down others.

I guess bus should just be always enabled as long as the PGC mix domain
is powered-up, right?

> 
> > 
> > > +		if (ret)
> > > +			dev_err(blk_ctl->dev, "Handshake failed when power off\n");
> > > +	}
> > > +
> > > +disable_clk:
> > > +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> > > +
> > > +	mutex_unlock(&blk_ctl->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int imx_blk_ctl_probe(struct platform_device *pdev) {
> > > +	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
> > > +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> > > +	struct generic_pm_domain *parent_genpd;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct device *active_pd;
> > > +	int ret;
> > > +
> > > +	pdev->dev.of_node = blk_ctl->dev->of_node;
> > > +
> > > +	if (domain->hw->active_pd_name) {
> > > +		active_pd = dev_pm_domain_attach_by_name(dev,
> > domain->hw->active_pd_name);
> > > +		if (IS_ERR_OR_NULL(active_pd)) {
> > > +			ret = PTR_ERR(active_pd) ? : -ENODATA;
> > > +			pdev->dev.of_node = NULL;
> > 
> > This is extremely ugly. I think we should not even have separate platform
> > drivers for the blk-ctl domains, there is just no reason for it. See below for
> > more comments in that direction.
> > 
> > > +			return ret;
> > > +		}
> > > +
> > > +		domain->active_pd = active_pd;
> > > +	} else {
> > > +		if (!blk_ctl->bus_domain) {
> > > +			pdev->dev.of_node = NULL;
> > > +			return -EPROBE_DEFER;
> > > +		}
> > > +	}
> > > +
> > > +	if (domain->hw->active_pd_name)
> > > +		parent_genpd = pd_to_genpd(active_pd->pm_domain);
> > > +	else
> > > +		parent_genpd = blk_ctl->bus_domain;
> > > +
> > > +	if (pm_genpd_add_subdomain(parent_genpd, &domain->genpd)) {
> > > +		dev_warn(dev, "failed to add subdomain: %s\n",
> > domain->genpd.name);
> > 
> > I don't see where the dispmix_bus domain and clock is kept enabled. I would
> > guess that the bus domain should be some kind of parent to the lcdif and mipi
> > domains,
> 
> Yes. vpumix-blk-ctl bus domain is parent to vpu-g1/g2 and similar to others.
> 
>  as I don't think it would be okay to disable the bus clock, while any
> > of the peripherals in the dispmix complex are still active. Am I missing
> > something here?
> 
> You are right, bus clock should be always kept enable if any periphals in
> dispmix is alive.
> 
> > 
> > > +	} else {
> > > +		mutex_lock(&blk_ctl->lock);
> > > +		domain->hooked = true;
> > > +		mutex_unlock(&blk_ctl->lock);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int imx_blk_ctl_remove(struct platform_device *pdev) {
> > > +	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
> > > +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> > > +	struct generic_pm_domain *parent_genpd;
> > > +	struct device *active_pd;
> > > +
> > > +	if (domain->hw->active_pd_name)
> > > +		parent_genpd = pd_to_genpd(active_pd->pm_domain);
> > 
> > This has probably never been tested. active_pd is undefined at this point, so
> > will most likely lead to a kernel crash.
> 
> My bad. Indeed, remove not tested.
> 
> > > +	else
> > > +		parent_genpd = blk_ctl->bus_domain;
> > > +
> > > +	pm_genpd_remove_subdomain(parent_genpd, &domain->genpd);
> > > +
> > > +	mutex_lock(&blk_ctl->lock);
> > > +	domain->hooked = false;
> > > +	mutex_unlock(&blk_ctl->lock);
> > > +
> > > +	if (domain->hw->active_pd_name)
> > > +		dev_pm_domain_detach(domain->active_pd, false);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct platform_device_id imx_blk_ctl_id[] = {
> > > +	{ "imx-vpumix-blk-ctl", },
> > > +	{ "imx-dispmix-blk-ctl", },
> > > +	{ },
> > > +};
> > > +
> > > +static struct platform_driver imx_blk_ctl_driver = {
> > > +	.driver = {
> > > +		.name = "imx-blk-ctl",
> > > +	},
> > > +	.probe    = imx_blk_ctl_probe,
> > > +	.remove   = imx_blk_ctl_remove,
> > > +	.id_table = imx_blk_ctl_id,
> > > +};
> > > +builtin_platform_driver(imx_blk_ctl_driver)
> > > +
> > > +static struct generic_pm_domain *imx_blk_ctl_genpd_xlate(struct
> > of_phandle_args *genpdspec,
> > > +							 void *data)
> > > +{
> > > +	struct genpd_onecell_data *genpd_data = data;
> > > +	unsigned int idx = genpdspec->args[0];
> > > +	struct imx_blk_ctl_domain *domain;
> > > +	struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
> > > +
> > > +	if (genpdspec->args_count != 1)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (idx >= genpd_data->num_domains)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (!genpd_data->domains[idx])
> > > +		return ERR_PTR(-ENOENT);
> > > +
> > > +	domain = to_imx_blk_ctl_pd(genpd_data->domains[idx]);
> > > +
> > > +	mutex_lock(&domain->blk_ctl->lock);
> > > +	if (domain->hooked)
> > > +		genpd = genpd_data->domains[idx];
> > > +	mutex_unlock(&domain->blk_ctl->lock);
> > > +
> > > +	return genpd;
> > > +}
> > > +
> > > +int imx_blk_ctl_register(struct device *dev) {
> > > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > > +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> > > +	int num = dev_data->pds_num;
> > > +	struct imx_blk_ctl_domain *domain;
> > > +	struct generic_pm_domain *genpd;
> > > +	struct platform_device *pd_pdev;
> > > +	int domain_index;
> > > +	int i, ret;
> > > +
> > > +	blk_ctl->onecell_data.num_domains = num;
> > > +	blk_ctl->onecell_data.xlate = imx_blk_ctl_genpd_xlate;
> > > +	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num, sizeof(struct
> > generic_pm_domain *),
> > > +						     GFP_KERNEL);
> > > +	if (!blk_ctl->onecell_data.domains)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0; i < num; i++) {
> > > +		domain_index = dev_data->pds[i].id;
> > > +		if (domain_index >= num) {
> > > +			dev_warn(dev, "Domain index %d is out of bounds\n",
> > domain_index);
> > > +			continue;
> > > +		}
> > > +
> > > +		domain = devm_kzalloc(dev, sizeof(struct imx_blk_ctl_domain),
> > GFP_KERNEL);
> > > +		if (!domain)
> > > +			goto error;
> > > +
> > > +		pd_pdev = platform_device_alloc(dev_data->name, domain_index);
> > > +		if (!pd_pdev) {
> > > +			dev_err(dev, "Failed to allocate platform device\n");
> > > +			goto error;
> > > +		}
> > 
> > We don't need a full blow platform device and a driver for the individual
> > domains. The only point where we need the device is to attach the parent
> > PGC power domains and for this we only need a device.
> > 
> > So we could either have a dummy device for this usage in the domain or we
> > could even reuse the device in the genpd, which is initialized in
> > pm_genpd_init.
> > 
> > Now while I think about it... genpd_dev_pm_attach_by_name already
> > allocates the virtual dummy device. I don't think we need to do anything here
> > on our own. Just get rid of the platform device and driver.
> 
> ok, let me rethink about it. If you have chance to be on IRC, I could talk
> with you later if I come about new implementation.

I'm in the IRC channel if you prefer to talk there.

Regards,
Lucas

RE: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver

[Peng Fan]

> Subject: Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver
> 
> Hi Peng,
> 
> Am Mittwoch, dem 07.07.2021 um 09:56 +0000 schrieb Peng Fan:
> > Hi Lucas,
> >
> > > Subject: Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver
> >
> > After rethinking about this driver, I think we still need expose to
> > pgc to touch the blk-ctl hardware bus handshake.
> >
> > Currently we are assuming the blk-ctl probe will finish the handshake.
> > But there is another case, saying gpu.
> >
> > ----------------------------------------------------------------------->
> > GPU on(1)
> >           VPUMIX on (2)
> >                         GPU off(3)
> >
> > Between GPU on/off, VPUMIX-BLK-CTL not done, so vpumix pgc handshake
> > not done. Then GPU off (3) will trigger failed to command pgc, because
> > the last pgc(vpu power on) not finished.
> >
> > I think this could be not avoided if we split the handshake into
> > blk-ctl driver. How do you think?
> >
> This is really unfortunate. So the PGC state machine really waits for the ADB
> handshake? 

Not very sure, just a guess here. Because of ....., I am not able to find
HW people to give explaination for the messy hardware design.

About your idea to not create a platform device as what gpcv2 driver did,
I rethink about that.
if we split the handshake in blk-ctl driver, we need saying
vpumix-blk-ctl depends on pgc_vpumix
pgc_vpu_g1/g2/h1 depends on vpumix-blk-ctl bus domain.
It is hard to do defer probe if we just use simple parent/child pd tree.

If we move the handshake back to gpcv2 driver, the blk-ctl driver
could be simplier I think, but have to add reference to blk-ctl node
in pgc node.

And rethink about the gpcv2 driver, should we add a mutex lock
in power on/off? I know that genpd has lock, but the lock is per genpd.

Considering GPU powering on, VPU may be powering off, the power
sequence maybe interrupted by the other. I thought this
may cause failed to command PGC, so better add a mutex lock
in pgc power on/off?


Until now my understanding was that this isn't hooked up in the
> state machine, but sequenced by software through writing and waiting for the
> bits in GPC_PU_PWRHSK.
> 
> > BTW: #linux-imx IRC moved to Libre.Chat
> >
> > >
> > > Hi Peng,
> > >
> > > Am Dienstag, dem 29.06.2021 um 15:29 +0800 schrieb Peng Fan (OSS):
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > The i.MX8MM introduces an IP named BLK_CTL and usually is
> > > > comprised of some GPRs.
> > > >
> > > > The GPRs has some clock bits and reset bits, but here we take it
> > > > as virtual PDs, because of the clock and power domain A/B lock
> > > > issue when taking it as a clock controller.
> > > >
> > > > For some bits, it might be good to also make it as a reset
> > > > controller, but to i.MX8MM, we not add that support for now.
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  drivers/soc/imx/Makefile  |   2 +-
> > > >  drivers/soc/imx/blk-ctl.c | 324
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/soc/imx/blk-ctl.h |  85 ++++++++++
> > > >  3 files changed, 410 insertions(+), 1 deletion(-)  create mode
> > > > 100644 drivers/soc/imx/blk-ctl.c  create mode 100644
> > > > drivers/soc/imx/blk-ctl.h
> > > >
> > > > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> > > > index 078dc918f4f3..d3d2b49a386c 100644
> > > > --- a/drivers/soc/imx/Makefile
> > > > +++ b/drivers/soc/imx/Makefile
> > > > @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o  endif
> > > >  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> > > >  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> > > > -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> > > > +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> > > > diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c
> > > > new file mode 100644 index 000000000000..cec1884202e0
> > > > --- /dev/null
> > > > +++ b/drivers/soc/imx/blk-ctl.c
> > > > @@ -0,0 +1,324 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright 2021 NXP.
> > > > + */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/platform_device.h> #include <linux/pm_runtime.h>
> > > > +#include <linux/pm_domain.h> #include <linux/regmap.h> #include
> > > > +<linux/slab.h>
> > > > +
> > > > +#include "blk-ctl.h"
> > > > +
> > > > +static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct
> > > > +generic_pm_domain *genpd) {
> > > > +	return container_of(genpd, struct imx_blk_ctl_domain, genpd); }
> > > > +
> > > > +static int imx_blk_ctl_enable_hsk(struct device *dev) {
> > > > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > > > +	const struct imx_blk_ctl_hw *hw = blk_ctl->dev_data->hw_hsk;
> > > > +	struct regmap *regmap = blk_ctl->regmap;
> > > > +	int ret;
> > > > +
> > > > +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > > +		ret = regmap_update_bits(regmap, hw->rst_offset,
> hw->rst_mask,
> > > hw->rst_mask);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask,
> > > > +hw->mask);
> > > > +
> > > > +	/* Wait for handshake */
> > > > +	udelay(5);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int imx_blk_ctl_power_on(struct generic_pm_domain *domain) {
> > > > +	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
> > > > +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> > > > +	struct regmap *regmap = blk_ctl->regmap;
> > > > +	const struct imx_blk_ctl_hw *hw =
> &blk_ctl->dev_data->pds[pd->id];
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&blk_ctl->lock);
> > > > +
> > > > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> > >
> > > I'm still not a fan of enabling all the clocks going into the
> > > blk-ctl to power up/down one specific domain. It's not really a
> > > problem with clocks, where the parents are always on, as the clock
> > > gate/ungate is pretty cheap, but as soon as you get to something
> > > like the display pixel clock, where the parent PLL may be shut down,
> > > the clock enable may easily be the most costly operation of this
> > > whole function, even if this specific clock isn't even needed for the domain
> in question.
> >
> > We had an agreement to use bulk clks in the beginning :) But I could
> > look into use dedicated clk, but that requires new logic to map each
> > pd with needed clks.
> >
> Yea, I didn't totally reject using bulk clks, but looking at the number of
> different clocks going into e.g. the i.MX8MP MEDIAMIX blk-ctl, I fear that
> using bulk clocks is actually making the power up/down for the blk-ctl
> domains much more costly than it needs to be. I think we should really think
> about this now, as using bulk clks vs. individual clocks for each domain will
> most likely have an impact on the DT binding, so will be very hard to change
> once the driver is accepted upstream.

ok, I see.

> 
> 
> > >
> > > > +	if (ret) {
> > > > +		mutex_unlock(&blk_ctl->lock);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (hw->flags & IMX_BLK_CTL_PD_HANDSHAKE) {
> > > > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > > > +		if (ret)
> > > > +			dev_err(blk_ctl->dev, "Handshake failed when power
> on\n");
> > > > +
> > > > +		/* Expected, handshake already handle reset*/
> > > > +		goto disable_clk;
> > > > +	}
> > > > +
> > > > +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > > +		ret = regmap_clear_bits(regmap, hw->rst_offset,
> hw->rst_mask);
> > > > +		if (ret)
> > > > +			goto disable_clk;
> > > > +
> > > > +		/* Wait for reset propagate */
> > > > +		udelay(5);
> > > > +
> > > > +		ret = regmap_update_bits(regmap, hw->rst_offset,
> hw->rst_mask,
> > > hw->rst_mask);
> > > > +		if (ret)
> > > > +			goto disable_clk;
> > > > +	}
> > > > +
> > > > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask,
> > > > +hw->mask);
> > > >
> > >
> > > I find this very hard to follow and reason about. Why do we even
> > > need those different paths for domains with or without the handshake?
> > >
> > > Shouldn't it be enough to just be enough to do the following in all
> > > cases:
> > > 1. release sft reset
> > > 2. enable sft clock
> > > 3. wait a little for reset to propagate or ADB to ack power up
> >
> > Reset flow is: assert reset, deassert reset, enable clk Handshake
> > flow: deassert reset, enable clk.
> >
> > Per my previous test, use reset flow for handshake seems not work. I
> > could recall clearly.
> >
> That's surprising to me, given that the blk-ctl resets should be asserted when
> we power up the GPC mix domains. I don't dispute your testing, but I would
> be very interested to learn what's going on in hardware that asserting a
> already asserted reset would break the power- up flow.

I could give a re-try, considering the code has changed a lot from the beginning.

You could also give a look at Marex's reset early patch, it maybe caused by
the blk-ctl patch or not, but he confirms after add reset early, gpu/vpu
not met failed to command PGC anymore.
> 
> > >
> > > > +disable_clk:
> > > > +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> > > > +
> > > > +	mutex_unlock(&blk_ctl->lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int imx_blk_ctl_power_off(struct generic_pm_domain *domain) {
> > > > +	struct imx_blk_ctl_domain *pd = to_imx_blk_ctl_pd(domain);
> > > > +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> > > > +	struct regmap *regmap = blk_ctl->regmap;
> > > > +	const struct imx_blk_ctl_hw *hw =
> &blk_ctl->dev_data->pds[pd->id];
> > > > +	int ret = 0;
> > > > +
> > > > +	mutex_lock(&blk_ctl->lock);
> > > > +
> > > > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> > > > +	if (ret) {
> > > > +		mutex_unlock(&blk_ctl->lock);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (!(hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)) {
> > > > +		ret = regmap_clear_bits(regmap, hw->offset, hw->mask);
> > > > +		if (ret)
> > > > +			goto disable_clk;
> > > > +
> > > > +		if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > > +			ret = regmap_clear_bits(regmap, hw->rst_offset,
> > > hw->rst_mask);
> > > > +			if (ret)
> > > > +				goto disable_clk;
> > > > +		}
> > > > +	} else {
> > > > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > >
> > > Why would we need to enable the handshake again in the power DOWN
> > > path?
> > > The clock/reset bits should still be set from the power up. The
> > > power down should probably just be a no-op for the blk-ctl bus
> > > domains, to allow the proper ADB handshake in the PGC domain
> power-down.
> >
> > I exported VPU-H1 as VPU-BUS for handshake, because they share same
> > bits.  But you raise a valid idea, I think I could drop VPU-BUS, then
> > just no-op here.
> 
> By share the same bits, you mean the ADB is connected to the same clock and
> reset as H1?

G1/G2/H1 all these three bits could be used for ADB handshake, I just use H1 here.


> 
> >
> > BTW: bus should be enabled when power down others.
> 
> I guess bus should just be always enabled as long as the PGC mix domain is
> powered-up, right?

Yes. correct.

> 
> >
> > >
> > > > +		if (ret)
> > > > +			dev_err(blk_ctl->dev, "Handshake failed when power
> off\n");
> > > > +	}
> > > > +
> > > > +disable_clk:
> > > > +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> > > > +
> > > > +	mutex_unlock(&blk_ctl->lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int imx_blk_ctl_probe(struct platform_device *pdev) {
> > > > +	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
> > > > +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> > > > +	struct generic_pm_domain *parent_genpd;
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct device *active_pd;
> > > > +	int ret;
> > > > +
> > > > +	pdev->dev.of_node = blk_ctl->dev->of_node;
> > > > +
> > > > +	if (domain->hw->active_pd_name) {
> > > > +		active_pd = dev_pm_domain_attach_by_name(dev,
> > > domain->hw->active_pd_name);
> > > > +		if (IS_ERR_OR_NULL(active_pd)) {
> > > > +			ret = PTR_ERR(active_pd) ? : -ENODATA;
> > > > +			pdev->dev.of_node = NULL;
> > >
> > > This is extremely ugly. I think we should not even have separate
> > > platform drivers for the blk-ctl domains, there is just no reason
> > > for it. See below for more comments in that direction.
> > >
> > > > +			return ret;
> > > > +		}
> > > > +
> > > > +		domain->active_pd = active_pd;
> > > > +	} else {
> > > > +		if (!blk_ctl->bus_domain) {
> > > > +			pdev->dev.of_node = NULL;
> > > > +			return -EPROBE_DEFER;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (domain->hw->active_pd_name)
> > > > +		parent_genpd = pd_to_genpd(active_pd->pm_domain);
> > > > +	else
> > > > +		parent_genpd = blk_ctl->bus_domain;
> > > > +
> > > > +	if (pm_genpd_add_subdomain(parent_genpd, &domain->genpd)) {
> > > > +		dev_warn(dev, "failed to add subdomain: %s\n",
> > > domain->genpd.name);
> > >
> > > I don't see where the dispmix_bus domain and clock is kept enabled.
> > > I would guess that the bus domain should be some kind of parent to
> > > the lcdif and mipi domains,
> >
> > Yes. vpumix-blk-ctl bus domain is parent to vpu-g1/g2 and similar to others.
> >
> >  as I don't think it would be okay to disable the bus clock, while any
> > > of the peripherals in the dispmix complex are still active. Am I
> > > missing something here?
> >
> > You are right, bus clock should be always kept enable if any periphals
> > in dispmix is alive.
> >
> > >
> > > > +	} else {
> > > > +		mutex_lock(&blk_ctl->lock);
> > > > +		domain->hooked = true;
> > > > +		mutex_unlock(&blk_ctl->lock);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int imx_blk_ctl_remove(struct platform_device *pdev) {
> > > > +	struct imx_blk_ctl_domain *domain = pdev->dev.platform_data;
> > > > +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> > > > +	struct generic_pm_domain *parent_genpd;
> > > > +	struct device *active_pd;
> > > > +
> > > > +	if (domain->hw->active_pd_name)
> > > > +		parent_genpd = pd_to_genpd(active_pd->pm_domain);
> > >
> > > This has probably never been tested. active_pd is undefined at this
> > > point, so will most likely lead to a kernel crash.
> >
> > My bad. Indeed, remove not tested.
> >
> > > > +	else
> > > > +		parent_genpd = blk_ctl->bus_domain;
> > > > +
> > > > +	pm_genpd_remove_subdomain(parent_genpd, &domain->genpd);
> > > > +
> > > > +	mutex_lock(&blk_ctl->lock);
> > > > +	domain->hooked = false;
> > > > +	mutex_unlock(&blk_ctl->lock);
> > > > +
> > > > +	if (domain->hw->active_pd_name)
> > > > +		dev_pm_domain_detach(domain->active_pd, false);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct platform_device_id imx_blk_ctl_id[] = {
> > > > +	{ "imx-vpumix-blk-ctl", },
> > > > +	{ "imx-dispmix-blk-ctl", },
> > > > +	{ },
> > > > +};
> > > > +
> > > > +static struct platform_driver imx_blk_ctl_driver = {
> > > > +	.driver = {
> > > > +		.name = "imx-blk-ctl",
> > > > +	},
> > > > +	.probe    = imx_blk_ctl_probe,
> > > > +	.remove   = imx_blk_ctl_remove,
> > > > +	.id_table = imx_blk_ctl_id,
> > > > +};
> > > > +builtin_platform_driver(imx_blk_ctl_driver)
> > > > +
> > > > +static struct generic_pm_domain *imx_blk_ctl_genpd_xlate(struct
> > > of_phandle_args *genpdspec,
> > > > +							 void *data)
> > > > +{
> > > > +	struct genpd_onecell_data *genpd_data = data;
> > > > +	unsigned int idx = genpdspec->args[0];
> > > > +	struct imx_blk_ctl_domain *domain;
> > > > +	struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
> > > > +
> > > > +	if (genpdspec->args_count != 1)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	if (idx >= genpd_data->num_domains)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	if (!genpd_data->domains[idx])
> > > > +		return ERR_PTR(-ENOENT);
> > > > +
> > > > +	domain = to_imx_blk_ctl_pd(genpd_data->domains[idx]);
> > > > +
> > > > +	mutex_lock(&domain->blk_ctl->lock);
> > > > +	if (domain->hooked)
> > > > +		genpd = genpd_data->domains[idx];
> > > > +	mutex_unlock(&domain->blk_ctl->lock);
> > > > +
> > > > +	return genpd;
> > > > +}
> > > > +
> > > > +int imx_blk_ctl_register(struct device *dev) {
> > > > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > > > +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> > > > +	int num = dev_data->pds_num;
> > > > +	struct imx_blk_ctl_domain *domain;
> > > > +	struct generic_pm_domain *genpd;
> > > > +	struct platform_device *pd_pdev;
> > > > +	int domain_index;
> > > > +	int i, ret;
> > > > +
> > > > +	blk_ctl->onecell_data.num_domains = num;
> > > > +	blk_ctl->onecell_data.xlate = imx_blk_ctl_genpd_xlate;
> > > > +	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num,
> > > > +sizeof(struct
> > > generic_pm_domain *),
> > > > +						     GFP_KERNEL);
> > > > +	if (!blk_ctl->onecell_data.domains)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	for (i = 0; i < num; i++) {
> > > > +		domain_index = dev_data->pds[i].id;
> > > > +		if (domain_index >= num) {
> > > > +			dev_warn(dev, "Domain index %d is out of bounds\n",
> > > domain_index);
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		domain = devm_kzalloc(dev, sizeof(struct imx_blk_ctl_domain),
> > > GFP_KERNEL);
> > > > +		if (!domain)
> > > > +			goto error;
> > > > +
> > > > +		pd_pdev = platform_device_alloc(dev_data->name,
> domain_index);
> > > > +		if (!pd_pdev) {
> > > > +			dev_err(dev, "Failed to allocate platform device\n");
> > > > +			goto error;
> > > > +		}
> > >
> > > We don't need a full blow platform device and a driver for the
> > > individual domains. The only point where we need the device is to
> > > attach the parent PGC power domains and for this we only need a device.
> > >
> > > So we could either have a dummy device for this usage in the domain
> > > or we could even reuse the device in the genpd, which is initialized
> > > in pm_genpd_init.
> > >
> > > Now while I think about it... genpd_dev_pm_attach_by_name already
> > > allocates the virtual dummy device. I don't think we need to do
> > > anything here on our own. Just get rid of the platform device and driver.
> >
> > ok, let me rethink about it. If you have chance to be on IRC, I could
> > talk with you later if I come about new implementation.
> 
> I'm in the IRC channel if you prefer to talk there.

BTW, do you have time to pick up the BLK-CTL part? I think you have
better architecture view than me.

Thanks,
Peng.

> 
> Regards,
> Lucas

Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver

[Lucas Stach]

Am Freitag, dem 09.07.2021 um 10:08 +0000 schrieb Peng Fan:
> > Subject: Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl driver
> > 
> > Hi Peng,
> > 
> > Am Mittwoch, dem 07.07.2021 um 09:56 +0000 schrieb Peng Fan:
> > > Hi Lucas,
> > > 
> > > > Subject: Re: [PATCH V8 3/4] soc: imx: Add generic blk-ctl
> > > > driver
> > > 
> > > After rethinking about this driver, I think we still need expose
> > > to
> > > pgc to touch the blk-ctl hardware bus handshake.
> > > 
> > > Currently we are assuming the blk-ctl probe will finish the
> > > handshake.
> > > But there is another case, saying gpu.
> > > 
> > > -----------------------------------------------------------------
> > > ------>
> > > GPU on(1)
> > >           VPUMIX on (2)
> > >                         GPU off(3)
> > > 
> > > Between GPU on/off, VPUMIX-BLK-CTL not done, so vpumix pgc
> > > handshake
> > > not done. Then GPU off (3) will trigger failed to command pgc,
> > > because
> > > the last pgc(vpu power on) not finished.
> > > 
> > > I think this could be not avoided if we split the handshake into
> > > blk-ctl driver. How do you think?
> > > 
> > This is really unfortunate. So the PGC state machine really waits
> > for the ADB
> > handshake? 
> 
> Not very sure, just a guess here. Because of ....., I am not able to
> find
> HW people to give explaination for the messy hardware design.
> 
It's somewhat disillusioning to hear that not even you are able to find
people able to give a proper explanation on how those things fit
together. :/

> About your idea to not create a platform device as what gpcv2 driver
> did,
> I rethink about that.
> if we split the handshake in blk-ctl driver, we need saying
> vpumix-blk-ctl depends on pgc_vpumix
> pgc_vpu_g1/g2/h1 depends on vpumix-blk-ctl bus domain.
> It is hard to do defer probe if we just use simple parent/child pd
> tree.
> 
> If we move the handshake back to gpcv2 driver, the blk-ctl driver
> could be simplier I think, but have to add reference to blk-ctl node
> in pgc node.

I'll spend some time and see if I can come up with a proper abstraction
here. This will probably take a little while, as I don't have a good
plan yet, so it will involve some typing of prototype driver code to
see how things work out.

> 
> And rethink about the gpcv2 driver, should we add a mutex lock
> in power on/off? I know that genpd has lock, but the lock is per
> genpd.
> 
> Considering GPU powering on, VPU may be powering off, the power
> sequence maybe interrupted by the other. I thought this
> may cause failed to command PGC, so better add a mutex lock
> in pgc power on/off?
> 
I don't think sequences for different domains are interacting with each
other, at least that's my understanding of the PGC hardware. The
example code 5 in the i.MX8MM reference manual at least hints at it
being totally legal to request multiple domains to power-up at the same
time and also there are no waits for sequence completion between
request to power-down different domains.

> 
> Until now my understanding was that this isn't hooked up in the
> > state machine, but sequenced by software through writing and
> > waiting for the
> > bits in GPC_PU_PWRHSK.
> > 
> > > BTW: #linux-imx IRC moved to Libre.Chat
> > > 
> > > > 
> > > > Hi Peng,
> > > > 
> > > > Am Dienstag, dem 29.06.2021 um 15:29 +0800 schrieb Peng Fan
> > > > (OSS):
> > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > > 
> > > > > The i.MX8MM introduces an IP named BLK_CTL and usually is
> > > > > comprised of some GPRs.
> > > > > 
> > > > > The GPRs has some clock bits and reset bits, but here we take
> > > > > it
> > > > > as virtual PDs, because of the clock and power domain A/B
> > > > > lock
> > > > > issue when taking it as a clock controller.
> > > > > 
> > > > > For some bits, it might be good to also make it as a reset
> > > > > controller, but to i.MX8MM, we not add that support for now.
> > > > > 
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---
> > > > >  drivers/soc/imx/Makefile  |   2 +-
> > > > >  drivers/soc/imx/blk-ctl.c | 324
> > > > > ++++++++++++++++++++++++++++++++++++++
> > > > >  drivers/soc/imx/blk-ctl.h |  85 ++++++++++
> > > > >  3 files changed, 410 insertions(+), 1 deletion(-)  create
> > > > > mode
> > > > > 100644 drivers/soc/imx/blk-ctl.c  create mode 100644
> > > > > drivers/soc/imx/blk-ctl.h
> > > > > 
> > > > > diff --git a/drivers/soc/imx/Makefile
> > > > > b/drivers/soc/imx/Makefile
> > > > > index 078dc918f4f3..d3d2b49a386c 100644
> > > > > --- a/drivers/soc/imx/Makefile
> > > > > +++ b/drivers/soc/imx/Makefile
> > > > > @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o  endif
> > > > >  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> > > > >  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> > > > > -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> > > > > +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> > > > > diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-
> > > > > ctl.c
> > > > > new file mode 100644 index 000000000000..cec1884202e0
> > > > > --- /dev/null
> > > > > +++ b/drivers/soc/imx/blk-ctl.c
> > > > > @@ -0,0 +1,324 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright 2021 NXP.
> > > > > + */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/err.h>
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/mutex.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/of_address.h>
> > > > > +#include <linux/of_device.h>
> > > > > +#include <linux/platform_device.h> #include
> > > > > <linux/pm_runtime.h>
> > > > > +#include <linux/pm_domain.h> #include <linux/regmap.h>
> > > > > #include
> > > > > +<linux/slab.h>
> > > > > +
> > > > > +#include "blk-ctl.h"
> > > > > +
> > > > > +static inline struct imx_blk_ctl_domain
> > > > > *to_imx_blk_ctl_pd(struct
> > > > > +generic_pm_domain *genpd) {
> > > > > +	return container_of(genpd, struct
> > > > > imx_blk_ctl_domain, genpd); }
> > > > > +
> > > > > +static int imx_blk_ctl_enable_hsk(struct device *dev) {
> > > > > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > > > > +	const struct imx_blk_ctl_hw *hw = blk_ctl->dev_data-
> > > > > >hw_hsk;
> > > > > +	struct regmap *regmap = blk_ctl->regmap;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > > > +		ret = regmap_update_bits(regmap, hw-
> > > > > >rst_offset,
> > hw->rst_mask,
> > > > hw->rst_mask);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	ret = regmap_update_bits(regmap, hw->offset, hw-
> > > > > >mask,
> > > > > +hw->mask);
> > > > > +
> > > > > +	/* Wait for handshake */
> > > > > +	udelay(5);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static int imx_blk_ctl_power_on(struct generic_pm_domain
> > > > > *domain) {
> > > > > +	struct imx_blk_ctl_domain *pd =
> > > > > to_imx_blk_ctl_pd(domain);
> > > > > +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> > > > > +	struct regmap *regmap = blk_ctl->regmap;
> > > > > +	const struct imx_blk_ctl_hw *hw =
> > &blk_ctl->dev_data->pds[pd->id];
> > > > > +	int ret;
> > > > > +
> > > > > +	mutex_lock(&blk_ctl->lock);
> > > > > +
> > > > > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks,
> > > > > blk_ctl->clks);
> > > > 
> > > > I'm still not a fan of enabling all the clocks going into the
> > > > blk-ctl to power up/down one specific domain. It's not really a
> > > > problem with clocks, where the parents are always on, as the
> > > > clock
> > > > gate/ungate is pretty cheap, but as soon as you get to
> > > > something
> > > > like the display pixel clock, where the parent PLL may be shut
> > > > down,
> > > > the clock enable may easily be the most costly operation of
> > > > this
> > > > whole function, even if this specific clock isn't even needed
> > > > for the domain
> > in question.
> > > 
> > > We had an agreement to use bulk clks in the beginning :) But I
> > > could
> > > look into use dedicated clk, but that requires new logic to map
> > > each
> > > pd with needed clks.
> > > 
> > Yea, I didn't totally reject using bulk clks, but looking at the
> > number of
> > different clocks going into e.g. the i.MX8MP MEDIAMIX blk-ctl, I
> > fear that
> > using bulk clocks is actually making the power up/down for the blk-
> > ctl
> > domains much more costly than it needs to be. I think we should
> > really think
> > about this now, as using bulk clks vs. individual clocks for each
> > domain will
> > most likely have an impact on the DT binding, so will be very hard
> > to change
> > once the driver is accepted upstream.
> 
> ok, I see.
> 
> > 
> > 
> > > > 
> > > > > +	if (ret) {
> > > > > +		mutex_unlock(&blk_ctl->lock);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	if (hw->flags & IMX_BLK_CTL_PD_HANDSHAKE) {
> > > > > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > > > > +		if (ret)
> > > > > +			dev_err(blk_ctl->dev, "Handshake
> > > > > failed when power
> > on\n");
> > > > > +
> > > > > +		/* Expected, handshake already handle
> > > > > reset*/
> > > > > +		goto disable_clk;
> > > > > +	}
> > > > > +
> > > > > +	if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > > > +		ret = regmap_clear_bits(regmap, hw-
> > > > > >rst_offset,
> > hw->rst_mask);
> > > > > +		if (ret)
> > > > > +			goto disable_clk;
> > > > > +
> > > > > +		/* Wait for reset propagate */
> > > > > +		udelay(5);
> > > > > +
> > > > > +		ret = regmap_update_bits(regmap, hw-
> > > > > >rst_offset,
> > hw->rst_mask,
> > > > hw->rst_mask);
> > > > > +		if (ret)
> > > > > +			goto disable_clk;
> > > > > +	}
> > > > > +
> > > > > +	ret = regmap_update_bits(regmap, hw->offset, hw-
> > > > > >mask,
> > > > > +hw->mask);
> > > > > 
> > > > 
> > > > I find this very hard to follow and reason about. Why do we
> > > > even
> > > > need those different paths for domains with or without the
> > > > handshake?
> > > > 
> > > > Shouldn't it be enough to just be enough to do the following in
> > > > all
> > > > cases:
> > > > 1. release sft reset
> > > > 2. enable sft clock
> > > > 3. wait a little for reset to propagate or ADB to ack power up
> > > 
> > > Reset flow is: assert reset, deassert reset, enable clk Handshake
> > > flow: deassert reset, enable clk.
> > > 
> > > Per my previous test, use reset flow for handshake seems not
> > > work. I
> > > could recall clearly.
> > > 
> > That's surprising to me, given that the blk-ctl resets should be
> > asserted when
> > we power up the GPC mix domains. I don't dispute your testing, but
> > I would
> > be very interested to learn what's going on in hardware that
> > asserting a
> > already asserted reset would break the power- up flow.
> 
> I could give a re-try, considering the code has changed a lot from
> the beginning.
> 
> You could also give a look at Marex's reset early patch, it maybe
> caused by
> the blk-ctl patch or not, but he confirms after add reset early,
> gpu/vpu
> not met failed to command PGC anymore.

I'll take a look at this. I'm still a bit puzzled as to why some of the
domains require a late, as in after the power-up request, reset.
Normally I would expect the reset to be level triggered, so it
shouldn't make a difference if the reset is asserted before or after
the power-up request.

> > 
> > > > 
> > > > > +disable_clk:
> > > > > +	clk_bulk_disable_unprepare(blk_ctl->num_clks,
> > > > > blk_ctl->clks);
> > > > > +
> > > > > +	mutex_unlock(&blk_ctl->lock);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static int imx_blk_ctl_power_off(struct generic_pm_domain
> > > > > *domain) {
> > > > > +	struct imx_blk_ctl_domain *pd =
> > > > > to_imx_blk_ctl_pd(domain);
> > > > > +	struct imx_blk_ctl *blk_ctl = pd->blk_ctl;
> > > > > +	struct regmap *regmap = blk_ctl->regmap;
> > > > > +	const struct imx_blk_ctl_hw *hw =
> > &blk_ctl->dev_data->pds[pd->id];
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	mutex_lock(&blk_ctl->lock);
> > > > > +
> > > > > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks,
> > > > > blk_ctl->clks);
> > > > > +	if (ret) {
> > > > > +		mutex_unlock(&blk_ctl->lock);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	if (!(hw->flags & IMX_BLK_CTL_PD_HANDSHAKE)) {
> > > > > +		ret = regmap_clear_bits(regmap, hw->offset,
> > > > > hw->mask);
> > > > > +		if (ret)
> > > > > +			goto disable_clk;
> > > > > +
> > > > > +		if (hw->flags & IMX_BLK_CTL_PD_RESET) {
> > > > > +			ret = regmap_clear_bits(regmap, hw-
> > > > > >rst_offset,
> > > > hw->rst_mask);
> > > > > +			if (ret)
> > > > > +				goto disable_clk;
> > > > > +		}
> > > > > +	} else {
> > > > > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > > > 
> > > > Why would we need to enable the handshake again in the power
> > > > DOWN
> > > > path?
> > > > The clock/reset bits should still be set from the power up. The
> > > > power down should probably just be a no-op for the blk-ctl bus
> > > > domains, to allow the proper ADB handshake in the PGC domain
> > power-down.
> > > 
> > > I exported VPU-H1 as VPU-BUS for handshake, because they share
> > > same
> > > bits.  But you raise a valid idea, I think I could drop VPU-BUS,
> > > then
> > > just no-op here.
> > 
> > By share the same bits, you mean the ADB is connected to the same
> > clock and
> > reset as H1?
> 
> G1/G2/H1 all these three bits could be used for ADB handshake, I just
> use H1 here.
> 
> 
> > 
> > > 
> > > BTW: bus should be enabled when power down others.
> > 
> > I guess bus should just be always enabled as long as the PGC mix
> > domain is
> > powered-up, right?
> 
> Yes. correct.
> 
> > 
> > > 
> > > > 
> > > > > +		if (ret)
> > > > > +			dev_err(blk_ctl->dev, "Handshake
> > > > > failed when power
> > off\n");
> > > > > +	}
> > > > > +
> > > > > +disable_clk:
> > > > > +	clk_bulk_disable_unprepare(blk_ctl->num_clks,
> > > > > blk_ctl->clks);
> > > > > +
> > > > > +	mutex_unlock(&blk_ctl->lock);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static int imx_blk_ctl_probe(struct platform_device *pdev) {
> > > > > +	struct imx_blk_ctl_domain *domain = pdev-
> > > > > >dev.platform_data;
> > > > > +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> > > > > +	struct generic_pm_domain *parent_genpd;
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct device *active_pd;
> > > > > +	int ret;
> > > > > +
> > > > > +	pdev->dev.of_node = blk_ctl->dev->of_node;
> > > > > +
> > > > > +	if (domain->hw->active_pd_name) {
> > > > > +		active_pd =
> > > > > dev_pm_domain_attach_by_name(dev,
> > > > domain->hw->active_pd_name);
> > > > > +		if (IS_ERR_OR_NULL(active_pd)) {
> > > > > +			ret = PTR_ERR(active_pd) ? : -
> > > > > ENODATA;
> > > > > +			pdev->dev.of_node = NULL;
> > > > 
> > > > This is extremely ugly. I think we should not even have
> > > > separate
> > > > platform drivers for the blk-ctl domains, there is just no
> > > > reason
> > > > for it. See below for more comments in that direction.
> > > > 
> > > > > +			return ret;
> > > > > +		}
> > > > > +
> > > > > +		domain->active_pd = active_pd;
> > > > > +	} else {
> > > > > +		if (!blk_ctl->bus_domain) {
> > > > > +			pdev->dev.of_node = NULL;
> > > > > +			return -EPROBE_DEFER;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	if (domain->hw->active_pd_name)
> > > > > +		parent_genpd = pd_to_genpd(active_pd-
> > > > > >pm_domain);
> > > > > +	else
> > > > > +		parent_genpd = blk_ctl->bus_domain;
> > > > > +
> > > > > +	if (pm_genpd_add_subdomain(parent_genpd, &domain-
> > > > > >genpd)) {
> > > > > +		dev_warn(dev, "failed to add subdomain:
> > > > > %s\n",
> > > > domain->genpd.name);
> > > > 
> > > > I don't see where the dispmix_bus domain and clock is kept
> > > > enabled.
> > > > I would guess that the bus domain should be some kind of parent
> > > > to
> > > > the lcdif and mipi domains,
> > > 
> > > Yes. vpumix-blk-ctl bus domain is parent to vpu-g1/g2 and similar
> > > to others.
> > > 
> > >  as I don't think it would be okay to disable the bus clock,
> > > while any
> > > > of the peripherals in the dispmix complex are still active. Am
> > > > I
> > > > missing something here?
> > > 
> > > You are right, bus clock should be always kept enable if any
> > > periphals
> > > in dispmix is alive.
> > > 
> > > > 
> > > > > +	} else {
> > > > > +		mutex_lock(&blk_ctl->lock);
> > > > > +		domain->hooked = true;
> > > > > +		mutex_unlock(&blk_ctl->lock);
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int imx_blk_ctl_remove(struct platform_device *pdev)
> > > > > {
> > > > > +	struct imx_blk_ctl_domain *domain = pdev-
> > > > > >dev.platform_data;
> > > > > +	struct imx_blk_ctl *blk_ctl = domain->blk_ctl;
> > > > > +	struct generic_pm_domain *parent_genpd;
> > > > > +	struct device *active_pd;
> > > > > +
> > > > > +	if (domain->hw->active_pd_name)
> > > > > +		parent_genpd = pd_to_genpd(active_pd-
> > > > > >pm_domain);
> > > > 
> > > > This has probably never been tested. active_pd is undefined at
> > > > this
> > > > point, so will most likely lead to a kernel crash.
> > > 
> > > My bad. Indeed, remove not tested.
> > > 
> > > > > +	else
> > > > > +		parent_genpd = blk_ctl->bus_domain;
> > > > > +
> > > > > +	pm_genpd_remove_subdomain(parent_genpd, &domain-
> > > > > >genpd);
> > > > > +
> > > > > +	mutex_lock(&blk_ctl->lock);
> > > > > +	domain->hooked = false;
> > > > > +	mutex_unlock(&blk_ctl->lock);
> > > > > +
> > > > > +	if (domain->hw->active_pd_name)
> > > > > +		dev_pm_domain_detach(domain->active_pd,
> > > > > false);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct platform_device_id imx_blk_ctl_id[] = {
> > > > > +	{ "imx-vpumix-blk-ctl", },
> > > > > +	{ "imx-dispmix-blk-ctl", },
> > > > > +	{ },
> > > > > +};
> > > > > +
> > > > > +static struct platform_driver imx_blk_ctl_driver = {
> > > > > +	.driver = {
> > > > > +		.name = "imx-blk-ctl",
> > > > > +	},
> > > > > +	.probe    = imx_blk_ctl_probe,
> > > > > +	.remove   = imx_blk_ctl_remove,
> > > > > +	.id_table = imx_blk_ctl_id,
> > > > > +};
> > > > > +builtin_platform_driver(imx_blk_ctl_driver)
> > > > > +
> > > > > +static struct generic_pm_domain
> > > > > *imx_blk_ctl_genpd_xlate(struct
> > > > of_phandle_args *genpdspec,
> > > > > +							
> > > > > void *data)
> > > > > +{
> > > > > +	struct genpd_onecell_data *genpd_data = data;
> > > > > +	unsigned int idx = genpdspec->args[0];
> > > > > +	struct imx_blk_ctl_domain *domain;
> > > > > +	struct generic_pm_domain *genpd = ERR_PTR(-
> > > > > EPROBE_DEFER);
> > > > > +
> > > > > +	if (genpdspec->args_count != 1)
> > > > > +		return ERR_PTR(-EINVAL);
> > > > > +
> > > > > +	if (idx >= genpd_data->num_domains)
> > > > > +		return ERR_PTR(-EINVAL);
> > > > > +
> > > > > +	if (!genpd_data->domains[idx])
> > > > > +		return ERR_PTR(-ENOENT);
> > > > > +
> > > > > +	domain = to_imx_blk_ctl_pd(genpd_data-
> > > > > >domains[idx]);
> > > > > +
> > > > > +	mutex_lock(&domain->blk_ctl->lock);
> > > > > +	if (domain->hooked)
> > > > > +		genpd = genpd_data->domains[idx];
> > > > > +	mutex_unlock(&domain->blk_ctl->lock);
> > > > > +
> > > > > +	return genpd;
> > > > > +}
> > > > > +
> > > > > +int imx_blk_ctl_register(struct device *dev) {
> > > > > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > > > > +	const struct imx_blk_ctl_dev_data *dev_data =
> > > > > blk_ctl->dev_data;
> > > > > +	int num = dev_data->pds_num;
> > > > > +	struct imx_blk_ctl_domain *domain;
> > > > > +	struct generic_pm_domain *genpd;
> > > > > +	struct platform_device *pd_pdev;
> > > > > +	int domain_index;
> > > > > +	int i, ret;
> > > > > +
> > > > > +	blk_ctl->onecell_data.num_domains = num;
> > > > > +	blk_ctl->onecell_data.xlate =
> > > > > imx_blk_ctl_genpd_xlate;
> > > > > +	blk_ctl->onecell_data.domains = devm_kcalloc(dev,
> > > > > num,
> > > > > +sizeof(struct
> > > > generic_pm_domain *),
> > > > > +						    
> > > > > GFP_KERNEL);
> > > > > +	if (!blk_ctl->onecell_data.domains)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	for (i = 0; i < num; i++) {
> > > > > +		domain_index = dev_data->pds[i].id;
> > > > > +		if (domain_index >= num) {
> > > > > +			dev_warn(dev, "Domain index %d is
> > > > > out of bounds\n",
> > > > domain_index);
> > > > > +			continue;
> > > > > +		}
> > > > > +
> > > > > +		domain = devm_kzalloc(dev, sizeof(struct
> > > > > imx_blk_ctl_domain),
> > > > GFP_KERNEL);
> > > > > +		if (!domain)
> > > > > +			goto error;
> > > > > +
> > > > > +		pd_pdev = platform_device_alloc(dev_data-
> > > > > >name,
> > domain_index);
> > > > > +		if (!pd_pdev) {
> > > > > +			dev_err(dev, "Failed to allocate
> > > > > platform device\n");
> > > > > +			goto error;
> > > > > +		}
> > > > 
> > > > We don't need a full blow platform device and a driver for the
> > > > individual domains. The only point where we need the device is
> > > > to
> > > > attach the parent PGC power domains and for this we only need a
> > > > device.
> > > > 
> > > > So we could either have a dummy device for this usage in the
> > > > domain
> > > > or we could even reuse the device in the genpd, which is
> > > > initialized
> > > > in pm_genpd_init.
> > > > 
> > > > Now while I think about it... genpd_dev_pm_attach_by_name
> > > > already
> > > > allocates the virtual dummy device. I don't think we need to do
> > > > anything here on our own. Just get rid of the platform device
> > > > and driver.
> > > 
> > > ok, let me rethink about it. If you have chance to be on IRC, I
> > > could
> > > talk with you later if I come about new implementation.
> > 
> > I'm in the IRC channel if you prefer to talk there.
> 
> BTW, do you have time to pick up the BLK-CTL part? I think you have
> better architecture view than me.

Yes, this whole power-domain/blk-ctl complex is blocking a lot of other
things I'm supposed to look into. I'll spend some time trying to sort
this out.

Regards,
Lucas