Re: [PATCH v4 2/4] dt-bindings: media: Document bindings for HDMI RX Controller

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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@yandex.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@kernel.org>
>>>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.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@collabora.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@collabora.com>

Regards,
Angelo

>>
>>>>>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.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@collabora.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@fdee0000 {
>>>
>>>>>> +      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@mailman.collabora.com
>>> To unsubscribe send an email to kernel-leave@mailman.collabora.com
>>> This list is managed by https://mailman.collabora.com
> _______________________________________________
> 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