[PATCH 0/6] Enable decoder for mt8183

[PATCH 0/6] Enable decoder for mt8183

[Nícolas F. R. A. Prado]

This series enables the hardware decoder present on mt8183. At first
glance, the only missing piece is the devicetree node for it, however,
simply adding it as is would cause an address collision between the
first register iospace and the clock-controller node, so a rework of the
dt-binding and driver, as well as addition of a clock, were needed
first.

Tested that H264 decoding works with the hardware decoder on
mt8183-kukui-jacuzzi-juniper-sku16, giving a fluster score of 98/135 on
the JVT-AVC_V1 test suite. And ensured other SoCs (MT8192 and MT8195)
still work as usual.


Nícolas F. R. A. Prado (5):
  media: dt-bindings: mediatek,vcodec: Allow single clock for mt8183
  media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks
  media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183
  media: mediatek: vcodec: Read HW active status from clock
  clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

Yunfei Dong (1):
  arm64: dts: mediatek: mt8183: Add decoder

 .../media/mediatek,vcodec-decoder.yaml        | 56 ++++++++++++++----
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 39 ++++++++++++
 drivers/clk/mediatek/clk-mt8183-vdec.c        |  5 ++
 .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 59 +++++++++++++++----
 .../mediatek/vcodec/mtk_vcodec_dec_hw.c       | 20 +++++--
 .../mediatek/vcodec/mtk_vcodec_dec_pm.c       | 12 +++-
 .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
 include/dt-bindings/clock/mt8183-clk.h        |  3 +-
 8 files changed, 165 insertions(+), 30 deletions(-)

-- 
2.40.1

[PATCH 1/6] media: dt-bindings: mediatek,vcodec: Allow single clock for mt8183

[Nícolas F. R. A. Prado]

MT8173 and MT8183 have different clocks, and consequently clock-names.
Relax the number of clocks and set clock-names based on compatible.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

 .../media/mediatek,vcodec-decoder.yaml        | 29 +++++++++++++------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
index fad59b486d5d..57d5ca776df0 100644
--- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
@@ -27,18 +27,12 @@ properties:
     maxItems: 1
 
   clocks:
+    minItems: 1
     maxItems: 8
 
   clock-names:
-    items:
-      - const: vcodecpll
-      - const: univpll_d2
-      - const: clk_cci400_sel
-      - const: vdec_sel
-      - const: vdecpll
-      - const: vencpll
-      - const: venc_lt_sel
-      - const: vdec_bus_clk_src
+    minItems: 1
+    maxItems: 8
 
   assigned-clocks: true
 
@@ -88,6 +82,11 @@ allOf:
       required:
         - mediatek,scp
 
+      properties:
+        clock-names:
+          items:
+            - const: vdec
+
   - if:
       properties:
         compatible:
@@ -99,6 +98,18 @@ allOf:
       required:
         - mediatek,vpu
 
+      properties:
+        clock-names:
+          items:
+            - const: vcodecpll
+            - const: univpll_d2
+            - const: clk_cci400_sel
+            - const: vdec_sel
+            - const: vdecpll
+            - const: vencpll
+            - const: venc_lt_sel
+            - const: vdec_bus_clk_src
+
 additionalProperties: false
 
 examples:
-- 
2.40.1

[PATCH 2/6] media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks

[Nícolas F. R. A. Prado]

On MT8183 it's not necessary to configure the parent for the clocks.
Remove the assigned-clocks and assigned-clock-parents from the required
list.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

 .../devicetree/bindings/media/mediatek,vcodec-decoder.yaml      | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
index 57d5ca776df0..6447e6c86f29 100644
--- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
@@ -67,8 +67,6 @@ required:
   - clocks
   - clock-names
   - iommus
-  - assigned-clocks
-  - assigned-clock-parents
 
 allOf:
   - if:
-- 
2.40.1

[PATCH 3/6] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183

[Nícolas F. R. A. Prado]

The binding expects the first register space to be VDEC_SYS. But on
mt8183, which uses the stateless decoders, this space is used only for
controlling clocks and resets, which are better described as separate
clock-controller and reset-controller nodes.

In fact, in mt8173's devicetree there are already such separate
clock-controller nodes, which cause duplicate addresses between the
vdecsys node and the vcodec node. But for this SoC, since the stateful
decoder code makes other uses of the VDEC_SYS register space, it's not
straightforward to remove it.

