[PATCH v6 00/12] Add imx8mp video support

[PATCH v6 00/12] Add imx8mp video support

[Miquel Raynal]

In order to display a boot picture or an error message, the i.MX8MP
display pipeline must be enabled. The SoC has support for various
interfaces (LVDS, HDMI, DSI). The one supported in this series is the
standard 4-lane LVDS output. The minimal setup is thus composed of:
* An LCD InterFace (LCDIF) with an AXI/APB interface, generating a pixel
  stream
* One LVDS Display Bridge (LDB), also named pixel mapper, which receives
  the pixel stream and route it to one or two (possibly combined) LVDS
  displays.
* All necessary clocks and power controls coming from the MEDIAMIX
  control block.

Patch 3 adds a very useful helper to the core in order to grab devices
through endpoints instead of being limited to phandles. Video pipelines
being often described using graphs endpoints, the introduced helper is
used several times in the serires (there are 3 LCDIF, one of them being
connected to the LDB, itself having 2 ports).

Patch 5 is a fix which is necessary for this series to work properly,
but is way broader than just this use case. In practice, when assigned
clocks are defined in the device tree, the clock uclass tries to assign
the parents first and then sets them to the correct frequency. This only
works if the parents have been enabled themselves. Otherwise we end-up
with a non-clocked parent. I believe this is not the intended behavior
in general, but more importantly on the i.MX8MP, there are "clock
slices" which have pre-requisites in order to be modified and selecting
an ungated parent is one of them.

All the other patches progressively build support for the whole video
pipeline. Regarding the LCDIF driver, there is already a similar driver
for older i.MX SoCs but I didn't manage to get it to work. It was
written more than a decade ago while device-model, clocks and others
were not yet generically supported. Thus, numerous ad-hoc solutions
were implemented, which no longer fit today's requirements. I preferred
to add a new "clean" driver instead of modifying the existing one
because of the too high risk of breaking these platforms. Once proper
clocks/power-domain descriptions will be added to them they might be
converted (and tested) to work with the "new" implementation, but going
the opposite way felt drawback.

---
Changes in v6:
- Another rebase on next.
- Fixed the clock 24M oscillator clock name due to recent changes.
- Fixed a subject prefix.
- Collected tags.
- Fixed the CI by adding two missing configuration items to secondary
  sandbox defconfigs.
- Link to v5: https://lore.kernel.org/r/20250326-ge-mainline-display-support-v5-0-ea4bc3fc9bca@bootlin.com

Changes in v5:
- Rebased on latest next. Fixed the conflicts.
- Used Svyatoslav's ofgraph code to rewrite the core uclass function.
- Fixed the tests, again.
- Added a small fix in a comment from Svyatoslav's series.
- Rebased on latest next because there were more incompatible
  changes. Fixed the conflicts in the clock driver.
- Link to v4: https://lore.kernel.org/r/20250129-ge-mainline-display-support-v4-0-a43b3657ed7c@bootlin.com

Changes in v4:
- Created a set of low-level helpers below power_domain_on/offoff() as
  asked by Simon.
- The EVK change has been dropped, as we cannot test it.
- Link to v3: https://lore.kernel.org/r/20250110-ge-mainline-display-support-v3-0-d2c0acea6feb@bootlin.com

Changes in v3:
- Fix a clock name in the imx8mp clock driver (mismatch between Linux
  and U-Boot).
- Fixed the Azure tests with extra changes in the test files
  and sandbox configurations:
  https://dev.azure.com/u-boot/u-boot/_build/results?buildId=10296&view=results
- Link to v2: https://lore.kernel.org/r/20241205-ge-mainline-display-support-v2-0-4683bb4388dc@bootlin.com

Changes in v2:
- Add ISP clocks to avoid harmless errors when shutting down the
  pipeline.
- Add power domain refcounting to avoid shutting down a power domain
  already disabled. This is useful when a device points several times to
  the same power domain in the DT.
- Ensure the mediamix driver frees all the power domains it got a
  reference on.
- Add an in-tree user as requested by Fabio: the EVK.
- Use the same Kconfig prompts as in Linux.
- Add sandbox tests for uclass_get_device_by_endpoint().
- Enclosed media clocks definitions within #if CONFIG_IS_ENABLED()
  guards to avoid bloating the SPL and configurations without video
  support, as Adam and Fabio advised.
- Disable the enabled clocks in the remove path of the mediamix block
  driver.
- Fix many typos.
- Fix checkpatch warnings.
- Rebase on next.
- Collect tags
- Link to v1: https://lore.kernel.org/r/20240910101344.110633-1-miquel.raynal@bootlin.com

---
Miquel Raynal (12):
      core: ofnode_graph: Fix a comment
      dm: doc: Fix example
      dm: core: Add a helper to retrieve devices through graph endpoints
      test: dm: test-fdt: Add checks for uclass_get_device_by_endpoint()
      power-domain: Add refcounting
      clk: Ensure the parent clocks are enabled while reparenting
      clk: imx8mp: Add media related clocks
      imx: power-domain: Describe the i.MX8 MEDIAMIX domain
      imx: power-domain: Add support for the MEDIAMIX control block
      video: imx: Fix Makefile in order to be able to add other imx drivers
      video: imx: Add LDB driver
      video: imx: Add LCDIF driver

 configs/sandbox64_defconfig                      |   2 +
 configs/sandbox_flattree_defconfig               |   2 +
 doc/develop/driver-model/design.rst              |   2 +-
 drivers/clk/clk-uclass.c                         |  21 +-
 drivers/clk/imx/clk-imx8mp.c                     |  69 +++++
 drivers/core/ofnode_graph.c                      |   2 +-
 drivers/core/uclass.c                            |  19 ++
 drivers/firmware/scmi/sandbox-scmi_devices.c     |   1 +
 drivers/power/domain/Kconfig                     |   7 +
 drivers/power/domain/Makefile                    |   1 +
 drivers/power/domain/imx8m-power-domain.c        |  17 ++
 drivers/power/domain/imx8mp-mediamix.c           | 208 +++++++++++++++
 drivers/power/domain/power-domain-uclass.c       |  40 ++-
 drivers/power/domain/sandbox-power-domain-test.c |   1 +
 drivers/video/Makefile                           |   2 +-
 drivers/video/imx/Kconfig                        |   9 +
 drivers/video/imx/Makefile                       |   4 +-
 drivers/video/imx/lcdif.c                        | 314 +++++++++++++++++++++++
 drivers/video/imx/ldb.c                          | 251 ++++++++++++++++++
 include/dm/uclass.h                              |  24 ++
 include/power-domain.h                           |  60 ++++-
 test/dm/power-domain.c                           |   2 +-
 test/dm/test-fdt.c                               |  20 +-
 23 files changed, 1056 insertions(+), 22 deletions(-)
---
base-commit: 3703298e57c8d749b3c06798f5873562754fb84c
change-id: 20241205-ge-mainline-display-support-00cf60fbd2cc

Best regards,
-- 
Miquel Raynal <miquel.raynal@bootlin.com>

[PATCH v6 01/12] core: ofnode_graph: Fix a comment

[Miquel Raynal]

Naming between the parameter list, the prototype and the main comment do
not match. Fix the comment which seems the be the one that is incorrect.

Fixes: 9057077cf4e1 ("core: ofnode: add of_graph parsing helpers")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/core/ofnode_graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/core/ofnode_graph.c b/drivers/core/ofnode_graph.c
index 90c92af3258ce73209e19c0d0ee6a8f35ef4e26a..175ac7687710f23245345346a8694ebdaf161c7a 100644
--- a/drivers/core/ofnode_graph.c
+++ b/drivers/core/ofnode_graph.c
@@ -98,7 +98,7 @@ ofnode ofnode_graph_get_port_by_id(ofnode parent, u32 id)
  * @id: id for the endpoint
  *
  * Return: ofnode in given endpoint or ofnode_null() if not found.
- * reg and port_reg are ignored when they are -1.
+ * reg_id and id are ignored when they are -1.
  */
 ofnode ofnode_graph_get_endpoint_by_regs(ofnode parent, int reg_id, int id)
 {

-- 
2.48.1

[PATCH v6 02/12] dm: doc: Fix example

[Miquel Raynal]

`.priv_data_size` does not exist. I believe the actual structure member
was supposed to be `.priv_auto`.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 doc/develop/driver-model/design.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/develop/driver-model/design.rst b/doc/develop/driver-model/design.rst
index 30093737200ccc5d34a0c713eac0a6cccc0829ae..633545944d1584fdd40736eaeec54c997b60ed67 100644
--- a/doc/develop/driver-model/design.rst
+++ b/doc/develop/driver-model/design.rst
@@ -312,7 +312,7 @@ drivers/demo/demo-shape.c):
 		.name	= "demo_shape_drv",
 		.id	= UCLASS_DEMO,
 		.ops	= &shape_ops,
-		.priv_data_size = sizeof(struct shape_data),
+		.priv_auto = sizeof(struct shape_data),
 	};
 
 

-- 
2.48.1

[PATCH v6 03/12] dm: core: Add a helper to retrieve devices through graph endpoints

[Miquel Raynal]

There are already several helpers to find a udevice based on its
position in a device tree, like getting a child or a node pointed by a
phandle, but there was no support for graph endpoints, which are very
common in display pipelines.

Add a new helper, named uclass_get_device_by_endpoint() which enters the
child graph reprensentation, looks for a specific port, then follows the
remote endpoint, and finally retrieves the first parent of the given
uclass_id.

This is a very handy and straightforward way to get a bridge or a panel
handle.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/core/uclass.c | 19 +++++++++++++++++++
 include/dm/uclass.h   | 24 ++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index f846a35d6b2537bab56bca158cedfb134b37f477..ce5e61bbaa63a181a1ea6781ad6a7f06be67c7e0 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -16,6 +16,7 @@
 #include <dm/device.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
+#include <dm/ofnode_graph.h>
 #include <dm/uclass.h>
 #include <dm/uclass-internal.h>
 #include <dm/util.h>
@@ -582,6 +583,24 @@ int uclass_get_device_by_phandle(enum uclass_id id, struct udevice *parent,
 	ret = uclass_find_device_by_phandle(id, parent, name, &dev);
 	return uclass_get_device_tail(dev, ret, devp);
 }
+
+int uclass_get_device_by_endpoint(enum uclass_id class_id, struct udevice *dev,
+				  int port_idx, int ep_idx, struct udevice **devp)
+{
+	ofnode node_source = dev_ofnode(dev);
+	ofnode node_dest = ofnode_graph_get_remote_node(node_source, port_idx, ep_idx);
+	struct udevice *target = NULL;
+	int ret;
+
+	if (!ofnode_valid(node_dest))
+		return -EINVAL;
+
+	ret = uclass_find_device_by_ofnode(class_id, node_dest, &target);
+	if (ret)
+		return -ENODEV;
+
+	return uclass_get_device_tail(target, 0, devp);
+}
 #endif
 
 /*
diff --git a/include/dm/uclass.h b/include/dm/uclass.h
index c279304092352200d5297ad2d17f527173ec506b..8fdd72725119aae6ad2b62f4b0c38866a910389c 100644
--- a/include/dm/uclass.h
+++ b/include/dm/uclass.h
@@ -333,6 +333,30 @@ int uclass_get_device_by_phandle(enum uclass_id id, struct udevice *parent,
 int uclass_get_device_by_driver(enum uclass_id id, const struct driver *drv,
 				struct udevice **devp);
 
+/**
+ * uclass_get_device_by_endpoint() - Get a uclass device for a remote endpoint
+ *
+ * This searches through the parents of the specified remote endpoint
+ * for the first device matching the uclass. Said otherwise, this helper
+ * goes through the graph (endpoint) representation and searches for
+ * matching devices. Endpoints can be subnodes of the "port" node or
+ * subnodes of ports identified with a reg property, themselves in a
+ * "ports" container.
+ *
+ * The device is probed to activate it ready for use.
+ *
+ * @class_id: uclass ID to look up
+ * @dev: Device to start from
+ * @port_idx: Index of the port to follow, -1 if there is a single 'port'
+ *            node without reg.
+ * @ep_idx: Index of the endpoint to follow, -1 if there is a single 'endpoint'
+ *          node without reg.
+ * @target: Returns pointer to the first device matching the expected uclass.
+ * Return: 0 if OK, -ve on error
+ */
+int uclass_get_device_by_endpoint(enum uclass_id class_id, struct udevice *dev,
+				  int port_idx, int ep_idx, struct udevice **target);
+
 /**
  * uclass_first_device() - Get the first device in a uclass
  *

-- 
2.48.1

[PATCH v6 04/12] test: dm: test-fdt: Add checks for uclass_get_device_by_endpoint()

[Miquel Raynal]

This is a new DM core helper. There is now a graph endpoint
representation in the sandbox test DTS, so we can just use it to verify
the helper proper behavior.

Suggested-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 configs/sandbox64_defconfig        |  2 ++
 configs/sandbox_flattree_defconfig |  2 ++
 test/dm/test-fdt.c                 | 20 +++++++++++++++++++-
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 7960b2ef42e7ceee2a3646888ae159cdde9b59e0..d49adb6adcb55084daf8d6c1a9f8c95d3e4083ac 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -256,6 +256,8 @@ CONFIG_CONSOLE_TRUETYPE=y
 CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y
 CONFIG_I2C_EDID=y
 CONFIG_VIDEO_SANDBOX_SDL=y
+CONFIG_VIDEO_BRIDGE=y
+CONFIG_VIDEO_BRIDGE_LVDS_CODEC=y
 CONFIG_OSD=y
 CONFIG_SANDBOX_OSD=y
 CONFIG_BMP_16BPP=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index 563093dd8a426f98cdcfa342655bebb82dbefe86..67c0ed7353efc252cb31b31d94415f37a1f74c99 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -220,6 +220,8 @@ CONFIG_CONSOLE_TRUETYPE=y
 CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y
 CONFIG_I2C_EDID=y
 CONFIG_VIDEO_SANDBOX_SDL=y
+CONFIG_VIDEO_BRIDGE=y
+CONFIG_VIDEO_BRIDGE_LVDS_CODEC=y
 CONFIG_OSD=y
 CONFIG_SANDBOX_OSD=y
 CONFIG_BMP_16BPP=y
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index af8cd617108ac2421f7275948be000a7cbaebda0..295fdcaa0d83eb6129e9c0aee7a9146b01772d97 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -792,7 +792,7 @@ DM_TEST(dm_test_fdt_disable_enable_by_path, UTF_SCAN_PDATA | UTF_SCAN_FDT);
 /* Test a few uclass phandle functions */
 static int dm_test_fdt_phandle(struct unit_test_state *uts)
 {
-	struct udevice *back, *dev, *dev2;
+	struct udevice *back, *dispc, *panel, *dev, *dev2;
 
 	ut_assertok(uclass_find_first_device(UCLASS_PANEL_BACKLIGHT, &back));
 	ut_assertnonnull(back);
@@ -807,6 +807,24 @@ static int dm_test_fdt_phandle(struct unit_test_state *uts)
 						 "power-supply", &dev2));
 	ut_asserteq_ptr(dev, dev2);
 
+	/* Test uclass_get_device_by_endpoint() */
+	ut_assertok(uclass_find_device_by_name(UCLASS_VIDEO_BRIDGE, "lvds-encoder", &dispc));
+	ut_assertnonnull(dispc);
+	ut_asserteq(0, device_active(dispc));
+	ut_assertok(uclass_find_device_by_name(UCLASS_PANEL, "panel", &panel));
+	ut_assertnonnull(panel);
+	ut_asserteq(0, device_active(panel));
+
+	ut_asserteq(-ENODEV, uclass_get_device_by_endpoint(UCLASS_PANEL_BACKLIGHT, dispc, 1, -1, &dev));
+	ut_asserteq(-EINVAL, uclass_get_device_by_endpoint(UCLASS_PANEL, dispc, 0, -1, &dev));
+	ut_assertok(uclass_get_device_by_endpoint(UCLASS_PANEL, dispc, 1, -1, &dev));
+	ut_asserteq_ptr(panel, dev);
+
+	ut_asserteq(-ENODEV, uclass_get_device_by_endpoint(UCLASS_PANEL_BACKLIGHT, panel, -1, -1, &dev2));
+	ut_asserteq(-EINVAL, uclass_get_device_by_endpoint(UCLASS_VIDEO_BRIDGE, panel, 1, -1, &dev2));
+	ut_assertok(uclass_get_device_by_endpoint(UCLASS_VIDEO_BRIDGE, panel, -1, -1, &dev2));
+	ut_asserteq_ptr(dispc, dev2);
+
 	return 0;
 }
 DM_TEST(dm_test_fdt_phandle, UTF_SCAN_PDATA | UTF_SCAN_FDT);

-- 
2.48.1

[PATCH v6 05/12] power-domain: Add refcounting

[Miquel Raynal]

It is very surprising that such an uclass, specifically designed to
handle resources that may be shared by different devices, is not keeping
the count of the number of times a power domain has been
enabled/disabled to avoid shutting it down unexpectedly or disabling it
several times.

Doing this causes troubles on eg. i.MX8MP because disabling power
domains can be done in recursive loops were the same power domain
disabled up to 4 times in a row. PGCs seem to have tight FSM internal
timings to respect and it is easy to produce a race condition that puts
the power domains in an unstable state, leading to ADB400 errors and
later crashes in Linux.

CI tests using power domains are slightly updated to make sure the count
of on/off calls is even and the results match what we *now* expect.

As we do not want to break existing users while stile getting
interesting error codes, the implementation is split between:
- a low-level helper reporting error codes if the requested transition
  could not be operated,
- a higher-level helper ignoring the "non error" codes, like EALREADY and
  EBUSY.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/firmware/scmi/sandbox-scmi_devices.c     |  1 +
 drivers/power/domain/power-domain-uclass.c       | 40 ++++++++++++++--
 drivers/power/domain/sandbox-power-domain-test.c |  1 +
 include/power-domain.h                           | 60 ++++++++++++++++++++----
 test/dm/power-domain.c                           |  2 +-
 5 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
index 96c2922b067e2886b3fa963bcd7e396f4569a569..9f253b0fd40f703a5ec11d34c197423d27ad8b01 100644
--- a/drivers/firmware/scmi/sandbox-scmi_devices.c
+++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
@@ -163,4 +163,5 @@ U_BOOT_DRIVER(sandbox_scmi_devices) = {
 	.priv_auto = sizeof(struct sandbox_scmi_device_priv),
 	.remove = sandbox_scmi_devices_remove,
 	.probe = sandbox_scmi_devices_probe,
+	.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
 };
diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
index 938bd8cbc9ffd1ba2109d702f886b6a99288d063..a6e5f9ed0369eb9d2dfa66edc9e938bac6720dab 100644
--- a/drivers/power/domain/power-domain-uclass.c
+++ b/drivers/power/domain/power-domain-uclass.c
@@ -12,6 +12,10 @@
 #include <power-domain-uclass.h>
 #include <dm/device-internal.h>
 
+struct power_domain_priv {
+	int on_count;
+};
+
 static inline struct power_domain_ops *power_domain_dev_ops(struct udevice *dev)
 {
 	return (struct power_domain_ops *)dev->driver->ops;
@@ -107,22 +111,49 @@ int power_domain_free(struct power_domain *power_domain)
 	return ops->rfree ? ops->rfree(power_domain) : 0;
 }
 
-int power_domain_on(struct power_domain *power_domain)
+int power_domain_on_lowlevel(struct power_domain *power_domain)
 {
+	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
 	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
+	int ret;
 
 	debug("%s(power_domain=%p)\n", __func__, power_domain);
 
-	return ops->on ? ops->on(power_domain) : 0;
+	if (priv->on_count++ > 0)
+		return -EALREADY;
+
+	ret = ops->on ? ops->on(power_domain) : 0;
+	if (ret) {
+		priv->on_count--;
+		return ret;
+	}
+
+	return 0;
 }
 
-int power_domain_off(struct power_domain *power_domain)
+int power_domain_off_lowlevel(struct power_domain *power_domain)
 {
+	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
 	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
+	int ret;
 
 	debug("%s(power_domain=%p)\n", __func__, power_domain);
 
-	return ops->off ? ops->off(power_domain) : 0;
+	if (priv->on_count <= 0) {
+		debug("Power domain %s already off.\n", power_domain->dev->name);
+		return -EALREADY;
+	}
+
+	if (priv->on_count-- > 1)
+		return -EBUSY;
+
+	ret = ops->off ? ops->off(power_domain) : 0;
+	if (ret) {
+		priv->on_count++;
+		return ret;
+	}
+
+	return 0;
 }
 
 #if CONFIG_IS_ENABLED(OF_REAL)
@@ -180,4 +211,5 @@ int dev_power_domain_off(struct udevice *dev)
 UCLASS_DRIVER(power_domain) = {
 	.id		= UCLASS_POWER_DOMAIN,
 	.name		= "power_domain",
+	.per_device_auto = sizeof(struct power_domain_priv),
 };
