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

[Sebastian Reichel]

Hi,

On Wed, Jul 24, 2024 at 03:20:28PM GMT, Johan Jonker wrote:
> 
> 
> 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@xxxxxxxxxx> wrote:
> >>>> On 7/19/24 14:40, Shreeya Patel wrote:
> >>>>> Document bindings for the Synopsys DesignWare HDMI RX Controller.
> >>>>>
> >>
> >>>>> Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> >>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxx>
> 
> Please have a look at the comments below before you tag.
> 
> >
> >>>>> Signed-off-by: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>> 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@xxxxxxxxxxxxx>
> >>>>> +
> >>>>> +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:

You are arguing with kernel drivers. Drivers can be changed at any
point in time, but DT bindings cannot, because they define an ABI.

> ===================================================================================================
> 
> 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" },

This is what is being used here. The only diference is, that the
driver currently uses the RK3588 specific compatible string instead
of the fallback string right now.

If another SoC vendor adds the same IP into their latest chip and
the driver has been proven to work with their hardware the driver
can be changed to bind against "snps,dw-hdmi-rx" instead of the
RK3588 specific compatible. Doing this change will keep
compatibility with existing DTs, if we add the fallback string now.
Until then we just carry it as an unused fallback.

> ===================================================================================================
> 
> 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"},

FWIW I think _this_ is a bad example. It is missing the SoC specific
compatible making applying of quirks harder than necessary. Just
because no quirks are needed now, does not mean it will stay that
way. E.g. if an Errata gets released that SFC on RK3588 must not be
run at 1 MHz it would be super useful to have an "rockchip,rk3588-sfc"
to match against. That's the reason for the rule #1 in the following
list

> ===================================================================================================
> 
> The rules:
> 
> 1: Compatible strings must be SoC orientated.
> 2: In Linux there's no priority in which string will probed first.

I initially thought you mean the list in the driver, but after
reading your remaining mail - this is just wrong. There is a
priority. If DT specifies

compatible = "main", "fallback";

Linux will first try to bind against "main". If that does not
work it will try to bind against "fallback". To give you a
simple example from the subsystem I maintain:

DT binding: Documentation/devicetree/bindings/power/supply/sbs,sbs-battery.yaml
Linux Kernel Driver: drivers/power/supply/sbs-battery.c

Valid compatibles according to the DT binding:
 compatible = "ti,bq20z45", "sbs,sbs-battery";
 compatible = "ti,bq20z65", "sbs,sbs-battery";
 compatible = "ti,bq20z75", "sbs,sbs-battery";
 compatible = "sbs,sbs-battery";

The driver has these of_device_id entries:
 { .compatible = "sbs,sbs-battery" },
 { .compatible = "ti,bq20z65", QUIRKS },
 { .compatible = "ti,bq20z75", QUIRKS },

The driver will probe ONCE in all cases.

compatible = "ti,bq20z45", "sbs,sbs-battery"; => probe happens with fallback string
compatible = "ti,bq20z65", "sbs,sbs-battery"; => probe happens with main string (QUIRKS apply)
compatible = "ti,bq20z75", "sbs,sbs-battery"; => probe happens with main string (QUIRKS apply)
compatible = "sbs,sbs-battery"; => probe happens with main string

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

That's wrong. We can remove the "rockchip,rk3588-hdmirx-ctrler" from
the kernel driver and use the fallback string at any point in time
__IF__ we make it mandatory that rockchip,rk3588-hdmirx-ctrler must
always be followed by the fallback compatible in DT. Because then we
will keep working with old DTs, since the old DT also has the
fallback compatible.

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

I don't know how you came to that conclusion, but it's simply wrong.
The above example will probe once using the "rockchip,rk3588-hdmirx-ctrler"
compatible. At that point the DT node is marked as processed, so
no other probe happens.

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

Yeah, which is used when the IP is from the SoC vendor itself (or
unknown) or if its not possible to use the generic IP compatible
anyways.

> [...]
> If the IP device registers are guaranteed remain the same then
> choose compatible method #2 and fix the driver.

Adapting the driver is the plan, but it cannot be done right now
because lack of information. This requires either another SoC with
this IP or at least the Synopsys documentation. We only have the
Rockchip documentation.

But the driver can be fixed in the future and the DT binding is
ABI. Thus the DT binding is prepared now to allow the driver looking
like your method #2 in the future. Until then the extra compatible
will just be ignored.

-- Sebastian

Attachment: signature.asc
Description: PGP signature