[PATCH 0/8] media: imx-pxp: add support for i.MX7D

[PATCH 0/8] media: imx-pxp: add support for i.MX7D

[Michael Tretter]

This series adds support for the PXP found on the i.MX7D to the imx-pxp
driver.

The PXP on the i.MX7D has a few differences compared to the one on the
i.MX6ULL. Especially, it has more processing blocks and slightly different
multiplexers to route the data between the blocks. Therefore, the driver must
configure a different data path depending on the platform.

While the PXP has a version register, the reported version is the same on the
i.MX6ULL and the i.MX7D. Therefore, we cannot use the version register to
change the driver behavior, but have to use the device tree compatible. The
driver still prints the found version to the log to help bringing up the PXP
on further platforms.

The patches are inspired by some earlier patches [0] by Laurent to add PXP
support to the i.MX7d. Compared to the earlier patches, these patches add
different behavior depending on the platform. Furthermore, the patches disable
only the LUT block, but keep the rotator block enabled, as it may now be
configured via the V4L2 rotate control.

Patch 1 converts the dt-binding to yaml.

Patches 2 to 5 cleanup and refactor the driver in preparation of handling
different PXP versions.

Patches 6 and 7 add the handling of different platforms and the i.MX7d
specific configuration.

Patch 8 adds the device tree node for the PXP to the i.MX7d device tree.

Michael

[0] https://lore.kernel.org/linux-media/20200510223100.11641-1-laurent.pinchart@ideasonboard.com/

Michael Tretter (8):
  media: dt-bindings: media: fsl-pxp: convert to yaml
  media: imx-pxp: detect PXP version
  media: imx-pxp: extract helper function to setup data path
  media: imx-pxp: explicitly disable unused blocks
  media: imx-pxp: disable LUT block
  media: imx-pxp: make data_path_ctrl0 platform dependent
  media: imx-pxp: add support for i.MX7D
  ARM: dts: imx7d: add node for PXP

 .../bindings/media/fsl,imx6ull-pxp.yaml       |  62 ++++++++
 .../devicetree/bindings/media/fsl-pxp.txt     |  26 ---
 arch/arm/boot/dts/imx7d.dtsi                  |   9 ++
 drivers/media/platform/nxp/imx-pxp.c          | 148 +++++++++++++++---
 4 files changed, 197 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
 delete mode 100644 Documentation/devicetree/bindings/media/fsl-pxp.txt

-- 
2.30.2

[PATCH 1/8] media: dt-bindings: media: fsl-pxp: convert to yaml

[Michael Tretter]

Convert the bindings of the Freescale Pixel Pipeline to YAML.

The conversion drops the previously listed compatibles for several SoCs.
It is unclear, if the PXP on these SoCs is compatible to any of the PXPs
on the existing SoCs and would allow to reuse the already defined
compatibles. The missing compatibles should be brought back when the
support for the PXP on these SoCs is added.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../bindings/media/fsl,imx6ull-pxp.yaml       | 62 +++++++++++++++++++
 .../devicetree/bindings/media/fsl-pxp.txt     | 26 --------
 2 files changed, 62 insertions(+), 26 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
 delete mode 100644 Documentation/devicetree/bindings/media/fsl-pxp.txt

diff --git a/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml b/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
new file mode 100644
index 000000000000..e5f227b84759
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/media/fsl,imx6ull-pxp.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Freescale Pixel Pipeline
+
+maintainers:
+  - Philipp Zabel <p.zabel@pengutronix.de>
+  - Michael Tretter <m.tretter@pengutronix.de>
+
+description:
+  The Pixel Pipeline (PXP) is a memory-to-memory graphics processing engine
+  that supports scaling, colorspace conversion, alpha blending, rotation, and
+  pixel conversion via lookup table. Different versions are present on various
+  i.MX SoCs from i.MX23 to i.MX7.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx6ul-pxp
+      - fsl,imx6ull-pxp
+      - fsl,imx7d-pxp
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: axi
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: False
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx6ul-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    pxp: pxp@21cc000 {
+        compatible = "fsl,imx6ull-pxp";
+        reg = <0x021cc000 0x4000>;
+        interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
+        clock-names = "axi";
+        clocks = <&clks IMX6UL_CLK_PXP>;
+    };
diff --git a/Documentation/devicetree/bindings/media/fsl-pxp.txt b/Documentation/devicetree/bindings/media/fsl-pxp.txt
deleted file mode 100644
index f8090e06530d..000000000000
--- a/Documentation/devicetree/bindings/media/fsl-pxp.txt
+++ /dev/null
@@ -1,26 +0,0 @@
-Freescale Pixel Pipeline
-========================
-
-The Pixel Pipeline (PXP) is a memory-to-memory graphics processing engine
-that supports scaling, colorspace conversion, alpha blending, rotation, and
-pixel conversion via lookup table. Different versions are present on various
-i.MX SoCs from i.MX23 to i.MX7.
-
-Required properties:
-- compatible: should be "fsl,<soc>-pxp", where SoC can be one of imx23, imx28,
-  imx6dl, imx6sl, imx6sll, imx6ul, imx6sx, imx6ull, or imx7d.
-- reg: the register base and size for the device registers
-- interrupts: the PXP interrupt, two interrupts for imx6ull and imx7d.
-- clock-names: should be "axi"
-- clocks: the PXP AXI clock
-
-Example:
-
-pxp@21cc000 {
-	compatible = "fsl,imx6ull-pxp";
-	reg = <0x021cc000 0x4000>;
-	interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
-		     <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
-	clock-names = "axi";
-	clocks = <&clks IMX6UL_CLK_PXP>;
-};
-- 
2.30.2

Re: [PATCH 1/8] media: dt-bindings: media: fsl-pxp: convert to yaml

[Rob Herring]

On Thu, 05 Jan 2023 14:47:22 +0100, Michael Tretter wrote:
> Convert the bindings of the Freescale Pixel Pipeline to YAML.
> 
> The conversion drops the previously listed compatibles for several SoCs.
> It is unclear, if the PXP on these SoCs is compatible to any of the PXPs
> on the existing SoCs and would allow to reuse the already defined
> compatibles. The missing compatibles should be brought back when the
> support for the PXP on these SoCs is added.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  .../bindings/media/fsl,imx6ull-pxp.yaml       | 62 +++++++++++++++++++
>  .../devicetree/bindings/media/fsl-pxp.txt     | 26 --------
>  2 files changed, 62 insertions(+), 26 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
>  delete mode 100644 Documentation/devicetree/bindings/media/fsl-pxp.txt
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230105134729.59542-2-m.tretter@pengutronix.de


pxp@20f0000: compatible:0: 'fsl,imx6sll-pxp' is not one of ['fsl,imx6ul-pxp', 'fsl,imx6ull-pxp', 'fsl,imx7d-pxp']
	arch/arm/boot/dts/imx6sll-evk.dtb
	arch/arm/boot/dts/imx6sll-kobo-clarahd.dtb
	arch/arm/boot/dts/imx6sll-kobo-librah2o.dtb

pxp@20f0000: compatible: ['fsl,imx6sll-pxp', 'fsl,imx6ull-pxp'] is too long
	arch/arm/boot/dts/imx6sll-evk.dtb
	arch/arm/boot/dts/imx6sll-kobo-clarahd.dtb
	arch/arm/boot/dts/imx6sll-kobo-librah2o.dtb

pxp@2218000: compatible:0: 'fsl,imx6sx-pxp' is not one of ['fsl,imx6ul-pxp', 'fsl,imx6ull-pxp', 'fsl,imx7d-pxp']
	arch/arm/boot/dts/imx6sx-nitrogen6sx.dtb
	arch/arm/boot/dts/imx6sx-sabreauto.dtb
	arch/arm/boot/dts/imx6sx-sdb.dtb
	arch/arm/boot/dts/imx6sx-sdb-mqs.dtb
	arch/arm/boot/dts/imx6sx-sdb-reva.dtb
	arch/arm/boot/dts/imx6sx-sdb-sai.dtb
	arch/arm/boot/dts/imx6sx-softing-vining-2000.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-basic.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-extended.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-full.dtb

pxp@2218000: compatible: ['fsl,imx6sx-pxp', 'fsl,imx6ull-pxp'] is too long
	arch/arm/boot/dts/imx6sx-nitrogen6sx.dtb
	arch/arm/boot/dts/imx6sx-sabreauto.dtb
	arch/arm/boot/dts/imx6sx-sdb.dtb
	arch/arm/boot/dts/imx6sx-sdb-mqs.dtb
	arch/arm/boot/dts/imx6sx-sdb-reva.dtb
	arch/arm/boot/dts/imx6sx-sdb-sai.dtb
	arch/arm/boot/dts/imx6sx-softing-vining-2000.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-basic.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-extended.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-full.dtb

pxp@2218000: 'power-domains' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/imx6sx-nitrogen6sx.dtb
	arch/arm/boot/dts/imx6sx-sabreauto.dtb
	arch/arm/boot/dts/imx6sx-sdb.dtb
	arch/arm/boot/dts/imx6sx-sdb-mqs.dtb
	arch/arm/boot/dts/imx6sx-sdb-reva.dtb
	arch/arm/boot/dts/imx6sx-sdb-sai.dtb
	arch/arm/boot/dts/imx6sx-softing-vining-2000.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-basic.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-extended.dtb
	arch/arm/boot/dts/imx6sx-udoo-neo-full.dtb

Re: [PATCH 1/8] media: dt-bindings: media: fsl-pxp: convert to yaml