In order to avoid the same address conflict to happen on mt8183,
since the only current use of the VDEC_SYS register space in
the driver is to read the status of a clock that indicates the hardware
is active, remove the VDEC_SYS register space from the binding and
describe an extra clock that will be used to directly check the hardware
status.

Also add reg-names to be able to tell that this new register schema is
used, so the driver can keep backward compatibility.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

 .../media/mediatek,vcodec-decoder.yaml        | 29 +++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
index 6447e6c86f29..36a53b2484d6 100644
--- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
@@ -21,17 +21,21 @@ properties:
       - mediatek,mt8183-vcodec-dec
 
   reg:
+    minItems: 11
     maxItems: 12
 
+  reg-names:
+    minItems: 11
+
   interrupts:
     maxItems: 1
 
   clocks:
-    minItems: 1
+    minItems: 2
     maxItems: 8
 
   clock-names:
-    minItems: 1
+    minItems: 2
     maxItems: 8
 
   assigned-clocks: true
@@ -84,6 +88,24 @@ allOf:
         clock-names:
           items:
             - const: vdec
+            - const: active
+
+        reg:
+          maxItems: 11
+
+        reg-names:
+          items:
+            - const: misc
+            - const: ld
+            - const: top
+            - const: cm
+            - const: ad
+            - const: av
+            - const: pp
+            - const: hwd
+            - const: hwq
+            - const: hwb
+            - const: hwg
 
   - if:
       properties:
@@ -108,6 +130,9 @@ allOf:
             - const: venc_lt_sel
             - const: vdec_bus_clk_src
 
+        reg:
+          minItems: 12
+
 additionalProperties: false
 
 examples:
-- 
2.40.1

[PATCH 4/6] media: mediatek: vcodec: Read HW active status from clock

[Nícolas F. R. A. Prado]

Remove the requirement of a VDEC_SYS reg iospace. To achieve that, rely
on the "active" clock being passed through the DT, and read its status
during IRQ handling to check whether the HW is active.

The old behavior is still present when reg-names aren't supplied, as to
keep backward compatibility.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

 .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 59 +++++++++++++++----
 .../mediatek/vcodec/mtk_vcodec_dec_hw.c       | 20 +++++--
 .../mediatek/vcodec/mtk_vcodec_dec_pm.c       | 12 +++-
 .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
 4 files changed, 74 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
index 9c652beb3f19..8038472fb67b 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
@@ -16,6 +16,7 @@
 #include <media/v4l2-mem2mem.h>
 #include <media/videobuf2-dma-contig.h>
 #include <media/v4l2-device.h>
+#include <linux/clk-provider.h>
 
 #include "mtk_vcodec_drv.h"
 #include "mtk_vcodec_dec.h"
@@ -38,22 +39,29 @@ static int mtk_vcodec_get_hw_count(struct mtk_vcodec_dev *dev)
 	}
 }
 
+static bool mtk_vcodec_is_hw_active(struct mtk_vcodec_dev *dev)
+{
+	u32 cg_status = 0;
+
+	if (!dev->reg_base[VDEC_SYS])
+		return __clk_is_enabled(dev->pm.vdec_active_clk);
+
+	cg_status = readl(dev->reg_base[VDEC_SYS]);
+	return (cg_status & VDEC_HW_ACTIVE) == 0;
+}
+
 static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv)
 {
 	struct mtk_vcodec_dev *dev = priv;
 	struct mtk_vcodec_ctx *ctx;
-	u32 cg_status = 0;
 	unsigned int dec_done_status = 0;
 	void __iomem *vdec_misc_addr = dev->reg_base[VDEC_MISC] +
 					VDEC_IRQ_CFG_REG;
 
 	ctx = mtk_vcodec_get_curr_ctx(dev, MTK_VDEC_CORE);
 
-	/* check if HW active or not */
-	cg_status = readl(dev->reg_base[0]);
-	if ((cg_status & VDEC_HW_ACTIVE) != 0) {
-		mtk_v4l2_err("DEC ISR, VDEC active is not 0x0 (0x%08x)",
-			     cg_status);
+	if (!mtk_vcodec_is_hw_active(dev)) {
+		mtk_v4l2_err("DEC ISR, VDEC active is not 0x0");
 		return IRQ_HANDLED;
 	}
 
@@ -82,6 +90,25 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)
 {
 	struct platform_device *pdev = dev->plat_dev;
 	int reg_num, i;
+	struct resource *res;
+	bool no_vdecsys_reg = false;
+	static const char * const mtk_dec_reg_names[] = {
+		"misc",
+		"ld",
+		"top",
+		"cm",
+		"ad",
+		"av",
+		"pp",
+		"hwd",
+		"hwq",
+		"hwb",
+		"hwg"
+	};
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "misc");
+	if (res)
+		no_vdecsys_reg = true;
 
 	/* Sizeof(u32) * 4 bytes for each register base. */
 	reg_num = of_property_count_elems_of_size(pdev->dev.of_node, "reg",
@@ -91,12 +118,22 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)
 		return -EINVAL;
 	}
 
