[PATCH v5 0/7] Enable decoder for mt8183

[PATCH v5 0/7] 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 new syscon phandle
property, 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 v5:
- Added explicit include to patch 5 following 0day report

Changes in v4:
- Removed VDEC_SYS reg from mt8173 as well
- Made driver handling cleaner

Changes in v3:
- Switched the handling of the VDEC_HW_ACTIVE bit to use a syscon
  instead of the 'active' clock

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 (6):
  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 register space
  media: mediatek: vcodec: Define address for VDEC_HW_ACTIVE
  media: mediatek: vcodec: Read HW active status from syscon
  arm64: dts: mediatek: mt8173: Drop VDEC_SYS reg from decoder

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

 .../media/mediatek,vcodec-decoder.yaml        | 67 ++++++++++++----
 arch/arm64/boot/dts/mediatek/mt8173.dtsi      |  8 +-
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 30 ++++++++
 .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 77 ++++++++++++++++---
 .../mediatek/vcodec/mtk_vcodec_dec_hw.c       |  4 +-
 .../mediatek/vcodec/mtk_vcodec_dec_hw.h       |  3 +-
 .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
 .../mediatek/vcodec/mtk_vcodec_util.c         | 15 ++++
 .../mediatek/vcodec/mtk_vcodec_util.h         |  2 +
 .../mediatek/vcodec/vdec/vdec_vp8_if.c        | 10 +--
 10 files changed, 178 insertions(+), 39 deletions(-)

-- 
2.41.0

[PATCH v5 1/7] 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.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

(no changes since v3)

Changes in v3:
- Reintroduced this commit from v1 since the active clock is no longer
  used.
- Further constrained clocks as suggested in v1.

 .../media/mediatek,vcodec-decoder.yaml        | 37 ++++++++++++++-----
 1 file changed, 28 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..1506d2693f7d 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,15 @@ allOf:
       required:
         - mediatek,scp
 
+      properties:
+        clocks:
+          minItems: 1
+          maxItems: 1
+
+        clock-names:
+          items:
+            - const: vdec
+
   - if:
       properties:
         compatible:
@@ -99,6 +102,22 @@ allOf:
       required:
         - mediatek,vpu
 
+      properties:
+        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 v5 2/7] 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 1506d2693f7d..1e56ece44aee 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.41.0

[PATCH v5 3/7] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS register space

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

The binding expects the first register space to be VDEC_SYS. However
this register space is already assigned to a different node on both
MT8173 and MT8183: a clock-controller node called 'vdecsys' which is
also a syscon.

In order to resolve the overlapping address ranges, remove the VDEC_SYS
register space from the video decoder, and add a new property to hold
the phandle to the syscon, so that iospace can still be handled.

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>

---

(no changes since v4)

Changes in v4:
- Removed VDEC_SYS reg from mt8173 as well
- Reworded commit

Changes in v3:
- Removed the active clock
- Added a mediatek,vdecsys syscon property

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        | 28 ++++++++++++++++---
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
index 1e56ece44aee..b401c67e3ba0 100644
--- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
@@ -21,7 +21,22 @@ properties:
       - mediatek,mt8183-vcodec-dec
 
   reg:
-    maxItems: 12
+    minItems: 11
+    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
 
   interrupts:
     maxItems: 1
@@ -60,6 +75,10 @@ properties:
     description:
       Describes point to scp.
 
+  mediatek,vdecsys:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Phandle to the vdecsys syscon node.
+
 required:
   - compatible
   - reg
@@ -67,6 +86,7 @@ required:
   - clocks
   - clock-names
   - iommus
+  - mediatek,vdecsys
 
 allOf:
   - if:
@@ -126,10 +146,9 @@ examples:
     #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/power/mt8173-power.h>
 
