[PATCH v3 0/6] Enable decoder for mt8183

[PATCH v3 0/6] Enable decoder for mt8183

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

This series enables the hardware decoder present on mt8183. At first
glance, the only missing piece is the devicetree node for it, however,
simply adding it as is would cause an address collision between the
first register iospace and the clock-controller node, so a rework of the
dt-binding and driver, as well as addition of a 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 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 (5):
  media: dt-bindings: mediatek,vcodec: Allow single clock for mt8183
  media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks
  media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183
  media: mediatek: vcodec: Define address for VDEC_HW_ACTIVE
  media: mediatek: vcodec: Read HW active status from syscon on MT8183

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

 .../media/mediatek,vcodec-decoder.yaml        | 69 +++++++++++++++---
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 30 ++++++++
 .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 71 ++++++++++++++++---
 .../mediatek/vcodec/mtk_vcodec_dec_hw.c       |  4 +-
 .../mediatek/vcodec/mtk_vcodec_dec_hw.h       |  3 +-
 .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
 6 files changed, 153 insertions(+), 25 deletions(-)

-- 
2.41.0

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

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

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

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

---

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 v3 2/6] media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks

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

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

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
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 v3 3/6] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183

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

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

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

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

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

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

---
I dropped the tags from this commit since a syscon is now used instead
of an extra clock.

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        | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
index 1e56ece44aee..2f625c50bbfe 100644
--- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
@@ -21,8 +21,13 @@ properties:
       - mediatek,mt8183-vcodec-dec
 
   reg:
+    minItems: 11
     maxItems: 12
 
+  reg-names:
+    minItems: 11
+    maxItems: 11
+
   interrupts:
     maxItems: 1
 
@@ -60,6 +65,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
@@ -79,8 +88,26 @@ allOf:
     then:
       required:
         - mediatek,scp
+        - mediatek,vdecsys
 
       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: 1
           maxItems: 1
@@ -101,6 +128,9 @@ allOf:
         - mediatek,vpu
 
       properties:
+        reg:
+          minItems: 12
+
         clocks:
           minItems: 8
           maxItems: 8
-- 
2.41.0

[PATCH v3 4/6] 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>

---

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..caa2d0a48a90 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 0x10
 #define VDEC_IRQ_CFG 0x11
 #define VDEC_IRQ_CLR 0x10
 #define VDEC_IRQ_CFG_REG 0xa4
-- 
2.41.0

[PATCH v3 5/6] media: mediatek: vcodec: Read HW active status from syscon on MT8183

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

Remove the requirement of a VDEC_SYS reg iospace for 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.

The old behavior is still present when reg-names aren't supplied, as
MT8173 still relies on it.

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

---
I dropped the tags from this commit since a syscon is now used instead
of an extra clock.

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      | 71 ++++++++++++++++---
 .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
 2 files changed, 61 insertions(+), 11 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..387ed26d6d5d 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
@@ -8,10 +8,12 @@
 #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 +40,37 @@ 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;
+	int val, ret;
+
+	if (!dev->reg_base[VDEC_SYS]) {
+		ret = regmap_read(dev->vdecsys_regmap, VDEC_HW_ACTIVE_ADDR, &val);
+		if (ret) {
+			mtk_v4l2_err("Failed to read VDEC active status");
+			return false;
+		}
+
+		return (val & VDEC_HW_ACTIVE_MASK) == 0;
+	}
+
+	cg_status = readl(dev->reg_base[VDEC_SYS] + VDEC_HW_ACTIVE_ADDR);
+	return (cg_status & VDEC_HW_ACTIVE_MASK) == 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] + 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 +99,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 +127,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]);
+		}
+	} 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]);
+		}
 	}
 
 	return 0;
@@ -118,6 +164,9 @@ static int mtk_vcodec_init_dec_resources(struct mtk_vcodec_dev *dev)
 	if (dev->dec_irq < 0)
 		return dev->dec_irq;
 