diff --git a/drivers/power/domain/sandbox-power-domain-test.c b/drivers/power/domain/sandbox-power-domain-test.c
index 08c15ef342b3dd3ce01807ee59b7e97337f7dde5..5b530974e942ffcba453e53be330baaf3a113a13 100644
--- a/drivers/power/domain/sandbox-power-domain-test.c
+++ b/drivers/power/domain/sandbox-power-domain-test.c
@@ -51,4 +51,5 @@ U_BOOT_DRIVER(sandbox_power_domain_test) = {
 	.id = UCLASS_MISC,
 	.of_match = sandbox_power_domain_test_ids,
 	.priv_auto	= sizeof(struct sandbox_power_domain_test),
+	.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
 };
diff --git a/include/power-domain.h b/include/power-domain.h
index 18525073e5e3534fcbac6fae4e18462f29a4dc49..ad33dea76ce5808beaa4fbf4388438a504e36027 100644
--- a/include/power-domain.h
+++ b/include/power-domain.h
@@ -147,37 +147,81 @@ static inline int power_domain_free(struct power_domain *power_domain)
 #endif
 
 /**
- * power_domain_on - Enable power to a power domain.
+ * power_domain_on_lowlevel - Enable power to a power domain (with refcounting)
  *
  * @power_domain:	A power domain struct that was previously successfully
  *		requested by power_domain_get().
- * Return: 0 if OK, or a negative error code.
+ * Return: 0 if the transition has been performed correctly,
+ *         -EALREADY if the domain is already on,
+ *         a negative error code otherwise.
  */
 #if CONFIG_IS_ENABLED(POWER_DOMAIN)
-int power_domain_on(struct power_domain *power_domain);
+int power_domain_on_lowlevel(struct power_domain *power_domain);
 #else
-static inline int power_domain_on(struct power_domain *power_domain)
+static inline int power_domain_on_lowlevel(struct power_domain *power_domain)
 {
 	return -ENOSYS;
 }
 #endif
 
 /**
- * power_domain_off - Disable power to a power domain.
+ * power_domain_on - Enable power to a power domain (ignores the actual state
+ *		      of the power domain)
  *
  * @power_domain:	A power domain struct that was previously successfully
  *		requested by power_domain_get().
- * Return: 0 if OK, or a negative error code.
+ * Return: a negative error code upon error during the transition, 0 otherwise.
+ */
+static inline int power_domain_on(struct power_domain *power_domain)
+{
+	int ret;
+
+	ret = power_domain_on_lowlevel(power_domain);
+	if (ret == -EALREADY)
+		ret = 0;
+
+	return ret;
+}
+
+/**
+ * power_domain_off_lowlevel - Disable power to a power domain (with refcounting)
+ *
+ * @power_domain:	A power domain struct that was previously successfully
+ *		requested by power_domain_get().
+ * Return: 0 if the transition has been performed correctly,
+ *         -EALREADY if the domain is already off,
+ *         -EBUSY if another device is keeping the domain on (but the refcounter
+ *         is decremented),
+ *         a negative error code otherwise.
  */
 #if CONFIG_IS_ENABLED(POWER_DOMAIN)
-int power_domain_off(struct power_domain *power_domain);
+int power_domain_off_lowlevel(struct power_domain *power_domain);
 #else
-static inline int power_domain_off(struct power_domain *power_domain)
+static inline int power_domain_off_lowlevel(struct power_domain *power_domain)
 {
 	return -ENOSYS;
 }
 #endif
 
