[PATCH v2 06/10] media: dt-bindings: Document SAMA5D4 VDEC bindings

[PATCH v2 06/10] media: dt-bindings: Document SAMA5D4 VDEC bindings

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Add devicetree binding documentation for the Hantro G1/G2 VDEC on
the Microchip SAMAS5D4 SoC.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
v2
 - Newly introduced
 - s/Atmel/Microchip/ (Nicolas)
 - Drop leading 0 in node name/address
---
 .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
new file mode 100644
index 000000000000..9cb2c0295d54
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs
+
+maintainers:
+  - Emil Velikov <emil.velikov@collabora.com>
+
+description:
+  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.
+
+properties:
+  compatible:
+    const: microchip,sama5d4-vdec
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    items:
+      - const: vdec
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: vdec_clk
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+        #include <dt-bindings/clock/at91.h>
+        #include <dt-bindings/interrupt-controller/irq.h>
+
+        vdec0: vdec@300000 {
+                compatible = "microchip,sama5d4-vdec";
+                reg = <0x00300000 0x100000>;
+                interrupts = <19 IRQ_TYPE_LEVEL_HIGH 4>;
+                interrupt-names = "vdec";
+                clocks = <&pmc PMC_TYPE_PERIPHERAL 19>;
+                clock-names = "vdec_clk";
+        };
-- 
2.30.1

[PATCH v2 07/10] media: hantro: add initial SAMA5D4 support

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

The SoC features a Hantro G1 compatible video decoder, supporting the
MPEG-2, VP8 and H264 codecs with resolutions up-to 1280x720.

Post-processing core is also available on the SoC.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
v2
 - Split DT and defconfig changes to separate patches (Eze)
 - s/Atmel/Microchip/ (Nicolas)
---
 drivers/staging/media/hantro/Kconfig          |  10 +-
 drivers/staging/media/hantro/Makefile         |   3 +
 drivers/staging/media/hantro/hantro_drv.c     |   3 +
 drivers/staging/media/hantro/hantro_hw.h      |   1 +
 .../staging/media/hantro/sama5d4_vdec_hw.c    | 117 ++++++++++++++++++
 5 files changed, 133 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/media/hantro/sama5d4_vdec_hw.c

diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig
index 5b6cf9f62b1a..20b1f6d7b69c 100644
--- a/drivers/staging/media/hantro/Kconfig
+++ b/drivers/staging/media/hantro/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 config VIDEO_HANTRO
 	tristate "Hantro VPU driver"
-	depends on ARCH_MXC || ARCH_ROCKCHIP || COMPILE_TEST
+	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || COMPILE_TEST
 	depends on VIDEO_DEV && VIDEO_V4L2
 	select MEDIA_CONTROLLER
 	select MEDIA_CONTROLLER_REQUEST_API
@@ -24,6 +24,14 @@ config VIDEO_HANTRO_IMX8M
 	help
 	  Enable support for i.MX8M SoCs.
 
+config VIDEO_HANTRO_SAMA5D4
+	bool "Hantro VDEC SAMA5D4 support"
+	depends on VIDEO_HANTRO
+	depends on ARCH_AT91 || COMPILE_TEST
+	default y
+	help
+	  Enable support for Microchip SAMA5D4 SoCs.
+
 config VIDEO_HANTRO_ROCKCHIP
 	bool "Hantro VPU Rockchip support"
 	depends on VIDEO_HANTRO
diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile
index 3747a32799b2..f4b99901eeee 100644
--- a/drivers/staging/media/hantro/Makefile
+++ b/drivers/staging/media/hantro/Makefile
@@ -22,6 +22,9 @@ hantro-vpu-y += \
 hantro-vpu-$(CONFIG_VIDEO_HANTRO_IMX8M) += \
 		imx8m_vpu_hw.o
 