[Michael Tretter]

On Thu, 05 Jan 2023 21:18:21 -0600, Rob Herring wrote:
> On Thu, 05 Jan 2023 14:47:22 +0100, Michael Tretter wrote:
> > Convert the bindings of the Freescale Pixel Pipeline to YAML.
> > 
> > The conversion drops the previously listed compatibles for several SoCs.
> > It is unclear, if the PXP on these SoCs is compatible to any of the PXPs
> > on the existing SoCs and would allow to reuse the already defined
> > compatibles. The missing compatibles should be brought back when the
> > support for the PXP on these SoCs is added.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  .../bindings/media/fsl,imx6ull-pxp.yaml       | 62 +++++++++++++++++++
> >  .../devicetree/bindings/media/fsl-pxp.txt     | 26 --------
> >  2 files changed, 62 insertions(+), 26 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/media/fsl-pxp.txt
> > 
> 
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.

I am surprised that these warnings didn't show up when I ran 'make
dtbs_check'. I will check if there is something wrong with my setup.

> 
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
> 
> Full log is available here: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230105134729.59542-2-m.tretter@pengutronix.de
> 
> 
> pxp@20f0000: compatible:0: 'fsl,imx6sll-pxp' is not one of ['fsl,imx6ul-pxp', 'fsl,imx6ull-pxp', 'fsl,imx7d-pxp']
> 	arch/arm/boot/dts/imx6sll-evk.dtb
> 	arch/arm/boot/dts/imx6sll-kobo-clarahd.dtb
> 	arch/arm/boot/dts/imx6sll-kobo-librah2o.dtb
> 
> pxp@20f0000: compatible: ['fsl,imx6sll-pxp', 'fsl,imx6ull-pxp'] is too long
> 	arch/arm/boot/dts/imx6sll-evk.dtb
> 	arch/arm/boot/dts/imx6sll-kobo-clarahd.dtb
> 	arch/arm/boot/dts/imx6sll-kobo-librah2o.dtb

This is an error in the schema. I dropped the fsl,imx6sll-pxp and
fsl,imx6sx-pxp compatibles, because I thought that they aren't used.

I will send a v2 to fix the schema.

Michael

> 
> pxp@2218000: compatible:0: 'fsl,imx6sx-pxp' is not one of ['fsl,imx6ul-pxp', 'fsl,imx6ull-pxp', 'fsl,imx7d-pxp']
> 	arch/arm/boot/dts/imx6sx-nitrogen6sx.dtb
> 	arch/arm/boot/dts/imx6sx-sabreauto.dtb
> 	arch/arm/boot/dts/imx6sx-sdb.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-mqs.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-reva.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-sai.dtb
> 	arch/arm/boot/dts/imx6sx-softing-vining-2000.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-basic.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-extended.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-full.dtb
> 
> pxp@2218000: compatible: ['fsl,imx6sx-pxp', 'fsl,imx6ull-pxp'] is too long
> 	arch/arm/boot/dts/imx6sx-nitrogen6sx.dtb
> 	arch/arm/boot/dts/imx6sx-sabreauto.dtb
> 	arch/arm/boot/dts/imx6sx-sdb.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-mqs.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-reva.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-sai.dtb
> 	arch/arm/boot/dts/imx6sx-softing-vining-2000.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-basic.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-extended.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-full.dtb
> 
> pxp@2218000: 'power-domains' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/imx6sx-nitrogen6sx.dtb
> 	arch/arm/boot/dts/imx6sx-sabreauto.dtb
> 	arch/arm/boot/dts/imx6sx-sdb.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-mqs.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-reva.dtb
> 	arch/arm/boot/dts/imx6sx-sdb-sai.dtb
> 	arch/arm/boot/dts/imx6sx-softing-vining-2000.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-basic.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-extended.dtb
> 	arch/arm/boot/dts/imx6sx-udoo-neo-full.dtb
> 
> 

Re: [PATCH 1/8] media: dt-bindings: media: fsl-pxp: convert to yaml

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:22PM +0100, Michael Tretter wrote:
> Convert the bindings of the Freescale Pixel Pipeline to YAML.
> 
> The conversion drops the previously listed compatibles for several SoCs.
> It is unclear, if the PXP on these SoCs is compatible to any of the PXPs
> on the existing SoCs and would allow to reuse the already defined
> compatibles. The missing compatibles should be brought back when the
> support for the PXP on these SoCs is added.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  .../bindings/media/fsl,imx6ull-pxp.yaml       | 62 +++++++++++++++++++
>  .../devicetree/bindings/media/fsl-pxp.txt     | 26 --------
>  2 files changed, 62 insertions(+), 26 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
>  delete mode 100644 Documentation/devicetree/bindings/media/fsl-pxp.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml b/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
> new file mode 100644
> index 000000000000..e5f227b84759
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/media/fsl,imx6ull-pxp.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Freescale Pixel Pipeline
> +
> +maintainers:
> +  - Philipp Zabel <p.zabel@pengutronix.de>
> +  - Michael Tretter <m.tretter@pengutronix.de>
> +
> +description:
> +  The Pixel Pipeline (PXP) is a memory-to-memory graphics processing engine
> +  that supports scaling, colorspace conversion, alpha blending, rotation, and
> +  pixel conversion via lookup table. Different versions are present on various
> +  i.MX SoCs from i.MX23 to i.MX7.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx6ul-pxp
> +      - fsl,imx6ull-pxp
> +      - fsl,imx7d-pxp
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2

Can you make the number of items conditional on the compatible string ?

> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: axi

I think this could be simplified to

  clock-names:
    const: axi

Up to you.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: False

s/False/false/

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx6ul-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    pxp: pxp@21cc000 {
> +        compatible = "fsl,imx6ull-pxp";
> +        reg = <0x021cc000 0x4000>;
> +        interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +        clock-names = "axi";
> +        clocks = <&clks IMX6UL_CLK_PXP>;
> +    };
> diff --git a/Documentation/devicetree/bindings/media/fsl-pxp.txt b/Documentation/devicetree/bindings/media/fsl-pxp.txt
> deleted file mode 100644
> index f8090e06530d..000000000000
> --- a/Documentation/devicetree/bindings/media/fsl-pxp.txt
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -Freescale Pixel Pipeline
> -========================
> -
> -The Pixel Pipeline (PXP) is a memory-to-memory graphics processing engine
> -that supports scaling, colorspace conversion, alpha blending, rotation, and
> -pixel conversion via lookup table. Different versions are present on various
> -i.MX SoCs from i.MX23 to i.MX7.
> -
> -Required properties:
> -- compatible: should be "fsl,<soc>-pxp", where SoC can be one of imx23, imx28,
> -  imx6dl, imx6sl, imx6sll, imx6ul, imx6sx, imx6ull, or imx7d.
> -- reg: the register base and size for the device registers
> -- interrupts: the PXP interrupt, two interrupts for imx6ull and imx7d.
> -- clock-names: should be "axi"
> -- clocks: the PXP AXI clock
> -
> -Example:
> -
> -pxp@21cc000 {
> -	compatible = "fsl,imx6ull-pxp";
> -	reg = <0x021cc000 0x4000>;
> -	interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> -		     <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> -	clock-names = "axi";
> -	clocks = <&clks IMX6UL_CLK_PXP>;
> -};

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/8] media: dt-bindings: media: fsl-pxp: convert to yaml

[Krzysztof Kozlowski]

On 05/01/2023 14:47, Michael Tretter wrote:
> Convert the bindings of the Freescale Pixel Pipeline to YAML.
> 
> The conversion drops the previously listed compatibles for several SoCs.
> It is unclear, if the PXP on these SoCs is compatible to any of the PXPs
> on the existing SoCs and would allow to reuse the already defined
> compatibles. The missing compatibles should be brought back when the
> support for the PXP on these SoCs is added.
> 

Subject: only one "media" prefix.

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  .../bindings/media/fsl,imx6ull-pxp.yaml       | 62 +++++++++++++++++++
>  .../devicetree/bindings/media/fsl-pxp.txt     | 26 --------
>  2 files changed, 62 insertions(+), 26 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
>  delete mode 100644 Documentation/devicetree/bindings/media/fsl-pxp.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml b/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
> new file mode 100644
> index 000000000000..e5f227b84759
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/media/fsl,imx6ull-pxp.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop qyotes from both.

> +
> +title: Freescale Pixel Pipeline
> +
> +maintainers:
> +  - Philipp Zabel <p.zabel@pengutronix.de>
> +  - Michael Tretter <m.tretter@pengutronix.de>
> +
> +description:
> +  The Pixel Pipeline (PXP) is a memory-to-memory graphics processing engine
> +  that supports scaling, colorspace conversion, alpha blending, rotation, and
> +  pixel conversion via lookup table. Different versions are present on various
> +  i.MX SoCs from i.MX23 to i.MX7.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx6ul-pxp
> +      - fsl,imx6ull-pxp
> +      - fsl,imx7d-pxp
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: axi
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names

Add allOf:if:then restricting interrupts per variant.

> +
> +additionalProperties: False
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx6ul-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +

Best regards,
Krzysztof

[PATCH 2/8] media: imx-pxp: detect PXP version

[Michael Tretter]

Different versions of the Pixel Pipeline have different blocks and their
routing may be different. Read the PXP_HW_VERSION register to determine
the version of the PXP and print it to the log for debugging purposes.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index 689ae5e6ac62..05abe40558b0 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -10,6 +10,7 @@
  * Pawel Osciak, <pawel@osciak.com>
  * Marek Szyprowski, <m.szyprowski@samsung.com>
  */
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
@@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info");
 #define MEM2MEM_HFLIP	(1 << 0)
 #define MEM2MEM_VFLIP	(1 << 1)
 