+/**
+ * power_domain_off - Disable power to a power domain (ignores the actual state
+ *		      of the power domain)
+ *
+ * @power_domain:	A power domain struct that was previously successfully
+ *		requested by power_domain_get().
+ * Return: a negative error code upon error during the transition, 0 otherwise.
+ */
+static inline int power_domain_off(struct power_domain *power_domain)
+{
+	int ret;
+
+	ret = power_domain_off_lowlevel(power_domain);
+	if (ret == -EALREADY || ret == -EBUSY)
+		ret = 0;
+
+	return ret;
+}
+
 /**
  * dev_power_domain_on - Enable power domains for a device .
  *
diff --git a/test/dm/power-domain.c b/test/dm/power-domain.c
index 896cf5b2ae9d26701150fad70e888f8b135a22b0..8a95f6bdb903be9d1993528d87d5cae0075a83e4 100644
--- a/test/dm/power-domain.c
+++ b/test/dm/power-domain.c
@@ -27,7 +27,7 @@ static int dm_test_power_domain(struct unit_test_state *uts)
 
 	ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "power-domain-test",
 					      &dev_test));
-	ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
+	ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
 						  TEST_POWER_DOMAIN));
 	ut_assertok(sandbox_power_domain_test_get(dev_test));
 

-- 
2.48.1

[PATCH v6 06/12] clk: Ensure the parent clocks are enabled while reparenting

[Miquel Raynal]

Reparenting a clock C with a new parent P means that C will only
continue clocking if P is already clocking when the mux is updated. In
case the parent is currently disabled, failures (stalls) are likely to
happen.

This is exactly what happens on i.MX8 when enabling the video
pipeline. We tell LCDIF clocks to use the VIDEO PLL as input, while the
VIDEO PLL is currently off. This all happens as part of the
assigned-clocks handling procedure, where the reparenting happens before
the enable() calls. Enabling the parents as part of the reparenting
procedure seems sane and also matches the logic applied in other parts
of the CCM.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/clk-uclass.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 90b70529a478d11d907cbd6b64d680d0c1db4d5c..4b3d812f9c6539a6b769990e3e11f14f49abb2a9 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -623,14 +623,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	if (!ops->set_parent)
 		return -ENOSYS;
 
-	ret = ops->set_parent(clk, parent);
-	if (ret)
+	ret = clk_enable(parent);
+	if (ret) {
+		printf("Cannot enable parent %s\n", parent->dev->name);
 		return ret;
+	}
 
-	if (CONFIG_IS_ENABLED(CLK_CCF))
+	ret = ops->set_parent(clk, parent);
+	if (ret) {
+		clk_disable(parent);
+		return ret;
+	}
+
+	if (CONFIG_IS_ENABLED(CLK_CCF)) {
 		ret = device_reparent(clk->dev, parent->dev);
+		if (ret) {
+			clk_disable(parent);
+			return ret;
+		}
+	}
 
-	return ret;
+	return 0;
 }
 
 int clk_enable(struct clk *clk)

-- 
2.48.1

[PATCH v6 07/12] clk: imx8mp: Add media related clocks

[Miquel Raynal]

These are all the clocks needed to get an LCD panel working, going
through one of the LCDIF and the LDB. The media AXI and APB clocks are
also described.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/imx/clk-imx8mp.c | 69 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index bad579f8d5e47fcf078c6ca25e651ddc00fcb30f..3d8ed21d3b91ce861317d234861b9e5de2590e2e 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -14,7 +14,14 @@
 
 #include "clk.h"
 
+#if CONFIG_IS_ENABLED(VIDEO)
+static u32 share_count_media;
+#endif
+
 static const char * const pll_ref_sels[] = { "osc_24m", "dummy", "dummy", "dummy", };
+#if CONFIG_IS_ENABLED(VIDEO)
+static const char * const video_pll1_bypass_sels[] = {"video_pll1", "video_pll1_ref_sel", };
+#endif
 static const char * const dram_pll_bypass_sels[] = {"dram_pll", "dram_pll_ref_sel", };
 static const char * const arm_pll_bypass_sels[] = {"arm_pll", "arm_pll_ref_sel", };
 static const char * const sys_pll1_bypass_sels[] = {"sys_pll1", "sys_pll1_ref_sel", };
@@ -31,6 +38,12 @@ static const char * const imx8mp_hsio_axi_sels[] = {"osc_24m", "sys_pll2_500m",
 						    "sys_pll2_100m", "sys_pll2_200m", "clk_ext2",
 						    "clk_ext4", "audio_pll2_out", };
 
+#if CONFIG_IS_ENABLED(VIDEO)
+static const char * const imx8mp_media_isp_sels[] = {"osc_24m", "sys_pll2_1000m", "sys_pll1_800m",
+						     "sys_pll3_out", "sys_pll1_400m", "audio_pll2_out",
+						     "clk_ext1", "sys_pll2_500m", };
+#endif
+
 static const char * const imx8mp_main_axi_sels[] = {"osc_24m", "sys_pll2_333m", "sys_pll1_800m",
 						    "sys_pll2_250m", "sys_pll2_1000m", "audio_pll1_out",
 						    "video_pll1_out", "sys_pll1_100m",};
@@ -43,6 +56,16 @@ static const char * const imx8mp_nand_usdhc_sels[] = {"osc_24m", "sys_pll1_266m"
 						      "sys_pll2_200m", "sys_pll1_133m", "sys_pll3_out",
 						      "sys_pll2_250m", "audio_pll1_out", };
 
+#if CONFIG_IS_ENABLED(VIDEO)
+static const char * const imx8mp_media_axi_sels[] = {"osc_24m", "sys_pll2_1000m", "sys_pll1_800m",
+						     "sys_pll3_out", "sys_pll1_40m", "audio_pll2_out",
+						     "clk_ext1", "sys_pll2_500m", };
+
+static const char * const imx8mp_media_apb_sels[] = {"osc_24m", "sys_pll2_125m", "sys_pll1_800m",
+					      "sys_pll3_out", "sys_pll1_40m", "audio_pll2_out",
+					      "clk_ext1", "sys_pll1_133m", };
+#endif
+
 static const char * const imx8mp_noc_sels[] = {"osc_24m", "sys_pll1_800m", "sys_pll3_out",
 					       "sys_pll2_1000m", "sys_pll2_500m", "audio_pll1_out",
 					       "video_pll1_out", "audio_pll2_out", };
@@ -175,6 +198,17 @@ static const char * const imx8mp_usdhc3_sels[] = {"osc_24m", "sys_pll1_400m", "s
 						  "sys_pll2_500m", "sys_pll3_out", "sys_pll1_266m",
 						  "audio_pll2_out", "sys_pll1_100m", };
 
+#if CONFIG_IS_ENABLED(VIDEO)
+static const char * const imx8mp_media_disp_pix_sels[] = {"osc_24m", "video_pll1_out", "audio_pll2_out",
+							  "audio_pll1_out", "sys_pll1_800m",
+							  "sys_pll2_1000m", "sys_pll3_out", "clk_ext4", };
+
+static const char * const imx8mp_media_ldb_sels[] = {"osc_24m", "sys_pll2_333m", "sys_pll2_100m",
+						     "sys_pll1_800m", "sys_pll2_1000m",
+						     "clk_ext2", "audio_pll2_out",
+						     "video_pll1_out", };
+#endif
+
 static const char * const imx8mp_enet_ref_sels[] = {"osc_24m", "sys_pll2_125m", "sys_pll2_50m",
 						    "sys_pll2_100m", "sys_pll1_160m", "audio_pll1_out",
 						    "video_pll1_out", "clk_ext4", };
@@ -199,12 +233,19 @@ static int imx8mp_clk_probe(struct udevice *dev)
 
 	clk_dm(IMX8MP_CLK_DUMMY, clk_register_fixed_rate(NULL, "dummy", 0));
 
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_VIDEO_PLL1_REF_SEL, imx_clk_mux(dev, "video_pll1_ref_sel", base + 0x28, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)));
+#endif
 	clk_dm(IMX8MP_DRAM_PLL_REF_SEL, imx_clk_mux(dev, "dram_pll_ref_sel", base + 0x50, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)));
 	clk_dm(IMX8MP_ARM_PLL_REF_SEL, imx_clk_mux(dev, "arm_pll_ref_sel", base + 0x84, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)));
 	clk_dm(IMX8MP_SYS_PLL1_REF_SEL, imx_clk_mux(dev, "sys_pll1_ref_sel", base + 0x94, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)));
 	clk_dm(IMX8MP_SYS_PLL2_REF_SEL, imx_clk_mux(dev, "sys_pll2_ref_sel", base + 0x104, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)));
 	clk_dm(IMX8MP_SYS_PLL3_REF_SEL, imx_clk_mux(dev, "sys_pll3_ref_sel", base + 0x114, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)));
 
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_VIDEO_PLL1, imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28,
+						  &imx_1443x_pll));
+#endif
 	clk_dm(IMX8MP_DRAM_PLL, imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50,
 						&imx_1443x_dram_pll));
 	clk_dm(IMX8MP_ARM_PLL, imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84,
@@ -216,12 +257,18 @@ static int imx8mp_clk_probe(struct udevice *dev)
 	clk_dm(IMX8MP_SYS_PLL3, imx_clk_pll14xx("sys_pll3", "sys_pll3_ref_sel", base + 0x114,
 						&imx_1416x_pll));
 
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_VIDEO_PLL1_BYPASS, imx_clk_mux_flags(dev, "video_pll1_bypass", base + 0x28, 16, 1, video_pll1_bypass_sels, ARRAY_SIZE(video_pll1_bypass_sels), CLK_SET_RATE_PARENT));
+#endif
 	clk_dm(IMX8MP_DRAM_PLL_BYPASS, imx_clk_mux_flags(dev, "dram_pll_bypass", base + 0x50, 4, 1, dram_pll_bypass_sels, ARRAY_SIZE(dram_pll_bypass_sels), CLK_SET_RATE_PARENT));
 	clk_dm(IMX8MP_ARM_PLL_BYPASS, imx_clk_mux_flags(dev, "arm_pll_bypass", base + 0x84, 4, 1, arm_pll_bypass_sels, ARRAY_SIZE(arm_pll_bypass_sels), CLK_SET_RATE_PARENT));
 	clk_dm(IMX8MP_SYS_PLL1_BYPASS, imx_clk_mux_flags(dev, "sys_pll1_bypass", base + 0x94, 4, 1, sys_pll1_bypass_sels, ARRAY_SIZE(sys_pll1_bypass_sels), CLK_SET_RATE_PARENT));
 	clk_dm(IMX8MP_SYS_PLL2_BYPASS, imx_clk_mux_flags(dev, "sys_pll2_bypass", base + 0x104, 4, 1, sys_pll2_bypass_sels, ARRAY_SIZE(sys_pll2_bypass_sels), CLK_SET_RATE_PARENT));
 	clk_dm(IMX8MP_SYS_PLL3_BYPASS, imx_clk_mux_flags(dev, "sys_pll3_bypass", base + 0x114, 4, 1, sys_pll3_bypass_sels, ARRAY_SIZE(sys_pll3_bypass_sels), CLK_SET_RATE_PARENT));
 
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_VIDEO_PLL1_OUT, imx_clk_gate(dev, "video_pll1_out", "video_pll1_bypass", base + 0x28, 13));
+#endif
 	clk_dm(IMX8MP_DRAM_PLL_OUT, imx_clk_gate(dev, "dram_pll_out", "dram_pll_bypass", base + 0x50, 13));
 	clk_dm(IMX8MP_ARM_PLL_OUT, imx_clk_gate(dev, "arm_pll_out", "arm_pll_bypass", base + 0x84, 11));
 	clk_dm(IMX8MP_SYS_PLL1_OUT, imx_clk_gate(dev, "sys_pll1_out", "sys_pll1_bypass", base + 0x94, 11));
@@ -267,13 +314,23 @@ static int imx8mp_clk_probe(struct udevice *dev)
 	clk_dm(IMX8MP_CLK_A53_DIV, imx_clk_divider2(dev, "arm_a53_div", "arm_a53_cg", base + 0x8000, 0, 3));
 
 	clk_dm(IMX8MP_CLK_HSIO_AXI, imx8m_clk_composite(dev, "hsio_axi", imx8mp_hsio_axi_sels, base + 0x8380));
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_CLK_MEDIA_ISP, imx8m_clk_composite(dev, "media_isp", imx8mp_media_isp_sels, base + 0x8400));
+#endif
 	clk_dm(IMX8MP_CLK_MAIN_AXI, imx8m_clk_composite_critical(dev, "main_axi", imx8mp_main_axi_sels, base + 0x8800));
 	clk_dm(IMX8MP_CLK_ENET_AXI, imx8m_clk_composite_critical(dev, "enet_axi", imx8mp_enet_axi_sels, base + 0x8880));
 	clk_dm(IMX8MP_CLK_NAND_USDHC_BUS, imx8m_clk_composite_critical(dev, "nand_usdhc_bus", imx8mp_nand_usdhc_sels, base + 0x8900));
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_CLK_MEDIA_AXI, imx8m_clk_composite(dev, "media_axi", imx8mp_media_axi_sels, base + 0x8a00));
+	clk_dm(IMX8MP_CLK_MEDIA_APB, imx8m_clk_composite(dev, "media_apb", imx8mp_media_apb_sels, base + 0x8a80));
+#endif
 	clk_dm(IMX8MP_CLK_NOC, imx8m_clk_composite_critical(dev, "noc", imx8mp_noc_sels, base + 0x8d00));
 	clk_dm(IMX8MP_CLK_NOC_IO, imx8m_clk_composite_critical(dev, "noc_io", imx8mp_noc_io_sels, base + 0x8d80));
 
 	clk_dm(IMX8MP_CLK_AHB, imx8m_clk_composite_critical(dev, "ahb_root", imx8mp_ahb_sels, base + 0x9000));
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_CLK_MEDIA_DISP2_PIX, imx8m_clk_composite(dev, "media_disp2_pix", imx8mp_media_disp_pix_sels, base + 0x9300));
+#endif
 
 	clk_dm(IMX8MP_CLK_IPG_ROOT, imx_clk_divider2(dev, "ipg_root", "ahb_root", base + 0x9080, 0, 1));
 
@@ -312,6 +369,10 @@ static int imx8mp_clk_probe(struct udevice *dev)
 
 	clk_dm(IMX8MP_CLK_WDOG, imx8m_clk_composite(dev, "wdog", imx8mp_wdog_sels, base + 0xb900));
 	clk_dm(IMX8MP_CLK_USDHC3, imx8m_clk_composite(dev, "usdhc3", imx8mp_usdhc3_sels, base + 0xbc80));
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_CLK_MEDIA_DISP1_PIX, imx8m_clk_composite(dev, "media_disp1_pix", imx8mp_media_disp_pix_sels, base + 0xbe00));
+	clk_dm(IMX8MP_CLK_MEDIA_LDB, imx8m_clk_composite(dev, "media_ldb", imx8mp_media_ldb_sels, base + 0xbf00));
+#endif
 
 	clk_dm(IMX8MP_CLK_DRAM_ALT_ROOT, imx_clk_fixed_factor(dev, "dram_alt_root", "dram_alt", 1, 4));
 	clk_dm(IMX8MP_CLK_DRAM_CORE, imx_clk_mux2_flags(dev, "dram_core_clk", base + 0x9800, 24, 1, imx8mp_dram_core_sels, ARRAY_SIZE(imx8mp_dram_core_sels), CLK_IS_CRITICAL));
@@ -355,6 +416,14 @@ static int imx8mp_clk_probe(struct udevice *dev)
 	clk_dm(IMX8MP_CLK_WDOG2_ROOT, imx_clk_gate4(dev, "wdog2_root_clk", "wdog", base + 0x4540, 0));
 	clk_dm(IMX8MP_CLK_WDOG3_ROOT, imx_clk_gate4(dev, "wdog3_root_clk", "wdog", base + 0x4550, 0));
 	clk_dm(IMX8MP_CLK_HSIO_ROOT, imx_clk_gate4(dev, "hsio_root_clk", "ipg_root", base + 0x45c0, 0));
+#if CONFIG_IS_ENABLED(VIDEO)
+	clk_dm(IMX8MP_CLK_MEDIA_APB_ROOT, imx_clk_gate2_shared2(dev, "media_apb_root_clk", "media_apb", base + 0x45d0, 0, &share_count_media));
+	clk_dm(IMX8MP_CLK_MEDIA_AXI_ROOT, imx_clk_gate2_shared2(dev, "media_axi_root_clk", "media_axi", base + 0x45d0, 0, &share_count_media));
+	clk_dm(IMX8MP_CLK_MEDIA_DISP1_PIX_ROOT, imx_clk_gate2_shared2(dev, "media_disp1_pix_root_clk", "media_disp1_pix", base + 0x45d0, 0, &share_count_media));
+	clk_dm(IMX8MP_CLK_MEDIA_DISP2_PIX_ROOT, imx_clk_gate2_shared2(dev, "media_disp2_pix_root_clk", "media_disp2_pix", base + 0x45d0, 0, &share_count_media));
+	clk_dm(IMX8MP_CLK_MEDIA_LDB_ROOT, imx_clk_gate2_shared2(dev, "media_ldb_root_clk", "media_ldb", base + 0x45d0, 0, &share_count_media));
+	clk_dm(IMX8MP_CLK_MEDIA_ISP_ROOT, imx_clk_gate2_shared2(dev, "media_isp_root_clk", "media_isp", base + 0x45d0, 0, &share_count_media));
+#endif
 
 	clk_dm(IMX8MP_CLK_USDHC3_ROOT, imx_clk_gate4(dev, "usdhc3_root_clk", "usdhc3", base + 0x45e0, 0));
 

-- 
2.48.1

[PATCH v6 08/12] imx: power-domain: Describe the i.MX8 MEDIAMIX domain

[Miquel Raynal]

Add support for the i.MX8 MEDIAMIX domain which is driving the power
over the whole display/rendering pipeline.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/power/domain/imx8m-power-domain.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/power/domain/imx8m-power-domain.c b/drivers/power/domain/imx8m-power-domain.c
index c22fbe60675e96a628f205d10a71b66c7b33ef00..e54ba5d9a5476f678fb48fb16e7217b031acd78a 100644
--- a/drivers/power/domain/imx8m-power-domain.c
+++ b/drivers/power/domain/imx8m-power-domain.c
@@ -40,6 +40,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define IMX8MN_MIPI_A53_DOMAIN			BIT(2)
 
 #define IMX8MP_HSIOMIX_A53_DOMAIN		BIT(19)
+#define IMX8MP_MEDIAMIX_A53_DOMAIN		BIT(12)
 #define IMX8MP_USB2_PHY_A53_DOMAIN		BIT(5)
 #define IMX8MP_USB1_PHY_A53_DOMAIN		BIT(4)
 #define IMX8MP_PCIE_PHY_A53_DOMAIN		BIT(3)
@@ -63,6 +64,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define IMX8MN_MIPI_SW_Pxx_REQ			BIT(0)
 
 #define IMX8MP_HSIOMIX_Pxx_REQ			BIT(17)
+#define IMX8MP_MEDIAMIX_Pxx_REQ                 BIT(10)
 #define IMX8MP_USB2_PHY_Pxx_REQ			BIT(3)
 #define IMX8MP_USB1_PHY_Pxx_REQ			BIT(2)
 #define IMX8MP_PCIE_PHY_SW_Pxx_REQ		BIT(1)
@@ -81,6 +83,9 @@ DECLARE_GLOBAL_DATA_PTR;
 #define IMX8MP_HSIOMIX_PWRDNACKN		BIT(28)
 #define IMX8MP_HSIOMIX_PWRDNREQN		BIT(12)
 
+#define IMX8MP_MEDIAMIX_PWRDNACKN		BIT(30)
+#define IMX8MP_MEDIAMIX_PWRDNREQN		BIT(14)
+
 /*
  * The PGC offset values in Reference Manual
  * (Rev. 1, 01/2018 and the older ones) GPC chapter's
@@ -101,6 +106,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define IMX8MP_PGC_PCIE			13
 #define IMX8MP_PGC_USB1			14
 #define IMX8MP_PGC_USB2			15
+#define IMX8MP_PGC_MEDIAMIX		22
 #define IMX8MP_PGC_HSIOMIX		29
 
 #define GPC_PGC_CTRL(n)			(0x800 + (n) * 0x40)
@@ -303,6 +309,17 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = {
 		.pgc = BIT(IMX8MP_PGC_HSIOMIX),
 		.keep_clocks = true,
 	},
+
+	[IMX8MP_POWER_DOMAIN_MEDIAMIX] = {
+		.bits = {
+			.pxx = IMX8MP_MEDIAMIX_Pxx_REQ,
+			.map = IMX8MP_MEDIAMIX_A53_DOMAIN,
+			.hskreq = IMX8MP_MEDIAMIX_PWRDNREQN,
+			.hskack = IMX8MP_MEDIAMIX_PWRDNACKN,
+		},
+		.pgc = BIT(IMX8MP_PGC_MEDIAMIX),
+		.keep_clocks = true,
+	},
 };
 
 static const struct imx_pgc_regs imx8mp_pgc_regs = {

-- 
2.48.1

[PATCH v6 09/12] imx: power-domain: Add support for the MEDIAMIX control block

[Miquel Raynal]

This block delivers power and clocks to the whole display and rendering
pipeline.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/power/domain/Kconfig           |   7 ++
 drivers/power/domain/Makefile          |   1 +
 drivers/power/domain/imx8mp-mediamix.c | 208 +++++++++++++++++++++++++++++++++
 3 files changed, 216 insertions(+)

diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig
index bd82d2f7044b3c3f8c88dea27c39193efedb4c84..5f5218bd8b5f4bed8283e91da61ac7c8c3eb97ae 100644
--- a/drivers/power/domain/Kconfig
+++ b/drivers/power/domain/Kconfig
@@ -47,6 +47,13 @@ config IMX8MP_HSIOMIX_BLKCTRL
 	help
 	  Enable support for manipulating NXP i.MX8MP on-SoC HSIOMIX block controller.
 
+config IMX8MP_MEDIAMIX_BLKCTRL
+	bool "Enable i.MX8MP MEDIAMIX domain driver"
+	depends on POWER_DOMAIN && IMX8MP
+	select CLK
+	help
+	  Enable support for manipulating NXP i.MX8MP on-SoC MEDIAMIX block controller.
+
 config MTK_POWER_DOMAIN
 	bool "Enable the MediaTek power domain driver"
 	depends on POWER_DOMAIN && ARCH_MEDIATEK
diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile
index 110646c503ab97941a2bb86caf53f94542008219..356ec07163fd299f00e81a629ffc06c663a26c25 100644
--- a/drivers/power/domain/Makefile
+++ b/drivers/power/domain/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o
 obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o
 obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o
 obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o
+obj-$(CONFIG_IMX8MP_MEDIAMIX_BLKCTRL) += imx8mp-mediamix.o
 obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o
 obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o
 obj-$(CONFIG_MESON_EE_POWER_DOMAIN) += meson-ee-pwrc.o
diff --git a/drivers/power/domain/imx8mp-mediamix.c b/drivers/power/domain/imx8mp-mediamix.c
new file mode 100644
index 0000000000000000000000000000000000000000..78c32ca3d3a87febdefd5d128d39d817674b8d32
--- /dev/null
+++ b/drivers/power/domain/imx8mp-mediamix.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * i.MX8 MEDIAMIX control block driver
+ * Copyright (C) 2024 Miquel Raynal <miquel.raynal@bootlin.com>
+ * Inspired from Marek Vasut <marex@denx.de> work on the hsiomix driver.
+ */
+
+#include <asm/io.h>
+#include <clk.h>
+#include <dm.h>
+#include <power-domain-uclass.h>
+#include <linux/delay.h>
+
+#include <dt-bindings/power/imx8mp-power.h>
+
+#define BLK_SFT_RSTN		0x0
+#define BLK_CLK_EN		0x4
+
+struct imx8mp_mediamix_priv {
+	void __iomem *base;
+	struct clk clk_apb;
+	struct clk clk_axi;
+	struct clk clk_disp2;
+	struct power_domain pd_bus;
+	struct power_domain pd_lcdif2;
+};
+
+static int imx8mp_mediamix_on(struct power_domain *power_domain)
+{
+	struct udevice *dev = power_domain->dev;
+	struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
+	struct power_domain *domain;
+	struct clk *clk;
+	u32 reset;
+	int ret;
+
+	switch (power_domain->id) {
+	case IMX8MP_MEDIABLK_PD_LCDIF_2:
+		domain = &priv->pd_lcdif2;
+		clk = &priv->clk_disp2;
+		reset = BIT(11) | BIT(12) | BIT(24);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Make sure bus domain is awake */
+	ret = power_domain_on(&priv->pd_bus);
+	if (ret)
+		return ret;
+
+	/* Put devices into reset */
+	clrbits_le32(priv->base + BLK_SFT_RSTN, reset);
+
+	/* Enable upstream clocks */
+	ret = clk_enable(&priv->clk_apb);
+	if (ret)
+		goto dis_bus_pd;
+
+	ret = clk_enable(&priv->clk_axi);
+	if (ret)
+		goto dis_apb_clk;
+
+	/* Enable blk-ctrl clock to allow reset to propagate */
+	ret = clk_enable(clk);
+	if (ret)
+		goto dis_axi_clk;
+	setbits_le32(priv->base + BLK_CLK_EN, reset);
+
+	/* Power up upstream GPC domain */
+	ret = power_domain_on(domain);
+	if (ret)
+		goto dis_lcdif_clk;
+
+	/* Wait for reset to propagate */
+	udelay(5);
+
+	/* Release reset */
+	setbits_le32(priv->base + BLK_SFT_RSTN, reset);
+
+	return 0;
+
+dis_lcdif_clk:
+	clk_disable(clk);
+dis_axi_clk:
+	clk_disable(&priv->clk_axi);
+dis_apb_clk:
+	clk_disable(&priv->clk_apb);
+dis_bus_pd:
+	power_domain_off(&priv->pd_bus);
+
+	return ret;
+}
+
+static int imx8mp_mediamix_off(struct power_domain *power_domain)
+{
+	struct udevice *dev = power_domain->dev;
+	struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
+	struct power_domain *domain;
+	struct clk *clk;
+	u32 reset;
+
+	switch (power_domain->id) {
+	case IMX8MP_MEDIABLK_PD_LCDIF_2:
+		domain = &priv->pd_lcdif2;
+		clk = &priv->clk_disp2;
+		reset = BIT(11) | BIT(12) | BIT(24);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Put devices into reset and disable clocks */
+	clrbits_le32(priv->base + BLK_SFT_RSTN, reset);
+	clrbits_le32(priv->base + BLK_CLK_EN, reset);
+
+	/* Power down upstream GPC domain */
+	power_domain_off(domain);
+
+	clk_disable(clk);
+	clk_disable(&priv->clk_axi);
+	clk_disable(&priv->clk_apb);
+
+	/* Allow bus domain to suspend */
+	power_domain_off(&priv->pd_bus);
+
+	return 0;
+}
+
+static int imx8mp_mediamix_of_xlate(struct power_domain *power_domain,
+				    struct ofnode_phandle_args *args)
+{
+	power_domain->id = args->args[0];
+
+	return 0;
+}
+
+static int imx8mp_mediamix_bind(struct udevice *dev)
+{
+	/* Bind child lcdif */
+	return dm_scan_fdt_dev(dev);
+}
+
+static int imx8mp_mediamix_probe(struct udevice *dev)
+{
+	struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	priv->base = dev_read_addr_ptr(dev);
+
+	ret = clk_get_by_name(dev, "apb", &priv->clk_apb);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_get_by_name(dev, "axi", &priv->clk_axi);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_get_by_name(dev, "disp2", &priv->clk_disp2);
+	if (ret < 0)
+		return ret;
+
+	ret = power_domain_get_by_name(dev, &priv->pd_bus, "bus");
+	if (ret < 0)
+		return ret;
+
+	ret = power_domain_get_by_name(dev, &priv->pd_lcdif2, "lcdif2");
+	if (ret < 0)
+		goto free_bus_pd;
+
+	return 0;
+
+free_bus_pd:
+	power_domain_free(&priv->pd_bus);
+	return ret;
+}
+
+static int imx8mp_mediamix_remove(struct udevice *dev)
+{
+	struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
+
+	power_domain_free(&priv->pd_lcdif2);
+	power_domain_free(&priv->pd_bus);
+
+	return 0;
+}
+
+static const struct udevice_id imx8mp_mediamix_ids[] = {
+	{ .compatible = "fsl,imx8mp-media-blk-ctrl" },
+	{ }
+};
+
+struct power_domain_ops imx8mp_mediamix_ops = {
+	.on = imx8mp_mediamix_on,
+	.off = imx8mp_mediamix_off,
+	.of_xlate = imx8mp_mediamix_of_xlate,
+};
+
+U_BOOT_DRIVER(imx8mp_mediamix) = {
+	.name		= "imx8mp_mediamix",
+	.id		= UCLASS_POWER_DOMAIN,
+	.of_match	= imx8mp_mediamix_ids,
+	.bind		= imx8mp_mediamix_bind,
+	.probe		= imx8mp_mediamix_probe,
+	.remove		= imx8mp_mediamix_remove,
+	.priv_auto	= sizeof(struct imx8mp_mediamix_priv),
+	.ops		= &imx8mp_mediamix_ops,
+};

-- 
2.48.1

[PATCH v6 10/12] video: imx: Fix Makefile in order to be able to add other imx drivers

[Miquel Raynal]

The IPUv3 is one IP part of the imx world, there are others, and
selecting the whole imx/ folder based on such a specific Kconfig symbol
is sub-optimal. Let's always enter the imx/ folder, and then selectively
compile parts of the folder based on the configuration.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/video/Makefile     | 2 +-
 drivers/video/imx/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 6073bc5234a93997a42d513feb360a075aa9d57f..8ebd596e11592b18fdc34542421ba165c1ec4fdb 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -53,7 +53,7 @@ obj-$(CONFIG_VIDEO_COREBOOT) += coreboot.o
 obj-$(CONFIG_VIDEO_DW_HDMI) += dw_hdmi.o
 obj-$(CONFIG_VIDEO_DW_MIPI_DSI) += dw_mipi_dsi.o
 obj-$(CONFIG_VIDEO_EFI) += efi.o
-obj-$(CONFIG_VIDEO_IPUV3) += imx/
+obj-y += imx/
 obj-$(CONFIG_VIDEO_IVYBRIDGE_IGD) += ivybridge_igd.o
 obj-$(CONFIG_VIDEO_LCD_ANX9804) += anx9804.o
 obj-$(CONFIG_VIDEO_LCD_ENDEAVORU) += endeavoru-panel.o
diff --git a/drivers/video/imx/Makefile b/drivers/video/imx/Makefile
index 179ea651fe8495e24206e2b06354a4f36651056c..46cc44d64b00959fb3880c7befb04a1ebdda1143 100644
--- a/drivers/video/imx/Makefile
+++ b/drivers/video/imx/Makefile
@@ -3,4 +3,4 @@
 # (C) Copyright 2000-2007
 # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
 
-obj-y += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o
+obj-$(CONFIG_VIDEO_IPUV3) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o

-- 
2.48.1

[PATCH v6 11/12] video: imx: Add LDB driver

[Miquel Raynal]

Add support for the LVDS Display Bridge (LDB) found on i.MX8MP.

When attached, the bridge driver looks for panels connected to one of
its two outputs and adapts its own configuration to use them. There is
currently no support for merged/split displays.

Note regarding the clock configuration:
The LDB output clock should be absolutely identical to the LCDIF output
clock so both blocks can talk to each other synchronously. However, the
LDB clock has an internal divisor of 7 (respectively 3.5 in dual
configuration) which means the LDB input clock must be explicitly set
once we know the configuration.

This driver was tested on i.MX8MP using a single panel connected to the
LVDS2 interface.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/video/imx/Kconfig  |   6 ++
 drivers/video/imx/Makefile |   1 +
 drivers/video/imx/ldb.c    | 251 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)

diff --git a/drivers/video/imx/Kconfig b/drivers/video/imx/Kconfig
index 34e8b640595b85678e386a3a251340031780a72e..c5cd2fecf780b62c0264b9627137517388aae1ab 100644
--- a/drivers/video/imx/Kconfig
+++ b/drivers/video/imx/Kconfig
@@ -13,3 +13,9 @@ config IMX_VIDEO_SKIP
 config IMX_HDMI
 	bool "Enable HDMI support in IPUv3"
 	depends on VIDEO_IPUV3
+
+config IMX_LDB
+	bool "Freescale i.MX8MP LDB bridge"
+	depends on VIDEO_BRIDGE
+	help
+	  Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
diff --git a/drivers/video/imx/Makefile b/drivers/video/imx/Makefile
index 46cc44d64b00959fb3880c7befb04a1ebdda1143..8d106589b7544ef9694613a2284e2fbfb564cf63 100644
--- a/drivers/video/imx/Makefile
+++ b/drivers/video/imx/Makefile
@@ -4,3 +4,4 @@
 # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
 
 obj-$(CONFIG_VIDEO_IPUV3) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o
+obj-$(CONFIG_IMX_LDB) += ldb.o
diff --git a/drivers/video/imx/ldb.c b/drivers/video/imx/ldb.c
new file mode 100644
index 0000000000000000000000000000000000000000..e918341c0a32232f0fc45aba8eb77a290064590c
--- /dev/null
+++ b/drivers/video/imx/ldb.c
@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Derived work from:
+ *   Philippe Cornu <philippe.cornu@st.com>
+ *   Yannick Fertre <yannick.fertre@st.com>
+ * Adapted by Miquel Raynal <miquel.raynal@bootlin.com>
+ */
+
+#define LOG_CATEGORY UCLASS_VIDEO_BRIDGE
+
+#include <clk.h>
+#include <dm.h>
+#include <log.h>
+#include <panel.h>
+#include <video_bridge.h>
+#include <asm/io.h>
+#include <linux/delay.h>
+
+#define LDB_CTRL_CH0_ENABLE BIT(0)
+#define LDB_CTRL_CH1_ENABLE BIT(2)
+#define LDB_CTRL_CH0_DATA_WIDTH BIT(5)
+#define LDB_CTRL_CH0_BIT_MAPPING BIT(6)
+#define LDB_CTRL_CH1_DATA_WIDTH BIT(7)
+#define LDB_CTRL_CH1_BIT_MAPPING BIT(8)
+#define LDB_CTRL_DI0_VSYNC_POLARITY BIT(9)
+#define LDB_CTRL_DI1_VSYNC_POLARITY BIT(10)
+
+#define LVDS_CTRL_CH0_EN BIT(0)
+#define LVDS_CTRL_CH1_EN BIT(1)
+#define LVDS_CTRL_VBG_EN BIT(2)
+#define LVDS_CTRL_PRE_EMPH_EN BIT(4)
+#define LVDS_CTRL_PRE_EMPH_ADJ(n) (((n) & 0x7) << 5)
+#define LVDS_CTRL_CC_ADJ(n) (((n) & 0x7) << 11)
+
+struct imx_ldb_priv {
+	struct clk ldb_clk;
+	void __iomem *ldb_ctrl;
+	void __iomem *lvds_ctrl;
+	struct udevice *lvds1;
+	struct udevice *lvds2;
+};
+
+static int imx_ldb_set_backlight(struct udevice *dev, int percent)
+{
+	struct imx_ldb_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	if (priv->lvds1) {
+		ret = panel_enable_backlight(priv->lvds1);
+		if (ret) {
+			debug("ldb: Cannot enable lvds1 backlight\n");
+			return ret;
+		}
+
+		ret = panel_set_backlight(priv->lvds1, percent);
+		if (ret)
+			return ret;
+	}
+
+	if (priv->lvds2) {
+		ret = panel_enable_backlight(priv->lvds2);
+		if (ret) {
+			debug("ldb: Cannot enable lvds2 backlight\n");
+			return ret;
+		}
+
+		ret = panel_set_backlight(priv->lvds2, percent);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int imx_ldb_of_to_plat(struct udevice *dev)
+{
+	struct imx_ldb_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	uclass_get_device_by_endpoint(UCLASS_PANEL, dev, 1, -1, &priv->lvds1);
+	uclass_get_device_by_endpoint(UCLASS_PANEL, dev, 2, -1, &priv->lvds2);
+	if (!priv->lvds1 && !priv->lvds2) {
+		debug("ldb: No remote panel for '%s' (ret=%d)\n",
+		      dev_read_name(dev), ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/* The block has a mysterious x7 internal divisor (x3.5 in dual configuration) */
+#define IMX_LDB_INTERNAL_DIVISOR(x) (((x) * 70) / 10)
+#define IMX_LDB_INTERNAL_DIVISOR_DUAL(x) (((x) * 35) / 10)
+
+static ulong imx_ldb_input_rate(struct imx_ldb_priv *priv,
+				struct display_timing *timings)
+{
+	ulong target_rate = timings->pixelclock.typ;
+
+	if (priv->lvds1 && priv->lvds2)
+		return IMX_LDB_INTERNAL_DIVISOR_DUAL(target_rate);
+
+	return IMX_LDB_INTERNAL_DIVISOR(target_rate);
+}
+
+static int imx_ldb_attach(struct udevice *dev)
+{
+	struct imx_ldb_priv *priv = dev_get_priv(dev);
+	struct display_timing timings;
+	bool format_jeida = false;
+	bool format_24bpp = true;
+	u32 ldb_ctrl = 0, lvds_ctrl;
+	ulong ldb_rate;
+	int ret;
+
+	/* TODO: update the 24bpp/jeida booleans with proper checks when they
+	 * will be supported.
+	 */
+	if (priv->lvds1) {
+		ret = panel_get_display_timing(priv->lvds1, &timings);
+		if (ret) {
+			ret = ofnode_decode_display_timing(dev_ofnode(priv->lvds1),
+							   0, &timings);
+			if (ret) {
+				printf("Cannot decode lvds1 timings (%d)\n", ret);
+				return ret;
+			}
+		}
+
+		ldb_ctrl |= LDB_CTRL_CH0_ENABLE;
+		if (format_24bpp)
+			ldb_ctrl |= LDB_CTRL_CH0_DATA_WIDTH;
+		if (format_jeida)
+			ldb_ctrl |= LDB_CTRL_CH0_BIT_MAPPING;
+		if (timings.flags & DISPLAY_FLAGS_VSYNC_HIGH)
+			ldb_ctrl |= LDB_CTRL_DI0_VSYNC_POLARITY;
+	}
+
+	if (priv->lvds2) {
+		ret = panel_get_display_timing(priv->lvds2, &timings);
+		if (ret) {
+			ret = ofnode_decode_display_timing(dev_ofnode(priv->lvds2),
+							   0, &timings);
+			if (ret) {
+				printf("Cannot decode lvds2 timings (%d)\n", ret);
+				return ret;
+			}
+		}
+
+		ldb_ctrl |= LDB_CTRL_CH1_ENABLE;
+		if (format_24bpp)
+			ldb_ctrl |= LDB_CTRL_CH1_DATA_WIDTH;
+		if (format_jeida)
+			ldb_ctrl |= LDB_CTRL_CH1_BIT_MAPPING;
+		if (timings.flags & DISPLAY_FLAGS_VSYNC_HIGH)
+			ldb_ctrl |= LDB_CTRL_DI1_VSYNC_POLARITY;
+	}
+
+	/*
+	 * Not all pixel clocks will work, as the final rate (after internal
+	 * integer division) should be identical to the LCDIF clock, otherwise
+	 * the rendering will appear resized/shimmering.
+	 */
+	ldb_rate = imx_ldb_input_rate(priv, &timings);
+	clk_set_rate(&priv->ldb_clk, ldb_rate);
+
+	writel(ldb_ctrl, priv->ldb_ctrl);
+
+	lvds_ctrl = LVDS_CTRL_CC_ADJ(2) | LVDS_CTRL_PRE_EMPH_EN |
+		    LVDS_CTRL_PRE_EMPH_ADJ(3) | LVDS_CTRL_VBG_EN;
+	writel(lvds_ctrl, priv->lvds_ctrl);
+
+	/* Wait for VBG to stabilize. */
+	udelay(15);
+
+	if (priv->lvds1)
+		lvds_ctrl |= LVDS_CTRL_CH0_EN;
+	if (priv->lvds2)
+		lvds_ctrl |= LVDS_CTRL_CH1_EN;
+
+	writel(lvds_ctrl, priv->lvds_ctrl);
+
+	return 0;
+}
+
+static int imx_ldb_probe(struct udevice *dev)
+{
+	struct imx_ldb_priv *priv = dev_get_priv(dev);
+	struct udevice *parent = dev_get_parent(dev);
+	fdt_addr_t parent_addr, child_addr;
+	int ret;
+
+	ret = clk_get_by_name(dev, "ldb", &priv->ldb_clk);
+	if (ret < 0)
+		return ret;
+
+	parent_addr = dev_read_addr(parent);
+	if (parent_addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+
+	child_addr = dev_read_addr_name(dev, "ldb");
+	if (child_addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+
+	priv->ldb_ctrl = map_physmem(parent_addr + child_addr, 0, MAP_NOCACHE);
+	if (!priv->ldb_ctrl)
+		return -EINVAL;
+
+	child_addr = dev_read_addr_name(dev, "lvds");
+	if (child_addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+
+	priv->lvds_ctrl = map_physmem(parent_addr + child_addr, 0, MAP_NOCACHE);
+	if (!priv->lvds_ctrl)
+		return -EINVAL;
+
+	ret = clk_enable(&priv->ldb_clk);
+	if (ret)
+		return ret;
+
+	ret = video_bridge_set_active(dev, true);
+	if (ret)
+		goto dis_clk;
+
+	return 0;
+
+dis_clk:
+	clk_disable(&priv->ldb_clk);
+
+	return ret;
+}
+
+struct video_bridge_ops imx_ldb_ops = {
+	.attach = imx_ldb_attach,
+	.set_backlight	= imx_ldb_set_backlight,
+};
+
+static const struct udevice_id imx_ldb_ids[] = {
+	{ .compatible = "fsl,imx8mp-ldb"},
+	{ }
+};
+
+U_BOOT_DRIVER(imx_ldb) = {
+	.name = "imx-lvds-display-bridge",
+	.id = UCLASS_VIDEO_BRIDGE,
+	.of_match = imx_ldb_ids,
+	.probe = imx_ldb_probe,
+	.of_to_plat = imx_ldb_of_to_plat,
+	.ops = &imx_ldb_ops,
+	.priv_auto = sizeof(struct imx_ldb_priv),
+};

-- 
2.48.1

[PATCH v6 12/12] video: imx: Add LCDIF driver

[Miquel Raynal]

Add support for the LCD interfaces (LCDIF1/2). When probed, these
interfaces request numerous clocks and power domains, attach the bridge
and look for a panel in order to retrieve its capabilities and
properties.

There is a similar existing driver in the upper folder for other i.MX
targets, I discovered this driver a bit late. It is not targeting the
i.MX8MP and I have no idea how different can the LCDIF be on this SoC,
but I did not manage to get it work, especially because it is not fully
compliant with the device-model, especially on the clocks/power
management side which is all ad-hoc. This is normal though, it was
contributed more than ten years ago.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/video/imx/Kconfig  |   3 +
 drivers/video/imx/Makefile |   1 +
 drivers/video/imx/lcdif.c  | 314 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)

diff --git a/drivers/video/imx/Kconfig b/drivers/video/imx/Kconfig
index c5cd2fecf780b62c0264b9627137517388aae1ab..12f11c2eea8c98cc341448923c80919afbceed46 100644
--- a/drivers/video/imx/Kconfig
+++ b/drivers/video/imx/Kconfig
@@ -19,3 +19,6 @@ config IMX_LDB
 	depends on VIDEO_BRIDGE
 	help
 	  Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
+
+config IMX_LCDIF
+	bool "i.MX LCDIFv3 LCD controller"
diff --git a/drivers/video/imx/Makefile b/drivers/video/imx/Makefile
index 8d106589b7544ef9694613a2284e2fbfb564cf63..1edf5a6bdf0c3afb218fbcd81384c7c277ca6b28 100644
--- a/drivers/video/imx/Makefile
+++ b/drivers/video/imx/Makefile
@@ -5,3 +5,4 @@
 
 obj-$(CONFIG_VIDEO_IPUV3) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o
 obj-$(CONFIG_IMX_LDB) += ldb.o
+obj-$(CONFIG_IMX_LCDIF) += lcdif.o
diff --git a/drivers/video/imx/lcdif.c b/drivers/video/imx/lcdif.c
new file mode 100644
index 0000000000000000000000000000000000000000..9f4fc7f51524449ecf8e155550e0fb3b641870d9
--- /dev/null
+++ b/drivers/video/imx/lcdif.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * i.MX8 LCD interface driver inspired from the Linux driver
+ * Copyright 2019 NXP
+ * Copyright 2024 Bootlin
+ * Adapted by Miquel Raynal <miquel.raynal@bootlin.com>
+ */
+
+#include <asm/io.h>
+#include <asm/mach-imx/dma.h>
+#include <clk.h>
+#include <dm.h>
+#include <panel.h>
+#include <power-domain.h>
+#include <video.h>
+#include <video_bridge.h>
+#include <linux/delay.h>
+
+#include "../videomodes.h"
+
+#define LCDIFV3_CTRL 0x0
+#define LCDIFV3_CTRL_SET 0x4
+#define LCDIFV3_CTRL_CLR 0x8
+#define   CTRL_INV_HS BIT(0)
+#define   CTRL_INV_VS BIT(1)
+#define   CTRL_INV_DE BIT(2)
+#define   CTRL_INV_PXCK BIT(3)
+#define   CTRL_CLK_GATE BIT(30)
+#define   CTRL_SW_RESET BIT(31)
+
+#define LCDIFV3_DISP_PARA 0x10
+#define   DISP_PARA_DISP_MODE_NORMAL 0
+#define   DISP_PARA_LINE_PATTERN_RGB_YUV 0
+#define   DISP_PARA_DISP_ON BIT(31)
+
+#define LCDIFV3_DISP_SIZE 0x14
+#define   DISP_SIZE_DELTA_X(x) ((x) & 0xffff)
+#define   DISP_SIZE_DELTA_Y(x) ((x) << 16)
+
+#define LCDIFV3_HSYN_PARA 0x18
+#define   HSYN_PARA_FP_H(x) ((x) & 0xffff)
+#define   HSYN_PARA_BP_H(x) ((x) << 16)
+
+#define LCDIFV3_VSYN_PARA 0x1C
+#define   VSYN_PARA_FP_V(x) ((x) & 0xffff)
+#define   VSYN_PARA_BP_V(x) ((x) << 16)
+
+#define LCDIFV3_VSYN_HSYN_WIDTH 0x20
+#define   VSYN_HSYN_PW_H(x) ((x) & 0xffff)
+#define   VSYN_HSYN_PW_V(x) ((x) << 16)
+
+#define LCDIFV3_CTRLDESCL0_1 0x200
+#define   CTRLDESCL0_1_WIDTH(x) ((x) & 0xffff)
+#define   CTRLDESCL0_1_HEIGHT(x) ((x) << 16)
+
+#define LCDIFV3_CTRLDESCL0_3 0x208
+#define   CTRLDESCL0_3_PITCH(x) ((x) & 0xFFFF)
+
+#define LCDIFV3_CTRLDESCL_LOW0_4 0x20C
+#define LCDIFV3_CTRLDESCL_HIGH0_4 0x210
+
+#define LCDIFV3_CTRLDESCL0_5 0x214
+#define   CTRLDESCL0_5_YUV_FORMAT(x) (((x) & 0x3) << 14)
+#define   CTRLDESCL0_5_BPP(x) (((x) & 0xf) << 24)
+#define     BPP32_ARGB8888 0x9
+#define   CTRLDESCL0_5_SHADOW_LOAD_EN BIT(30)
+#define   CTRLDESCL0_5_EN BIT(31)
+
+struct lcdifv3_priv {
+	void __iomem *base;
+	struct clk pix_clk;
+	struct power_domain pd;
+	struct udevice *panel;
+	struct udevice *bridge;
+};
+
+static void lcdifv3_set_mode(struct lcdifv3_priv *priv,
+			     struct display_timing *timings)
+{
+	u32 reg;
+
+	writel(DISP_SIZE_DELTA_X(timings->hactive.typ) |
+	       DISP_SIZE_DELTA_Y(timings->vactive.typ),
+	       priv->base + LCDIFV3_DISP_SIZE);
+
+	writel(HSYN_PARA_FP_H(timings->hfront_porch.typ) |
+	       HSYN_PARA_BP_H(timings->hback_porch.typ),
+	       priv->base + LCDIFV3_HSYN_PARA);
+
+	writel(VSYN_PARA_BP_V(timings->vback_porch.typ) |
+	       VSYN_PARA_FP_V(timings->vfront_porch.typ),
+	       priv->base + LCDIFV3_VSYN_PARA);
+
+	writel(VSYN_HSYN_PW_H(timings->hsync_len.typ) |
+	       VSYN_HSYN_PW_V(timings->vsync_len.typ),
+	       priv->base + LCDIFV3_VSYN_HSYN_WIDTH);
+
+	writel(CTRLDESCL0_1_WIDTH(timings->hactive.typ) |
+	       CTRLDESCL0_1_HEIGHT(timings->vactive.typ),
+	       priv->base + LCDIFV3_CTRLDESCL0_1);
+
+	if (timings->flags & DISPLAY_FLAGS_HSYNC_LOW)
+		writel(CTRL_INV_HS, priv->base + LCDIFV3_CTRL_SET);
+	else
+		writel(CTRL_INV_HS, priv->base + LCDIFV3_CTRL_CLR);
+
+	if (timings->flags & DISPLAY_FLAGS_VSYNC_LOW)
+		writel(CTRL_INV_VS, priv->base + LCDIFV3_CTRL_SET);
+	else
+		writel(CTRL_INV_VS, priv->base + LCDIFV3_CTRL_CLR);
+
+	if (timings->flags & DISPLAY_FLAGS_DE_LOW)
+		writel(CTRL_INV_DE, priv->base + LCDIFV3_CTRL_SET);
+	else
+		writel(CTRL_INV_DE, priv->base + LCDIFV3_CTRL_CLR);
+
+	if (timings->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
+		writel(CTRL_INV_PXCK, priv->base + LCDIFV3_CTRL_SET);
+	else
+		writel(CTRL_INV_PXCK, priv->base + LCDIFV3_CTRL_CLR);
+
+	writel(0, priv->base + LCDIFV3_DISP_PARA);
+
+	reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5);
+	reg &= ~(CTRLDESCL0_5_BPP(0xf) | CTRLDESCL0_5_YUV_FORMAT(0x3));
+	reg |= CTRLDESCL0_5_BPP(BPP32_ARGB8888);
+	writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5);
+}
+
+static void lcdifv3_enable_controller(struct lcdifv3_priv *priv)
+{
+	u32 reg;
+
+	reg = readl(priv->base + LCDIFV3_DISP_PARA);
+	reg |= DISP_PARA_DISP_ON;
+	writel(reg, priv->base + LCDIFV3_DISP_PARA);
+
+	reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5);
+	reg |= CTRLDESCL0_5_EN;
+	writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5);
+}
+
+static int lcdifv3_video_sync(struct udevice *dev)
+{
+	struct lcdifv3_priv *priv = dev_get_priv(dev);
+	u32 reg;
+
+	reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5);
+	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
+	writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5);
+
+	return 0;
+}
+
+static void lcdifv3_init(struct udevice *dev, struct display_timing *timings)
+{
+	struct video_uc_plat *plat = dev_get_uclass_plat(dev);
+	struct lcdifv3_priv *priv = dev_get_priv(dev);
+
+	clk_set_rate(&priv->pix_clk, timings->pixelclock.typ);
+
+	writel(CTRL_SW_RESET | CTRL_CLK_GATE, priv->base + LCDIFV3_CTRL_CLR);
+	udelay(10);
+
+	lcdifv3_set_mode(priv, timings);
+
+	writel(plat->base & 0xFFFFFFFF, priv->base + LCDIFV3_CTRLDESCL_LOW0_4);
+	writel(plat->base >> 32, priv->base + LCDIFV3_CTRLDESCL_HIGH0_4);
+
+	writel(CTRLDESCL0_3_PITCH(timings->hactive.typ * 4), /* 32bpp */
+	       priv->base + LCDIFV3_CTRLDESCL0_3);
+
+	lcdifv3_enable_controller(priv);
+}
+
+static int lcdifv3_video_probe(struct udevice *dev)
+{
+	struct video_uc_plat *plat = dev_get_uclass_plat(dev);
+	struct video_priv *uc_priv = dev_get_uclass_priv(dev);
+	struct lcdifv3_priv *priv = dev_get_priv(dev);
+	struct clk axi_clk, disp_axi_clk;
+	struct display_timing timings;
+	u32 fb_start, fb_end;
+	int ret;
+
+	ret = power_domain_get(dev, &priv->pd);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_get_by_name(dev, "pix", &priv->pix_clk);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_get_by_name(dev, "axi", &axi_clk);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_get_by_name(dev, "disp_axi", &disp_axi_clk);
+	if (ret < 0)
+		return ret;
+
+	ret = power_domain_on(&priv->pd);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(&priv->pix_clk);
+	if (ret)
+		goto dis_pd;
+
+	ret = clk_enable(&axi_clk);
+	if (ret)
+		goto dis_pix_clk;
+
+	ret = clk_enable(&disp_axi_clk);
+	if (ret)
+		goto dis_axi_clk;
+
+	priv->base = dev_remap_addr(dev);
+	if (!priv->base) {
+		ret = -EINVAL;
+		goto dis_clks;
+	}
+
+	/* Attach bridge */
+	ret = uclass_get_device_by_endpoint(UCLASS_VIDEO_BRIDGE, dev,
+					    -1, -1, &priv->bridge);
+	if (ret)
+		goto dis_clks;
+
+	ret = video_bridge_attach(priv->bridge);
+	if (ret)
+		goto dis_clks;
+
+	ret = video_bridge_set_backlight(priv->bridge, 80);
+	if (ret)
+		goto dis_clks;
+
+	/* Attach panels */
+	ret = uclass_get_device_by_endpoint(UCLASS_PANEL, priv->bridge,
+					    1, -1, &priv->panel);
+	if (ret) {
+		ret = uclass_get_device_by_endpoint(UCLASS_PANEL, priv->bridge,
+						    2, -1, &priv->panel);
+		if (ret)
+			goto dis_clks;
+	}
+
+	ret = panel_get_display_timing(priv->panel, &timings);
+	if (ret) {
+		ret = ofnode_decode_display_timing(dev_ofnode(priv->panel),
+						   0, &timings);
+		if (ret) {
+			printf("Cannot decode panel timings (%d)\n", ret);
+			goto dis_clks;
+		}
+	}
+
+	lcdifv3_init(dev, &timings);
+
+	/* Only support 32bpp for now */
+	uc_priv->bpix = VIDEO_BPP32;
+	uc_priv->xsize = timings.hactive.typ;
+	uc_priv->ysize = timings.vactive.typ;
+
+	/* Enable dcache for the frame buffer */
+	fb_start = plat->base & ~(MMU_SECTION_SIZE - 1);
+	fb_end = ALIGN(plat->base + plat->size, 1 << MMU_SECTION_SHIFT);
+	mmu_set_region_dcache_behaviour(fb_start, fb_end - fb_start,
+					DCACHE_WRITEBACK);
+	video_set_flush_dcache(dev, true);
+
+	return 0;
+
+dis_clks:
+	clk_disable(&disp_axi_clk);
+dis_axi_clk:
+	clk_disable(&axi_clk);
+dis_pix_clk:
+	clk_disable(&priv->pix_clk);
+dis_pd:
+	power_domain_off(&priv->pd);
+
+	return ret;
+}
+
+static int lcdifv3_video_bind(struct udevice *dev)
+{
+	struct video_uc_plat *plat = dev_get_uclass_plat(dev);
+
+	/* Max size supported by LCDIF */
+	plat->size = 1920 * 1080 * VNBYTES(VIDEO_BPP32);
+
+	return 0;
+}
+
+static const struct udevice_id lcdifv3_video_ids[] = {
+	{ .compatible = "fsl,imx8mp-lcdif" },
+	{ }
+};
+
+static struct video_ops lcdifv3_video_ops = {
+	.video_sync = lcdifv3_video_sync,
+};
+
+U_BOOT_DRIVER(lcdifv3_video) = {
+	.name = "lcdif",
+	.id = UCLASS_VIDEO,
+	.of_match = lcdifv3_video_ids,
+	.bind = lcdifv3_video_bind,
+	.ops = &lcdifv3_video_ops,
+	.probe = lcdifv3_video_probe,
+	.priv_auto = sizeof(struct lcdifv3_priv),
+	.flags = DM_FLAG_PRE_RELOC | DM_FLAG_OS_PREPARE,
+};

-- 
2.48.1

Re: [PATCH v6 03/12] dm: core: Add a helper to retrieve devices through graph endpoints

[Miquel Raynal]

> +int uclass_get_device_by_endpoint(enum uclass_id class_id, struct udevice *dev,
> +				  int port_idx, int ep_idx, struct udevice **devp)
> +{
> +	ofnode node_source = dev_ofnode(dev);
> +	ofnode node_dest = ofnode_graph_get_remote_node(node_source, port_idx, ep_idx);
> +	struct udevice *target = NULL;
> +	int ret;
> +
> +	if (!ofnode_valid(node_dest))
> +		return -EINVAL;
> +
> +	ret = uclass_find_device_by_ofnode(class_id, node_dest, &target);
> +	if (ret)
> +		return -ENODEV;
> +
> +	return uclass_get_device_tail(target, 0, devp);

Please disregard this version, I forgot to update this helper. Going
through another validation now...

Sorry for the noise,
Miquèl

Re: [PATCH v6 09/12] imx: power-domain: Add support for the MEDIAMIX control block

[Adam Ford]

On Thu, Apr 3, 2025 at 2:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> This block delivers power and clocks to the whole display and rendering
> pipeline.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/power/domain/Kconfig           |   7 ++
>  drivers/power/domain/Makefile          |   1 +
>  drivers/power/domain/imx8mp-mediamix.c | 208 +++++++++++++++++++++++++++++++++
>  3 files changed, 216 insertions(+)
>
> diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig
> index bd82d2f7044b3c3f8c88dea27c39193efedb4c84..5f5218bd8b5f4bed8283e91da61ac7c8c3eb97ae 100644
> --- a/drivers/power/domain/Kconfig
> +++ b/drivers/power/domain/Kconfig
> @@ -47,6 +47,13 @@ config IMX8MP_HSIOMIX_BLKCTRL
>         help
>           Enable support for manipulating NXP i.MX8MP on-SoC HSIOMIX block controller.
>
> +config IMX8MP_MEDIAMIX_BLKCTRL
> +       bool "Enable i.MX8MP MEDIAMIX domain driver"
> +       depends on POWER_DOMAIN && IMX8MP
> +       select CLK
> +       help
> +         Enable support for manipulating NXP i.MX8MP on-SoC MEDIAMIX block controller.
> +
>  config MTK_POWER_DOMAIN
>         bool "Enable the MediaTek power domain driver"
>         depends on POWER_DOMAIN && ARCH_MEDIATEK
> diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile
> index 110646c503ab97941a2bb86caf53f94542008219..356ec07163fd299f00e81a629ffc06c663a26c25 100644
> --- a/drivers/power/domain/Makefile
> +++ b/drivers/power/domain/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o
>  obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o
>  obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o
>  obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o
> +obj-$(CONFIG_IMX8MP_MEDIAMIX_BLKCTRL) += imx8mp-mediamix.o
>  obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o
>  obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_EE_POWER_DOMAIN) += meson-ee-pwrc.o
> diff --git a/drivers/power/domain/imx8mp-mediamix.c b/drivers/power/domain/imx8mp-mediamix.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..78c32ca3d3a87febdefd5d128d39d817674b8d32
> --- /dev/null
> +++ b/drivers/power/domain/imx8mp-mediamix.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX8 MEDIAMIX control block driver
> + * Copyright (C) 2024 Miquel Raynal <miquel.raynal@bootlin.com>
> + * Inspired from Marek Vasut <marex@denx.de> work on the hsiomix driver.
> + */
> +
> +#include <asm/io.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <power-domain-uclass.h>
> +#include <linux/delay.h>
> +
> +#include <dt-bindings/power/imx8mp-power.h>
> +
> +#define BLK_SFT_RSTN           0x0
> +#define BLK_CLK_EN             0x4
> +
> +struct imx8mp_mediamix_priv {
> +       void __iomem *base;
> +       struct clk clk_apb;
> +       struct clk clk_axi;
> +       struct clk clk_disp2;
> +       struct power_domain pd_bus;
> +       struct power_domain pd_lcdif2;
> +};
> +
> +static int imx8mp_mediamix_on(struct power_domain *power_domain)
> +{
> +       struct udevice *dev = power_domain->dev;
> +       struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
> +       struct power_domain *domain;
> +       struct clk *clk;
> +       u32 reset;
> +       int ret;
> +
> +       switch (power_domain->id) {
> +       case IMX8MP_MEDIABLK_PD_LCDIF_2:
> +               domain = &priv->pd_lcdif2;
> +               clk = &priv->clk_disp2;
> +               reset = BIT(11) | BIT(12) | BIT(24);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       /* Make sure bus domain is awake */
> +       ret = power_domain_on(&priv->pd_bus);
> +       if (ret)
> +               return ret;
> +
> +       /* Put devices into reset */
> +       clrbits_le32(priv->base + BLK_SFT_RSTN, reset);
> +
> +       /* Enable upstream clocks */
> +       ret = clk_enable(&priv->clk_apb);
> +       if (ret)
> +               goto dis_bus_pd;
> +
> +       ret = clk_enable(&priv->clk_axi);
> +       if (ret)
> +               goto dis_apb_clk;
> +
> +       /* Enable blk-ctrl clock to allow reset to propagate */
> +       ret = clk_enable(clk);

I am only offering a suggestion if you ever need to push another rev.
It appears that all the clocks are either on or off and if any fail,
you immediately turn off any clocks that were enabled.  There are some
bulk clock options where you can basically tell the driver to turn
them all on or all off.

clk_get_bulk, clk_enable_bulk, clk_disable_bulk, and clk_release_all
are the functions I am thinking of.  Your imx8mp_mediamix_priv
structure could then replace all the individual clocks with struct
clk_bulk.

That might shrink the overall code and simplify the readability.


> +       if (ret)
> +               goto dis_axi_clk;
> +       setbits_le32(priv->base + BLK_CLK_EN, reset);
> +
> +       /* Power up upstream GPC domain */
> +       ret = power_domain_on(domain);
> +       if (ret)
> +               goto dis_lcdif_clk;
> +
> +       /* Wait for reset to propagate */
> +       udelay(5);

On Linux, the GPCv2 driver was updated to increase a delay from 5 to
10 uSeconds [1] .  I don't know if that's needed here, but it's
something to consider.

adam

[1] - https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/pmdomain/imx?h=next-20250403&id=2379fb937de5333991c567eefd7d11b98977d059
> +
> +       /* Release reset */
> +       setbits_le32(priv->base + BLK_SFT_RSTN, reset);
> +
> +       return 0;
> +
> +dis_lcdif_clk:
> +       clk_disable(clk);
> +dis_axi_clk:
> +       clk_disable(&priv->clk_axi);
> +dis_apb_clk:
> +       clk_disable(&priv->clk_apb);
> +dis_bus_pd:
> +       power_domain_off(&priv->pd_bus);
> +
> +       return ret;
> +}
> +
> +static int imx8mp_mediamix_off(struct power_domain *power_domain)
> +{
> +       struct udevice *dev = power_domain->dev;
> +       struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
> +       struct power_domain *domain;
> +       struct clk *clk;
> +       u32 reset;
> +
> +       switch (power_domain->id) {
> +       case IMX8MP_MEDIABLK_PD_LCDIF_2:
> +               domain = &priv->pd_lcdif2;
> +               clk = &priv->clk_disp2;
> +               reset = BIT(11) | BIT(12) | BIT(24);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       /* Put devices into reset and disable clocks */
> +       clrbits_le32(priv->base + BLK_SFT_RSTN, reset);
> +       clrbits_le32(priv->base + BLK_CLK_EN, reset);
> +
> +       /* Power down upstream GPC domain */
> +       power_domain_off(domain);
> +
> +       clk_disable(clk);
> +       clk_disable(&priv->clk_axi);
> +       clk_disable(&priv->clk_apb);
> +
> +       /* Allow bus domain to suspend */
> +       power_domain_off(&priv->pd_bus);
> +
> +       return 0;
> +}
> +
> +static int imx8mp_mediamix_of_xlate(struct power_domain *power_domain,
> +                                   struct ofnode_phandle_args *args)
> +{
> +       power_domain->id = args->args[0];
> +
> +       return 0;
> +}
> +
> +static int imx8mp_mediamix_bind(struct udevice *dev)
> +{
> +       /* Bind child lcdif */
> +       return dm_scan_fdt_dev(dev);
> +}
> +
> +static int imx8mp_mediamix_probe(struct udevice *dev)
> +{
> +       struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
> +       int ret;
> +
> +       priv->base = dev_read_addr_ptr(dev);
> +
> +       ret = clk_get_by_name(dev, "apb", &priv->clk_apb);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = clk_get_by_name(dev, "axi", &priv->clk_axi);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = clk_get_by_name(dev, "disp2", &priv->clk_disp2);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = power_domain_get_by_name(dev, &priv->pd_bus, "bus");
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = power_domain_get_by_name(dev, &priv->pd_lcdif2, "lcdif2");
> +       if (ret < 0)
> +               goto free_bus_pd;
> +
> +       return 0;
> +
> +free_bus_pd:
> +       power_domain_free(&priv->pd_bus);
> +       return ret;
> +}
> +
> +static int imx8mp_mediamix_remove(struct udevice *dev)
> +{
> +       struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
> +
> +       power_domain_free(&priv->pd_lcdif2);
> +       power_domain_free(&priv->pd_bus);
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id imx8mp_mediamix_ids[] = {
> +       { .compatible = "fsl,imx8mp-media-blk-ctrl" },
> +       { }
> +};
> +
> +struct power_domain_ops imx8mp_mediamix_ops = {
> +       .on = imx8mp_mediamix_on,
> +       .off = imx8mp_mediamix_off,
> +       .of_xlate = imx8mp_mediamix_of_xlate,
> +};
> +
> +U_BOOT_DRIVER(imx8mp_mediamix) = {
> +       .name           = "imx8mp_mediamix",
> +       .id             = UCLASS_POWER_DOMAIN,
> +       .of_match       = imx8mp_mediamix_ids,
> +       .bind           = imx8mp_mediamix_bind,
> +       .probe          = imx8mp_mediamix_probe,
> +       .remove         = imx8mp_mediamix_remove,
> +       .priv_auto      = sizeof(struct imx8mp_mediamix_priv),
> +       .ops            = &imx8mp_mediamix_ops,
> +};
>
> --
> 2.48.1
>

Re: [PATCH v6 12/12] video: imx: Add LCDIF driver

[Adam Ford]

On Thu, Apr 3, 2025 at 2:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Add support for the LCD interfaces (LCDIF1/2). When probed, these
> interfaces request numerous clocks and power domains, attach the bridge
> and look for a panel in order to retrieve its capabilities and
> properties.
>
> There is a similar existing driver in the upper folder for other i.MX
> targets, I discovered this driver a bit late. It is not targeting the
> i.MX8MP and I have no idea how different can the LCDIF be on this SoC,
> but I did not manage to get it work, especially because it is not fully
> compliant with the device-model, especially on the clocks/power
> management side which is all ad-hoc. This is normal though, it was
> contributed more than ten years ago.

I think there was some discussion in the LKML about how similar /
different he LCD interfaces are between the 8MP and other NXP/i.MX
boards.

since there are three instances of this LCDIF, when most other boards
have just one, do you know what happens if someone's device tree has
all three enabled? Is the video mirrored, or does it just go to one of
the instances?

adam
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/video/imx/Kconfig  |   3 +
>  drivers/video/imx/Makefile |   1 +
>  drivers/video/imx/lcdif.c  | 314 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 318 insertions(+)
>
> diff --git a/drivers/video/imx/Kconfig b/drivers/video/imx/Kconfig
> index c5cd2fecf780b62c0264b9627137517388aae1ab..12f11c2eea8c98cc341448923c80919afbceed46 100644
> --- a/drivers/video/imx/Kconfig
> +++ b/drivers/video/imx/Kconfig
> @@ -19,3 +19,6 @@ config IMX_LDB
>         depends on VIDEO_BRIDGE
>         help
>           Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
> +
> +config IMX_LCDIF
> +       bool "i.MX LCDIFv3 LCD controller"
> diff --git a/drivers/video/imx/Makefile b/drivers/video/imx/Makefile
> index 8d106589b7544ef9694613a2284e2fbfb564cf63..1edf5a6bdf0c3afb218fbcd81384c7c277ca6b28 100644
> --- a/drivers/video/imx/Makefile
> +++ b/drivers/video/imx/Makefile
> @@ -5,3 +5,4 @@
>
>  obj-$(CONFIG_VIDEO_IPUV3) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o
>  obj-$(CONFIG_IMX_LDB) += ldb.o
> +obj-$(CONFIG_IMX_LCDIF) += lcdif.o
> diff --git a/drivers/video/imx/lcdif.c b/drivers/video/imx/lcdif.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..9f4fc7f51524449ecf8e155550e0fb3b641870d9
> --- /dev/null
> +++ b/drivers/video/imx/lcdif.c
> @@ -0,0 +1,314 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * i.MX8 LCD interface driver inspired from the Linux driver
> + * Copyright 2019 NXP
> + * Copyright 2024 Bootlin
> + * Adapted by Miquel Raynal <miquel.raynal@bootlin.com>
> + */
> +
> +#include <asm/io.h>
> +#include <asm/mach-imx/dma.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <panel.h>
> +#include <power-domain.h>
> +#include <video.h>
> +#include <video_bridge.h>
> +#include <linux/delay.h>
> +
> +#include "../videomodes.h"
> +
> +#define LCDIFV3_CTRL 0x0
> +#define LCDIFV3_CTRL_SET 0x4
> +#define LCDIFV3_CTRL_CLR 0x8
> +#define   CTRL_INV_HS BIT(0)
> +#define   CTRL_INV_VS BIT(1)
> +#define   CTRL_INV_DE BIT(2)
> +#define   CTRL_INV_PXCK BIT(3)
> +#define   CTRL_CLK_GATE BIT(30)
> +#define   CTRL_SW_RESET BIT(31)
> +
> +#define LCDIFV3_DISP_PARA 0x10
> +#define   DISP_PARA_DISP_MODE_NORMAL 0
> +#define   DISP_PARA_LINE_PATTERN_RGB_YUV 0
> +#define   DISP_PARA_DISP_ON BIT(31)
> +
> +#define LCDIFV3_DISP_SIZE 0x14
> +#define   DISP_SIZE_DELTA_X(x) ((x) & 0xffff)
> +#define   DISP_SIZE_DELTA_Y(x) ((x) << 16)
> +
> +#define LCDIFV3_HSYN_PARA 0x18
> +#define   HSYN_PARA_FP_H(x) ((x) & 0xffff)
> +#define   HSYN_PARA_BP_H(x) ((x) << 16)
> +
> +#define LCDIFV3_VSYN_PARA 0x1C
> +#define   VSYN_PARA_FP_V(x) ((x) & 0xffff)
> +#define   VSYN_PARA_BP_V(x) ((x) << 16)
> +
> +#define LCDIFV3_VSYN_HSYN_WIDTH 0x20
> +#define   VSYN_HSYN_PW_H(x) ((x) & 0xffff)
> +#define   VSYN_HSYN_PW_V(x) ((x) << 16)
> +
> +#define LCDIFV3_CTRLDESCL0_1 0x200
> +#define   CTRLDESCL0_1_WIDTH(x) ((x) & 0xffff)
> +#define   CTRLDESCL0_1_HEIGHT(x) ((x) << 16)
> +
> +#define LCDIFV3_CTRLDESCL0_3 0x208
> +#define   CTRLDESCL0_3_PITCH(x) ((x) & 0xFFFF)
> +
> +#define LCDIFV3_CTRLDESCL_LOW0_4 0x20C
> +#define LCDIFV3_CTRLDESCL_HIGH0_4 0x210
> +
> +#define LCDIFV3_CTRLDESCL0_5 0x214
> +#define   CTRLDESCL0_5_YUV_FORMAT(x) (((x) & 0x3) << 14)
> +#define   CTRLDESCL0_5_BPP(x) (((x) & 0xf) << 24)
> +#define     BPP32_ARGB8888 0x9
> +#define   CTRLDESCL0_5_SHADOW_LOAD_EN BIT(30)
> +#define   CTRLDESCL0_5_EN BIT(31)
> +
> +struct lcdifv3_priv {
> +       void __iomem *base;
> +       struct clk pix_clk;
> +       struct power_domain pd;
> +       struct udevice *panel;
> +       struct udevice *bridge;
> +};
> +
> +static void lcdifv3_set_mode(struct lcdifv3_priv *priv,
> +                            struct display_timing *timings)
> +{
> +       u32 reg;
> +
> +       writel(DISP_SIZE_DELTA_X(timings->hactive.typ) |
> +              DISP_SIZE_DELTA_Y(timings->vactive.typ),
> +              priv->base + LCDIFV3_DISP_SIZE);
> +
> +       writel(HSYN_PARA_FP_H(timings->hfront_porch.typ) |
> +              HSYN_PARA_BP_H(timings->hback_porch.typ),
> +              priv->base + LCDIFV3_HSYN_PARA);
> +
> +       writel(VSYN_PARA_BP_V(timings->vback_porch.typ) |
> +              VSYN_PARA_FP_V(timings->vfront_porch.typ),
> +              priv->base + LCDIFV3_VSYN_PARA);
> +
> +       writel(VSYN_HSYN_PW_H(timings->hsync_len.typ) |
> +              VSYN_HSYN_PW_V(timings->vsync_len.typ),
> +              priv->base + LCDIFV3_VSYN_HSYN_WIDTH);
> +
> +       writel(CTRLDESCL0_1_WIDTH(timings->hactive.typ) |
> +              CTRLDESCL0_1_HEIGHT(timings->vactive.typ),
> +              priv->base + LCDIFV3_CTRLDESCL0_1);
> +
> +       if (timings->flags & DISPLAY_FLAGS_HSYNC_LOW)
> +               writel(CTRL_INV_HS, priv->base + LCDIFV3_CTRL_SET);
> +       else
> +               writel(CTRL_INV_HS, priv->base + LCDIFV3_CTRL_CLR);
> +
> +       if (timings->flags & DISPLAY_FLAGS_VSYNC_LOW)
> +               writel(CTRL_INV_VS, priv->base + LCDIFV3_CTRL_SET);
> +       else
> +               writel(CTRL_INV_VS, priv->base + LCDIFV3_CTRL_CLR);
> +
> +       if (timings->flags & DISPLAY_FLAGS_DE_LOW)
> +               writel(CTRL_INV_DE, priv->base + LCDIFV3_CTRL_SET);
> +       else
> +               writel(CTRL_INV_DE, priv->base + LCDIFV3_CTRL_CLR);
> +
> +       if (timings->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
> +               writel(CTRL_INV_PXCK, priv->base + LCDIFV3_CTRL_SET);
> +       else
> +               writel(CTRL_INV_PXCK, priv->base + LCDIFV3_CTRL_CLR);
> +
> +       writel(0, priv->base + LCDIFV3_DISP_PARA);
> +
> +       reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5);
> +       reg &= ~(CTRLDESCL0_5_BPP(0xf) | CTRLDESCL0_5_YUV_FORMAT(0x3));
> +       reg |= CTRLDESCL0_5_BPP(BPP32_ARGB8888);
> +       writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5);
> +}
> +
> +static void lcdifv3_enable_controller(struct lcdifv3_priv *priv)
> +{
> +       u32 reg;
> +
> +       reg = readl(priv->base + LCDIFV3_DISP_PARA);
> +       reg |= DISP_PARA_DISP_ON;
> +       writel(reg, priv->base + LCDIFV3_DISP_PARA);
> +
> +       reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5);
> +       reg |= CTRLDESCL0_5_EN;
> +       writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5);
> +}
> +
> +static int lcdifv3_video_sync(struct udevice *dev)
> +{
> +       struct lcdifv3_priv *priv = dev_get_priv(dev);
> +       u32 reg;
> +
> +       reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5);
> +       reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> +       writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5);
> +
> +       return 0;
> +}
> +
> +static void lcdifv3_init(struct udevice *dev, struct display_timing *timings)
> +{
> +       struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> +       struct lcdifv3_priv *priv = dev_get_priv(dev);
> +
> +       clk_set_rate(&priv->pix_clk, timings->pixelclock.typ);
> +
> +       writel(CTRL_SW_RESET | CTRL_CLK_GATE, priv->base + LCDIFV3_CTRL_CLR);
> +       udelay(10);
> +
> +       lcdifv3_set_mode(priv, timings);
> +
> +       writel(plat->base & 0xFFFFFFFF, priv->base + LCDIFV3_CTRLDESCL_LOW0_4);
> +       writel(plat->base >> 32, priv->base + LCDIFV3_CTRLDESCL_HIGH0_4);
> +
> +       writel(CTRLDESCL0_3_PITCH(timings->hactive.typ * 4), /* 32bpp */
> +              priv->base + LCDIFV3_CTRLDESCL0_3);
> +
> +       lcdifv3_enable_controller(priv);
> +}
> +
> +static int lcdifv3_video_probe(struct udevice *dev)
> +{
> +       struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> +       struct video_priv *uc_priv = dev_get_uclass_priv(dev);
> +       struct lcdifv3_priv *priv = dev_get_priv(dev);
> +       struct clk axi_clk, disp_axi_clk;
> +       struct display_timing timings;
> +       u32 fb_start, fb_end;
> +       int ret;
> +
> +       ret = power_domain_get(dev, &priv->pd);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = clk_get_by_name(dev, "pix", &priv->pix_clk);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = clk_get_by_name(dev, "axi", &axi_clk);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = clk_get_by_name(dev, "disp_axi", &disp_axi_clk);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = power_domain_on(&priv->pd);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_enable(&priv->pix_clk);
> +       if (ret)
> +               goto dis_pd;
> +
> +       ret = clk_enable(&axi_clk);
> +       if (ret)
> +               goto dis_pix_clk;
> +
> +       ret = clk_enable(&disp_axi_clk);
> +       if (ret)
> +               goto dis_axi_clk;
> +
> +       priv->base = dev_remap_addr(dev);
> +       if (!priv->base) {
> +               ret = -EINVAL;
> +               goto dis_clks;
> +       }
> +
> +       /* Attach bridge */
> +       ret = uclass_get_device_by_endpoint(UCLASS_VIDEO_BRIDGE, dev,
> +                                           -1, -1, &priv->bridge);
> +       if (ret)
> +               goto dis_clks;
> +
> +       ret = video_bridge_attach(priv->bridge);
> +       if (ret)
> +               goto dis_clks;
> +
> +       ret = video_bridge_set_backlight(priv->bridge, 80);
> +       if (ret)
> +               goto dis_clks;
> +
> +       /* Attach panels */
> +       ret = uclass_get_device_by_endpoint(UCLASS_PANEL, priv->bridge,
> +                                           1, -1, &priv->panel);
> +       if (ret) {
> +               ret = uclass_get_device_by_endpoint(UCLASS_PANEL, priv->bridge,
> +                                                   2, -1, &priv->panel);
> +               if (ret)
> +                       goto dis_clks;
> +       }
> +
> +       ret = panel_get_display_timing(priv->panel, &timings);
> +       if (ret) {
> +               ret = ofnode_decode_display_timing(dev_ofnode(priv->panel),
> +                                                  0, &timings);
> +               if (ret) {
> +                       printf("Cannot decode panel timings (%d)\n", ret);
> +                       goto dis_clks;
> +               }
> +       }
> +
> +       lcdifv3_init(dev, &timings);
> +
> +       /* Only support 32bpp for now */
> +       uc_priv->bpix = VIDEO_BPP32;
> +       uc_priv->xsize = timings.hactive.typ;
> +       uc_priv->ysize = timings.vactive.typ;
> +
> +       /* Enable dcache for the frame buffer */
> +       fb_start = plat->base & ~(MMU_SECTION_SIZE - 1);
> +       fb_end = ALIGN(plat->base + plat->size, 1 << MMU_SECTION_SHIFT);
> +       mmu_set_region_dcache_behaviour(fb_start, fb_end - fb_start,
> +                                       DCACHE_WRITEBACK);
> +       video_set_flush_dcache(dev, true);
> +
> +       return 0;
> +
> +dis_clks:
> +       clk_disable(&disp_axi_clk);
> +dis_axi_clk:
> +       clk_disable(&axi_clk);
> +dis_pix_clk:
> +       clk_disable(&priv->pix_clk);
> +dis_pd:
> +       power_domain_off(&priv->pd);
> +
> +       return ret;
> +}
> +
> +static int lcdifv3_video_bind(struct udevice *dev)
> +{
> +       struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> +
> +       /* Max size supported by LCDIF */
> +       plat->size = 1920 * 1080 * VNBYTES(VIDEO_BPP32);
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id lcdifv3_video_ids[] = {
> +       { .compatible = "fsl,imx8mp-lcdif" },
> +       { }
> +};
> +
> +static struct video_ops lcdifv3_video_ops = {
> +       .video_sync = lcdifv3_video_sync,
> +};
> +
> +U_BOOT_DRIVER(lcdifv3_video) = {
> +       .name = "lcdif",
> +       .id = UCLASS_VIDEO,
> +       .of_match = lcdifv3_video_ids,
> +       .bind = lcdifv3_video_bind,
> +       .ops = &lcdifv3_video_ops,
> +       .probe = lcdifv3_video_probe,
> +       .priv_auto = sizeof(struct lcdifv3_priv),
> +       .flags = DM_FLAG_PRE_RELOC | DM_FLAG_OS_PREPARE,
> +};
>
> --
> 2.48.1
>

Re: [PATCH v6 06/12] clk: Ensure the parent clocks are enabled while reparenting

[Adam Ford]

On Thu, Apr 3, 2025 at 2:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Reparenting a clock C with a new parent P means that C will only
> continue clocking if P is already clocking when the mux is updated. In
> case the parent is currently disabled, failures (stalls) are likely to
> happen.

Marek made some changes to the clock registration to address some
parent-child relationships.  Do you know if this patch is still
needed?

adam
>
> This is exactly what happens on i.MX8 when enabling the video
> pipeline. We tell LCDIF clocks to use the VIDEO PLL as input, while the
> VIDEO PLL is currently off. This all happens as part of the
> assigned-clocks handling procedure, where the reparenting happens before
> the enable() calls. Enabling the parents as part of the reparenting
> procedure seems sane and also matches the logic applied in other parts
> of the CCM.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/clk/clk-uclass.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 90b70529a478d11d907cbd6b64d680d0c1db4d5c..4b3d812f9c6539a6b769990e3e11f14f49abb2a9 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -623,14 +623,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>         if (!ops->set_parent)
>                 return -ENOSYS;
>
> -       ret = ops->set_parent(clk, parent);
> -       if (ret)
> +       ret = clk_enable(parent);
> +       if (ret) {
> +               printf("Cannot enable parent %s\n", parent->dev->name);
>                 return ret;
> +       }
>
> -       if (CONFIG_IS_ENABLED(CLK_CCF))
> +       ret = ops->set_parent(clk, parent);
> +       if (ret) {
> +               clk_disable(parent);
> +               return ret;
> +       }
> +
> +       if (CONFIG_IS_ENABLED(CLK_CCF)) {
>                 ret = device_reparent(clk->dev, parent->dev);
> +               if (ret) {
> +                       clk_disable(parent);
> +                       return ret;
> +               }
> +       }
>
> -       return ret;
> +       return 0;
>  }
>
>  int clk_enable(struct clk *clk)
>
> --
> 2.48.1
>

Re: [PATCH v6 12/12] video: imx: Add LCDIF driver

[Miquel Raynal]

Hi Adam,

> since there are three instances of this LCDIF, when most other boards
> have just one, do you know what happens if someone's device tree has
> all three enabled? Is the video mirrored, or does it just go to one of
> the instances?

IIRC they are not connected to the same outputs, there is one for MIPI
DSI, one for LVDS displays and one for HDMI. The LVDS instance is
connected to a bridge (called LDB) which may join or mirror two LVDS
displays.

Thanks,
Miquèl

Re: [PATCH v6 09/12] imx: power-domain: Add support for the MEDIAMIX control block

[Miquel Raynal]

Hi Adam,

>> +       /* Enable upstream clocks */
>> +       ret = clk_enable(&priv->clk_apb);
>> +       if (ret)
>> +               goto dis_bus_pd;
>> +
>> +       ret = clk_enable(&priv->clk_axi);
>> +       if (ret)
>> +               goto dis_apb_clk;
>> +
>> +       /* Enable blk-ctrl clock to allow reset to propagate */
>> +       ret = clk_enable(clk);
>
> I am only offering a suggestion if you ever need to push another rev.
> It appears that all the clocks are either on or off and if any fail,
> you immediately turn off any clocks that were enabled.  There are some
> bulk clock options where you can basically tell the driver to turn
> them all on or all off.
>
> clk_get_bulk, clk_enable_bulk, clk_disable_bulk, and clk_release_all
> are the functions I am thinking of.  Your imx8mp_mediamix_priv
> structure could then replace all the individual clocks with struct
> clk_bulk.
>
> That might shrink the overall code and simplify the readability.

These bulk functions only work if you get all clocks from a list. We
currently do not want all the clocks (simply because some are useless in
this case and also because they have not yet been defined). The clks
property define all these:
"apb", "axi", "cam1", "cam2", "disp1", "disp2", "isp", "phy";

>> +       if (ret)
>> +               goto dis_axi_clk;
>> +       setbits_le32(priv->base + BLK_CLK_EN, reset);
>> +
>> +       /* Power up upstream GPC domain */
>> +       ret = power_domain_on(domain);
>> +       if (ret)
>> +               goto dis_lcdif_clk;
>> +
>> +       /* Wait for reset to propagate */
>> +       udelay(5);
>
> On Linux, the GPCv2 driver was updated to increase a delay from 5 to
> 10 uSeconds [1] .  I don't know if that's needed here, but it's
> something to consider.

Thanks a lot for the pointer, I will integrate this change, even if it
is experimental, let's spare ourselves a round of ghost bug hunting.

> adam
>
> [1] -
> https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/pmdomain/imx?h=next-20250403&id=2379fb937de5333991c567eefd7d11b98977d059

Thanks!
Miquèl

Re: [PATCH v6 00/12] Add imx8mp video support

[Fabio Estevam]

On Thu, Apr 3, 2025 at 4:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Changes in v6:
> - Another rebase on next.
> - Fixed the clock 24M oscillator clock name due to recent changes.
> - Fixed a subject prefix.
> - Collected tags.
> - Fixed the CI by adding two missing configuration items to secondary
>   sandbox defconfigs.
> - Link to v5: https://lore.kernel.org/r/20250326-ge-mainline-display-support-v5-0-ea4bc3fc9bca@bootlin.com

Applied the series, thanks.

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Francis, Neha]

