[PATCH 0/6] media: rkisp1: Prepare for HDR stitching support

[PATCH 0/6] media: rkisp1: Prepare for HDR stitching support

[Laurent Pinchart]

Hello everybody,

This patch series prepares the rkisp1 driver for HDR stitching support
by adding a new clock and power domain.

The ISP instance integrated in the NXP i.MX8MP includes an HDR stitching
module. Unlike other ISP modules, the HDR stitching module requires the
pixel clock to be enabled to access control registers, otherwise the
system freezes. To make the problem more complex, the pixel clock is
gated by the media-blk, which controls the clock gate through the
MIPI_CSI2 power domain.

Adding the pixel clock to the ISP DT node is easy, but enabling the gate
as part of the ISP power domain would be more difficult. This series
instead adds the MIPI_CSI2 power domain as a secondary power domain for
the ISP. Given that the ISP can't be used without the CSI-2 receiver in
the i.MX8MP, this won't result in extra power consumption. Lucas, your
feedback on this approach would be appreciated.

Patches 1/6 and 2/6 update the DT binding to add the clock and power
domain. Patches 3/6 then refactors clock handling in the rkisp1 driver,
and patches 4/6 and 5/6 add support for the additional clock and power
domain. They are optional to avoid breaking backward compatibility with
older device trees. Finally, patch 6/6 updates imx8mp.dtsi to add the
clock and power domain.

The series has been tested by trying to read the HDR stitching registers
at probe time. Without these changes the system locks up, with this
series applied the registers read correctly.

Laurent Pinchart (6):
  dt-bindings: media: rkisp1: Require pclk clock on i.MX8MP variant
  dt-bindings: media: rkisp1: Add second power domain on i.MX8MP
  media: rkisp1: Refactor clocks initialization
  media: rkisp1: Acquire pclk clock on i.MX8MP
  media: rkisp1: Add support for multiple power domains
  arm64: dts: imx8mp: Add pclk clock and second power domain for the ISP

 .../bindings/media/rockchip-isp1.yaml         |  23 +++-
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     |  18 ++-
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  17 ++-
 .../platform/rockchip/rkisp1/rkisp1-dev.c     | 123 +++++++++++++-----
 4 files changed, 140 insertions(+), 41 deletions(-)


base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
-- 
Regards,

Laurent Pinchart

[PATCH 1/6] dt-bindings: media: rkisp1: Require pclk clock on i.MX8MP variant

[Laurent Pinchart]

The ISP integrated in the NXP i.MX8MP requires the pclk clock to access
the HDR stitching registers. Make it mandatory in the DT binding.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/devicetree/bindings/media/rockchip-isp1.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
index 6be00aca4181..6b360cbb42e0 100644
--- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
@@ -155,6 +155,10 @@ allOf:
             const: fsl,imx8mp-isp
     then:
       properties:
+        clocks:
+          minItems: 4
+        clock-names:
+          minItems: 4
         iommus: false
         phys: false
         phy-names: false
-- 
Regards,

Laurent Pinchart

[PATCH 2/6] dt-bindings: media: rkisp1: Add second power domain on i.MX8MP

[Laurent Pinchart]

In the NXP i.MX8MP, the pclk clock required by the ISP is gated by the
MIPI CSI-2 power domain. Add it to the power-domains property, and
require specifying power-domain-names accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../bindings/media/rockchip-isp1.yaml         | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
index 6b360cbb42e0..477c21417e75 100644
--- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
@@ -71,7 +71,16 @@ properties:
     const: dphy
 
   power-domains:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: ISP power domain
+      - description: MIPI CSI-2 power domain
+
+  power-domain-names:
+    minItems: 1
+    items:
+      - const: isp
+      - const: csi2
 
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
@@ -162,11 +171,19 @@ allOf:
         iommus: false
         phys: false
         phy-names: false
+        power-domains:
+          minItems: 2
+        power-domain-names:
+          minItems: 2
       required:
         - fsl,blk-ctrl
+        - power-domain-names
     else:
       properties:
         fsl,blk-ctrl: false
+        power-domains:
+          maxItems: 1
+        power-domain-names: false
       required:
         - iommus
         - phys
-- 
Regards,

Laurent Pinchart

[PATCH 3/6] media: rkisp1: Refactor clocks initialization

[Laurent Pinchart]

ISP instances in different SoCs differ in the number of clocks they use,
but not in the clock names. Refactor clocks initialization to avoid
duplicating the clock names per platform, and lower the total number of
clocks from 8 to 4 as no platform uses more than 4 clocks. Use a static
assert to ensure at build time that the size of the arrays match.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  8 +--
 .../platform/rockchip/rkisp1/rkisp1-dev.c     | 60 ++++++++++---------
 2 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index ca952fd0829b..c4af1277fc6b 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -55,7 +55,7 @@ struct regmap;
 #define RKISP1_BUS_INFO				"platform:" RKISP1_DRIVER_NAME
 
 /* maximum number of clocks */
-#define RKISP1_MAX_BUS_CLK			8
+#define RKISP1_MAX_BUS_CLK			4
 
 /* a bitmask of the ready stats */
 #define RKISP1_STATS_MEAS_MASK			(RKISP1_CIF_ISP_AWB_DONE |	\