+#define PXP_VERSION_MAJOR(version) \
+	FIELD_GET(BM_PXP_VERSION_MAJOR, version)
+#define PXP_VERSION_MINOR(version) \
+	FIELD_GET(BM_PXP_VERSION_MINOR, version)
+
 #define dprintk(dev, fmt, arg...) \
 	v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg)
 
@@ -192,6 +198,8 @@ struct pxp_dev {
 	struct clk		*clk;
 	void __iomem		*mmio;
 
+	u32			hw_version;
+
 	atomic_t		num_inst;
 	struct mutex		dev_mutex;
 	spinlock_t		irqlock;
@@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev)
 	return 0;
 }
 
+static u32 pxp_read_version(struct pxp_dev *dev)
+{
+	return readl(dev->mmio + HW_PXP_VERSION);
+}
+
 static int pxp_probe(struct platform_device *pdev)
 {
 	struct pxp_dev *dev;
@@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 
+	dev->hw_version = pxp_read_version(dev);
+	dev_info(&pdev->dev, "PXP Version %d.%d\n",
+		 PXP_VERSION_MAJOR(dev->hw_version),
+		 PXP_VERSION_MINOR(dev->hw_version));
+
 	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
 	if (ret)
 		goto err_clk;
-- 
2.30.2

Re: [PATCH 2/8] media: imx-pxp: detect PXP version

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:23PM +0100, Michael Tretter wrote:
> Different versions of the Pixel Pipeline have different blocks and their
> routing may be different. Read the PXP_HW_VERSION register to determine
> the version of the PXP and print it to the log for debugging purposes.

Is there a specific reason you chose to read the version register
instead of basing the decision on the compatible string ?

> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index 689ae5e6ac62..05abe40558b0 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -10,6 +10,7 @@
>   * Pawel Osciak, <pawel@osciak.com>
>   * Marek Szyprowski, <m.szyprowski@samsung.com>
>   */
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info");
>  #define MEM2MEM_HFLIP	(1 << 0)
>  #define MEM2MEM_VFLIP	(1 << 1)
>  
> +#define PXP_VERSION_MAJOR(version) \
> +	FIELD_GET(BM_PXP_VERSION_MAJOR, version)
> +#define PXP_VERSION_MINOR(version) \
> +	FIELD_GET(BM_PXP_VERSION_MINOR, version)
> +
>  #define dprintk(dev, fmt, arg...) \
>  	v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg)
>  
> @@ -192,6 +198,8 @@ struct pxp_dev {
>  	struct clk		*clk;
>  	void __iomem		*mmio;
>  
> +	u32			hw_version;
> +
>  	atomic_t		num_inst;
>  	struct mutex		dev_mutex;
>  	spinlock_t		irqlock;
> @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev)
>  	return 0;
>  }
>  
> +static u32 pxp_read_version(struct pxp_dev *dev)
> +{
> +	return readl(dev->mmio + HW_PXP_VERSION);
> +}
> +
>  static int pxp_probe(struct platform_device *pdev)
>  {
>  	struct pxp_dev *dev;
> @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev)
>  		goto err_clk;
>  	}
>  
> +	dev->hw_version = pxp_read_version(dev);
> +	dev_info(&pdev->dev, "PXP Version %d.%d\n",

As the version can't be negative, I'd use %u.%u.

> +		 PXP_VERSION_MAJOR(dev->hw_version),
> +		 PXP_VERSION_MINOR(dev->hw_version));
> +

The driver now prints two messages at probe time, it would be nice to
combine them, or remove the other one. That's a candidate for a future
patch though.

>  	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>  	if (ret)
>  		goto err_clk;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/8] media: imx-pxp: detect PXP version

[Laurent Pinchart]

On Fri, Jan 06, 2023 at 01:47:32PM +0200, Laurent Pinchart wrote:
> Hi Michael,
> 
> Thank you for the patch.
> 
> On Thu, Jan 05, 2023 at 02:47:23PM +0100, Michael Tretter wrote:
> > Different versions of the Pixel Pipeline have different blocks and their
> > routing may be different. Read the PXP_HW_VERSION register to determine
> > the version of the PXP and print it to the log for debugging purposes.
> 
> Is there a specific reason you chose to read the version register
> instead of basing the decision on the compatible string ?

Reading the rest of the series, you use the compatible strings later,
and never use the hw_version field. I'm tempted to propose dropping this
patch.

> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > index 689ae5e6ac62..05abe40558b0 100644
> > --- a/drivers/media/platform/nxp/imx-pxp.c
> > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > @@ -10,6 +10,7 @@
> >   * Pawel Osciak, <pawel@osciak.com>
> >   * Marek Szyprowski, <m.szyprowski@samsung.com>
> >   */
> > +#include <linux/bitfield.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/dma-mapping.h>
> > @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info");
> >  #define MEM2MEM_HFLIP	(1 << 0)
> >  #define MEM2MEM_VFLIP	(1 << 1)
> >  
> > +#define PXP_VERSION_MAJOR(version) \
> > +	FIELD_GET(BM_PXP_VERSION_MAJOR, version)
> > +#define PXP_VERSION_MINOR(version) \
> > +	FIELD_GET(BM_PXP_VERSION_MINOR, version)
> > +
> >  #define dprintk(dev, fmt, arg...) \
> >  	v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg)
> >  
> > @@ -192,6 +198,8 @@ struct pxp_dev {
> >  	struct clk		*clk;
> >  	void __iomem		*mmio;
> >  
> > +	u32			hw_version;
> > +
> >  	atomic_t		num_inst;
> >  	struct mutex		dev_mutex;
> >  	spinlock_t		irqlock;
> > @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev)
> >  	return 0;
> >  }
> >  
> > +static u32 pxp_read_version(struct pxp_dev *dev)
> > +{
> > +	return readl(dev->mmio + HW_PXP_VERSION);
> > +}
> > +
> >  static int pxp_probe(struct platform_device *pdev)
> >  {
> >  	struct pxp_dev *dev;
> > @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev)
> >  		goto err_clk;
> >  	}
> >  
> > +	dev->hw_version = pxp_read_version(dev);
> > +	dev_info(&pdev->dev, "PXP Version %d.%d\n",
> 
> As the version can't be negative, I'd use %u.%u.
> 
> > +		 PXP_VERSION_MAJOR(dev->hw_version),
> > +		 PXP_VERSION_MINOR(dev->hw_version));
> > +
> 
> The driver now prints two messages at probe time, it would be nice to
> combine them, or remove the other one. That's a candidate for a future
> patch though.
> 
> >  	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> >  	if (ret)
> >  		goto err_clk;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/8] media: imx-pxp: detect PXP version

[Michael Tretter]

On Fri, 06 Jan 2023 14:28:47 +0200, Laurent Pinchart wrote:
> On Fri, Jan 06, 2023 at 01:47:32PM +0200, Laurent Pinchart wrote:
> > Thank you for the patch.
> > 
> > On Thu, Jan 05, 2023 at 02:47:23PM +0100, Michael Tretter wrote:
> > > Different versions of the Pixel Pipeline have different blocks and their
> > > routing may be different. Read the PXP_HW_VERSION register to determine
> > > the version of the PXP and print it to the log for debugging purposes.
> > 
> > Is there a specific reason you chose to read the version register
> > instead of basing the decision on the compatible string ?
> 
> Reading the rest of the series, you use the compatible strings later,
> and never use the hw_version field. I'm tempted to propose dropping this
> patch.

My first try was to use the version register, but it turned out that the
version is the same at least on i.MX6ULL and i.MX7D. I kept it to avoid that
others fall into the same trap.

> 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > > index 689ae5e6ac62..05abe40558b0 100644
> > > --- a/drivers/media/platform/nxp/imx-pxp.c
> > > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > > @@ -10,6 +10,7 @@
> > >   * Pawel Osciak, <pawel@osciak.com>
> > >   * Marek Szyprowski, <m.szyprowski@samsung.com>
> > >   */
> > > +#include <linux/bitfield.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/dma-mapping.h>
> > > @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info");
> > >  #define MEM2MEM_HFLIP	(1 << 0)
> > >  #define MEM2MEM_VFLIP	(1 << 1)
> > >  
> > > +#define PXP_VERSION_MAJOR(version) \
> > > +	FIELD_GET(BM_PXP_VERSION_MAJOR, version)
> > > +#define PXP_VERSION_MINOR(version) \
> > > +	FIELD_GET(BM_PXP_VERSION_MINOR, version)
> > > +
> > >  #define dprintk(dev, fmt, arg...) \
> > >  	v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg)
> > >  
> > > @@ -192,6 +198,8 @@ struct pxp_dev {
> > >  	struct clk		*clk;
> > >  	void __iomem		*mmio;
> > >  
> > > +	u32			hw_version;
> > > +
> > >  	atomic_t		num_inst;
> > >  	struct mutex		dev_mutex;
> > >  	spinlock_t		irqlock;
> > > @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev)
> > >  	return 0;
> > >  }
> > >  
> > > +static u32 pxp_read_version(struct pxp_dev *dev)
> > > +{
> > > +	return readl(dev->mmio + HW_PXP_VERSION);
> > > +}
> > > +
> > >  static int pxp_probe(struct platform_device *pdev)
> > >  {
> > >  	struct pxp_dev *dev;
> > > @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev)
> > >  		goto err_clk;
> > >  	}
> > >  
> > > +	dev->hw_version = pxp_read_version(dev);
> > > +	dev_info(&pdev->dev, "PXP Version %d.%d\n",
> > 
> > As the version can't be negative, I'd use %u.%u.
> > 
> > > +		 PXP_VERSION_MAJOR(dev->hw_version),
> > > +		 PXP_VERSION_MINOR(dev->hw_version));
> > > +
> > 
> > The driver now prints two messages at probe time, it would be nice to
> > combine them, or remove the other one. That's a candidate for a future
> > patch though.