On 4/3/2025 1:09 PM, Miquel Raynal wrote:
> It is very surprising that such an uclass, specifically designed to
> handle resources that may be shared by different devices, is not keeping
> the count of the number of times a power domain has been
> enabled/disabled to avoid shutting it down unexpectedly or disabling it
> several times.
> 
> Doing this causes troubles on eg. i.MX8MP because disabling power
> domains can be done in recursive loops were the same power domain
> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> timings to respect and it is easy to produce a race condition that puts
> the power domains in an unstable state, leading to ADB400 errors and
> later crashes in Linux.
> 
> CI tests using power domains are slightly updated to make sure the count
> of on/off calls is even and the results match what we *now* expect.
> 
> As we do not want to break existing users while stile getting
> interesting error codes, the implementation is split between:
> - a low-level helper reporting error codes if the requested transition
>   could not be operated,
> - a higher-level helper ignoring the "non error" codes, like EALREADY and
>   EBUSY.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---

This commit looks to be breaking K3 platforms boot, please see "Latest Boot
Summary" [0]. Reverting the commit fixes boot.

[0] https://lcpd.itg.ti.com/upstream/k3-boot-build/main/build-trends.html

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Nishanth Menon]

On 23:06-20250414, Francis, Neha wrote:
> On 4/3/2025 1:09 PM, Miquel Raynal wrote:
> > It is very surprising that such an uclass, specifically designed to
> > handle resources that may be shared by different devices, is not keeping
> > the count of the number of times a power domain has been
> > enabled/disabled to avoid shutting it down unexpectedly or disabling it
> > several times.
> > 
> > Doing this causes troubles on eg. i.MX8MP because disabling power
> > domains can be done in recursive loops were the same power domain
> > disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> > timings to respect and it is easy to produce a race condition that puts
> > the power domains in an unstable state, leading to ADB400 errors and
> > later crashes in Linux.
> > 
> > CI tests using power domains are slightly updated to make sure the count
> > of on/off calls is even and the results match what we *now* expect.
> > 
> > As we do not want to break existing users while stile getting
> > interesting error codes, the implementation is split between:
> > - a low-level helper reporting error codes if the requested transition
> >   could not be operated,
> > - a higher-level helper ignoring the "non error" codes, like EALREADY and
> >   EBUSY.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> 
> This commit looks to be breaking K3 platforms boot, please see "Latest Boot
> Summary" [0]. Reverting the commit fixes boot.
> 
> [0] https://lcpd.itg.ti.com/upstream/k3-boot-build/main/build-trends.html

