| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <65059bdb-4d4c-44d6-80a0-39f29f963a2e@collabora.com>
Date: Thu, 25 Jul 2024 09:58:11 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Johan Jonker <jbx6244@...dex.com>,
Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Shreeya Patel <shreeya.patel@...labora.com>, heiko@...ech.de,
mchehab@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
p.zabel@...gutronix.de, jose.abreu@...opsys.com, nelson.costa@...opsys.com,
shawn.wen@...k-chips.com, nicolas.dufresne@...labora.com,
hverkuil@...all.nl, hverkuil-cisco@...all.nl, kernel@...labora.com,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org,
Dmitry Osipenko <dmitry.osipenko@...labora.com>
Subject: Re: [PATCH v4 2/4] dt-bindings: media: Document bindings for HDMI RX
Controller
Il 24/07/24 15:20, Johan Jonker ha scritto:
>
>
> On 7/23/24 19:28, Sebastian Reichel wrote:
>> Hi,
>>
>> On Tue, Jul 23, 2024 at 01:16:00PM GMT, Johan Jonker wrote:
>>> On 7/22/24 15:53, Shreeya Patel wrote:
>>>> On Saturday, July 20, 2024 16:14 IST, Johan Jonker <jbx6244@...dex.com> wrote:
>>>>> On 7/19/24 14:40, Shreeya Patel wrote:
>>>>>> Document bindings for the Synopsys DesignWare HDMI RX Controller.
>>>>>>
>>>
>>>>>> Reviewed-by: Rob Herring <robh@...nel.org>
>>>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
>>>
>>> Remove to trigger a new review.
>>
>> Rob and Dmitry both already reviewed the version with the fallback
>> compatible. I don't think the rename of hdmirx_cma to hdmi_receiver_cma
>> warrant a new review. Also FWIW:
>>
>
>> Reviewed-by: Sebastian Reichel <sebastian.reichel@...labora.com>
>
> Please have a look at the comments below before you tag.
>
I have checked the (mostly wrong) comments before tagging.
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Regards,
Angelo
>>
>>>>>> Signed-off-by: Shreeya Patel <shreeya.patel@...labora.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v4 :-
>>>>>> - No change
>>>>>>
>>>>>> Changes in v3 :-
>>>>>> - Rename hdmirx_cma to hdmi_receiver_cma
>>>>>> - Add a Reviewed-by tag
>>>>>>
>>>>>> Changes in v2 :-
>>>>>> - Add a description for the hardware
>>>>>> - Rename resets, vo1 grf and HPD properties
>>>>>> - Add a proper description for grf and vo1-grf phandles
>>>>>> - Rename the HDMI Input node name to hdmi-receiver
>>>>>> - Improve the subject line
>>>>>> - Include gpio header file in example to fix dt_binding_check failure
>>>>>>
>>>>>> .../bindings/media/snps,dw-hdmi-rx.yaml | 132 ++++++++++++++++++
>>>>>> 1 file changed, 132 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..96ae1e2d2816
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
>>>>>> @@ -0,0 +1,132 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +# Device Tree bindings for Synopsys DesignWare HDMI RX Controller
>>>>>> +
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/media/snps,dw-hdmi-rx.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Synopsys DesignWare HDMI RX Controller
>>>>>> +
>>>>>> +maintainers:
>>>>>> + - Shreeya Patel <shreeya.patel@...labora.com>
>>>>>> +
>>>>>> +description:
>>>>>> + Synopsys DesignWare HDMI Input Controller preset on RK3588 SoCs
>>>>>> + allowing devices to receive and decode high-resolution video streams
>>>>>> + from external sources like media players, cameras, laptops, etc.
>>>>>> +
>>>>>> +properties:
>>>>>> + compatible:
>>>>>> + items:
>>>>>> + - const: rockchip,rk3588-hdmirx-ctrler
>>>>>
>>>
>>>>>> + - const: snps,dw-hdmi-rx
>>>
>>> remove
>>>
>>>>>
>
> Relevant compatible methods in use for Rockchip drivers:
>
> ===================================================================================================
>
> Compatible method #1:
> Probe is triggered by a SoC orientated string.
>
> compatible = "rockchip,rk3588-hdmirx-ctrler";
>
> If for example a new SoC rk3599 is released that has the same device properties
> then the old string can be used as fallback string.
>
> compatible = ""rockchip,rk3599-hdmirx-ctrler" , "rockchip,rk3588-hdmirx-ctrler";
>
> The driver structure:
> { .compatible = "rockchip,rk3588-hdmirx-ctrler" },
>
> ===================================================================================================
> Compatible method #2:
> Probe is triggered by a IP orientated fallback string.
>
> compatible = "rockchip,rk3588-hdmirx-ctrler" , "snps,dw-hdmi-rx";
>
> If for example a new SoC rk3599 is released that has the same device properties
> then add the same fallback string.
>
> compatible = ""rockchip,rk3599-hdmirx-ctrler" , "snps,dw-hdmi-rx";
>
> The driver structure:
> { .compatible = "snps,dw-hdmi-rx" },
>
> If for example a new SoC rk3599 is released that has NOT the same device properties
> then use method #1.
>
> The driver structure:
> { .compatible = "rockchip,rk3599-hdmirx-ctrler" .data = &rk3599_ops },
> { .compatible = "snps,dw-hdmi-rx" },
>
> ===================================================================================================
>
> Compatible method #3:
> Probe is triggered by a vendor orientated fallback string.
>
> Special case only useful if the driver is written long after all SoCs are released.
> The standalone IP has a version register and the driver can handle all the feature difference
> inside the IP depending on the version register.
>
> compatible = "rockchip,sfc";
>
> The driver structure:
> { .compatible = "rockchip,sfc"},
>
> ===================================================================================================
>
> The rules:
>
> 1: Compatible strings must be SoC orientated.
> 2: In Linux there's no priority in which string will probed first.
> 3: There is a commitment that old DT's should still work with newer kernels.
>
>>>>> What's the point of having a fallback string when there's no common code, but instead only the first string is used?
>>>>>
>>>>> +static const struct of_device_id hdmirx_id[] = {
>>>>> + { .compatible = "rockchip,rk3588-hdmirx-ctrler" },
>>>>> + { },
>>>>> +};
>>>>>
>
> The consequence of the third rule is that drivers must continue to support this string once added and
> can not be removed as suggested below.
>
> If for example the fallback is added later it will trigger 2 probes and it breaks rule #2.
> Only one of string is allowed to trigger a probe in the driver.
>
> This is wrong:
> compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
>
> { .compatible = "rockchip,rk3588-hdmirx-ctrler" },
> { .compatible = "snps,dw-hdmi-rx" },
>
> Ones a compatible method is chosen the driver must stick to it.
>
> ===================================================================================================
>
>>>>
>>>
>>>> We believe the HDMIRX driver can be used for the Synopsys IP on other SoCs
>>>> in the future, which is why we have added snps,dw-hdmi-rx as the fallback compatible.
>>>> Currently, we have tested the driver only on the RK3588 Rock5B, so we are using the
>>>> rockchip,rk3588-hdmirx-ctrler compatible in the driver instead of the fallback one.
>>>
>>> The rule that compatible strings (for internal SoC components)
>>> must be SoC orientated also applies to the fallback string.
>>> "snps,xxxx" does not refer to an independent SoC.
>
> This refers to compatible method #1.
>
>>
>> Where did you learn that? Having non-SoC specific generic fallback
>> compatibles is pretty much standard throughout the kernel. See for
>> example these RK3588 DesignWare compatibles:
>>
>> Synopsys Serial Controller:
>> Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
>> compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";
>
> Compatible method #2:
> { .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
>
>>
>> Synopsys USB3 Controller:
>> Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
>> compatible = "rockchip,rk3588-dwc3", "snps,dwc3";
>
> Compatible method #2:
> {
> .compatible = "snps,dwc3"
> },
>
>>
>> Synopsys Ethernet Controller:
>> Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
>
> Compatible method #1:
> { .compatible = "rockchip,rk3588-gmac", .data = &rk3588_ops },
>
> of_device_is_compatible(np, "snps,dwmac-4.20a") ||
>
>>
>> Synsopsys SATA Controller:
>> Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml
>> compatible = "rockchip,rk3588-dwc-ahci", "snps,dwc-ahci"
>
> Compatible method #2:
> { .compatible = "snps,dwc-ahci", &ahci_dwc_plat },
>
>>
>> It's also not specific to Synopsys (but RK3588 has a lot of Synopsys
>> design incl. the HDMI-RX IP currently worked on by Shreeya). Here
>> are some other examples:
>>
>> ARM Mali GPU:
>> Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
>> compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
>
> Should be compatible method #2:
> { .compatible = "rockchip,rk3588-mali" },
> { .compatible = "arm,mali-valhall-csf" },
>
> This is wrong!
> Each strings will trigger a probe.
> The string "rockchip,rk3588-mali" should be removed.
>
> Review was done by Collabora people and without including the Rockchip mail list.
> https://patchwork.freedesktop.org/patch/msgid/20240229162230.2634044-12-boris.brezillon@collabora.com
>
> Could someone look at this and test.
>
>>
>> Generic EHCI:
>> Documentation/devicetree/bindings/usb/generic-ehci.yaml
>> compatible = "rockchip,rk3588-ehci", "generic-ehci";
>
> compatible method #2:
> { .compatible = "generic-ehci", },
>
>>
>> As you can see almost everything in RK3588 has a non SoC specific
>> fallback :) It's also not a Rockchip/RK3588 specific thing, but
>> I think you should be able to find enough references yourself by
>> looking into the kernel's DTS files.
>
> You are mixing up 2 compatible methods.
> The driver has compatible method #1 and the DT has method #2.
>
>>
>>> Don't invent strings for devices that we don't know yet if it
>>> might or might not be compatible in the future.
>>
>> Right now it's a sensible assumption, that an operating system driver
>> for this hardware (i.e. not necessarily the one submitted by Shreeya
>> right now) can handle the Synopsys HDMI receiver hardware from different
>> SoCs just like it is the case for other Synopsys IP.
>>
>> Whatever is being done now is set in stone, since DT is considered
>> ABI. So without the fallback compatible being available in DT from
>> the beginning we need to carry the RK3588 specific compatible in the
>
>> kernel driver forever. OTOH if we add the generic one now, the kernel
>> can switch to use the generic one at any point in time and ignore the
>> RK3588 specific one.
>
> Ignoring breaks rule #3 as explained above.
>
> For you the task to select a compatible method:
>
> If the IP device registers are guaranteed remain the same then choose compatible method #2 and fix the driver.
> If in doubt choose compatible method #1 and fix the binding.
>
> Johan
>
>>
>> Greetings,
>>
>> -- Sebastian
>>
>>> Johan
>>>
>>>>
>>>>
>>>> Thanks,
>>>> Shreeya Patel
>>>>
>>>>>> +
>>>>>> + 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: axi
>>>>>> + - const: apb
>>>>>> + - const: ref
>>>>>> + - const: biu
>>>>>> +
>>>>>> + memory-region:
>>>>>> + maxItems: 1
>>>>>> +
>>>>>> + hpd-gpios:
>>>>>> + description: GPIO specifier for HPD.
>>>>>> + maxItems: 1
>>>>>> +
>>>>>> + rockchip,grf:
>>>>>> + $ref: /schemas/types.yaml#/definitions/phandle
>>>>>> + description:
>>>>>> + The phandle of the syscon node for the general register file
>>>>>> + containing HDMIRX PHY status bits.
>>>>>> +
>>>>>> + rockchip,vo1-grf:
>>>>>> + $ref: /schemas/types.yaml#/definitions/phandle
>>>>>> + description:
>>>>>> + The phandle of the syscon node for the Video Output GRF register
>>>>>> + to enable EDID transfer through SDAIN and SCLIN.
>>>>>> +
>>>>>> +required:
>>>>>> + - compatible
>>>>>> + - reg
>>>>>> + - interrupts
>>>>>> + - interrupt-names
>>>>>> + - clocks
>>>>>> + - clock-names
>>>>>> + - power-domains
>>>>>> + - resets
>>>>>> + - pinctrl-0
>>>>>> + - hpd-gpios
>>>>>> +
>>>>>> +additionalProperties: false
>>>>>> +
>>>>>> +examples:
>>>>>> + - |
>>>>>> + #include <dt-bindings/clock/rockchip,rk3588-cru.h>
>>>>>> + #include <dt-bindings/gpio/gpio.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>
>>>>>> + hdmi_receiver: hdmi-receiver@...e0000 {
>>>
>>>>>> + compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
>>>
>>> compatible = "rockchip,rk3588-hdmirx-ctrler";
>>>
>>>>>> + reg = <0xfdee0000 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>;
>>>>>> + interrupt-names = "cec", "hdmi", "dma";
>>>>>> + clocks = <&cru ACLK_HDMIRX>,
>>>>>> + <&cru CLK_HDMIRX_AUD>,
>>>>>> + <&cru CLK_CR_PARA>,
>>>>>> + <&cru PCLK_HDMIRX>,
>>>>>> + <&cru CLK_HDMIRX_REF>,
>>>>>> + <&cru PCLK_S_HDMIRX>,
>>>>>> + <&cru HCLK_VO1>;
>>>>>> + clock-names = "aclk",
>>>>>> + "audio",
>>>>>> + "cr_para",
>>>>>> + "pclk",
>>>>>> + "ref",
>>>>>> + "hclk_s_hdmirx",
>>>>>> + "hclk_vo1";
>>>>>> + power-domains = <&power RK3588_PD_VO1>;
>>>>>> + resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>,
>>>>>> + <&cru SRST_HDMIRX_REF>, <&cru SRST_A_HDMIRX_BIU>;
>>>>>> + reset-names = "axi", "apb", "ref", "biu";
>>>>>> + memory-region = <&hdmi_receiver_cma>;
>>>>>> + pinctrl-0 = <&hdmim1_rx_cec &hdmim1_rx_hpdin &hdmim1_rx_scl &hdmim1_rx_sda &hdmirx_5v_detection>;
>>>>>> + pinctrl-names = "default";
>>>>>> + hpd-gpios = <&gpio1 22 GPIO_ACTIVE_LOW>;
>>>>>> + };
>>>>
>>> _______________________________________________
>>> Kernel mailing list -- kernel@...lman.collabora.com
>>> To unsubscribe send an email to kernel-leave@...lman.collabora.com
>>> This list is managed by https://mailman.collabora.com
> _______________________________________________
> Kernel mailing list -- kernel@...lman.collabora.com
> To unsubscribe send an email to kernel-leave@...lman.collabora.com
> This list is managed by https://mailman.collabora.com
Powered by blists - more mailing lists