+	dev->vdecsys_regmap = syscon_regmap_lookup_by_phandle_optional(pdev->dev.of_node,
+								       "mediatek,vdecsys");
+
 	irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
 	ret = devm_request_irq(&pdev->dev, dev->dec_irq,
 			       mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
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;
 
-- 
2.41.0

Re: [PATCH v3 4/6] media: mediatek: vcodec: Define address for VDEC_HW_ACTIVE

[AngeloGioacchino Del Regno]

Il 20/06/23 02:03, Nícolas F. R. A. Prado ha scritto:
> 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>
> 
> ---
> 
> 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(-)
> 

..snip..

> 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..caa2d0a48a90 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 0x10

#define VDEC_HW_ACTIVE_MASK	BIT(4)

...because it's a bit, as you wrote in the commit description :-)

Cheers,
Angelo

>   #define VDEC_IRQ_CFG 0x11
>   #define VDEC_IRQ_CLR 0x10
>   #define VDEC_IRQ_CFG_REG 0xa4

Re: [PATCH v3 5/6] media: mediatek: vcodec: Read HW active status from syscon on MT8183

[AngeloGioacchino Del Regno]

Il 20/06/23 02:03, Nícolas F. R. A. Prado ha scritto:
> Remove the requirement of a VDEC_SYS reg iospace for 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.
> 
> The old behavior is still present when reg-names aren't supplied, as
> MT8173 still relies on it.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> ---
> I dropped the tags from this commit since a syscon is now used instead
> of an extra clock.
> 
> 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      | 71 ++++++++++++++++---
>   .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
>   2 files changed, 61 insertions(+), 11 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..387ed26d6d5d 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> @@ -8,10 +8,12 @@
>   #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 +40,37 @@ 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;
> +	int val, ret;
> +
> +	if (!dev->reg_base[VDEC_SYS]) {
> +		ret = regmap_read(dev->vdecsys_regmap, VDEC_HW_ACTIVE_ADDR, &val);
> +		if (ret) {
> +			mtk_v4l2_err("Failed to read VDEC active status");
> +			return false;
> +		}
> +
> +		return (val & VDEC_HW_ACTIVE_MASK) == 0;
> +	}
> +
> +	cg_status = readl(dev->reg_base[VDEC_SYS] + VDEC_HW_ACTIVE_ADDR);
> +	return (cg_status & VDEC_HW_ACTIVE_MASK) == 0;

You can either do...

{
	if (dev->vdecsys_regmap) {
		ret = regmap_read(......., &cg_status);
		if (ret) {
			mtk_v4l2_err(...)
			return false;
		}
	} else {
		cg_status = readl(....)
	}
	return !FIELD_GET(VDEC_HW_ACTIVE_MASK, cg_status);
}

.... or ....

{
	if (dev->vdecsys_regmap)
		return !regmap_test_bits(dev->vdecsys_regmap, VDEC_HW_ACTIVE_ADDR,
					 VDEC_HW_ACTIVE_MASK);

	cg_status = readl(....);
	return !FIELD_GET(VDEC_HW_ACTIVE_MASK, cg_status);
}

That's way cleaner :-)

> +}
> +
>   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 +99,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;

bool has_vdecsys_reg;

> +	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 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;

> +	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 +127,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) {

...so here you invert the branch

	if (has_vdecsys_reg) {
		.... byname ioremap ....
		parse syscon regmap here, not later!
	} else {
		.... by id ioremap ....
	}

> +		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]);
> +		}
>   	}
>   
>   	return 0;
> @@ -118,6 +164,9 @@ static int mtk_vcodec_init_dec_resources(struct mtk_vcodec_dev *dev)
>   	if (dev->dec_irq < 0)
>   		return dev->dec_irq;
>   
> +	dev->vdecsys_regmap = syscon_regmap_lookup_by_phandle_optional(pdev->dev.of_node,
> +								       "mediatek,vdecsys");
> +

It makes no sense to try to get a handle to this syscon if we're on the older
layout with vdecsys in the `reg` list: in that case, we can safely assume that
we don't have any mediatek,vdecsys syscon.

Cheers,
Angelo

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

[Krzysztof Kozlowski]

On 20/06/2023 02:03, Nícolas F. R. A. Prado wrote:
> MT8173 and MT8183 have different clocks, and consequently clock-names.
> Relax the number of clocks and set clock-names based on compatible.
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> 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 v3 3/6] media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183

[Krzysztof Kozlowski]

On 20/06/2023 02:03, 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 hardware controlled clock, remove
> the VDEC_SYS register space from the binding and describe an extra
> syscon that will be used to directly check the hardware status.
> 
> Also add reg-names to be able to tell that this new register schema is
> used, so the driver can keep backward compatibility.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> ---
> I dropped the tags from this commit since a syscon is now used instead
> of an extra clock.
> 
> 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        | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> index 1e56ece44aee..2f625c50bbfe 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> @@ -21,8 +21,13 @@ properties:
>        - mediatek,mt8183-vcodec-dec
>  
>    reg:
> +    minItems: 11
>      maxItems: 12
>  
> +  reg-names:
> +    minItems: 11
> +    maxItems: 11

maxItems: 12

> +
>    interrupts:
>      maxItems: 1
>  
> @@ -60,6 +65,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
> @@ -79,8 +88,26 @@ allOf:
>      then:
>        required:
>          - mediatek,scp
> +        - mediatek,vdecsys
>  
>        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: 1
>            maxItems: 1
> @@ -101,6 +128,9 @@ allOf:
>          - mediatek,vpu
>  
>        properties:
> +        reg:
> +          minItems: 12


What about reg-names here? They should be also defined and in sync with
regs.

Best regards,
Krzysztof

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

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

On Tue, Jun 20, 2023 at 10:12:14AM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2023 02:03, 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 hardware controlled clock, remove
> > the VDEC_SYS register space from the binding and describe an extra
> > syscon that will be used to directly check the hardware status.
> > 
> > Also add reg-names to be able to tell that this new register schema is
> > used, so the driver can keep backward compatibility.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > 
> > ---
> > I dropped the tags from this commit since a syscon is now used instead
> > of an extra clock.
> > 
> > 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        | 30 +++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> > index 1e56ece44aee..2f625c50bbfe 100644
> > --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> > @@ -21,8 +21,13 @@ properties:
> >        - mediatek,mt8183-vcodec-dec
> >  
> >    reg:
> > +    minItems: 11
> >      maxItems: 12
> >  
> > +  reg-names:
> > +    minItems: 11
> > +    maxItems: 11
> 
> maxItems: 12
> 
> > +
> >    interrupts:
> >      maxItems: 1
> >  
> > @@ -60,6 +65,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
> > @@ -79,8 +88,26 @@ allOf:
> >      then:
> >        required:
> >          - mediatek,scp
> > +        - mediatek,vdecsys
> >  
> >        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: 1
> >            maxItems: 1
> > @@ -101,6 +128,9 @@ allOf:
> >          - mediatek,vpu
> >  
> >        properties:
> > +        reg:
> > +          minItems: 12
> 
> 
> What about reg-names here? They should be also defined and in sync with
> regs.