Uggh. This link is a TI internal link. Nutshell, it is broken
between:
v2025.04-921-gcb7555e93075 : https://source.denx.de/u-boot/u-boot.git
and
v2025.04-1041-g407d68638fe3 : https://source.denx.de/u-boot/u-boot.git

Neha bisected it down to 197376fbf300e92afa0a1583815d9c9eb52d613a commit
which is this patch.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

On Mon, Apr 14, 2025 at 01:07:27PM -0500, Nishanth Menon wrote:
> On 23:06-20250414, Francis, Neha wrote:
> > On 4/3/2025 1:09 PM, Miquel Raynal wrote:
> > > It is very surprising that such an uclass, specifically designed to
> > > handle resources that may be shared by different devices, is not keeping
> > > the count of the number of times a power domain has been
> > > enabled/disabled to avoid shutting it down unexpectedly or disabling it
> > > several times.
> > > 
> > > Doing this causes troubles on eg. i.MX8MP because disabling power
> > > domains can be done in recursive loops were the same power domain
> > > disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> > > timings to respect and it is easy to produce a race condition that puts
> > > the power domains in an unstable state, leading to ADB400 errors and
> > > later crashes in Linux.
> > > 
> > > CI tests using power domains are slightly updated to make sure the count
> > > of on/off calls is even and the results match what we *now* expect.
> > > 
> > > As we do not want to break existing users while stile getting
> > > interesting error codes, the implementation is split between:
> > > - a low-level helper reporting error codes if the requested transition
> > >   could not be operated,
> > > - a higher-level helper ignoring the "non error" codes, like EALREADY and
> > >   EBUSY.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > 
> > This commit looks to be breaking K3 platforms boot, please see "Latest Boot
> > Summary" [0]. Reverting the commit fixes boot.
> > 
> > [0] https://lcpd.itg.ti.com/upstream/k3-boot-build/main/build-trends.html
> 
> Uggh. This link is a TI internal link. Nutshell, it is broken
> between:
> v2025.04-921-gcb7555e93075 : https://source.denx.de/u-boot/u-boot.git
> and
> v2025.04-1041-g407d68638fe3 : https://source.denx.de/u-boot/u-boot.git
> 
> Neha bisected it down to 197376fbf300e92afa0a1583815d9c9eb52d613a commit
> which is this patch.