I would reduce the level to dev_debug. Then the version is not always printed,
but it can be easily enabled if necessary for the bringup on another platform.

Michael

> > 
> > >  	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> > >  	if (ret)
> > >  		goto err_clk;

Re: [PATCH 2/8] media: imx-pxp: detect PXP version

[Laurent Pinchart]

On Fri, Jan 06, 2023 at 03:01:41PM +0100, Michael Tretter wrote:
> On Fri, 06 Jan 2023 14:28:47 +0200, Laurent Pinchart wrote:
> > On Fri, Jan 06, 2023 at 01:47:32PM +0200, Laurent Pinchart wrote:
> > > Thank you for the patch.
> > > 
> > > On Thu, Jan 05, 2023 at 02:47:23PM +0100, Michael Tretter wrote:
> > > > Different versions of the Pixel Pipeline have different blocks and their
> > > > routing may be different. Read the PXP_HW_VERSION register to determine
> > > > the version of the PXP and print it to the log for debugging purposes.
> > > 
> > > Is there a specific reason you chose to read the version register
> > > instead of basing the decision on the compatible string ?
> > 
> > Reading the rest of the series, you use the compatible strings later,
> > and never use the hw_version field. I'm tempted to propose dropping this
> > patch.
> 
> My first try was to use the version register, but it turned out that the
> version is the same at least on i.MX6ULL and i.MX7D. I kept it to avoid that
> others fall into the same trap.
> 
> > 
> > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > ---
> > > >  drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > > > index 689ae5e6ac62..05abe40558b0 100644
> > > > --- a/drivers/media/platform/nxp/imx-pxp.c
> > > > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > > > @@ -10,6 +10,7 @@
> > > >   * Pawel Osciak, <pawel@osciak.com>
> > > >   * Marek Szyprowski, <m.szyprowski@samsung.com>
> > > >   */
> > > > +#include <linux/bitfield.h>
> > > >  #include <linux/clk.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/dma-mapping.h>
> > > > @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info");
> > > >  #define MEM2MEM_HFLIP	(1 << 0)
> > > >  #define MEM2MEM_VFLIP	(1 << 1)
> > > >  
> > > > +#define PXP_VERSION_MAJOR(version) \
> > > > +	FIELD_GET(BM_PXP_VERSION_MAJOR, version)
> > > > +#define PXP_VERSION_MINOR(version) \
> > > > +	FIELD_GET(BM_PXP_VERSION_MINOR, version)
> > > > +
> > > >  #define dprintk(dev, fmt, arg...) \
> > > >  	v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg)
> > > >  
> > > > @@ -192,6 +198,8 @@ struct pxp_dev {
> > > >  	struct clk		*clk;
> > > >  	void __iomem		*mmio;
> > > >  
> > > > +	u32			hw_version;
> > > > +
> > > >  	atomic_t		num_inst;
> > > >  	struct mutex		dev_mutex;
> > > >  	spinlock_t		irqlock;
> > > > @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static u32 pxp_read_version(struct pxp_dev *dev)
> > > > +{
> > > > +	return readl(dev->mmio + HW_PXP_VERSION);
> > > > +}
> > > > +
> > > >  static int pxp_probe(struct platform_device *pdev)
> > > >  {
> > > >  	struct pxp_dev *dev;
> > > > @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev)
> > > >  		goto err_clk;
> > > >  	}
> > > >  
> > > > +	dev->hw_version = pxp_read_version(dev);
> > > > +	dev_info(&pdev->dev, "PXP Version %d.%d\n",
> > > 
> > > As the version can't be negative, I'd use %u.%u.
> > > 
> > > > +		 PXP_VERSION_MAJOR(dev->hw_version),
> > > > +		 PXP_VERSION_MINOR(dev->hw_version));
> > > > +
> > > 
> > > The driver now prints two messages at probe time, it would be nice to
> > > combine them, or remove the other one. That's a candidate for a future
> > > patch though.
> 
> I would reduce the level to dev_debug. Then the version is not always printed,
> but it can be easily enabled if necessary for the bringup on another platform.

Fine with me.

> > > >  	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> > > >  	if (ret)
> > > >  		goto err_clk;

-- 
Regards,

Laurent Pinchart

[PATCH 3/8] media: imx-pxp: extract helper function to setup data path

[Michael Tretter]

The driver must configure the data path through the Pixel Pipeline.

Currently, the driver is using a fixed setup, but once there are
different pipeline configurations, it is helpful to have a dedicated
function for determining the register value for the data path.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/media/platform/nxp/imx-pxp.c | 62 +++++++++++++++++++---------
 1 file changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index 05abe40558b0..a957fee88829 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -726,6 +726,47 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
 	}
 }
 
+static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
+{
+	u32 ctrl0;
+
+	ctrl0 = 0;
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
+
+	return ctrl0;
+}
+
+static void pxp_set_data_path(struct pxp_ctx *ctx)
+{
+	struct pxp_dev *dev = ctx->dev;
+	u32 ctrl0;
+	u32 ctrl1;
+
+	ctrl0 = pxp_data_path_ctrl0(ctx);
+
+	ctrl1 = 0;
+	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
+	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
+
+	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
+	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
+}
+
 static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
 		     struct vb2_v4l2_buffer *out_vb)
 {
@@ -912,26 +953,7 @@ static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
 	/* bypass LUT */
 	writel(BM_PXP_LUT_CTRL_BYPASS, dev->mmio + HW_PXP_LUT_CTRL);
 
-	writel(BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0)|
-	       BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0),
-	       dev->mmio + HW_PXP_DATA_PATH_CTRL0);
-	writel(BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1) |
-	       BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1),
-	       dev->mmio + HW_PXP_DATA_PATH_CTRL1);
+	pxp_set_data_path(ctx);
 
 	writel(0xffff, dev->mmio + HW_PXP_IRQ_MASK);
 
-- 
2.30.2

Re: [PATCH 3/8] media: imx-pxp: extract helper function to setup data path

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:24PM +0100, Michael Tretter wrote:
> The driver must configure the data path through the Pixel Pipeline.
> 
> Currently, the driver is using a fixed setup, but once there are
> different pipeline configurations, it is helpful to have a dedicated
> function for determining the register value for the data path.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/nxp/imx-pxp.c | 62 +++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index 05abe40558b0..a957fee88829 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -726,6 +726,47 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
>  	}
>  }
>  
> +static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> +{
> +	u32 ctrl0;
> +
> +	ctrl0 = 0;
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
> +
> +	return ctrl0;
> +}
> +
> +static void pxp_set_data_path(struct pxp_ctx *ctx)
> +{
> +	struct pxp_dev *dev = ctx->dev;
> +	u32 ctrl0;
> +	u32 ctrl1;
> +
> +	ctrl0 = pxp_data_path_ctrl0(ctx);
> +
> +	ctrl1 = 0;
> +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
> +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
> +
> +	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> +	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
> +}
> +
>  static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
>  		     struct vb2_v4l2_buffer *out_vb)
>  {
> @@ -912,26 +953,7 @@ static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
>  	/* bypass LUT */
>  	writel(BM_PXP_LUT_CTRL_BYPASS, dev->mmio + HW_PXP_LUT_CTRL);
>  
> -	writel(BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0)|
> -	       BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0),
> -	       dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> -	writel(BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1) |
> -	       BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1),
> -	       dev->mmio + HW_PXP_DATA_PATH_CTRL1);
> +	pxp_set_data_path(ctx);
>  
>  	writel(0xffff, dev->mmio + HW_PXP_IRQ_MASK);
>  

-- 
Regards,

Laurent Pinchart

[PATCH 4/8] media: imx-pxp: explicitly disable unused blocks

[Michael Tretter]

Various multiplexers in the pipeline are not used with the currently
configured data path. Disable all unused multiplexers by selecting the
"no output" (3) option.

The datasheet doesn't explicitly require this, but the PXP has been seen
to hang after processing a few hundreds of frames otherwise.

As at it, add documentation for the multiplexers that are actually
relevant for the data path.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/media/platform/nxp/imx-pxp.c | 30 +++++++++++++++++-----------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index a957fee88829..6ffd07cda965 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -731,22 +731,28 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
 	u32 ctrl0;
 
 	ctrl0 = 0;
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
+	/* Bypass Dithering x3CH */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
+	/* Select Rotation */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
+	/* Select LUT */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
+	/* Select MUX8 for LUT */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
+	/* Select CSC 2 */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
+	/* Bypass Rotation 2 */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);
 
 	return ctrl0;
 }
@@ -760,8 +766,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
 	ctrl0 = pxp_data_path_ctrl0(ctx);
 
 	ctrl1 = 0;
-	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
-	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
+	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
+	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
 
 	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
 	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
-- 
2.30.2