That's intentional. As described in the commit message, mt8173 will keep having
the VDEC_SYS iospace, while mt8183 won't. And we use the presence of reg-names
to tell them apart.

So, mt8173 has 12 regs, no reg-names and no syscon, while mt8183 has 11 regs,
with reg-names and the syscon.

Thanks,
Nícolas

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

[Krzysztof Kozlowski]

On 20/06/2023 14:46, Nícolas F. R. A. Prado wrote:
> On Tue, Jun 20, 2023 at 10:12:14AM +0200, Krzysztof Kozlowski wrote:
>> On 20/06/2023 02:03, 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 hardware controlled clock, remove
>>> the VDEC_SYS register space from the binding and describe an extra
>>> syscon that will be used to directly check the hardware status.
>>>
>>> Also add reg-names to be able to tell that this new register schema is
>>> used, so the driver can keep backward compatibility.
>>>
>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>>
>>> ---
>>> I dropped the tags from this commit since a syscon is now used instead
>>> of an extra clock.
>>>
>>> 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        | 30 +++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
>>> index 1e56ece44aee..2f625c50bbfe 100644
>>> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
>>> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
>>> @@ -21,8 +21,13 @@ properties:
>>>        - mediatek,mt8183-vcodec-dec
>>>  
>>>    reg:
>>> +    minItems: 11
>>>      maxItems: 12
>>>  
>>> +  reg-names:
>>> +    minItems: 11
>>> +    maxItems: 11
>>
>> maxItems: 12
>>
>>> +
>>>    interrupts:
>>>      maxItems: 1
>>>  
>>> @@ -60,6 +65,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
>>> @@ -79,8 +88,26 @@ allOf:
>>>      then:
>>>        required:
>>>          - mediatek,scp
>>> +        - mediatek,vdecsys
>>>  
>>>        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: 1
>>>            maxItems: 1
>>> @@ -101,6 +128,9 @@ allOf:
>>>          - mediatek,vpu
>>>  
>>>        properties:
>>> +        reg:
>>> +          minItems: 12
>>
>>
>> What about reg-names here? They should be also defined and in sync with
>> regs.
> 
> That's intentional. As described in the commit message, mt8173 will keep having
> the VDEC_SYS iospace, while mt8183 won't. And we use the presence of reg-names
> to tell them apart.
> 
> So, mt8173 has 12 regs, no reg-names and no syscon, while mt8183 has 11 regs,
> with reg-names and the syscon.

reg-names is not the way to tell apart variants. Compatible is. If you
add reg-names for one variant, it's expected to have it defined for
other as well, because the order of items in reg is always fixed.

Best regards,
Krzysztof

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

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

On Tue, Jun 20, 2023 at 03:00:00PM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2023 14:46, Nícolas F. R. A. Prado wrote:
> > On Tue, Jun 20, 2023 at 10:12:14AM +0200, Krzysztof Kozlowski wrote:
> >> On 20/06/2023 02:03, 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 hardware controlled clock, remove
> >>> the VDEC_SYS register space from the binding and describe an extra
> >>> syscon that will be used to directly check the hardware status.
> >>>
> >>> Also add reg-names to be able to tell that this new register schema is
> >>> used, so the driver can keep backward compatibility.
> >>>
> >>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> >>>
> >>> ---
> >>> I dropped the tags from this commit since a syscon is now used instead
> >>> of an extra clock.
> >>>
> >>> 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        | 30 +++++++++++++++++++
> >>>  1 file changed, 30 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> >>> index 1e56ece44aee..2f625c50bbfe 100644
> >>> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> >>> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> >>> @@ -21,8 +21,13 @@ properties:
> >>>        - mediatek,mt8183-vcodec-dec
> >>>  
> >>>    reg:
> >>> +    minItems: 11
> >>>      maxItems: 12
> >>>  
> >>> +  reg-names:
> >>> +    minItems: 11
> >>> +    maxItems: 11
> >>
> >> maxItems: 12
> >>
> >>> +
> >>>    interrupts:
> >>>      maxItems: 1
> >>>  
> >>> @@ -60,6 +65,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
> >>> @@ -79,8 +88,26 @@ allOf:
> >>>      then:
> >>>        required:
> >>>          - mediatek,scp
> >>> +        - mediatek,vdecsys
> >>>  
> >>>        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: 1
> >>>            maxItems: 1
> >>> @@ -101,6 +128,9 @@ allOf:
> >>>          - mediatek,vpu
> >>>  
> >>>        properties:
> >>> +        reg:
> >>> +          minItems: 12
> >>
> >>
> >> What about reg-names here? They should be also defined and in sync with
> >> regs.
> > 
> > That's intentional. As described in the commit message, mt8173 will keep having
> > the VDEC_SYS iospace, while mt8183 won't. And we use the presence of reg-names
> > to tell them apart.
> > 
> > So, mt8173 has 12 regs, no reg-names and no syscon, while mt8183 has 11 regs,
> > with reg-names and the syscon.
> 
> reg-names is not the way to tell apart variants. Compatible is. If you
> add reg-names for one variant, it's expected to have it defined for
> other as well, because the order of items in reg is always fixed.