+hantro-vpu-$(CONFIG_VIDEO_HANTRO_SAMA5D4) += \
+		sama5d4_vdec_hw.o
+
 hantro-vpu-$(CONFIG_VIDEO_HANTRO_ROCKCHIP) += \
 		rk3288_vpu_hw.o \
 		rk3399_vpu_hw.o
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index e5f200e64993..a530c0eda595 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -478,6 +478,9 @@ static const struct of_device_id of_hantro_match[] = {
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_IMX8M
 	{ .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
+#endif
+#ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
+	{ .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
 #endif
 	{ /* sentinel */ }
 };
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 73c71bb2320c..4d39da1d1581 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -152,6 +152,7 @@ extern const struct hantro_variant rk3399_vpu_variant;
 extern const struct hantro_variant rk3328_vpu_variant;
 extern const struct hantro_variant rk3288_vpu_variant;
 extern const struct hantro_variant imx8mq_vpu_variant;
+extern const struct hantro_variant sama5d4_vdec_variant;
 
 extern const struct hantro_postproc_regs hantro_g1_postproc_regs;
 
diff --git a/drivers/staging/media/hantro/sama5d4_vdec_hw.c b/drivers/staging/media/hantro/sama5d4_vdec_hw.c
new file mode 100644
index 000000000000..58ae72c2b723
--- /dev/null
+++ b/drivers/staging/media/hantro/sama5d4_vdec_hw.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Hantro VDEC driver
+ *
+ * Copyright (C) 2021 Collabora Ltd, Emil Velikov <emil.velikov@collabora.com>
+ */
+
+#include "hantro.h"
+
+/*
+ * Supported formats.
+ */
+
+static const struct hantro_fmt sama5d4_vdec_postproc_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_YUYV,
+		.codec_mode = HANTRO_MODE_NONE,
+	},
+};
+
+static const struct hantro_fmt sama5d4_vdec_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12,
+		.codec_mode = HANTRO_MODE_NONE,
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_MPEG2_SLICE,
+		.codec_mode = HANTRO_MODE_MPEG2_DEC,
+		.max_depth = 2,
+		.frmsize = {
+			.min_width = 48,
+			.max_width = 1280,
+			.step_width = MB_DIM,
+			.min_height = 48,
+			.max_height = 720,
+			.step_height = MB_DIM,
+		},
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_VP8_FRAME,
+		.codec_mode = HANTRO_MODE_VP8_DEC,
+		.max_depth = 2,
+		.frmsize = {
+			.min_width = 48,
+			.max_width = 1280,
+			.step_width = MB_DIM,
+			.min_height = 48,
+			.max_height = 720,
+			.step_height = MB_DIM,
+		},
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_H264_SLICE,
+		.codec_mode = HANTRO_MODE_H264_DEC,
+		.max_depth = 2,
+		.frmsize = {
+			.min_width = 48,
+			.max_width = 1280,
+			.step_width = MB_DIM,
+			.min_height = 48,
+			.max_height = 720,
+			.step_height = MB_DIM,
+		},
+	},
+};
+
+static int sama5d4_hw_init(struct hantro_dev *vpu)
+{
+	return 0;
+}
+
+/*
+ * Supported codec ops.
+ */
+
+static const struct hantro_codec_ops sama5d4_vdec_codec_ops[] = {
+	[HANTRO_MODE_MPEG2_DEC] = {
+		.run = hantro_g1_mpeg2_dec_run,
+		.reset = hantro_g1_reset,
+		.init = hantro_mpeg2_dec_init,
+		.exit = hantro_mpeg2_dec_exit,
+	},
+	[HANTRO_MODE_VP8_DEC] = {
+		.run = hantro_g1_vp8_dec_run,
+		.reset = hantro_g1_reset,
+		.init = hantro_vp8_dec_init,
+		.exit = hantro_vp8_dec_exit,
+	},
+	[HANTRO_MODE_H264_DEC] = {
+		.run = hantro_g1_h264_dec_run,
+		.reset = hantro_g1_reset,
+		.init = hantro_h264_dec_init,
+		.exit = hantro_h264_dec_exit,
+	},
+};
+
+static const struct hantro_irq sama5d4_irqs[] = {
+	{ "vdec", hantro_g1_irq },
+};
+
+static const char * const sama5d4_clk_names[] = { "vdec_clk" };
+
+const struct hantro_variant sama5d4_vdec_variant = {
+	.dec_fmts = sama5d4_vdec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(sama5d4_vdec_fmts),
+	.postproc_fmts = sama5d4_vdec_postproc_fmts,
+	.num_postproc_fmts = ARRAY_SIZE(sama5d4_vdec_postproc_fmts),
+	.postproc_regs = &hantro_g1_postproc_regs,
+	.codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
+		 HANTRO_H264_DECODER,
+	.codec_ops = sama5d4_vdec_codec_ops,
+	.init = sama5d4_hw_init,
+	.irqs = sama5d4_irqs,
+	.num_irqs = ARRAY_SIZE(sama5d4_irqs),
+	.clk_names = sama5d4_clk_names,
+	.num_clocks = ARRAY_SIZE(sama5d4_clk_names),
+};
-- 
2.30.1