-	for (i = 0; i < reg_num; i++) {
-		dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i);
-		if (IS_ERR(dev->reg_base[i]))
-			return PTR_ERR(dev->reg_base[i]);
+	if (!no_vdecsys_reg) {
+		for (i = 0; i < reg_num; i++) {
+			dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i);
+			if (IS_ERR(dev->reg_base[i]))
+				return PTR_ERR(dev->reg_base[i]);
 
-		mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev->reg_base[i]);
+			mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev->reg_base[i]);
+		}
+	} else {
+		for (i = 0; i < reg_num; i++) {
+			dev->reg_base[i+1] = devm_platform_ioremap_resource_byname(pdev, mtk_dec_reg_names[i]);
+			if (IS_ERR(dev->reg_base[i+1]))
+				return PTR_ERR(dev->reg_base[i+1]);
+
+			mtk_v4l2_debug(2, "reg[%d] base=%p", i+1, dev->reg_base[i+1]);
+		}
 	}
 
 	return 0;
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c
index b753bf54ebd9..4e786821015d 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c
@@ -11,6 +11,7 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
+#include <linux/clk-provider.h>
 
 #include "mtk_vcodec_drv.h"
 #include "mtk_vcodec_dec.h"
@@ -63,22 +64,29 @@ static int mtk_vdec_hw_prob_done(struct mtk_vcodec_dev *vdec_dev)
 	return 0;
 }
 
+static bool mtk_vcodec_is_hw_active(struct mtk_vdec_hw_dev *dev)
+{
+	u32 cg_status;
+
+	if (!dev->reg_base[VDEC_HW_SYS])
+		return __clk_is_enabled(dev->pm.vdec_active_clk);
+
+	cg_status = readl(dev->reg_base[VDEC_HW_SYS]);
+	return (cg_status & VDEC_HW_ACTIVE) == 0;
+}
+
 static irqreturn_t mtk_vdec_hw_irq_handler(int irq, void *priv)
 {
 	struct mtk_vdec_hw_dev *dev = priv;
 	struct mtk_vcodec_ctx *ctx;
-	u32 cg_status;
 	unsigned int dec_done_status;
 	void __iomem *vdec_misc_addr = dev->reg_base[VDEC_HW_MISC] +
 					VDEC_IRQ_CFG_REG;
 
 	ctx = mtk_vcodec_get_curr_ctx(dev->main_dev, dev->hw_idx);
 
-	/* check if HW active or not */
-	cg_status = readl(dev->reg_base[VDEC_HW_SYS]);
-	if (cg_status & VDEC_HW_ACTIVE) {
-		mtk_v4l2_err("vdec active is not 0x0 (0x%08x)",
-			     cg_status);
+	if (!mtk_vcodec_is_hw_active(dev)) {
+		mtk_v4l2_err("vdec active is not 0x0");
 		return IRQ_HANDLED;
 	}
 
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c
index 777d445999e9..53e621965950 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c
@@ -51,6 +51,9 @@ int mtk_vcodec_init_dec_clk(struct platform_device *pdev, struct mtk_vcodec_pm *
 				clk_info->clk_name);
 			return PTR_ERR(clk_info->vcodec_clk);
 		}
+
+		if (strcmp(clk_info->clk_name, "active") == 0)
+			pm->vdec_active_clk = clk_info->vcodec_clk;
 	}
 
 	return 0;