But differentiating with compatible in this case would be wrong, since it's not
not something inherent to the SoC. We really just want to be able to tell
whether the vdecsys iospace is supplied as a reg or as a syscon.

This series focuses on getting the mt8183 decoder working, and as part of that
introduces the binding and DT node for mt8183 with vdecsys as a syscon instead
of a reg, to avoid introducing new 'duplicate unit-address' DT warnings.

But in a separate series we could drop vdecsys from mt8173's reg as well,
passing it as a syscon instead, which would solve the warning on that platform,
though some more driver changes would be needed to be able to handle it for that
SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped
from their regs to have a correct memory description.

Thanks,
Nícolas

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

[Krzysztof Kozlowski]

On 20/06/2023 18:31, Nícolas F. R. A. Prado wrote:
> On Tue, Jun 20, 2023 at 03:00:00PM +0200, Krzysztof Kozlowski wrote:
>> On 20/06/2023 14:46, Nícolas F. R. A. Prado wrote:
>>> On Tue, Jun 20, 2023 at 10:12:14AM +0200, Krzysztof Kozlowski wrote:
>>>> On 20/06/2023 02:03, 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 hardware controlled clock, remove
>>>>> the VDEC_SYS register space from the binding and describe an extra
>>>>> syscon that will be used to directly check the hardware status.
>>>>>
>>>>> Also add reg-names to be able to tell that this new register schema is
>>>>> used, so the driver can keep backward compatibility.
>>>>>
>>>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>>>>
>>>>> ---
>>>>> I dropped the tags from this commit since a syscon is now used instead
>>>>> of an extra clock.
>>>>>
>>>>> 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        | 30 +++++++++++++++++++
>>>>>  1 file changed, 30 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
>>>>> index 1e56ece44aee..2f625c50bbfe 100644
>>>>> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
>>>>> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
>>>>> @@ -21,8 +21,13 @@ properties:
>>>>>        - mediatek,mt8183-vcodec-dec
>>>>>  
>>>>>    reg:
>>>>> +    minItems: 11
>>>>>      maxItems: 12
>>>>>  
>>>>> +  reg-names:
>>>>> +    minItems: 11
>>>>> +    maxItems: 11
>>>>
>>>> maxItems: 12
>>>>
>>>>> +
>>>>>    interrupts:
>>>>>      maxItems: 1
>>>>>  
>>>>> @@ -60,6 +65,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
>>>>> @@ -79,8 +88,26 @@ allOf:
>>>>>      then:
>>>>>        required:
>>>>>          - mediatek,scp
>>>>> +        - mediatek,vdecsys
>>>>>  
>>>>>        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: 1
>>>>>            maxItems: 1
>>>>> @@ -101,6 +128,9 @@ allOf:
>>>>>          - mediatek,vpu
>>>>>  
>>>>>        properties:
>>>>> +        reg:
>>>>> +          minItems: 12
>>>>
>>>>
>>>> What about reg-names here? They should be also defined and in sync with
>>>> regs.
>>>
>>> That's intentional. As described in the commit message, mt8173 will keep having
>>> the VDEC_SYS iospace, while mt8183 won't. And we use the presence of reg-names
>>> to tell them apart.
>>>
>>> So, mt8173 has 12 regs, no reg-names and no syscon, while mt8183 has 11 regs,
>>> with reg-names and the syscon.
>>
>> reg-names is not the way to tell apart variants. Compatible is. If you
>> add reg-names for one variant, it's expected to have it defined for
>> other as well, because the order of items in reg is always fixed.
> 
> But differentiating with compatible in this case would be wrong, since it's not
> not something inherent to the SoC. We really just want to be able to tell
> whether the vdecsys iospace is supplied as a reg or as a syscon.

Wait, you should not have one device or even family of devices taking
their IO space with two different methods. It's exactly the same device,
the same bus.

> 
> This series focuses on getting the mt8183 decoder working, and as part of that
> introduces the binding and DT node for mt8183 with vdecsys as a syscon instead
> of a reg, to avoid introducing new 'duplicate unit-address' DT warnings.

I got patches 1, 2, 3 and 6, nothing more so I cannot comment on what
else you are trying to do here. Since you did not cc me, it's not relevant.

Your DTS change does nothing like switching from MMIO to syscon.

But anyway this variant comes with some set of regs and reg-names. Other
variant comes with different set. In all cases they should be defined,
even by "defined" means not allowed.

> 
> But in a separate series we could drop vdecsys from mt8173's reg as well,
> passing it as a syscon instead, which would solve the warning on that platform,
> though some more driver changes would be needed to be able to handle it for that
> SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped
> from their regs to have a correct memory description.
> 