[PATCH v2 08/10] ARM: dts: sama5d4: enable Hantro G1 VDEC

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Add the SAMA5D4 VDEC module which comprises Hantro G1 video decoder
core.

Cc: Rob Herring <robh+dt@kernel.org
Cc: Frank Rowand <frowand.list@gmail.com
Cc: devicetree@vger.kernel.org
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
v2
 - Split out of larger patch (Eze)
 - s/Atmel/Microchip/ (Nicolas)
 - Drop leading 0 in node name/address
---
 arch/arm/boot/dts/sama5d4.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 05c55875835d..88284f60feb1 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -101,6 +101,15 @@ nfc_sram: sram@100000 {
 			ranges = <0 0x100000 0x2400>;
 		};
 
+		vdec0: vdec@300000 {
+			compatible = "microchip,sama5d4-vdec";
+			reg = <0x00300000 0x100000>;
+			interrupts = <19 IRQ_TYPE_LEVEL_HIGH 4>;
+			interrupt-names = "vdec";
+			clocks = <&pmc PMC_TYPE_PERIPHERAL 19>;
+			clock-names = "vdec_clk";
+		};
+
 		usb0: gadget@400000 {
 			compatible = "atmel,sama5d3-udc";
 			reg = <0x00400000 0x100000
-- 
2.30.1

Re: [PATCH v2 00/10] Microship SAMA5D4 VPU support et al

[Ezequiel Garcia]

On Thu, 2021-03-11 at 15:40 +0000, Emil Velikov wrote:
> Hi all
> 
> This series adds support for the Microchip SAMA5D4 VPU, which it based
> on the Hantro G1.
> 
> The hardware can support up-to 1280x720 for each of the MPEG2, VP8 and
> H264 codecs. There is only a single decoder and no encoders on the SoC.
> 
> The Hantro G1 post-processing is also enabled on the platform.
> 
> To minimise duplication, the series starts with a few small cleanups.
> 
> 
> As you may have noticed, this is my first patches series to linux-media,
> so any tips how to make this as smoother process are appreciated.
> 
> 
> Changes in v2:
>  - Add testing results in the cover letter (thanks Eze)
>  - s/Atmel/Microchip/ through the series (thanks Nicolas)
>  - Split defconfig change into separate commit (thanks Eze, Nicolas)
>  - Added Reviewed-by and Fixes tags (thanks Philipp)
>  - Split DT into separate commit, wrote binding document, fixup minor DT
>    warnings (thanks Eze)
>  - Rebased on top of 5.12-rc2, as per Linus' email to avoid 5.12-rc1
>    https://lwn.net/Articles/848265/
> 
> 
> 
> Testing
> -------
> 
>  - v4l-compliance
> 
> Command used:
>   v4l2-compliance -m0
> 
> Output summary:
> 
> v4l2-compliance 1.21.0-4740, 32 bits, 32-bit time_t
> v4l2-compliance SHA: f253495fa6de 2021-03-06 15:32:09
> 
> Compliance test for hantro-vpu device /dev/media0:
> 
> Total for hantro-vpu device /dev/media0: 8, Succeeded: 8, Failed: 0,
> Warnings: 0
> 
> Compliance test for hantro-vpu device /dev/video0:
> 
> Total for hantro-vpu device /dev/video0: 46, Succeeded: 46, Failed: 0,
> Warnings: 0
> 
> 
>  - Post-processor testing
> 
> Command used:
>   gst-launch-1.0 -v filesrc location=test.mp4  ! decodebin3 !
> video/x-raw,format=YUY2 ! ...
> 
> Confirmed the VPU is used by observing the interrupts triggering, strace
> showed extra v4l2 ioctls - VIDIOC_S_FMT(... V4L2_PIX_FMT_YUYV  ...)
> 
> 
>  - MPEG2 testing, custom ffmpeg from
>    https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3
> 
> Command used:
>   ffmpeg -hwaccel drm -i mpeg2.mpeg2 -f rawvideo -pix_fmt yuv420p out.raw
> 
> Confirmed the VPU is used by observing the interrupts triggering, strace
> showed the v4l2 ioctls being used plus played back the resulting file.
> 
> 
>  - VP8 testing, using fluster
> 
> Command used:
>   fluster.py run -ts VP8-TEST-VECTORS -d GStreamer-VP8-V4L2SL-Gst1.0
> 
> Output summary:
> 
> Running test suite VP8-TEST-VECTORS with decoder GStreamer-VP8-V4L2SL-Gst1.0
> Ran 61 tests in 103.273s
> 
> FAILED (failures=9, errors=2)
> 
> 
>  - H264 testing, using fluster
> 
> Command used:
>   fluster.py run -ts JVT-AVC_V1 -d GStreamer-H.264-V4L2SL-Gst1.0
> 
> Output summary:
> 
> Running test suite JVT-AVC_V1 with decoder GStreamer-H.264-V4L2SL-Gst1.0
> Ran 135 tests in 420.444s
> 
> FAILED (failures=9, errors=55)
> 
> 
> Looking forward to your feedback,
> Emil
> 
> 
> Emil Velikov (10):
>   media: hantro: use G1_REG_INTERRUPT directly for the mpeg2
>   media: hantro: imx: reuse MB_DIM define
>   media: hantro: imx: remove duplicate dec_base init
>   media: hantro: imx: remove unused include
>   media: hantro: introduce hantro_g1.c for common API

For patches 1-5:

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

>   media: dt-bindings: Document SAMA5D4 VDEC bindings

This one need to be reviewed by DT maintainers, I think.

>   media: hantro: add initial SAMA5D4 support

For patch 7:

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

>   ARM: dts: sama5d4: enable Hantro G1 VDEC
>   ARM: configs: at91: sama5: update with savedefconfig
>   ARM: configs: at91: sama5: enable the Hantro G1 engine
> 

These need review from Microchip maintainers.

Thanks!
Ezequiel

>  .../media/microchip,sama5d4-vdec.yaml         |  59 +++++++++
>  arch/arm/boot/dts/sama5d4.dtsi                |   9 ++
>  arch/arm/configs/sama5_defconfig              |  40 +++---
>  drivers/staging/media/hantro/Kconfig          |  10 +-
>  drivers/staging/media/hantro/Makefile         |   4 +
>  drivers/staging/media/hantro/hantro_drv.c     |   3 +
>  drivers/staging/media/hantro/hantro_g1.c      |  39 ++++++
>  .../media/hantro/hantro_g1_mpeg2_dec.c        |   5 +-
>  drivers/staging/media/hantro/hantro_hw.h      |   4 +
>  drivers/staging/media/hantro/imx8m_vpu_hw.c   |  27 +---
>  drivers/staging/media/hantro/rk3288_vpu_hw.c  |  36 +-----
>  .../staging/media/hantro/sama5d4_vdec_hw.c    | 117 ++++++++++++++++++
>  12 files changed, 274 insertions(+), 79 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
>  create mode 100644 drivers/staging/media/hantro/hantro_g1.c
>  create mode 100644 drivers/staging/media/hantro/sama5d4_vdec_hw.c
> 

Re: [PATCH v2 00/10] Microship SAMA5D4 VPU support et al

[Emil Velikov]

On Tue, 16 Mar 2021 at 17:23, Ezequiel Garcia <ezequiel@collabora.com> wrote:

> On Thu, 2021-03-11 at 15:40 +0000, Emil Velikov wrote:
> > Emil Velikov (10):
> >   media: hantro: use G1_REG_INTERRUPT directly for the mpeg2
> >   media: hantro: imx: reuse MB_DIM define
> >   media: hantro: imx: remove duplicate dec_base init
> >   media: hantro: imx: remove unused include
> >   media: hantro: introduce hantro_g1.c for common API
>
> For patches 1-5:
>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>
> >   media: dt-bindings: Document SAMA5D4 VDEC bindings
>
> This one need to be reviewed by DT maintainers, I think.
>
Rob can you help with this one?

> >   media: hantro: add initial SAMA5D4 support
>
> For patch 7:
>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>
> >   ARM: dts: sama5d4: enable Hantro G1 VDEC
> >   ARM: configs: at91: sama5: update with savedefconfig
> >   ARM: configs: at91: sama5: enable the Hantro G1 engine
> >
>
> These need review from Microchip maintainers.
>
Alexandre, Ludovic, Nicolas
Do you have any input of the patches or series as a whole?
If you prefer we can drop the last two patches for the defconfig. I've
included those for posterity.

Thanks for the review Eze.
Would you recommend that I resend the series with your R-B or it's
better to wait for feedback from others?

-Emil

Re: [PATCH v2 00/10] Microship SAMA5D4 VPU support et al

[Nicolas Ferre]

Emil,

On 24/03/2021 at 13:49, Emil Velikov wrote:
> On Tue, 16 Mar 2021 at 17:23, Ezequiel Garcia <ezequiel@collabora.com> wrote:
> 
>> On Thu, 2021-03-11 at 15:40 +0000, Emil Velikov wrote:
>>> Emil Velikov (10):
>>>    media: hantro: use G1_REG_INTERRUPT directly for the mpeg2
>>>    media: hantro: imx: reuse MB_DIM define
>>>    media: hantro: imx: remove duplicate dec_base init
>>>    media: hantro: imx: remove unused include
>>>    media: hantro: introduce hantro_g1.c for common API
>>
>> For patches 1-5:
>>
>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>>
>>>    media: dt-bindings: Document SAMA5D4 VDEC bindings
>>
>> This one need to be reviewed by DT maintainers, I think.
>>
> Rob can you help with this one?
> 
>>>    media: hantro: add initial SAMA5D4 support
>>
>> For patch 7:
>>
>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>>
>>>    ARM: dts: sama5d4: enable Hantro G1 VDEC
>>>    ARM: configs: at91: sama5: update with savedefconfig
>>>    ARM: configs: at91: sama5: enable the Hantro G1 engine
>>>
>>
>> These need review from Microchip maintainers.
>>
> Alexandre, Ludovic, Nicolas
> Do you have any input of the patches or series as a whole?

The patch series looks good to me.

If needed on patches that we don't take ourselves, you can add my:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Now, when we have the tag from Rob, how to coordinate these different 
pieces? Will it go through the media git tree? Will we benefit from a 
stable branch to share or will we just have to wait for the driver to 
hit Mainline before adding the defconfig and DT patches?

> If you prefer we can drop the last two patches for the defconfig. I've
> included those for posterity.
No strong opinion on my side, except that defconfig stuff might be 
better handled in at91 + arm-soc trees because we'll have other changes 
to queue for 5.13.

> Thanks for the review Eze.
> Would you recommend that I resend the series with your R-B or it's
> better to wait for feedback from others?

Thanks a lot for this nice work. Best regards,
-- 
Nicolas Ferre

Re: [PATCH v2 06/10] media: dt-bindings: Document SAMA5D4 VDEC bindings

[Rob Herring]

On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Add devicetree binding documentation for the Hantro G1/G2 VDEC on
> the Microchip SAMAS5D4 SoC.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: devicetree@vger.kernel.org>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> v2
>  - Newly introduced
>  - s/Atmel/Microchip/ (Nicolas)
>  - Drop leading 0 in node name/address
> ---
>  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> new file mode 100644
> index 000000000000..9cb2c0295d54
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs
> +
> +maintainers:
> +  - Emil Velikov <emil.velikov@collabora.com>
> +
> +description:
> +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.
> +
> +properties:
> +  compatible:
> +    const: microchip,sama5d4-vdec
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    items:
> +      - const: vdec

Why do you need a name? *-names are used to distinguish multiple entries 
and don't add anything if only a single entry.

> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: vdec_clk

And here too? These are typically named for either the name of input 
signal (hclk, aclk, etc.) or function ('core', 'bus', etc.). 

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        #include <dt-bindings/clock/at91.h>
> +        #include <dt-bindings/interrupt-controller/irq.h>
> +
> +        vdec0: vdec@300000 {
> +                compatible = "microchip,sama5d4-vdec";
> +                reg = <0x00300000 0x100000>;
> +                interrupts = <19 IRQ_TYPE_LEVEL_HIGH 4>;
> +                interrupt-names = "vdec";
> +                clocks = <&pmc PMC_TYPE_PERIPHERAL 19>;
> +                clock-names = "vdec_clk";
> +        };
> -- 
> 2.30.1
> 

Re: [PATCH v2 00/10] Microship SAMA5D4 VPU support et al

[Alexandre Belloni]

On 24/03/2021 14:44:14+0100, Nicolas Ferre wrote:
> Now, when we have the tag from Rob, how to coordinate these different
> pieces? Will it go through the media git tree? Will we benefit from a stable
> branch to share or will we just have to wait for the driver to hit Mainline
> before adding the defconfig and DT patches?
> 

I think the defconfig and dt patches can go through at91 as soon as we
get Rob's ack. There is no build dependency so it can be taken at any
time. Worst case, we end up with a selected config option that doesn't
exist.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 06/10] media: dt-bindings: Document SAMA5D4 VDEC bindings