Re: [PATCH 4/8] media: imx-pxp: explicitly disable unused blocks

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:25PM +0100, Michael Tretter wrote:
> Various multiplexers in the pipeline are not used with the currently
> configured data path. Disable all unused multiplexers by selecting the
> "no output" (3) option.
> 
> The datasheet doesn't explicitly require this, but the PXP has been seen
> to hang after processing a few hundreds of frames otherwise.

On which platform(s) have you noticed that ?

> As at it, add documentation for the multiplexers that are actually
> relevant for the data path.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/media/platform/nxp/imx-pxp.c | 30 +++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index a957fee88829..6ffd07cda965 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -731,22 +731,28 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
>  	u32 ctrl0;
>  
>  	ctrl0 = 0;
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
> +	/* Bypass Dithering x3CH */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
> +	/* Select Rotation */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
> +	/* Select LUT */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
> +	/* Select MUX8 for LUT */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
> +	/* Select CSC 2 */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
> +	/* Bypass Rotation 2 */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);

The muxes being disabled look fine to me, but the values of MUX8, MUX12
and MUX14 look strange based on the i.MX7D reference manual. Maybe the
register values were different in previous SoCs ? I haven't found any
other relevant reference manual that document the mux values, I may have
overlooked something.

Anyway, this isn't an issue with this patch, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	return ctrl0;
>  }
> @@ -760,8 +766,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
>  	ctrl0 = pxp_data_path_ctrl0(ctx);
>  
>  	ctrl1 = 0;
> -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
> -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
> +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
> +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
>  
>  	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
>  	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/8] media: imx-pxp: explicitly disable unused blocks

[Michael Tretter]

On Fri, 06 Jan 2023 14:26:40 +0200, Laurent Pinchart wrote:
> On Thu, Jan 05, 2023 at 02:47:25PM +0100, Michael Tretter wrote:
> > Various multiplexers in the pipeline are not used with the currently
> > configured data path. Disable all unused multiplexers by selecting the
> > "no output" (3) option.
> > 
> > The datasheet doesn't explicitly require this, but the PXP has been seen
> > to hang after processing a few hundreds of frames otherwise.
> 
> On which platform(s) have you noticed that ?

I didn't observe this myself, but took this information from the comment in
your earlier patch [0] that disables the unused multiplexers.

https://lore.kernel.org/linux-media/20200510223100.11641-2-laurent.pinchart@ideasonboard.com/

> 
> > As at it, add documentation for the multiplexers that are actually
> > relevant for the data path.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/media/platform/nxp/imx-pxp.c | 30 +++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > index a957fee88829..6ffd07cda965 100644
> > --- a/drivers/media/platform/nxp/imx-pxp.c
> > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > @@ -731,22 +731,28 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> >  	u32 ctrl0;
> >  
> >  	ctrl0 = 0;
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
> > +	/* Bypass Dithering x3CH */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
> > +	/* Select Rotation */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
> > +	/* Select LUT */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
> > +	/* Select MUX8 for LUT */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
> > +	/* Select CSC 2 */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
> > +	/* Bypass Rotation 2 */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);
> 
> The muxes being disabled look fine to me, but the values of MUX8, MUX12
> and MUX14 look strange based on the i.MX7D reference manual. Maybe the
> register values were different in previous SoCs ? I haven't found any
> other relevant reference manual that document the mux values, I may have
> overlooked something.

The MUX8, MUX12 and MUX14 are documented in the i.MX6ULL reference manual
section 41.11.51 and their location and function in the data path is shown in
Figure 41-1. "PXP Architecture" on page 2490.

Michael

> 
> Anyway, this isn't an issue with this patch, so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  
> >  	return ctrl0;
> >  }
> > @@ -760,8 +766,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
> >  	ctrl0 = pxp_data_path_ctrl0(ctx);
> >  
> >  	ctrl1 = 0;
> > -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
> > -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
> > +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
> > +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
> >  
> >  	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> >  	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);

Re: [PATCH 4/8] media: imx-pxp: explicitly disable unused blocks

[Laurent Pinchart]

Hi Michael,

On Fri, Jan 06, 2023 at 03:08:55PM +0100, Michael Tretter wrote:
> On Fri, 06 Jan 2023 14:26:40 +0200, Laurent Pinchart wrote:
> > On Thu, Jan 05, 2023 at 02:47:25PM +0100, Michael Tretter wrote:
> > > Various multiplexers in the pipeline are not used with the currently
> > > configured data path. Disable all unused multiplexers by selecting the
> > > "no output" (3) option.
> > > 
> > > The datasheet doesn't explicitly require this, but the PXP has been seen
> > > to hang after processing a few hundreds of frames otherwise.
> > 
> > On which platform(s) have you noticed that ?
> 
> I didn't observe this myself, but took this information from the comment in
> your earlier patch [0] that disables the unused multiplexers.
> 
> https://lore.kernel.org/linux-media/20200510223100.11641-2-laurent.pinchart@ideasonboard.com/

I suppose I should trust myself :-)

> > > As at it, add documentation for the multiplexers that are actually
> > > relevant for the data path.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/media/platform/nxp/imx-pxp.c | 30 +++++++++++++++++-----------
> > >  1 file changed, 18 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > > index a957fee88829..6ffd07cda965 100644
> > > --- a/drivers/media/platform/nxp/imx-pxp.c
> > > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > > @@ -731,22 +731,28 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> > >  	u32 ctrl0;
> > >  
> > >  	ctrl0 = 0;
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
> > > +	/* Bypass Dithering x3CH */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
> > > +	/* Select Rotation */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
> > > +	/* Select LUT */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
> > > +	/* Select MUX8 for LUT */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
> > > +	/* Select CSC 2 */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
> > > +	/* Bypass Rotation 2 */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);
> > 
> > The muxes being disabled look fine to me, but the values of MUX8, MUX12
> > and MUX14 look strange based on the i.MX7D reference manual. Maybe the
> > register values were different in previous SoCs ? I haven't found any
> > other relevant reference manual that document the mux values, I may have
> > overlooked something.
> 
> The MUX8, MUX12 and MUX14 are documented in the i.MX6ULL reference manual
> section 41.11.51 and their location and function in the data path is shown in
> Figure 41-1. "PXP Architecture" on page 2490.

I've seen that, but as far as I can tell, the description of the
register in section 41.11.51 doesn't tell what the different MUX* field
value map to. Figure 41-1 shows that, for instance, MUX8 handles CSC2
bypass, but it doesn't tell what value corresponds to what path.

> > Anyway, this isn't an issue with this patch, so
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > >  
> > >  	return ctrl0;
> > >  }
> > > @@ -760,8 +766,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
> > >  	ctrl0 = pxp_data_path_ctrl0(ctx);
> > >  
> > >  	ctrl1 = 0;
> > > -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
> > > -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
> > > +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
> > > +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
> > >  
> > >  	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> > >  	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);

-- 
Regards,

Laurent Pinchart

[PATCH 5/8] media: imx-pxp: disable LUT block

[Michael Tretter]

The LUT block is always configured in bypass mode.

Take it entirely out of the pipeline by disabling it and routing the
data path around the LUT.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/media/platform/nxp/imx-pxp.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index 6ffd07cda965..1d649b9cadad 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -737,11 +737,10 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
 	/* Select Rotation */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
-	/* Select LUT */
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
+	/* Bypass LUT */
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(1);
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
-	/* Select MUX8 for LUT */
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(3);
 	/* Select CSC 2 */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
@@ -966,7 +965,7 @@ static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
 	/* ungate, enable PS/AS/OUT and PXP operation */
 	writel(BM_PXP_CTRL_IRQ_ENABLE, dev->mmio + HW_PXP_CTRL_SET);
 	writel(BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
-	       BM_PXP_CTRL_ENABLE_LUT | BM_PXP_CTRL_ENABLE_ROTATE0 |
+	       BM_PXP_CTRL_ENABLE_ROTATE0 |
 	       BM_PXP_CTRL_ENABLE_PS_AS_OUT, dev->mmio + HW_PXP_CTRL_SET);
 
 	return 0;
-- 
2.30.2

Re: [PATCH 5/8] media: imx-pxp: disable LUT block

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:26PM +0100, Michael Tretter wrote:
> The LUT block is always configured in bypass mode.
> 
> Take it entirely out of the pipeline by disabling it and routing the
> data path around the LUT.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/nxp/imx-pxp.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index 6ffd07cda965..1d649b9cadad 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -737,11 +737,10 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
>  	/* Select Rotation */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
> -	/* Select LUT */
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
> +	/* Bypass LUT */
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(1);
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
> -	/* Select MUX8 for LUT */
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(3);
>  	/* Select CSC 2 */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
> @@ -966,7 +965,7 @@ static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
>  	/* ungate, enable PS/AS/OUT and PXP operation */
>  	writel(BM_PXP_CTRL_IRQ_ENABLE, dev->mmio + HW_PXP_CTRL_SET);
>  	writel(BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> -	       BM_PXP_CTRL_ENABLE_LUT | BM_PXP_CTRL_ENABLE_ROTATE0 |
> +	       BM_PXP_CTRL_ENABLE_ROTATE0 |
>  	       BM_PXP_CTRL_ENABLE_PS_AS_OUT, dev->mmio + HW_PXP_CTRL_SET);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

[PATCH 6/8] media: imx-pxp: make data_path_ctrl0 platform dependent

[Michael Tretter]

Unfortunately, the PXP_HW_VERSION register reports the PXP on the i.MX7D
and on the i.MX6ULL as version 3.0, although the PXP versions on these
SoCs have significant differences.