And assuming it's the same failure I got reported this morning by one of
my coworkers, we just get:
U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')

For example on console.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Francesco Dolcini]

On Mon, Apr 14, 2025 at 02:06:35PM -0600, Tom Rini wrote:
> On Mon, Apr 14, 2025 at 01:07:27PM -0500, Nishanth Menon wrote:
> > On 23:06-20250414, Francis, Neha wrote:
> > > On 4/3/2025 1:09 PM, Miquel Raynal wrote:
> > > > It is very surprising that such an uclass, specifically designed to
> > > > handle resources that may be shared by different devices, is not keeping
> > > > the count of the number of times a power domain has been
> > > > enabled/disabled to avoid shutting it down unexpectedly or disabling it
> > > > several times.
> > > > 
> > > > Doing this causes troubles on eg. i.MX8MP because disabling power
> > > > domains can be done in recursive loops were the same power domain
> > > > disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> > > > timings to respect and it is easy to produce a race condition that puts
> > > > the power domains in an unstable state, leading to ADB400 errors and
> > > > later crashes in Linux.
> > > > 
> > > > CI tests using power domains are slightly updated to make sure the count
> > > > of on/off calls is even and the results match what we *now* expect.
> > > > 
> > > > As we do not want to break existing users while stile getting
> > > > interesting error codes, the implementation is split between:
> > > > - a low-level helper reporting error codes if the requested transition
> > > >   could not be operated,
> > > > - a higher-level helper ignoring the "non error" codes, like EALREADY and
> > > >   EBUSY.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > 
> > > This commit looks to be breaking K3 platforms boot, please see "Latest Boot
> > > Summary" [0]. Reverting the commit fixes boot.
> > > 
> > > [0] https://lcpd.itg.ti.com/upstream/k3-boot-build/main/build-trends.html
> > 
> > Uggh. This link is a TI internal link. Nutshell, it is broken
> > between:
> > v2025.04-921-gcb7555e93075 : https://source.denx.de/u-boot/u-boot.git
> > and
> > v2025.04-1041-g407d68638fe3 : https://source.denx.de/u-boot/u-boot.git
> > 
> > Neha bisected it down to 197376fbf300e92afa0a1583815d9c9eb52d613a commit
> > which is this patch.
> 
> And assuming it's the same failure I got reported this morning by one of
> my coworkers, we just get:
> U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
> SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')

I have not tried reverting that commit yet, but I do have the same
failure.

Francesco

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Neha Malcom Francis]

On 15/04/25 02:30, Francesco Dolcini wrote:
> On Mon, Apr 14, 2025 at 02:06:35PM -0600, Tom Rini wrote:
>> On Mon, Apr 14, 2025 at 01:07:27PM -0500, Nishanth Menon wrote:
>>> On 23:06-20250414, Francis, Neha wrote:
>>>> On 4/3/2025 1:09 PM, Miquel Raynal wrote:
>>>>> It is very surprising that such an uclass, specifically designed to
>>>>> handle resources that may be shared by different devices, is not keeping
>>>>> the count of the number of times a power domain has been
>>>>> enabled/disabled to avoid shutting it down unexpectedly or disabling it
>>>>> several times.
>>>>>
>>>>> Doing this causes troubles on eg. i.MX8MP because disabling power
>>>>> domains can be done in recursive loops were the same power domain
>>>>> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
>>>>> timings to respect and it is easy to produce a race condition that puts
>>>>> the power domains in an unstable state, leading to ADB400 errors and
>>>>> later crashes in Linux.
>>>>>
>>>>> CI tests using power domains are slightly updated to make sure the count
>>>>> of on/off calls is even and the results match what we *now* expect.
>>>>>
>>>>> As we do not want to break existing users while stile getting
>>>>> interesting error codes, the implementation is split between:
>>>>> - a low-level helper reporting error codes if the requested transition
>>>>>   could not be operated,
>>>>> - a higher-level helper ignoring the "non error" codes, like EALREADY and
>>>>>   EBUSY.
>>>>>
>>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> ---
>>>>
>>>> This commit looks to be breaking K3 platforms boot, please see "Latest Boot
>>>> Summary" [0]. Reverting the commit fixes boot.
>>>>
>>>> [0] https://lcpd.itg.ti.com/upstream/k3-boot-build/main/build-trends.html

My bad on the internal link.

>>>
>>> Uggh. This link is a TI internal link. Nutshell, it is broken
>>> between:
>>> v2025.04-921-gcb7555e93075 : https://source.denx.de/u-boot/u-boot.git
>>> and
>>> v2025.04-1041-g407d68638fe3 : https://source.denx.de/u-boot/u-boot.git
>>>
>>> Neha bisected it down to 197376fbf300e92afa0a1583815d9c9eb52d613a commit
>>> which is this patch.
>>
>> And assuming it's the same failure I got reported this morning by one of
>> my coworkers, we just get:
>> U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
>> SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')

This is not the failure I am seeing, we hang before console comes up so
no prints. Looks like the different failure signature is due to TIFS
(SYSFW) firmware being different (v9.2.7 vs. 11.0.4)
> I have not tried reverting that commit yet, but I do have the same
> failure.
> 
> Francesco
> 



-- 
Thanking You
Neha Malcom Francis

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Francesco Dolcini]

Hello Tom, Neha

On Tue, Apr 15, 2025 at 10:50:42AM +0530, Neha Malcom Francis wrote:
> 
> 
> On 15/04/25 02:30, Francesco Dolcini wrote:
> > On Mon, Apr 14, 2025 at 02:06:35PM -0600, Tom Rini wrote:
> >> On Mon, Apr 14, 2025 at 01:07:27PM -0500, Nishanth Menon wrote:
> >>> On 23:06-20250414, Francis, Neha wrote:
> >>>> On 4/3/2025 1:09 PM, Miquel Raynal wrote:
> >>>>> It is very surprising that such an uclass, specifically designed to
> >>>>> handle resources that may be shared by different devices, is not keeping
> >>>>> the count of the number of times a power domain has been
> >>>>> enabled/disabled to avoid shutting it down unexpectedly or disabling it
> >>>>> several times.
> >>>>>
> >>>>> Doing this causes troubles on eg. i.MX8MP because disabling power
> >>>>> domains can be done in recursive loops were the same power domain
> >>>>> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> >>>>> timings to respect and it is easy to produce a race condition that puts
> >>>>> the power domains in an unstable state, leading to ADB400 errors and
> >>>>> later crashes in Linux.
> >>>>>
> >>>>> CI tests using power domains are slightly updated to make sure the count
> >>>>> of on/off calls is even and the results match what we *now* expect.
> >>>>>
> >>>>> As we do not want to break existing users while stile getting
> >>>>> interesting error codes, the implementation is split between:
> >>>>> - a low-level helper reporting error codes if the requested transition
> >>>>>   could not be operated,
> >>>>> - a higher-level helper ignoring the "non error" codes, like EALREADY and
> >>>>>   EBUSY.
> >>>>>
> >>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>> ---
> >>>>
> >>>> This commit looks to be breaking K3 platforms boot, please see "Latest Boot
> >>>> Summary" [0]. Reverting the commit fixes boot.
> >>>>
> >>>> [0] https://lcpd.itg.ti.com/upstream/k3-boot-build/main/build-trends.html
> 
> My bad on the internal link.
> 
> >>>
> >>> Uggh. This link is a TI internal link. Nutshell, it is broken
> >>> between:
> >>> v2025.04-921-gcb7555e93075 : https://source.denx.de/u-boot/u-boot.git
> >>> and
> >>> v2025.04-1041-g407d68638fe3 : https://source.denx.de/u-boot/u-boot.git
> >>>
> >>> Neha bisected it down to 197376fbf300e92afa0a1583815d9c9eb52d613a commit
> >>> which is this patch.
> >>
> >> And assuming it's the same failure I got reported this morning by one of
> >> my coworkers, we just get:
> >> U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
> >> SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')
> 
> This is not the failure I am seeing, we hang before console comes up so
> no prints. Looks like the different failure signature is due to TIFS
> (SYSFW) firmware being different (v9.2.7 vs. 11.0.4)

To me it was failing freezing just after this

U-Boot SPL 2025.04-01076-g739ad58dbee8 (Apr 14 2025 - 17:23:46 +0200)
SYSFW ABI: 4.0 (firmware rev 0x000b '11.0.7--v11.00.07 (Fancy Rat)')

and, with commit 197376fbf300 ("power-domain: Add refcounting") reverted
the issue is fixed and I get to the U-Boot command line.

Francesco

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Miquel Raynal]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

Hello,

Thanks for the report.

>> >>> Neha bisected it down to 197376fbf300e92afa0a1583815d9c9eb52d613a commit
>> >>> which is this patch.
>> >>
>> >> And assuming it's the same failure I got reported this morning by one of
>> >> my coworkers, we just get:
>> >> U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
>> >> SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')
>> 
>> This is not the failure I am seeing, we hang before console comes up so
>> no prints. Looks like the different failure signature is due to TIFS
>> (SYSFW) firmware being different (v9.2.7 vs. 11.0.4)
>
> To me it was failing freezing just after this
>
> U-Boot SPL 2025.04-01076-g739ad58dbee8 (Apr 14 2025 - 17:23:46 +0200)
> SYSFW ABI: 4.0 (firmware rev 0x000b '11.0.7--v11.00.07 (Fancy Rat)')
>
> and, with commit 197376fbf300 ("power-domain: Add refcounting") reverted
> the issue is fixed and I get to the U-Boot command line.

Francesco, are you also testing on K3 platforms?

Can one of you boot with the patch below applied? It should partially
revert the commit to the ancient behaviour, while adding more debug
traces (please enable the debug logs as well). This should clarify
which one of the 3 different path is likely failing. It should also help
identify who's the user that fails to enable/disable its own power
domain. I will need to know what board/SoC was used for the test, so I
can look the relevant driver up.

Thanks for your help,
Miquèl

---


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-HACK-power-domain-Add-debug-traces-without-actually-.patch --]
[-- Type: text/x-patch, Size: 1915 bytes --]