[Emil Velikov]

On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > Add devicetree binding documentation for the Hantro G1/G2 VDEC on
> > the Microchip SAMAS5D4 SoC.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org>
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> > v2
> >  - Newly introduced
> >  - s/Atmel/Microchip/ (Nicolas)
> >  - Drop leading 0 in node name/address
> > ---
> >  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > new file mode 100644
> > index 000000000000..9cb2c0295d54
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs
> > +
> > +maintainers:
> > +  - Emil Velikov <emil.velikov@collabora.com>
> > +
> > +description:
> > +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    const: microchip,sama5d4-vdec
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: vdec
>
> Why do you need a name? *-names are used to distinguish multiple entries
> and don't add anything if only a single entry.
>
On one hand the names are used to describe the hardware - the SAMA
board uses designated clock and interrupt lines - both called "vdec".
Additionally the names are ultimately required by the underlying API -
platform_get_irq_byname and  devm_clk_bulk_get respectively.
How can we get the respective entries without the name?

Skimming through the existing dts file -
arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of
examples where the device tree binding contains the name for a single
clock/interrupt.

Thanks
Emil

Re: [PATCH v2 00/10] Microship SAMA5D4 VPU support et al

[Emil Velikov]

Greetings all,

On Thu, 25 Mar 2021 at 08:48, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 24/03/2021 14:44:14+0100, Nicolas Ferre wrote:
> > Now, when we have the tag from Rob, how to coordinate these different
> > pieces? Will it go through the media git tree? Will we benefit from a stable
> > branch to share or will we just have to wait for the driver to hit Mainline
> > before adding the defconfig and DT patches?
> >
Thanks for the Acked-by Nicolas.