-    vcodec_dec: vcodec@16000000 {
+    vcodec_dec: vcodec@16020000 {
       compatible = "mediatek,mt8173-vcodec-dec";
-      reg = <0x16000000 0x100>,   /*VDEC_SYS*/
-          <0x16020000 0x1000>,  /*VDEC_MISC*/
+      reg = <0x16020000 0x1000>,  /*VDEC_MISC*/
           <0x16021000 0x800>,   /*VDEC_LD*/
           <0x16021800 0x800>,   /*VDEC_TOP*/
           <0x16022000 0x1000>,  /*VDEC_CM*/
@@ -150,6 +169,7 @@ examples:
              <&iommu M4U_PORT_HW_VDEC_VLD_EXT>,
              <&iommu M4U_PORT_HW_VDEC_VLD2_EXT>;
       mediatek,vpu = <&vpu>;
+      mediatek,vdecsys = <&vdecsys>;
       power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
       clocks = <&apmixedsys CLK_APMIXED_VCODECPLL>,
              <&topckgen CLK_TOP_UNIVPLL_D2>,
-- 
2.41.0

[PATCH v5 4/7] media: mediatek: vcodec: Define address for VDEC_HW_ACTIVE

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

The VDEC_HW_ACTIVE bit is located at offset 0, bit 4 of the VDECSYS
iospace. Only the mask was previously defined, with the address being
implicit. Explicitly define the address, and append a '_MASK' suffix to
the mask, to make accesses to this bit clearer.

This commit brings no functional change.

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

---

(no changes since v4)

Changes in v4:
- Made use of BIT() macro for mask

Changes in v3:
- Added this commit

 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c | 4 ++--
 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c  | 4 ++--
 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.h  | 3 ++-
 3 files changed, 6 insertions(+), 5 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 d41f2121b94f..83780d29a9cf 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
@@ -50,8 +50,8 @@ static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv)
 	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) {
+	cg_status = readl(dev->reg_base[0] + VDEC_HW_ACTIVE_ADDR);
+	if ((cg_status & VDEC_HW_ACTIVE_MASK) != 0) {
 		mtk_v4l2_err("DEC ISR, VDEC active is not 0x0 (0x%08x)",
 			     cg_status);
 		return IRQ_HANDLED;
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 e1cb2f8dca33..41aa66c7295b 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c
@@ -75,8 +75,8 @@ static irqreturn_t mtk_vdec_hw_irq_handler(int irq, void *priv)
 	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) {
+	cg_status = readl(dev->reg_base[VDEC_HW_SYS] + VDEC_HW_ACTIVE_ADDR);
+	if (cg_status & VDEC_HW_ACTIVE_MASK) {
 		mtk_v4l2_err("vdec active is not 0x0 (0x%08x)",
 			     cg_status);
 		return IRQ_HANDLED;
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.h
index 36faa8d9d681..ff250e3be78e 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.h
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.h
@@ -12,7 +12,8 @@
 
 #include "mtk_vcodec_drv.h"
 
-#define VDEC_HW_ACTIVE 0x10
+#define VDEC_HW_ACTIVE_ADDR 0x0
+#define VDEC_HW_ACTIVE_MASK BIT(4)
 #define VDEC_IRQ_CFG 0x11
 #define VDEC_IRQ_CLR 0x10
 #define VDEC_IRQ_CFG_REG 0xa4
-- 
2.41.0

[PATCH v5 5/7] media: mediatek: vcodec: Read HW active status from syscon

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

Remove the requirement of a VDEC_SYS reg iospace for both MT8173 and
MT8183. To achieve that, rely on a vdecsys syscon to be passed through
the DT, and use it to directly read the VDEC_HW_ACTIVE bit during IRQ
handling to check whether the HW is active. Also update the VP8 stateful
decoder to use the syscon, if present, for writes to VDEC_SYS.

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>

---

Changes in v5:
- Added explicit linux/bitfield.h include for FIELD_GET(), following
  0day report

Changes in v4:
- Added new helper and updated VP8 stateful decoder to use it, so the
  syscon can also be used by mt8173
- Made handling cleaner
- Reworded commit

Changes in v3:
- Switched handling of VDEC_HW_ACTIVE to use a syscon instead of the
  'active' clock
- Reworded commit
- Removed changes to subdev part of driver, since they aren't used by
  MT8183

 .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 77 ++++++++++++++++---
 .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
 .../mediatek/vcodec/mtk_vcodec_util.c         | 15 ++++
 .../mediatek/vcodec/mtk_vcodec_util.h         |  2 +
 .../mediatek/vcodec/vdec/vdec_vp8_if.c        | 10 +--
 5 files changed, 87 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 83780d29a9cf..742b6903d030 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
@@ -5,13 +5,16 @@
  *         Tiffany Lin <tiffany.lin@mediatek.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/of.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-mem2mem.h>
 #include <media/videobuf2-dma-contig.h>
@@ -38,22 +41,30 @@ 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;
+
+	if (dev->vdecsys_regmap)
+		return !regmap_test_bits(dev->vdecsys_regmap, VDEC_HW_ACTIVE_ADDR,
+					 VDEC_HW_ACTIVE_MASK);
+
+	cg_status = readl(dev->reg_base[VDEC_SYS] + VDEC_HW_ACTIVE_ADDR);
+	return !FIELD_GET(VDEC_HW_ACTIVE_MASK, cg_status);
+}
+
 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] + VDEC_HW_ACTIVE_ADDR);