@@ -139,8 +139,7 @@ enum rkisp1_feature {
 /*
  * struct rkisp1_info - Model-specific ISP Information
  *
- * @clks: array of ISP clock names
- * @clk_size: number of entries in the @clks array
+ * @num_clocks: number of clocks
  * @isrs: array of ISP interrupt descriptors
  * @isr_size: number of entries in the @isrs array
  * @isp_ver: ISP version
@@ -152,8 +151,7 @@ enum rkisp1_feature {
  * ISP model, version, or integration in a particular SoC.
  */
 struct rkisp1_info {
-	const char * const *clks;
-	unsigned int clk_size;
+	unsigned int num_clocks;
 	const struct rkisp1_isr_data *isrs;
 	unsigned int isr_size;
 	enum rkisp1_cif_isp_version isp_ver;
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index dc65a7924f8a..0788b7a64ae9 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
  */
 
+#include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/interrupt.h>
 #include <linux/mfd/syscon.h>
@@ -491,13 +492,6 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
 	return ret;
 }
 
-static const char * const px30_isp_clks[] = {
-	"isp",
-	"aclk",
-	"hclk",
-	"pclk",
-};
-
 static const struct rkisp1_isr_data px30_isp_isrs[] = {
 	{ "isp", rkisp1_isp_isr, BIT(RKISP1_IRQ_ISP) },
 	{ "mi", rkisp1_capture_isr, BIT(RKISP1_IRQ_MI) },
@@ -505,8 +499,7 @@ static const struct rkisp1_isr_data px30_isp_isrs[] = {
 };
 
 static const struct rkisp1_info px30_isp_info = {
-	.clks = px30_isp_clks,
-	.clk_size = ARRAY_SIZE(px30_isp_clks),
+	.num_clocks = 4,
 	.isrs = px30_isp_isrs,
 	.isr_size = ARRAY_SIZE(px30_isp_isrs),
 	.isp_ver = RKISP1_V12,
@@ -518,19 +511,12 @@ static const struct rkisp1_info px30_isp_info = {
 	.max_height = 2448,
 };
 
-static const char * const rk3399_isp_clks[] = {
-	"isp",
-	"aclk",
-	"hclk",
-};
-
 static const struct rkisp1_isr_data rk3399_isp_isrs[] = {
 	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) | BIT(RKISP1_IRQ_MIPI) },
 };
 
 static const struct rkisp1_info rk3399_isp_info = {
-	.clks = rk3399_isp_clks,
-	.clk_size = ARRAY_SIZE(rk3399_isp_clks),
+	.num_clocks = 3,
 	.isrs = rk3399_isp_isrs,
 	.isr_size = ARRAY_SIZE(rk3399_isp_isrs),
 	.isp_ver = RKISP1_V10,
@@ -542,19 +528,12 @@ static const struct rkisp1_info rk3399_isp_info = {
 	.max_height = 3312,
 };
 
-static const char * const imx8mp_isp_clks[] = {
-	"isp",
-	"hclk",
-	"aclk",
-};
-
 static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
 	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) },
 };
 
 static const struct rkisp1_info imx8mp_isp_info = {
-	.clks = imx8mp_isp_clks,
-	.clk_size = ARRAY_SIZE(imx8mp_isp_clks),
+	.num_clocks = 3,
 	.isrs = imx8mp_isp_isrs,
 	.isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
 	.isp_ver = RKISP1_V_IMX8MP,
@@ -582,6 +561,32 @@ static const struct of_device_id rkisp1_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, rkisp1_of_match);
 
+static const char * const rkisp1_clk_names[] = {
+	"isp",
+	"aclk",
+	"hclk",
+	"pclk",
+};
+
+static int rkisp1_init_clocks(struct rkisp1_device *rkisp1)
+{
+	const struct rkisp1_info *info = rkisp1->info;
+	unsigned int i;
+	int ret;
+
+	static_assert(ARRAY_SIZE(rkisp1_clk_names) == ARRAY_SIZE(rkisp1->clks));
+
+	for (i = 0; i < info->num_clocks; i++)
+		rkisp1->clks[i].id = rkisp1_clk_names[i];
+
+	ret = devm_clk_bulk_get(rkisp1->dev, info->num_clocks, rkisp1->clks);
+	if (ret)
+		return ret;
+
+	rkisp1->clk_size = info->num_clocks;
+	return 0;
+}
+
 static int rkisp1_probe(struct platform_device *pdev)
 {
 	const struct rkisp1_info *info;
@@ -639,12 +644,9 @@ static int rkisp1_probe(struct platform_device *pdev)
 		}
 	}
 
-	for (i = 0; i < info->clk_size; i++)
-		rkisp1->clks[i].id = info->clks[i];
-	ret = devm_clk_bulk_get(dev, info->clk_size, rkisp1->clks);
+	ret = rkisp1_init_clocks(rkisp1);
 	if (ret)
 		return ret;
-	rkisp1->clk_size = info->clk_size;
 
 	if (info->isp_ver == RKISP1_V_IMX8MP) {
 		unsigned int id;
-- 
Regards,

Laurent Pinchart

[PATCH 4/6] media: rkisp1: Acquire pclk clock on i.MX8MP

[Laurent Pinchart]

The ISP instances in the NXP i.MX8MP need the input pixel clock to be
enabled in order to access the HDR stitching registers. The clock should
ideally be mandatory, but that would break backward compatibility with
old DT. Try to acquire it as an optional clock instead.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-dev.c      | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index 0788b7a64ae9..fb4ccf497bad 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -584,6 +584,24 @@ static int rkisp1_init_clocks(struct rkisp1_device *rkisp1)
 		return ret;
 
 	rkisp1->clk_size = info->num_clocks;
+
+	/*
+	 * On i.MX8MP the pclk clock is needed to access the HDR stitching
+	 * registers, but wasn't required by DT bindings. Try to acquire it as
+	 * an optional clock to avoid breaking backward compatibility.
+	 */
+	if (info->isp_ver == RKISP1_V_IMX8MP) {
+		struct clk *clk;
+
+		clk = devm_clk_get_optional(rkisp1->dev, "pclk");
+		if (IS_ERR(clk))
+			return dev_err_probe(rkisp1->dev, PTR_ERR(clk),
+					     "Failed to acquire pclk clock\n");
+
+		if (clk)
+			rkisp1->clks[rkisp1->clk_size++].clk = clk;
+	}
+
 	return 0;
 }
 
-- 
Regards,

Laurent Pinchart

[PATCH 5/6] media: rkisp1: Add support for multiple power domains

[Laurent Pinchart]

The ISP instances in the NXP i.MX8MP need two power domains. While
single power domains are managed automatically by the device core,
support for multiple power domains requires manually attaching to the
power domains. Do so based on platform data.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  9 ++++
 .../platform/rockchip/rkisp1/rkisp1-dev.c     | 45 +++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index c4af1277fc6b..ce900440c78d 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -24,6 +24,7 @@
 #include "rkisp1-regs.h"
 
 struct dentry;
