Both the i.MX8MQ and i.MX8MM have G1 and G2 decoders.
The two decoders are similar, but the imx8mm lacks the
post-processor, so they will have distinct compatible flags.
Splitting the i.MX8MQ VPU into G1 and G2 makes it easier to
control them independently since the TRM of both the i.MX8MQ and
i.MX8MM list them as distinct IP blocks. With them being split,
the power-domain can shift to the vpu-blk-ctrl which is available
on both i.MX8MQ and i.MX8MM but some of bits are different, so
they'll have separate device tree bindings.
Lastly, with the G1 and G2 operational, enable the i.MX8MM.
On the i.MX8MM, the clock speed of 600MHz was chosen to match
the default of the kernel repo from NXP and can be overwritten
by board files for anyone who under/over volts the power rail.
The repo used as the starting point was:
git://linuxtv.org/hverkuil/media_tree.git for-v5.17e
This media hantro group has been especially helpful in helping
me get up to speed on how it works.
Fluster was run on the i.MX8M Mini at 600 MHz:
./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0
Ran 55/61 tests successfully in 8.299 secs
./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0
Ran 90/135 tests successfully in 71.200 secs
./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0
Ran 139/303 tests successfully in 218.079 secs
Adam Ford (7):
dt-bindings: media: nxp,imx8mq-vpu: Support split G1 and G2 nodes with
vpu-blk-ctrl
media: hantro: Allow i.MX8MQ G1 and G2 to run independently
arm64: dts: imx8mq: Enable both G1 and G2 VPU's with vpu-blk-ctrl
arm64: dts: imx8mm: Fix VPU Hanging
dt-bindings: media: nxp,imx8mq-vpu: Enable support for i.MX8M Mini
media: hantro: Add support for i.MX8MM
arm64: dts: imx8mm: Enable Hantro G1 and G2 video decoders
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 | 85 ++++++++++++++-----
.../soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml | 71 ++++++++++++++++
arch/arm64/boot/dts/freescale/imx8mm.dtsi | 29 ++++++-
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 69 +++++++++------
drivers/soc/imx/imx8m-blk-ctrl.c | 68 ++++++++++++++-
drivers/staging/media/hantro/hantro_drv.c | 3 +
drivers/staging/media/hantro/hantro_hw.h | 3 +
drivers/staging/media/hantro/imx8m_vpu_hw.c | 84 +++++++++++++++---
include/dt-bindings/power/imx8mq-power.h | 3 +
9 files changed, 357 insertions(+), 58 deletions(-)
create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml
base-commit: d1888b0bfd2ddef2e8a81505ffa200b92cc32e0c
--
2.32.0
From: Lucas Stach <[email protected]>
This adds the defines for the power domains provided by the VPU
blk-ctrl on the i.MX8MQ.
Signed-off-by: Lucas Stach <[email protected]>
---
include/dt-bindings/power/imx8mq-power.h | 3 +++
1 file changed, 3 insertions(+)
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
From: Lucas Stach <[email protected]>
This adds the DT binding for the i.MX8MQ VPU blk-ctrl.
Signed-off-by: Lucas Stach <[email protected]>
Signed-off-by: Adam Ford <[email protected]>
---
.../soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml | 71 +++++++++++++++++++
1 file changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml
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 <[email protected]>
+
+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
The G1 and G2 are separate decoder blocks that are enabled by the
vpu-blk-ctrl power-domain controller, which now has a proper driver.
Update the bindings to support separate nodes for the G1 and G2
decoders using the proper driver or the older unified node with
the legacy controls.
To be compatible with older DT the driver, mark certain items as
deprecated and retain the backwards compatible example.
Signed-off-by: Adam Ford <[email protected]>
---
.../bindings/media/nxp,imx8mq-vpu.yaml | 83 ++++++++++++++-----
1 file changed, 64 insertions(+), 19 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
index 762be3f96ce9..eeb7bd6281f9 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
@@ -15,29 +15,39 @@ description:
properties:
compatible:
- const: nxp,imx8mq-vpu
+ oneOf:
+ - const: nxp,imx8mq-vpu
+ deprecated: true
+ - const: nxp,imx8mq-vpu-g1
+ - const: nxp,imx8mq-vpu-g2
reg:
+ minItems: 1
maxItems: 3
reg-names:
+ minItems: 1
items:
- const: g1
- const: g2
- const: ctrl
interrupts:
+ minItems: 1
maxItems: 2
interrupt-names:
+ minItems: 1
items:
- const: g1
- const: g2
clocks:
+ minItems: 1
maxItems: 3
clock-names:
+ minItems: 1
items:
- const: g1
- const: g2
@@ -58,22 +68,57 @@ required:
additionalProperties: false
examples:
+ # Device node example backwards compatibility
- |
- #include <dt-bindings/clock/imx8mq-clock.h>
- #include <dt-bindings/interrupt-controller/arm-gic.h>
-
- 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";
- 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>;
- };
+ #include <dt-bindings/clock/imx8mq-clock.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ 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";
+ 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>;
+ };
+
+ # VPU G1 with vpu-blk-ctrl
+ - |
+ #include <dt-bindings/clock/imx8mq-clock.h>
+ #include <dt-bindings/power/imx8mq-power.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ 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 with vpu-blk-ctrl
+ - |
+ #include <dt-bindings/clock/imx8mq-clock.h>
+ #include <dt-bindings/power/imx8mq-power.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ 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>;
+ };
--
2.32.0
From: Lucas Stach <[email protected]>
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 <[email protected]>
Signed-off-by: Adam Ford <[email protected]>
---
drivers/soc/imx/imx8m-blk-ctrl.c | 68 +++++++++++++++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)
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
With the Hantro G1 and G2 now setup to run independently, update
the device tree to allow both to operate. This requires the
vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs
certain clock enabled to handle the gating of the G1 and G2
fuses, 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 <[email protected]>
---
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 69 +++++++++++++++--------
1 file changed, 45 insertions(+), 24 deletions(-)
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
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
while still retaining backwards compatibility with older device
trees for those using G1.
Signed-off-by: Adam Ford <[email protected]>
---
drivers/staging/media/hantro/hantro_drv.c | 1 +
drivers/staging/media/hantro/hantro_hw.h | 1 +
drivers/staging/media/hantro/imx8m_vpu_hw.c | 56 ++++++++++++++++-----
3 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index ab2467998d29..e7afda388ee5 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -609,6 +609,7 @@ static const struct of_device_id of_hantro_match[] = {
#endif
#ifdef CONFIG_VIDEO_HANTRO_IMX8M
{ .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_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
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index cff817ca8d22..96b14b43a4af 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -299,6 +299,7 @@ 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;
diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
index 1a43f6fceef9..e0af4b93d3c6 100644
--- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
+++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
@@ -223,13 +223,6 @@ static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
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.
*/
@@ -255,17 +248,33 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
},
};
+static const struct hantro_codec_ops imx8mq_vpu_g1_codec_ops[] = {
+ [HANTRO_MODE_MPEG2_DEC] = {
+ .run = hantro_g1_mpeg2_dec_run,
+ .init = hantro_mpeg2_dec_init,
+ .exit = hantro_mpeg2_dec_exit,
+ },
+ [HANTRO_MODE_VP8_DEC] = {
+ .run = hantro_g1_vp8_dec_run,
+ .init = hantro_vp8_dec_init,
+ .exit = hantro_vp8_dec_exit,
+ },
+ [HANTRO_MODE_H264_DEC] = {
+ .run = hantro_g1_h264_dec_run,
+ .init = hantro_h264_dec_init,
+ .exit = hantro_h264_dec_exit,
+ },
+};
+
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,
},
@@ -285,6 +294,10 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
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 = {
.dec_fmts = imx8m_vpu_dec_fmts,
@@ -305,6 +318,23 @@ const struct hantro_variant imx8mq_vpu_variant = {
.num_regs = ARRAY_SIZE(imx8mq_reg_names)
};
+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,
+ .num_postproc_fmts = ARRAY_SIZE(imx8m_vpu_postproc_fmts),
+ .postproc_ops = &hantro_g1_postproc_ops,
+ .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
+ HANTRO_H264_DECODER,
+ .codec_ops = imx8mq_vpu_g1_codec_ops,
+ .irqs = imx8mq_irqs,
+ .num_irqs = ARRAY_SIZE(imx8mq_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 = {
.dec_offset = 0x0,
.dec_fmts = imx8m_vpu_g2_dec_fmts,
@@ -314,10 +344,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
The i.MX8M mini appears to have a similar G1 and G2 decoder but the
post-procesing isn't present. Add compatible flags to support
G1 and G2 without post-processing.
Signed-off-by: Adam Ford <[email protected]>
---
Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
index eeb7bd6281f9..fb6c2ab1b2dc 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
@@ -20,6 +20,8 @@ properties:
deprecated: true
- const: nxp,imx8mq-vpu-g1
- const: nxp,imx8mq-vpu-g2
+ - const: nxp, imx8mm-vpu-g1
+ - const: nxp, imx8mm-vpu-g2
reg:
minItems: 1
--
2.32.0
The vpumix power domain has a reset assigned to it, however
when used, it causes a system hang. Testing has shown that
it does not appear to be needed anywhere.
Fixes: d39d4bb15310 ("arm64: dts: imx8mm: add GPC node")
Signed-off-by: Adam Ford <[email protected]>
---
arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index c2f3f118f82e..f13d31ebfcbd 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -681,7 +681,6 @@ pgc_vpumix: power-domain@6 {
clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>;
assigned-clocks = <&clk IMX8MM_CLK_VPU_BUS>;
assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>;
- resets = <&src IMX8MQ_RESET_VPU_RESET>;
};
pgc_vpu_g1: power-domain@7 {
--
2.32.0
The i.MX8MM has a G1 and G2 video decoder, so add support in
the driver for it with the post-processing removed.
Signed-off-by: Adam Ford <[email protected]>
---
drivers/staging/media/hantro/hantro_drv.c | 2 ++
drivers/staging/media/hantro/hantro_hw.h | 2 ++
drivers/staging/media/hantro/imx8m_vpu_hw.c | 28 +++++++++++++++++++++
3 files changed, 32 insertions(+)
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index e7afda388ee5..118c4fa3d556 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -608,6 +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,imx8mm-vpu-g1", .data = &imx8mm_vpu_g1_variant, },
+ { .compatible = "nxp,imx8mm-vpu-g2", .data = &imx8mm_vpu_g2_variant },
{ .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
{ .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant },
{ .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 96b14b43a4af..6ae1aeed2e16 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -299,6 +299,8 @@ enum hantro_enc_fmt {
ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
};
+extern const struct hantro_variant imx8mm_vpu_g1_variant;
+extern const struct hantro_variant imx8mm_vpu_g2_variant;
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;
diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
index e0af4b93d3c6..65bb6f464dc9 100644
--- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
+++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
@@ -351,3 +351,31 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
.reg_names = imx8mq_g2_reg_names,
.num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
};
+
+const struct hantro_variant imx8mm_vpu_g1_variant = {
+ .dec_fmts = imx8m_vpu_dec_fmts,
+ .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
+ .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
+ HANTRO_H264_DECODER,
+ .codec_ops = imx8mq_vpu_g1_codec_ops,
+ .irqs = imx8mq_irqs,
+ .num_irqs = ARRAY_SIZE(imx8mq_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 imx8mm_vpu_g2_variant = {
+ .dec_offset = 0x0,
+ .dec_fmts = imx8m_vpu_g2_dec_fmts,
+ .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_g2_dec_fmts),
+ .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
+ .codec_ops = imx8mq_vpu_g2_codec_ops,
+ .irqs = imx8mq_g2_irqs,
+ .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
+ .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
There are two decoders on the i.MX8M Mini controlled by the
vpu-blk-ctrl. The G1 supports H264 and VP8 while the
G2 support HEVC and VP9.
Signed-off-by: Adam Ford <[email protected]>
---
arch/arm64/boot/dts/freescale/imx8mm.dtsi | 28 +++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index f13d31ebfcbd..4682f1f5238d 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -1196,6 +1196,28 @@ gpu_2d: gpu@38008000 {
power-domains = <&pgc_gpu>;
};
+ vpu_g1: video-codec@38300000 {
+ compatible = "nxp,imx8mm-vpu-g1";
+ reg = <0x38300000 0x10000>;
+ reg-names = "g1";
+ interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "g1";
+ clocks = <&clk IMX8MM_CLK_VPU_G1_ROOT>;
+ clock-names = "g1";
+ power-domains = <&vpu_blk_ctrl IMX8MM_VPUBLK_PD_G1>;
+ };
+
+ vpu_g2: video-codec@38310000 {
+ compatible = "nxp,imx8mm-vpu-g2";
+ reg = <0x38310000 0x10000>;
+ reg-names = "g2";
+ interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "g2";
+ clocks = <&clk IMX8MM_CLK_VPU_G2_ROOT>;
+ clock-names = "g2";
+ power-domains = <&vpu_blk_ctrl IMX8MM_VPUBLK_PD_G2>;
+ };
+
vpu_blk_ctrl: blk-ctrl@38330000 {
compatible = "fsl,imx8mm-vpu-blk-ctrl", "syscon";
reg = <0x38330000 0x100>;
@@ -1206,6 +1228,12 @@ vpu_blk_ctrl: blk-ctrl@38330000 {
<&clk IMX8MM_CLK_VPU_G2_ROOT>,
<&clk IMX8MM_CLK_VPU_H1_ROOT>;
clock-names = "g1", "g2", "h1";
+ assigned-clocks = <&clk IMX8MM_CLK_VPU_G1>,
+ <&clk IMX8MM_CLK_VPU_G2>;
+ assigned-clock-parents = <&clk IMX8MM_VPU_PLL_OUT>,
+ <&clk IMX8MM_VPU_PLL_OUT>;
+ assigned-clock-rates = <600000000>,
+ <600000000>;
#power-domain-cells = <1>;
};
--
2.32.0
On Wed, 08 Dec 2021 16:50:23 -0600, Adam Ford wrote:
> The G1 and G2 are separate decoder blocks that are enabled by the
> vpu-blk-ctrl power-domain controller, which now has a proper driver.
> Update the bindings to support separate nodes for the G1 and G2
> decoders using the proper driver or the older unified node with
> the legacy controls.
>
> To be compatible with older DT the driver, mark certain items as
> deprecated and retain the backwards compatible example.
>
> Signed-off-by: Adam Ford <[email protected]>
> ---
> .../bindings/media/nxp,imx8mq-vpu.yaml | 83 ++++++++++++++-----
> 1 file changed, 64 insertions(+), 19 deletions(-)
>
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:
Error: Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.example.dts:62.23-27 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/1565517
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.
Hi,
Thanks for the patch.
On Wed, Dec 08, 2021 at 04:50:23PM -0600, Adam Ford wrote:
> The G1 and G2 are separate decoder blocks that are enabled by the
> vpu-blk-ctrl power-domain controller, which now has a proper driver.
> Update the bindings to support separate nodes for the G1 and G2
> decoders using the proper driver or the older unified node with
> the legacy controls.
>
> To be compatible with older DT the driver, mark certain items as
> deprecated and retain the backwards compatible example.
>
> Signed-off-by: Adam Ford <[email protected]>
> ---
> .../bindings/media/nxp,imx8mq-vpu.yaml | 83 ++++++++++++++-----
> 1 file changed, 64 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> index 762be3f96ce9..eeb7bd6281f9 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> @@ -15,29 +15,39 @@ description:
>
> properties:
> compatible:
> - const: nxp,imx8mq-vpu
> + oneOf:
> + - const: nxp,imx8mq-vpu
> + deprecated: true
> + - const: nxp,imx8mq-vpu-g1
> + - const: nxp,imx8mq-vpu-g2
>
> reg:
> + minItems: 1
> maxItems: 3
Is it really useful to keep the deprecated binding nxp,imx8mq-vpu
as something supported by the binding file?
In other words, can we drop the deprecated binding from this file,
while keeping the support in the driver for legacy device-trees?
[..]
> +
> + # VPU G1 with vpu-blk-ctrl
> + - |
> + #include <dt-bindings/clock/imx8mq-clock.h>
> + #include <dt-bindings/power/imx8mq-power.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + 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";
reg-names, interrupt-names and clock-names should be removed
given for this device there's only one of each.
This will make the binding actually quite easier, but it also
means you need to make some changes to struct hantro_variant imx8mq_vpu_g1_variant
to make it work properly.
See Rob's feedback on the SAMA5 VPU binding:
https://yhbt.net/lore/all/[email protected]/
Also, take a look at drivers/staging/media/hantro/sama5d4_vdec_hw.c
for reference.
> + power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>;
> + };
> +
> + # VPU G2 with vpu-blk-ctrl
> + - |
> + #include <dt-bindings/clock/imx8mq-clock.h>
> + #include <dt-bindings/power/imx8mq-power.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + vpu_g2: video-codec@38310000 {
> + compatible = "nxp,imx8mq-vpu-g2";
> + reg = <0x38310000 0x10000>;
> + reg-names "g2";
And same here.
Thanks!
Ezequiel
On Thu, Dec 9, 2021 at 4:26 AM Ezequiel Garcia
<[email protected]> wrote:
>
> Hi,
>
> Thanks for the patch.
>
> On Wed, Dec 08, 2021 at 04:50:23PM -0600, Adam Ford wrote:
> > The G1 and G2 are separate decoder blocks that are enabled by the
> > vpu-blk-ctrl power-domain controller, which now has a proper driver.
> > Update the bindings to support separate nodes for the G1 and G2
> > decoders using the proper driver or the older unified node with
> > the legacy controls.
> >
> > To be compatible with older DT the driver, mark certain items as
> > deprecated and retain the backwards compatible example.
> >
> > Signed-off-by: Adam Ford <[email protected]>
> > ---
> > .../bindings/media/nxp,imx8mq-vpu.yaml | 83 ++++++++++++++-----
> > 1 file changed, 64 insertions(+), 19 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > index 762be3f96ce9..eeb7bd6281f9 100644
> > --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > @@ -15,29 +15,39 @@ description:
> >
> > properties:
> > compatible:
> > - const: nxp,imx8mq-vpu
> > + oneOf:
> > + - const: nxp,imx8mq-vpu
> > + deprecated: true
> > + - const: nxp,imx8mq-vpu-g1
> > + - const: nxp,imx8mq-vpu-g2
> >
> > reg:
> > + minItems: 1
> > maxItems: 3
>
> Is it really useful to keep the deprecated binding nxp,imx8mq-vpu
> as something supported by the binding file?
Since I was told that the driver needed to be backwards compatible, i
wanted to make sure that any attempts to build the old device tree
would not fail
>
> In other words, can we drop the deprecated binding from this file,
> while keeping the support in the driver for legacy device-trees?
I was trying to represent both the old driver binding and the new one
at the same time. I thought that's what I was told to do.
>
> [..]
> > +
> > + # VPU G1 with vpu-blk-ctrl
> > + - |
> > + #include <dt-bindings/clock/imx8mq-clock.h>
> > + #include <dt-bindings/power/imx8mq-power.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > + 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";
>
> reg-names, interrupt-names and clock-names should be removed
> given for this device there's only one of each.
>
I attempted to remove the reg-names, but it failed to enumerate for me
when I did that.
> This will make the binding actually quite easier, but it also
> means you need to make some changes to struct hantro_variant imx8mq_vpu_g1_variant
> to make it work properly.
>
> See Rob's feedback on the SAMA5 VPU binding:
>
> https://yhbt.net/lore/all/[email protected]/
>
> Also, take a look at drivers/staging/media/hantro/sama5d4_vdec_hw.c
> for reference.
I can try again using this as an example.
>
> > + power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>;
> > + };
> > +
> > + # VPU G2 with vpu-blk-ctrl
> > + - |
> > + #include <dt-bindings/clock/imx8mq-clock.h>
> > + #include <dt-bindings/power/imx8mq-power.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > + vpu_g2: video-codec@38310000 {
> > + compatible = "nxp,imx8mq-vpu-g2";
> > + reg = <0x38310000 0x10000>;
> > + reg-names "g2";
>
> And same here.
>
> Thanks!
> Ezequiel
On Thu, Dec 09, 2021 at 05:36:04AM -0600, Adam Ford wrote:
> On Thu, Dec 9, 2021 at 4:26 AM Ezequiel Garcia
> <[email protected]> wrote:
> >
> > Hi,
> >
> > Thanks for the patch.
> >
> > On Wed, Dec 08, 2021 at 04:50:23PM -0600, Adam Ford wrote:
> > > The G1 and G2 are separate decoder blocks that are enabled by the
> > > vpu-blk-ctrl power-domain controller, which now has a proper driver.
> > > Update the bindings to support separate nodes for the G1 and G2
> > > decoders using the proper driver or the older unified node with
> > > the legacy controls.
> > >
> > > To be compatible with older DT the driver, mark certain items as
> > > deprecated and retain the backwards compatible example.
> > >
> > > Signed-off-by: Adam Ford <[email protected]>
> > > ---
> > > .../bindings/media/nxp,imx8mq-vpu.yaml | 83 ++++++++++++++-----
> > > 1 file changed, 64 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > index 762be3f96ce9..eeb7bd6281f9 100644
> > > --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > @@ -15,29 +15,39 @@ description:
> > >
> > > properties:
> > > compatible:
> > > - const: nxp,imx8mq-vpu
> > > + oneOf:
> > > + - const: nxp,imx8mq-vpu
> > > + deprecated: true
> > > + - const: nxp,imx8mq-vpu-g1
> > > + - const: nxp,imx8mq-vpu-g2
> > >
> > > reg:
> > > + minItems: 1
> > > maxItems: 3
> >
> > Is it really useful to keep the deprecated binding nxp,imx8mq-vpu
> > as something supported by the binding file?
>
> Since I was told that the driver needed to be backwards compatible, i
> wanted to make sure that any attempts to build the old device tree
> would not fail
I'm not convinced changing the binding at all is correct. 'The driver
structure is changing and I want the binding to align with it' is not a
reason. Are G1 and G2 actually separate, independent blocks where we
could have 1 or both of them? And what about other platforms using this
block?
Even if the driver handles the old binding, a new dtb with an old kernel
is broken. It's up to the platform to care or not, but you have to
highlight that.
> > In other words, can we drop the deprecated binding from this file,
> > while keeping the support in the driver for legacy device-trees?
>
> I was trying to represent both the old driver binding and the new one
> at the same time. I thought that's what I was told to do.
I don't care so much if we have a schema for old binding. I'd rather
have warnings if the binding has not been updated. Eventually I want to
be able to test for compatibility by testing DTs with different schema
versions. We've got to get to 0 warnings first though...
Rob
Am Freitag, dem 10.12.2021 um 09:36 -0600 schrieb Rob Herring:
> On Thu, Dec 09, 2021 at 05:36:04AM -0600, Adam Ford wrote:
> > On Thu, Dec 9, 2021 at 4:26 AM Ezequiel Garcia
> > <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > Thanks for the patch.
> > >
> > > On Wed, Dec 08, 2021 at 04:50:23PM -0600, Adam Ford wrote:
> > > > The G1 and G2 are separate decoder blocks that are enabled by the
> > > > vpu-blk-ctrl power-domain controller, which now has a proper driver.
> > > > Update the bindings to support separate nodes for the G1 and G2
> > > > decoders using the proper driver or the older unified node with
> > > > the legacy controls.
> > > >
> > > > To be compatible with older DT the driver, mark certain items as
> > > > deprecated and retain the backwards compatible example.
> > > >
> > > > Signed-off-by: Adam Ford <[email protected]>
> > > > ---
> > > > .../bindings/media/nxp,imx8mq-vpu.yaml | 83 ++++++++++++++-----
> > > > 1 file changed, 64 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > > index 762be3f96ce9..eeb7bd6281f9 100644
> > > > --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > > @@ -15,29 +15,39 @@ description:
> > > >
> > > > properties:
> > > > compatible:
> > > > - const: nxp,imx8mq-vpu
> > > > + oneOf:
> > > > + - const: nxp,imx8mq-vpu
> > > > + deprecated: true
> > > > + - const: nxp,imx8mq-vpu-g1
> > > > + - const: nxp,imx8mq-vpu-g2
> > > >
> > > > reg:
> > > > + minItems: 1
> > > > maxItems: 3
> > >
> > > Is it really useful to keep the deprecated binding nxp,imx8mq-vpu
> > > as something supported by the binding file?
> >
> > Since I was told that the driver needed to be backwards compatible, i
> > wanted to make sure that any attempts to build the old device tree
> > would not fail
>
> I'm not convinced changing the binding at all is correct. 'The driver
> structure is changing and I want the binding to align with it' is not a
> reason. Are G1 and G2 actually separate, independent blocks where we
> could have 1 or both of them? And what about other platforms using this
> block?
Yes, they are totally independent video decoder peripherals, handling
different codecs. While I'm not aware that there is a SKU that only
uses one of them, there is a provision in the fuses to disable either
one of the VPU peripherals, so they clearly can work independently.
Smashing them together in one DT node was a mistake IMO. Both VPUs do
not share more than a common power-domain and use the same AMBA domain
bridge to hook into to SoC NoC. On the i.MX8M Mini we have a similar
VPU subsystem, but with nested power domains, so G1, G2 and the new H1
encoder on this chip can even be powered-gated individually.
I agree that the commit message should point out that new DT + old
kernel is not a supported configuration. It isn't optimal, but IMHO a
small price to pay for the ability to handle all the i.MX8M* family VPU
subsystems in the same way with a proper blk-ctrl driver for the common
clock and reset block and the VPUs being independent peripherals.
>
> Even if the driver handles the old binding, a new dtb with an old kernel
> is broken. It's up to the platform to care or not, but you have to
> highlight that.
>
>
> > > In other words, can we drop the deprecated binding from this file,
> > > while keeping the support in the driver for legacy device-trees?
> >
> > I was trying to represent both the old driver binding and the new one
> > at the same time. I thought that's what I was told to do.
>
> I don't care so much if we have a schema for old binding. I'd rather
> have warnings if the binding has not been updated. Eventually I want to
> be able to test for compatibility by testing DTs with different schema
> versions. We've got to get to 0 warnings first though...
I'm in favor of dropping the old binding from the schema. New DTs
should clearly use the new binding and old DTs shouldn't change
anymore, so validation is less useful there.
Regards,
Lucas
On Fri, Dec 10, 2021 at 9:58 AM Lucas Stach <[email protected]> wrote:
>
> Am Freitag, dem 10.12.2021 um 09:36 -0600 schrieb Rob Herring:
> > On Thu, Dec 09, 2021 at 05:36:04AM -0600, Adam Ford wrote:
> > > On Thu, Dec 9, 2021 at 4:26 AM Ezequiel Garcia
> > > <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > On Wed, Dec 08, 2021 at 04:50:23PM -0600, Adam Ford wrote:
> > > > > The G1 and G2 are separate decoder blocks that are enabled by the
> > > > > vpu-blk-ctrl power-domain controller, which now has a proper driver.
> > > > > Update the bindings to support separate nodes for the G1 and G2
> > > > > decoders using the proper driver or the older unified node with
> > > > > the legacy controls.
> > > > >
> > > > > To be compatible with older DT the driver, mark certain items as
> > > > > deprecated and retain the backwards compatible example.
> > > > >
> > > > > Signed-off-by: Adam Ford <[email protected]>
> > > > > ---
> > > > > .../bindings/media/nxp,imx8mq-vpu.yaml | 83 ++++++++++++++-----
> > > > > 1 file changed, 64 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > > > index 762be3f96ce9..eeb7bd6281f9 100644
> > > > > --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > > > @@ -15,29 +15,39 @@ description:
> > > > >
> > > > > properties:
> > > > > compatible:
> > > > > - const: nxp,imx8mq-vpu
> > > > > + oneOf:
> > > > > + - const: nxp,imx8mq-vpu
> > > > > + deprecated: true
> > > > > + - const: nxp,imx8mq-vpu-g1
> > > > > + - const: nxp,imx8mq-vpu-g2
> > > > >
> > > > > reg:
> > > > > + minItems: 1
> > > > > maxItems: 3
> > > >
> > > > Is it really useful to keep the deprecated binding nxp,imx8mq-vpu
> > > > as something supported by the binding file?
> > >
> > > Since I was told that the driver needed to be backwards compatible, i
> > > wanted to make sure that any attempts to build the old device tree
> > > would not fail
> >
> > I'm not convinced changing the binding at all is correct. 'The driver
> > structure is changing and I want the binding to align with it' is not a
> > reason. Are G1 and G2 actually separate, independent blocks where we
> > could have 1 or both of them? And what about other platforms using this
> > block?
>
> Yes, they are totally independent video decoder peripherals, handling
> different codecs. While I'm not aware that there is a SKU that only
> uses one of them, there is a provision in the fuses to disable either
> one of the VPU peripherals, so they clearly can work independently.
>
> Smashing them together in one DT node was a mistake IMO. Both VPUs do
> not share more than a common power-domain and use the same AMBA domain
> bridge to hook into to SoC NoC. On the i.MX8M Mini we have a similar
> VPU subsystem, but with nested power domains, so G1, G2 and the new H1
> encoder on this chip can even be powered-gated individually.
>
> I agree that the commit message should point out that new DT + old
> kernel is not a supported configuration. It isn't optimal, but IMHO a
> small price to pay for the ability to handle all the i.MX8M* family VPU
> subsystems in the same way with a proper blk-ctrl driver for the common
> clock and reset block and the VPUs being independent peripherals.
>
> >
> > Even if the driver handles the old binding, a new dtb with an old kernel
> > is broken. It's up to the platform to care or not, but you have to
> > highlight that.
> >
> >
> > > > In other words, can we drop the deprecated binding from this file,
> > > > while keeping the support in the driver for legacy device-trees?
> > >
> > > I was trying to represent both the old driver binding and the new one
> > > at the same time. I thought that's what I was told to do.
> >
> > I don't care so much if we have a schema for old binding. I'd rather
> > have warnings if the binding has not been updated. Eventually I want to
> > be able to test for compatibility by testing DTs with different schema
> > versions. We've got to get to 0 warnings first though...
>
> I'm in favor of dropping the old binding from the schema. New DTs
> should clearly use the new binding and old DTs shouldn't change
> anymore, so validation is less useful there.
I wonder if it makes sense to have a more standardized hantro schema
for all the users instead of one dedicated to each platform using a
hantro driver. The unified schema could have all the various
compatible flags so the driver still knows which features are
enabled/disabled and it's very clear who all the users are for it. I
would think it could cover both the encoder and decoder variants as
well. We do something similar for 8250 serial ports.
If so, can someone from the media group suggest the best one to
follow? Ezequiel suggested I look at the sama5d4-vdec to help reduce
some clutter in the driver. If that binding is good enough, should I
just add the compatible flags to that and potentially rename it.
adam
>
> Regards,
> Lucas
>
> >
> > I'm in favor of dropping the old binding from the schema. New DTs
> > should clearly use the new binding and old DTs shouldn't change
> > anymore, so validation is less useful there.
>
> I wonder if it makes sense to have a more standardized hantro schema
> for all the users instead of one dedicated to each platform using a
> hantro driver. The unified schema could have all the various
> compatible flags so the driver still knows which features are
> enabled/disabled and it's very clear who all the users are for it. I
> would think it could cover both the encoder and decoder variants as
> well. We do something similar for 8250 serial ports.
>
> If so, can someone from the media group suggest the best one to
> follow? Ezequiel suggested I look at the sama5d4-vdec to help reduce
> some clutter in the driver. If that binding is good enough, should I
> just add the compatible flags to that and potentially rename it.
>
A unified schema might sound elegant and compelling, but I suspect
it can bite us down the road, due to differences in the platform integration.
Seems we are all on-board going forward with a patch similar to this one,
but also dropping the deprecated binding from the schema (while of course
still supporting old DTBs on new kernels).
Rob's suggestion about having a warning in the platform if the binding has
not been updated, sounds like very useful too.
Keep in mind v5.16-rc5 is near, which means we still have a few weeks to
discuss this patch, until the next linux-media merge season.
Thanks,
Ezequiel
On Wed, Dec 08, 2021 at 04:50:27PM -0600, Adam Ford wrote:
> The i.MX8M mini appears to have a similar G1 and G2 decoder but the
> post-procesing isn't present. Add compatible flags to support
> G1 and G2 without post-processing.
>
> Signed-off-by: Adam Ford <[email protected]>
> ---
> Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> index eeb7bd6281f9..fb6c2ab1b2dc 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> @@ -20,6 +20,8 @@ properties:
> deprecated: true
> - const: nxp,imx8mq-vpu-g1
> - const: nxp,imx8mq-vpu-g2
> + - const: nxp, imx8mm-vpu-g1
> + - const: nxp, imx8mm-vpu-g2
space ^
>
> reg:
> minItems: 1
> --
> 2.32.0
>
>