From 4b128872b2dbcdfb626b8683fbb6c75d17a5089c Mon Sep 17 00:00:00 2001
From: Miquel Raynal <miquel.raynal@bootlin.com>
Date: Tue, 15 Apr 2025 10:07:05 +0200
Subject: [PATCH] HACK: power-domain: Add debug traces without actually failing

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/power/domain/power-domain-uclass.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
index a6e5f9ed0369..18ace2cc7256 100644
--- a/drivers/power/domain/power-domain-uclass.c
+++ b/drivers/power/domain/power-domain-uclass.c
@@ -117,10 +117,12 @@ int power_domain_on_lowlevel(struct power_domain *power_domain)
 	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
 	int ret;
 
-	debug("%s(power_domain=%p)\n", __func__, power_domain);
+	debug("%s(power_domain=%p, id %d)\n", __func__, power_domain, power_domain->id);
 
-	if (priv->on_count++ > 0)
-		return -EALREADY;
+	if (priv->on_count++ > 0) {
+		debug("Power domain %s already on.\n", power_domain->dev->name);
+		//return -EALREADY;
+	}
 
 	ret = ops->on ? ops->on(power_domain) : 0;
 	if (ret) {
@@ -137,15 +139,17 @@ int power_domain_off_lowlevel(struct power_domain *power_domain)
 	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
 	int ret;
 
-	debug("%s(power_domain=%p)\n", __func__, power_domain);
+	debug("%s(power_domain=%p, id %d)\n", __func__, power_domain, power_domain->id);
 
 	if (priv->on_count <= 0) {
 		debug("Power domain %s already off.\n", power_domain->dev->name);
-		return -EALREADY;
+		//return -EALREADY;
 	}
 
-	if (priv->on_count-- > 1)
-		return -EBUSY;
+	if (priv->on_count-- > 1) {
+		debug("Power domain %s still in use.\n", power_domain->dev->name);
+		//return -EBUSY;
+	}
 
 	ret = ops->off ? ops->off(power_domain) : 0;
 	if (ret) {
-- 
2.48.1

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Neha Malcom Francis]

Hi Miquel

On 15/04/25 13:45, Miquel Raynal wrote:
> Hello,
> 
> Thanks for the report.
> 
>>>>>> Neha bisected it down to 197376fbf300e92afa0a1583815d9c9eb52d613a commit
>>>>>> which is this patch.
>>>>>
>>>>> And assuming it's the same failure I got reported this morning by one of
>>>>> my coworkers, we just get:
>>>>> U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
>>>>> SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')
>>>
>>> This is not the failure I am seeing, we hang before console comes up so
>>> no prints. Looks like the different failure signature is due to TIFS
>>> (SYSFW) firmware being different (v9.2.7 vs. 11.0.4)
>>
>> To me it was failing freezing just after this
>>
>> U-Boot SPL 2025.04-01076-g739ad58dbee8 (Apr 14 2025 - 17:23:46 +0200)
>> SYSFW ABI: 4.0 (firmware rev 0x000b '11.0.7--v11.00.07 (Fancy Rat)')
>>
>> and, with commit 197376fbf300 ("power-domain: Add refcounting") reverted
>> the issue is fixed and I get to the U-Boot command line.
> 
> Francesco, are you also testing on K3 platforms?
> 

Yes he would be, since the firmware print is K3 firmware.

> Can one of you boot with the patch below applied? It should partially
> revert the commit to the ancient behaviour, while adding more debug
> traces (please enable the debug logs as well). This should clarify
> which one of the 3 different path is likely failing. It should also help
> identify who's the user that fails to enable/disable its own power
> domain. I will need to know what board/SoC was used for the test, so I
> can look the relevant driver up.

Booted on j784s4_evm

https://gist.github.com/nehamalcom/b09687a523bec89f9df3537fdd99b6f3

Currently debugging on my build where I hang in console_init itself, I
think the path for failure is different here, will confirm.

> 
> Thanks for your help,
> Miquèl
> 
> ---
> 

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Miquel Raynal]

>> Francesco, are you also testing on K3 platforms?
>> 
>
> Yes he would be, since the firmware print is K3 firmware.
>
>> Can one of you boot with the patch below applied? It should partially
>> revert the commit to the ancient behaviour, while adding more debug
>> traces (please enable the debug logs as well). This should clarify
>> which one of the 3 different path is likely failing. It should also help
>> identify who's the user that fails to enable/disable its own power
>> domain. I will need to know what board/SoC was used for the test, so I
>> can look the relevant driver up.
>
> Booted on j784s4_evm
>
> https://gist.github.com/nehamalcom/b09687a523bec89f9df3537fdd99b6f3
>
> Currently debugging on my build where I hang in console_init itself, I
> think the path for failure is different here, will confirm.

Thanks a lot for your feedback. Normally by applying the diff shared in
my previous e-mail, you should have the console back. If not, maybe
there is something else involved and the blamed commit is just the first
showing an underlying problem.

Thanks,
Miquèl

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Neha Malcom Francis]

Hi Miquel

On 15/04/25 15:20, Miquel Raynal wrote:
>>> Francesco, are you also testing on K3 platforms?
>>>
>>
>> Yes he would be, since the firmware print is K3 firmware.
>>
>>> Can one of you boot with the patch below applied? It should partially
>>> revert the commit to the ancient behaviour, while adding more debug
>>> traces (please enable the debug logs as well). This should clarify
>>> which one of the 3 different path is likely failing. It should also help
>>> identify who's the user that fails to enable/disable its own power
>>> domain. I will need to know what board/SoC was used for the test, so I
>>> can look the relevant driver up.
>>
>> Booted on j784s4_evm
>>
>> https://gist.github.com/nehamalcom/b09687a523bec89f9df3537fdd99b6f3

^
>>
>> Currently debugging on my build where I hang in console_init itself, I
>> think the path for failure is different here, will confirm.
> 
> Thanks a lot for your feedback. Normally by applying the diff shared in
> my previous e-mail, you should have the console back. If not, maybe
> there is something else involved and the blamed commit is just the first
> showing an underlying problem.

I do have console back with that patch, see the gist link above. What I
meant was the path of failure seems to be different, it's looking like
with my version of firmware the serial driver probe
(ns16550_serial_probe) tries to do a readb() and fails, this could
possibly be because the device was not powered on to begin with. Whereas
in another case (what Francesco is seeing), this issue is not run into.


> 
> Thanks,
> Miquèl

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Francesco Dolcini]

On Tue, Apr 15, 2025 at 03:29:09PM +0530, Neha Malcom Francis wrote:
> On 15/04/25 15:20, Miquel Raynal wrote:
> >>> Francesco, are you also testing on K3 platforms?
> >>>
> >>
> >> Yes he would be, since the firmware print is K3 firmware.
> >>
> >>> Can one of you boot with the patch below applied? It should partially
> >>> revert the commit to the ancient behaviour, while adding more debug
> >>> traces (please enable the debug logs as well). This should clarify
> >>> which one of the 3 different path is likely failing. It should also help
> >>> identify who's the user that fails to enable/disable its own power
> >>> domain. I will need to know what board/SoC was used for the test, so I
> >>> can look the relevant driver up.
> >>
> >> Booted on j784s4_evm
> >>
> >> https://gist.github.com/nehamalcom/b09687a523bec89f9df3537fdd99b6f3
> 
> ^
> >>
> >> Currently debugging on my build where I hang in console_init itself, I
> >> think the path for failure is different here, will confirm.
> > 
> > Thanks a lot for your feedback. Normally by applying the diff shared in
> > my previous e-mail, you should have the console back. If not, maybe
> > there is something else involved and the blamed commit is just the first
> > showing an underlying problem.
> 
> I do have console back with that patch, see the gist link above. What I
> meant was the path of failure seems to be different, it's looking like
> with my version of firmware the serial driver probe
> (ns16550_serial_probe) tries to do a readb() and fails, this could
> possibly be because the device was not powered on to begin with. Whereas
> in another case (what Francesco is seeing), this issue is not run into.

For the record, my logs and tests are on Verdin AM62 (that uses a TI AM625 SoC).

Francesco

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Miquel Raynal]

On 15/04/2025 at 15:29:09 +0530, Neha Malcom Francis <n-francis@ti.com> wrote:

> Hi Miquel
>
> On 15/04/25 15:20, Miquel Raynal wrote:
>>>> Francesco, are you also testing on K3 platforms?
>>>>
>>>
>>> Yes he would be, since the firmware print is K3 firmware.
>>>
>>>> Can one of you boot with the patch below applied? It should partially
>>>> revert the commit to the ancient behaviour, while adding more debug
>>>> traces (please enable the debug logs as well). This should clarify
>>>> which one of the 3 different path is likely failing. It should also help
>>>> identify who's the user that fails to enable/disable its own power
>>>> domain. I will need to know what board/SoC was used for the test, so I
>>>> can look the relevant driver up.
>>>
>>> Booted on j784s4_evm
>>>
>>> https://gist.github.com/nehamalcom/b09687a523bec89f9df3537fdd99b6f3
>
> ^

Ah, sorry! This is very interesting.

There are two things which I do not understand:
- if we look at the very first power domains, their id is 15 and 14
  respectively. I do not see these numbers in any device tree, am I
  looking incorrectly? Could you help me identify what are the devices
  requesting these IDs?
- all power domains appear to be already enabled the very first time we
  enable them. I believe this is the root cause of the problem, as they
  should have count_on to 0 when booting. I checked again,
  'per_device_auto' is a field that is zeroed when allocated (using
  calloc() in this case) so I am really puzzled about this. It looks
  like "something else" is using this data field, which would be
  wrong.

>>>
>>> Currently debugging on my build where I hang in console_init itself, I
>>> think the path for failure is different here, will confirm.
>> 
>> Thanks a lot for your feedback. Normally by applying the diff shared in
>> my previous e-mail, you should have the console back. If not, maybe
>> there is something else involved and the blamed commit is just the first
>> showing an underlying problem.
>
> I do have console back with that patch, see the gist link above. What I
> meant was the path of failure seems to be different, it's looking like
> with my version of firmware the serial driver probe
> (ns16550_serial_probe) tries to do a readb() and fails, this could
> possibly be because the device was not powered on to begin with. Whereas
> in another case (what Francesco is seeing), this issue is not run
> into.

Clear.

Could you make another round with this extra change?

Thanks!
Miquèl

--- a/drivers/power/domain/power-domain-uclass.c
+++ b/drivers/power/domain/power-domain-uclass.c
@@ -119,14 +119,18 @@ int power_domain_on_lowlevel(struct power_domain *power_domain)
 
        debug("%s(power_domain=%p, id %d)\n", __func__, power_domain, power_domain->id);
 
-       if (priv->on_count++ > 0) {
-               debug("Power domain %s already on.\n", power_domain->dev->name);
+       if (priv->on_count > 0) {
+               debug("Power domain %s already on (usage count: %d).\n", power_domain->dev->name, priv->on_count);
+               priv->on_count++;
                //return -EALREADY;
+       } else {
+               priv->on_count++;
        }
 
        ret = ops->on ? ops->on(power_domain) : 0;
        if (ret) {
                priv->on_count--;
+               debug("Power domain %s on failed (usage count: %d).\n", power_domain->dev->name, priv->on_count);
                return ret;
        }
 

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Samuel Holland]

Hi Miquel,

