[PATCH v2 0/5] Enable decoder for mt8183

[PATCH v2 0/5] 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.

Changes in v2:
- Merged commit 1 (media: dt-bindings: mediatek,vcodec: Allow single
  clock for mt8183) into commit 3 (media: dt-bindings: mediatek,vcodec:
  Remove VDEC_SYS for mt8183)
- Further constrained properties in dt-binding
- Added CLK_IGNORE_UNUSED flag to active clock
- Reformatted reg-names in DT node

Nícolas F. R. A. Prado (4):
  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        | 65 +++++++++++++++----
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 30 +++++++++
 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.41.0

[PATCH v2 1/5] 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>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

(no changes since v1)

 .../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 fad59b486d5d..63be42560948 100644
--- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
@@ -73,8 +73,6 @@ required:
   - clocks
   - clock-names
   - iommus
-  - assigned-clocks
-  - assigned-clock-parents
 
 allOf:
   - if:
-- 
2.41.0

[PATCH v2 2/5] 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.

While adding the active clock, split the mt8183 clocks since there are
less of them than in mt8173. This is done in this same commit to avoid
changing the number of clocks twice.

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>

---

Changes in v2:
- Merged with patch 1 (media: dt-bindings: mediatek,vcodec: Allow single
  clock for mt8183) to avoid changing number of clocks twice
- Added maxItems to reg-names
- Constrained clocks for each compatible
- Reordered properties for each compatible

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

diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
index 63be42560948..2b29748b1d22 100644
--- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
@@ -21,24 +21,23 @@ properties:
       - mediatek,mt8183-vcodec-dec
 
   reg:
+    minItems: 11
     maxItems: 12
 
+  reg-names:
+    minItems: 11
+    maxItems: 11
+
   interrupts:
     maxItems: 1
 
   clocks:
+    minItems: 2
     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: 2
+    maxItems: 8
 
   assigned-clocks: true
 
@@ -86,6 +85,33 @@ allOf:
       required:
         - mediatek,scp
 
+      properties:
+        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
+
+        clocks:
+          minItems: 2
+          maxItems: 2
+
+        clock-names:
+          items:
+            - const: vdec
+            - const: active
+
   - if:
       properties:
         compatible:
@@ -97,6 +123,25 @@ allOf:
       required:
         - mediatek,vpu
 
+      properties:
+        reg:
+          minItems: 12
+
+        clocks:
+          minItems: 8
+          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
+
 additionalProperties: false
 
 examples:
-- 
2.41.0

[PATCH v2 3/5] 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>
---

(no changes since v1)

 .../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.41.0

[PATCH v2 4/5] 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>

---

Changes in v2:
- Added CLK_IGNORE_UNUSED flag

 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..03c4f1acfdb8 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_FLAGS(_id, _name, _parent, &vdec0_cg_regs, _shift,	\
+		&mtk_clk_gate_ops_setclr, CLK_IGNORE_UNUSED)
+
 #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.41.0

[PATCH v2 5/5] 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>

---

Changes in v2:
- Reformatted reg-names to fit in fewer lines

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

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 5169779d01df..28ad6c480430 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -2019,6 +2019,36 @@ 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.41.0

Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock

[AngeloGioacchino Del Regno]

Il 07/06/23 22:53, Nícolas F. R. A. Prado ha scritto:
> 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>

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

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

[AngeloGioacchino Del Regno]

Il 07/06/23 22:53, 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>
> 

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

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

[AngeloGioacchino Del Regno]

Il 07/06/23 22:53, 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>

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

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

[Chen-Yu Tsai]

On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> 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>
>
> ---
>
> Changes in v2:
> - Added CLK_IGNORE_UNUSED flag
>
>  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..03c4f1acfdb8 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_FLAGS(_id, _name, _parent, &vdec0_cg_regs, _shift,     \
> +               &mtk_clk_gate_ops_setclr, CLK_IGNORE_UNUSED)

I think what you want is a read-only gate clock only used for reading back
the status. The ops would only have .is_enabled.