-	if ((cg_status & VDEC_HW_ACTIVE_MASK) != 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 +93,33 @@ 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 has_vdecsys_reg;
+	static const char * const mtk_dec_reg_names[] = {
+		"misc",
+		"ld",
+		"top",
+		"cm",
+		"ad",
+		"av",
+		"pp",
+		"hwd",
+		"hwq",
+		"hwb",
+		"hwg"
+	};
+
+	/*
+	 * If we have reg-names in devicetree, this means that we're on a new
+	 * register organization, which implies that the VDEC_SYS iospace gets
+	 * R/W through a syscon (regmap).
+	 * Here we try to get the "misc" iostart only to check if we have reg-names
+	 */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "misc");
+	if (res)
+		has_vdecsys_reg = false;
+	else
+		has_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 +129,29 @@ 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 (has_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]);
+		}
+	} 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, dev->reg_base[i]);
+			mtk_v4l2_debug(2, "reg[%d] base=%p", i+1, dev->reg_base[i+1]);
+		}
+
+		dev->vdecsys_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+								      "mediatek,vdecsys");
+		if (IS_ERR(dev->vdecsys_regmap)) {
+			dev_err(&pdev->dev, "Missing mediatek,vdecsys property");
+			return PTR_ERR(dev->vdecsys_regmap);
+		}
 	}
 
 	return 0;
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
index f17d67e781c9..0b430936f67d 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
@@ -489,6 +489,7 @@ struct mtk_vcodec_dev {
 	void __iomem *reg_base[NUM_MAX_VCODEC_REG_BASE];
 	const struct mtk_vcodec_dec_pdata *vdec_pdata;
 	const struct mtk_vcodec_enc_pdata *venc_pdata;
+	struct regmap *vdecsys_regmap;
 
 	struct mtk_vcodec_fw *fw_handler;
 
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
index f214e6f67005..8aaa5eb45444 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/regmap.h>
 
 #include "mtk_vcodec_dec_hw.h"
 #include "mtk_vcodec_drv.h"
@@ -34,6 +35,20 @@ void __iomem *mtk_vcodec_get_reg_addr(struct mtk_vcodec_ctx *data,
 }
 EXPORT_SYMBOL(mtk_vcodec_get_reg_addr);
 
+int mtk_vcodec_write_vdecsys(struct mtk_vcodec_ctx *ctx, unsigned int reg,
+			     unsigned int val)
+{
+	struct mtk_vcodec_dev *dev = ctx->dev;
+
+	if (dev->vdecsys_regmap)
+		return regmap_write(dev->vdecsys_regmap, reg, val);
+
+	writel(val, dev->reg_base[VDEC_SYS] + reg);
+
+	return 0;
+}
+EXPORT_SYMBOL(mtk_vcodec_write_vdecsys);
+
 int mtk_vcodec_mem_alloc(struct mtk_vcodec_ctx *data,
 			struct mtk_vcodec_mem *mem)
 {
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
index 88d389b65f13..c8bb4fc5153f 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
@@ -70,6 +70,8 @@ extern int mtk_vcodec_dbg;
 
 void __iomem *mtk_vcodec_get_reg_addr(struct mtk_vcodec_ctx *data,
 				unsigned int reg_idx);
+int mtk_vcodec_write_vdecsys(struct mtk_vcodec_ctx *ctx, unsigned int reg,
+			     unsigned int val);
 int mtk_vcodec_mem_alloc(struct mtk_vcodec_ctx *data,
 				struct mtk_vcodec_mem *mem);
 void mtk_vcodec_mem_free(struct mtk_vcodec_ctx *data,
diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
index 88c046731754..2592fa37b4c8 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
@@ -91,7 +91,6 @@ struct vdec_vp8_vsi {
 
 /**
  * struct vdec_vp8_hw_reg_base - HW register base
- * @sys		: base address for sys
  * @misc	: base address for misc
  * @ld		: base address for ld
  * @top		: base address for top
@@ -100,7 +99,6 @@ struct vdec_vp8_vsi {
  * @hwb		: base address for hwb
  */
 struct vdec_vp8_hw_reg_base {
-	void __iomem *sys;
 	void __iomem *misc;
 	void __iomem *ld;
 	void __iomem *top;
@@ -170,7 +168,6 @@ static void get_hw_reg_base(struct vdec_vp8_inst *inst)
 	inst->reg_base.top = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_TOP);
 	inst->reg_base.cm = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_CM);
 	inst->reg_base.hwd = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_HWD);
-	inst->reg_base.sys = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_SYS);
 	inst->reg_base.misc = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_MISC);
 	inst->reg_base.ld = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_LD);
 	inst->reg_base.hwb = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_HWB);
@@ -222,17 +219,16 @@ static void read_hw_segmentation_data(struct vdec_vp8_inst *inst)
 static void enable_hw_rw_function(struct vdec_vp8_inst *inst)
 {
 	u32 val = 0;
-	void __iomem *sys = inst->reg_base.sys;
 	void __iomem *misc = inst->reg_base.misc;
 	void __iomem *ld = inst->reg_base.ld;
 	void __iomem *hwb = inst->reg_base.hwb;
 	void __iomem *hwd = inst->reg_base.hwd;
 
-	writel(0x1, sys + VP8_RW_CKEN_SET);
+	mtk_vcodec_write_vdecsys(inst->ctx, VP8_RW_CKEN_SET, 0x1);
 	writel(0x101, ld + VP8_WO_VLD_SRST);
 	writel(0x101, hwb + VP8_WO_VLD_SRST);
 
-	writel(1, sys);
+	mtk_vcodec_write_vdecsys(inst->ctx, 0, 0x1);
 	val = readl(misc + VP8_RW_MISC_SRST);
 	writel((val & 0xFFFFFFFE), misc + VP8_RW_MISC_SRST);
 
@@ -241,7 +237,7 @@ static void enable_hw_rw_function(struct vdec_vp8_inst *inst)
 	writel(0x71201100, misc + VP8_RW_MISC_FUNC_CON);
 	writel(0x0, ld + VP8_WO_VLD_SRST);
 	writel(0x0, hwb + VP8_WO_VLD_SRST);
-	writel(0x1, sys + VP8_RW_DCM_CON);
+	mtk_vcodec_write_vdecsys(inst->ctx, VP8_RW_DCM_CON, 0x1);
 	writel(0x1, misc + VP8_RW_MISC_DCM_CON);
 	writel(0x1, hwd + VP8_RW_VP8_CTRL);
 }