Use the compatible to configure the ctrl0 register as required dependent
on the platform.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/media/platform/nxp/imx-pxp.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index 1d649b9cadad..4e182f80a36b 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -19,6 +19,7 @@
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 
@@ -191,6 +192,11 @@ static struct pxp_fmt *find_format(struct v4l2_format *f)
 	return &formats[k];
 }
 
+struct pxp_ctx;
+struct pxp_pdata {
+	u32 (*data_path_ctrl0)(struct pxp_ctx *ctx);
+};
+
 struct pxp_dev {
 	struct v4l2_device	v4l2_dev;
 	struct video_device	vfd;
@@ -199,6 +205,7 @@ struct pxp_dev {
 	void __iomem		*mmio;
 
 	u32			hw_version;
+	const struct pxp_pdata	*pdata;
 
 	atomic_t		num_inst;
 	struct mutex		dev_mutex;
@@ -726,7 +733,7 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
 	}
 }
 
-static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
+static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx)
 {
 	u32 ctrl0;
 
@@ -756,6 +763,16 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
 	return ctrl0;
 }
 
+static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
+{
+	struct pxp_dev *dev = ctx->dev;
+
+	if (dev->pdata && dev->pdata->data_path_ctrl0)
+		return dev->pdata->data_path_ctrl0(ctx);
+
+	return pxp_imx6ull_data_path_ctrl0(ctx);
+}
+
 static void pxp_set_data_path(struct pxp_ctx *ctx)
 {
 	struct pxp_dev *dev = ctx->dev;
@@ -1711,6 +1728,8 @@ static int pxp_probe(struct platform_device *pdev)
 	if (!dev)
 		return -ENOMEM;
 
+	dev->pdata = of_device_get_match_data(&pdev->dev);
+
 	dev->clk = devm_clk_get(&pdev->dev, "axi");
 	if (IS_ERR(dev->clk)) {
 		ret = PTR_ERR(dev->clk);
@@ -1811,8 +1830,12 @@ static int pxp_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct pxp_pdata pxp_imx6ull_pdata = {
+	.data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0,
+};
+
 static const struct of_device_id pxp_dt_ids[] = {
-	{ .compatible = "fsl,imx6ull-pxp", .data = NULL },
+	{ .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, pxp_dt_ids);
-- 
2.30.2

Re: [PATCH 6/8] media: imx-pxp: make data_path_ctrl0 platform dependent

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:27PM +0100, Michael Tretter wrote:
> Unfortunately, the PXP_HW_VERSION register reports the PXP on the i.MX7D
> and on the i.MX6ULL as version 3.0, although the PXP versions on these
> SoCs have significant differences.
> 
> Use the compatible to configure the ctrl0 register as required dependent
> on the platform.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/media/platform/nxp/imx-pxp.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index 1d649b9cadad..4e182f80a36b 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -19,6 +19,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  
> @@ -191,6 +192,11 @@ static struct pxp_fmt *find_format(struct v4l2_format *f)
>  	return &formats[k];
>  }
>  
> +struct pxp_ctx;

Please add a blank line here.

> +struct pxp_pdata {
> +	u32 (*data_path_ctrl0)(struct pxp_ctx *ctx);
> +};
> +
>  struct pxp_dev {
>  	struct v4l2_device	v4l2_dev;
>  	struct video_device	vfd;
> @@ -199,6 +205,7 @@ struct pxp_dev {
>  	void __iomem		*mmio;
>  
>  	u32			hw_version;
> +	const struct pxp_pdata	*pdata;
>  
>  	atomic_t		num_inst;
>  	struct mutex		dev_mutex;
> @@ -726,7 +733,7 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
>  	}
>  }
>  
> -static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> +static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx)
>  {
>  	u32 ctrl0;
>  
> @@ -756,6 +763,16 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
>  	return ctrl0;
>  }
>  
> +static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> +{
> +	struct pxp_dev *dev = ctx->dev;
> +
> +	if (dev->pdata && dev->pdata->data_path_ctrl0)
> +		return dev->pdata->data_path_ctrl0(ctx);
> +
> +	return pxp_imx6ull_data_path_ctrl0(ctx);

Do you need this fallback, given that all compatible strings give you
valid pdata ? I'd rather be explicit.

This function then becomes so small that I would inline it in the
caller.

> +}
> +
>  static void pxp_set_data_path(struct pxp_ctx *ctx)
>  {
>  	struct pxp_dev *dev = ctx->dev;
> @@ -1711,6 +1728,8 @@ static int pxp_probe(struct platform_device *pdev)
>  	if (!dev)
>  		return -ENOMEM;
>  
> +	dev->pdata = of_device_get_match_data(&pdev->dev);
> +
>  	dev->clk = devm_clk_get(&pdev->dev, "axi");
>  	if (IS_ERR(dev->clk)) {
>  		ret = PTR_ERR(dev->clk);
> @@ -1811,8 +1830,12 @@ static int pxp_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct pxp_pdata pxp_imx6ull_pdata = {
> +	.data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0,
> +};
> +
>  static const struct of_device_id pxp_dt_ids[] = {
> -	{ .compatible = "fsl,imx6ull-pxp", .data = NULL },
> +	{ .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, pxp_dt_ids);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] media: imx-pxp: make data_path_ctrl0 platform dependent

[Michael Tretter]

On Fri, 06 Jan 2023 14:30:52 +0200, Laurent Pinchart wrote:
> On Thu, Jan 05, 2023 at 02:47:27PM +0100, Michael Tretter wrote:
> > Unfortunately, the PXP_HW_VERSION register reports the PXP on the i.MX7D
> > and on the i.MX6ULL as version 3.0, although the PXP versions on these
> > SoCs have significant differences.
> > 
> > Use the compatible to configure the ctrl0 register as required dependent
> > on the platform.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/media/platform/nxp/imx-pxp.c | 27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > index 1d649b9cadad..4e182f80a36b 100644
> > --- a/drivers/media/platform/nxp/imx-pxp.c
> > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> >  
> > @@ -191,6 +192,11 @@ static struct pxp_fmt *find_format(struct v4l2_format *f)
> >  	return &formats[k];
> >  }
> >  
> > +struct pxp_ctx;
> 
> Please add a blank line here.
> 
> > +struct pxp_pdata {
> > +	u32 (*data_path_ctrl0)(struct pxp_ctx *ctx);
> > +};
> > +
> >  struct pxp_dev {
> >  	struct v4l2_device	v4l2_dev;
> >  	struct video_device	vfd;
> > @@ -199,6 +205,7 @@ struct pxp_dev {
> >  	void __iomem		*mmio;
> >  
> >  	u32			hw_version;
> > +	const struct pxp_pdata	*pdata;
> >  
> >  	atomic_t		num_inst;
> >  	struct mutex		dev_mutex;
> > @@ -726,7 +733,7 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
> >  	}
> >  }
> >  
> > -static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> > +static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx)
> >  {
> >  	u32 ctrl0;
> >  
> > @@ -756,6 +763,16 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> >  	return ctrl0;
> >  }
> >  
> > +static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> > +{
> > +	struct pxp_dev *dev = ctx->dev;
> > +
> > +	if (dev->pdata && dev->pdata->data_path_ctrl0)
> > +		return dev->pdata->data_path_ctrl0(ctx);
> > +
> > +	return pxp_imx6ull_data_path_ctrl0(ctx);
> 
> Do you need this fallback, given that all compatible strings give you
> valid pdata ? I'd rather be explicit.
> 
> This function then becomes so small that I would inline it in the
> caller.

I was a bit paranoid that there may be cases in which pdata is not set. I will
change this to assume that pdata is always valid and just be explicit.

Michael

> 
> > +}
> > +
> >  static void pxp_set_data_path(struct pxp_ctx *ctx)
> >  {
> >  	struct pxp_dev *dev = ctx->dev;
> > @@ -1711,6 +1728,8 @@ static int pxp_probe(struct platform_device *pdev)
> >  	if (!dev)
> >  		return -ENOMEM;
> >  
> > +	dev->pdata = of_device_get_match_data(&pdev->dev);
> > +
> >  	dev->clk = devm_clk_get(&pdev->dev, "axi");
> >  	if (IS_ERR(dev->clk)) {
> >  		ret = PTR_ERR(dev->clk);
> > @@ -1811,8 +1830,12 @@ static int pxp_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static const struct pxp_pdata pxp_imx6ull_pdata = {
> > +	.data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0,
> > +};
> > +
> >  static const struct of_device_id pxp_dt_ids[] = {
> > -	{ .compatible = "fsl,imx6ull-pxp", .data = NULL },
> > +	{ .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(of, pxp_dt_ids);

Re: [PATCH 6/8] media: imx-pxp: make data_path_ctrl0 platform dependent

[Laurent Pinchart]

On Fri, Jan 06, 2023 at 03:11:41PM +0100, Michael Tretter wrote:
> On Fri, 06 Jan 2023 14:30:52 +0200, Laurent Pinchart wrote:
> > On Thu, Jan 05, 2023 at 02:47:27PM +0100, Michael Tretter wrote:
> > > Unfortunately, the PXP_HW_VERSION register reports the PXP on the i.MX7D
> > > and on the i.MX6ULL as version 3.0, although the PXP versions on these
> > > SoCs have significant differences.
> > > 
> > > Use the compatible to configure the ctrl0 register as required dependent
> > > on the platform.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/media/platform/nxp/imx-pxp.c | 27 +++++++++++++++++++++++++--
> > >  1 file changed, 25 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > > index 1d649b9cadad..4e182f80a36b 100644
> > > --- a/drivers/media/platform/nxp/imx-pxp.c
> > > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/iopoll.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/slab.h>
> > >  
> > > @@ -191,6 +192,11 @@ static struct pxp_fmt *find_format(struct v4l2_format *f)
> > >  	return &formats[k];
> > >  }
> > >  
> > > +struct pxp_ctx;
> > 
> > Please add a blank line here.
> > 
> > > +struct pxp_pdata {
> > > +	u32 (*data_path_ctrl0)(struct pxp_ctx *ctx);
> > > +};
> > > +
> > >  struct pxp_dev {
> > >  	struct v4l2_device	v4l2_dev;
> > >  	struct video_device	vfd;
> > > @@ -199,6 +205,7 @@ struct pxp_dev {
> > >  	void __iomem		*mmio;
> > >  
> > >  	u32			hw_version;
> > > +	const struct pxp_pdata	*pdata;
> > >  
> > >  	atomic_t		num_inst;
> > >  	struct mutex		dev_mutex;
> > > @@ -726,7 +733,7 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
> > >  	}
> > >  }
> > >  
> > > -static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> > > +static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx)
> > >  {
> > >  	u32 ctrl0;
> > >  
> > > @@ -756,6 +763,16 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> > >  	return ctrl0;
> > >  }
> > >  
> > > +static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> > > +{
> > > +	struct pxp_dev *dev = ctx->dev;
> > > +
> > > +	if (dev->pdata && dev->pdata->data_path_ctrl0)
> > > +		return dev->pdata->data_path_ctrl0(ctx);
> > > +
> > > +	return pxp_imx6ull_data_path_ctrl0(ctx);
> > 
> > Do you need this fallback, given that all compatible strings give you
> > valid pdata ? I'd rather be explicit.
> > 
> > This function then becomes so small that I would inline it in the
> > caller.
> 
> I was a bit paranoid that there may be cases in which pdata is not set. I will
> change this to assume that pdata is always valid and just be explicit.

If you're worried that future addition of platform support could add a
compatible string with NULL .data, you could catch that in probe(). I'm
not worried personally, as it would crash on first use, so the developer
submitting the patch should notice.

> > > +}
> > > +
> > >  static void pxp_set_data_path(struct pxp_ctx *ctx)
> > >  {
> > >  	struct pxp_dev *dev = ctx->dev;
> > > @@ -1711,6 +1728,8 @@ static int pxp_probe(struct platform_device *pdev)
> > >  	if (!dev)
> > >  		return -ENOMEM;
> > >  
> > > +	dev->pdata = of_device_get_match_data(&pdev->dev);
> > > +
> > >  	dev->clk = devm_clk_get(&pdev->dev, "axi");
> > >  	if (IS_ERR(dev->clk)) {
> > >  		ret = PTR_ERR(dev->clk);
> > > @@ -1811,8 +1830,12 @@ static int pxp_remove(struct platform_device *pdev)
> > >  	return 0;
> > >  }
> > >  
> > > +static const struct pxp_pdata pxp_imx6ull_pdata = {
> > > +	.data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0,
> > > +};
> > > +
> > >  static const struct of_device_id pxp_dt_ids[] = {
> > > -	{ .compatible = "fsl,imx6ull-pxp", .data = NULL },
> > > +	{ .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata },
> > >  	{ },
> > >  };
> > >  MODULE_DEVICE_TABLE(of, pxp_dt_ids);

-- 
Regards,

Laurent Pinchart

[PATCH 7/8] media: imx-pxp: add support for i.MX7D

[Michael Tretter]

The i.MX7D needs a different data path configuration than the i.MX6ULL.
Configure the data path as close as possible to the data path on the
i.MX6ULL.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/media/platform/nxp/imx-pxp.c | 36 ++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index 4e182f80a36b..04cc8df2a498 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -763,6 +763,37 @@ static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx)
 	return ctrl0;
 }
 
+static u32 pxp_imx7d_data_path_ctrl0(struct pxp_ctx *ctx)
+{
+	u32 ctrl0;
+
+	ctrl0 = 0;
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
+	/* Select Rotation 0 */
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
+	/* Select MUX11 for Rotation 0 */
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(1);
+	/* Bypass LUT */
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(1);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(3);
+	/* Select CSC 2 */
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
+	/* Select Composite Alpha Blending/Color Key 0 for CSC 2 */
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(1);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
+	/* Bypass Rotation 1 */
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);
+
+	return ctrl0;
+}
+
 static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
 {
 	struct pxp_dev *dev = ctx->dev;
@@ -1834,8 +1865,13 @@ static const struct pxp_pdata pxp_imx6ull_pdata = {
 	.data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0,
 };
 
+static const struct pxp_pdata pxp_imx7d_pdata = {
+	.data_path_ctrl0 = pxp_imx7d_data_path_ctrl0,
+};
+
 static const struct of_device_id pxp_dt_ids[] = {
 	{ .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata },
+	{ .compatible = "fsl,imx7d-pxp", .data = &pxp_imx7d_pdata },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, pxp_dt_ids);
-- 
2.30.2

Re: [PATCH 7/8] media: imx-pxp: add support for i.MX7D

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:28PM +0100, Michael Tretter wrote:
> The i.MX7D needs a different data path configuration than the i.MX6ULL.
> Configure the data path as close as possible to the data path on the
> i.MX6ULL.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/nxp/imx-pxp.c | 36 ++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index 4e182f80a36b..04cc8df2a498 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -763,6 +763,37 @@ static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx)
>  	return ctrl0;
>  }
>  
> +static u32 pxp_imx7d_data_path_ctrl0(struct pxp_ctx *ctx)
> +{
> +	u32 ctrl0;
> +
> +	ctrl0 = 0;
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
> +	/* Select Rotation 0 */
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
> +	/* Select MUX11 for Rotation 0 */
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(1);
> +	/* Bypass LUT */
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(1);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(3);
> +	/* Select CSC 2 */
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
> +	/* Select Composite Alpha Blending/Color Key 0 for CSC 2 */
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(1);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
> +	/* Bypass Rotation 1 */
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);
> +
> +	return ctrl0;
> +}
> +
>  static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
>  {
>  	struct pxp_dev *dev = ctx->dev;
> @@ -1834,8 +1865,13 @@ static const struct pxp_pdata pxp_imx6ull_pdata = {
>  	.data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0,
>  };
>  
> +static const struct pxp_pdata pxp_imx7d_pdata = {
> +	.data_path_ctrl0 = pxp_imx7d_data_path_ctrl0,
> +};
> +
>  static const struct of_device_id pxp_dt_ids[] = {
>  	{ .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata },
> +	{ .compatible = "fsl,imx7d-pxp", .data = &pxp_imx7d_pdata },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, pxp_dt_ids);

-- 
Regards,

Laurent Pinchart

[PATCH 8/8] ARM: dts: imx7d: add node for PXP

[Michael Tretter]

The i.MX7d contains a Pixel Pipeline in version 3.0. Add the device tree
node to make it available.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 arch/arm/boot/dts/imx7d.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index 7ceb7c09f7ad..728cc9413a7c 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -165,6 +165,15 @@ pcie_phy: pcie-phy@306d0000 {
 		  reg = <0x306d0000 0x10000>;
 		  status = "disabled";
 	};
+
+	pxp: pxp@30700000 {
+		compatible = "fsl,imx7d-pxp";
+		reg = <0x30700000 0x10000>;
+		interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&clks IMX7D_PXP_CLK>;
+		clock-names = "axi";
+	};
 };
 
 &aips3 {
-- 
2.30.2

Re: [PATCH 8/8] ARM: dts: imx7d: add node for PXP

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:29PM +0100, Michael Tretter wrote:
> The i.MX7d contains a Pixel Pipeline in version 3.0. Add the device tree
> node to make it available.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  arch/arm/boot/dts/imx7d.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
> index 7ceb7c09f7ad..728cc9413a7c 100644
> --- a/arch/arm/boot/dts/imx7d.dtsi
> +++ b/arch/arm/boot/dts/imx7d.dtsi
> @@ -165,6 +165,15 @@ pcie_phy: pcie-phy@306d0000 {
>  		  reg = <0x306d0000 0x10000>;
>  		  status = "disabled";
>  	};
> +
> +	pxp: pxp@30700000 {
> +		compatible = "fsl,imx7d-pxp";

Hmmm... The i.MX7S also has a PXP that seems compatible. I thus wonder
if we shouldn't move this node to imx7s.dtsi.

> +		reg = <0x30700000 0x10000>;
> +		interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +			<GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;

Nitpicking, alignment ?

> +		clocks = <&clks IMX7D_PXP_CLK>;
> +		clock-names = "axi";
> +	};
>  };
>  
>  &aips3 {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] ARM: dts: imx7d: add node for PXP