@@ -84,6 +87,9 @@ static void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm)
 
 	dec_clk = &pm->vdec_clk;
 	for (i = 0; i < dec_clk->clk_num; i++) {
+		if (strcmp(dec_clk->clk_info[i].clk_name, "active") == 0)
+			continue;
+
 		ret = clk_prepare_enable(dec_clk->clk_info[i].vcodec_clk);
 		if (ret) {
 			mtk_v4l2_err("clk_prepare_enable %d %s fail %d", i,
@@ -104,8 +110,12 @@ static void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm)
 	int i;
 
 	dec_clk = &pm->vdec_clk;
-	for (i = dec_clk->clk_num - 1; i >= 0; i--)
+	for (i = dec_clk->clk_num - 1; i >= 0; i--) {
+		if (strcmp(dec_clk->clk_info[i].clk_name, "active") == 0)
+			continue;
+
 		clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
+	}
 }
 
 static void mtk_vcodec_dec_enable_irq(struct mtk_vcodec_dev *vdec_dev, int hw_idx)
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
index 9acab54fd650..180e74c69042 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
@@ -208,6 +208,7 @@ struct mtk_vcodec_pm {
 	struct mtk_vcodec_clk	vdec_clk;
 	struct mtk_vcodec_clk	venc_clk;
 	struct device	*dev;
+	struct clk *vdec_active_clk;
 };
 
 /**
-- 
2.40.1

[PATCH 5/6] clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

[Nícolas F. R. A. Prado]

Add the CLK_VDEC_ACTIVE clock to the vdec clock driver. This clock is
enabled by the VPU once it starts decoding.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

 drivers/clk/mediatek/clk-mt8183-vdec.c | 5 +++++
 include/dt-bindings/clock/mt8183-clk.h | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/mediatek/clk-mt8183-vdec.c b/drivers/clk/mediatek/clk-mt8183-vdec.c
index 513b7956cbea..5830934a6d25 100644
--- a/drivers/clk/mediatek/clk-mt8183-vdec.c
+++ b/drivers/clk/mediatek/clk-mt8183-vdec.c
@@ -27,6 +27,10 @@ static const struct mtk_gate_regs vdec1_cg_regs = {
 	GATE_MTK(_id, _name, _parent, &vdec0_cg_regs, _shift,	\
 		&mtk_clk_gate_ops_setclr_inv)
 
+#define GATE_VDEC0(_id, _name, _parent, _shift)		\
+	GATE_MTK(_id, _name, _parent, &vdec0_cg_regs, _shift,	\
+		&mtk_clk_gate_ops_setclr)
+
 #define GATE_VDEC1_I(_id, _name, _parent, _shift)		\
 	GATE_MTK(_id, _name, _parent, &vdec1_cg_regs, _shift,	\
 		&mtk_clk_gate_ops_setclr_inv)
@@ -34,6 +38,7 @@ static const struct mtk_gate_regs vdec1_cg_regs = {
 static const struct mtk_gate vdec_clks[] = {
 	/* VDEC0 */
 	GATE_VDEC0_I(CLK_VDEC_VDEC, "vdec_vdec", "mm_sel", 0),
+	GATE_VDEC0(CLK_VDEC_ACTIVE, "vdec_active", "mm_sel", 4),
 	/* VDEC1 */
 	GATE_VDEC1_I(CLK_VDEC_LARB1, "vdec_larb1", "mm_sel", 0),
 };
diff --git a/include/dt-bindings/clock/mt8183-clk.h b/include/dt-bindings/clock/mt8183-clk.h
index a7b470b0ec8a..32dd7d91dbe2 100644
--- a/include/dt-bindings/clock/mt8183-clk.h
+++ b/include/dt-bindings/clock/mt8183-clk.h
@@ -357,7 +357,8 @@
 /* VDEC_GCON */
 #define CLK_VDEC_VDEC			0
 #define CLK_VDEC_LARB1			1
-#define CLK_VDEC_NR_CLK			2
+#define CLK_VDEC_ACTIVE			2
+#define CLK_VDEC_NR_CLK			3
 
 /* VENC_GCON */
 #define CLK_VENC_LARB			0
-- 
2.40.1

[PATCH 6/6] arm64: dts: mediatek: mt8183: Add decoder

[Nícolas F. R. A. Prado]

From: Yunfei Dong <yunfei.dong@mediatek.com>

Add node for the hardware decoder present on the MT8183 SoC.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
Signed-off-by: Qianqian Yan <qianqian.yan@mediatek.com>
Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 39 ++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 5169779d01df..8bb10ed67e87 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -2019,6 +2019,45 @@ vdecsys: syscon@16000000 {
 			#clock-cells = <1>;
 		};
 