-- 
2.41.0

[PATCH v5 6/7] arm64: dts: mediatek: mt8173: Drop VDEC_SYS reg from decoder

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

Remove the VDEC_SYS register space from the decoder, so that the node
address becomes that of VDEC_MISC, solving the long-standing conflicting
addresses between this node and the vdecsys clock-controller node:

arch/arm64/boot/dts/mediatek/mt8173.dtsi:1365.38-1369.5: Warning (unique_unit_address_if_enabled): /soc/clock-controller@16000000: duplicate unit-address (also used in node /soc/vcodec@16000000)

The driver makes use of this register space, however, so also add a
phandle to the VDEC_SYS syscon to maintain functionality.

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

---

(no changes since v4)

Changes in v4:
- Added this commit

 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index c47d7d900f28..cac4cd0a0320 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -1368,10 +1368,9 @@ vdecsys: clock-controller@16000000 {
 			#clock-cells = <1>;
 		};
 
-		vcodec_dec: vcodec@16000000 {
+		vcodec_dec: vcodec@16020000 {
 			compatible = "mediatek,mt8173-vcodec-dec";
-			reg = <0 0x16000000 0 0x100>,	/* VDEC_SYS */
-			      <0 0x16020000 0 0x1000>,	/* VDEC_MISC */
+			reg = <0 0x16020000 0 0x1000>,	/* VDEC_MISC */
 			      <0 0x16021000 0 0x800>,	/* VDEC_LD */
 			      <0 0x16021800 0 0x800>,	/* VDEC_TOP */
 			      <0 0x16022000 0 0x1000>,	/* VDEC_CM */
@@ -1382,6 +1381,8 @@ vcodec_dec: vcodec@16000000 {
 			      <0 0x16027000 0 0x800>,	/* VDEC_HWQ */
 			      <0 0x16027800 0 0x800>,	/* VDEC_HWB */
 			      <0 0x16028400 0 0x400>;	/* VDEC_HWG */
+			reg-names = "misc", "ld", "top", "cm", "ad", "av", "pp",
+				    "hwd", "hwq", "hwb", "hwg";
 			interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_LOW>;
 			iommus = <&iommu M4U_PORT_HW_VDEC_MC_EXT>,
 				 <&iommu M4U_PORT_HW_VDEC_PP_EXT>,
@@ -1392,6 +1393,7 @@ vcodec_dec: vcodec@16000000 {
 				 <&iommu M4U_PORT_HW_VDEC_VLD_EXT>,
 				 <&iommu M4U_PORT_HW_VDEC_VLD2_EXT>;
 			mediatek,vpu = <&vpu>;
+			mediatek,vdecsys = <&vdecsys>;
 			power-domains = <&spm MT8173_POWER_DOMAIN_VDEC>;
 			clocks = <&apmixedsys CLK_APMIXED_VCODECPLL>,
 				 <&topckgen CLK_TOP_UNIVPLL_D2>,
-- 
2.41.0

[PATCH v5 7/7] 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>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

---

(no changes since v3)

Changes in v3:
- Dropped 'active' clock and added the 'mediatek,vdecsys' syscon phandle
  property instead

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..4144f1ed3ff0 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>;
+			mediatek,vdecsys = <&vdecsys>;
+			power-domains = <&spm MT8183_POWER_DOMAIN_VDEC>;
+			clocks = <&vdecsys CLK_VDEC_VDEC>;
+			clock-names = "vdec";
+		};
+
 		larb1: larb@16010000 {
 			compatible = "mediatek,mt8183-smi-larb";
 			reg = <0 0x16010000 0 0x1000>;
-- 
2.41.0

Re: [PATCH v5 3/7] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS register space

[Krzysztof Kozlowski]

On 30/06/2023 17:14, Nícolas F. R. A. Prado wrote:
> The binding expects the first register space to be VDEC_SYS. However
> this register space is already assigned to a different node on both
> MT8173 and MT8183: a clock-controller node called 'vdecsys' which is
> also a syscon.
> 
> In order to resolve the overlapping address ranges, remove the VDEC_SYS
> register space from the video decoder, and add a new property to hold
> the phandle to the syscon, so that iospace can still be handled.
> 
> 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: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v5 5/7] media: mediatek: vcodec: Read HW active status from syscon

[Hans Verkuil]

On 30/06/2023 17:14, Nícolas F. R. A. Prado wrote:
> Remove the requirement of a VDEC_SYS reg iospace for both MT8173 and
> MT8183. To achieve that, rely on a vdecsys syscon to be passed through
> the DT, and use it to directly read the VDEC_HW_ACTIVE bit during IRQ
> handling to check whether the HW is active. Also update the VP8 stateful
> decoder to use the syscon, if present, for writes to VDEC_SYS.
> 
> 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>
> 
> ---
> 
> Changes in v5:
> - Added explicit linux/bitfield.h include for FIELD_GET(), following
>   0day report
> 
> Changes in v4:
> - Added new helper and updated VP8 stateful decoder to use it, so the
>   syscon can also be used by mt8173
> - Made handling cleaner
> - Reworded commit
> 
> Changes in v3:
> - Switched handling of VDEC_HW_ACTIVE to use a syscon instead of the
>   'active' clock
> - Reworded commit
> - Removed changes to subdev part of driver, since they aren't used by
>   MT8183
> 
>  .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 77 ++++++++++++++++---
>  .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
>  .../mediatek/vcodec/mtk_vcodec_util.c         | 15 ++++
>  .../mediatek/vcodec/mtk_vcodec_util.h         |  2 +
>  .../mediatek/vcodec/vdec/vdec_vp8_if.c        | 10 +--
>  5 files changed, 87 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 83780d29a9cf..742b6903d030 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> @@ -5,13 +5,16 @@
>   *         Tiffany Lin <tiffany.lin@mediatek.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/of.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-mem2mem.h>
>  #include <media/videobuf2-dma-contig.h>
> @@ -38,22 +41,30 @@ 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;
> +
> +	if (dev->vdecsys_regmap)
> +		return !regmap_test_bits(dev->vdecsys_regmap, VDEC_HW_ACTIVE_ADDR,
> +					 VDEC_HW_ACTIVE_MASK);
> +
> +	cg_status = readl(dev->reg_base[VDEC_SYS] + VDEC_HW_ACTIVE_ADDR);
> +	return !FIELD_GET(VDEC_HW_ACTIVE_MASK, cg_status);
> +}
> +
>  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] + VDEC_HW_ACTIVE_ADDR);
> -	if ((cg_status & VDEC_HW_ACTIVE_MASK) != 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 +93,33 @@ 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 has_vdecsys_reg;
> +	static const char * const mtk_dec_reg_names[] = {
> +		"misc",
> +		"ld",
> +		"top",
> +		"cm",
> +		"ad",
> +		"av",
> +		"pp",
> +		"hwd",
> +		"hwq",
> +		"hwb",
> +		"hwg"
> +	};
> +
> +	/*
> +	 * If we have reg-names in devicetree, this means that we're on a new
> +	 * register organization, which implies that the VDEC_SYS iospace gets
> +	 * R/W through a syscon (regmap).
> +	 * Here we try to get the "misc" iostart only to check if we have reg-names
> +	 */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "misc");
> +	if (res)
> +		has_vdecsys_reg = false;
> +	else
> +		has_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 +129,29 @@ 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 (has_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]);
> +		}
> +	} 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, dev->reg_base[i]);
> +			mtk_v4l2_debug(2, "reg[%d] base=%p", i+1, dev->reg_base[i+1]);
> +		}
> +
> +		dev->vdecsys_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +								      "mediatek,vdecsys");
> +		if (IS_ERR(dev->vdecsys_regmap)) {
> +			dev_err(&pdev->dev, "Missing mediatek,vdecsys property");
> +			return PTR_ERR(dev->vdecsys_regmap);
> +		}
>  	}
>  
>  	return 0;
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> index f17d67e781c9..0b430936f67d 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> @@ -489,6 +489,7 @@ struct mtk_vcodec_dev {
>  	void __iomem *reg_base[NUM_MAX_VCODEC_REG_BASE];
>  	const struct mtk_vcodec_dec_pdata *vdec_pdata;
>  	const struct mtk_vcodec_enc_pdata *venc_pdata;
> +	struct regmap *vdecsys_regmap;

You forgot to add the kerneldoc documentation for this new field.

If you just give me the documentation of this field then I can modify the
patch. That's actually easier for me.

Regards,

	Hans

Re: [PATCH v5 5/7] media: mediatek: vcodec: Read HW active status from syscon

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

On Fri, Jul 21, 2023 at 01:44:10PM +0200, Hans Verkuil wrote:
> On 30/06/2023 17:14, Nícolas F. R. A. Prado wrote:
[..]
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > @@ -489,6 +489,7 @@ struct mtk_vcodec_dev {
> >  	void __iomem *reg_base[NUM_MAX_VCODEC_REG_BASE];
> >  	const struct mtk_vcodec_dec_pdata *vdec_pdata;
> >  	const struct mtk_vcodec_enc_pdata *venc_pdata;
> > +	struct regmap *vdecsys_regmap;
> 
> You forgot to add the kerneldoc documentation for this new field.
> 
> If you just give me the documentation of this field then I can modify the
> patch. That's actually easier for me.

Sorry about that. Seems like I'm not getting a kerneldoc warning due to that,
I'll look into why so I can catch this next time.

This is the documentation:

@vdecsys_regmap: VDEC_SYS register space passed through syscon

Or if a patch that applies on top would make it easier to fixup:

cat 0001-media-mediatek-vcodec-Add-missing-kerneldoc-for-vdec.patch
From ee6a6d619dbe60c8f4947188a9b9bbeafc3616f7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?N=C3=ADcolas=20F=2E=20R=2E=20A=2E=20Prado?=
 <nfraprado@collabora.com>
Date: Fri, 21 Jul 2023 16:33:54 +0200
Subject: [PATCH] media: mediatek: vcodec: Add missing kerneldoc for
 vdecsys_regmap
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
 drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
index 0b430936f67d..c38eb62bc72a 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
@@ -441,6 +441,7 @@ struct mtk_vcodec_enc_pdata {
  * @reg_base: Mapped address of MTK Vcodec registers.
  * @vdec_pdata: decoder IC-specific data
  * @venc_pdata: encoder IC-specific data
+ * @vdecsys_regmap: VDEC_SYS register space passed through syscon
  *
  * @fw_handler: used to communicate with the firmware.
  * @id_counter: used to identify current opened instance
--
2.30.2

Thanks,
Nícolas

Re: [PATCH v5 5/7] media: mediatek: vcodec: Read HW active status from syscon

[Hans Verkuil]

Hi Nicolas,

On 30/06/2023 17:14, Nícolas F. R. A. Prado wrote:
> Remove the requirement of a VDEC_SYS reg iospace for both MT8173 and
> MT8183. To achieve that, rely on a vdecsys syscon to be passed through
> the DT, and use it to directly read the VDEC_HW_ACTIVE bit during IRQ
> handling to check whether the HW is active. Also update the VP8 stateful
> decoder to use the syscon, if present, for writes to VDEC_SYS.
> 
> 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>
> 
> ---
> 
> Changes in v5:
> - Added explicit linux/bitfield.h include for FIELD_GET(), following
>   0day report
> 
> Changes in v4:
> - Added new helper and updated VP8 stateful decoder to use it, so the
>   syscon can also be used by mt8173
> - Made handling cleaner
> - Reworded commit
> 
> Changes in v3:
> - Switched handling of VDEC_HW_ACTIVE to use a syscon instead of the
>   'active' clock
> - Reworded commit
> - Removed changes to subdev part of driver, since they aren't used by
>   MT8183
> 
>  .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 77 ++++++++++++++++---
>  .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
>  .../mediatek/vcodec/mtk_vcodec_util.c         | 15 ++++
>  .../mediatek/vcodec/mtk_vcodec_util.h         |  2 +
>  .../mediatek/vcodec/vdec/vdec_vp8_if.c        | 10 +--
>  5 files changed, 87 insertions(+), 18 deletions(-)

This patch introduced this new smatch error:

drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c:143 mtk_vcodec_get_reg_bases() error: buffer overflow 'mtk_dec_reg_names' 11 <= 11

I think it is due to:

if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE) {

in mtk_vcodec_get_reg_bases(): the '>' should probably be '>='.

Can you post a follow-up patch fixing this?

Regards,

	Hans

> 
> 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 83780d29a9cf..742b6903d030 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> @@ -5,13 +5,16 @@
>   *         Tiffany Lin <tiffany.lin@mediatek.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/of.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-mem2mem.h>
>  #include <media/videobuf2-dma-contig.h>
> @@ -38,22 +41,30 @@ 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;
> +
> +	if (dev->vdecsys_regmap)
> +		return !regmap_test_bits(dev->vdecsys_regmap, VDEC_HW_ACTIVE_ADDR,
> +					 VDEC_HW_ACTIVE_MASK);
> +
> +	cg_status = readl(dev->reg_base[VDEC_SYS] + VDEC_HW_ACTIVE_ADDR);
> +	return !FIELD_GET(VDEC_HW_ACTIVE_MASK, cg_status);
> +}
> +
>  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] + VDEC_HW_ACTIVE_ADDR);
> -	if ((cg_status & VDEC_HW_ACTIVE_MASK) != 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 +93,33 @@ 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 has_vdecsys_reg;
> +	static const char * const mtk_dec_reg_names[] = {
> +		"misc",
> +		"ld",
> +		"top",
> +		"cm",
> +		"ad",
> +		"av",
> +		"pp",
> +		"hwd",
> +		"hwq",
> +		"hwb",
> +		"hwg"
> +	};
> +
> +	/*
> +	 * If we have reg-names in devicetree, this means that we're on a new
> +	 * register organization, which implies that the VDEC_SYS iospace gets
> +	 * R/W through a syscon (regmap).
> +	 * Here we try to get the "misc" iostart only to check if we have reg-names
> +	 */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "misc");
> +	if (res)
> +		has_vdecsys_reg = false;
> +	else
> +		has_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 +129,29 @@ 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 (has_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]);
> +		}
> +	} 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, dev->reg_base[i]);
> +			mtk_v4l2_debug(2, "reg[%d] base=%p", i+1, dev->reg_base[i+1]);
> +		}
> +
> +		dev->vdecsys_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +								      "mediatek,vdecsys");
> +		if (IS_ERR(dev->vdecsys_regmap)) {
> +			dev_err(&pdev->dev, "Missing mediatek,vdecsys property");
> +			return PTR_ERR(dev->vdecsys_regmap);
> +		}
>  	}
>  
>  	return 0;
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> index f17d67e781c9..0b430936f67d 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> @@ -489,6 +489,7 @@ struct mtk_vcodec_dev {
>  	void __iomem *reg_base[NUM_MAX_VCODEC_REG_BASE];
>  	const struct mtk_vcodec_dec_pdata *vdec_pdata;
>  	const struct mtk_vcodec_enc_pdata *venc_pdata;
> +	struct regmap *vdecsys_regmap;
>  
>  	struct mtk_vcodec_fw *fw_handler;
>  
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
> index f214e6f67005..8aaa5eb45444 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
> @@ -8,6 +8,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/regmap.h>
>  
>  #include "mtk_vcodec_dec_hw.h"
>  #include "mtk_vcodec_drv.h"
> @@ -34,6 +35,20 @@ void __iomem *mtk_vcodec_get_reg_addr(struct mtk_vcodec_ctx *data,
>  }
>  EXPORT_SYMBOL(mtk_vcodec_get_reg_addr);
>  
> +int mtk_vcodec_write_vdecsys(struct mtk_vcodec_ctx *ctx, unsigned int reg,
> +			     unsigned int val)
> +{
> +	struct mtk_vcodec_dev *dev = ctx->dev;
> +
> +	if (dev->vdecsys_regmap)
> +		return regmap_write(dev->vdecsys_regmap, reg, val);
> +
> +	writel(val, dev->reg_base[VDEC_SYS] + reg);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mtk_vcodec_write_vdecsys);
> +
>  int mtk_vcodec_mem_alloc(struct mtk_vcodec_ctx *data,
>  			struct mtk_vcodec_mem *mem)
>  {
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> index 88d389b65f13..c8bb4fc5153f 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> @@ -70,6 +70,8 @@ extern int mtk_vcodec_dbg;
>  
>  void __iomem *mtk_vcodec_get_reg_addr(struct mtk_vcodec_ctx *data,
>  				unsigned int reg_idx);
> +int mtk_vcodec_write_vdecsys(struct mtk_vcodec_ctx *ctx, unsigned int reg,
> +			     unsigned int val);
>  int mtk_vcodec_mem_alloc(struct mtk_vcodec_ctx *data,
>  				struct mtk_vcodec_mem *mem);
>  void mtk_vcodec_mem_free(struct mtk_vcodec_ctx *data,
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
> index 88c046731754..2592fa37b4c8 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
> @@ -91,7 +91,6 @@ struct vdec_vp8_vsi {
>  
>  /**
>   * struct vdec_vp8_hw_reg_base - HW register base
> - * @sys		: base address for sys
>   * @misc	: base address for misc
>   * @ld		: base address for ld
>   * @top		: base address for top
> @@ -100,7 +99,6 @@ struct vdec_vp8_vsi {
>   * @hwb		: base address for hwb
>   */
>  struct vdec_vp8_hw_reg_base {
> -	void __iomem *sys;
>  	void __iomem *misc;
>  	void __iomem *ld;
>  	void __iomem *top;
> @@ -170,7 +168,6 @@ static void get_hw_reg_base(struct vdec_vp8_inst *inst)
>  	inst->reg_base.top = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_TOP);
>  	inst->reg_base.cm = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_CM);
>  	inst->reg_base.hwd = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_HWD);
> -	inst->reg_base.sys = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_SYS);
>  	inst->reg_base.misc = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_MISC);
>  	inst->reg_base.ld = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_LD);
>  	inst->reg_base.hwb = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_HWB);
> @@ -222,17 +219,16 @@ static void read_hw_segmentation_data(struct vdec_vp8_inst *inst)
>  static void enable_hw_rw_function(struct vdec_vp8_inst *inst)
>  {
>  	u32 val = 0;
> -	void __iomem *sys = inst->reg_base.sys;
>  	void __iomem *misc = inst->reg_base.misc;
>  	void __iomem *ld = inst->reg_base.ld;
>  	void __iomem *hwb = inst->reg_base.hwb;
>  	void __iomem *hwd = inst->reg_base.hwd;
>  
> -	writel(0x1, sys + VP8_RW_CKEN_SET);
> +	mtk_vcodec_write_vdecsys(inst->ctx, VP8_RW_CKEN_SET, 0x1);
>  	writel(0x101, ld + VP8_WO_VLD_SRST);
>  	writel(0x101, hwb + VP8_WO_VLD_SRST);
>  
> -	writel(1, sys);
> +	mtk_vcodec_write_vdecsys(inst->ctx, 0, 0x1);
>  	val = readl(misc + VP8_RW_MISC_SRST);
>  	writel((val & 0xFFFFFFFE), misc + VP8_RW_MISC_SRST);
>  
> @@ -241,7 +237,7 @@ static void enable_hw_rw_function(struct vdec_vp8_inst *inst)
>  	writel(0x71201100, misc + VP8_RW_MISC_FUNC_CON);
>  	writel(0x0, ld + VP8_WO_VLD_SRST);
>  	writel(0x0, hwb + VP8_WO_VLD_SRST);
> -	writel(0x1, sys + VP8_RW_DCM_CON);
> +	mtk_vcodec_write_vdecsys(inst->ctx, VP8_RW_DCM_CON, 0x1);
>  	writel(0x1, misc + VP8_RW_MISC_DCM_CON);
>  	writel(0x1, hwd + VP8_RW_VP8_CTRL);
>  }