>
> I think the defconfig and dt patches can go through at91 as soon as we
> get Rob's ack. There is no build dependency so it can be taken at any
> time. Worst case, we end up with a selected config option that doesn't
> exist.
>
My personal preference is to merge everything in one go.
I believe it will be easier from maintainer's point of view, plus odds
of conflicts with the AT91 tree are close to zero.

Then again, as long as the maintainers are happy - I'm fine either way.

Thanks
Emil

Re: [PATCH v2 00/10] Microship SAMA5D4 VPU support et al

[Nicolas Ferre]

On 25/03/2021 at 09:48, Alexandre Belloni wrote:
> On 24/03/2021 14:44:14+0100, Nicolas Ferre wrote:
>> Now, when we have the tag from Rob, how to coordinate these different
>> pieces? Will it go through the media git tree? Will we benefit from a stable
>> branch to share or will we just have to wait for the driver to hit Mainline
>> before adding the defconfig and DT patches?
>>
> 
> I think the defconfig and dt patches can go through at91 as soon as we
> get Rob's ack. There is no build dependency so it can be taken at any
> time. Worst case, we end up with a selected config option that doesn't
> exist.

Agreed, and it simplify things.

My only concern is with triggering some of the bots while checking for 
DT compatible string definition.

Regards,
   Nicolas