+		vcodec_dec: video-codec@16020000 {
+			compatible = "mediatek,mt8183-vcodec-dec";
+			reg = <0 0x16020000 0 0x1000>,		/* VDEC_MISC */
+			      <0 0x16021000 0 0x800>,		/* VDEC_VLD */
+			      <0 0x16021800 0 0x800>,		/* VDEC_TOP */
+			      <0 0x16022000 0 0x1000>,		/* VDEC_MC */
+			      <0 0x16023000 0 0x1000>,		/* VDEC_AVCVLD */
+			      <0 0x16024000 0 0x1000>,		/* VDEC_AVCMV */
+			      <0 0x16025000 0 0x1000>,		/* VDEC_PP */
+			      <0 0x16026800 0 0x800>,		/* VP8_VD */
+			      <0 0x16027000 0 0x800>,		/* VP6_VD */
+			      <0 0x16027800 0 0x800>,		/* VP8_VL */
+			      <0 0x16028400 0 0x400>;		/* VP9_VD */
+			reg-names = "misc",
+				    "ld",
+				    "top",
+				    "cm",
+				    "ad",
+				    "av",
+				    "pp",
+				    "hwd",
+				    "hwq",
+				    "hwb",
+				    "hwg";
+			interrupts = <GIC_SPI 250 IRQ_TYPE_LEVEL_LOW>;
+			iommus = <&iommu M4U_PORT_HW_VDEC_MC_EXT>,
+				 <&iommu M4U_PORT_HW_VDEC_PP_EXT>,
+				 <&iommu M4U_PORT_HW_VDEC_VLD_EXT>,
+				 <&iommu M4U_PORT_HW_VDEC_AVC_MV_EXT>,
+				 <&iommu M4U_PORT_HW_VDEC_PRED_RD_EXT>,
+				 <&iommu M4U_PORT_HW_VDEC_PRED_WR_EXT>,
+				 <&iommu M4U_PORT_HW_VDEC_PPWRAP_EXT>;
+			mediatek,scp = <&scp>;
+			power-domains = <&spm MT8183_POWER_DOMAIN_VDEC>;
+			clocks = <&vdecsys CLK_VDEC_VDEC>,
+				 <&vdecsys CLK_VDEC_ACTIVE>;
+			clock-names = "vdec", "active";
+		};
+
 		larb1: larb@16010000 {
 			compatible = "mediatek,mt8183-smi-larb";
 			reg = <0 0x16010000 0 0x1000>;
-- 
2.40.1

Re: [PATCH 1/6] media: dt-bindings: mediatek,vcodec: Allow single clock for mt8183

[Matthias Brugger]

On 05/06/2023 18:20, Nícolas F. R. A. Prado wrote:
> MT8173 and MT8183 have different clocks, and consequently clock-names.
> Relax the number of clocks and set clock-names based on compatible.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> 
> ---
> 
>   .../media/mediatek,vcodec-decoder.yaml        | 29 +++++++++++++------
>   1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> index fad59b486d5d..57d5ca776df0 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> @@ -27,18 +27,12 @@ properties:
>       maxItems: 1
>   
>     clocks:
> +    minItems: 1
>       maxItems: 8
>   
>     clock-names:
> -    items:
> -      - const: vcodecpll
> -      - const: univpll_d2
> -      - const: clk_cci400_sel
> -      - const: vdec_sel
> -      - const: vdecpll
> -      - const: vencpll
> -      - const: venc_lt_sel
> -      - const: vdec_bus_clk_src
> +    minItems: 1
> +    maxItems: 8
>   
>     assigned-clocks: true
>   
> @@ -88,6 +82,11 @@ allOf:
>         required:
>           - mediatek,scp
>   
> +      properties:
> +        clock-names:
> +          items:
> +            - const: vdec
> +
>     - if:
>         properties:
>           compatible:
> @@ -99,6 +98,18 @@ allOf:
>         required:
>           - mediatek,vpu
>   
> +      properties:
> +        clock-names:
> +          items:
> +            - const: vcodecpll
> +            - const: univpll_d2
> +            - const: clk_cci400_sel
> +            - const: vdec_sel
> +            - const: vdecpll
> +            - const: vencpll
> +            - const: venc_lt_sel
> +            - const: vdec_bus_clk_src
> +
>   additionalProperties: false
>   
>   examples:

Re: [PATCH 2/6] media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks

[Matthias Brugger]

On 05/06/2023 18:20, Nícolas F. R. A. Prado wrote:
> On MT8183 it's not necessary to configure the parent for the clocks.
> Remove the assigned-clocks and assigned-clock-parents from the required
> list.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
> 
>   .../devicetree/bindings/media/mediatek,vcodec-decoder.yaml      | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> index 57d5ca776df0..6447e6c86f29 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> @@ -67,8 +67,6 @@ required:
>     - clocks
>     - clock-names
>     - iommus
> -  - assigned-clocks
> -  - assigned-clock-parents
>   
>   allOf:
>     - if:

Re: [PATCH 2/6] media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks

[AngeloGioacchino Del Regno]

Il 05/06/23 18:20, Nícolas F. R. A. Prado ha scritto:
> On MT8183 it's not necessary to configure the parent for the clocks.
> Remove the assigned-clocks and assigned-clock-parents from the required
> list.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

It should not be mandatory to configure clock parents manually on any model
anyway...

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 1/6] media: dt-bindings: mediatek,vcodec: Allow single clock for mt8183