On 2025-04-03 2:39 AM, Miquel Raynal wrote:
> It is very surprising that such an uclass, specifically designed to
> handle resources that may be shared by different devices, is not keeping
> the count of the number of times a power domain has been
> enabled/disabled to avoid shutting it down unexpectedly or disabling it
> several times.
> 
> Doing this causes troubles on eg. i.MX8MP because disabling power
> domains can be done in recursive loops were the same power domain
> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> timings to respect and it is easy to produce a race condition that puts
> the power domains in an unstable state, leading to ADB400 errors and
> later crashes in Linux.
> 
> CI tests using power domains are slightly updated to make sure the count
> of on/off calls is even and the results match what we *now* expect.
> 
> As we do not want to break existing users while stile getting
> interesting error codes, the implementation is split between:
> - a low-level helper reporting error codes if the requested transition
>   could not be operated,
> - a higher-level helper ignoring the "non error" codes, like EALREADY and
>   EBUSY.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/firmware/scmi/sandbox-scmi_devices.c     |  1 +
>  drivers/power/domain/power-domain-uclass.c       | 40 ++++++++++++++--
>  drivers/power/domain/sandbox-power-domain-test.c |  1 +
>  include/power-domain.h                           | 60 ++++++++++++++++++++----
>  test/dm/power-domain.c                           |  2 +-
>  5 files changed, 91 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
> index 96c2922b067e2886b3fa963bcd7e396f4569a569..9f253b0fd40f703a5ec11d34c197423d27ad8b01 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_devices.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
> @@ -163,4 +163,5 @@ U_BOOT_DRIVER(sandbox_scmi_devices) = {
>  	.priv_auto = sizeof(struct sandbox_scmi_device_priv),
>  	.remove = sandbox_scmi_devices_remove,
>  	.probe = sandbox_scmi_devices_probe,
> +	.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
>  };
> diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
> index 938bd8cbc9ffd1ba2109d702f886b6a99288d063..a6e5f9ed0369eb9d2dfa66edc9e938bac6720dab 100644
> --- a/drivers/power/domain/power-domain-uclass.c
> +++ b/drivers/power/domain/power-domain-uclass.c
> @@ -12,6 +12,10 @@
>  #include <power-domain-uclass.h>
>  #include <dm/device-internal.h>
>  
> +struct power_domain_priv {
> +	int on_count;
> +};
> +
>  static inline struct power_domain_ops *power_domain_dev_ops(struct udevice *dev)
>  {
>  	return (struct power_domain_ops *)dev->driver->ops;
> @@ -107,22 +111,49 @@ int power_domain_free(struct power_domain *power_domain)
>  	return ops->rfree ? ops->rfree(power_domain) : 0;
>  }
>  
> -int power_domain_on(struct power_domain *power_domain)
> +int power_domain_on_lowlevel(struct power_domain *power_domain)
>  {
> +	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>  	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
> +	int ret;
>  
>  	debug("%s(power_domain=%p)\n", __func__, power_domain);
>  
> -	return ops->on ? ops->on(power_domain) : 0;
> +	if (priv->on_count++ > 0)
> +		return -EALREADY;

This change is broken for power domain providers with #power-domain-cells = <1>,
which can have multiple domains per provider device. There would need to be a
separate reference count per domain, and currently the uclass doesn't know the
range of valid domain IDs.

Regards,
Samuel

> +
> +	ret = ops->on ? ops->on(power_domain) : 0;
> +	if (ret) {
> +		priv->on_count--;
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
> -int power_domain_off(struct power_domain *power_domain)
> +int power_domain_off_lowlevel(struct power_domain *power_domain)
>  {
> +	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>  	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
> +	int ret;
>  
>  	debug("%s(power_domain=%p)\n", __func__, power_domain);
>  
> -	return ops->off ? ops->off(power_domain) : 0;
> +	if (priv->on_count <= 0) {
> +		debug("Power domain %s already off.\n", power_domain->dev->name);
> +		return -EALREADY;
> +	}
> +
> +	if (priv->on_count-- > 1)
> +		return -EBUSY;
> +
> +	ret = ops->off ? ops->off(power_domain) : 0;
> +	if (ret) {
> +		priv->on_count++;
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  #if CONFIG_IS_ENABLED(OF_REAL)
> @@ -180,4 +211,5 @@ int dev_power_domain_off(struct udevice *dev)
>  UCLASS_DRIVER(power_domain) = {
>  	.id		= UCLASS_POWER_DOMAIN,
>  	.name		= "power_domain",
> +	.per_device_auto = sizeof(struct power_domain_priv),
>  };
> diff --git a/drivers/power/domain/sandbox-power-domain-test.c b/drivers/power/domain/sandbox-power-domain-test.c
> index 08c15ef342b3dd3ce01807ee59b7e97337f7dde5..5b530974e942ffcba453e53be330baaf3a113a13 100644
> --- a/drivers/power/domain/sandbox-power-domain-test.c
> +++ b/drivers/power/domain/sandbox-power-domain-test.c
> @@ -51,4 +51,5 @@ U_BOOT_DRIVER(sandbox_power_domain_test) = {
>  	.id = UCLASS_MISC,
>  	.of_match = sandbox_power_domain_test_ids,
>  	.priv_auto	= sizeof(struct sandbox_power_domain_test),
> +	.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
>  };
> diff --git a/include/power-domain.h b/include/power-domain.h
> index 18525073e5e3534fcbac6fae4e18462f29a4dc49..ad33dea76ce5808beaa4fbf4388438a504e36027 100644
> --- a/include/power-domain.h
> +++ b/include/power-domain.h
> @@ -147,37 +147,81 @@ static inline int power_domain_free(struct power_domain *power_domain)
>  #endif
>  
>  /**
> - * power_domain_on - Enable power to a power domain.
> + * power_domain_on_lowlevel - Enable power to a power domain (with refcounting)
>   *
>   * @power_domain:	A power domain struct that was previously successfully
>   *		requested by power_domain_get().
> - * Return: 0 if OK, or a negative error code.
> + * Return: 0 if the transition has been performed correctly,
> + *         -EALREADY if the domain is already on,
> + *         a negative error code otherwise.
>   */
>  #if CONFIG_IS_ENABLED(POWER_DOMAIN)
> -int power_domain_on(struct power_domain *power_domain);
> +int power_domain_on_lowlevel(struct power_domain *power_domain);
>  #else
> -static inline int power_domain_on(struct power_domain *power_domain)
> +static inline int power_domain_on_lowlevel(struct power_domain *power_domain)
>  {
>  	return -ENOSYS;
>  }
>  #endif
>  
>  /**
> - * power_domain_off - Disable power to a power domain.
> + * power_domain_on - Enable power to a power domain (ignores the actual state
> + *		      of the power domain)
>   *
>   * @power_domain:	A power domain struct that was previously successfully
>   *		requested by power_domain_get().
> - * Return: 0 if OK, or a negative error code.
> + * Return: a negative error code upon error during the transition, 0 otherwise.
> + */
> +static inline int power_domain_on(struct power_domain *power_domain)
> +{
> +	int ret;
> +
> +	ret = power_domain_on_lowlevel(power_domain);
> +	if (ret == -EALREADY)
> +		ret = 0;
> +
> +	return ret;
> +}
> +
> +/**
> + * power_domain_off_lowlevel - Disable power to a power domain (with refcounting)
> + *
> + * @power_domain:	A power domain struct that was previously successfully
> + *		requested by power_domain_get().
> + * Return: 0 if the transition has been performed correctly,
> + *         -EALREADY if the domain is already off,
> + *         -EBUSY if another device is keeping the domain on (but the refcounter
> + *         is decremented),
> + *         a negative error code otherwise.
>   */
>  #if CONFIG_IS_ENABLED(POWER_DOMAIN)
> -int power_domain_off(struct power_domain *power_domain);
> +int power_domain_off_lowlevel(struct power_domain *power_domain);
>  #else
> -static inline int power_domain_off(struct power_domain *power_domain)
> +static inline int power_domain_off_lowlevel(struct power_domain *power_domain)
>  {
>  	return -ENOSYS;
>  }
>  #endif
>  
> +/**
> + * power_domain_off - Disable power to a power domain (ignores the actual state
> + *		      of the power domain)
> + *
> + * @power_domain:	A power domain struct that was previously successfully
> + *		requested by power_domain_get().
> + * Return: a negative error code upon error during the transition, 0 otherwise.
> + */
> +static inline int power_domain_off(struct power_domain *power_domain)
> +{
> +	int ret;
> +
> +	ret = power_domain_off_lowlevel(power_domain);
> +	if (ret == -EALREADY || ret == -EBUSY)
> +		ret = 0;
> +
> +	return ret;
> +}
> +
>  /**
>   * dev_power_domain_on - Enable power domains for a device .
>   *
> diff --git a/test/dm/power-domain.c b/test/dm/power-domain.c
> index 896cf5b2ae9d26701150fad70e888f8b135a22b0..8a95f6bdb903be9d1993528d87d5cae0075a83e4 100644
> --- a/test/dm/power-domain.c
> +++ b/test/dm/power-domain.c
> @@ -27,7 +27,7 @@ static int dm_test_power_domain(struct unit_test_state *uts)
>  
>  	ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "power-domain-test",
>  					      &dev_test));
> -	ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
> +	ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
>  						  TEST_POWER_DOMAIN));
>  	ut_assertok(sandbox_power_domain_test_get(dev_test));
>  
> 

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Neha Malcom Francis]

On 15/04/25 18:13, Miquel Raynal wrote:
> On 15/04/2025 at 15:29:09 +0530, Neha Malcom Francis <n-francis@ti.com> wrote:
> 
>> Hi Miquel
>>
>> On 15/04/25 15:20, Miquel Raynal wrote:
>>>>> Francesco, are you also testing on K3 platforms?
>>>>>
>>>>
>>>> Yes he would be, since the firmware print is K3 firmware.
>>>>
>>>>> Can one of you boot with the patch below applied? It should partially
>>>>> revert the commit to the ancient behaviour, while adding more debug
>>>>> traces (please enable the debug logs as well). This should clarify
>>>>> which one of the 3 different path is likely failing. It should also help
>>>>> identify who's the user that fails to enable/disable its own power
>>>>> domain. I will need to know what board/SoC was used for the test, so I
>>>>> can look the relevant driver up.
>>>>
>>>> Booted on j784s4_evm
>>>>
>>>> https://gist.github.com/nehamalcom/b09687a523bec89f9df3537fdd99b6f3
>>
>> ^
> 
> Ah, sorry! This is very interesting.
> 
> There are two things which I do not understand:
> - if we look at the very first power domains, their id is 15 and 14
>   respectively. I do not see these numbers in any device tree, am I
>   looking incorrectly? Could you help me identify what are the devices
>   requesting these IDs?

Yeah this might have been confusing. We have device IDs which are what
is mentioned in the device tree mapped to the power domain (LPSC) IDs
via arch/arm/mach-k3/r5/j784s4/dev-data.c which is consumed by a
rudimentary power domain driver ti-power-domain.c

I have enabled a debug log in ti-power-domain.c so you can see what is
the device ID that is eventually mapped to it's corresponding power
domain ID. So from:

ti_power_domain_of_xlate(power_domain=41c65ecc, id=191)
power_domain_on_lowlevel(power_domain=41c65ecc, id 15)

You can see that LPSC 15 was mapped from dev ID 191 which is from
memorycontroller@2990000. Hope this helps.

> - all power domains appear to be already enabled the very first time we
>   enable them. I believe this is the root cause of the problem, as they
>   should have count_on to 0 when booting. I checked again,

We're seeing only the logs after console is up. We are definitely
enabling up some power domains for the first time prior to the very
first print.

>   'per_device_auto' is a field that is zeroed when allocated (using
>   calloc() in this case) so I am really puzzled about this. It looks
>   like "something else" is using this data field, which would be
>   wrong.
> 
>>>>
>>>> Currently debugging on my build where I hang in console_init itself, I
>>>> think the path for failure is different here, will confirm.
>>>
>>> Thanks a lot for your feedback. Normally by applying the diff shared in
>>> my previous e-mail, you should have the console back. If not, maybe
>>> there is something else involved and the blamed commit is just the first
>>> showing an underlying problem.
>>
>> I do have console back with that patch, see the gist link above. What I
>> meant was the path of failure seems to be different, it's looking like
>> with my version of firmware the serial driver probe
>> (ns16550_serial_probe) tries to do a readb() and fails, this could
>> possibly be because the device was not powered on to begin with. Whereas
>> in another case (what Francesco is seeing), this issue is not run
>> into.
> 
> Clear.
> 
> Could you make another round with this extra change?

https://gist.github.com/nehamalcom/6772169d2b26d89ddcdeaf6049a8ff8f

This includes the debug logs you've provided as well as the debug log I
added as mentioned above.

Maybe I need to go over the power framework in detail, but it seems like
we (K3 devices) have a singular power-controller node that internally
goes ahead and turns on the respective power domain via our
ti,sci-pm-domain drivers. So the power-domain uclass sees only one power
domain i.e. power-controller and keeps incrementing it's on_count (see
the logs, I debugged even before the prints come we start from 0 and
keep incrementing the same power-controller's on_count). But maybe what
you expect when you wrote this patch is something similar to the
fsl,imx8mq-gpc drivers where each power-domain is a device node with
it's own on_count (see arm64/freescale/imx8mq.dtsi)?


> 
> Thanks!
> Miquèl
> 
> --- a/drivers/power/domain/power-domain-uclass.c
> +++ b/drivers/power/domain/power-domain-uclass.c
> @@ -119,14 +119,18 @@ int power_domain_on_lowlevel(struct power_domain *power_domain)
>  
>         debug("%s(power_domain=%p, id %d)\n", __func__, power_domain, power_domain->id);
>  
> -       if (priv->on_count++ > 0) {
> -               debug("Power domain %s already on.\n", power_domain->dev->name);
> +       if (priv->on_count > 0) {
> +               debug("Power domain %s already on (usage count: %d).\n", power_domain->dev->name, priv->on_count);
> +               priv->on_count++;
>                 //return -EALREADY;
> +       } else {
> +               priv->on_count++;
>         }
>  
>         ret = ops->on ? ops->on(power_domain) : 0;
>         if (ret) {
>                 priv->on_count--;
> +               debug("Power domain %s on failed (usage count: %d).\n", power_domain->dev->name, priv->on_count);
>                 return ret;
>         }
>  

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Neha Malcom Francis]

Hi Samuel

On 16/04/25 06:44, Samuel Holland wrote:
> Hi Miquel,
> 
> On 2025-04-03 2:39 AM, Miquel Raynal wrote:
>> It is very surprising that such an uclass, specifically designed to
>> handle resources that may be shared by different devices, is not keeping
>> the count of the number of times a power domain has been
>> enabled/disabled to avoid shutting it down unexpectedly or disabling it
>> several times.
>>
>> Doing this causes troubles on eg. i.MX8MP because disabling power
>> domains can be done in recursive loops were the same power domain
>> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
>> timings to respect and it is easy to produce a race condition that puts
>> the power domains in an unstable state, leading to ADB400 errors and
>> later crashes in Linux.
>>
>> CI tests using power domains are slightly updated to make sure the count
>> of on/off calls is even and the results match what we *now* expect.
>>
>> As we do not want to break existing users while stile getting
>> interesting error codes, the implementation is split between:
>> - a low-level helper reporting error codes if the requested transition
>>   could not be operated,
>> - a higher-level helper ignoring the "non error" codes, like EALREADY and
>>   EBUSY.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  drivers/firmware/scmi/sandbox-scmi_devices.c     |  1 +
>>  drivers/power/domain/power-domain-uclass.c       | 40 ++++++++++++++--
>>  drivers/power/domain/sandbox-power-domain-test.c |  1 +
>>  include/power-domain.h                           | 60 ++++++++++++++++++++----
>>  test/dm/power-domain.c                           |  2 +-
>>  5 files changed, 91 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
>> index 96c2922b067e2886b3fa963bcd7e396f4569a569..9f253b0fd40f703a5ec11d34c197423d27ad8b01 100644
>> --- a/drivers/firmware/scmi/sandbox-scmi_devices.c
>> +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
>> @@ -163,4 +163,5 @@ U_BOOT_DRIVER(sandbox_scmi_devices) = {
>>  	.priv_auto = sizeof(struct sandbox_scmi_device_priv),
>>  	.remove = sandbox_scmi_devices_remove,
>>  	.probe = sandbox_scmi_devices_probe,
>> +	.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
>>  };
>> diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
>> index 938bd8cbc9ffd1ba2109d702f886b6a99288d063..a6e5f9ed0369eb9d2dfa66edc9e938bac6720dab 100644
>> --- a/drivers/power/domain/power-domain-uclass.c
>> +++ b/drivers/power/domain/power-domain-uclass.c
>> @@ -12,6 +12,10 @@
>>  #include <power-domain-uclass.h>
>>  #include <dm/device-internal.h>
>>  
>> +struct power_domain_priv {
>> +	int on_count;
>> +};
>> +
>>  static inline struct power_domain_ops *power_domain_dev_ops(struct udevice *dev)
>>  {
>>  	return (struct power_domain_ops *)dev->driver->ops;
>> @@ -107,22 +111,49 @@ int power_domain_free(struct power_domain *power_domain)
>>  	return ops->rfree ? ops->rfree(power_domain) : 0;
>>  }
>>  
>> -int power_domain_on(struct power_domain *power_domain)
>> +int power_domain_on_lowlevel(struct power_domain *power_domain)
>>  {
>> +	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>>  	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
>> +	int ret;
>>  
>>  	debug("%s(power_domain=%p)\n", __func__, power_domain);
>>  
>> -	return ops->on ? ops->on(power_domain) : 0;
>> +	if (priv->on_count++ > 0)
>> +		return -EALREADY;
> 
> This change is broken for power domain providers with #power-domain-cells = <1>,
> which can have multiple domains per provider device. There would need to be a
> separate reference count per domain, and currently the uclass doesn't know the
> range of valid domain IDs.

I didn't see this reply earlier, would've saved some time debugging to
come to the same conclusion :) but yes this is the reason for breaking.

> 
> Regards,
> Samuel
> 
>> +
>> +	ret = ops->on ? ops->on(power_domain) : 0;
>> +	if (ret) {
>> +		priv->on_count--;
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>  }
>>  
>> -int power_domain_off(struct power_domain *power_domain)
>> +int power_domain_off_lowlevel(struct power_domain *power_domain)
>>  {
>> +	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>>  	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
>> +	int ret;
>>  
>>  	debug("%s(power_domain=%p)\n", __func__, power_domain);
>>  
>> -	return ops->off ? ops->off(power_domain) : 0;
>> +	if (priv->on_count <= 0) {
>> +		debug("Power domain %s already off.\n", power_domain->dev->name);
>> +		return -EALREADY;
>> +	}
>> +
>> +	if (priv->on_count-- > 1)
>> +		return -EBUSY;
>> +
>> +	ret = ops->off ? ops->off(power_domain) : 0;
>> +	if (ret) {
>> +		priv->on_count++;
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>  }
>>  
>>  #if CONFIG_IS_ENABLED(OF_REAL)
>> @@ -180,4 +211,5 @@ int dev_power_domain_off(struct udevice *dev)
>>  UCLASS_DRIVER(power_domain) = {
>>  	.id		= UCLASS_POWER_DOMAIN,
>>  	.name		= "power_domain",
>> +	.per_device_auto = sizeof(struct power_domain_priv),
>>  };
>> diff --git a/drivers/power/domain/sandbox-power-domain-test.c b/drivers/power/domain/sandbox-power-domain-test.c
>> index 08c15ef342b3dd3ce01807ee59b7e97337f7dde5..5b530974e942ffcba453e53be330baaf3a113a13 100644
>> --- a/drivers/power/domain/sandbox-power-domain-test.c
>> +++ b/drivers/power/domain/sandbox-power-domain-test.c
>> @@ -51,4 +51,5 @@ U_BOOT_DRIVER(sandbox_power_domain_test) = {
>>  	.id = UCLASS_MISC,
>>  	.of_match = sandbox_power_domain_test_ids,
>>  	.priv_auto	= sizeof(struct sandbox_power_domain_test),
>> +	.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
>>  };
>> diff --git a/include/power-domain.h b/include/power-domain.h
>> index 18525073e5e3534fcbac6fae4e18462f29a4dc49..ad33dea76ce5808beaa4fbf4388438a504e36027 100644
>> --- a/include/power-domain.h
>> +++ b/include/power-domain.h
>> @@ -147,37 +147,81 @@ static inline int power_domain_free(struct power_domain *power_domain)
>>  #endif
>>  
>>  /**
>> - * power_domain_on - Enable power to a power domain.
>> + * power_domain_on_lowlevel - Enable power to a power domain (with refcounting)
>>   *
>>   * @power_domain:	A power domain struct that was previously successfully
>>   *		requested by power_domain_get().
>> - * Return: 0 if OK, or a negative error code.
>> + * Return: 0 if the transition has been performed correctly,
>> + *         -EALREADY if the domain is already on,
>> + *         a negative error code otherwise.
>>   */
>>  #if CONFIG_IS_ENABLED(POWER_DOMAIN)
>> -int power_domain_on(struct power_domain *power_domain);
>> +int power_domain_on_lowlevel(struct power_domain *power_domain);
>>  #else
>> -static inline int power_domain_on(struct power_domain *power_domain)
>> +static inline int power_domain_on_lowlevel(struct power_domain *power_domain)
>>  {
>>  	return -ENOSYS;
>>  }
>>  #endif
>>  
>>  /**
>> - * power_domain_off - Disable power to a power domain.
>> + * power_domain_on - Enable power to a power domain (ignores the actual state
>> + *		      of the power domain)
>>   *
>>   * @power_domain:	A power domain struct that was previously successfully
>>   *		requested by power_domain_get().
>> - * Return: 0 if OK, or a negative error code.
>> + * Return: a negative error code upon error during the transition, 0 otherwise.
>> + */
>> +static inline int power_domain_on(struct power_domain *power_domain)
>> +{
>> +	int ret;
>> +
>> +	ret = power_domain_on_lowlevel(power_domain);
>> +	if (ret == -EALREADY)
>> +		ret = 0;
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * power_domain_off_lowlevel - Disable power to a power domain (with refcounting)
>> + *
>> + * @power_domain:	A power domain struct that was previously successfully
>> + *		requested by power_domain_get().
>> + * Return: 0 if the transition has been performed correctly,
>> + *         -EALREADY if the domain is already off,
>> + *         -EBUSY if another device is keeping the domain on (but the refcounter
>> + *         is decremented),
>> + *         a negative error code otherwise.
>>   */
>>  #if CONFIG_IS_ENABLED(POWER_DOMAIN)
>> -int power_domain_off(struct power_domain *power_domain);
>> +int power_domain_off_lowlevel(struct power_domain *power_domain);
>>  #else
>> -static inline int power_domain_off(struct power_domain *power_domain)
>> +static inline int power_domain_off_lowlevel(struct power_domain *power_domain)
>>  {
>>  	return -ENOSYS;
>>  }
>>  #endif
>>  
>> +/**
>> + * power_domain_off - Disable power to a power domain (ignores the actual state
>> + *		      of the power domain)
>> + *
>> + * @power_domain:	A power domain struct that was previously successfully
>> + *		requested by power_domain_get().
>> + * Return: a negative error code upon error during the transition, 0 otherwise.
>> + */
>> +static inline int power_domain_off(struct power_domain *power_domain)
>> +{
>> +	int ret;
>> +
>> +	ret = power_domain_off_lowlevel(power_domain);
>> +	if (ret == -EALREADY || ret == -EBUSY)
>> +		ret = 0;
>> +
>> +	return ret;
>> +}
>> +
>>  /**
>>   * dev_power_domain_on - Enable power domains for a device .
>>   *
>> diff --git a/test/dm/power-domain.c b/test/dm/power-domain.c
>> index 896cf5b2ae9d26701150fad70e888f8b135a22b0..8a95f6bdb903be9d1993528d87d5cae0075a83e4 100644
>> --- a/test/dm/power-domain.c
>> +++ b/test/dm/power-domain.c
>> @@ -27,7 +27,7 @@ static int dm_test_power_domain(struct unit_test_state *uts)
>>  
>>  	ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "power-domain-test",
>>  					      &dev_test));
>> -	ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
>> +	ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
>>  						  TEST_POWER_DOMAIN));
>>  	ut_assertok(sandbox_power_domain_test_get(dev_test));
>>  
>>
> 

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Miquel Raynal]

Hello,

>>> -int power_domain_on(struct power_domain *power_domain)
>>> +int power_domain_on_lowlevel(struct power_domain *power_domain)
>>>  {
>>> +	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>>>  	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
>>> +	int ret;
>>>  
>>>  	debug("%s(power_domain=%p)\n", __func__, power_domain);
>>>  
>>> -	return ops->on ? ops->on(power_domain) : 0;
>>> +	if (priv->on_count++ > 0)
>>> +		return -EALREADY;
>> 
>> This change is broken for power domain providers with #power-domain-cells = <1>,
>> which can have multiple domains per provider device. There would need to be a
>> separate reference count per domain, and currently the uclass doesn't know the
>> range of valid domain IDs.
>
> I didn't see this reply earlier, would've saved some time debugging to
> come to the same conclusion :) but yes this is the reason for
> breaking.

That's indeed the reason, thanks a lot for figuring this out. I am
looking for a solution. I can reproduce on imx8mp by enabling the two
LCD interfaces, as they have a similar pattern as on k3 platform: a
single power domain node and one cell for figuring out which PD to
enable.

The uclass does not save any data, so I don't have an immediate fix to
propose. Let me dig a bit more into that and find a solution.

Thanks,
Miquèl

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Neha Malcom Francis]

Hi Miquel

On 16/04/25 14:36, Miquel Raynal wrote:
> Hello,
> 
>>>> -int power_domain_on(struct power_domain *power_domain)
>>>> +int power_domain_on_lowlevel(struct power_domain *power_domain)
>>>>  {
>>>> +	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>>>>  	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
>>>> +	int ret;
>>>>  
>>>>  	debug("%s(power_domain=%p)\n", __func__, power_domain);
>>>>  
>>>> -	return ops->on ? ops->on(power_domain) : 0;
>>>> +	if (priv->on_count++ > 0)
>>>> +		return -EALREADY;
>>>
>>> This change is broken for power domain providers with #power-domain-cells = <1>,
>>> which can have multiple domains per provider device. There would need to be a
>>> separate reference count per domain, and currently the uclass doesn't know the
>>> range of valid domain IDs.
>>
>> I didn't see this reply earlier, would've saved some time debugging to
>> come to the same conclusion :) but yes this is the reason for
>> breaking.
> 
> That's indeed the reason, thanks a lot for figuring this out. I am
> looking for a solution. I can reproduce on imx8mp by enabling the two
> LCD interfaces, as they have a similar pattern as on k3 platform: a
> single power domain node and one cell for figuring out which PD to
> enable.
> 
> The uclass does not save any data, so I don't have an immediate fix to
> propose. Let me dig a bit more into that and find a solution.
> 

Thanks!

Meanwhile, could we revert this patch to keep the platforms from breaking?


> Thanks,
> Miquèl

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Wadim Egorov]

Am 16.04.25 um 12:50 schrieb Neha Malcom Francis:
> Hi Miquel
> 
> On 16/04/25 14:36, Miquel Raynal wrote:
>> Hello,
>>
>>>>> -int power_domain_on(struct power_domain *power_domain)
>>>>> +int power_domain_on_lowlevel(struct power_domain *power_domain)
>>>>>   {
>>>>> +	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>>>>>   	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
>>>>> +	int ret;
>>>>>   
>>>>>   	debug("%s(power_domain=%p)\n", __func__, power_domain);
>>>>>   
>>>>> -	return ops->on ? ops->on(power_domain) : 0;
>>>>> +	if (priv->on_count++ > 0)
>>>>> +		return -EALREADY;
>>>>
>>>> This change is broken for power domain providers with #power-domain-cells = <1>,
>>>> which can have multiple domains per provider device. There would need to be a
>>>> separate reference count per domain, and currently the uclass doesn't know the
>>>> range of valid domain IDs.
>>>
>>> I didn't see this reply earlier, would've saved some time debugging to
>>> come to the same conclusion :) but yes this is the reason for
>>> breaking.
>>
>> That's indeed the reason, thanks a lot for figuring this out. I am
>> looking for a solution. I can reproduce on imx8mp by enabling the two
>> LCD interfaces, as they have a similar pattern as on k3 platform: a
>> single power domain node and one cell for figuring out which PD to
>> enable.
>>
>> The uclass does not save any data, so I don't have an immediate fix to
>> propose. Let me dig a bit more into that and find a solution.
>>
> 
> Thanks!
> 
> Meanwhile, could we revert this patch to keep the platforms from breaking?

Would be nice, just took me the same route to pin this commit and find 
out about this discussion. I think more people will run into the same 
issue soon

> 
> 
>> Thanks,
>> Miquèl
> 

Re: [PATCH v6 05/12] power-domain: Add refcounting

[Miquel Raynal]

On 16/04/2025 at 16:20:06 +03, Wadim Egorov <w.egorov@phytec.de> wrote:

> Am 16.04.25 um 12:50 schrieb Neha Malcom Francis:
>> Hi Miquel
>> On 16/04/25 14:36, Miquel Raynal wrote:
>>> Hello,
>>>
>>>>>> -int power_domain_on(struct power_domain *power_domain)
>>>>>> +int power_domain_on_lowlevel(struct power_domain *power_domain)
>>>>>>   {
>>>>>> +	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>>>>>>   	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
>>>>>> +	int ret;
>>>>>>     	debug("%s(power_domain=%p)\n", __func__, power_domain);
>>>>>>   -	return ops->on ? ops->on(power_domain) : 0;
>>>>>> +	if (priv->on_count++ > 0)
>>>>>> +		return -EALREADY;
>>>>>
>>>>> This change is broken for power domain providers with #power-domain-cells = <1>,
>>>>> which can have multiple domains per provider device. There would need to be a
>>>>> separate reference count per domain, and currently the uclass doesn't know the
>>>>> range of valid domain IDs.
>>>>
>>>> I didn't see this reply earlier, would've saved some time debugging to
>>>> come to the same conclusion :) but yes this is the reason for
>>>> breaking.
>>>
>>> That's indeed the reason, thanks a lot for figuring this out. I am
>>> looking for a solution. I can reproduce on imx8mp by enabling the two
>>> LCD interfaces, as they have a similar pattern as on k3 platform: a
>>> single power domain node and one cell for figuring out which PD to
>>> enable.
>>>
>>> The uclass does not save any data, so I don't have an immediate fix to
>>> propose. Let me dig a bit more into that and find a solution.
>>>
>> Thanks!
>> Meanwhile, could we revert this patch to keep the platforms from
>> breaking?
>
> Would be nice, just took me the same route to pin this commit and find
> out about this discussion. I think more people will run into the same
> issue soon

Go ahead, unfortunately, it's not straightforward to fix properly.

Thanks,
Miquèl