-- 
Nicolas Ferre

Re: [PATCH v2 06/10] media: dt-bindings: Document SAMA5D4 VDEC bindings

[Ezequiel Garcia]

Hi Emil,

On Thu, 2021-03-25 at 14:14 +0000, Emil Velikov wrote:
> On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote:
> > 
> > On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > > 
> > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on
> > > the Microchip SAMAS5D4 SoC.
> > > 
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > Cc: devicetree@vger.kernel.org>
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > ---
> > > v2
> > >  - Newly introduced
> > >  - s/Atmel/Microchip/ (Nicolas)
> > >  - Drop leading 0 in node name/address
> > > ---
> > >  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++
> > >  1 file changed, 59 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-
> > > vdec.yaml
> > > new file mode 100644
> > > index 000000000000..9cb2c0295d54
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > > @@ -0,0 +1,59 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs
> > > +
> > > +maintainers:
> > > +  - Emil Velikov <emil.velikov@collabora.com>
> > > +
> > > +description:
> > > +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: microchip,sama5d4-vdec
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: vdec
> > 
> > Why do you need a name? *-names are used to distinguish multiple entries
> > and don't add anything if only a single entry.
> > 
> On one hand the names are used to describe the hardware - the SAMA
> board uses designated clock and interrupt lines - both called "vdec".
> Additionally the names are ultimately required by the underlying API -
> platform_get_irq_byname and  devm_clk_bulk_get respectively.
> How can we get the respective entries without the name?
> 