[Michael Tretter]

On Fri, 06 Jan 2023 14:36:41 +0200, Laurent Pinchart wrote:
> Hi Michael,
> 
> Thank you for the patch.
> 
> On Thu, Jan 05, 2023 at 02:47:29PM +0100, Michael Tretter wrote:
> > The i.MX7d contains a Pixel Pipeline in version 3.0. Add the device tree
> > node to make it available.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  arch/arm/boot/dts/imx7d.dtsi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
> > index 7ceb7c09f7ad..728cc9413a7c 100644
> > --- a/arch/arm/boot/dts/imx7d.dtsi
> > +++ b/arch/arm/boot/dts/imx7d.dtsi
> > @@ -165,6 +165,15 @@ pcie_phy: pcie-phy@306d0000 {
> >  		  reg = <0x306d0000 0x10000>;
> >  		  status = "disabled";
> >  	};
> > +
> > +	pxp: pxp@30700000 {
> > +		compatible = "fsl,imx7d-pxp";
> 
> Hmmm... The i.MX7S also has a PXP that seems compatible. I thus wonder
> if we shouldn't move this node to imx7s.dtsi.

The i.MX7S has a PXP at the same address, but the architecture in the
reference manual (Figure 13-71. PXP Architecture, p. 3797) looks slightly
different wrt. the location of the multiplexers. The reference manual is also
conspicuously lacking documentation of the DATA_PATH_CTRL0 register.

I wouldn't risk adding the node to the imx7s.dtsi and would rather keep the
option to add a different compatible for the i.MX7S to be able to handle the
difference.

Michael

> 
> > +		reg = <0x30700000 0x10000>;
> > +		interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > +			<GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> 
> Nitpicking, alignment ?
> 
> > +		clocks = <&clks IMX7D_PXP_CLK>;
> > +		clock-names = "axi";
> > +	};
> >  };
> >  
> >  &aips3 {

Re: [PATCH 8/8] ARM: dts: imx7d: add node for PXP

[Laurent Pinchart]

On Fri, Jan 06, 2023 at 03:36:21PM +0100, Michael Tretter wrote:
> On Fri, 06 Jan 2023 14:36:41 +0200, Laurent Pinchart wrote:
> > Hi Michael,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Jan 05, 2023 at 02:47:29PM +0100, Michael Tretter wrote:
> > > The i.MX7d contains a Pixel Pipeline in version 3.0. Add the device tree
> > > node to make it available.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  arch/arm/boot/dts/imx7d.dtsi | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
> > > index 7ceb7c09f7ad..728cc9413a7c 100644
> > > --- a/arch/arm/boot/dts/imx7d.dtsi
> > > +++ b/arch/arm/boot/dts/imx7d.dtsi
> > > @@ -165,6 +165,15 @@ pcie_phy: pcie-phy@306d0000 {
> > >  		  reg = <0x306d0000 0x10000>;
> > >  		  status = "disabled";
> > >  	};
> > > +
> > > +	pxp: pxp@30700000 {
> > > +		compatible = "fsl,imx7d-pxp";
> > 
> > Hmmm... The i.MX7S also has a PXP that seems compatible. I thus wonder
> > if we shouldn't move this node to imx7s.dtsi.
> 
> The i.MX7S has a PXP at the same address, but the architecture in the
> reference manual (Figure 13-71. PXP Architecture, p. 3797) looks slightly
> different wrt. the location of the multiplexers. The reference manual is also
> conspicuously lacking documentation of the DATA_PATH_CTRL0 register.
> 
> I wouldn't risk adding the node to the imx7s.dtsi and would rather keep the
> option to add a different compatible for the i.MX7S to be able to handle the
> difference.

OK, fine with me.

> > > +		reg = <0x30700000 0x10000>;
> > > +		interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > +			<GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> > 
> > Nitpicking, alignment ?
> > 
> > > +		clocks = <&clks IMX7D_PXP_CLK>;
> > > +		clock-names = "axi";
> > > +	};
> > >  };
> > >  
> > >  &aips3 {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/8] media: imx-pxp: add support for i.MX7D