ChenYu

Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock

[Chen-Yu Tsai]

On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> 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>
> ---
>
> (no changes since v1)
>
>  .../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>

                   ^^^^^^^^^^^^^^

This seems like a violation of the API separation.

>  #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);

AFAIK this is still around for clk drivers that haven't moved to clk_hw.
It shouldn't be used by clock consumers. Would it be better to just pass
a syscon?

ChenYu


> +
> +       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.41.0
>
>

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

[AngeloGioacchino Del Regno]

Il 08/06/23 09:43, Chen-Yu Tsai ha scritto:
> On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
> <nfraprado@collabora.com> wrote:
>>
>> 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>
>>
>> ---
>>
>> Changes in v2:
>> - Added CLK_IGNORE_UNUSED flag
>>
>>   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..03c4f1acfdb8 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_FLAGS(_id, _name, _parent, &vdec0_cg_regs, _shift,     \
>> +               &mtk_clk_gate_ops_setclr, CLK_IGNORE_UNUSED)
> 
> I think what you want is a read-only gate clock only used for reading back
> the status. The ops would only have .is_enabled.

Technically, you're right... but I would delay the introduction of a RO GATE_MTK
clock for later, as it's not worth adding that for just one clock driver usage.

We're checking if the same can be applied to other SoCs as well - if it can,
then it would make sense to do that (small, yes), work... the point here is to
enable MT8183 decoders ASAP to enable decoder tests in KernelCI, along with
all of the other MediaTek Chromebooks.

Though, if you think that it is a good idea to add a RO gate right now, I don't
have any strong opinions against that.

Cheers,
Angelo

Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock

[AngeloGioacchino Del Regno]

Il 08/06/23 10:12, Chen-Yu Tsai ha scritto:
> On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
> <nfraprado@collabora.com> wrote:
>>
>> 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>
>> ---
>>
>> (no changes since v1)
>>
>>   .../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>
> 
>                     ^^^^^^^^^^^^^^
> 
> This seems like a violation of the API separation.
> 
>>   #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);
> 
> AFAIK this is still around for clk drivers that haven't moved to clk_hw.
> It shouldn't be used by clock consumers. Would it be better to just pass
> a syscon?
> 

This is a legit usage of __clk_is_enabled().... because that's what we're really
doing here, we're checking if a clock got enabled by the underlying MCU (as that
clock goes up after the VDEC boots).

If this is *not* acceptable as it is, we will have to add a clock API call to
check if a clock is enabled... but it didn't seem worth doing since we don't
expect anyone else to have any legit usage of that, or at least, we don't know
about anyone else needing that...

As for the syscon, that's something that we've been discussing as well... the
thing is: we're really *really* checking if a clock is enabled, so we should
be using clock related calls... reading from a syscon means that we'd have to
perform a register read (of.. again.. a clock) outside of the clock framework
which, in my opinion, wouldn't be clean; I'd expect that to become a bit messy
in the future too, should more MediaTek SoCs (I think MT8192/95 are already in
the list, Nicolas please correct me if I'm wrong here) need the same thing, as
we'd be adding more definitions around.

Cheers,
Angelo

Re: [PATCH v2 2/5] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

On Wed, Jun 07, 2023 at 04:53:39PM -0400, 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.
> 
> While adding the active clock, split the mt8183 clocks since there are
> less of them than in mt8173. This is done in this same commit to avoid
> changing the number of clocks twice.
> 
> Also add reg-names to be able to tell that this new register schema is
> used, so the driver can keep backward compatibility.

Rationale here seems to make sense to me & seems like whatever
functionality, or lack thereof, for the mt8183 will be preserved w/ the
old devicetree.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock

[AngeloGioacchino Del Regno]

Il 09/06/23 01:56, Stephen Boyd ha scritto:
> Quoting AngeloGioacchino Del Regno (2023-06-08 02:01:58)
>> Il 08/06/23 10:12, Chen-Yu Tsai ha scritto:
>>> On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
>>> <nfraprado@collabora.com> wrote:
>>>> 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>
>>>
>>>                      ^^^^^^^^^^^^^^
>>>
>>> This seems like a violation of the API separation.
> 
> Yes.
> 
>>>
>>>>    #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);
>>>
>>> AFAIK this is still around for clk drivers that haven't moved to clk_hw.
>>> It shouldn't be used by clock consumers. Would it be better to just pass
>>> a syscon?
>>>
>>
>> This is a legit usage of __clk_is_enabled().... because that's what we're really
>> doing here, we're checking if a clock got enabled by the underlying MCU (as that
>> clock goes up after the VDEC boots).
>>
>> If this is *not* acceptable as it is, we will have to add a clock API call to
>> check if a clock is enabled... but it didn't seem worth doing since we don't
>> expect anyone else to have any legit usage of that, or at least, we don't know
>> about anyone else needing that...
> 
> The design of the clk.h API has been that no clk consumer should need to
> find out if a clk is enabled. Instead, the clk consumer should enable
> the clk if they want it enabled. Is there no other way to know that the
> vcodec hardware is active?
> 

