Message-ID: <80090f6e-7bc8-422a-bb2a-0c0a4abf32f0@yandex.com>
Date: Wed, 24 Jul 2024 15:20:28 +0200
From: Johan Jonker <jbx6244@...dex.com>
To: 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



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.

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

Powered by blists - more mailing lists