There are APIs to get the resource by index as opposed to by name.
E.g. platform_get_irq.

However, you are confusing here internal kernel implementation
with devicetree bindings. The latter is just a representation
of the hardware.

> Skimming through the existing dts file -
> arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of
> examples where the device tree binding contains the name for a single
> clock/interrupt.
> 


Note that dtsi and dts are the device-tree files, not the bindings.
And even if you find examples, the fact that there exist examples
doesn't mean it's the right thing to do.

Thanks,
Ezequiel

Re: [PATCH v2 06/10] media: dt-bindings: Document SAMA5D4 VDEC bindings

[Ezequiel Garcia]

Hi Rob,

Thanks for reviewing.

On Wed, 2021-03-24 at 09:17 -0600, Rob Herring wrote:
> On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> > 
> > Add devicetree binding documentation for the Hantro G1/G2 VDEC on
> > the Microchip SAMAS5D4 SoC.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org>
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> > v2
> >  - Newly introduced
> >  - s/Atmel/Microchip/ (Nicolas)
> >  - Drop leading 0 in node name/address
> > ---
> >  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-
> > vdec.yaml
> > new file mode 100644
> > index 000000000000..9cb2c0295d54
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs
> > +
> > +maintainers:
> > +  - Emil Velikov <emil.velikov@collabora.com>
> > +
> > +description:
> > +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    const: microchip,sama5d4-vdec
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: vdec
> 
> Why do you need a name? *-names are used to distinguish multiple entries 
> and don't add anything if only a single entry.
> 

I guess you are right.

I was about to argue that it makes backwards compatibility easier,
but I suspect that's not the case either.

> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: vdec_clk
> 
> And here too? These are typically named for either the name of input 
> signal (hclk, aclk, etc.) or function ('core', 'bus', etc.). 
> 

Indeed, "core" might be a better choice. But it seems we don't need
names here.

Thanks,
Ezequiel

Re: [PATCH v2 00/10] Microship SAMA5D4 VPU support et al

[Nicolas Ferre]

On 25/03/2021 at 15:22, Emil Velikov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Greetings all,
> 
> On Thu, 25 Mar 2021 at 08:48, Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
>>
>> On 24/03/2021 14:44:14+0100, Nicolas Ferre wrote:
>>> Now, when we have the tag from Rob, how to coordinate these different
>>> pieces? Will it go through the media git tree? Will we benefit from a stable
>>> branch to share or will we just have to wait for the driver to hit Mainline
>>> before adding the defconfig and DT patches?
>>>
> Thanks for the Acked-by Nicolas.
> 
>>
>> I think the defconfig and dt patches can go through at91 as soon as we
>> get Rob's ack. There is no build dependency so it can be taken at any
>> time. Worst case, we end up with a selected config option that doesn't
>> exist.
>>
> My personal preference is to merge everything in one go.
> I believe it will be easier from maintainer's point of view, plus odds
> of conflicts with the AT91 tree are close to zero.
> 
> Then again, as long as the maintainers are happy - I'm fine either way.

I'm taking defconfig 2 last patches of your series right now. No need to 
include them in subsequent versions.