Sure, but I don't understand how does it affect defining and making
specific regs/reg-names or keeping them loose.

Best regards,
Krzysztof

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

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

On Wed, Jun 21, 2023 at 10:41:49AM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2023 18:31, Nícolas F. R. A. Prado wrote:
> > On Tue, Jun 20, 2023 at 03:00:00PM +0200, Krzysztof Kozlowski wrote:
> >> On 20/06/2023 14:46, Nícolas F. R. A. Prado wrote:
> >>> On Tue, Jun 20, 2023 at 10:12:14AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 20/06/2023 02:03, 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 hardware controlled clock, remove
> >>>>> the VDEC_SYS register space from the binding and describe an extra
> >>>>> syscon that will be used to directly check the hardware status.
> >>>>>
> >>>>> Also add reg-names to be able to tell that this new register schema is
> >>>>> used, so the driver can keep backward compatibility.
> >>>>>
> >>>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> >>>>>
> >>>>> ---
> >>>>> I dropped the tags from this commit since a syscon is now used instead
> >>>>> of an extra clock.
> >>>>>
> >>>>> 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        | 30 +++++++++++++++++++
> >>>>>  1 file changed, 30 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> >>>>> index 1e56ece44aee..2f625c50bbfe 100644
> >>>>> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
> >>>>> @@ -21,8 +21,13 @@ properties:
> >>>>>        - mediatek,mt8183-vcodec-dec
> >>>>>  
> >>>>>    reg:
> >>>>> +    minItems: 11
> >>>>>      maxItems: 12
> >>>>>  
> >>>>> +  reg-names:
> >>>>> +    minItems: 11
> >>>>> +    maxItems: 11
> >>>>
> >>>> maxItems: 12
> >>>>
> >>>>> +
> >>>>>    interrupts:
> >>>>>      maxItems: 1
> >>>>>  
> >>>>> @@ -60,6 +65,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
> >>>>> @@ -79,8 +88,26 @@ allOf:
> >>>>>      then:
> >>>>>        required:
> >>>>>          - mediatek,scp
> >>>>> +        - mediatek,vdecsys
> >>>>>  
> >>>>>        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: 1
> >>>>>            maxItems: 1
> >>>>> @@ -101,6 +128,9 @@ allOf:
> >>>>>          - mediatek,vpu
> >>>>>  
> >>>>>        properties:
> >>>>> +        reg:
> >>>>> +          minItems: 12
> >>>>
> >>>>
> >>>> What about reg-names here? They should be also defined and in sync with
> >>>> regs.
> >>>
> >>> That's intentional. As described in the commit message, mt8173 will keep having
> >>> the VDEC_SYS iospace, while mt8183 won't. And we use the presence of reg-names
> >>> to tell them apart.
> >>>
> >>> So, mt8173 has 12 regs, no reg-names and no syscon, while mt8183 has 11 regs,
> >>> with reg-names and the syscon.
> >>
> >> reg-names is not the way to tell apart variants. Compatible is. If you
> >> add reg-names for one variant, it's expected to have it defined for
> >> other as well, because the order of items in reg is always fixed.
> > 
> > But differentiating with compatible in this case would be wrong, since it's not
> > not something inherent to the SoC. We really just want to be able to tell
> > whether the vdecsys iospace is supplied as a reg or as a syscon.
> 
> Wait, you should not have one device or even family of devices taking
> their IO space with two different methods. It's exactly the same device,
> the same bus.

The VDEC_SYS IO space is arguably not part of the vcodec IO space, since it is
declared by a different node. Hence we shouldn't be getting it through reg, but
instead through a syscon, to avoid clashing addresses.

In other words, the VDEC_SYS IO space shouldn't have been in the binding as a
'reg' in the first place. And since we want to:
1. Keep backward compatibility
2. Fix the 'duplicate unit-address' DT warning

We will need to support both ways - reg or syscon - of passing the VDEC_SYS IO
space moving forward.

> 
> > 
> > This series focuses on getting the mt8183 decoder working, and as part of that
> > introduces the binding and DT node for mt8183 with vdecsys as a syscon instead
> > of a reg, to avoid introducing new 'duplicate unit-address' DT warnings.
> 
> I got patches 1, 2, 3 and 6, nothing more so I cannot comment on what
> else you are trying to do here. Since you did not cc me, it's not relevant.

That's ok, the driver changes aren't relevant to this discussion.

> 
> Your DTS change does nothing like switching from MMIO to syscon.

The original downstream DT node used MMIO for VDEC_SYS, but I've adapted that
commit to use a syscon. So the commit is implicitly doing the switch, it just
doesn't show because the node wasn't upstreamed before on mt8183.

> 
> But anyway this variant comes with some set of regs and reg-names. Other
> variant comes with different set. In all cases they should be defined,
> even by "defined" means not allowed.

I'm not sure what you mean. Are you suggesting to disable reg-names on mt8173?

> 
> > 
> > But in a separate series we could drop vdecsys from mt8173's reg as well,
> > passing it as a syscon instead, which would solve the warning on that platform,
> > though some more driver changes would be needed to be able to handle it for that
> > SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped
> > from their regs to have a correct memory description.
> > 
> 
> Sure, but I don't understand how does it affect defining and making
> specific regs/reg-names or keeping them loose.

