[RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[Adam Ford]

Currently, the VPU in the i.MQ8MQ is appearing as one codec, but in
reality, it's two IP blocks called G1 and G2.  There is initialization
code in VPU code to pull some clocks, resets and other features which
has been integrated into the vpu-blk-ctrl for the i.MX8M Mini and a
similar method can be used to make the VPU codec's operate as 
stand-alone cores without having to know the details of each other
or the quirks unique to the i.MX8MQ, so the remaining code can be
left more generic.

This series was started by Lucas Stach with one by Benjamin Gaignard.
Most patches have been modified slightly by me.  It's in an RFC state
because I wasn't sure how to best handle the signatures and wasn't sure
if I could base it off the branch I did.

Since the g-streamer and media trees are in a constant state of
change, this series is based on

git://linuxtv.org/hverkuil/media_tree.git for-v5.17e

The downstream code from NXP shows the G1 and G2 clocks running
at 600MHz, but between the TRM and the datasheet, there is some
discrepancy.  Because the NXP reference code used 600MHz, that is
what was chosen here.  Users who need to adjust their G1 and G2
clocks can do so in their board files.

Fluster Results:

./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
Ran 90/135 tests successfully               in 61.966 secs

./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
Ran 55/61 tests successfully               in 7.660 secs


./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
Ran 144/303 tests successfully               in 162.665 secs

Changes log:

V2:  Make vpu-blk-ctrl enable G2 clock when enabling fuses.
     Remove syscon from device tree and binding example
     Added modified nxp,imx8mq-vpu.yaml from Benjamin Gaignard 

Adam Ford (2):
  media: hantro: split i.MX8MQ G1 and G2 code
  arm64: dts: imx8mq: Split i.MX8MQ G1 and G2 with vpu-blk-ctrl

Benjamin Gaignard (1):
  dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support

Lucas Stach (3):
  dt-bindings: power: imx8mq: add defines for VPU blk-ctrl domains
  dt-bindings: soc: add binding for i.MX8MQ VPU blk-ctrl
  soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl

 .../bindings/media/nxp,imx8mq-vpu.yaml        |  58 +++++----
 .../soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml      |  71 +++++++++++
 arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  69 ++++++----
 drivers/soc/imx/imx8m-blk-ctrl.c              |  68 +++++++++-
 drivers/staging/media/hantro/hantro_drv.c     |   4 +-
 drivers/staging/media/hantro/hantro_hw.h      |   2 +-
 drivers/staging/media/hantro/imx8m_vpu_hw.c   | 119 +++---------------
 include/dt-bindings/power/imx8mq-power.h      |   3 +
 8 files changed, 237 insertions(+), 157 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml


base-commit: d1888b0bfd2ddef2e8a81505ffa200b92cc32e0c
-- 
2.32.0

[RFC V2 1/6] dt-bindings: power: imx8mq: add defines for VPU blk-ctrl domains

[Adam Ford]

From: Lucas Stach <l.stach@pengutronix.de>

This adds the defines for the power domains provided by the VPU
blk-ctrl on the i.MX8MQ.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

diff --git a/include/dt-bindings/power/imx8mq-power.h b/include/dt-bindings/power/imx8mq-power.h
index 8a513bd9166e..9f7d0f1e7c32 100644
--- a/include/dt-bindings/power/imx8mq-power.h
+++ b/include/dt-bindings/power/imx8mq-power.h
@@ -18,4 +18,7 @@
 #define IMX8M_POWER_DOMAIN_MIPI_CSI2	9
 #define IMX8M_POWER_DOMAIN_PCIE2	10
 
+#define IMX8MQ_VPUBLK_PD_G1		0
+#define IMX8MQ_VPUBLK_PD_G2		1
+
 #endif
-- 
2.32.0

[RFC V2 2/6] dt-bindings: soc: add binding for i.MX8MQ VPU blk-ctrl

[Adam Ford]

From: Lucas Stach <l.stach@pengutronix.de>

This adds the DT binding for the i.MX8MQ VPU blk-ctrl.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml
new file mode 100644
index 000000000000..7263ebedf09f
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX8MQ VPU blk-ctrl
+
+maintainers:
+  - Lucas Stach <l.stach@pengutronix.de>
+
+description:
+  The i.MX8MQ VPU blk-ctrl is a top-level peripheral providing access to
+  the NoC and ensuring proper power sequencing of the VPU peripherals
+  located in the VPU domain of the SoC.
+
+properties:
+  compatible:
+    items:
+      - const: fsl,imx8mq-vpu-blk-ctrl
+
+  reg:
+    maxItems: 1
+
+  '#power-domain-cells':
+    const: 1
+
+  power-domains:
+    minItems: 3
+    maxItems: 3
+
+  power-domain-names:
+    items:
+      - const: bus
+      - const: g1
+      - const: g2
+
+  clocks:
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: g1
+      - const: g2
+
+required:
+  - compatible
+  - reg
+  - power-domains
+  - power-domain-names
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mq-clock.h>
+    #include <dt-bindings/power/imx8mq-power.h>
+
+    vpu_blk_ctrl: blk-ctrl@38320000 {
+      compatible = "fsl,imx8mq-vpu-blk-ctrl";
+      reg = <0x38320000 0x100>;
+      power-domains = <&pgc_vpu>, <&pgc_vpu>, <&pgc_vpu>;
+      power-domain-names = "bus", "g1", "g2";
+      clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
+               <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
+      clock-names = "g1", "g2";
+      #power-domain-cells = <1>;
+    };
-- 
2.32.0

[RFC V2 3/6] dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support

[Adam Ford]

From: Benjamin Gaignard <benjamin.gaignard@collabora.com>

Introducing the G2 hevc video decoder requires modifications of the bindings to allow
one node per VPU.

VPUs share one hardware control block which is provided as a phandle on
a syscon.
Each node has now one reg and one interrupt.
Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.

To be compatible with older DT the driver is still capable to use the 'ctrl'
reg-name even if it is deprecated now.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
index 762be3f96ce9..eaeba4ce262a 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
@@ -15,37 +15,36 @@ description:
 
 properties:
   compatible:
-    const: nxp,imx8mq-vpu
+    oneOf:
+      - const: nxp,imx8mq-vpu-g1
+      - const: nxp,imx8mq-vpu-g2
 
   reg:
-    maxItems: 3
-
-  reg-names:
-    items:
-      - const: g1
-      - const: g2
-      - const: ctrl
+    maxItems: 1
 
   interrupts:
-    maxItems: 2
+    maxItems: 1
 
   interrupt-names:
-    items:
+    oneOf:
       - const: g1
       - const: g2
 
   clocks:
-    maxItems: 3
+    maxItems: 1
 
   clock-names:
-    items:
+    oneOf:
       - const: g1
       - const: g2
-      - const: bus
 
   power-domains:
     maxItems: 1
 
+  nxp,imx8m-vpu-ctrl:
+    description: Specifies a phandle to syscon VPU hardware control block
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+
 required:
   - compatible
   - reg
@@ -60,20 +59,27 @@ additionalProperties: false
 examples:
   - |
         #include <dt-bindings/clock/imx8mq-clock.h>
+        #include <dt-bindings/power/imx8mq-power.h>
         #include <dt-bindings/interrupt-controller/arm-gic.h>
 
-        vpu: video-codec@38300000 {
+        vpu_g1: video-codec@38300000 {
                 compatible = "nxp,imx8mq-vpu";
-                reg = <0x38300000 0x10000>,
-                      <0x38310000 0x10000>,
-                      <0x38320000 0x10000>;
-                reg-names = "g1", "g2", "ctrl";
-                interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
-                             <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
-                interrupt-names = "g1", "g2";
-                clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
-                         <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
-                         <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
-                clock-names = "g1", "g2", "bus";
-                power-domains = <&pgc_vpu>;
+                reg = <0x38300000 0x10000>;
+                reg-names "g1";
+                interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+                interrupt-names = "g1";
+                clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
+                clock-names = "g1";
+                power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>;
+        };
+
+        vpu_g2: video-codec@38310000 {
+                compatible = "nxp,imx8mq-vpu-g2";
+                reg = <0x38300000 0x10000>;
+                reg-names "g2";
+                interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+                interrupt-names = "g2";
+                clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
+                clock-names = "g2";
+                power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G2>;
         };
-- 
2.32.0

Re: [RFC V2 3/6] dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support

[Rob Herring]

On Mon, 06 Dec 2021 19:54:42 -0600, Adam Ford wrote:
> From: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> 
> Introducing the G2 hevc video decoder requires modifications of the bindings to allow
> one node per VPU.
> 
> VPUs share one hardware control block which is provided as a phandle on
> a syscon.
> Each node has now one reg and one interrupt.
> Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.
> 
> To be compatible with older DT the driver is still capable to use the 'ctrl'
> reg-name even if it is deprecated now.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml: properties:clock-names:oneOf: [{'const': 'g1'}, {'const': 'g2'}] should not be valid under {'items': {'propertyNames': {'const': 'const'}, 'required': ['const']}}
	hint: Use 'enum' rather than 'oneOf' + 'const' entries
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml: properties:interrupt-names:oneOf: [{'const': 'g1'}, {'const': 'g2'}] should not be valid under {'items': {'propertyNames': {'const': 'const'}, 'required': ['const']}}
	hint: Use 'enum' rather than 'oneOf' + 'const' entries
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml: properties:compatible:oneOf: [{'const': 'nxp,imx8mq-vpu-g1'}, {'const': 'nxp,imx8mq-vpu-g2'}] should not be valid under {'items': {'propertyNames': {'const': 'const'}, 'required': ['const']}}
	hint: Use 'enum' rather than 'oneOf' + 'const' entries
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml: ignoring, error in schema: properties: interrupt-names: oneOf
warning: no schema found in file: ./Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
Error: Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.example.dts:26.27-31 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:373: Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1413: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1564457

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [RFC V2 3/6] dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support

[Rob Herring]

On Mon, Dec 06, 2021 at 07:54:42PM -0600, Adam Ford wrote:
> From: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> 
> Introducing the G2 hevc video decoder requires modifications of the bindings to allow
> one node per VPU.

Why? It looks like the G2 part was already described. If you are 
changing this because you want 2 drivers for G1 and G2, then NAK. DT 
nodes and drivers don't have to be 1:1. This change is breaking 
compatibility.

> 
> VPUs share one hardware control block which is provided as a phandle on
> a syscon.

That's not really ideal. Is this really a separate block?

> Each node has now one reg and one interrupt.
> Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.
> 
> To be compatible with older DT the driver is still capable to use the 'ctrl'
> reg-name even if it is deprecated now.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> index 762be3f96ce9..eaeba4ce262a 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> @@ -15,37 +15,36 @@ description:
>  
>  properties:
>    compatible:
> -    const: nxp,imx8mq-vpu
> +    oneOf:
> +      - const: nxp,imx8mq-vpu-g1
> +      - const: nxp,imx8mq-vpu-g2
>  
>    reg:
> -    maxItems: 3
> -
> -  reg-names:
> -    items:
> -      - const: g1
> -      - const: g2
> -      - const: ctrl
> +    maxItems: 1
>  
>    interrupts:
> -    maxItems: 2
> +    maxItems: 1
>  
>    interrupt-names:
> -    items:
> +    oneOf:
>        - const: g1
>        - const: g2
>  
>    clocks:
> -    maxItems: 3
> +    maxItems: 1
>  
>    clock-names:
> -    items:
> +    oneOf:
>        - const: g1
>        - const: g2
> -      - const: bus
>  
>    power-domains:
>      maxItems: 1
>  
> +  nxp,imx8m-vpu-ctrl:
> +    description: Specifies a phandle to syscon VPU hardware control block
> +    $ref: "/schemas/types.yaml#/definitions/phandle"

This is optional?

> +
>  required:
>    - compatible
>    - reg
> @@ -60,20 +59,27 @@ additionalProperties: false
>  examples:
>    - |
>          #include <dt-bindings/clock/imx8mq-clock.h>
> +        #include <dt-bindings/power/imx8mq-power.h>
>          #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
> -        vpu: video-codec@38300000 {
> +        vpu_g1: video-codec@38300000 {
>                  compatible = "nxp,imx8mq-vpu";
> -                reg = <0x38300000 0x10000>,
> -                      <0x38310000 0x10000>,
> -                      <0x38320000 0x10000>;
> -                reg-names = "g1", "g2", "ctrl";
> -                interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> -                             <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> -                interrupt-names = "g1", "g2";
> -                clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> -                         <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
> -                         <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> -                clock-names = "g1", "g2", "bus";
> -                power-domains = <&pgc_vpu>;
> +                reg = <0x38300000 0x10000>;
> +                reg-names "g1";
> +                interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +                interrupt-names = "g1";
> +                clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
> +                clock-names = "g1";
> +                power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>;
> +        };
> +
> +        vpu_g2: video-codec@38310000 {
> +                compatible = "nxp,imx8mq-vpu-g2";
> +                reg = <0x38300000 0x10000>;
> +                reg-names "g2";
> +                interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +                interrupt-names = "g2";
> +                clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
> +                clock-names = "g2";
> +                power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G2>;
>          };
> -- 
> 2.32.0
> 
> 

Re: [RFC V2 3/6] dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support

[Lucas Stach]

Hi Rob,

Am Dienstag, dem 07.12.2021 um 09:14 -0600 schrieb Rob Herring:
> On Mon, Dec 06, 2021 at 07:54:42PM -0600, Adam Ford wrote:
> > From: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > 
> > Introducing the G2 hevc video decoder requires modifications of the bindings to allow
> > one node per VPU.
> 
> Why? It looks like the G2 part was already described. If you are 
> changing this because you want 2 drivers for G1 and G2, then NAK. DT 
> nodes and drivers don't have to be 1:1. This change is breaking 
> compatibility.

We can keep the compatibility by just keeping the code in the VPU
driver to handle the G1 block as-is. The VPU block on the imx8mq is
really three peripherals: the control block working together with the
power domain controller to provide clocks and resets and the G1 and G2
VPU cores.

> 
> > 
> > VPUs share one hardware control block which is provided as a phandle on
> > a syscon.
> 
> That's not really ideal. Is this really a separate block?
> 
This part of the commit message is not accurate anymore. The control
block is in fact so separate from the VPU that we even added a new
driver to handle those control blocks: the imx8m blk-ctrl driver. The
VPU driver doesn't need handle this control block anymore, it's now
handled via the power-domain abstraction.

Regards,
Lucas

> > Each node has now one reg and one interrupt.
> > Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.
> > 
> > To be compatible with older DT the driver is still capable to use the 'ctrl'
> > reg-name even if it is deprecated now.
> > 
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > 
> > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > index 762be3f96ce9..eaeba4ce262a 100644
> > --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > @@ -15,37 +15,36 @@ description:
> >  
> >  properties:
> >    compatible:
> > -    const: nxp,imx8mq-vpu
> > +    oneOf:
> > +      - const: nxp,imx8mq-vpu-g1
> > +      - const: nxp,imx8mq-vpu-g2
> >  
> >    reg:
> > -    maxItems: 3
> > -
> > -  reg-names:
> > -    items:
> > -      - const: g1
> > -      - const: g2
> > -      - const: ctrl
> > +    maxItems: 1
> >  
> >    interrupts:
> > -    maxItems: 2
> > +    maxItems: 1
> >  
> >    interrupt-names:
> > -    items:
> > +    oneOf:
> >        - const: g1
> >        - const: g2
> >  
> >    clocks:
> > -    maxItems: 3
> > +    maxItems: 1
> >  
> >    clock-names:
> > -    items:
> > +    oneOf:
> >        - const: g1
> >        - const: g2
> > -      - const: bus
> >  
> >    power-domains:
> >      maxItems: 1
> >  
> > +  nxp,imx8m-vpu-ctrl:
> > +    description: Specifies a phandle to syscon VPU hardware control block
> > +    $ref: "/schemas/types.yaml#/definitions/phandle"
> 
> This is optional?
> 
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -60,20 +59,27 @@ additionalProperties: false
> >  examples:
> >    - |
> >          #include <dt-bindings/clock/imx8mq-clock.h>
> > +        #include <dt-bindings/power/imx8mq-power.h>
> >          #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  
> > -        vpu: video-codec@38300000 {
> > +        vpu_g1: video-codec@38300000 {
> >                  compatible = "nxp,imx8mq-vpu";
> > -                reg = <0x38300000 0x10000>,
> > -                      <0x38310000 0x10000>,
> > -                      <0x38320000 0x10000>;
> > -                reg-names = "g1", "g2", "ctrl";
> > -                interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > -                             <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> > -                interrupt-names = "g1", "g2";
> > -                clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> > -                         <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
> > -                         <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> > -                clock-names = "g1", "g2", "bus";
> > -                power-domains = <&pgc_vpu>;
> > +                reg = <0x38300000 0x10000>;
> > +                reg-names "g1";
> > +                interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > +                interrupt-names = "g1";
> > +                clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
> > +                clock-names = "g1";
> > +                power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>;
> > +        };
> > +
> > +        vpu_g2: video-codec@38310000 {
> > +                compatible = "nxp,imx8mq-vpu-g2";
> > +                reg = <0x38300000 0x10000>;
> > +                reg-names "g2";
> > +                interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> > +                interrupt-names = "g2";
> > +                clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
> > +                clock-names = "g2";
> > +                power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G2>;
> >          };
> > -- 
> > 2.32.0
> > 
> > 

[RFC V2 4/6] soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl

[Adam Ford]

From: Lucas Stach <l.stach@pengutronix.de>

This adds the necessary bits to drive the VPU blk-ctrl on the i.MX8MQ, to
avoid putting more of this functionality into the decoder driver.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
index 519b3651d1d9..a7c12e8fa89b 100644
--- a/drivers/soc/imx/imx8m-blk-ctrl.c
+++ b/drivers/soc/imx/imx8m-blk-ctrl.c
@@ -14,6 +14,7 @@
 #include <linux/clk.h>
 
 #include <dt-bindings/power/imx8mm-power.h>
+#include <dt-bindings/power/imx8mq-power.h>
 
 #define BLK_SFT_RSTN	0x0
 #define BLK_CLK_EN	0x4
@@ -498,6 +499,68 @@ static const struct imx8m_blk_ctrl_data imx8mm_disp_blk_ctl_dev_data = {
 	.num_domains = ARRAY_SIZE(imx8mm_disp_blk_ctl_domain_data),
 };
 
+static int imx8mq_vpu_power_notifier(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	struct imx8m_blk_ctrl *bc = container_of(nb, struct imx8m_blk_ctrl,
+						 power_nb);
+
+	if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF)
+		return NOTIFY_OK;
+
+	/*
+	 * The ADB in the VPUMIX domain has no separate reset and clock
+	 * enable bits, but is ungated and reset together with the VPUs. The
+	 * reset and clock enable inputs to the ADB is a logical OR of the
+	 * VPU bits. In order to set the G2 fuse bits, the G2 clock must
+	 * also be enabled.
+	 */
+	regmap_set_bits(bc->regmap, BLK_SFT_RSTN, BIT(0) | BIT(1));
+	regmap_set_bits(bc->regmap, BLK_CLK_EN, BIT(0) | BIT(1));
+
+	if (action == GENPD_NOTIFY_ON) {
+		/*
+		 * On power up we have no software backchannel to the GPC to
+		 * wait for the ADB handshake to happen, so we just delay for a
+		 * bit. On power down the GPC driver waits for the handshake.
+		 */
+		udelay(5);
+
+		/* set "fuse" bits to enable the VPUs */
+		regmap_set_bits(bc->regmap, 0x8, 0xffffffff);
+		regmap_set_bits(bc->regmap, 0xc, 0xffffffff);
+		regmap_set_bits(bc->regmap, 0x10, 0xffffffff);
+	}
+
+	return NOTIFY_OK;
+}
+
+static const struct imx8m_blk_ctrl_domain_data imx8mq_vpu_blk_ctl_domain_data[] = {
+	[IMX8MQ_VPUBLK_PD_G1] = {
+		.name = "vpublk-g1",
+		.clk_names = (const char *[]){ "g1", },
+		.num_clks = 1,
+		.gpc_name = "g1",
+		.rst_mask = BIT(1),
+		.clk_mask = BIT(1),
+	},
+	[IMX8MQ_VPUBLK_PD_G2] = {
+		.name = "vpublk-g2",
+		.clk_names = (const char *[]){ "g2", },
+		.num_clks = 1,
+		.gpc_name = "g2",
+		.rst_mask = BIT(0),
+		.clk_mask = BIT(0),
+	},
+};
+
+static const struct imx8m_blk_ctrl_data imx8mq_vpu_blk_ctl_dev_data = {
+	.max_reg = 0x14,
+	.power_notifier_fn = imx8mq_vpu_power_notifier,
+	.domains = imx8mq_vpu_blk_ctl_domain_data,
+	.num_domains = ARRAY_SIZE(imx8mq_vpu_blk_ctl_domain_data),
+};
+
 static const struct of_device_id imx8m_blk_ctrl_of_match[] = {
 	{
 		.compatible = "fsl,imx8mm-vpu-blk-ctrl",
@@ -505,7 +568,10 @@ static const struct of_device_id imx8m_blk_ctrl_of_match[] = {
 	}, {
 		.compatible = "fsl,imx8mm-disp-blk-ctrl",
 		.data = &imx8mm_disp_blk_ctl_dev_data
-	} ,{
+	}, {
+		.compatible = "fsl,imx8mq-vpu-blk-ctrl",
+		.data = &imx8mq_vpu_blk_ctl_dev_data
+	}, {
 		/* Sentinel */
 	}
 };
-- 
2.32.0

[RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

[Adam Ford]

The VPU in the i.MX8MQ is really the combination of Hantro G1 and
Hantro G2. With the updated vpu-blk-ctrl, the power domains system
can enable and disable them separately as well as pull them out of
reset. This simplifies the code and lets them run independently.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index ab2467998d29..d803252a5aba 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = {
 	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_IMX8M
-	{ .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
-	{ .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
+	{ .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, },
+	{ .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, },
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
 	{ .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index cff817ca8d22..122b83a16663 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -299,8 +299,8 @@ enum hantro_enc_fmt {
 	ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
 };
 
+extern const struct hantro_variant imx8mq_vpu_g1_variant;
 extern const struct hantro_variant imx8mq_vpu_g2_variant;
-extern const struct hantro_variant imx8mq_vpu_variant;
 extern const struct hantro_variant px30_vpu_variant;
 extern const struct hantro_variant rk3036_vpu_variant;
 extern const struct hantro_variant rk3066_vpu_variant;
diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
index 1a43f6fceef9..c9f6e8472258 100644
--- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
+++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
@@ -13,67 +13,6 @@
 #include "hantro_g1_regs.h"
 #include "hantro_g2_regs.h"
 
-#define CTRL_SOFT_RESET		0x00
-#define RESET_G1		BIT(1)
-#define RESET_G2		BIT(0)
-
-#define CTRL_CLOCK_ENABLE	0x04
-#define CLOCK_G1		BIT(1)
-#define CLOCK_G2		BIT(0)
-
-#define CTRL_G1_DEC_FUSE	0x08
-#define CTRL_G1_PP_FUSE		0x0c
-#define CTRL_G2_DEC_FUSE	0x10
-
-static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
-{
-	u32 val;
-
-	/* Assert */
-	val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
-	val &= ~reset_bits;
-	writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
-
-	udelay(2);
-
-	/* Release */
-	val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
-	val |= reset_bits;
-	writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
-}
-
-static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
-{
-	u32 val;
-
-	val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
-	val |= clock_bits;
-	writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
-}
-
-static int imx8mq_runtime_resume(struct hantro_dev *vpu)
-{
-	int ret;
-
-	ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks);
-	if (ret) {
-		dev_err(vpu->dev, "Failed to enable clocks\n");
-		return ret;
-	}
-
-	imx8m_soft_reset(vpu, RESET_G1 | RESET_G2);
-	imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
-
-	/* Set values of the fuse registers */
-	writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
-	writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
-	writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
-
-	clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
-
-	return 0;
-}
-
 /*
  * Supported formats.
  */
@@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
-{
-	vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
-
-	return 0;
-}
-
-static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
-{
-	struct hantro_dev *vpu = ctx->dev;
-
-	imx8m_soft_reset(vpu, RESET_G1);
-}
-
-static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
-{
-	struct hantro_dev *vpu = ctx->dev;
-
-	imx8m_soft_reset(vpu, RESET_G2);
-}
-
 /*
  * Supported codec ops.
  */
@@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
 static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
 	[HANTRO_MODE_MPEG2_DEC] = {
 		.run = hantro_g1_mpeg2_dec_run,
-		.reset = imx8m_vpu_g1_reset,
 		.init = hantro_mpeg2_dec_init,
 		.exit = hantro_mpeg2_dec_exit,
 	},
 	[HANTRO_MODE_VP8_DEC] = {
 		.run = hantro_g1_vp8_dec_run,
-		.reset = imx8m_vpu_g1_reset,
 		.init = hantro_vp8_dec_init,
 		.exit = hantro_vp8_dec_exit,
 	},
 	[HANTRO_MODE_H264_DEC] = {
 		.run = hantro_g1_h264_dec_run,
-		.reset = imx8m_vpu_g1_reset,
 		.init = hantro_h264_dec_init,
 		.exit = hantro_h264_dec_exit,
 	},
@@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
 static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
 	[HANTRO_MODE_HEVC_DEC] = {
 		.run = hantro_g2_hevc_dec_run,
-		.reset = imx8m_vpu_g2_reset,
 		.init = hantro_hevc_dec_init,
 		.exit = hantro_hevc_dec_exit,
 	},
 	[HANTRO_MODE_VP9_DEC] = {
 		.run = hantro_g2_vp9_dec_run,
 		.done = hantro_g2_vp9_dec_done,
-		.reset = imx8m_vpu_g2_reset,
 		.init = hantro_vp9_dec_init,
 		.exit = hantro_vp9_dec_exit,
 	},
@@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
  * VPU variants.
  */
 
-static const struct hantro_irq imx8mq_irqs[] = {
+static const struct hantro_irq imx8mq_g1_irqs[] = {
 	{ "g1", imx8m_vpu_g1_irq },
 };
 
@@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
 	{ "g2", imx8m_vpu_g2_irq },
 };
 
-static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
-static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
+static const char * const imx8mq_g1_clk_names[] = { "g1" };
+static const char * const imx8mq_g1_reg_names[] = { "g1" };
+static const char * const imx8mq_g2_clk_names[] = { "g2" };
+static const char * const imx8mq_g2_reg_names[] = { "g2" };
 
-const struct hantro_variant imx8mq_vpu_variant = {
+const struct hantro_variant imx8mq_vpu_g1_variant = {
 	.dec_fmts = imx8m_vpu_dec_fmts,
 	.num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
 	.postproc_fmts = imx8m_vpu_postproc_fmts,
@@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = {
 	.codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
 		 HANTRO_H264_DECODER,
 	.codec_ops = imx8mq_vpu_codec_ops,
-	.init = imx8mq_vpu_hw_init,
-	.runtime_resume = imx8mq_runtime_resume,
-	.irqs = imx8mq_irqs,
-	.num_irqs = ARRAY_SIZE(imx8mq_irqs),
-	.clk_names = imx8mq_clk_names,
-	.num_clocks = ARRAY_SIZE(imx8mq_clk_names),
-	.reg_names = imx8mq_reg_names,
-	.num_regs = ARRAY_SIZE(imx8mq_reg_names)
+	.irqs = imx8mq_g1_irqs,
+	.num_irqs = ARRAY_SIZE(imx8mq_g1_irqs),
+	.clk_names = imx8mq_g1_clk_names,
+	.num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
+	.reg_names = imx8mq_g1_reg_names,
+	.num_regs = ARRAY_SIZE(imx8mq_g1_reg_names),
 };
 
 const struct hantro_variant imx8mq_vpu_g2_variant = {
@@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
 	.postproc_ops = &hantro_g2_postproc_ops,
 	.codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
 	.codec_ops = imx8mq_vpu_g2_codec_ops,
-	.init = imx8mq_vpu_hw_init,
-	.runtime_resume = imx8mq_runtime_resume,
 	.irqs = imx8mq_g2_irqs,
 	.num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
-	.clk_names = imx8mq_clk_names,
-	.num_clocks = ARRAY_SIZE(imx8mq_clk_names),
+	.clk_names = imx8mq_g2_clk_names,
+	.num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
+	.reg_names = imx8mq_g2_reg_names,
+	.num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
 };
-- 
2.32.0

Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

[Ezequiel Garcia]

Hi Adam,

Thanks for the good work! This is looking quite promising.

On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote:
> The VPU in the i.MX8MQ is really the combination of Hantro G1 and
> Hantro G2. With the updated vpu-blk-ctrl, the power domains system
> can enable and disable them separately as well as pull them out of
> reset. This simplifies the code and lets them run independently.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index ab2467998d29..d803252a5aba 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = {
>  	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
>  #endif
>  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> -	{ .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
> -	{ .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
> +	{ .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, },

I think it's important to clarify that you are breaking support
for the previous device-tree binding. Not only because of the compatible
string change, but because the binding is now quite different.

Note that in the past Benjamin tried to avoid this.
IIRC, his proposal was backwards compatible.

If this is unavoidable, due to how the blk-ctrl is handled, then that's
fine. Given it's a staging driver, we can still play these games.

Having said that, let's please make this very clear in the commit
description, to it's clear for developers forward-porting their kernels.
This applies not only to this commit, but to all commits that affect
the binding.

Thanks!
Ezequiel

> +	{ .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, },
>  #endif
>  #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
>  	{ .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index cff817ca8d22..122b83a16663 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -299,8 +299,8 @@ enum hantro_enc_fmt {
>  	ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
>  };
>  
> +extern const struct hantro_variant imx8mq_vpu_g1_variant;
>  extern const struct hantro_variant imx8mq_vpu_g2_variant;
> -extern const struct hantro_variant imx8mq_vpu_variant;
>  extern const struct hantro_variant px30_vpu_variant;
>  extern const struct hantro_variant rk3036_vpu_variant;
>  extern const struct hantro_variant rk3066_vpu_variant;
> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> index 1a43f6fceef9..c9f6e8472258 100644
> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> @@ -13,67 +13,6 @@
>  #include "hantro_g1_regs.h"
>  #include "hantro_g2_regs.h"
>  
> -#define CTRL_SOFT_RESET		0x00
> -#define RESET_G1		BIT(1)
> -#define RESET_G2		BIT(0)
> -
> -#define CTRL_CLOCK_ENABLE	0x04
> -#define CLOCK_G1		BIT(1)
> -#define CLOCK_G2		BIT(0)
> -
> -#define CTRL_G1_DEC_FUSE	0x08
> -#define CTRL_G1_PP_FUSE		0x0c
> -#define CTRL_G2_DEC_FUSE	0x10
> -
> -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> -{
> -	u32 val;
> -
> -	/* Assert */
> -	val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> -	val &= ~reset_bits;
> -	writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> -
> -	udelay(2);
> -
> -	/* Release */
> -	val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> -	val |= reset_bits;
> -	writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> -}
> -
> -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> -{
> -	u32 val;
> -
> -	val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> -	val |= clock_bits;
> -	writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> -}
> -
> -static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> -{
> -	int ret;
> -
> -	ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks);
> -	if (ret) {
> -		dev_err(vpu->dev, "Failed to enable clocks\n");
> -		return ret;
> -	}
> -
> -	imx8m_soft_reset(vpu, RESET_G1 | RESET_G2);
> -	imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
> -
> -	/* Set values of the fuse registers */
> -	writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> -	writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> -	writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> -
> -	clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
> -
> -	return 0;
> -}
> -
>  /*
>   * Supported formats.
>   */
> @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
> -{
> -	vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
> -
> -	return 0;
> -}
> -
> -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
> -{
> -	struct hantro_dev *vpu = ctx->dev;
> -
> -	imx8m_soft_reset(vpu, RESET_G1);
> -}
> -
> -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> -{
> -	struct hantro_dev *vpu = ctx->dev;
> -
> -	imx8m_soft_reset(vpu, RESET_G2);
> -}
> -
>  /*
>   * Supported codec ops.
>   */
> @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
>  static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
>  	[HANTRO_MODE_MPEG2_DEC] = {
>  		.run = hantro_g1_mpeg2_dec_run,
> -		.reset = imx8m_vpu_g1_reset,
>  		.init = hantro_mpeg2_dec_init,
>  		.exit = hantro_mpeg2_dec_exit,
>  	},
>  	[HANTRO_MODE_VP8_DEC] = {
>  		.run = hantro_g1_vp8_dec_run,
> -		.reset = imx8m_vpu_g1_reset,
>  		.init = hantro_vp8_dec_init,
>  		.exit = hantro_vp8_dec_exit,
>  	},
>  	[HANTRO_MODE_H264_DEC] = {
>  		.run = hantro_g1_h264_dec_run,
> -		.reset = imx8m_vpu_g1_reset,
>  		.init = hantro_h264_dec_init,
>  		.exit = hantro_h264_dec_exit,
>  	},
> @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
>  static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
>  	[HANTRO_MODE_HEVC_DEC] = {
>  		.run = hantro_g2_hevc_dec_run,
> -		.reset = imx8m_vpu_g2_reset,
>  		.init = hantro_hevc_dec_init,
>  		.exit = hantro_hevc_dec_exit,
>  	},
>  	[HANTRO_MODE_VP9_DEC] = {
>  		.run = hantro_g2_vp9_dec_run,
>  		.done = hantro_g2_vp9_dec_done,
> -		.reset = imx8m_vpu_g2_reset,
>  		.init = hantro_vp9_dec_init,
>  		.exit = hantro_vp9_dec_exit,
>  	},
> @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
>   * VPU variants.
>   */
>  
> -static const struct hantro_irq imx8mq_irqs[] = {
> +static const struct hantro_irq imx8mq_g1_irqs[] = {
>  	{ "g1", imx8m_vpu_g1_irq },
>  };
>  
> @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
>  	{ "g2", imx8m_vpu_g2_irq },
>  };
>  
> -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
> -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
> +static const char * const imx8mq_g1_clk_names[] = { "g1" };
> +static const char * const imx8mq_g1_reg_names[] = { "g1" };
> +static const char * const imx8mq_g2_clk_names[] = { "g2" };
> +static const char * const imx8mq_g2_reg_names[] = { "g2" };
>  
> -const struct hantro_variant imx8mq_vpu_variant = {
> +const struct hantro_variant imx8mq_vpu_g1_variant = {
>  	.dec_fmts = imx8m_vpu_dec_fmts,
>  	.num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
>  	.postproc_fmts = imx8m_vpu_postproc_fmts,
> @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = {
>  	.codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
>  		 HANTRO_H264_DECODER,
>  	.codec_ops = imx8mq_vpu_codec_ops,
> -	.init = imx8mq_vpu_hw_init,
> -	.runtime_resume = imx8mq_runtime_resume,
> -	.irqs = imx8mq_irqs,
> -	.num_irqs = ARRAY_SIZE(imx8mq_irqs),
> -	.clk_names = imx8mq_clk_names,
> -	.num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> -	.reg_names = imx8mq_reg_names,
> -	.num_regs = ARRAY_SIZE(imx8mq_reg_names)
> +	.irqs = imx8mq_g1_irqs,
> +	.num_irqs = ARRAY_SIZE(imx8mq_g1_irqs),
> +	.clk_names = imx8mq_g1_clk_names,
> +	.num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
> +	.reg_names = imx8mq_g1_reg_names,
> +	.num_regs = ARRAY_SIZE(imx8mq_g1_reg_names),
>  };
>  
>  const struct hantro_variant imx8mq_vpu_g2_variant = {
> @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
>  	.postproc_ops = &hantro_g2_postproc_ops,
>  	.codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
>  	.codec_ops = imx8mq_vpu_g2_codec_ops,
> -	.init = imx8mq_vpu_hw_init,
> -	.runtime_resume = imx8mq_runtime_resume,
>  	.irqs = imx8mq_g2_irqs,
>  	.num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
> -	.clk_names = imx8mq_clk_names,
> -	.num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> +	.clk_names = imx8mq_g2_clk_names,
> +	.num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
> +	.reg_names = imx8mq_g2_reg_names,
> +	.num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
>  };
> -- 
> 2.32.0
> 

Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

[Adam Ford]

On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> Hi Adam,
>
> Thanks for the good work! This is looking quite promising.
>
> On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote:
> > The VPU in the i.MX8MQ is really the combination of Hantro G1 and
> > Hantro G2. With the updated vpu-blk-ctrl, the power domains system
> > can enable and disable them separately as well as pull them out of
> > reset. This simplifies the code and lets them run independently.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index ab2467998d29..d803252a5aba 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = {
> >       { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
> >  #endif
> >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > -     { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
> > -     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
> > +     { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, },
>
> I think it's important to clarify that you are breaking support
> for the previous device-tree binding. Not only because of the compatible
> string change, but because the binding is now quite different.
>
> Note that in the past Benjamin tried to avoid this.
> IIRC, his proposal was backwards compatible.

I was looking at how to do this, but the stand-alone vpu bindings did
the reset and the fuses manually, it causes issues.  When the block
control is enabled those memory locations for the resets and fuses are
assigned to the blk-ctrl driver, so attempting to access them from a
different driver was a violation.

When I started looking at this, the stand-alone VPU was trying to be
both G1 and G2, but when I was testing it before I made changes, I
didn't see the G2 function at all. When I was done the G2 seemed to
work, so it seems like this is an improvement.  If you want me to keep
the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1
back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd
have to add the blk-ctrl reference, so it seemed to me like a better
alternative to deprecate the old binding/driver and replace it with
the new one because of the significant changes.  Since I'd like to
rebase the i.MX8M Mini I did on this work, it seemed weird to have
nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and
nxp,imx8mm-vpu-g2 where the only one without a Gx name was the
original 8MQ binding but limited to G1 functionality and the G2
stripped out.
>
> If this is unavoidable, due to how the blk-ctrl is handled, then that's
> fine. Given it's a staging driver, we can still play these games.
>
> Having said that, let's please make this very clear in the commit
> description, to it's clear for developers forward-porting their kernels.
> This applies not only to this commit, but to all commits that affect
> the binding.

When I post the formal patch, I will add a note that this explicitly
deprecates the old bindings due to the fact that the G1 and G2 are
separate IP blocks in the reference manual, and blk-ctrl is a
completely separate driver altogether.

>
> Thanks!
> Ezequiel
>
> > +     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, },
> >  #endif
> >  #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
> >       { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > index cff817ca8d22..122b83a16663 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -299,8 +299,8 @@ enum hantro_enc_fmt {
> >       ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
> >  };
> >
> > +extern const struct hantro_variant imx8mq_vpu_g1_variant;
> >  extern const struct hantro_variant imx8mq_vpu_g2_variant;
> > -extern const struct hantro_variant imx8mq_vpu_variant;
> >  extern const struct hantro_variant px30_vpu_variant;
> >  extern const struct hantro_variant rk3036_vpu_variant;
> >  extern const struct hantro_variant rk3066_vpu_variant;
> > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > index 1a43f6fceef9..c9f6e8472258 100644
> > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > @@ -13,67 +13,6 @@
> >  #include "hantro_g1_regs.h"
> >  #include "hantro_g2_regs.h"
> >
> > -#define CTRL_SOFT_RESET              0x00
> > -#define RESET_G1             BIT(1)
> > -#define RESET_G2             BIT(0)
> > -
> > -#define CTRL_CLOCK_ENABLE    0x04
> > -#define CLOCK_G1             BIT(1)
> > -#define CLOCK_G2             BIT(0)
> > -
> > -#define CTRL_G1_DEC_FUSE     0x08
> > -#define CTRL_G1_PP_FUSE              0x0c
> > -#define CTRL_G2_DEC_FUSE     0x10
> > -
> > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> > -{
> > -     u32 val;
> > -
> > -     /* Assert */
> > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > -     val &= ~reset_bits;
> > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > -
> > -     udelay(2);
> > -
> > -     /* Release */
> > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > -     val |= reset_bits;
> > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > -}
> > -
> > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> > -{
> > -     u32 val;
> > -
> > -     val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > -     val |= clock_bits;
> > -     writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > -}
> > -
> > -static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> > -{
> > -     int ret;
> > -
> > -     ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks);
> > -     if (ret) {
> > -             dev_err(vpu->dev, "Failed to enable clocks\n");
> > -             return ret;
> > -     }
> > -
> > -     imx8m_soft_reset(vpu, RESET_G1 | RESET_G2);
> > -     imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
> > -
> > -     /* Set values of the fuse registers */
> > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> > -
> > -     clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > -
> > -     return 0;
> > -}
> > -
> >  /*
> >   * Supported formats.
> >   */
> > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
> >       return IRQ_HANDLED;
> >  }
> >
> > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
> > -{
> > -     vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
> > -
> > -     return 0;
> > -}
> > -
> > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
> > -{
> > -     struct hantro_dev *vpu = ctx->dev;
> > -
> > -     imx8m_soft_reset(vpu, RESET_G1);
> > -}
> > -
> > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > -{
> > -     struct hantro_dev *vpu = ctx->dev;
> > -
> > -     imx8m_soft_reset(vpu, RESET_G2);
> > -}
> > -
> >  /*
> >   * Supported codec ops.
> >   */
> > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> >  static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> >       [HANTRO_MODE_MPEG2_DEC] = {
> >               .run = hantro_g1_mpeg2_dec_run,
> > -             .reset = imx8m_vpu_g1_reset,
> >               .init = hantro_mpeg2_dec_init,
> >               .exit = hantro_mpeg2_dec_exit,
> >       },
> >       [HANTRO_MODE_VP8_DEC] = {
> >               .run = hantro_g1_vp8_dec_run,
> > -             .reset = imx8m_vpu_g1_reset,
> >               .init = hantro_vp8_dec_init,
> >               .exit = hantro_vp8_dec_exit,
> >       },
> >       [HANTRO_MODE_H264_DEC] = {
> >               .run = hantro_g1_h264_dec_run,
> > -             .reset = imx8m_vpu_g1_reset,
> >               .init = hantro_h264_dec_init,
> >               .exit = hantro_h264_dec_exit,
> >       },
> > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> >  static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> >       [HANTRO_MODE_HEVC_DEC] = {
> >               .run = hantro_g2_hevc_dec_run,
> > -             .reset = imx8m_vpu_g2_reset,
> >               .init = hantro_hevc_dec_init,
> >               .exit = hantro_hevc_dec_exit,
> >       },
> >       [HANTRO_MODE_VP9_DEC] = {
> >               .run = hantro_g2_vp9_dec_run,
> >               .done = hantro_g2_vp9_dec_done,
> > -             .reset = imx8m_vpu_g2_reset,
> >               .init = hantro_vp9_dec_init,
> >               .exit = hantro_vp9_dec_exit,
> >       },
> > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> >   * VPU variants.
> >   */
> >
> > -static const struct hantro_irq imx8mq_irqs[] = {
> > +static const struct hantro_irq imx8mq_g1_irqs[] = {
> >       { "g1", imx8m_vpu_g1_irq },
> >  };
> >
> > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
> >       { "g2", imx8m_vpu_g2_irq },
> >  };
> >
> > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
> > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
> > +static const char * const imx8mq_g1_clk_names[] = { "g1" };
> > +static const char * const imx8mq_g1_reg_names[] = { "g1" };
> > +static const char * const imx8mq_g2_clk_names[] = { "g2" };
> > +static const char * const imx8mq_g2_reg_names[] = { "g2" };
> >
> > -const struct hantro_variant imx8mq_vpu_variant = {
> > +const struct hantro_variant imx8mq_vpu_g1_variant = {
> >       .dec_fmts = imx8m_vpu_dec_fmts,
> >       .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
> >       .postproc_fmts = imx8m_vpu_postproc_fmts,
> > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = {
> >       .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
> >                HANTRO_H264_DECODER,
> >       .codec_ops = imx8mq_vpu_codec_ops,
> > -     .init = imx8mq_vpu_hw_init,
> > -     .runtime_resume = imx8mq_runtime_resume,
> > -     .irqs = imx8mq_irqs,
> > -     .num_irqs = ARRAY_SIZE(imx8mq_irqs),
> > -     .clk_names = imx8mq_clk_names,
> > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > -     .reg_names = imx8mq_reg_names,
> > -     .num_regs = ARRAY_SIZE(imx8mq_reg_names)
> > +     .irqs = imx8mq_g1_irqs,
> > +     .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs),
> > +     .clk_names = imx8mq_g1_clk_names,
> > +     .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
> > +     .reg_names = imx8mq_g1_reg_names,
> > +     .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names),
> >  };
> >
> >  const struct hantro_variant imx8mq_vpu_g2_variant = {
> > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
> >       .postproc_ops = &hantro_g2_postproc_ops,
> >       .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
> >       .codec_ops = imx8mq_vpu_g2_codec_ops,
> > -     .init = imx8mq_vpu_hw_init,
> > -     .runtime_resume = imx8mq_runtime_resume,
> >       .irqs = imx8mq_g2_irqs,
> >       .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
> > -     .clk_names = imx8mq_clk_names,
> > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > +     .clk_names = imx8mq_g2_clk_names,
> > +     .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
> > +     .reg_names = imx8mq_g2_reg_names,
> > +     .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
> >  };
> > --
> > 2.32.0
> >

Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

[Lucas Stach]

Am Dienstag, dem 07.12.2021 um 07:24 -0600 schrieb Adam Ford:
> On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> > 
> > Hi Adam,
> > 
> > Thanks for the good work! This is looking quite promising.
> > 
> > On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote:
> > > The VPU in the i.MX8MQ is really the combination of Hantro G1 and
> > > Hantro G2. With the updated vpu-blk-ctrl, the power domains system
> > > can enable and disable them separately as well as pull them out of
> > > reset. This simplifies the code and lets them run independently.
> > > 
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > 
> > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > > index ab2467998d29..d803252a5aba 100644
> > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = {
> > >       { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
> > >  #endif
> > >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > > -     { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
> > > -     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
> > > +     { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, },
> > 
> > I think it's important to clarify that you are breaking support
> > for the previous device-tree binding. Not only because of the compatible
> > string change, but because the binding is now quite different.
> > 
> > Note that in the past Benjamin tried to avoid this.
> > IIRC, his proposal was backwards compatible.
> 
> I was looking at how to do this, but the stand-alone vpu bindings did
> the reset and the fuses manually, it causes issues.  When the block
> control is enabled those memory locations for the resets and fuses are
> assigned to the blk-ctrl driver, so attempting to access them from a
> different driver was a violation.
> 
> When I started looking at this, the stand-alone VPU was trying to be
> both G1 and G2, but when I was testing it before I made changes, I
> didn't see the G2 function at all. When I was done the G2 seemed to
> work, so it seems like this is an improvement.  If you want me to keep
> the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1
> back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd
> have to add the blk-ctrl reference, so it seemed to me like a better
> alternative to deprecate the old binding/driver and replace it with
> the new one because of the significant changes.  Since I'd like to
> rebase the i.MX8M Mini I did on this work, it seemed weird to have
> nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and
> nxp,imx8mm-vpu-g2 where the only one without a Gx name was the
> original 8MQ binding but limited to G1 functionality and the G2
> stripped out.

I would very much appreciate if we could keep the driver code for the
old binding. It does not need to have any new additional functionality,
but just keep the existing G1 h.264 decode when booted on a old DT with
the old VPU description and no blk-ctrl, so we don't regress
functionality when a new kernel is booted with a old DT.

New functionality with the G2 can depend on the new VPU binding and the
blk-ctrl driver.

Regards,
Lucas

> > 
> > If this is unavoidable, due to how the blk-ctrl is handled, then that's
> > fine. Given it's a staging driver, we can still play these games.
> > 
> > Having said that, let's please make this very clear in the commit
> > description, to it's clear for developers forward-porting their kernels.
> > This applies not only to this commit, but to all commits that affect
> > the binding.
> 
> When I post the formal patch, I will add a note that this explicitly
> deprecates the old bindings due to the fact that the G1 and G2 are
> separate IP blocks in the reference manual, and blk-ctrl is a
> completely separate driver altogether.
> 
> > 
> > Thanks!
> > Ezequiel
> > 
> > > +     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, },
> > >  #endif
> > >  #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
> > >       { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > > index cff817ca8d22..122b83a16663 100644
> > > --- a/drivers/staging/media/hantro/hantro_hw.h
> > > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > > @@ -299,8 +299,8 @@ enum hantro_enc_fmt {
> > >       ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
> > >  };
> > > 
> > > +extern const struct hantro_variant imx8mq_vpu_g1_variant;
> > >  extern const struct hantro_variant imx8mq_vpu_g2_variant;
> > > -extern const struct hantro_variant imx8mq_vpu_variant;
> > >  extern const struct hantro_variant px30_vpu_variant;
> > >  extern const struct hantro_variant rk3036_vpu_variant;
> > >  extern const struct hantro_variant rk3066_vpu_variant;
> > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > index 1a43f6fceef9..c9f6e8472258 100644
> > > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > @@ -13,67 +13,6 @@
> > >  #include "hantro_g1_regs.h"
> > >  #include "hantro_g2_regs.h"
> > > 
> > > -#define CTRL_SOFT_RESET              0x00
> > > -#define RESET_G1             BIT(1)
> > > -#define RESET_G2             BIT(0)
> > > -
> > > -#define CTRL_CLOCK_ENABLE    0x04
> > > -#define CLOCK_G1             BIT(1)
> > > -#define CLOCK_G2             BIT(0)
> > > -
> > > -#define CTRL_G1_DEC_FUSE     0x08
> > > -#define CTRL_G1_PP_FUSE              0x0c
> > > -#define CTRL_G2_DEC_FUSE     0x10
> > > -
> > > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> > > -{
> > > -     u32 val;
> > > -
> > > -     /* Assert */
> > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > -     val &= ~reset_bits;
> > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > -
> > > -     udelay(2);
> > > -
> > > -     /* Release */
> > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > -     val |= reset_bits;
> > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > -}
> > > -
> > > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> > > -{
> > > -     u32 val;
> > > -
> > > -     val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > -     val |= clock_bits;
> > > -     writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > -}
> > > -
> > > -static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> > > -{
> > > -     int ret;
> > > -
> > > -     ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks);
> > > -     if (ret) {
> > > -             dev_err(vpu->dev, "Failed to enable clocks\n");
> > > -             return ret;
> > > -     }
> > > -
> > > -     imx8m_soft_reset(vpu, RESET_G1 | RESET_G2);
> > > -     imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
> > > -
> > > -     /* Set values of the fuse registers */
> > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> > > -
> > > -     clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > > -
> > > -     return 0;
> > > -}
> > > -
> > >  /*
> > >   * Supported formats.
> > >   */
> > > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
> > >       return IRQ_HANDLED;
> > >  }
> > > 
> > > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
> > > -{
> > > -     vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
> > > -
> > > -     return 0;
> > > -}
> > > -
> > > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
> > > -{
> > > -     struct hantro_dev *vpu = ctx->dev;
> > > -
> > > -     imx8m_soft_reset(vpu, RESET_G1);
> > > -}
> > > -
> > > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > -{
> > > -     struct hantro_dev *vpu = ctx->dev;
> > > -
> > > -     imx8m_soft_reset(vpu, RESET_G2);
> > > -}
> > > -
> > >  /*
> > >   * Supported codec ops.
> > >   */
> > > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > >  static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > >       [HANTRO_MODE_MPEG2_DEC] = {
> > >               .run = hantro_g1_mpeg2_dec_run,
> > > -             .reset = imx8m_vpu_g1_reset,
> > >               .init = hantro_mpeg2_dec_init,
> > >               .exit = hantro_mpeg2_dec_exit,
> > >       },
> > >       [HANTRO_MODE_VP8_DEC] = {
> > >               .run = hantro_g1_vp8_dec_run,
> > > -             .reset = imx8m_vpu_g1_reset,
> > >               .init = hantro_vp8_dec_init,
> > >               .exit = hantro_vp8_dec_exit,
> > >       },
> > >       [HANTRO_MODE_H264_DEC] = {
> > >               .run = hantro_g1_h264_dec_run,
> > > -             .reset = imx8m_vpu_g1_reset,
> > >               .init = hantro_h264_dec_init,
> > >               .exit = hantro_h264_dec_exit,
> > >       },
> > > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > >  static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > >       [HANTRO_MODE_HEVC_DEC] = {
> > >               .run = hantro_g2_hevc_dec_run,
> > > -             .reset = imx8m_vpu_g2_reset,
> > >               .init = hantro_hevc_dec_init,
> > >               .exit = hantro_hevc_dec_exit,
> > >       },
> > >       [HANTRO_MODE_VP9_DEC] = {
> > >               .run = hantro_g2_vp9_dec_run,
> > >               .done = hantro_g2_vp9_dec_done,
> > > -             .reset = imx8m_vpu_g2_reset,
> > >               .init = hantro_vp9_dec_init,
> > >               .exit = hantro_vp9_dec_exit,
> > >       },
> > > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > >   * VPU variants.
> > >   */
> > > 
> > > -static const struct hantro_irq imx8mq_irqs[] = {
> > > +static const struct hantro_irq imx8mq_g1_irqs[] = {
> > >       { "g1", imx8m_vpu_g1_irq },
> > >  };
> > > 
> > > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
> > >       { "g2", imx8m_vpu_g2_irq },
> > >  };
> > > 
> > > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
> > > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
> > > +static const char * const imx8mq_g1_clk_names[] = { "g1" };
> > > +static const char * const imx8mq_g1_reg_names[] = { "g1" };
> > > +static const char * const imx8mq_g2_clk_names[] = { "g2" };
> > > +static const char * const imx8mq_g2_reg_names[] = { "g2" };
> > > 
> > > -const struct hantro_variant imx8mq_vpu_variant = {
> > > +const struct hantro_variant imx8mq_vpu_g1_variant = {
> > >       .dec_fmts = imx8m_vpu_dec_fmts,
> > >       .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
> > >       .postproc_fmts = imx8m_vpu_postproc_fmts,
> > > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = {
> > >       .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
> > >                HANTRO_H264_DECODER,
> > >       .codec_ops = imx8mq_vpu_codec_ops,
> > > -     .init = imx8mq_vpu_hw_init,
> > > -     .runtime_resume = imx8mq_runtime_resume,
> > > -     .irqs = imx8mq_irqs,
> > > -     .num_irqs = ARRAY_SIZE(imx8mq_irqs),
> > > -     .clk_names = imx8mq_clk_names,
> > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > -     .reg_names = imx8mq_reg_names,
> > > -     .num_regs = ARRAY_SIZE(imx8mq_reg_names)
> > > +     .irqs = imx8mq_g1_irqs,
> > > +     .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs),
> > > +     .clk_names = imx8mq_g1_clk_names,
> > > +     .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
> > > +     .reg_names = imx8mq_g1_reg_names,
> > > +     .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names),
> > >  };
> > > 
> > >  const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
> > >       .postproc_ops = &hantro_g2_postproc_ops,
> > >       .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
> > >       .codec_ops = imx8mq_vpu_g2_codec_ops,
> > > -     .init = imx8mq_vpu_hw_init,
> > > -     .runtime_resume = imx8mq_runtime_resume,
> > >       .irqs = imx8mq_g2_irqs,
> > >       .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
> > > -     .clk_names = imx8mq_clk_names,
> > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > +     .clk_names = imx8mq_g2_clk_names,
> > > +     .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
> > > +     .reg_names = imx8mq_g2_reg_names,
> > > +     .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
> > >  };
> > > --
> > > 2.32.0
> > > 

Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

[Adam Ford]

On Tue, Dec 7, 2021 at 7:41 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Dienstag, dem 07.12.2021 um 07:24 -0600 schrieb Adam Ford:
> > On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia
> > <ezequiel@vanguardiasur.com.ar> wrote:
> > >
> > > Hi Adam,
> > >
> > > Thanks for the good work! This is looking quite promising.
> > >
> > > On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote:
> > > > The VPU in the i.MX8MQ is really the combination of Hantro G1 and
> > > > Hantro G2. With the updated vpu-blk-ctrl, the power domains system
> > > > can enable and disable them separately as well as pull them out of
> > > > reset. This simplifies the code and lets them run independently.
> > > >
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > > > index ab2467998d29..d803252a5aba 100644
> > > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = {
> > > >       { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
> > > >  #endif
> > > >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > > > -     { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
> > > > -     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
> > > > +     { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, },
> > >
> > > I think it's important to clarify that you are breaking support
> > > for the previous device-tree binding. Not only because of the compatible
> > > string change, but because the binding is now quite different.
> > >
> > > Note that in the past Benjamin tried to avoid this.
> > > IIRC, his proposal was backwards compatible.
> >
> > I was looking at how to do this, but the stand-alone vpu bindings did
> > the reset and the fuses manually, it causes issues.  When the block
> > control is enabled those memory locations for the resets and fuses are
> > assigned to the blk-ctrl driver, so attempting to access them from a
> > different driver was a violation.
> >
> > When I started looking at this, the stand-alone VPU was trying to be
> > both G1 and G2, but when I was testing it before I made changes, I
> > didn't see the G2 function at all. When I was done the G2 seemed to
> > work, so it seems like this is an improvement.  If you want me to keep
> > the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1
> > back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd
> > have to add the blk-ctrl reference, so it seemed to me like a better
> > alternative to deprecate the old binding/driver and replace it with
> > the new one because of the significant changes.  Since I'd like to
> > rebase the i.MX8M Mini I did on this work, it seemed weird to have
> > nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and
> > nxp,imx8mm-vpu-g2 where the only one without a Gx name was the
> > original 8MQ binding but limited to G1 functionality and the G2
> > stripped out.
>
> I would very much appreciate if we could keep the driver code for the
> old binding. It does not need to have any new additional functionality,
> but just keep the existing G1 h.264 decode when booted on a old DT with
> the old VPU description and no blk-ctrl, so we don't regress
> functionality when a new kernel is booted with a old DT.
>
> New functionality with the G2 can depend on the new VPU binding and the
> blk-ctrl driver.

How does that work when both the original VPU and the blk-ctrl attempt
to manipulate the reset and clock lines?  The original binding for the
vpu was assigned:

reg = <0x38300000 0x10000>,
          <0x38310000 0x10000>,
          <0x38320000 0x10000>;
reg-names = "g1", "g2", "ctrl";

If G2 is going to run from 38310000 and vpu-blk-ctrl run from
38320000, they'll collide.


>
> Regards,
> Lucas
>
> > >
> > > If this is unavoidable, due to how the blk-ctrl is handled, then that's
> > > fine. Given it's a staging driver, we can still play these games.
> > >
> > > Having said that, let's please make this very clear in the commit
> > > description, to it's clear for developers forward-porting their kernels.
> > > This applies not only to this commit, but to all commits that affect
> > > the binding.
> >
> > When I post the formal patch, I will add a note that this explicitly
> > deprecates the old bindings due to the fact that the G1 and G2 are
> > separate IP blocks in the reference manual, and blk-ctrl is a
> > completely separate driver altogether.
> >
> > >
> > > Thanks!
> > > Ezequiel
> > >
> > > > +     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, },
> > > >  #endif
> > > >  #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
> > > >       { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> > > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > > > index cff817ca8d22..122b83a16663 100644
> > > > --- a/drivers/staging/media/hantro/hantro_hw.h
> > > > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > > > @@ -299,8 +299,8 @@ enum hantro_enc_fmt {
> > > >       ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
> > > >  };
> > > >
> > > > +extern const struct hantro_variant imx8mq_vpu_g1_variant;
> > > >  extern const struct hantro_variant imx8mq_vpu_g2_variant;
> > > > -extern const struct hantro_variant imx8mq_vpu_variant;
> > > >  extern const struct hantro_variant px30_vpu_variant;
> > > >  extern const struct hantro_variant rk3036_vpu_variant;
> > > >  extern const struct hantro_variant rk3066_vpu_variant;
> > > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > index 1a43f6fceef9..c9f6e8472258 100644
> > > > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > @@ -13,67 +13,6 @@
> > > >  #include "hantro_g1_regs.h"
> > > >  #include "hantro_g2_regs.h"
> > > >
> > > > -#define CTRL_SOFT_RESET              0x00
> > > > -#define RESET_G1             BIT(1)
> > > > -#define RESET_G2             BIT(0)
> > > > -
> > > > -#define CTRL_CLOCK_ENABLE    0x04
> > > > -#define CLOCK_G1             BIT(1)
> > > > -#define CLOCK_G2             BIT(0)
> > > > -
> > > > -#define CTRL_G1_DEC_FUSE     0x08
> > > > -#define CTRL_G1_PP_FUSE              0x0c
> > > > -#define CTRL_G2_DEC_FUSE     0x10
> > > > -
> > > > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> > > > -{
> > > > -     u32 val;
> > > > -
> > > > -     /* Assert */
> > > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > -     val &= ~reset_bits;
> > > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > -
> > > > -     udelay(2);
> > > > -
> > > > -     /* Release */
> > > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > -     val |= reset_bits;
> > > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > -}
> > > > -
> > > > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> > > > -{
> > > > -     u32 val;
> > > > -
> > > > -     val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > > -     val |= clock_bits;
> > > > -     writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > > -}
> > > > -
> > > > -static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> > > > -{
> > > > -     int ret;
> > > > -
> > > > -     ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks);
> > > > -     if (ret) {
> > > > -             dev_err(vpu->dev, "Failed to enable clocks\n");
> > > > -             return ret;
> > > > -     }
> > > > -
> > > > -     imx8m_soft_reset(vpu, RESET_G1 | RESET_G2);
> > > > -     imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
> > > > -
> > > > -     /* Set values of the fuse registers */
> > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> > > > -
> > > > -     clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > > > -
> > > > -     return 0;
> > > > -}
> > > > -
> > > >  /*
> > > >   * Supported formats.
> > > >   */
> > > > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
> > > >       return IRQ_HANDLED;
> > > >  }
> > > >
> > > > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
> > > > -{
> > > > -     vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
> > > > -
> > > > -     return 0;
> > > > -}
> > > > -
> > > > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
> > > > -{
> > > > -     struct hantro_dev *vpu = ctx->dev;
> > > > -
> > > > -     imx8m_soft_reset(vpu, RESET_G1);
> > > > -}
> > > > -
> > > > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > > -{
> > > > -     struct hantro_dev *vpu = ctx->dev;
> > > > -
> > > > -     imx8m_soft_reset(vpu, RESET_G2);
> > > > -}
> > > > -
> > > >  /*
> > > >   * Supported codec ops.
> > > >   */
> > > > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > >  static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > > >       [HANTRO_MODE_MPEG2_DEC] = {
> > > >               .run = hantro_g1_mpeg2_dec_run,
> > > > -             .reset = imx8m_vpu_g1_reset,
> > > >               .init = hantro_mpeg2_dec_init,
> > > >               .exit = hantro_mpeg2_dec_exit,
> > > >       },
> > > >       [HANTRO_MODE_VP8_DEC] = {
> > > >               .run = hantro_g1_vp8_dec_run,
> > > > -             .reset = imx8m_vpu_g1_reset,
> > > >               .init = hantro_vp8_dec_init,
> > > >               .exit = hantro_vp8_dec_exit,
> > > >       },
> > > >       [HANTRO_MODE_H264_DEC] = {
> > > >               .run = hantro_g1_h264_dec_run,
> > > > -             .reset = imx8m_vpu_g1_reset,
> > > >               .init = hantro_h264_dec_init,
> > > >               .exit = hantro_h264_dec_exit,
> > > >       },
> > > > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > > >  static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > > >       [HANTRO_MODE_HEVC_DEC] = {
> > > >               .run = hantro_g2_hevc_dec_run,
> > > > -             .reset = imx8m_vpu_g2_reset,
> > > >               .init = hantro_hevc_dec_init,
> > > >               .exit = hantro_hevc_dec_exit,
> > > >       },
> > > >       [HANTRO_MODE_VP9_DEC] = {
> > > >               .run = hantro_g2_vp9_dec_run,
> > > >               .done = hantro_g2_vp9_dec_done,
> > > > -             .reset = imx8m_vpu_g2_reset,
> > > >               .init = hantro_vp9_dec_init,
> > > >               .exit = hantro_vp9_dec_exit,
> > > >       },
> > > > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > > >   * VPU variants.
> > > >   */
> > > >
> > > > -static const struct hantro_irq imx8mq_irqs[] = {
> > > > +static const struct hantro_irq imx8mq_g1_irqs[] = {
> > > >       { "g1", imx8m_vpu_g1_irq },
> > > >  };
> > > >
> > > > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
> > > >       { "g2", imx8m_vpu_g2_irq },
> > > >  };
> > > >
> > > > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
> > > > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
> > > > +static const char * const imx8mq_g1_clk_names[] = { "g1" };
> > > > +static const char * const imx8mq_g1_reg_names[] = { "g1" };
> > > > +static const char * const imx8mq_g2_clk_names[] = { "g2" };
> > > > +static const char * const imx8mq_g2_reg_names[] = { "g2" };
> > > >
> > > > -const struct hantro_variant imx8mq_vpu_variant = {
> > > > +const struct hantro_variant imx8mq_vpu_g1_variant = {
> > > >       .dec_fmts = imx8m_vpu_dec_fmts,
> > > >       .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
> > > >       .postproc_fmts = imx8m_vpu_postproc_fmts,
> > > > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = {
> > > >       .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
> > > >                HANTRO_H264_DECODER,
> > > >       .codec_ops = imx8mq_vpu_codec_ops,
> > > > -     .init = imx8mq_vpu_hw_init,
> > > > -     .runtime_resume = imx8mq_runtime_resume,
> > > > -     .irqs = imx8mq_irqs,
> > > > -     .num_irqs = ARRAY_SIZE(imx8mq_irqs),
> > > > -     .clk_names = imx8mq_clk_names,
> > > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > > -     .reg_names = imx8mq_reg_names,
> > > > -     .num_regs = ARRAY_SIZE(imx8mq_reg_names)
> > > > +     .irqs = imx8mq_g1_irqs,
> > > > +     .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs),
> > > > +     .clk_names = imx8mq_g1_clk_names,
> > > > +     .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
> > > > +     .reg_names = imx8mq_g1_reg_names,
> > > > +     .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names),
> > > >  };
> > > >
> > > >  const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > >       .postproc_ops = &hantro_g2_postproc_ops,
> > > >       .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
> > > >       .codec_ops = imx8mq_vpu_g2_codec_ops,
> > > > -     .init = imx8mq_vpu_hw_init,
> > > > -     .runtime_resume = imx8mq_runtime_resume,
> > > >       .irqs = imx8mq_g2_irqs,
> > > >       .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
> > > > -     .clk_names = imx8mq_clk_names,
> > > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > > +     .clk_names = imx8mq_g2_clk_names,
> > > > +     .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
> > > > +     .reg_names = imx8mq_g2_reg_names,
> > > > +     .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
> > > >  };
> > > > --
> > > > 2.32.0
> > > >
>
>

Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

[Lucas Stach]

Am Dienstag, dem 07.12.2021 um 07:50 -0600 schrieb Adam Ford:
> On Tue, Dec 7, 2021 at 7:41 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Dienstag, dem 07.12.2021 um 07:24 -0600 schrieb Adam Ford:
> > > On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia
> > > <ezequiel@vanguardiasur.com.ar> wrote:
> > > > 
> > > > Hi Adam,
> > > > 
> > > > Thanks for the good work! This is looking quite promising.
> > > > 
> > > > On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote:
> > > > > The VPU in the i.MX8MQ is really the combination of Hantro G1 and
> > > > > Hantro G2. With the updated vpu-blk-ctrl, the power domains system
> > > > > can enable and disable them separately as well as pull them out of
> > > > > reset. This simplifies the code and lets them run independently.
> > > > > 
> > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > 
> > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > > > > index ab2467998d29..d803252a5aba 100644
> > > > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > > > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = {
> > > > >       { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
> > > > >  #endif
> > > > >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > > > > -     { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
> > > > > -     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
> > > > > +     { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, },
> > > > 
> > > > I think it's important to clarify that you are breaking support
> > > > for the previous device-tree binding. Not only because of the compatible
> > > > string change, but because the binding is now quite different.
> > > > 
> > > > Note that in the past Benjamin tried to avoid this.
> > > > IIRC, his proposal was backwards compatible.
> > > 
> > > I was looking at how to do this, but the stand-alone vpu bindings did
> > > the reset and the fuses manually, it causes issues.  When the block
> > > control is enabled those memory locations for the resets and fuses are
> > > assigned to the blk-ctrl driver, so attempting to access them from a
> > > different driver was a violation.
> > > 
> > > When I started looking at this, the stand-alone VPU was trying to be
> > > both G1 and G2, but when I was testing it before I made changes, I
> > > didn't see the G2 function at all. When I was done the G2 seemed to
> > > work, so it seems like this is an improvement.  If you want me to keep
> > > the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1
> > > back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd
> > > have to add the blk-ctrl reference, so it seemed to me like a better
> > > alternative to deprecate the old binding/driver and replace it with
> > > the new one because of the significant changes.  Since I'd like to
> > > rebase the i.MX8M Mini I did on this work, it seemed weird to have
> > > nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and
> > > nxp,imx8mm-vpu-g2 where the only one without a Gx name was the
> > > original 8MQ binding but limited to G1 functionality and the G2
> > > stripped out.
> > 
> > I would very much appreciate if we could keep the driver code for the
> > old binding. It does not need to have any new additional functionality,
> > but just keep the existing G1 h.264 decode when booted on a old DT with
> > the old VPU description and no blk-ctrl, so we don't regress
> > functionality when a new kernel is booted with a old DT.
> > 
> > New functionality with the G2 can depend on the new VPU binding and the
> > blk-ctrl driver.
> 
> How does that work when both the original VPU and the blk-ctrl attempt
> to manipulate the reset and clock lines?  The original binding for the
> vpu was assigned:
> 
> reg = <0x38300000 0x10000>,
>           <0x38310000 0x10000>,
>           <0x38320000 0x10000>;
> reg-names = "g1", "g2", "ctrl";
> 
> If G2 is going to run from 38310000 and vpu-blk-ctrl run from
> 38320000, they'll collide.
> 
It's not going to work, but it also doesn't have to. Either you have a
old DT where the VPU driver will poke the blk-ctrl registers, but no
blk-ctrl driver, or you have a new DT where the VPU driver leaves the
blk-ctrl region alone and the blk-ctrl driver needs to handle it.

Just don't support mixing the old VPU DT binding with the new blk-ctrl
way of doing things. The only thing that needs to keep working is a
unchannged old DT, where the VPU uses the old binding, but no blk-ctrl
is present as a separate node.

Regards,
Lucas
> 
> > 
> > Regards,
> > Lucas
> > 
> > > > 
> > > > If this is unavoidable, due to how the blk-ctrl is handled, then that's
> > > > fine. Given it's a staging driver, we can still play these games.
> > > > 
> > > > Having said that, let's please make this very clear in the commit
> > > > description, to it's clear for developers forward-porting their kernels.
> > > > This applies not only to this commit, but to all commits that affect
> > > > the binding.
> > > 
> > > When I post the formal patch, I will add a note that this explicitly
> > > deprecates the old bindings due to the fact that the G1 and G2 are
> > > separate IP blocks in the reference manual, and blk-ctrl is a
> > > completely separate driver altogether.
> > > 
> > > > 
> > > > Thanks!
> > > > Ezequiel
> > > > 
> > > > > +     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, },
> > > > >  #endif
> > > > >  #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
> > > > >       { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> > > > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > > > > index cff817ca8d22..122b83a16663 100644
> > > > > --- a/drivers/staging/media/hantro/hantro_hw.h
> > > > > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > > > > @@ -299,8 +299,8 @@ enum hantro_enc_fmt {
> > > > >       ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
> > > > >  };
> > > > > 
> > > > > +extern const struct hantro_variant imx8mq_vpu_g1_variant;
> > > > >  extern const struct hantro_variant imx8mq_vpu_g2_variant;
> > > > > -extern const struct hantro_variant imx8mq_vpu_variant;
> > > > >  extern const struct hantro_variant px30_vpu_variant;
> > > > >  extern const struct hantro_variant rk3036_vpu_variant;
> > > > >  extern const struct hantro_variant rk3066_vpu_variant;
> > > > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > index 1a43f6fceef9..c9f6e8472258 100644
> > > > > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > @@ -13,67 +13,6 @@
> > > > >  #include "hantro_g1_regs.h"
> > > > >  #include "hantro_g2_regs.h"
> > > > > 
> > > > > -#define CTRL_SOFT_RESET              0x00
> > > > > -#define RESET_G1             BIT(1)
> > > > > -#define RESET_G2             BIT(0)
> > > > > -
> > > > > -#define CTRL_CLOCK_ENABLE    0x04
> > > > > -#define CLOCK_G1             BIT(1)
> > > > > -#define CLOCK_G2             BIT(0)
> > > > > -
> > > > > -#define CTRL_G1_DEC_FUSE     0x08
> > > > > -#define CTRL_G1_PP_FUSE              0x0c
> > > > > -#define CTRL_G2_DEC_FUSE     0x10
> > > > > -
> > > > > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> > > > > -{
> > > > > -     u32 val;
> > > > > -
> > > > > -     /* Assert */
> > > > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > -     val &= ~reset_bits;
> > > > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > -
> > > > > -     udelay(2);
> > > > > -
> > > > > -     /* Release */
> > > > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > -     val |= reset_bits;
> > > > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > -}
> > > > > -
> > > > > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> > > > > -{
> > > > > -     u32 val;
> > > > > -
> > > > > -     val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > > > -     val |= clock_bits;
> > > > > -     writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > > > -}
> > > > > -
> > > > > -static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> > > > > -{
> > > > > -     int ret;
> > > > > -
> > > > > -     ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks);
> > > > > -     if (ret) {
> > > > > -             dev_err(vpu->dev, "Failed to enable clocks\n");
> > > > > -             return ret;
> > > > > -     }
> > > > > -
> > > > > -     imx8m_soft_reset(vpu, RESET_G1 | RESET_G2);
> > > > > -     imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
> > > > > -
> > > > > -     /* Set values of the fuse registers */
> > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> > > > > -
> > > > > -     clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > > > > -
> > > > > -     return 0;
> > > > > -}
> > > > > -
> > > > >  /*
> > > > >   * Supported formats.
> > > > >   */
> > > > > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
> > > > >       return IRQ_HANDLED;
> > > > >  }
> > > > > 
> > > > > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
> > > > > -{
> > > > > -     vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
> > > > > -
> > > > > -     return 0;
> > > > > -}
> > > > > -
> > > > > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
> > > > > -{
> > > > > -     struct hantro_dev *vpu = ctx->dev;
> > > > > -
> > > > > -     imx8m_soft_reset(vpu, RESET_G1);
> > > > > -}
> > > > > -
> > > > > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > > > -{
> > > > > -     struct hantro_dev *vpu = ctx->dev;
> > > > > -
> > > > > -     imx8m_soft_reset(vpu, RESET_G2);
> > > > > -}
> > > > > -
> > > > >  /*
> > > > >   * Supported codec ops.
> > > > >   */
> > > > > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > > >  static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > > > >       [HANTRO_MODE_MPEG2_DEC] = {
> > > > >               .run = hantro_g1_mpeg2_dec_run,
> > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > >               .init = hantro_mpeg2_dec_init,
> > > > >               .exit = hantro_mpeg2_dec_exit,
> > > > >       },
> > > > >       [HANTRO_MODE_VP8_DEC] = {
> > > > >               .run = hantro_g1_vp8_dec_run,
> > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > >               .init = hantro_vp8_dec_init,
> > > > >               .exit = hantro_vp8_dec_exit,
> > > > >       },
> > > > >       [HANTRO_MODE_H264_DEC] = {
> > > > >               .run = hantro_g1_h264_dec_run,
> > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > >               .init = hantro_h264_dec_init,
> > > > >               .exit = hantro_h264_dec_exit,
> > > > >       },
> > > > > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > > > >  static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > > > >       [HANTRO_MODE_HEVC_DEC] = {
> > > > >               .run = hantro_g2_hevc_dec_run,
> > > > > -             .reset = imx8m_vpu_g2_reset,
> > > > >               .init = hantro_hevc_dec_init,
> > > > >               .exit = hantro_hevc_dec_exit,
> > > > >       },
> > > > >       [HANTRO_MODE_VP9_DEC] = {
> > > > >               .run = hantro_g2_vp9_dec_run,
> > > > >               .done = hantro_g2_vp9_dec_done,
> > > > > -             .reset = imx8m_vpu_g2_reset,
> > > > >               .init = hantro_vp9_dec_init,
> > > > >               .exit = hantro_vp9_dec_exit,
> > > > >       },
> > > > > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > > > >   * VPU variants.
> > > > >   */
> > > > > 
> > > > > -static const struct hantro_irq imx8mq_irqs[] = {
> > > > > +static const struct hantro_irq imx8mq_g1_irqs[] = {
> > > > >       { "g1", imx8m_vpu_g1_irq },
> > > > >  };
> > > > > 
> > > > > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
> > > > >       { "g2", imx8m_vpu_g2_irq },
> > > > >  };
> > > > > 
> > > > > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
> > > > > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
> > > > > +static const char * const imx8mq_g1_clk_names[] = { "g1" };
> > > > > +static const char * const imx8mq_g1_reg_names[] = { "g1" };
> > > > > +static const char * const imx8mq_g2_clk_names[] = { "g2" };
> > > > > +static const char * const imx8mq_g2_reg_names[] = { "g2" };
> > > > > 
> > > > > -const struct hantro_variant imx8mq_vpu_variant = {
> > > > > +const struct hantro_variant imx8mq_vpu_g1_variant = {
> > > > >       .dec_fmts = imx8m_vpu_dec_fmts,
> > > > >       .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
> > > > >       .postproc_fmts = imx8m_vpu_postproc_fmts,
> > > > > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = {
> > > > >       .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
> > > > >                HANTRO_H264_DECODER,
> > > > >       .codec_ops = imx8mq_vpu_codec_ops,
> > > > > -     .init = imx8mq_vpu_hw_init,
> > > > > -     .runtime_resume = imx8mq_runtime_resume,
> > > > > -     .irqs = imx8mq_irqs,
> > > > > -     .num_irqs = ARRAY_SIZE(imx8mq_irqs),
> > > > > -     .clk_names = imx8mq_clk_names,
> > > > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > > > -     .reg_names = imx8mq_reg_names,
> > > > > -     .num_regs = ARRAY_SIZE(imx8mq_reg_names)
> > > > > +     .irqs = imx8mq_g1_irqs,
> > > > > +     .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs),
> > > > > +     .clk_names = imx8mq_g1_clk_names,
> > > > > +     .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
> > > > > +     .reg_names = imx8mq_g1_reg_names,
> > > > > +     .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names),
> > > > >  };
> > > > > 
> > > > >  const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > > > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > > >       .postproc_ops = &hantro_g2_postproc_ops,
> > > > >       .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
> > > > >       .codec_ops = imx8mq_vpu_g2_codec_ops,
> > > > > -     .init = imx8mq_vpu_hw_init,
> > > > > -     .runtime_resume = imx8mq_runtime_resume,
> > > > >       .irqs = imx8mq_g2_irqs,
> > > > >       .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
> > > > > -     .clk_names = imx8mq_clk_names,
> > > > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > > > +     .clk_names = imx8mq_g2_clk_names,
> > > > > +     .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
> > > > > +     .reg_names = imx8mq_g2_reg_names,
> > > > > +     .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
> > > > >  };
> > > > > --
> > > > > 2.32.0
> > > > > 
> > 
> > 

Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

[Adam Ford]

On Tue, Dec 7, 2021 at 7:56 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Dienstag, dem 07.12.2021 um 07:50 -0600 schrieb Adam Ford:
> > On Tue, Dec 7, 2021 at 7:41 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Am Dienstag, dem 07.12.2021 um 07:24 -0600 schrieb Adam Ford:
> > > > On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia
> > > > <ezequiel@vanguardiasur.com.ar> wrote:
> > > > >
> > > > > Hi Adam,
> > > > >
> > > > > Thanks for the good work! This is looking quite promising.
> > > > >
> > > > > On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote:
> > > > > > The VPU in the i.MX8MQ is really the combination of Hantro G1 and
> > > > > > Hantro G2. With the updated vpu-blk-ctrl, the power domains system
> > > > > > can enable and disable them separately as well as pull them out of
> > > > > > reset. This simplifies the code and lets them run independently.
> > > > > >
> > > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > >
> > > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > > > > > index ab2467998d29..d803252a5aba 100644
> > > > > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > > > > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = {
> > > > > >       { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
> > > > > >  #endif
> > > > > >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > > > > > -     { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
> > > > > > -     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
> > > > > > +     { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, },
> > > > >
> > > > > I think it's important to clarify that you are breaking support
> > > > > for the previous device-tree binding. Not only because of the compatible
> > > > > string change, but because the binding is now quite different.
> > > > >
> > > > > Note that in the past Benjamin tried to avoid this.
> > > > > IIRC, his proposal was backwards compatible.
> > > >
> > > > I was looking at how to do this, but the stand-alone vpu bindings did
> > > > the reset and the fuses manually, it causes issues.  When the block
> > > > control is enabled those memory locations for the resets and fuses are
> > > > assigned to the blk-ctrl driver, so attempting to access them from a
> > > > different driver was a violation.
> > > >
> > > > When I started looking at this, the stand-alone VPU was trying to be
> > > > both G1 and G2, but when I was testing it before I made changes, I
> > > > didn't see the G2 function at all. When I was done the G2 seemed to
> > > > work, so it seems like this is an improvement.  If you want me to keep
> > > > the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1
> > > > back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd
> > > > have to add the blk-ctrl reference, so it seemed to me like a better
> > > > alternative to deprecate the old binding/driver and replace it with
> > > > the new one because of the significant changes.  Since I'd like to
> > > > rebase the i.MX8M Mini I did on this work, it seemed weird to have
> > > > nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and
> > > > nxp,imx8mm-vpu-g2 where the only one without a Gx name was the
> > > > original 8MQ binding but limited to G1 functionality and the G2
> > > > stripped out.
> > >
> > > I would very much appreciate if we could keep the driver code for the
> > > old binding. It does not need to have any new additional functionality,
> > > but just keep the existing G1 h.264 decode when booted on a old DT with
> > > the old VPU description and no blk-ctrl, so we don't regress
> > > functionality when a new kernel is booted with a old DT.
> > >
> > > New functionality with the G2 can depend on the new VPU binding and the
> > > blk-ctrl driver.
> >
> > How does that work when both the original VPU and the blk-ctrl attempt
> > to manipulate the reset and clock lines?  The original binding for the
> > vpu was assigned:
> >
> > reg = <0x38300000 0x10000>,
> >           <0x38310000 0x10000>,
> >           <0x38320000 0x10000>;
> > reg-names = "g1", "g2", "ctrl";
> >
> > If G2 is going to run from 38310000 and vpu-blk-ctrl run from
> > 38320000, they'll collide.
> >
> It's not going to work, but it also doesn't have to. Either you have a
> old DT where the VPU driver will poke the blk-ctrl registers, but no
> blk-ctrl driver, or you have a new DT where the VPU driver leaves the
> blk-ctrl region alone and the blk-ctrl driver needs to handle it.
>
> Just don't support mixing the old VPU DT binding with the new blk-ctrl
> way of doing things. The only thing that needs to keep working is a
> unchannged old DT, where the VPU uses the old binding, but no blk-ctrl
> is present as a separate node.

I think I understand.  I'll leave the old code for the old binding in
the driver and add the new code for the new bindings with blk-ctrl.
Once the device tree is migrated to the new bindings, the old one
becomes harmless, but still works with old device trees lacking the
blk-ctrl.  That makes sense.  In my head, I wasn't thinking about
mixing older device trees.

adam
>
> Regards,
> Lucas
> >
> > >
> > > Regards,
> > > Lucas
> > >
> > > > >
> > > > > If this is unavoidable, due to how the blk-ctrl is handled, then that's
> > > > > fine. Given it's a staging driver, we can still play these games.
> > > > >
> > > > > Having said that, let's please make this very clear in the commit
> > > > > description, to it's clear for developers forward-porting their kernels.
> > > > > This applies not only to this commit, but to all commits that affect
> > > > > the binding.
> > > >
> > > > When I post the formal patch, I will add a note that this explicitly
> > > > deprecates the old bindings due to the fact that the G1 and G2 are
> > > > separate IP blocks in the reference manual, and blk-ctrl is a
> > > > completely separate driver altogether.
> > > >
> > > > >
> > > > > Thanks!
> > > > > Ezequiel
> > > > >
> > > > > > +     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, },
> > > > > >  #endif
> > > > > >  #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
> > > > > >       { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> > > > > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > > > > > index cff817ca8d22..122b83a16663 100644
> > > > > > --- a/drivers/staging/media/hantro/hantro_hw.h
> > > > > > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > > > > > @@ -299,8 +299,8 @@ enum hantro_enc_fmt {
> > > > > >       ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
> > > > > >  };
> > > > > >
> > > > > > +extern const struct hantro_variant imx8mq_vpu_g1_variant;
> > > > > >  extern const struct hantro_variant imx8mq_vpu_g2_variant;
> > > > > > -extern const struct hantro_variant imx8mq_vpu_variant;
> > > > > >  extern const struct hantro_variant px30_vpu_variant;
> > > > > >  extern const struct hantro_variant rk3036_vpu_variant;
> > > > > >  extern const struct hantro_variant rk3066_vpu_variant;
> > > > > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > > index 1a43f6fceef9..c9f6e8472258 100644
> > > > > > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > > @@ -13,67 +13,6 @@
> > > > > >  #include "hantro_g1_regs.h"
> > > > > >  #include "hantro_g2_regs.h"
> > > > > >
> > > > > > -#define CTRL_SOFT_RESET              0x00
> > > > > > -#define RESET_G1             BIT(1)
> > > > > > -#define RESET_G2             BIT(0)
> > > > > > -
> > > > > > -#define CTRL_CLOCK_ENABLE    0x04
> > > > > > -#define CLOCK_G1             BIT(1)
> > > > > > -#define CLOCK_G2             BIT(0)
> > > > > > -
> > > > > > -#define CTRL_G1_DEC_FUSE     0x08
> > > > > > -#define CTRL_G1_PP_FUSE              0x0c
> > > > > > -#define CTRL_G2_DEC_FUSE     0x10
> > > > > > -
> > > > > > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> > > > > > -{
> > > > > > -     u32 val;
> > > > > > -
> > > > > > -     /* Assert */
> > > > > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > > -     val &= ~reset_bits;
> > > > > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > > -
> > > > > > -     udelay(2);
> > > > > > -
> > > > > > -     /* Release */
> > > > > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > > -     val |= reset_bits;
> > > > > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > > -}
> > > > > > -
> > > > > > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> > > > > > -{
> > > > > > -     u32 val;
> > > > > > -
> > > > > > -     val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > > > > -     val |= clock_bits;
> > > > > > -     writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > > > > -}
> > > > > > -
> > > > > > -static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> > > > > > -{
> > > > > > -     int ret;
> > > > > > -
> > > > > > -     ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks);
> > > > > > -     if (ret) {
> > > > > > -             dev_err(vpu->dev, "Failed to enable clocks\n");
> > > > > > -             return ret;
> > > > > > -     }
> > > > > > -
> > > > > > -     imx8m_soft_reset(vpu, RESET_G1 | RESET_G2);
> > > > > > -     imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
> > > > > > -
> > > > > > -     /* Set values of the fuse registers */
> > > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> > > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> > > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> > > > > > -
> > > > > > -     clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > > > > > -
> > > > > > -     return 0;
> > > > > > -}
> > > > > > -
> > > > > >  /*
> > > > > >   * Supported formats.
> > > > > >   */
> > > > > > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
> > > > > >       return IRQ_HANDLED;
> > > > > >  }
> > > > > >
> > > > > > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
> > > > > > -{
> > > > > > -     vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
> > > > > > -
> > > > > > -     return 0;
> > > > > > -}
> > > > > > -
> > > > > > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
> > > > > > -{
> > > > > > -     struct hantro_dev *vpu = ctx->dev;
> > > > > > -
> > > > > > -     imx8m_soft_reset(vpu, RESET_G1);
> > > > > > -}
> > > > > > -
> > > > > > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > > > > -{
> > > > > > -     struct hantro_dev *vpu = ctx->dev;
> > > > > > -
> > > > > > -     imx8m_soft_reset(vpu, RESET_G2);
> > > > > > -}
> > > > > > -
> > > > > >  /*
> > > > > >   * Supported codec ops.
> > > > > >   */
> > > > > > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > > > >  static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > > > > >       [HANTRO_MODE_MPEG2_DEC] = {
> > > > > >               .run = hantro_g1_mpeg2_dec_run,
> > > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > > >               .init = hantro_mpeg2_dec_init,
> > > > > >               .exit = hantro_mpeg2_dec_exit,
> > > > > >       },
> > > > > >       [HANTRO_MODE_VP8_DEC] = {
> > > > > >               .run = hantro_g1_vp8_dec_run,
> > > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > > >               .init = hantro_vp8_dec_init,
> > > > > >               .exit = hantro_vp8_dec_exit,
> > > > > >       },
> > > > > >       [HANTRO_MODE_H264_DEC] = {
> > > > > >               .run = hantro_g1_h264_dec_run,
> > > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > > >               .init = hantro_h264_dec_init,
> > > > > >               .exit = hantro_h264_dec_exit,
> > > > > >       },
> > > > > > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > > > > >  static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > > > > >       [HANTRO_MODE_HEVC_DEC] = {
> > > > > >               .run = hantro_g2_hevc_dec_run,
> > > > > > -             .reset = imx8m_vpu_g2_reset,
> > > > > >               .init = hantro_hevc_dec_init,
> > > > > >               .exit = hantro_hevc_dec_exit,
> > > > > >       },
> > > > > >       [HANTRO_MODE_VP9_DEC] = {
> > > > > >               .run = hantro_g2_vp9_dec_run,
> > > > > >               .done = hantro_g2_vp9_dec_done,
> > > > > > -             .reset = imx8m_vpu_g2_reset,
> > > > > >               .init = hantro_vp9_dec_init,
> > > > > >               .exit = hantro_vp9_dec_exit,
> > > > > >       },
> > > > > > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > > > > >   * VPU variants.
> > > > > >   */
> > > > > >
> > > > > > -static const struct hantro_irq imx8mq_irqs[] = {
> > > > > > +static const struct hantro_irq imx8mq_g1_irqs[] = {
> > > > > >       { "g1", imx8m_vpu_g1_irq },
> > > > > >  };
> > > > > >
> > > > > > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
> > > > > >       { "g2", imx8m_vpu_g2_irq },
> > > > > >  };
> > > > > >
> > > > > > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
> > > > > > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
> > > > > > +static const char * const imx8mq_g1_clk_names[] = { "g1" };
> > > > > > +static const char * const imx8mq_g1_reg_names[] = { "g1" };
> > > > > > +static const char * const imx8mq_g2_clk_names[] = { "g2" };
> > > > > > +static const char * const imx8mq_g2_reg_names[] = { "g2" };
> > > > > >
> > > > > > -const struct hantro_variant imx8mq_vpu_variant = {
> > > > > > +const struct hantro_variant imx8mq_vpu_g1_variant = {
> > > > > >       .dec_fmts = imx8m_vpu_dec_fmts,
> > > > > >       .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
> > > > > >       .postproc_fmts = imx8m_vpu_postproc_fmts,
> > > > > > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = {
> > > > > >       .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
> > > > > >                HANTRO_H264_DECODER,
> > > > > >       .codec_ops = imx8mq_vpu_codec_ops,
> > > > > > -     .init = imx8mq_vpu_hw_init,
> > > > > > -     .runtime_resume = imx8mq_runtime_resume,
> > > > > > -     .irqs = imx8mq_irqs,
> > > > > > -     .num_irqs = ARRAY_SIZE(imx8mq_irqs),
> > > > > > -     .clk_names = imx8mq_clk_names,
> > > > > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > > > > -     .reg_names = imx8mq_reg_names,
> > > > > > -     .num_regs = ARRAY_SIZE(imx8mq_reg_names)
> > > > > > +     .irqs = imx8mq_g1_irqs,
> > > > > > +     .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs),
> > > > > > +     .clk_names = imx8mq_g1_clk_names,
> > > > > > +     .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
> > > > > > +     .reg_names = imx8mq_g1_reg_names,
> > > > > > +     .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names),
> > > > > >  };
> > > > > >
> > > > > >  const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > > > > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > > > >       .postproc_ops = &hantro_g2_postproc_ops,
> > > > > >       .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
> > > > > >       .codec_ops = imx8mq_vpu_g2_codec_ops,
> > > > > > -     .init = imx8mq_vpu_hw_init,
> > > > > > -     .runtime_resume = imx8mq_runtime_resume,
> > > > > >       .irqs = imx8mq_g2_irqs,
> > > > > >       .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
> > > > > > -     .clk_names = imx8mq_clk_names,
> > > > > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > > > > +     .clk_names = imx8mq_g2_clk_names,
> > > > > > +     .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
> > > > > > +     .reg_names = imx8mq_g2_reg_names,
> > > > > > +     .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
> > > > > >  };
> > > > > > --
> > > > > > 2.32.0
> > > > > >
> > >
> > >
>
>

Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

[Lucas Stach]

Am Dienstag, dem 07.12.2021 um 08:07 -0600 schrieb Adam Ford:
> > > > > > 
[...]
> > > > > > I think it's important to clarify that you are breaking support
> > > > > > for the previous device-tree binding. Not only because of the compatible
> > > > > > string change, but because the binding is now quite different.
> > > > > > 
> > > > > > Note that in the past Benjamin tried to avoid this.
> > > > > > IIRC, his proposal was backwards compatible.
> > > > > 
> > > > > I was looking at how to do this, but the stand-alone vpu bindings did
> > > > > the reset and the fuses manually, it causes issues.  When the block
> > > > > control is enabled those memory locations for the resets and fuses are
> > > > > assigned to the blk-ctrl driver, so attempting to access them from a
> > > > > different driver was a violation.
> > > > > 
> > > > > When I started looking at this, the stand-alone VPU was trying to be
> > > > > both G1 and G2, but when I was testing it before I made changes, I
> > > > > didn't see the G2 function at all. When I was done the G2 seemed to
> > > > > work, so it seems like this is an improvement.  If you want me to keep
> > > > > the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1
> > > > > back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd
> > > > > have to add the blk-ctrl reference, so it seemed to me like a better
> > > > > alternative to deprecate the old binding/driver and replace it with
> > > > > the new one because of the significant changes.  Since I'd like to
> > > > > rebase the i.MX8M Mini I did on this work, it seemed weird to have
> > > > > nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and
> > > > > nxp,imx8mm-vpu-g2 where the only one without a Gx name was the
> > > > > original 8MQ binding but limited to G1 functionality and the G2
> > > > > stripped out.
> > > > 
> > > > I would very much appreciate if we could keep the driver code for the
> > > > old binding. It does not need to have any new additional functionality,
> > > > but just keep the existing G1 h.264 decode when booted on a old DT with
> > > > the old VPU description and no blk-ctrl, so we don't regress
> > > > functionality when a new kernel is booted with a old DT.
> > > > 
> > > > New functionality with the G2 can depend on the new VPU binding and the
> > > > blk-ctrl driver.
> > > 
> > > How does that work when both the original VPU and the blk-ctrl attempt
> > > to manipulate the reset and clock lines?  The original binding for the
> > > vpu was assigned:
> > > 
> > > reg = <0x38300000 0x10000>,
> > >           <0x38310000 0x10000>,
> > >           <0x38320000 0x10000>;
> > > reg-names = "g1", "g2", "ctrl";
> > > 
> > > If G2 is going to run from 38310000 and vpu-blk-ctrl run from
> > > 38320000, they'll collide.
> > > 
> > It's not going to work, but it also doesn't have to. Either you have a
> > old DT where the VPU driver will poke the blk-ctrl registers, but no
> > blk-ctrl driver, or you have a new DT where the VPU driver leaves the
> > blk-ctrl region alone and the blk-ctrl driver needs to handle it.
> > 
> > Just don't support mixing the old VPU DT binding with the new blk-ctrl
> > way of doing things. The only thing that needs to keep working is a
> > unchannged old DT, where the VPU uses the old binding, but no blk-ctrl
> > is present as a separate node.
> 
> I think I understand.  I'll leave the old code for the old binding in
> the driver and add the new code for the new bindings with blk-ctrl.
> Once the device tree is migrated to the new bindings, the old one
> becomes harmless, but still works with old device trees lacking the
> blk-ctrl.  That makes sense.  In my head, I wasn't thinking about
> mixing older device trees.

Exactly. While most people don't use it this way, the kernel and DT are
supposed to be independent, e.g. the DTB could be included in the
device firmware, while the kernel could be updated via a distro.

To not break this use-case without a good reason, new kernels should
try to not regress functionality with a existing binary DT. We can
mandate DT updates for new functionality (like being able to use the G2
core only with the new blk-ctrl + split VPU binding), but we shouldn't
break existing features if there isn't a very good reason to do so.
Keeping the bit of code in the VPU driver to keep the old G1 only
decoding working with a new kernel isn't much of a burden IMHO, so we
should try to keep it alive.

Regards,
Lucas

[RFC V2 6/6] arm64: dts: imx8mq: Split i.MX8MQ G1 and G2 with vpu-blk-ctrl

[Adam Ford]

With the Hantro G1 and G2 now setup to run independently, update
the device tree to enable it again.  This requires the vpu-blk-ctrl
node to be configured, and the clock-parents and clock-rates for
the various VPU's to be moved into the pgc_vpu because they cannot
get re-parented once enabled, and the pgc_vpu is the highest
in the chain.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 972766b67a15..3ed2644bd500 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -711,7 +711,21 @@ pgc_gpu: power-domain@5 {
 					pgc_vpu: power-domain@6 {
 						#power-domain-cells = <0>;
 						reg = <IMX8M_POWER_DOMAIN_VPU>;
-						clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
+						clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>,
+							 <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
+							 <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
+						assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,
+								  <&clk IMX8MQ_CLK_VPU_G2>,
+								  <&clk IMX8MQ_CLK_VPU_BUS>,
+								  <&clk IMX8MQ_VPU_PLL_BYPASS>;
+						assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>,
+									 <&clk IMX8MQ_VPU_PLL_OUT>,
+									 <&clk IMX8MQ_SYS1_PLL_800M>,
+									 <&clk IMX8MQ_VPU_PLL>;
+						assigned-clock-rates = <600000000>,
+								       <600000000>,
+								       <800000000>,
+								       <0>;
 					};
 
 					pgc_disp: power-domain@7 {
@@ -1432,30 +1446,37 @@ usb3_phy1: usb-phy@382f0040 {
 			status = "disabled";
 		};
 
-		vpu: video-codec@38300000 {
-			compatible = "nxp,imx8mq-vpu";
-			reg = <0x38300000 0x10000>,
-			      <0x38310000 0x10000>,
-			      <0x38320000 0x10000>;
-			reg-names = "g1", "g2", "ctrl";
-			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-names = "g1", "g2";
+		vpu_g1: video-codec@38300000 {
+			compatible = "nxp,imx8mq-vpu-g1";
+			reg = <0x38300000 0x10000>;
+			reg-names = "g1";
+			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "g1";
+			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>;
+			clock-names = "g1";
+			power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>;
+		};
+
+		vpu_g2: video-codec@38310000 {
+			compatible = "nxp,imx8mq-vpu-g2";
+			reg = <0x38310000 0x10000>;
+			reg-names = "g2";
+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "g2";
+			clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
+			clock-names = "g2";
+			power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G2>;
+		};
+
+		vpu_blk_ctrl: blk-ctrl@38320000 {
+			compatible = "fsl,imx8mq-vpu-blk-ctrl";
+			reg = <0x38320000 0x100>;
+			power-domains = <&pgc_vpu>, <&pgc_vpu>, <&pgc_vpu>;
+			power-domain-names = "bus", "g1", "g2";
 			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
-				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
-				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
-			clock-names = "g1", "g2", "bus";
-			assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,
-					  <&clk IMX8MQ_CLK_VPU_G2>,
-					  <&clk IMX8MQ_CLK_VPU_BUS>,
-					  <&clk IMX8MQ_VPU_PLL_BYPASS>;
-			assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>,
-						 <&clk IMX8MQ_VPU_PLL_OUT>,
-						 <&clk IMX8MQ_SYS1_PLL_800M>,
-						 <&clk IMX8MQ_VPU_PLL>;
-			assigned-clock-rates = <600000000>, <600000000>,
-					       <800000000>, <0>;
-			power-domains = <&pgc_vpu>;
+				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>;
+			clock-names = "g1", "g2";
+			#power-domain-cells = <1>;
 		};
 
 		pcie0: pcie@33800000 {
-- 
2.32.0

Re: [RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[Benjamin Gaignard]

Le 07/12/2021 à 02:54, Adam Ford a écrit :

> Currently, the VPU in the i.MQ8MQ is appearing as one codec, but in
> reality, it's two IP blocks called G1 and G2.  There is initialization
> code in VPU code to pull some clocks, resets and other features which
> has been integrated into the vpu-blk-ctrl for the i.MX8M Mini and a
> similar method can be used to make the VPU codec's operate as
> stand-alone cores without having to know the details of each other
> or the quirks unique to the i.MX8MQ, so the remaining code can be
> left more generic.
>
> This series was started by Lucas Stach with one by Benjamin Gaignard.
> Most patches have been modified slightly by me.  It's in an RFC state
> because I wasn't sure how to best handle the signatures and wasn't sure
> if I could base it off the branch I did.
>
> Since the g-streamer and media trees are in a constant state of
> change, this series is based on
>
> git://linuxtv.org/hverkuil/media_tree.git for-v5.17e
>
> The downstream code from NXP shows the G1 and G2 clocks running
> at 600MHz, but between the TRM and the datasheet, there is some
> discrepancy.  Because the NXP reference code used 600MHz, that is
> what was chosen here.  Users who need to adjust their G1 and G2
> clocks can do so in their board files.

Hi Adam,

Thanks for your patches, I have been able to reproduce VP9 results on my side (Fluster 147/303).
In past I have notice spurious errors when using 600MHz clock on HEVC decode but not with 300MHz.

Regards,
Benjamin

>
> Fluster Results:
>
> ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> Ran 90/135 tests successfully               in 61.966 secs
>
> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> Ran 55/61 tests successfully               in 7.660 secs
>
>
> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> Ran 144/303 tests successfully               in 162.665 secs
>
> Changes log:
>
> V2:  Make vpu-blk-ctrl enable G2 clock when enabling fuses.
>       Remove syscon from device tree and binding example
>       Added modified nxp,imx8mq-vpu.yaml from Benjamin Gaignard
>
> Adam Ford (2):
>    media: hantro: split i.MX8MQ G1 and G2 code
>    arm64: dts: imx8mq: Split i.MX8MQ G1 and G2 with vpu-blk-ctrl
>
> Benjamin Gaignard (1):
>    dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support
>
> Lucas Stach (3):
>    dt-bindings: power: imx8mq: add defines for VPU blk-ctrl domains
>    dt-bindings: soc: add binding for i.MX8MQ VPU blk-ctrl
>    soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl
>
>   .../bindings/media/nxp,imx8mq-vpu.yaml        |  58 +++++----
>   .../soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml      |  71 +++++++++++
>   arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  69 ++++++----
>   drivers/soc/imx/imx8m-blk-ctrl.c              |  68 +++++++++-
>   drivers/staging/media/hantro/hantro_drv.c     |   4 +-
>   drivers/staging/media/hantro/hantro_hw.h      |   2 +-
>   drivers/staging/media/hantro/imx8m_vpu_hw.c   | 119 +++---------------
>   include/dt-bindings/power/imx8mq-power.h      |   3 +
>   8 files changed, 237 insertions(+), 157 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml
>
>
> base-commit: d1888b0bfd2ddef2e8a81505ffa200b92cc32e0c

Re: [RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[Lucas Stach]

Am Mittwoch, dem 08.12.2021 um 10:32 +0100 schrieb Benjamin Gaignard:
> Le 07/12/2021 à 02:54, Adam Ford a écrit :
> 
> > Currently, the VPU in the i.MQ8MQ is appearing as one codec, but in
> > reality, it's two IP blocks called G1 and G2.  There is initialization
> > code in VPU code to pull some clocks, resets and other features which
> > has been integrated into the vpu-blk-ctrl for the i.MX8M Mini and a
> > similar method can be used to make the VPU codec's operate as
> > stand-alone cores without having to know the details of each other
> > or the quirks unique to the i.MX8MQ, so the remaining code can be
> > left more generic.
> > 
> > This series was started by Lucas Stach with one by Benjamin Gaignard.
> > Most patches have been modified slightly by me.  It's in an RFC state
> > because I wasn't sure how to best handle the signatures and wasn't sure
> > if I could base it off the branch I did.
> > 
> > Since the g-streamer and media trees are in a constant state of
> > change, this series is based on
> > 
> > git://linuxtv.org/hverkuil/media_tree.git for-v5.17e
> > 
> > The downstream code from NXP shows the G1 and G2 clocks running
> > at 600MHz, but between the TRM and the datasheet, there is some
> > discrepancy.  Because the NXP reference code used 600MHz, that is
> > what was chosen here.  Users who need to adjust their G1 and G2
> > clocks can do so in their board files.
> 
> Hi Adam,
> 
> Thanks for your patches, I have been able to reproduce VP9 results on my side (Fluster 147/303).
> In past I have notice spurious errors when using 600MHz clock on HEVC decode but not with 300MHz.

The max supported G2 clock frequency is 660MHz but needs a higher
voltage. The maximum supported  frequency at the default 0.9V is
550MHz. We should not configure the clocks for the higher than that, as
long as there is no support in the VPU driver to scale the voltage
along with the frequency. Same as with the GPU we should stick to base
frequency levels for the nominal operating mode.

Regards,
Lucas

> 
> Regards,
> Benjamin
> 
> > 
> > Fluster Results:
> > 
> > ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> > Ran 90/135 tests successfully               in 61.966 secs
> > 
> > ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> > Ran 55/61 tests successfully               in 7.660 secs
> > 
> > 
> > ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> > Ran 144/303 tests successfully               in 162.665 secs
> > 
> > Changes log:
> > 
> > V2:  Make vpu-blk-ctrl enable G2 clock when enabling fuses.
> >       Remove syscon from device tree and binding example
> >       Added modified nxp,imx8mq-vpu.yaml from Benjamin Gaignard
> > 
> > Adam Ford (2):
> >    media: hantro: split i.MX8MQ G1 and G2 code
> >    arm64: dts: imx8mq: Split i.MX8MQ G1 and G2 with vpu-blk-ctrl
> > 
> > Benjamin Gaignard (1):
> >    dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support
> > 
> > Lucas Stach (3):
> >    dt-bindings: power: imx8mq: add defines for VPU blk-ctrl domains
> >    dt-bindings: soc: add binding for i.MX8MQ VPU blk-ctrl
> >    soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl
> > 
> >   .../bindings/media/nxp,imx8mq-vpu.yaml        |  58 +++++----
> >   .../soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml      |  71 +++++++++++
> >   arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  69 ++++++----
> >   drivers/soc/imx/imx8m-blk-ctrl.c              |  68 +++++++++-
> >   drivers/staging/media/hantro/hantro_drv.c     |   4 +-
> >   drivers/staging/media/hantro/hantro_hw.h      |   2 +-
> >   drivers/staging/media/hantro/imx8m_vpu_hw.c   | 119 +++---------------
> >   include/dt-bindings/power/imx8mq-power.h      |   3 +
> >   8 files changed, 237 insertions(+), 157 deletions(-)
> >   create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml
> > 
> > 
> > base-commit: d1888b0bfd2ddef2e8a81505ffa200b92cc32e0c

Re: [RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[Benjamin Gaignard]

Le 08/12/2021 à 11:32, Lucas Stach a écrit :
> Am Mittwoch, dem 08.12.2021 um 10:32 +0100 schrieb Benjamin Gaignard:
>> Le 07/12/2021 à 02:54, Adam Ford a écrit :
>>
>>> Currently, the VPU in the i.MQ8MQ is appearing as one codec, but in
>>> reality, it's two IP blocks called G1 and G2.  There is initialization
>>> code in VPU code to pull some clocks, resets and other features which
>>> has been integrated into the vpu-blk-ctrl for the i.MX8M Mini and a
>>> similar method can be used to make the VPU codec's operate as
>>> stand-alone cores without having to know the details of each other
>>> or the quirks unique to the i.MX8MQ, so the remaining code can be
>>> left more generic.
>>>
>>> This series was started by Lucas Stach with one by Benjamin Gaignard.
>>> Most patches have been modified slightly by me.  It's in an RFC state
>>> because I wasn't sure how to best handle the signatures and wasn't sure
>>> if I could base it off the branch I did.
>>>
>>> Since the g-streamer and media trees are in a constant state of
>>> change, this series is based on
>>>
>>> git://linuxtv.org/hverkuil/media_tree.git for-v5.17e
>>>
>>> The downstream code from NXP shows the G1 and G2 clocks running
>>> at 600MHz, but between the TRM and the datasheet, there is some
>>> discrepancy.  Because the NXP reference code used 600MHz, that is
>>> what was chosen here.  Users who need to adjust their G1 and G2
>>> clocks can do so in their board files.
>> Hi Adam,
>>
>> Thanks for your patches, I have been able to reproduce VP9 results on my side (Fluster 147/303).
>> In past I have notice spurious errors when using 600MHz clock on HEVC decode but not with 300MHz.

The results for Fluster HEVC are 77/147 so no regressions :-)

Regards,
Benjamin

>> The max supported G2 clock frequency is 660MHz but needs a higher
>> voltage. The maximum supported  frequency at the default 0.9V is
>> 550MHz. We should not configure the clocks for the higher than that, as
>> long as there is no support in the VPU driver to scale the voltage
>> along with the frequency. Same as with the GPU we should stick to base
>> frequency levels for the nominal operating mode.
>>
>> Regards,
>> Lucas
>>
>> Regards,
>> Benjamin
>>
>>> Fluster Results:
>>>
>>> ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
>>> Ran 90/135 tests successfully               in 61.966 secs
>>>
>>> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
>>> Ran 55/61 tests successfully               in 7.660 secs
>>>
>>>
>>> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
>>> Ran 144/303 tests successfully               in 162.665 secs
>>>
>>> Changes log:
>>>
>>> V2:  Make vpu-blk-ctrl enable G2 clock when enabling fuses.
>>>        Remove syscon from device tree and binding example
>>>        Added modified nxp,imx8mq-vpu.yaml from Benjamin Gaignard
>>>
>>> Adam Ford (2):
>>>     media: hantro: split i.MX8MQ G1 and G2 code
>>>     arm64: dts: imx8mq: Split i.MX8MQ G1 and G2 with vpu-blk-ctrl
>>>
>>> Benjamin Gaignard (1):
>>>     dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support
>>>
>>> Lucas Stach (3):
>>>     dt-bindings: power: imx8mq: add defines for VPU blk-ctrl domains
>>>     dt-bindings: soc: add binding for i.MX8MQ VPU blk-ctrl
>>>     soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl
>>>
>>>    .../bindings/media/nxp,imx8mq-vpu.yaml        |  58 +++++----
>>>    .../soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml      |  71 +++++++++++
>>>    arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  69 ++++++----
>>>    drivers/soc/imx/imx8m-blk-ctrl.c              |  68 +++++++++-
>>>    drivers/staging/media/hantro/hantro_drv.c     |   4 +-
>>>    drivers/staging/media/hantro/hantro_hw.h      |   2 +-
>>>    drivers/staging/media/hantro/imx8m_vpu_hw.c   | 119 +++---------------
>>>    include/dt-bindings/power/imx8mq-power.h      |   3 +
>>>    8 files changed, 237 insertions(+), 157 deletions(-)
>>>    create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml
>>>
>>>
>>> base-commit: d1888b0bfd2ddef2e8a81505ffa200b92cc32e0c
>

Re: [RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[Adam Ford]

On Wed, Dec 8, 2021 at 7:36 AM Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
>
>
> Le 08/12/2021 à 11:32, Lucas Stach a écrit :
> > Am Mittwoch, dem 08.12.2021 um 10:32 +0100 schrieb Benjamin Gaignard:
> >> Le 07/12/2021 à 02:54, Adam Ford a écrit :
> >>
> >>> Currently, the VPU in the i.MQ8MQ is appearing as one codec, but in
> >>> reality, it's two IP blocks called G1 and G2.  There is initialization
> >>> code in VPU code to pull some clocks, resets and other features which
> >>> has been integrated into the vpu-blk-ctrl for the i.MX8M Mini and a
> >>> similar method can be used to make the VPU codec's operate as
> >>> stand-alone cores without having to know the details of each other
> >>> or the quirks unique to the i.MX8MQ, so the remaining code can be
> >>> left more generic.
> >>>
> >>> This series was started by Lucas Stach with one by Benjamin Gaignard.
> >>> Most patches have been modified slightly by me.  It's in an RFC state
> >>> because I wasn't sure how to best handle the signatures and wasn't sure
> >>> if I could base it off the branch I did.
> >>>
> >>> Since the g-streamer and media trees are in a constant state of
> >>> change, this series is based on
> >>>
> >>> git://linuxtv.org/hverkuil/media_tree.git for-v5.17e
> >>>
> >>> The downstream code from NXP shows the G1 and G2 clocks running
> >>> at 600MHz, but between the TRM and the datasheet, there is some
> >>> discrepancy.  Because the NXP reference code used 600MHz, that is
> >>> what was chosen here.  Users who need to adjust their G1 and G2
> >>> clocks can do so in their board files.
> >> Hi Adam,
> >>
> >> Thanks for your patches, I have been able to reproduce VP9 results on my side (Fluster 147/303).
> >> In past I have notice spurious errors when using 600MHz clock on HEVC decode but not with 300MHz.
>
> The results for Fluster HEVC are 77/147 so no regressions :-)
>
> Regards,
> Benjamin
>
> >> The max supported G2 clock frequency is 660MHz but needs a higher
> >> voltage. The maximum supported  frequency at the default 0.9V is
> >> 550MHz. We should not configure the clocks for the higher than that, as
> >> long as there is no support in the VPU driver to scale the voltage
> >> along with the frequency. Same as with the GPU we should stick to base
> >> frequency levels for the nominal operating mode.

Lucas,

After reviewing the comments from Benjamin, I re-ran the VP9 tests
with the G2 running at 300MHz,and the number of passing VP9 tests
increased to 148 from 144 with an increase of time to 250.502 secs
from 162.665 secs.

While the datasheet reads that the G2 can run faster, the i.MX 8M
Dual/8M QuadLite/8M Quad Applications Processors Reference Manual,
Rev. 3.1, 06/2021, table 5-1  shows the VPU_G2_CLK_ROOT has a max
clock of 300MHz.  I might be inclined to agree with Benjamin on the
300MHz and let people who want to push their hardware overwrite the
default clocks since it increases functionality.

I wonder if someone from NXP can comment

adam

> >>
> >> Regards,
> >> Lucas
> >>
> >> Regards,
> >> Benjamin
> >>
> >>> Fluster Results:
> >>>
> >>> ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
> >>> Ran 90/135 tests successfully               in 61.966 secs
> >>>
> >>> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
> >>> Ran 55/61 tests successfully               in 7.660 secs
> >>>
> >>>
> >>> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
> >>> Ran 144/303 tests successfully               in 162.665 secs
> >>>
> >>> Changes log:
> >>>
> >>> V2:  Make vpu-blk-ctrl enable G2 clock when enabling fuses.
> >>>        Remove syscon from device tree and binding example
> >>>        Added modified nxp,imx8mq-vpu.yaml from Benjamin Gaignard
> >>>
> >>> Adam Ford (2):
> >>>     media: hantro: split i.MX8MQ G1 and G2 code
> >>>     arm64: dts: imx8mq: Split i.MX8MQ G1 and G2 with vpu-blk-ctrl
> >>>
> >>> Benjamin Gaignard (1):
> >>>     dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support
> >>>
> >>> Lucas Stach (3):
> >>>     dt-bindings: power: imx8mq: add defines for VPU blk-ctrl domains
> >>>     dt-bindings: soc: add binding for i.MX8MQ VPU blk-ctrl
> >>>     soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl
> >>>
> >>>    .../bindings/media/nxp,imx8mq-vpu.yaml        |  58 +++++----
> >>>    .../soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml      |  71 +++++++++++
> >>>    arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  69 ++++++----
> >>>    drivers/soc/imx/imx8m-blk-ctrl.c              |  68 +++++++++-
> >>>    drivers/staging/media/hantro/hantro_drv.c     |   4 +-
> >>>    drivers/staging/media/hantro/hantro_hw.h      |   2 +-
> >>>    drivers/staging/media/hantro/imx8m_vpu_hw.c   | 119 +++---------------
> >>>    include/dt-bindings/power/imx8mq-power.h      |   3 +
> >>>    8 files changed, 237 insertions(+), 157 deletions(-)
> >>>    create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml
> >>>
> >>>
> >>> base-commit: d1888b0bfd2ddef2e8a81505ffa200b92cc32e0c
> >

Re: [RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[Lucas Stach]

Am Mittwoch, dem 08.12.2021 um 09:13 -0600 schrieb Adam Ford:
> On Wed, Dec 8, 2021 at 7:36 AM Benjamin Gaignard
> <benjamin.gaignard@collabora.com> wrote:
> > 
> > 
> > Le 08/12/2021 à 11:32, Lucas Stach a écrit :
> > > Am Mittwoch, dem 08.12.2021 um 10:32 +0100 schrieb Benjamin Gaignard:
> > > > Le 07/12/2021 à 02:54, Adam Ford a écrit :
> > > > 
> > > > > Currently, the VPU in the i.MQ8MQ is appearing as one codec, but in
> > > > > reality, it's two IP blocks called G1 and G2.  There is initialization
> > > > > code in VPU code to pull some clocks, resets and other features which
> > > > > has been integrated into the vpu-blk-ctrl for the i.MX8M Mini and a
> > > > > similar method can be used to make the VPU codec's operate as
> > > > > stand-alone cores without having to know the details of each other
> > > > > or the quirks unique to the i.MX8MQ, so the remaining code can be
> > > > > left more generic.
> > > > > 
> > > > > This series was started by Lucas Stach with one by Benjamin Gaignard.
> > > > > Most patches have been modified slightly by me.  It's in an RFC state
> > > > > because I wasn't sure how to best handle the signatures and wasn't sure
> > > > > if I could base it off the branch I did.
> > > > > 
> > > > > Since the g-streamer and media trees are in a constant state of
> > > > > change, this series is based on
> > > > > 
> > > > > git://linuxtv.org/hverkuil/media_tree.git for-v5.17e
> > > > > 
> > > > > The downstream code from NXP shows the G1 and G2 clocks running
> > > > > at 600MHz, but between the TRM and the datasheet, there is some
> > > > > discrepancy.  Because the NXP reference code used 600MHz, that is
> > > > > what was chosen here.  Users who need to adjust their G1 and G2
> > > > > clocks can do so in their board files.
> > > > Hi Adam,
> > > > 
> > > > Thanks for your patches, I have been able to reproduce VP9 results on my side (Fluster 147/303).
> > > > In past I have notice spurious errors when using 600MHz clock on HEVC decode but not with 300MHz.
> > 
> > The results for Fluster HEVC are 77/147 so no regressions :-)
> > 
> > Regards,
> > Benjamin
> > 
> > > > The max supported G2 clock frequency is 660MHz but needs a higher
> > > > voltage. The maximum supported  frequency at the default 0.9V is
> > > > 550MHz. We should not configure the clocks for the higher than that, as
> > > > long as there is no support in the VPU driver to scale the voltage
> > > > along with the frequency. Same as with the GPU we should stick to base
> > > > frequency levels for the nominal operating mode.
> 
> Lucas,
> 
> After reviewing the comments from Benjamin, I re-ran the VP9 tests
> with the G2 running at 300MHz,and the number of passing VP9 tests
> increased to 148 from 144 with an increase of time to 250.502 secs
> from 162.665 secs.
> 
> While the datasheet reads that the G2 can run faster, the i.MX 8M
> Dual/8M QuadLite/8M Quad Applications Processors Reference Manual,
> Rev. 3.1, 06/2021, table 5-1  shows the VPU_G2_CLK_ROOT has a max
> clock of 300MHz.  I might be inclined to agree with Benjamin on the
> 300MHz and let people who want to push their hardware overwrite the
> default clocks since it increases functionality.
> 
I'm quite surprised that the G2 codec should have such a much lower max
frequency compared with the G1. While the table from the RM hasn't been
obviously incorrect for any other clock, I'm still inclined to believe
the frequencies stated in the datasheet.

> I wonder if someone from NXP can comment
> 
Yea, not sure how to proceed here. 300MHz is the safe bet, but we are
leaving quite some performance on the table if the datasheet is
correct. Without some help from NXP it's probably hard to validate 
which max frequency statement is correct. It doesn't really help that
the NXP downstream kernel seems to drive the G2 at 600MHz, which is
neither the 550MHz nominal mode max, nor the 660MHz overdrive mode max.

Regards,
Lucas

Re: [RFC V2 0/6] media: Hantro: Split iMX8MQ VPU into G1 and G2 with blk-ctrl support

[Chris Healy]

On Wed, Dec 8, 2021 at 7:39 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Mittwoch, dem 08.12.2021 um 09:13 -0600 schrieb Adam Ford:
> > On Wed, Dec 8, 2021 at 7:36 AM Benjamin Gaignard
> > <benjamin.gaignard@collabora.com> wrote:
> > >
> > >
> > > Le 08/12/2021 à 11:32, Lucas Stach a écrit :
> > > > Am Mittwoch, dem 08.12.2021 um 10:32 +0100 schrieb Benjamin Gaignard:
> > > > > Le 07/12/2021 à 02:54, Adam Ford a écrit :
> > > > >
> > > > > > Currently, the VPU in the i.MQ8MQ is appearing as one codec, but in
> > > > > > reality, it's two IP blocks called G1 and G2.  There is initialization
> > > > > > code in VPU code to pull some clocks, resets and other features which
> > > > > > has been integrated into the vpu-blk-ctrl for the i.MX8M Mini and a
> > > > > > similar method can be used to make the VPU codec's operate as
> > > > > > stand-alone cores without having to know the details of each other
> > > > > > or the quirks unique to the i.MX8MQ, so the remaining code can be
> > > > > > left more generic.
> > > > > >
> > > > > > This series was started by Lucas Stach with one by Benjamin Gaignard.
> > > > > > Most patches have been modified slightly by me.  It's in an RFC state
> > > > > > because I wasn't sure how to best handle the signatures and wasn't sure
> > > > > > if I could base it off the branch I did.
> > > > > >
> > > > > > Since the g-streamer and media trees are in a constant state of
> > > > > > change, this series is based on
> > > > > >
> > > > > > git://linuxtv.org/hverkuil/media_tree.git for-v5.17e
> > > > > >
> > > > > > The downstream code from NXP shows the G1 and G2 clocks running
> > > > > > at 600MHz, but between the TRM and the datasheet, there is some
> > > > > > discrepancy.  Because the NXP reference code used 600MHz, that is
> > > > > > what was chosen here.  Users who need to adjust their G1 and G2
> > > > > > clocks can do so in their board files.
> > > > > Hi Adam,
> > > > >
> > > > > Thanks for your patches, I have been able to reproduce VP9 results on my side (Fluster 147/303).
> > > > > In past I have notice spurious errors when using 600MHz clock on HEVC decode but not with 300MHz.
> > >
> > > The results for Fluster HEVC are 77/147 so no regressions :-)
> > >
> > > Regards,
> > > Benjamin
> > >
> > > > > The max supported G2 clock frequency is 660MHz but needs a higher
> > > > > voltage. The maximum supported  frequency at the default 0.9V is
> > > > > 550MHz. We should not configure the clocks for the higher than that, as
> > > > > long as there is no support in the VPU driver to scale the voltage
> > > > > along with the frequency. Same as with the GPU we should stick to base
> > > > > frequency levels for the nominal operating mode.
> >
> > Lucas,
> >
> > After reviewing the comments from Benjamin, I re-ran the VP9 tests
> > with the G2 running at 300MHz,and the number of passing VP9 tests
> > increased to 148 from 144 with an increase of time to 250.502 secs
> > from 162.665 secs.
> >
> > While the datasheet reads that the G2 can run faster, the i.MX 8M
> > Dual/8M QuadLite/8M Quad Applications Processors Reference Manual,
> > Rev. 3.1, 06/2021, table 5-1  shows the VPU_G2_CLK_ROOT has a max
> > clock of 300MHz.  I might be inclined to agree with Benjamin on the
> > 300MHz and let people who want to push their hardware overwrite the
> > default clocks since it increases functionality.
> >
> I'm quite surprised that the G2 codec should have such a much lower max
> frequency compared with the G1. While the table from the RM hasn't been
> obviously incorrect for any other clock, I'm still inclined to believe
> the frequencies stated in the datasheet.
>
> > I wonder if someone from NXP can comment
> >
> Yea, not sure how to proceed here. 300MHz is the safe bet, but we are
> leaving quite some performance on the table if the datasheet is
> correct. Without some help from NXP it's probably hard to validate
> which max frequency statement is correct. It doesn't really help that
> the NXP downstream kernel seems to drive the G2 at 600MHz, which is
> neither the 550MHz nominal mode max, nor the 660MHz overdrive mode max.

According to the NXP i.MX8MQ docs, "The G2 decoder conforms to the
HEVC Main/Main 10 profiles and can decode streams up to level 5.1."

If I'm reading the levels correctly, level 5.1 = 4,096×2,160@60.0.  If
the decoder can achieve this throughput at 300MHz, then there should
be no reason to go higher?  My speculation though is that when run at
300MHz, the decoder is not able to decode this fast.

Adam, can you determine the throughput at the different frequencies to
see if 300MHz can support 4K60 decode?

>
> Regards,
> Lucas
>