Re: [PATCH v5 5/7] media: mediatek: vcodec: Read HW active status from syscon

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

On Tue, Jul 25, 2023 at 12:15:03PM +0200, Hans Verkuil wrote:
> Hi Nicolas,
> 
> On 30/06/2023 17:14, Nícolas F. R. A. Prado wrote:
> > Remove the requirement of a VDEC_SYS reg iospace for both MT8173 and
> > MT8183. To achieve that, rely on a vdecsys syscon to be passed through
> > the DT, and use it to directly read the VDEC_HW_ACTIVE bit during IRQ
> > handling to check whether the HW is active. Also update the VP8 stateful
> > decoder to use the syscon, if present, for writes to VDEC_SYS.
> > 
> > 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>
> > 
> > ---
> > 
> > Changes in v5:
> > - Added explicit linux/bitfield.h include for FIELD_GET(), following
> >   0day report
> > 
> > Changes in v4:
> > - Added new helper and updated VP8 stateful decoder to use it, so the
> >   syscon can also be used by mt8173
> > - Made handling cleaner
> > - Reworded commit
> > 
> > Changes in v3:
> > - Switched handling of VDEC_HW_ACTIVE to use a syscon instead of the
> >   'active' clock
> > - Reworded commit
> > - Removed changes to subdev part of driver, since they aren't used by
> >   MT8183
> > 
> >  .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 77 ++++++++++++++++---
> >  .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
> >  .../mediatek/vcodec/mtk_vcodec_util.c         | 15 ++++
> >  .../mediatek/vcodec/mtk_vcodec_util.h         |  2 +
> >  .../mediatek/vcodec/vdec/vdec_vp8_if.c        | 10 +--
> >  5 files changed, 87 insertions(+), 18 deletions(-)
> 
> This patch introduced this new smatch error:
> 
> drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c:143 mtk_vcodec_get_reg_bases() error: buffer overflow 'mtk_dec_reg_names' 11 <= 11
> 
> I think it is due to:
> 
> if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE) {
> 
> in mtk_vcodec_get_reg_bases(): the '>' should probably be '>='.
> 
> Can you post a follow-up patch fixing this?

Hi Hans,

sorry about that, and thanks for noticing it.

I've just sent the fix:
https://lore.kernel.org/all/20230725204043.569799-1-nfraprado@collabora.com

Thanks,
Nícolas

Re: [PATCH v5 6/7] arm64: dts: mediatek: mt8173: Drop VDEC_SYS reg from decoder

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

On Fri, Jun 30, 2023 at 11:14:12AM -0400, Nícolas F. R. A. Prado wrote:
> Remove the VDEC_SYS register space from the decoder, so that the node
> address becomes that of VDEC_MISC, solving the long-standing conflicting
> addresses between this node and the vdecsys clock-controller node:
> 
> arch/arm64/boot/dts/mediatek/mt8173.dtsi:1365.38-1369.5: Warning (unique_unit_address_if_enabled): /soc/clock-controller@16000000: duplicate unit-address (also used in node /soc/vcodec@16000000)
> 
> The driver makes use of this register space, however, so also add a
> phandle to the VDEC_SYS syscon to maintain functionality.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Hi Matthias,

just to let you know that Hans has already picked up patches 1-5, and they're on
their way to the media tree:
https://lore.kernel.org/all/af0772c6-7052-ce13-dbf3-d403b06aad02@xs4all.nl

So feel free to pick patches 6 and 7. Though note that these DT changes depend
on those driver patches (commits 1-5) to work.

Thanks,
Nícolas