+struct dev_pm_domain_list;
 struct regmap;
 
 /*
@@ -146,6 +147,8 @@ enum rkisp1_feature {
  * @features: bitmask of rkisp1_feature features implemented by the ISP
  * @max_width: maximum input frame width
  * @max_height: maximum input frame height
+ * @pm_domains.names: name of the power domains
+ * @pm_domains.count: number of power domains
  *
  * This structure contains information about the ISP specific to a particular
  * ISP model, version, or integration in a particular SoC.
@@ -158,6 +161,10 @@ struct rkisp1_info {
 	unsigned int features;
 	unsigned int max_width;
 	unsigned int max_height;
+	struct {
+		const char * const *names;
+		unsigned int count;
+	} pm_domains;
 };
 
 /*
@@ -479,6 +486,7 @@ struct rkisp1_debug {
  * @dev:	   a pointer to the struct device
  * @clk_size:	   number of clocks
  * @clks:	   array of clocks
+ * @pm_domains:    power domains
  * @gasket:	   the gasket - i.MX8MP only
  * @gasket_id:	   the gasket ID (0 or 1) - i.MX8MP only
  * @v4l2_dev:	   v4l2_device variable
@@ -503,6 +511,7 @@ struct rkisp1_device {
 	struct device *dev;
 	unsigned int clk_size;
 	struct clk_bulk_data clks[RKISP1_MAX_BUS_CLK];
+	struct dev_pm_domain_list *pm_domains;
 	struct regmap *gasket;
 	unsigned int gasket_id;
 	struct v4l2_device v4l2_dev;
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index fb4ccf497bad..1791c02a40ae 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -17,6 +17,7 @@
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-mc.h>
@@ -532,6 +533,11 @@ static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
 	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) },
 };
 
+static const char * const imx8mp_isp_pm_domains[] = {
+	"isp",
+	"csi2",
+};
+
 static const struct rkisp1_info imx8mp_isp_info = {
 	.num_clocks = 3,
 	.isrs = imx8mp_isp_isrs,
@@ -542,6 +548,10 @@ static const struct rkisp1_info imx8mp_isp_info = {
 		  | RKISP1_FEATURE_COMPAND,
 	.max_width = 4096,
 	.max_height = 3072,
+	.pm_domains = {
+		.names = imx8mp_isp_pm_domains,
+		.count = ARRAY_SIZE(imx8mp_isp_pm_domains),
+	},
 };
 
 static const struct of_device_id rkisp1_of_match[] = {
@@ -605,6 +615,37 @@ static int rkisp1_init_clocks(struct rkisp1_device *rkisp1)
 	return 0;
 }
 
+static int rkisp1_init_pm_domains(struct rkisp1_device *rkisp1)
+{
+	const struct rkisp1_info *info = rkisp1->info;
+	struct dev_pm_domain_attach_data pm_domain_data = {
+		.pd_names = info->pm_domains.names,
+		.num_pd_names = info->pm_domains.count,
+	};
+	int ret;
+
+	/*
+	 * Most platforms have a single power domain, which the PM domain core
+	 * automatically attaches at probe time. When that's the case there's
+	 * nothing to do here.
+	 */
+	if (rkisp1->dev->pm_domain)
+		return 0;
+
+	if (!pm_domain_data.num_pd_names)
+		return 0;
+
+	ret = devm_pm_domain_attach_list(rkisp1->dev, &pm_domain_data,
+					 &rkisp1->pm_domains);
+	if (ret < 0) {
+		dev_err_probe(rkisp1->dev, ret,
+			      "Failed to attach power domains\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int rkisp1_probe(struct platform_device *pdev)
 {
 	const struct rkisp1_info *info;
@@ -666,6 +707,10 @@ static int rkisp1_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = rkisp1_init_pm_domains(rkisp1);
+	if (ret)
+		return ret;
+
 	if (info->isp_ver == RKISP1_V_IMX8MP) {
 		unsigned int id;
 
-- 
Regards,

Laurent Pinchart

[PATCH 6/6] arm64: dts: imx8mp: Add pclk clock and second power domain for the ISP

[Laurent Pinchart]

The ISP HDR stitching registers are clocked by the pixel clock, which is
gated by the MIPI_CSI2 power domain. Attempting to access those
registers with the clock off locks up the system. Fix this by adding the
pclk clock and the MIPI_CSI2 secondary power domain.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index c409a1d1e851..223f956b5442 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1704,9 +1704,12 @@ isp_0: isp@32e10000 {
 				interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>,
 					 <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>,
-					 <&clk IMX8MP_CLK_MEDIA_APB_ROOT>;
-				clock-names = "isp", "aclk", "hclk";
-				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>;
+					 <&clk IMX8MP_CLK_MEDIA_APB_ROOT>,
+					 <&clk IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT>;
+				clock-names = "isp", "aclk", "hclk", "pclk";
+				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>,
+						<&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
+				power-domain-names = "isp", "csi2";
 				fsl,blk-ctrl = <&media_blk_ctrl 0>;
 				status = "disabled";
 
@@ -1726,9 +1729,12 @@ isp_1: isp@32e20000 {
 				interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>,
 					 <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>,
-					 <&clk IMX8MP_CLK_MEDIA_APB_ROOT>;
-				clock-names = "isp", "aclk", "hclk";
-				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>;
+					 <&clk IMX8MP_CLK_MEDIA_APB_ROOT>,
+					 <&clk IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT>;
+				clock-names = "isp", "aclk", "hclk", "pclk";
+				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>,
+						<&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>;
+				power-domain-names = "isp", "csi2";
 				fsl,blk-ctrl = <&media_blk_ctrl 1>;
 				status = "disabled";
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] dt-bindings: media: rkisp1: Require pclk clock on i.MX8MP variant

[Rob Herring (Arm)]

On Mon, 16 Jun 2025 04:11:10 +0300, Laurent Pinchart wrote:
> The ISP integrated in the NXP i.MX8MP requires the pclk clock to access
> the HDR stitching registers. Make it mandatory in the DT binding.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/devicetree/bindings/media/rockchip-isp1.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 2/6] dt-bindings: media: rkisp1: Add second power domain on i.MX8MP

[Rob Herring (Arm)]

On Mon, 16 Jun 2025 04:11:11 +0300, Laurent Pinchart wrote:
> In the NXP i.MX8MP, the pclk clock required by the ISP is gated by the
> MIPI CSI-2 power domain. Add it to the power-domains property, and
> require specifying power-domain-names accordingly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../bindings/media/rockchip-isp1.yaml         | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 3/6] media: rkisp1: Refactor clocks initialization

[Jacopo Mondi]

Hi Laurent

On Mon, Jun 16, 2025 at 04:11:12AM +0300, Laurent Pinchart wrote:
> ISP instances in different SoCs differ in the number of clocks they use,
> but not in the clock names. Refactor clocks initialization to avoid
> duplicating the clock names per platform, and lower the total number of
> clocks from 8 to 4 as no platform uses more than 4 clocks. Use a static
> assert to ensure at build time that the size of the arrays match.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  8 +--
>  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 60 ++++++++++---------
>  2 files changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index ca952fd0829b..c4af1277fc6b 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -55,7 +55,7 @@ struct regmap;
>  #define RKISP1_BUS_INFO				"platform:" RKISP1_DRIVER_NAME
>
>  /* maximum number of clocks */
> -#define RKISP1_MAX_BUS_CLK			8
> +#define RKISP1_MAX_BUS_CLK			4
>
>  /* a bitmask of the ready stats */
>  #define RKISP1_STATS_MEAS_MASK			(RKISP1_CIF_ISP_AWB_DONE |	\
> @@ -139,8 +139,7 @@ enum rkisp1_feature {
>  /*
>   * struct rkisp1_info - Model-specific ISP Information
>   *
> - * @clks: array of ISP clock names
> - * @clk_size: number of entries in the @clks array
> + * @num_clocks: number of clocks
>   * @isrs: array of ISP interrupt descriptors
>   * @isr_size: number of entries in the @isrs array
>   * @isp_ver: ISP version
> @@ -152,8 +151,7 @@ enum rkisp1_feature {
>   * ISP model, version, or integration in a particular SoC.
>   */
>  struct rkisp1_info {
> -	const char * const *clks;
> -	unsigned int clk_size;
> +	unsigned int num_clocks;
>  	const struct rkisp1_isr_data *isrs;
>  	unsigned int isr_size;
>  	enum rkisp1_cif_isp_version isp_ver;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index dc65a7924f8a..0788b7a64ae9 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -8,6 +8,7 @@
>   * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
>   */
>
> +#include <linux/build_bug.h>
>  #include <linux/clk.h>
>  #include <linux/interrupt.h>
>  #include <linux/mfd/syscon.h>
> @@ -491,13 +492,6 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
>  	return ret;
>  }
>
> -static const char * const px30_isp_clks[] = {
> -	"isp",
> -	"aclk",
> -	"hclk",
> -	"pclk",
> -};
> -
>  static const struct rkisp1_isr_data px30_isp_isrs[] = {
>  	{ "isp", rkisp1_isp_isr, BIT(RKISP1_IRQ_ISP) },
>  	{ "mi", rkisp1_capture_isr, BIT(RKISP1_IRQ_MI) },
> @@ -505,8 +499,7 @@ static const struct rkisp1_isr_data px30_isp_isrs[] = {
>  };
>
>  static const struct rkisp1_info px30_isp_info = {
> -	.clks = px30_isp_clks,
> -	.clk_size = ARRAY_SIZE(px30_isp_clks),
> +	.num_clocks = 4,
>  	.isrs = px30_isp_isrs,
>  	.isr_size = ARRAY_SIZE(px30_isp_isrs),
>  	.isp_ver = RKISP1_V12,
> @@ -518,19 +511,12 @@ static const struct rkisp1_info px30_isp_info = {
>  	.max_height = 2448,
>  };
>
> -static const char * const rk3399_isp_clks[] = {
> -	"isp",
> -	"aclk",
> -	"hclk",
> -};
> -
>  static const struct rkisp1_isr_data rk3399_isp_isrs[] = {
>  	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) | BIT(RKISP1_IRQ_MIPI) },
>  };
>
>  static const struct rkisp1_info rk3399_isp_info = {
> -	.clks = rk3399_isp_clks,
> -	.clk_size = ARRAY_SIZE(rk3399_isp_clks),
> +	.num_clocks = 3,
>  	.isrs = rk3399_isp_isrs,
>  	.isr_size = ARRAY_SIZE(rk3399_isp_isrs),
>  	.isp_ver = RKISP1_V10,
> @@ -542,19 +528,12 @@ static const struct rkisp1_info rk3399_isp_info = {
>  	.max_height = 3312,
>  };
>
> -static const char * const imx8mp_isp_clks[] = {
> -	"isp",
> -	"hclk",
> -	"aclk",

it seems to me that clk_bulk_prepare_enable() guarantees ordering
of the clock sources enablement (I know regulators do not, if
I'm not mistaken).

Is it a problem that imx8mp used to declare hclk before aclk as the
other platforms do ?

> -};
> -
>  static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
>  	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) },
>  };
>
>  static const struct rkisp1_info imx8mp_isp_info = {
> -	.clks = imx8mp_isp_clks,
> -	.clk_size = ARRAY_SIZE(imx8mp_isp_clks),
> +	.num_clocks = 3,
>  	.isrs = imx8mp_isp_isrs,
>  	.isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
>  	.isp_ver = RKISP1_V_IMX8MP,
> @@ -582,6 +561,32 @@ static const struct of_device_id rkisp1_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, rkisp1_of_match);
>
> +static const char * const rkisp1_clk_names[] = {

You could also presize the array with RKISP1_MAX_BUS_CLK so that you
won't need an assertion ? It's true however that if RKISP1_MAX_BUS_CLK
 > the number of declared elements you won't get a warning..

> +	"isp",
> +	"aclk",
> +	"hclk",
> +	"pclk",
> +};
> +
> +static int rkisp1_init_clocks(struct rkisp1_device *rkisp1)
> +{
> +	const struct rkisp1_info *info = rkisp1->info;
> +	unsigned int i;
> +	int ret;
> +
> +	static_assert(ARRAY_SIZE(rkisp1_clk_names) == ARRAY_SIZE(rkisp1->clks));
> +
> +	for (i = 0; i < info->num_clocks; i++)
> +		rkisp1->clks[i].id = rkisp1_clk_names[i];
> +
> +	ret = devm_clk_bulk_get(rkisp1->dev, info->num_clocks, rkisp1->clks);
> +	if (ret)
> +		return ret;
> +
> +	rkisp1->clk_size = info->num_clocks;

rkisp1->clk_size is now only used in rkisp1_runtime_suspend() which
can easily access rkisp->info->num_clocks. Would it be better to not
duplicate the information in your opinion ?

> +	return 0;
> +}
> +
>  static int rkisp1_probe(struct platform_device *pdev)
>  {
>  	const struct rkisp1_info *info;
> @@ -639,12 +644,9 @@ static int rkisp1_probe(struct platform_device *pdev)
>  		}
>  	}
>
> -	for (i = 0; i < info->clk_size; i++)
> -		rkisp1->clks[i].id = info->clks[i];
> -	ret = devm_clk_bulk_get(dev, info->clk_size, rkisp1->clks);
> +	ret = rkisp1_init_clocks(rkisp1);
>  	if (ret)
>  		return ret;
> -	rkisp1->clk_size = info->clk_size;
>
>  	if (info->isp_ver == RKISP1_V_IMX8MP) {
>  		unsigned int id;
> --
> Regards,
>
> Laurent Pinchart
>
>

Re: [PATCH 4/6] media: rkisp1: Acquire pclk clock on i.MX8MP

[Jacopo Mondi]

Hi Laurent

On Mon, Jun 16, 2025 at 04:11:13AM +0300, Laurent Pinchart wrote:
> The ISP instances in the NXP i.MX8MP need the input pixel clock to be
> enabled in order to access the HDR stitching registers. The clock should
> ideally be mandatory, but that would break backward compatibility with
> old DT. Try to acquire it as an optional clock instead.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-dev.c      | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 0788b7a64ae9..fb4ccf497bad 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -584,6 +584,24 @@ static int rkisp1_init_clocks(struct rkisp1_device *rkisp1)
>  		return ret;
>
>  	rkisp1->clk_size = info->num_clocks;
> +
> +	/*
> +	 * On i.MX8MP the pclk clock is needed to access the HDR stitching
> +	 * registers, but wasn't required by DT bindings. Try to acquire it as
> +	 * an optional clock to avoid breaking backward compatibility.
> +	 */
> +	if (info->isp_ver == RKISP1_V_IMX8MP) {
> +		struct clk *clk;
> +
> +		clk = devm_clk_get_optional(rkisp1->dev, "pclk");
> +		if (IS_ERR(clk))
> +			return dev_err_probe(rkisp1->dev, PTR_ERR(clk),
> +					     "Failed to acquire pclk clock\n");
> +
> +		if (clk)
> +			rkisp1->clks[rkisp1->clk_size++].clk = clk;

Ah no, we still need rkisp1->clk_size!

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> +	}
> +
>  	return 0;
>  }
>
> --
> Regards,
>
> Laurent Pinchart
>
>

Re: [PATCH 3/6] media: rkisp1: Refactor clocks initialization

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 08:48:23PM +0200, Jacopo Mondi wrote:
> On Mon, Jun 16, 2025 at 04:11:12AM +0300, Laurent Pinchart wrote:
> > ISP instances in different SoCs differ in the number of clocks they use,
> > but not in the clock names. Refactor clocks initialization to avoid
> > duplicating the clock names per platform, and lower the total number of
> > clocks from 8 to 4 as no platform uses more than 4 clocks. Use a static
> > assert to ensure at build time that the size of the arrays match.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  8 +--
> >  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 60 ++++++++++---------
> >  2 files changed, 34 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index ca952fd0829b..c4af1277fc6b 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -55,7 +55,7 @@ struct regmap;
> >  #define RKISP1_BUS_INFO				"platform:" RKISP1_DRIVER_NAME
> >
> >  /* maximum number of clocks */
> > -#define RKISP1_MAX_BUS_CLK			8
> > +#define RKISP1_MAX_BUS_CLK			4
> >
> >  /* a bitmask of the ready stats */
> >  #define RKISP1_STATS_MEAS_MASK			(RKISP1_CIF_ISP_AWB_DONE |	\
> > @@ -139,8 +139,7 @@ enum rkisp1_feature {
> >  /*
> >   * struct rkisp1_info - Model-specific ISP Information
> >   *
> > - * @clks: array of ISP clock names
> > - * @clk_size: number of entries in the @clks array
> > + * @num_clocks: number of clocks
> >   * @isrs: array of ISP interrupt descriptors
> >   * @isr_size: number of entries in the @isrs array
> >   * @isp_ver: ISP version
> > @@ -152,8 +151,7 @@ enum rkisp1_feature {
> >   * ISP model, version, or integration in a particular SoC.
> >   */
> >  struct rkisp1_info {
> > -	const char * const *clks;
> > -	unsigned int clk_size;
> > +	unsigned int num_clocks;
> >  	const struct rkisp1_isr_data *isrs;
> >  	unsigned int isr_size;
> >  	enum rkisp1_cif_isp_version isp_ver;
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > index dc65a7924f8a..0788b7a64ae9 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -8,6 +8,7 @@
> >   * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> >   */
> >
> > +#include <linux/build_bug.h>
> >  #include <linux/clk.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/mfd/syscon.h>
> > @@ -491,13 +492,6 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
> >  	return ret;
> >  }
> >
> > -static const char * const px30_isp_clks[] = {
> > -	"isp",
> > -	"aclk",
> > -	"hclk",
> > -	"pclk",
> > -};
> > -
> >  static const struct rkisp1_isr_data px30_isp_isrs[] = {
> >  	{ "isp", rkisp1_isp_isr, BIT(RKISP1_IRQ_ISP) },
> >  	{ "mi", rkisp1_capture_isr, BIT(RKISP1_IRQ_MI) },
> > @@ -505,8 +499,7 @@ static const struct rkisp1_isr_data px30_isp_isrs[] = {
> >  };
> >
> >  static const struct rkisp1_info px30_isp_info = {
> > -	.clks = px30_isp_clks,
> > -	.clk_size = ARRAY_SIZE(px30_isp_clks),
> > +	.num_clocks = 4,
> >  	.isrs = px30_isp_isrs,
> >  	.isr_size = ARRAY_SIZE(px30_isp_isrs),
> >  	.isp_ver = RKISP1_V12,
> > @@ -518,19 +511,12 @@ static const struct rkisp1_info px30_isp_info = {
> >  	.max_height = 2448,
> >  };
> >
> > -static const char * const rk3399_isp_clks[] = {
> > -	"isp",
> > -	"aclk",
> > -	"hclk",
> > -};
> > -
> >  static const struct rkisp1_isr_data rk3399_isp_isrs[] = {
> >  	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) | BIT(RKISP1_IRQ_MIPI) },
> >  };
> >
> >  static const struct rkisp1_info rk3399_isp_info = {
> > -	.clks = rk3399_isp_clks,
> > -	.clk_size = ARRAY_SIZE(rk3399_isp_clks),
> > +	.num_clocks = 3,
> >  	.isrs = rk3399_isp_isrs,
> >  	.isr_size = ARRAY_SIZE(rk3399_isp_isrs),
> >  	.isp_ver = RKISP1_V10,
> > @@ -542,19 +528,12 @@ static const struct rkisp1_info rk3399_isp_info = {
> >  	.max_height = 3312,
> >  };
> >
> > -static const char * const imx8mp_isp_clks[] = {
> > -	"isp",
> > -	"hclk",
> > -	"aclk",
> 
> it seems to me that clk_bulk_prepare_enable() guarantees ordering
> of the clock sources enablement (I know regulators do not, if
> I'm not mistaken).
> 
> Is it a problem that imx8mp used to declare hclk before aclk as the
> other platforms do ?

No, I don't think the order matters.

> > -};
> > -
> >  static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
> >  	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) },
> >  };
> >
> >  static const struct rkisp1_info imx8mp_isp_info = {
> > -	.clks = imx8mp_isp_clks,
> > -	.clk_size = ARRAY_SIZE(imx8mp_isp_clks),
> > +	.num_clocks = 3,
> >  	.isrs = imx8mp_isp_isrs,
> >  	.isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
> >  	.isp_ver = RKISP1_V_IMX8MP,
> > @@ -582,6 +561,32 @@ static const struct of_device_id rkisp1_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, rkisp1_of_match);
> >
> > +static const char * const rkisp1_clk_names[] = {
> 
> You could also presize the array with RKISP1_MAX_BUS_CLK so that you
> won't need an assertion ? It's true however that if RKISP1_MAX_BUS_CLK
>  > the number of declared elements you won't get a warning..

Yes, that's why I didn't do it.

> > +	"isp",
> > +	"aclk",
> > +	"hclk",
> > +	"pclk",
> > +};
> > +
> > +static int rkisp1_init_clocks(struct rkisp1_device *rkisp1)
> > +{
> > +	const struct rkisp1_info *info = rkisp1->info;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	static_assert(ARRAY_SIZE(rkisp1_clk_names) == ARRAY_SIZE(rkisp1->clks));
> > +
> > +	for (i = 0; i < info->num_clocks; i++)
> > +		rkisp1->clks[i].id = rkisp1_clk_names[i];
> > +
> > +	ret = devm_clk_bulk_get(rkisp1->dev, info->num_clocks, rkisp1->clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	rkisp1->clk_size = info->num_clocks;
> 
> rkisp1->clk_size is now only used in rkisp1_runtime_suspend() which
> can easily access rkisp->info->num_clocks. Would it be better to not
> duplicate the information in your opinion ?

You've now found in 4/6 that we still need it :-)

> > +	return 0;
> > +}
> > +
> >  static int rkisp1_probe(struct platform_device *pdev)
> >  {
> >  	const struct rkisp1_info *info;
> > @@ -639,12 +644,9 @@ static int rkisp1_probe(struct platform_device *pdev)
> >  		}
> >  	}
> >
> > -	for (i = 0; i < info->clk_size; i++)
> > -		rkisp1->clks[i].id = info->clks[i];
> > -	ret = devm_clk_bulk_get(dev, info->clk_size, rkisp1->clks);
> > +	ret = rkisp1_init_clocks(rkisp1);
> >  	if (ret)
> >  		return ret;
> > -	rkisp1->clk_size = info->clk_size;
> >
> >  	if (info->isp_ver == RKISP1_V_IMX8MP) {
> >  		unsigned int id;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] media: rkisp1: Add support for multiple power domains

[Jacopo Mondi]

Hi Laurent

On Mon, Jun 16, 2025 at 04:11:14AM +0300, Laurent Pinchart wrote:
> The ISP instances in the NXP i.MX8MP need two power domains. While
> single power domains are managed automatically by the device core,
> support for multiple power domains requires manually attaching to the
> power domains. Do so based on platform data.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  9 ++++
>  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 45 +++++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index c4af1277fc6b..ce900440c78d 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -24,6 +24,7 @@
>  #include "rkisp1-regs.h"
>
>  struct dentry;
> +struct dev_pm_domain_list;
>  struct regmap;
>
>  /*
> @@ -146,6 +147,8 @@ enum rkisp1_feature {
>   * @features: bitmask of rkisp1_feature features implemented by the ISP
>   * @max_width: maximum input frame width
>   * @max_height: maximum input frame height
> + * @pm_domains.names: name of the power domains
> + * @pm_domains.count: number of power domains
>   *
>   * This structure contains information about the ISP specific to a particular
>   * ISP model, version, or integration in a particular SoC.
> @@ -158,6 +161,10 @@ struct rkisp1_info {
>  	unsigned int features;
>  	unsigned int max_width;
>  	unsigned int max_height;
> +	struct {
> +		const char * const *names;
> +		unsigned int count;
> +	} pm_domains;
>  };
>
>  /*
> @@ -479,6 +486,7 @@ struct rkisp1_debug {
>   * @dev:	   a pointer to the struct device
>   * @clk_size:	   number of clocks
>   * @clks:	   array of clocks
> + * @pm_domains:    power domains
>   * @gasket:	   the gasket - i.MX8MP only
>   * @gasket_id:	   the gasket ID (0 or 1) - i.MX8MP only
>   * @v4l2_dev:	   v4l2_device variable
> @@ -503,6 +511,7 @@ struct rkisp1_device {
>  	struct device *dev;
>  	unsigned int clk_size;
>  	struct clk_bulk_data clks[RKISP1_MAX_BUS_CLK];
> +	struct dev_pm_domain_list *pm_domains;
>  	struct regmap *gasket;
>  	unsigned int gasket_id;
>  	struct v4l2_device v4l2_dev;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index fb4ccf497bad..1791c02a40ae 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -17,6 +17,7 @@
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-mc.h>
> @@ -532,6 +533,11 @@ static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
>  	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) },
>  };
>
> +static const char * const imx8mp_isp_pm_domains[] = {
> +	"isp",
> +	"csi2",
> +};
> +
>  static const struct rkisp1_info imx8mp_isp_info = {
>  	.num_clocks = 3,
>  	.isrs = imx8mp_isp_isrs,
> @@ -542,6 +548,10 @@ static const struct rkisp1_info imx8mp_isp_info = {
>  		  | RKISP1_FEATURE_COMPAND,
>  	.max_width = 4096,
>  	.max_height = 3072,
> +	.pm_domains = {
> +		.names = imx8mp_isp_pm_domains,
> +		.count = ARRAY_SIZE(imx8mp_isp_pm_domains),
> +	},
>  };
>
>  static const struct of_device_id rkisp1_of_match[] = {
> @@ -605,6 +615,37 @@ static int rkisp1_init_clocks(struct rkisp1_device *rkisp1)
>  	return 0;
>  }
>
> +static int rkisp1_init_pm_domains(struct rkisp1_device *rkisp1)
> +{
> +	const struct rkisp1_info *info = rkisp1->info;
> +	struct dev_pm_domain_attach_data pm_domain_data = {
> +		.pd_names = info->pm_domains.names,
> +		.num_pd_names = info->pm_domains.count,
> +	};
> +	int ret;
> +
> +	/*
> +	 * Most platforms have a single power domain, which the PM domain core
> +	 * automatically attaches at probe time. When that's the case there's
> +	 * nothing to do here.
> +	 */
> +	if (rkisp1->dev->pm_domain)
> +		return 0;

So if multiple domains are declated in dts the PM core ignores them ?
I presume so otherwise you would be getting the first power domain
twice...

> +
> +	if (!pm_domain_data.num_pd_names)
> +		return 0;

You could also only call this function for imx8mp and avoid the
checks, but I agree this version is probably more generic

> +
> +	ret = devm_pm_domain_attach_list(rkisp1->dev, &pm_domain_data,
> +					 &rkisp1->pm_domains);

I was wondering if the third argument could actually be a local
variable, but __devm_add_action() copies it to then be later passed
to devm_pm_domain_detach_list(), so it needs to be stored somewhere
and not declared on the stack..

No comments but just questions I gave myself an answer to already, so

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> +	if (ret < 0) {
> +		dev_err_probe(rkisp1->dev, ret,
> +			      "Failed to attach power domains\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int rkisp1_probe(struct platform_device *pdev)
>  {
>  	const struct rkisp1_info *info;
> @@ -666,6 +707,10 @@ static int rkisp1_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>
> +	ret = rkisp1_init_pm_domains(rkisp1);
> +	if (ret)
> +		return ret;
> +
>  	if (info->isp_ver == RKISP1_V_IMX8MP) {
>  		unsigned int id;
>
> --
> Regards,
>
> Laurent Pinchart
>
>

Re: [PATCH 3/6] media: rkisp1: Refactor clocks initialization

[Jacopo Mondi]

Hi Laurent

On Thu, Aug 21, 2025 at 10:02:59PM +0300, Laurent Pinchart wrote:
> On Thu, Aug 21, 2025 at 08:48:23PM +0200, Jacopo Mondi wrote:
> > On Mon, Jun 16, 2025 at 04:11:12AM +0300, Laurent Pinchart wrote:
> > > ISP instances in different SoCs differ in the number of clocks they use,
> > > but not in the clock names. Refactor clocks initialization to avoid
> > > duplicating the clock names per platform, and lower the total number of
> > > clocks from 8 to 4 as no platform uses more than 4 clocks. Use a static
> > > assert to ensure at build time that the size of the arrays match.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  8 +--
> > >  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 60 ++++++++++---------
> > >  2 files changed, 34 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > index ca952fd0829b..c4af1277fc6b 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > @@ -55,7 +55,7 @@ struct regmap;
> > >  #define RKISP1_BUS_INFO				"platform:" RKISP1_DRIVER_NAME
> > >
> > >  /* maximum number of clocks */
> > > -#define RKISP1_MAX_BUS_CLK			8
> > > +#define RKISP1_MAX_BUS_CLK			4
> > >
> > >  /* a bitmask of the ready stats */
> > >  #define RKISP1_STATS_MEAS_MASK			(RKISP1_CIF_ISP_AWB_DONE |	\
> > > @@ -139,8 +139,7 @@ enum rkisp1_feature {
> > >  /*
> > >   * struct rkisp1_info - Model-specific ISP Information
> > >   *
> > > - * @clks: array of ISP clock names
> > > - * @clk_size: number of entries in the @clks array
> > > + * @num_clocks: number of clocks
> > >   * @isrs: array of ISP interrupt descriptors
> > >   * @isr_size: number of entries in the @isrs array
> > >   * @isp_ver: ISP version
> > > @@ -152,8 +151,7 @@ enum rkisp1_feature {
> > >   * ISP model, version, or integration in a particular SoC.
> > >   */
> > >  struct rkisp1_info {
> > > -	const char * const *clks;
> > > -	unsigned int clk_size;
> > > +	unsigned int num_clocks;
> > >  	const struct rkisp1_isr_data *isrs;
> > >  	unsigned int isr_size;
> > >  	enum rkisp1_cif_isp_version isp_ver;
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > > index dc65a7924f8a..0788b7a64ae9 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > > @@ -8,6 +8,7 @@
> > >   * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> > >   */
> > >
> > > +#include <linux/build_bug.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/mfd/syscon.h>
> > > @@ -491,13 +492,6 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
> > >  	return ret;
> > >  }
> > >
> > > -static const char * const px30_isp_clks[] = {
> > > -	"isp",
> > > -	"aclk",
> > > -	"hclk",
> > > -	"pclk",
> > > -};
> > > -
> > >  static const struct rkisp1_isr_data px30_isp_isrs[] = {
> > >  	{ "isp", rkisp1_isp_isr, BIT(RKISP1_IRQ_ISP) },
> > >  	{ "mi", rkisp1_capture_isr, BIT(RKISP1_IRQ_MI) },
> > > @@ -505,8 +499,7 @@ static const struct rkisp1_isr_data px30_isp_isrs[] = {
> > >  };
> > >
> > >  static const struct rkisp1_info px30_isp_info = {
> > > -	.clks = px30_isp_clks,
> > > -	.clk_size = ARRAY_SIZE(px30_isp_clks),
> > > +	.num_clocks = 4,
> > >  	.isrs = px30_isp_isrs,
> > >  	.isr_size = ARRAY_SIZE(px30_isp_isrs),
> > >  	.isp_ver = RKISP1_V12,
> > > @@ -518,19 +511,12 @@ static const struct rkisp1_info px30_isp_info = {
> > >  	.max_height = 2448,
> > >  };
> > >
> > > -static const char * const rk3399_isp_clks[] = {
> > > -	"isp",
> > > -	"aclk",
> > > -	"hclk",
> > > -};
> > > -
> > >  static const struct rkisp1_isr_data rk3399_isp_isrs[] = {
> > >  	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) | BIT(RKISP1_IRQ_MIPI) },
> > >  };
> > >
> > >  static const struct rkisp1_info rk3399_isp_info = {
> > > -	.clks = rk3399_isp_clks,
> > > -	.clk_size = ARRAY_SIZE(rk3399_isp_clks),
> > > +	.num_clocks = 3,
> > >  	.isrs = rk3399_isp_isrs,
> > >  	.isr_size = ARRAY_SIZE(rk3399_isp_isrs),
> > >  	.isp_ver = RKISP1_V10,
> > > @@ -542,19 +528,12 @@ static const struct rkisp1_info rk3399_isp_info = {
> > >  	.max_height = 3312,
> > >  };
> > >
> > > -static const char * const imx8mp_isp_clks[] = {
> > > -	"isp",
> > > -	"hclk",
> > > -	"aclk",
> >
> > it seems to me that clk_bulk_prepare_enable() guarantees ordering
> > of the clock sources enablement (I know regulators do not, if
> > I'm not mistaken).
> >
> > Is it a problem that imx8mp used to declare hclk before aclk as the
> > other platforms do ?
>
> No, I don't think the order matters.
>
> > > -};
> > > -
> > >  static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
> > >  	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) },
> > >  };
> > >
> > >  static const struct rkisp1_info imx8mp_isp_info = {
> > > -	.clks = imx8mp_isp_clks,
> > > -	.clk_size = ARRAY_SIZE(imx8mp_isp_clks),
> > > +	.num_clocks = 3,
> > >  	.isrs = imx8mp_isp_isrs,
> > >  	.isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
> > >  	.isp_ver = RKISP1_V_IMX8MP,
> > > @@ -582,6 +561,32 @@ static const struct of_device_id rkisp1_of_match[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(of, rkisp1_of_match);
> > >
> > > +static const char * const rkisp1_clk_names[] = {
> >
> > You could also presize the array with RKISP1_MAX_BUS_CLK so that you
> > won't need an assertion ? It's true however that if RKISP1_MAX_BUS_CLK
> >  > the number of declared elements you won't get a warning..
>
> Yes, that's why I didn't do it.
>
> > > +	"isp",
> > > +	"aclk",
> > > +	"hclk",
> > > +	"pclk",
> > > +};
> > > +
> > > +static int rkisp1_init_clocks(struct rkisp1_device *rkisp1)
> > > +{
> > > +	const struct rkisp1_info *info = rkisp1->info;
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > > +	static_assert(ARRAY_SIZE(rkisp1_clk_names) == ARRAY_SIZE(rkisp1->clks));
> > > +
> > > +	for (i = 0; i < info->num_clocks; i++)
> > > +		rkisp1->clks[i].id = rkisp1_clk_names[i];
> > > +
> > > +	ret = devm_clk_bulk_get(rkisp1->dev, info->num_clocks, rkisp1->clks);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	rkisp1->clk_size = info->num_clocks;
> >
> > rkisp1->clk_size is now only used in rkisp1_runtime_suspend() which
> > can easily access rkisp->info->num_clocks. Would it be better to not
> > duplicate the information in your opinion ?
>
> You've now found in 4/6 that we still need it :-)
>

Yes I did, so I can now
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> > > +	return 0;
> > > +}
> > > +
> > >  static int rkisp1_probe(struct platform_device *pdev)
> > >  {
> > >  	const struct rkisp1_info *info;
> > > @@ -639,12 +644,9 @@ static int rkisp1_probe(struct platform_device *pdev)
> > >  		}
> > >  	}
> > >
> > > -	for (i = 0; i < info->clk_size; i++)
> > > -		rkisp1->clks[i].id = info->clks[i];
> > > -	ret = devm_clk_bulk_get(dev, info->clk_size, rkisp1->clks);
> > > +	ret = rkisp1_init_clocks(rkisp1);
> > >  	if (ret)
> > >  		return ret;
> > > -	rkisp1->clk_size = info->clk_size;
> > >
> > >  	if (info->isp_ver == RKISP1_V_IMX8MP) {
> > >  		unsigned int id;
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 6/6] arm64: dts: imx8mp: Add pclk clock and second power domain for the ISP

[Shawn Guo]

On Mon, Jun 16, 2025 at 04:11:15AM +0300, Laurent Pinchart wrote:
> The ISP HDR stitching registers are clocked by the pixel clock, which is
> gated by the MIPI_CSI2 power domain. Attempting to access those
> registers with the clock off locks up the system. Fix this by adding the
> pclk clock and the MIPI_CSI2 secondary power domain.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Applied, thanks!