[AngeloGioacchino Del Regno]

Il 05/06/23 18:20, Nícolas F. R. A. Prado ha scritto:
> MT8173 and MT8183 have different clocks, and consequently clock-names.
> Relax the number of clocks and set clock-names based on compatible.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 3/6] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183

[AngeloGioacchino Del Regno]

Il 05/06/23 18:20, Nícolas F. R. A. Prado ha scritto:
> The binding expects the first register space to be VDEC_SYS. But on
> mt8183, which uses the stateless decoders, this space is used only for
> controlling clocks and resets, which are better described as separate
> clock-controller and reset-controller nodes.
> 
> In fact, in mt8173's devicetree there are already such separate
> clock-controller nodes, which cause duplicate addresses between the
> vdecsys node and the vcodec node. But for this SoC, since the stateful
> decoder code makes other uses of the VDEC_SYS register space, it's not
> straightforward to remove it.
> 
> In order to avoid the same address conflict to happen on mt8183,
> since the only current use of the VDEC_SYS register space in
> the driver is to read the status of a clock that indicates the hardware
> is active, remove the VDEC_SYS register space from the binding and
> describe an extra clock that will be used to directly check the hardware
> status.
> 
> Also add reg-names to be able to tell that this new register schema is
> used, so the driver can keep backward compatibility.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 5/6] clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

[AngeloGioacchino Del Regno]

Il 05/06/23 18:20, Nícolas F. R. A. Prado ha scritto:
> Add the CLK_VDEC_ACTIVE clock to the vdec clock driver. This clock is
> enabled by the VPU once it starts decoding.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Since this is hw managed, it's a good idea to add CLK_IGNORE_UNUSED to this
clock's flags to avoid potential lockups at boot. Please add that flag.

Cheers,
Angelo

Re: [PATCH 6/6] arm64: dts: mediatek: mt8183: Add decoder

[AngeloGioacchino Del Regno]