[Laurent Pinchart]

Hi Michael,

On Thu, Jan 05, 2023 at 02:47:21PM +0100, Michael Tretter wrote:
> This series adds support for the PXP found on the i.MX7D to the imx-pxp
> driver.
> 
> The PXP on the i.MX7D has a few differences compared to the one on the
> i.MX6ULL. Especially, it has more processing blocks and slightly different
> multiplexers to route the data between the blocks. Therefore, the driver must
> configure a different data path depending on the platform.
> 
> While the PXP has a version register, the reported version is the same on the
> i.MX6ULL and the i.MX7D. Therefore, we cannot use the version register to
> change the driver behavior, but have to use the device tree compatible. The
> driver still prints the found version to the log to help bringing up the PXP
> on further platforms.
> 
> The patches are inspired by some earlier patches [0] by Laurent to add PXP
> support to the i.MX7d. Compared to the earlier patches, these patches add
> different behavior depending on the platform. Furthermore, the patches disable
> only the LUT block, but keep the rotator block enabled, as it may now be
> configured via the V4L2 rotate control.

Sounds good to me.

> Patch 1 converts the dt-binding to yaml.
> 
> Patches 2 to 5 cleanup and refactor the driver in preparation of handling
> different PXP versions.
> 
> Patches 6 and 7 add the handling of different platforms and the i.MX7d
> specific configuration.
> 
> Patch 8 adds the device tree node for the PXP to the i.MX7d device tree.

I've reviewed the whole series and comments are mostly minor. As you're
reminding me of the PXP, I'll take this as an opportunity to post
patches that I've had in my tree for way too long :-) There will be
minor conflicts with yours, so I'll first rebase them on this series,
assuming that v2 will be similar in places where conflicts occur.

> Michael
> 
> [0] https://lore.kernel.org/linux-media/20200510223100.11641-1-laurent.pinchart@ideasonboard.com/
> 
> Michael Tretter (8):
>   media: dt-bindings: media: fsl-pxp: convert to yaml
>   media: imx-pxp: detect PXP version
>   media: imx-pxp: extract helper function to setup data path
>   media: imx-pxp: explicitly disable unused blocks
>   media: imx-pxp: disable LUT block
>   media: imx-pxp: make data_path_ctrl0 platform dependent
>   media: imx-pxp: add support for i.MX7D
>   ARM: dts: imx7d: add node for PXP
> 
>  .../bindings/media/fsl,imx6ull-pxp.yaml       |  62 ++++++++
>  .../devicetree/bindings/media/fsl-pxp.txt     |  26 ---
>  arch/arm/boot/dts/imx7d.dtsi                  |   9 ++
>  drivers/media/platform/nxp/imx-pxp.c          | 148 +++++++++++++++---
>  4 files changed, 197 insertions(+), 48 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
>  delete mode 100644 Documentation/devicetree/bindings/media/fsl-pxp.txt

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/8] media: imx-pxp: add support for i.MX7D

[Michael Tretter]

On Fri, 06 Jan 2023 14:41:32 +0200, Laurent Pinchart wrote:
> Hi Michael,
> 
> On Thu, Jan 05, 2023 at 02:47:21PM +0100, Michael Tretter wrote:
> > This series adds support for the PXP found on the i.MX7D to the imx-pxp
> > driver.
> > 
> > The PXP on the i.MX7D has a few differences compared to the one on the
> > i.MX6ULL. Especially, it has more processing blocks and slightly different
> > multiplexers to route the data between the blocks. Therefore, the driver must
> > configure a different data path depending on the platform.
> > 
> > While the PXP has a version register, the reported version is the same on the
> > i.MX6ULL and the i.MX7D. Therefore, we cannot use the version register to
> > change the driver behavior, but have to use the device tree compatible. The
> > driver still prints the found version to the log to help bringing up the PXP
> > on further platforms.
> > 
> > The patches are inspired by some earlier patches [0] by Laurent to add PXP
> > support to the i.MX7d. Compared to the earlier patches, these patches add
> > different behavior depending on the platform. Furthermore, the patches disable
> > only the LUT block, but keep the rotator block enabled, as it may now be
> > configured via the V4L2 rotate control.
> 
> Sounds good to me.
> 
> > Patch 1 converts the dt-binding to yaml.
> > 
> > Patches 2 to 5 cleanup and refactor the driver in preparation of handling
> > different PXP versions.
> > 
> > Patches 6 and 7 add the handling of different platforms and the i.MX7d
> > specific configuration.
> > 
> > Patch 8 adds the device tree node for the PXP to the i.MX7d device tree.
> 
> I've reviewed the whole series and comments are mostly minor. As you're
> reminding me of the PXP, I'll take this as an opportunity to post
> patches that I've had in my tree for way too long :-) There will be
> minor conflicts with yours, so I'll first rebase them on this series,
> assuming that v2 will be similar in places where conflicts occur.

Thanks for the review! I will send a v2 to address your comments.

Thanks for your other patches as well. I will take a look and probably just
include them in my v2.

Michael

> 
> > Michael
> > 
> > [0] https://lore.kernel.org/linux-media/20200510223100.11641-1-laurent.pinchart@ideasonboard.com/
> > 
> > Michael Tretter (8):
> >   media: dt-bindings: media: fsl-pxp: convert to yaml
> >   media: imx-pxp: detect PXP version
> >   media: imx-pxp: extract helper function to setup data path
> >   media: imx-pxp: explicitly disable unused blocks
> >   media: imx-pxp: disable LUT block
> >   media: imx-pxp: make data_path_ctrl0 platform dependent
> >   media: imx-pxp: add support for i.MX7D
> >   ARM: dts: imx7d: add node for PXP
> > 
> >  .../bindings/media/fsl,imx6ull-pxp.yaml       |  62 ++++++++
> >  .../devicetree/bindings/media/fsl-pxp.txt     |  26 ---
> >  arch/arm/boot/dts/imx7d.dtsi                  |   9 ++
> >  drivers/media/platform/nxp/imx-pxp.c          | 148 +++++++++++++++---
> >  4 files changed, 197 insertions(+), 48 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/media/fsl,imx6ull-pxp.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/media/fsl-pxp.txt