For DT, I'm waiting for settlement on refined code. As indicated by 
Alexandre, changes will need to travel through arm-soc tree so we'll 
coordinate when patches are ready.

Thanks a lot! Best regards,
   Nicolas


-- 
Nicolas Ferre

Re: [PATCH v2 00/10] Microship SAMA5D4 VPU support et al

[Emil Velikov]

On Mon, 29 Mar 2021 at 10:54, Nicolas Ferre <nicolas.ferre@microchip.com> wrote:
>
> On 25/03/2021 at 15:22, Emil Velikov wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Greetings all,
> >
> > On Thu, 25 Mar 2021 at 08:48, Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> >>
> >> On 24/03/2021 14:44:14+0100, Nicolas Ferre wrote:
> >>> Now, when we have the tag from Rob, how to coordinate these different
> >>> pieces? Will it go through the media git tree? Will we benefit from a stable
> >>> branch to share or will we just have to wait for the driver to hit Mainline
> >>> before adding the defconfig and DT patches?
> >>>
> > Thanks for the Acked-by Nicolas.
> >
> >>
> >> I think the defconfig and dt patches can go through at91 as soon as we
> >> get Rob's ack. There is no build dependency so it can be taken at any
> >> time. Worst case, we end up with a selected config option that doesn't
> >> exist.
> >>
> > My personal preference is to merge everything in one go.
> > I believe it will be easier from maintainer's point of view, plus odds
> > of conflicts with the AT91 tree are close to zero.
> >
> > Then again, as long as the maintainers are happy - I'm fine either way.
>
> I'm taking defconfig 2 last patches of your series right now. No need to
> include them in subsequent versions.
>
> For DT, I'm waiting for settlement on refined code. As indicated by
> Alexandre, changes will need to travel through arm-soc tree so we'll
> coordinate when patches are ready.
>
Ack, dropped from v3 (also fixed the Microchip typo).

Thanks again
Emil

Re: [PATCH v2 06/10] media: dt-bindings: Document SAMA5D4 VDEC bindings

[Emil Velikov]

On Fri, 26 Mar 2021 at 14:29, Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> Hi Emil,
>
> On Thu, 2021-03-25 at 14:14 +0000, Emil Velikov wrote:
> > On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > >
> > > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on
> > > > the Microchip SAMAS5D4 SoC.
> > > >
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > > Cc: devicetree@vger.kernel.org>
> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > > ---
> > > > v2
> > > >  - Newly introduced
> > > >  - s/Atmel/Microchip/ (Nicolas)
> > > >  - Drop leading 0 in node name/address
> > > > ---
> > > >  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++
> > > >  1 file changed, 59 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-
> > > > vdec.yaml
> > > > new file mode 100644
> > > > index 000000000000..9cb2c0295d54
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > > > @@ -0,0 +1,59 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"
> > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > > +
> > > > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs
> > > > +
> > > > +maintainers:
> > > > +  - Emil Velikov <emil.velikov@collabora.com>
> > > > +
> > > > +description:
> > > > +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: microchip,sama5d4-vdec
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupt-names:
> > > > +    items:
> > > > +      - const: vdec
> > >
> > > Why do you need a name? *-names are used to distinguish multiple entries
> > > and don't add anything if only a single entry.
> > >
> > On one hand the names are used to describe the hardware - the SAMA
> > board uses designated clock and interrupt lines - both called "vdec".
> > Additionally the names are ultimately required by the underlying API -
> > platform_get_irq_byname and  devm_clk_bulk_get respectively.
> > How can we get the respective entries without the name?
> >
>
> There are APIs to get the resource by index as opposed to by name.
> E.g. platform_get_irq.
>
> However, you are confusing here internal kernel implementation
> with devicetree bindings. The latter is just a representation
> of the hardware.
>
My train of thought was that if we're missing API to fetch the entries
via non-name means, then there's a reasonable precedent to expose the
name. Clearly I missed the API, so my argument is just off.

> > Skimming through the existing dts file -
> > arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of
> > examples where the device tree binding contains the name for a single
> > clock/interrupt.
> >
>
>
> Note that dtsi and dts are the device-tree files, not the bindings.
> And even if you find examples, the fact that there exist examples
> doesn't mean it's the right thing to do.
>
Fully agreed. Thanks for the correction and hints.

The v3, just submitted, lacks the names all together.

-Emil