We need some way to tell in the driver whether the first reg is VDEC_SYS or not.
Since so far reg-names have not been used for the vcodec, the simplest, and
cleanest, way to do it, is to add reg-names when VDEC_SYS is not present. When
the other SoCs are updated to no longer have the first reg as VDEC_SYS, they
would also have reg-names added to their binding, to clearly indicate that.

For example, for mt8173 we currently have

		vcodec_dec: vcodec@16000000 {
			compatible = "mediatek,mt8173-vcodec-dec";
			reg = <0 0x16000000 0 0x100>,	/* VDEC_SYS */
			      <0 0x16020000 0 0x1000>,	/* VDEC_MISC */
			      <0 0x16021000 0 0x800>,	/* VDEC_LD */
			      <0 0x16021800 0 0x800>,	/* VDEC_TOP */
			      <0 0x16022000 0 0x1000>,	/* VDEC_CM */
			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
			      <0 0x16027000 0 0x800>,	/* VDEC_HWQ */
			      <0 0x16027800 0 0x800>,	/* VDEC_HWB */
			      <0 0x16028400 0 0x400>;	/* VDEC_HWG */

In a future series, when removing VDEC_SYS from it, it would become

		vcodec_dec: vcodec@16020000 {
			compatible = "mediatek,mt8173-vcodec-dec";
			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 */
			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
			      <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";
			mediatek,vdecsys = <&vdecsys>;

Thanks,
Nícolas

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

[Krzysztof Kozlowski]

On 21/06/2023 20:00, Nícolas F. R. A. Prado wrote:
>>
>> But anyway this variant comes with some set of regs and reg-names. Other
>> variant comes with different set. In all cases they should be defined,
>> even by "defined" means not allowed.
> 
> I'm not sure what you mean. Are you suggesting to disable reg-names on mt8173?

That's one of the options if for some reason you don't want to define them.

> 
>>
>>>
>>> But in a separate series we could drop vdecsys from mt8173's reg as well,
>>> passing it as a syscon instead, which would solve the warning on that platform,
>>> though some more driver changes would be needed to be able to handle it for that
>>> SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped
>>> from their regs to have a correct memory description.
>>>
>>
>> Sure, but I don't understand how does it affect defining and making
>> specific regs/reg-names or keeping them loose.
> 
> We need some way to tell in the driver whether the first reg is VDEC_SYS or not.
> Since so far reg-names have not been used for the vcodec, the simplest, and
> cleanest, way to do it, is to add reg-names when VDEC_SYS is not present. When
> the other SoCs are updated to no longer have the first reg as VDEC_SYS, they
> would also have reg-names added to their binding, to clearly indicate that.

Don't use reg-names for that. The order of entries is anyway strict.

> 
> For example, for mt8173 we currently have
> 
> 		vcodec_dec: vcodec@16000000 {
> 			compatible = "mediatek,mt8173-vcodec-dec";
> 			reg = <0 0x16000000 0 0x100>,	/* VDEC_SYS */
> 			      <0 0x16020000 0 0x1000>,	/* VDEC_MISC */
> 			      <0 0x16021000 0 0x800>,	/* VDEC_LD */
> 			      <0 0x16021800 0 0x800>,	/* VDEC_TOP */
> 			      <0 0x16022000 0 0x1000>,	/* VDEC_CM */
> 			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
> 			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
> 			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
> 			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
> 			      <0 0x16027000 0 0x800>,	/* VDEC_HWQ */
> 			      <0 0x16027800 0 0x800>,	/* VDEC_HWB */
> 			      <0 0x16028400 0 0x400>;	/* VDEC_HWG */
> 
> In a future series, when removing VDEC_SYS from it, it would become
> 
> 		vcodec_dec: vcodec@16020000 {
> 			compatible = "mediatek,mt8173-vcodec-dec";
> 			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 */
> 			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
> 			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
> 			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
> 			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
> 			      <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";

So you want to use reg-names to avoid ABI break. This is not the reason
not to define reg-names for other case.



Best regards,
Krzysztof

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

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

On Fri, Jun 23, 2023 at 06:21:31PM +0200, Krzysztof Kozlowski wrote:
> On 21/06/2023 20:00, Nícolas F. R. A. Prado wrote:
> >>
> >> But anyway this variant comes with some set of regs and reg-names. Other
> >> variant comes with different set. In all cases they should be defined,
> >> even by "defined" means not allowed.
> > 
> > I'm not sure what you mean. Are you suggesting to disable reg-names on mt8173?
> 
> That's one of the options if for some reason you don't want to define them.
> 
> > 
> >>
> >>>
> >>> But in a separate series we could drop vdecsys from mt8173's reg as well,
> >>> passing it as a syscon instead, which would solve the warning on that platform,
> >>> though some more driver changes would be needed to be able to handle it for that
> >>> SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped
> >>> from their regs to have a correct memory description.
> >>>
> >>
> >> Sure, but I don't understand how does it affect defining and making
> >> specific regs/reg-names or keeping them loose.
> > 
> > We need some way to tell in the driver whether the first reg is VDEC_SYS or not.
> > Since so far reg-names have not been used for the vcodec, the simplest, and
> > cleanest, way to do it, is to add reg-names when VDEC_SYS is not present. When
> > the other SoCs are updated to no longer have the first reg as VDEC_SYS, they
> > would also have reg-names added to their binding, to clearly indicate that.
> 
> Don't use reg-names for that. The order of entries is anyway strict.

Since the order of entries is strict, if I remove VDEC_SYS from mt8183, I also
need to remove it from mt8173, is that what you mean? I would still check for
the presence of reg-names in the driver to differentiate whether the old or new
binding is used, you just don't want different reg-names between compatibles in
the binding?

> 
> > 
> > For example, for mt8173 we currently have
> > 
> > 		vcodec_dec: vcodec@16000000 {
> > 			compatible = "mediatek,mt8173-vcodec-dec";
> > 			reg = <0 0x16000000 0 0x100>,	/* VDEC_SYS */
> > 			      <0 0x16020000 0 0x1000>,	/* VDEC_MISC */
> > 			      <0 0x16021000 0 0x800>,	/* VDEC_LD */
> > 			      <0 0x16021800 0 0x800>,	/* VDEC_TOP */
> > 			      <0 0x16022000 0 0x1000>,	/* VDEC_CM */
> > 			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
> > 			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
> > 			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
> > 			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
> > 			      <0 0x16027000 0 0x800>,	/* VDEC_HWQ */
> > 			      <0 0x16027800 0 0x800>,	/* VDEC_HWB */
> > 			      <0 0x16028400 0 0x400>;	/* VDEC_HWG */
> > 
> > In a future series, when removing VDEC_SYS from it, it would become
> > 
> > 		vcodec_dec: vcodec@16020000 {
> > 			compatible = "mediatek,mt8173-vcodec-dec";
> > 			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 */
> > 			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
> > 			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
> > 			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
> > 			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
> > 			      <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";
> 
> So you want to use reg-names to avoid ABI break. This is not the reason
> not to define reg-names for other case.

There will be an ABI break anyway when the first reg is removed (as shown
above), I'm just trying to avoid churn: adding a reg-name that will be removed
later.

Thanks,
Nícolas

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

[Krzysztof Kozlowski]

On 26/06/2023 15:54, Nícolas F. R. A. Prado wrote:
> On Fri, Jun 23, 2023 at 06:21:31PM +0200, Krzysztof Kozlowski wrote:
>> On 21/06/2023 20:00, Nícolas F. R. A. Prado wrote:
>>>>
>>>> But anyway this variant comes with some set of regs and reg-names. Other
>>>> variant comes with different set. In all cases they should be defined,
>>>> even by "defined" means not allowed.
>>>
>>> I'm not sure what you mean. Are you suggesting to disable reg-names on mt8173?
>>
>> That's one of the options if for some reason you don't want to define them.
>>
>>>
>>>>
>>>>>
>>>>> But in a separate series we could drop vdecsys from mt8173's reg as well,
>>>>> passing it as a syscon instead, which would solve the warning on that platform,
>>>>> though some more driver changes would be needed to be able to handle it for that
>>>>> SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped
>>>>> from their regs to have a correct memory description.
>>>>>
>>>>
>>>> Sure, but I don't understand how does it affect defining and making
>>>> specific regs/reg-names or keeping them loose.
>>>
>>> We need some way to tell in the driver whether the first reg is VDEC_SYS or not.
>>> Since so far reg-names have not been used for the vcodec, the simplest, and
>>> cleanest, way to do it, is to add reg-names when VDEC_SYS is not present. When
>>> the other SoCs are updated to no longer have the first reg as VDEC_SYS, they
>>> would also have reg-names added to their binding, to clearly indicate that.
>>
>> Don't use reg-names for that. The order of entries is anyway strict.
> 
> Since the order of entries is strict, if I remove VDEC_SYS from mt8183, I also
> need to remove it from mt8173, is that what you mean?

It's different compatible, so it can have different entries.


> I would still check for
> the presence of reg-names in the driver to differentiate whether the old or new
> binding is used, you just don't want different reg-names between compatibles in
> the binding?

I wrote already what I want:

  In all cases they should be defined, even by "defined" means not allowed.

Now of course the best would be if the reg-names are always the same, at
least in respect of order of items. This is what we try to do for all
devices.

> 
>>
>>>
>>> For example, for mt8173 we currently have
>>>
>>> 		vcodec_dec: vcodec@16000000 {
>>> 			compatible = "mediatek,mt8173-vcodec-dec";
>>> 			reg = <0 0x16000000 0 0x100>,	/* VDEC_SYS */
>>> 			      <0 0x16020000 0 0x1000>,	/* VDEC_MISC */
>>> 			      <0 0x16021000 0 0x800>,	/* VDEC_LD */
>>> 			      <0 0x16021800 0 0x800>,	/* VDEC_TOP */
>>> 			      <0 0x16022000 0 0x1000>,	/* VDEC_CM */
>>> 			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
>>> 			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
>>> 			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
>>> 			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
>>> 			      <0 0x16027000 0 0x800>,	/* VDEC_HWQ */
>>> 			      <0 0x16027800 0 0x800>,	/* VDEC_HWB */
>>> 			      <0 0x16028400 0 0x400>;	/* VDEC_HWG */
>>>
>>> In a future series, when removing VDEC_SYS from it, it would become
>>>
>>> 		vcodec_dec: vcodec@16020000 {
>>> 			compatible = "mediatek,mt8173-vcodec-dec";
>>> 			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 */
>>> 			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
>>> 			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
>>> 			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
>>> 			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
>>> 			      <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";
>>
>> So you want to use reg-names to avoid ABI break. This is not the reason
>> not to define reg-names for other case.
> 
> There will be an ABI break anyway when the first reg is removed (as shown
> above), I'm just trying to avoid churn: adding a reg-name that will be removed
> later.

So remove the reg-name now and there will be no "later"?

Best regards,
Krzysztof

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

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

On Mon, Jun 26, 2023 at 05:30:07PM +0200, Krzysztof Kozlowski wrote:
> On 26/06/2023 15:54, Nícolas F. R. A. Prado wrote:
> > On Fri, Jun 23, 2023 at 06:21:31PM +0200, Krzysztof Kozlowski wrote:
> >> On 21/06/2023 20:00, Nícolas F. R. A. Prado wrote:
> >>>>
> >>>> But anyway this variant comes with some set of regs and reg-names. Other
> >>>> variant comes with different set. In all cases they should be defined,
> >>>> even by "defined" means not allowed.
> >>>
> >>> I'm not sure what you mean. Are you suggesting to disable reg-names on mt8173?
> >>
> >> That's one of the options if for some reason you don't want to define them.
> >>
> >>>
> >>>>
> >>>>>
> >>>>> But in a separate series we could drop vdecsys from mt8173's reg as well,
> >>>>> passing it as a syscon instead, which would solve the warning on that platform,
> >>>>> though some more driver changes would be needed to be able to handle it for that
> >>>>> SoC. The newer SoCs like mt8192, mt8195, etc, should also get vdecsys dropped
> >>>>> from their regs to have a correct memory description.
> >>>>>
> >>>>
> >>>> Sure, but I don't understand how does it affect defining and making
> >>>> specific regs/reg-names or keeping them loose.
> >>>
> >>> We need some way to tell in the driver whether the first reg is VDEC_SYS or not.
> >>> Since so far reg-names have not been used for the vcodec, the simplest, and
> >>> cleanest, way to do it, is to add reg-names when VDEC_SYS is not present. When
> >>> the other SoCs are updated to no longer have the first reg as VDEC_SYS, they
> >>> would also have reg-names added to their binding, to clearly indicate that.
> >>
> >> Don't use reg-names for that. The order of entries is anyway strict.
> > 
> > Since the order of entries is strict, if I remove VDEC_SYS from mt8183, I also
> > need to remove it from mt8173, is that what you mean?
> 
> It's different compatible, so it can have different entries.
> 
> 
> > I would still check for
> > the presence of reg-names in the driver to differentiate whether the old or new
> > binding is used, you just don't want different reg-names between compatibles in
> > the binding?
> 
> I wrote already what I want:
> 
>   In all cases they should be defined, even by "defined" means not allowed.
> 
> Now of course the best would be if the reg-names are always the same, at
> least in respect of order of items. This is what we try to do for all
> devices.
> 
> > 
> >>
> >>>
> >>> For example, for mt8173 we currently have
> >>>
> >>> 		vcodec_dec: vcodec@16000000 {
> >>> 			compatible = "mediatek,mt8173-vcodec-dec";
> >>> 			reg = <0 0x16000000 0 0x100>,	/* VDEC_SYS */
> >>> 			      <0 0x16020000 0 0x1000>,	/* VDEC_MISC */
> >>> 			      <0 0x16021000 0 0x800>,	/* VDEC_LD */
> >>> 			      <0 0x16021800 0 0x800>,	/* VDEC_TOP */
> >>> 			      <0 0x16022000 0 0x1000>,	/* VDEC_CM */
> >>> 			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
> >>> 			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
> >>> 			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
> >>> 			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
> >>> 			      <0 0x16027000 0 0x800>,	/* VDEC_HWQ */
> >>> 			      <0 0x16027800 0 0x800>,	/* VDEC_HWB */
> >>> 			      <0 0x16028400 0 0x400>;	/* VDEC_HWG */
> >>>
> >>> In a future series, when removing VDEC_SYS from it, it would become
> >>>
> >>> 		vcodec_dec: vcodec@16020000 {
> >>> 			compatible = "mediatek,mt8173-vcodec-dec";
> >>> 			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 */
> >>> 			      <0 0x16023000 0 0x1000>,	/* VDEC_AD */
> >>> 			      <0 0x16024000 0 0x1000>,	/* VDEC_AV */
> >>> 			      <0 0x16025000 0 0x1000>,	/* VDEC_PP */
> >>> 			      <0 0x16026800 0 0x800>,	/* VDEC_HWD */
> >>> 			      <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";
> >>
> >> So you want to use reg-names to avoid ABI break. This is not the reason
> >> not to define reg-names for other case.
> > 
> > There will be an ABI break anyway when the first reg is removed (as shown
> > above), I'm just trying to avoid churn: adding a reg-name that will be removed
> > later.
> 
> So remove the reg-name now and there will be no "later"?

OK, I'll send a v4 with VDEC_SYS also removed from mt8173.

Thanks,
Nícolas