Il 05/06/23 18:20, Nícolas F. R. A. Prado ha scritto:
> From: Yunfei Dong <yunfei.dong@mediatek.com>
> 
> Add node for the hardware decoder present on the MT8183 SoC.
> 
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> Signed-off-by: Qianqian Yan <qianqian.yan@mediatek.com>
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
> 
>   arch/arm64/boot/dts/mediatek/mt8183.dtsi | 39 ++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> index 5169779d01df..8bb10ed67e87 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -2019,6 +2019,45 @@ vdecsys: syscon@16000000 {
>   			#clock-cells = <1>;
>   		};
>   
> +		vcodec_dec: video-codec@16020000 {
> +			compatible = "mediatek,mt8183-vcodec-dec";
> +			reg = <0 0x16020000 0 0x1000>,		/* VDEC_MISC */
> +			      <0 0x16021000 0 0x800>,		/* VDEC_VLD */
> +			      <0 0x16021800 0 0x800>,		/* VDEC_TOP */
> +			      <0 0x16022000 0 0x1000>,		/* VDEC_MC */
> +			      <0 0x16023000 0 0x1000>,		/* VDEC_AVCVLD */
> +			      <0 0x16024000 0 0x1000>,		/* VDEC_AVCMV */
> +			      <0 0x16025000 0 0x1000>,		/* VDEC_PP */
> +			      <0 0x16026800 0 0x800>,		/* VP8_VD */
> +			      <0 0x16027000 0 0x800>,		/* VP6_VD */
> +			      <0 0x16027800 0 0x800>,		/* VP8_VL */
> +			      <0 0x16028400 0 0x400>;		/* VP9_VD */
> +			reg-names = "misc",
> +				    "ld",
> +				    "top",
> +				    "cm",
> +				    "ad",
> +				    "av",
> +				    "pp",
> +				    "hwd",
> +				    "hwq",
> +				    "hwb",
> +				    "hwg";

Do we really need one line for each 2/3 characters reg name? :-P

Regards,
Angelo

Re: [PATCH 1/6] media: dt-bindings: mediatek,vcodec: Allow single clock for mt8183

[Krzysztof Kozlowski]

On 05/06/2023 18:20, Nícolas F. R. A. Prado wrote:
> MT8173 and MT8183 have different clocks, and consequently clock-names.
> Relax the number of clocks and set clock-names based on compatible.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> ---
> 
>  .../media/mediatek,vcodec-decoder.yaml        | 29 +++++++++++++------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> index fad59b486d5d..57d5ca776df0 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> @@ -27,18 +27,12 @@ properties:
>      maxItems: 1
>  
>    clocks:
> +    minItems: 1
>      maxItems: 8
>  
>    clock-names:
> -    items:
> -      - const: vcodecpll
> -      - const: univpll_d2
> -      - const: clk_cci400_sel
> -      - const: vdec_sel
> -      - const: vdecpll
> -      - const: vencpll
> -      - const: venc_lt_sel
> -      - const: vdec_bus_clk_src
> +    minItems: 1
> +    maxItems: 8
>  
>    assigned-clocks: true
>  
> @@ -88,6 +82,11 @@ allOf:
>        required:
>          - mediatek,scp
>  
> +      properties:
> +        clock-names:
> +          items:
> +            - const: vdec

You should constrain also clocks as they must be in sync with names.

> +
>    - if:
>        properties:
>          compatible:
> @@ -99,6 +98,18 @@ allOf:
>        required:
>          - mediatek,vpu
>  
> +      properties:
> +        clock-names:
> +          items:
> +            - const: vcodecpll
> +            - const: univpll_d2
> +            - const: clk_cci400_sel
> +            - const: vdec_sel
> +            - const: vdecpll
> +            - const: vencpll
> +            - const: venc_lt_sel
> +            - const: vdec_bus_clk_src

Ditto.


Best regards,
Krzysztof

Re: [PATCH 2/6] media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks

[Krzysztof Kozlowski]

On 05/06/2023 18:20, Nícolas F. R. A. Prado wrote:
> On MT8183 it's not necessary to configure the parent for the clocks.
> Remove the assigned-clocks and assigned-clock-parents from the required
> list.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 3/6] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183

[Krzysztof Kozlowski]

On 05/06/2023 18:20, Nícolas F. R. A. Prado wrote:
> The binding expects the first register space to be VDEC_SYS. But on
> mt8183, which uses the stateless decoders, this space is used only for
> controlling clocks and resets, which are better described as separate
> clock-controller and reset-controller nodes.
> 
> In fact, in mt8173's devicetree there are already such separate
> clock-controller nodes, which cause duplicate addresses between the
> vdecsys node and the vcodec node. But for this SoC, since the stateful
> decoder code makes other uses of the VDEC_SYS register space, it's not
> straightforward to remove it.
> 
> In order to avoid the same address conflict to happen on mt8183,
> since the only current use of the VDEC_SYS register space in
> the driver is to read the status of a clock that indicates the hardware
> is active, remove the VDEC_SYS register space from the binding and
> describe an extra clock that will be used to directly check the hardware
> status.
> 
> Also add reg-names to be able to tell that this new register schema is
> used, so the driver can keep backward compatibility.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
> 
>  .../media/mediatek,vcodec-decoder.yaml        | 29 +++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> index 6447e6c86f29..36a53b2484d6 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> @@ -21,17 +21,21 @@ properties:
>        - mediatek,mt8183-vcodec-dec
>  
>    reg:
> +    minItems: 11
>      maxItems: 12
>  
> +  reg-names:
> +    minItems: 11

maxItems

> +
>    interrupts:
>      maxItems: 1
>  
>    clocks:
> -    minItems: 1
> +    minItems: 2

It does not make any sense. Just two patches ago you made it 1! Don't
add incorrect values which are immediately changed in the same patchset.