The firmware gives an indication of "boot done", but that's for the "core" part
of the vcodec... then it manages this clock internally to enable/disable the
"compute" IP of the decoder.

As far as I know (and I've been researching about this) the firmware will not
give any "decoder powered, clocked - ready to get data" indication, and the
only way that we have to judge whether it is in this specific state or not is
to check if the "VDEC_ACTIVE" clock got enabled by the firmware.

That's *synthetically* the whole story...

>>
>> As for the syscon, that's something that we've been discussing as well... the
>> thing is: we're really *really* checking if a clock is enabled, so we should
>> be using clock related calls... reading from a syscon means that we'd have to
>> perform a register read (of.. again.. a clock) outside of the clock framework
>> which, in my opinion, wouldn't be clean; I'd expect that to become a bit messy
>> in the future too, should more MediaTek SoCs (I think MT8192/95 are already in
>> the list, Nicolas please correct me if I'm wrong here) need the same thing, as
>> we'd be adding more definitions around.
>>

Re: [PATCH v2 0/5] Enable decoder for mt8183

[Hans Verkuil]

Hi Nicolas,

On 07/06/2023 22:53, Nícolas F. R. A. Prado wrote:
> 
> 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.
> 
> Changes in v2:
> - Merged commit 1 (media: dt-bindings: mediatek,vcodec: Allow single
>   clock for mt8183) into commit 3 (media: dt-bindings: mediatek,vcodec:
>   Remove VDEC_SYS for mt8183)
> - Further constrained properties in dt-binding
> - Added CLK_IGNORE_UNUSED flag to active clock
> - Reformatted reg-names in DT node
> 
> Nícolas F. R. A. Prado (4):
>   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

Is the clk patch independent from the others? It's not clear to me.

If the clk patch has to go in together with the media patches, then
please let me know and post a v3 where the clk patch is also CC-ed to
the linux-media mailinglist to ensure it ends up in our patchwork system.

And in that case I need a Acked-by from the clk maintainer as well.

If it is independent, then there is no need for a v3 (at least, not
for this).

Regards,

	Hans

> 
> Yunfei Dong (1):
>   arm64: dts: mediatek: mt8183: Add decoder
> 
>  .../media/mediatek,vcodec-decoder.yaml        | 65 +++++++++++++++----
>  arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 30 +++++++++
>  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(-)
> 

Re: [PATCH v2 0/5] Enable decoder for mt8183

[AngeloGioacchino Del Regno]

Il 12/06/23 09:02, Hans Verkuil ha scritto:
> Hi Nicolas,
> 
> On 07/06/2023 22:53, Nícolas F. R. A. Prado wrote:
>>
>> 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.
>>
>> Changes in v2:
>> - Merged commit 1 (media: dt-bindings: mediatek,vcodec: Allow single
>>    clock for mt8183) into commit 3 (media: dt-bindings: mediatek,vcodec:
>>    Remove VDEC_SYS for mt8183)
>> - Further constrained properties in dt-binding
>> - Added CLK_IGNORE_UNUSED flag to active clock
>> - Reformatted reg-names in DT node
>>
>> Nícolas F. R. A. Prado (4):
>>    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
> 
> Is the clk patch independent from the others? It's not clear to me.
> 
> If the clk patch has to go in together with the media patches, then
> please let me know and post a v3 where the clk patch is also CC-ed to
> the linux-media mailinglist to ensure it ends up in our patchwork system.
> 
> And in that case I need a Acked-by from the clk maintainer as well.
> 
> If it is independent, then there is no need for a v3 (at least, not
> for this).
> 

The clock patch is not independent, as in the devicetree changes will not
work without the addition of that clock (and of course even fail building),
so that series needs a v3.

Nícolas, please go on and send a v3 as requested.

Cheers,
Angelo

> Regards,
> 
> 	Hans
> 
>>
>> Yunfei Dong (1):
>>    arm64: dts: mediatek: mt8183: Add decoder
>>
>>   .../media/mediatek,vcodec-decoder.yaml        | 65 +++++++++++++++----
>>   arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 30 +++++++++
>>   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(-)
>>
> 
> 

Re: [PATCH v2 0/5] Enable decoder for mt8183

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

On Mon, Jun 12, 2023 at 11:53:51AM +0200, AngeloGioacchino Del Regno wrote:
> Il 12/06/23 09:02, Hans Verkuil ha scritto:
> > Hi Nicolas,
> > 
> > On 07/06/2023 22:53, Nícolas F. R. A. Prado wrote:
> > > 
> > > 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.
> > > 
> > > Changes in v2:
> > > - Merged commit 1 (media: dt-bindings: mediatek,vcodec: Allow single
> > >    clock for mt8183) into commit 3 (media: dt-bindings: mediatek,vcodec:
> > >    Remove VDEC_SYS for mt8183)
> > > - Further constrained properties in dt-binding
> > > - Added CLK_IGNORE_UNUSED flag to active clock
> > > - Reformatted reg-names in DT node
> > > 
> > > Nícolas F. R. A. Prado (4):
> > >    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
> > 
> > Is the clk patch independent from the others? It's not clear to me.
> > 
> > If the clk patch has to go in together with the media patches, then
> > please let me know and post a v3 where the clk patch is also CC-ed to
> > the linux-media mailinglist to ensure it ends up in our patchwork system.
> > 
> > And in that case I need a Acked-by from the clk maintainer as well.
> > 
> > If it is independent, then there is no need for a v3 (at least, not
> > for this).
> > 
> 
> The clock patch is not independent, as in the devicetree changes will not
> work without the addition of that clock (and of course even fail building),

Yes, but that means that the devicetree patch is dependent of the clock patch,
but the clock patch is independent from the media patches, and can therefore be
applied through the clock tree as usual.

So, the media patches (first three) can be merged through the media tree, the
clock patch (patch 4) through the clock tree independently, and the only
requirement is that the DT patch (last one) is only applied by Matthias after
the clock patch is present, to avoid build problems, and also after the media
patches are present, to avoid dt-binding errors.

Thanks,
Nícolas

Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock

[AngeloGioacchino Del Regno]

Il 12/06/23 21:19, Stephen Boyd ha scritto:
> Quoting AngeloGioacchino Del Regno (2023-06-09 00:42:13)
>> Il 09/06/23 01:56, Stephen Boyd ha scritto:
>>> Quoting AngeloGioacchino Del Regno (2023-06-08 02:01:58)
>>>> Il 08/06/23 10:12, Chen-Yu Tsai ha scritto:
>>>>> On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
>>>>> <nfraprado@collabora.com> wrote:
>>>>>> 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
>>>>>
>>>>> AFAIK this is still around for clk drivers that haven't moved to clk_hw.
>>>>> It shouldn't be used by clock consumers. Would it be better to just pass
>>>>> a syscon?
>>>>>
>>>>
>>>> This is a legit usage of __clk_is_enabled().... because that's what we're really
>>>> doing here, we're checking if a clock got enabled by the underlying MCU (as that
>>>> clock goes up after the VDEC boots).
>>>>
>>>> If this is *not* acceptable as it is, we will have to add a clock API call to
>>>> check if a clock is enabled... but it didn't seem worth doing since we don't
>>>> expect anyone else to have any legit usage of that, or at least, we don't know
>>>> about anyone else needing that...
>>>
>>> The design of the clk.h API has been that no clk consumer should need to
>>> find out if a clk is enabled. Instead, the clk consumer should enable
>>> the clk if they want it enabled. Is there no other way to know that the
>>> vcodec hardware is active?
>>>
>>
>> The firmware gives an indication of "boot done", but that's for the "core" part
>> of the vcodec... then it manages this clock internally to enable/disable the
>> "compute" IP of the decoder.
>>
>> As far as I know (and I've been researching about this) the firmware will not
>> give any "decoder powered, clocked - ready to get data" indication, and the
>> only way that we have to judge whether it is in this specific state or not is
>> to check if the "VDEC_ACTIVE" clock got enabled by the firmware.
> 
> Is Linux ever going to use clk consumer APIs like clk_enable/clk_disable
> on this VDEC_ACTIVE clk? If the answer is no, then there isn't any
> reason to put it in the clk framework, and probably syscon is the way to
> go for now.
> 

Not for the current platform, but that may change in future SoCs... we're not sure.

> Another approach could be to wait for some amount of time after telling
> firmware to power up and assume the hardware is active.
> 

That would be highly error prone though. Expecting that the HW is alive means that
we're 100% sure that both firmware and driver are doing the right thing at every
moment, which is something that we'd like to assume but, realistically, for safety
reasons we just don't.

Should we anyway go for a syscon *now* and then change it to a clock later, if any
new platform needs this as a clock?

I'm in doubt now on how to proceed.

> ----
> 
> I see that the __clk_is_enabled() API is being used in some other
> consumer drivers. I think at one point we were down to one or two users.
> I'll try to remove this function entirely, but it will still be possible
> to get at the clk_hw for a clk with __clk_get_hw() and then call
> clk_hw_is_enabled().
> 

Makes sense.

Regards,
Angelo

Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock

[AngeloGioacchino Del Regno]

Il 15/06/23 02:40, Stephen Boyd ha scritto:
> Quoting AngeloGioacchino Del Regno (2023-06-14 01:13:43)
>> Il 12/06/23 21:19, Stephen Boyd ha scritto:
>>> Quoting AngeloGioacchino Del Regno (2023-06-09 00:42:13)
>>>> Il 09/06/23 01:56, Stephen Boyd ha scritto:
>>>>> Quoting AngeloGioacchino Del Regno (2023-06-08 02:01:58)
>>>>>> Il 08/06/23 10:12, Chen-Yu Tsai ha scritto:
>>>>>>> On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
>>>>>>> <nfraprado@collabora.com> wrote:
>>>>
>>>> The firmware gives an indication of "boot done", but that's for the "core" part
>>>> of the vcodec... then it manages this clock internally to enable/disable the
>>>> "compute" IP of the decoder.
>>>>
>>>> As far as I know (and I've been researching about this) the firmware will not
>>>> give any "decoder powered, clocked - ready to get data" indication, and the
>>>> only way that we have to judge whether it is in this specific state or not is
>>>> to check if the "VDEC_ACTIVE" clock got enabled by the firmware.
>>>
>>> Is Linux ever going to use clk consumer APIs like clk_enable/clk_disable
>>> on this VDEC_ACTIVE clk? If the answer is no, then there isn't any
>>> reason to put it in the clk framework, and probably syscon is the way to
>>> go for now.
>>>
>>
>> Not for the current platform, but that may change in future SoCs... we're not sure.
> 
> If you're not using the clk consumer APIs then it shouldn't be a clk.
> 
>>
>>> Another approach could be to wait for some amount of time after telling
>>> firmware to power up and assume the hardware is active.
>>>
>>
>> That would be highly error prone though. Expecting that the HW is alive means that
>> we're 100% sure that both firmware and driver are doing the right thing at every
>> moment, which is something that we'd like to assume but, realistically, for safety
>> reasons we just don't.
>>
>> Should we anyway go for a syscon *now* and then change it to a clock later, if any
>> new platform needs this as a clock?
> 
> Yeah. Or implement this as a power domain and have it read the register
> directly waiting to return from the power_on()?

A power domain would force us to incorrectly describe the hardware in the bindings
though, I think... so, Nícolas, please, let's go for a syscon at this point, as it
really looks like being the only viable option.

Stephen, many thanks for the valuable suggestions and the nice conversation.

Cheers!
Angelo

Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock

[AngeloGioacchino Del Regno]

Il 15/06/23 19:40, Stephen Boyd ha scritto:
> Quoting AngeloGioacchino Del Regno (2023-06-15 00:30:56)
>> Il 15/06/23 02:40, Stephen Boyd ha scritto:
>>> Quoting AngeloGioacchino Del Regno (2023-06-14 01:13:43)
>>>> Il 12/06/23 21:19, Stephen Boyd ha scritto:
>>>>> Quoting AngeloGioacchino Del Regno (2023-06-09 00:42:13)
>>>>>> Il 09/06/23 01:56, Stephen Boyd ha scritto:
>>>>>>> Quoting AngeloGioacchino Del Regno (2023-06-08 02:01:58)
>>>>>>>> Il 08/06/23 10:12, Chen-Yu Tsai ha scritto:
>>>>>>>>> On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
>>>>>>>>> <nfraprado@collabora.com> wrote:
>>>>>>
>>>>>> The firmware gives an indication of "boot done", but that's for the "core" part
>>>>>> of the vcodec... then it manages this clock internally to enable/disable the
>>>>>> "compute" IP of the decoder.
>>>>>>
>>>>>> As far as I know (and I've been researching about this) the firmware will not
>>>>>> give any "decoder powered, clocked - ready to get data" indication, and the
>>>>>> only way that we have to judge whether it is in this specific state or not is
>>>>>> to check if the "VDEC_ACTIVE" clock got enabled by the firmware.
>>>>>
>>>>> Is Linux ever going to use clk consumer APIs like clk_enable/clk_disable
>>>>> on this VDEC_ACTIVE clk? If the answer is no, then there isn't any
>>>>> reason to put it in the clk framework, and probably syscon is the way to
>>>>> go for now.
>>>>>
>>>>
>>>> Not for the current platform, but that may change in future SoCs... we're not sure.
>>>
>>> If you're not using the clk consumer APIs then it shouldn't be a clk.
>>>
>>>>
>>>>> Another approach could be to wait for some amount of time after telling
>>>>> firmware to power up and assume the hardware is active.
>>>>>
>>>>
>>>> That would be highly error prone though. Expecting that the HW is alive means that
>>>> we're 100% sure that both firmware and driver are doing the right thing at every
>>>> moment, which is something that we'd like to assume but, realistically, for safety
>>>> reasons we just don't.
>>>>
>>>> Should we anyway go for a syscon *now* and then change it to a clock later, if any
>>>> new platform needs this as a clock?
>>>
>>> Yeah. Or implement this as a power domain and have it read the register
>>> directly waiting to return from the power_on()?
>>
>> A power domain would force us to incorrectly describe the hardware in the bindings
>> though, I think... so, Nícolas, please, let's go for a syscon at this point, as it
> 
> You don't have to add the power domain in DT, do you? You can populate a
> power domain in software directly?
> 

Right. I didn't evaluate that possibility at all. Looks good!

>> really looks like being the only viable option.
>>
>> Stephen, many thanks for the valuable suggestions and the nice conversation.
>>
> 

Re: [PATCH v2 1/5] media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks

[Alexandre Mergnat]

On 07/06/2023 22:53, 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.

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

-- 
Regards,
Alexandre