| Message ID | 20240216094922.257674-1-shreeya.patel@collabora.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Synopsys DesignWare HDMI RX Controller | expand |
On 16/02/2024 10:49, Shreeya Patel wrote: > Document bindings for the Synopsys DesignWare HDMI RX Controller. > > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > --- > .../bindings/media/snps,dw-hdmi-rx.yaml | 128 ++++++++++++++++++ > 1 file changed, 128 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml > > diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml > new file mode 100644 > index 000000000000..a70d96b548ee > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml > @@ -0,0 +1,128 @@ > +# SPDX-License-Identifier: (GPL-3.0 OR BSD-2-Clause) Use license checkpatch tells you. > +# Device Tree bindings for Synopsys DesignWare HDMI RX Controller > + > +--- > +$id: http://devicetree.org/schemas/media/snps,dw-hdmi-rx.yaml# Why this is a media, not display? Does RX means input? Lack of hardware description does not help? > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Synopsys DesignWare HDMI RX Controller > + > +maintainers: > + - Shreeya Patel <shreeya.patel@collabora.com> > + description: > +properties: > + compatible: > + items: > + - const: rockchip,rk3588-hdmirx-ctrler > + - const: snps,dw-hdmi-rx > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 3 > + > + interrupt-names: > + items: > + - const: cec > + - const: hdmi > + - const: dma > + > + clocks: > + maxItems: 7 > + > + clock-names: > + items: > + - const: aclk > + - const: audio > + - const: cr_para > + - const: pclk > + - const: ref > + - const: hclk_s_hdmirx > + - const: hclk_vo1 > + > + power-domains: > + maxItems: 1 > + > + resets: > + maxItems: 4 > + > + reset-names: > + items: > + - const: rst_a > + - const: rst_p > + - const: rst_ref > + - const: rst_biu Drop rest_ prefix > + > + pinctrl-names: > + const: default Drop > + > + memory-region: > + maxItems: 1 > + > + hdmirx-5v-detection-gpios: > + description: GPIO specifier for 5V detection. Detection of what? Isn't this HPD? > + maxItems: 1 > + > + rockchip,grf: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + The phandle of the syscon node for the GRF register. Instead describe what for. Basically 80% of your description is redundant and only "GRF register" brings some information. > + > + rockchip,vo1_grf: No underscores. > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + The phandle of the syscon node for the VO1 GRF register. Same problem. > + > +required: > + - compatible > + - reg > + - interrupts > + - interrupt-names > + - clocks > + - clock-names > + - power-domains > + - resets > + - pinctrl-0 > + - pinctrl-names Why? Drop. > + - hdmirx-5v-detection-gpios > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/rockchip,rk3588-cru.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/power/rk3588-power.h> > + #include <dt-bindings/reset/rockchip,rk3588-cru.h> > + hdmirx_ctrler: hdmirx-controller@fdee0000 { What is hdmirx-controller? Isn't this just hdmi@? Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx"; > + reg = <0x0 0xfdee0000 0x0 0x6000>; > + interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 436 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH 0>; Best regards, Krzysztof
On Friday, February 16, 2024 15:31 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 16/02/2024 10:49, Shreeya Patel wrote: > > Document bindings for the Synopsys DesignWare HDMI RX Controller. > > > > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > > A nit, subject: drop second/last, redundant "bindings for". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > > --- > > .../bindings/media/snps,dw-hdmi-rx.yaml | 128 ++++++++++++++++++ > > 1 file changed, 128 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml > > new file mode 100644 > > index 000000000000..a70d96b548ee > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml > > @@ -0,0 +1,128 @@ > > +# SPDX-License-Identifier: (GPL-3.0 OR BSD-2-Clause) > > Use license checkpatch tells you. > > > +# Device Tree bindings for Synopsys DesignWare HDMI RX Controller > > + > > +--- > > +$id: http://devicetree.org/schemas/media/snps,dw-hdmi-rx.yaml# > > Why this is a media, not display? Does RX means input? Lack of hardware > description does not help? > Yes, RX means input and this binding doc is for the HDMI INPUT controller. I'll add some description in v2 > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Synopsys DesignWare HDMI RX Controller > > + > > +maintainers: > > + - Shreeya Patel <shreeya.patel@collabora.com> > > + > > description: > > > +properties: > > + compatible: > > + items: > > + - const: rockchip,rk3588-hdmirx-ctrler > > + - const: snps,dw-hdmi-rx > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 3 > > + > > + interrupt-names: > > + items: > > + - const: cec > > + - const: hdmi > > + - const: dma > > + > > + clocks: > > + maxItems: 7 > > + > > + clock-names: > > + items: > > + - const: aclk > > + - const: audio > > + - const: cr_para > > + - const: pclk > > + - const: ref > > + - const: hclk_s_hdmirx > > + - const: hclk_vo1 > > + > > + power-domains: > > + maxItems: 1 > > + > > + resets: > > + maxItems: 4 > > + > > + reset-names: > > + items: > > + - const: rst_a > > + - const: rst_p > > + - const: rst_ref > > + - const: rst_biu > > Drop rest_ prefix > > > + > > + pinctrl-names: > > + const: default > > Drop > > > + > > + memory-region: > > + maxItems: 1 > > + > > + hdmirx-5v-detection-gpios: > > + description: GPIO specifier for 5V detection. > > Detection of what? Isn't this HPD? > > > + maxItems: 1 > > + > > + rockchip,grf: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + The phandle of the syscon node for the GRF register. > > Instead describe what for. Basically 80% of your description is > redundant and only "GRF register" brings some information. > > > > + > > + rockchip,vo1_grf: > > No underscores. > > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + The phandle of the syscon node for the VO1 GRF register. > > Same problem. > > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - interrupt-names > > + - clocks > > + - clock-names > > + - power-domains > > + - resets > > + - pinctrl-0 > > + - pinctrl-names > > Why? Drop. > > > + - hdmirx-5v-detection-gpios > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/rockchip,rk3588-cru.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/power/rk3588-power.h> > > + #include <dt-bindings/reset/rockchip,rk3588-cru.h> > > + hdmirx_ctrler: hdmirx-controller@fdee0000 { > > What is hdmirx-controller? Isn't this just hdmi@? > Writing just hdmi would imply hdmi output I think so that name will not be appropriate here. > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > This documentation doesn't have any generic name for HDMI INPUT but maybe we can use the name hdmi-receiver like some other existing binding has it here :- Documentation/devicetree/bindings/media/i2c/tda1997x.txt Thanks, Shreeya Patel > > > + compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx"; > > + reg = <0x0 0xfdee0000 0x0 0x6000>; > > + interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 436 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH 0>; > > > > Best regards, > Krzysztof > > _______________________________________________ > Kernel mailing list -- kernel@mailman.collabora.com > To unsubscribe send an email to kernel-leave@mailman.collabora.com > This list is managed by https://mailman.collabora.com
On 21/02/2024 21:55, Shreeya Patel wrote: >>> + - hdmirx-5v-detection-gpios >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/clock/rockchip,rk3588-cru.h> >>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + #include <dt-bindings/power/rk3588-power.h> >>> + #include <dt-bindings/reset/rockchip,rk3588-cru.h> >>> + hdmirx_ctrler: hdmirx-controller@fdee0000 { >> >> What is hdmirx-controller? Isn't this just hdmi@? >> > > Writing just hdmi would imply hdmi output I think so that name > will not be appropriate here. > >> Node names should be generic. See also an explanation and list of >> examples (not exhaustive) in DT specification: >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >> > > This documentation doesn't have any generic name for HDMI INPUT > but maybe we can use the name hdmi-receiver like some other existing > binding has it here :- > Documentation/devicetree/bindings/media/i2c/tda1997x.txt Yes, it is fine. You did not respond to other comments, so I assume you agree with them. Best regards, Krzysztof
This series implements support for the Synopsys DesignWare HDMI RX Controller, being compliant with standard HDMI 1.4b and HDMI 2.0. Features that are currently supported by the HDMI RX driver have been tested on rock5b board using a HDMI to micro-HDMI cable. It is recommended to use a good quality cable as there were multiple issues seen during testing the driver. Please note the below information :- * This patch series depends on series from Sebastian [0] about improving GATE_LINK support. * While testing the driver on rock5b we noticed that the binary BL31 from Rockchip contains some unknown code to get the HDMI-RX PHY access working. With TF-A BL31, the HDMI-RX PHY doesn't work as expected since there are no interrupts seen for rk_hdmirx-hdmi leading to some failures in the driver [1]. [0] https://lore.kernel.org/all/20240126182919.48402-1-sebastian.reichel@collabora.com/ [1] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/trusted-firmware-a/-/issues/1 To test the HDMI RX Controller driver, following example commands can be used :- root@debian-rockchip-rock5b-rk3588:~# v4l2-ctl --verbose -d /dev/video0 \ --set-fmt-video=width=1920,height=1080,pixelformat='BGR3' --stream-mmap=4 \ --stream-skip=3 --stream-count=100 --stream-to=/home/hdmiin4k.raw --stream-poll root@debian-rockchip-rock5b-rk3588:~# ffmpeg -f rawvideo -vcodec rawvideo \ -s 1920x1080 -r 60 -pix_fmt bgr24 -i /home/hdmiin4k.raw output.mkv Following is the v4l2-compliance test result :- root@debian-rockchip-rock5b-rk3588:~# v4l2-compliance -d /dev/video0 v4l2-compliance 1.27.0-5174, 64 bits, 64-bit time_t v4l2-compliance SHA: d700deb14368 2024-01-18 12:19:05 Compliance test for snps_hdmirx device /dev/video0: Driver Info: Driver name : snps_hdmirx Card type : snps_hdmirx Bus info : platform: snps_hdmirx Driver version : 6.8.0 Capabilities : 0x84201000 Video Capture Multiplanar Streaming Extended Pix Format Device Capabilities Device Caps : 0x04201000 Video Capture Multiplanar Streaming Extended Pix Format Required ioctls: test VIDIOC_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/video0 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK test VIDIOC_DV_TIMINGS_CAP: OK test VIDIOC_G/S_EDID: OK Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 2 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test CREATE_BUFS maximum buffers: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Total for snps_hdmirx device /dev/video0: 46, Succeeded: 46, Failed: 0, Warnings: 0 Shreeya Patel (4): clk: rockchip: rst-rk3588: Add BIU reset dt-bindings: media: Document bindings for HDMI RX Controller arm64: dts: rockchip: Add device tree support for HDMI RX Controller media: platform: synopsys: Add support for hdmi input driver .../bindings/media/snps,dw-hdmi-rx.yaml | 128 + .../boot/dts/rockchip/rk3588-pinctrl.dtsi | 41 + arch/arm64/boot/dts/rockchip/rk3588.dtsi | 50 + drivers/clk/rockchip/rst-rk3588.c | 1 + drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/synopsys/Kconfig | 3 + drivers/media/platform/synopsys/Makefile | 2 + .../media/platform/synopsys/hdmirx/Kconfig | 18 + .../media/platform/synopsys/hdmirx/Makefile | 4 + .../platform/synopsys/hdmirx/snps_hdmirx.c | 2856 +++++++++++++++++ .../platform/synopsys/hdmirx/snps_hdmirx.h | 394 +++ .../synopsys/hdmirx/snps_hdmirx_cec.c | 289 ++ .../synopsys/hdmirx/snps_hdmirx_cec.h | 46 + .../dt-bindings/reset/rockchip,rk3588-cru.h | 2 + 15 files changed, 3836 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml create mode 100644 drivers/media/platform/synopsys/Kconfig create mode 100644 drivers/media/platform/synopsys/Makefile create mode 100644 drivers/media/platform/synopsys/hdmirx/Kconfig create mode 100644 drivers/media/platform/synopsys/hdmirx/Makefile create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.c create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.h