>      maxItems: 8
>  
>    clock-names:
> -    minItems: 1
> +    minItems: 2
>      maxItems: 8
>  
>    assigned-clocks: true
> @@ -84,6 +88,24 @@ allOf:
>          clock-names:
>            items:
>              - const: vdec
> +            - const: active
> +
> +        reg:
> +          maxItems: 11
> +
> +        reg-names:
> +          items:
> +            - const: misc
> +            - const: ld
> +            - const: top
> +            - const: cm
> +            - const: ad
> +            - const: av
> +            - const: pp
> +            - const: hwd
> +            - const: hwq
> +            - const: hwb
> +            - const: hwg
>  
>    - if:
>        properties:
> @@ -108,6 +130,9 @@ allOf:
>              - const: venc_lt_sel
>              - const: vdec_bus_clk_src
>  
> +        reg:
> +          minItems: 12

so max can be 1000?



Best regards,
Krzysztof

Re: [PATCH 3/6] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183

[Nícolas F. R. A. Prado]

On Tue, Jun 06, 2023 at 11:16:12AM +0200, Krzysztof Kozlowski wrote:
> On 05/06/2023 18:20, Nícolas F. R. A. Prado wrote:
> > The binding expects the first register space to be VDEC_SYS. But on
> > mt8183, which uses the stateless decoders, this space is used only for
> > controlling clocks and resets, which are better described as separate
> > clock-controller and reset-controller nodes.
> > 
> > In fact, in mt8173's devicetree there are already such separate
> > clock-controller nodes, which cause duplicate addresses between the
> > vdecsys node and the vcodec node. But for this SoC, since the stateful
> > decoder code makes other uses of the VDEC_SYS register space, it's not
> > straightforward to remove it.
> > 
> > In order to avoid the same address conflict to happen on mt8183,
> > since the only current use of the VDEC_SYS register space in
> > the driver is to read the status of a clock that indicates the hardware
> > is active, remove the VDEC_SYS register space from the binding and
> > describe an extra clock that will be used to directly check the hardware
> > status.
> > 
> > Also add reg-names to be able to tell that this new register schema is
> > used, so the driver can keep backward compatibility.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> > 
> >  .../media/mediatek,vcodec-decoder.yaml        | 29 +++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> > index 6447e6c86f29..36a53b2484d6 100644
> > --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> > @@ -21,17 +21,21 @@ properties:
> >        - mediatek,mt8183-vcodec-dec
> >  
> >    reg:
> > +    minItems: 11
> >      maxItems: 12
> >  
> > +  reg-names:
> > +    minItems: 11
> 
> maxItems
> 
> > +
> >    interrupts:
> >      maxItems: 1
> >  
> >    clocks:
> > -    minItems: 1
> > +    minItems: 2
> 
> It does not make any sense. Just two patches ago you made it 1! Don't
> add incorrect values which are immediately changed in the same patchset.

It's not an incorrect value. At the point that commit was added, the first reg
was still VDEC_SYS, so an active clock wasn't required. This commit removes the
VDEC_SYS reg, and so the active clock becomes necessary. Splitting it like this
made the most sense to me, and I thought it would also simplify review. But
since it seems to be a problem I'll merge commit 1 with this one in v2 to avoid
changing the number of clocks twice.

> 
> >      maxItems: 8
> >  
> >    clock-names:
> > -    minItems: 1
> > +    minItems: 2
> >      maxItems: 8
> >  
> >    assigned-clocks: true
> > @@ -84,6 +88,24 @@ allOf:
> >          clock-names:
> >            items:
> >              - const: vdec
> > +            - const: active
> > +
> > +        reg:
> > +          maxItems: 11
> > +
> > +        reg-names:
> > +          items:
> > +            - const: misc
> > +            - const: ld
> > +            - const: top
> > +            - const: cm
> > +            - const: ad
> > +            - const: av
> > +            - const: pp
> > +            - const: hwd
> > +            - const: hwq
> > +            - const: hwb
> > +            - const: hwg
> >  
> >    - if:
> >        properties:
> > @@ -108,6 +130,9 @@ allOf:
> >              - const: venc_lt_sel
> >              - const: vdec_bus_clk_src
> >  
> > +        reg:
> > +          minItems: 12
> 
> so max can be 1000?

No, there's 'maxItems: 12' up above in the general case.

Thanks,
Nícolas