Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6043 bytes --]

On Mon, Aug 19, 2024 at 01:29:30AM +0300, Cristian Ciocaltea wrote:
> Rockchip RK3588 SoC integrates the Synopsys DesignWare HDMI 2.1
> Quad-Pixel (QP) TX controller IP.
> 
> Since this is a new IP block, quite different from those used in the
> previous generations of Rockchip SoCs, add a dedicated binding file.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../display/rockchip/rockchip,dw-hdmi-qp.yaml      | 170 +++++++++++++++++++++
>  1 file changed, 170 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi-qp.yaml
> new file mode 100644
> index 000000000000..de470923d823
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi-qp.yaml

Filename matching the compatible please.

> @@ -0,0 +1,170 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi-qp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip DW HDMI QP TX Encoder
> +
> +maintainers:
> +  - Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> +
> +description:
> +  Rockchip RK3588 SoC integrates the Synopsys DesignWare HDMI QP TX controller
> +  IP and a HDMI/eDP TX Combo PHY based on a Samsung IP block.
> +
> +allOf:
> +  - $ref: /schemas/display/bridge/synopsys,dw-hdmi-qp.yaml#
> +  - $ref: /schemas/sound/dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3588-dw-hdmi-qp
> +
> +  clocks:
> +    minItems: 4
> +    items:
> +      - {}
> +      - {}
> +      - {}
> +      - {}

Why have you chosen to do things like this?  I find it makes things less
clear than reiterating the names of the required clocks.

> +      # The next clocks are optional, but shall be specified in this
> +      # order when present.
> +      - description: TMDS/FRL link clock
> +      - description: Video datapath clock

I don't get what you mean by optional. You have one SoC, either they are
or are not connected, unless there's multiple instances of this IP block
on the SoC and some do and some do not have these clocks?
Ditto for the interrupts.

> +
> +  clock-names:
> +    minItems: 4
> +    items:
> +      - {}
> +      - {}
> +      - {}
> +      - {}
> +      - enum: [hdp, hclk_vo1]
> +      - const: hclk_vo1
> +
> +  interrupts:
> +    items:
> +      - {}
> +      - {}
> +      - {}
> +      - {}
> +      - description: HPD interrupt
> +
> +  interrupt-names:
> +    items:
> +      - {}
> +      - {}
> +      - {}
> +      - {}
> +      - const: hpd
> +
> +  phys:
> +    maxItems: 1
> +    description: The HDMI/eDP PHY.
> +
> +  phy-names:
> +    const: hdmi
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    minItems: 2
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: ref
> +      - const: hdp
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +  rockchip,grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Most HDMI QP related data is accessed through SYS GRF regs.
> +
> +  rockchip,vo1-grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Additional HDMI QP related data is accessed through VO1 GRF regs.

Why are these required? What prevents you looking up the syscons by
compatible?

Cheers,
Conor.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - phys
> +  - phy-names
> +  - ports
> +  - resets
> +  - reset-names
> +  - rockchip,grf
> +  - rockchip,vo1-grf
> +
> +unevaluatedProperties: 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>
> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      hdmi@fde80000 {
> +        compatible = "rockchip,rk3588-dw-hdmi-qp";
> +        reg = <0x0 0xfde80000 0x0 0x20000>;
> +        clocks = <&cru PCLK_HDMITX0>,
> +                 <&cru CLK_HDMITX0_EARC>,
> +                 <&cru CLK_HDMITX0_REF>,
> +                 <&cru MCLK_I2S5_8CH_TX>,
> +                 <&cru CLK_HDMIHDP0>,
> +                 <&cru HCLK_VO1>;
> +        clock-names = "pclk", "earc", "ref", "aud", "hdp", "hclk_vo1";
> +        interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH 0>,
> +                     <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH 0>,
> +                     <GIC_SPI 171 IRQ_TYPE_LEVEL_HIGH 0>,
> +                     <GIC_SPI 172 IRQ_TYPE_LEVEL_HIGH 0>,
> +                     <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH 0>;
> +        interrupt-names = "avp", "cec", "earc", "main", "hpd";
> +        phys = <&hdptxphy_hdmi0>;
> +        phy-names = "hdmi";
> +        power-domains = <&power RK3588_PD_VO1>;
> +        resets = <&cru SRST_HDMITX0_REF>, <&cru SRST_HDMIHDP0>;
> +        reset-names = "ref", "hdp";
> +        rockchip,grf = <&sys_grf>;
> +        rockchip,vo1-grf = <&vo1_grf>;
> +        #sound-dai-cells = <0>;
> +
> +        ports {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          port@0 {
> +            reg = <0>;
> +
> +            hdmi0_in_vp0: endpoint {
> +                remote-endpoint = <&vp0_out_hdmi0>;
> +            };
> +          };
> +
> +          port@1 {
> +            reg = <1>;
> +
> +            hdmi0_out_con0: endpoint {
> +                remote-endpoint = <&hdmi_con0_in>;
> +            };
> +          };
> +        };
> +      };
> +    };
> 
> -- 
> 2.46.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5649 bytes --]

On Tue, Aug 20, 2024 at 03:37:44PM +0300, Cristian Ciocaltea wrote:
> On 8/19/24 7:53 PM, Conor Dooley wrote:
> > On Mon, Aug 19, 2024 at 01:29:30AM +0300, Cristian Ciocaltea wrote:
> >> Rockchip RK3588 SoC integrates the Synopsys DesignWare HDMI 2.1
> >> Quad-Pixel (QP) TX controller IP.
> >>
> >> Since this is a new IP block, quite different from those used in the
> >> previous generations of Rockchip SoCs, add a dedicated binding file.
> >>
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> ---
> >>  .../display/rockchip/rockchip,dw-hdmi-qp.yaml      | 170 +++++++++++++++++++++
> >>  1 file changed, 170 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi-qp.yaml
> >> new file mode 100644
> >> index 000000000000..de470923d823
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi-qp.yaml
> > 
> > Filename matching the compatible please.
> 
> RK3588 happens to be the first Rockchip SoC using the QP TX controller, but
> more are expected to come, e.g. RK3576.  Should we add 'rk3588-' to the
> filename and let it being dropped when the 2nd SoC is added?

Yes to the former, no to the latter.

> 
> >> @@ -0,0 +1,170 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/rockchip/rockchip,dw-hdmi-qp.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Rockchip DW HDMI QP TX Encoder
> >> +
> >> +maintainers:
> >> +  - Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> +
> >> +description:
> >> +  Rockchip RK3588 SoC integrates the Synopsys DesignWare HDMI QP TX controller
> >> +  IP and a HDMI/eDP TX Combo PHY based on a Samsung IP block.
> >> +
> >> +allOf:
> >> +  - $ref: /schemas/display/bridge/synopsys,dw-hdmi-qp.yaml#
> >> +  - $ref: /schemas/sound/dai-common.yaml#
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - rockchip,rk3588-dw-hdmi-qp
> >> +
> >> +  clocks:
> >> +    minItems: 4
> >> +    items:
> >> +      - {}
> >> +      - {}
> >> +      - {}
> >> +      - {}
> > 
> > Why have you chosen to do things like this?  I find it makes things less
> > clear than reiterating the names of the required clocks.
> 
> I've just followed the approach used in rockchip,dw-hdmi.yaml.  Personally,
> I preferred this for making a clear distinction between common and custom
> items, in addition to reducing content dupplication. 
> 
> If readability is more important/desired, I will expand the items.  For
> consistency, I assume clock-names, interrupts and interrupt-names below
> should be treated similarly.

I don't feel particularly strongly here FWIW. If you chose to do it, do
it for all properties, yes.

> >> +      # The next clocks are optional, but shall be specified in this
> >> +      # order when present.
> >> +      - description: TMDS/FRL link clock
> >> +      - description: Video datapath clock
> > 
> > I don't get what you mean by optional. You have one SoC, either they are
> > or are not connected, unless there's multiple instances of this IP block
> > on the SoC and some do and some do not have these clocks?
> > Ditto for the interrupts.
> 
> They were handled as such in vendor tree, probably assuming other SoC
> variants might not need them.  I agree it doesn't make sense to have them
> optional at this point.  Will fix this in the next revision.
> 
> >> +
> >> +  clock-names:
> >> +    minItems: 4
> >> +    items:
> >> +      - {}
> >> +      - {}
> >> +      - {}
> >> +      - {}
> >> +      - enum: [hdp, hclk_vo1]
> >> +      - const: hclk_vo1
> >> +
> >> +  interrupts:
> >> +    items:
> >> +      - {}
> >> +      - {}
> >> +      - {}
> >> +      - {}
> >> +      - description: HPD interrupt
> >> +
> >> +  interrupt-names:
> >> +    items:
> >> +      - {}
> >> +      - {}
> >> +      - {}
> >> +      - {}
> >> +      - const: hpd
> >> +
> >> +  phys:
> >> +    maxItems: 1
> >> +    description: The HDMI/eDP PHY.
> >> +
> >> +  phy-names:
> >> +    const: hdmi
> >> +
> >> +  power-domains:
> >> +    maxItems: 1
> >> +
> >> +  resets:
> >> +    minItems: 2
> >> +    maxItems: 2
> >> +
> >> +  reset-names:
> >> +    items:
> >> +      - const: ref
> >> +      - const: hdp
> >> +
> >> +  "#sound-dai-cells":
> >> +    const: 0
> >> +
> >> +  rockchip,grf:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle
> >> +    description:
> >> +      Most HDMI QP related data is accessed through SYS GRF regs.
> >> +
> >> +  rockchip,vo1-grf:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle
> >> +    description:
> >> +      Additional HDMI QP related data is accessed through VO1 GRF regs.
> > 
> > Why are these required? What prevents you looking up the syscons by
> > compatible?
> 
> That is for getting the proper instance:

Ah, that makes sense. I am, however, curious why these have the same
compatible when they have different sized regions allocated to them.

> 	vo0_grf: syscon@fd5a6000 {
> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> 		reg = <0x0 0xfd5a6000 0x0 0x2000>;
> 		clocks = <&cru PCLK_VO0GRF>;
> 	};
> 
> 	vo1_grf: syscon@fd5a8000 {
> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> 		reg = <0x0 0xfd5a8000 0x0 0x100>;
> 		clocks = <&cru PCLK_VO1GRF>;
> 	};
> 
> Thanks for reviewing,
> Cristian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

On Tue, Aug 20, 2024 at 11:12:45PM +0300, Cristian Ciocaltea wrote:
> On 8/20/24 7:14 PM, Conor Dooley wrote:
> > On Tue, Aug 20, 2024 at 03:37:44PM +0300, Cristian Ciocaltea wrote:
> >> On 8/19/24 7:53 PM, Conor Dooley wrote:
> >>> On Mon, Aug 19, 2024 at 01:29:30AM +0300, Cristian Ciocaltea wrote:
> >>>> +  rockchip,grf:
> >>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>> +    description:
> >>>> +      Most HDMI QP related data is accessed through SYS GRF regs.
> >>>> +
> >>>> +  rockchip,vo1-grf:
> >>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>> +    description:
> >>>> +      Additional HDMI QP related data is accessed through VO1 GRF regs.
> >>>
> >>> Why are these required? What prevents you looking up the syscons by
> >>> compatible?
> >>
> >> That is for getting the proper instance:
> > 
> > Ah, that makes sense. I am, however, curious why these have the same
> > compatible when they have different sized regions allocated to them.
> 
> Good question, didn't notice.  I've just checked the TRM and, in both
> cases, the maximum register offset is within the 0x100 range.  Presumably
> this is nothing but an inconsistency, as the syscons have been added in
> separate commits.

Is that TRM publicly available? I do find it curious that devices sound
like they have different contents have the same compatible. In my view,
that is incorrect and they should have unique compatibles if the
contents (and therefore the programming model) differs.

> 
> >> 	vo0_grf: syscon@fd5a6000 {
> >> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> >> 		reg = <0x0 0xfd5a6000 0x0 0x2000>;
> >> 		clocks = <&cru PCLK_VO0GRF>;
> >> 	};
> >>
> >> 	vo1_grf: syscon@fd5a8000 {
> >> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> >> 		reg = <0x0 0xfd5a8000 0x0 0x100>;
> >> 		clocks = <&cru PCLK_VO1GRF>;
> >> 	};


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2774 bytes --]

Cristian, Heiko,

On Wed, Aug 21, 2024 at 11:38:01PM +0300, Cristian Ciocaltea wrote:
> On 8/21/24 6:07 PM, Conor Dooley wrote:
> > On Tue, Aug 20, 2024 at 11:12:45PM +0300, Cristian Ciocaltea wrote:
> >> On 8/20/24 7:14 PM, Conor Dooley wrote:
> >>> On Tue, Aug 20, 2024 at 03:37:44PM +0300, Cristian Ciocaltea wrote:
> >>>> On 8/19/24 7:53 PM, Conor Dooley wrote:
> >>>>> On Mon, Aug 19, 2024 at 01:29:30AM +0300, Cristian Ciocaltea wrote:
> >>>>>> +  rockchip,grf:
> >>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>> +    description:
> >>>>>> +      Most HDMI QP related data is accessed through SYS GRF regs.
> >>>>>> +
> >>>>>> +  rockchip,vo1-grf:
> >>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>> +    description:
> >>>>>> +      Additional HDMI QP related data is accessed through VO1 GRF regs.
> >>>>>
> >>>>> Why are these required? What prevents you looking up the syscons by
> >>>>> compatible?
> >>>>
> >>>> That is for getting the proper instance:
> >>>
> >>> Ah, that makes sense. I am, however, curious why these have the same
> >>> compatible when they have different sized regions allocated to them.
> >>
> >> Good question, didn't notice.  I've just checked the TRM and, in both
> >> cases, the maximum register offset is within the 0x100 range.  Presumably
> >> this is nothing but an inconsistency, as the syscons have been added in
> >> separate commits.
> > 
> > Is that TRM publicly available? I do find it curious that devices sound
> > like they have different contents have the same compatible. In my view,
> > that is incorrect and they should have unique compatibles if the
> > contents (and therefore the programming model) differs.
> 
> Don't know if there's an official location to get it from, but a quick
> search on internet shows a few repos providing them, e.g. [1].
> 
> Comparing "6.14 VO0_GRF Register Description" at pg. 777 with "6.15 VO1_GRF
> Register Description" at pg. 786 (from Part1) reveals the layout is mostly
> similar, with a few variations though.

Page references and everything, thank you very much. I don't think those
two GRFs should have the same compatibles, they're, as you say, similar
but not identical. Seems like a bug to me!

Heiko, what do you think?

> [1] https://github.com/FanX-Tek/rk3588-TRM-and-Datasheet
> 
> >>
> >>>> 	vo0_grf: syscon@fd5a6000 {
> >>>> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> >>>> 		reg = <0x0 0xfd5a6000 0x0 0x2000>;
> >>>> 		clocks = <&cru PCLK_VO0GRF>;
> >>>> 	};
> >>>>
> >>>> 	vo1_grf: syscon@fd5a8000 {
> >>>> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> >>>> 		reg = <0x0 0xfd5a8000 0x0 0x100>;
> >>>> 		clocks = <&cru PCLK_VO1GRF>;
> >>>> 	};
> >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Heiko Stuebner]

Am 21. August 2024 23:28:55 MESZ schrieb Conor Dooley <conor@kernel.org>:
>Cristian, Heiko,
>
>On Wed, Aug 21, 2024 at 11:38:01PM +0300, Cristian Ciocaltea wrote:
>> On 8/21/24 6:07 PM, Conor Dooley wrote:
>> > On Tue, Aug 20, 2024 at 11:12:45PM +0300, Cristian Ciocaltea wrote:
>> >> On 8/20/24 7:14 PM, Conor Dooley wrote:
>> >>> On Tue, Aug 20, 2024 at 03:37:44PM +0300, Cristian Ciocaltea wrote:
>> >>>> On 8/19/24 7:53 PM, Conor Dooley wrote:
>> >>>>> On Mon, Aug 19, 2024 at 01:29:30AM +0300, Cristian Ciocaltea wrote:
>> >>>>>> +  rockchip,grf:
>> >>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> >>>>>> +    description:
>> >>>>>> +      Most HDMI QP related data is accessed through SYS GRF regs.
>> >>>>>> +
>> >>>>>> +  rockchip,vo1-grf:
>> >>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> >>>>>> +    description:
>> >>>>>> +      Additional HDMI QP related data is accessed through VO1 GRF regs.
>> >>>>>
>> >>>>> Why are these required? What prevents you looking up the syscons by
>> >>>>> compatible?
>> >>>>
>> >>>> That is for getting the proper instance:
>> >>>
>> >>> Ah, that makes sense. I am, however, curious why these have the same
>> >>> compatible when they have different sized regions allocated to them.
>> >>
>> >> Good question, didn't notice.  I've just checked the TRM and, in both
>> >> cases, the maximum register offset is within the 0x100 range.  Presumably
>> >> this is nothing but an inconsistency, as the syscons have been added in
>> >> separate commits.
>> > 
>> > Is that TRM publicly available? I do find it curious that devices sound
>> > like they have different contents have the same compatible. In my view,
>> > that is incorrect and they should have unique compatibles if the
>> > contents (and therefore the programming model) differs.
>> 
>> Don't know if there's an official location to get it from, but a quick
>> search on internet shows a few repos providing them, e.g. [1].
>> 
>> Comparing "6.14 VO0_GRF Register Description" at pg. 777 with "6.15 VO1_GRF
>> Register Description" at pg. 786 (from Part1) reveals the layout is mostly
>> similar, with a few variations though.
>
>Page references and everything, thank you very much. I don't think those
>two GRFs should have the same compatibles, they're, as you say, similar
>but not identical. Seems like a bug to me!
>
>Heiko, what do you think?

Yes, while the register names sound similar, looking at the bit definitions this evening revealed that they handle vastly different settings.

So I guess we should fix the compatibles. They are all about graphics stuff and HDMI actually is the first output, so right now WE can at least still claim the no-users joker ;-)


Heiko

>
>> [1] https://github.com/FanX-Tek/rk3588-TRM-and-Datasheet
>> 
>> >>
>> >>>> 	vo0_grf: syscon@fd5a6000 {
>> >>>> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
>> >>>> 		reg = <0x0 0xfd5a6000 0x0 0x2000>;
>> >>>> 		clocks = <&cru PCLK_VO0GRF>;
>> >>>> 	};
>> >>>>
>> >>>> 	vo1_grf: syscon@fd5a8000 {
>> >>>> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
>> >>>> 		reg = <0x0 0xfd5a8000 0x0 0x100>;
>> >>>> 		clocks = <&cru PCLK_VO1GRF>;
>> >>>> 	};
>> >

-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Heiko Stübner]

Am Donnerstag, 22. August 2024, 01:22:16 CEST schrieb Cristian Ciocaltea:
> On 8/22/24 12:38 AM, Heiko Stuebner wrote:
> > 
> > 
> > Am 21. August 2024 23:28:55 MESZ schrieb Conor Dooley <conor@kernel.org>:
> >> Cristian, Heiko,
> >>
> >> On Wed, Aug 21, 2024 at 11:38:01PM +0300, Cristian Ciocaltea wrote:
> >>> On 8/21/24 6:07 PM, Conor Dooley wrote:
> >>>> On Tue, Aug 20, 2024 at 11:12:45PM +0300, Cristian Ciocaltea wrote:
> >>>>> On 8/20/24 7:14 PM, Conor Dooley wrote:
> >>>>>> On Tue, Aug 20, 2024 at 03:37:44PM +0300, Cristian Ciocaltea wrote:
> >>>>>>> On 8/19/24 7:53 PM, Conor Dooley wrote:
> >>>>>>>> On Mon, Aug 19, 2024 at 01:29:30AM +0300, Cristian Ciocaltea wrote:
> >>>>>>>>> +  rockchip,grf:
> >>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>>>>> +    description:
> >>>>>>>>> +      Most HDMI QP related data is accessed through SYS GRF regs.
> >>>>>>>>> +
> >>>>>>>>> +  rockchip,vo1-grf:
> >>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>>>>> +    description:
> >>>>>>>>> +      Additional HDMI QP related data is accessed through VO1 GRF regs.
> >>>>>>>>
> >>>>>>>> Why are these required? What prevents you looking up the syscons by
> >>>>>>>> compatible?
> >>>>>>>
> >>>>>>> That is for getting the proper instance:
> >>>>>>
> >>>>>> Ah, that makes sense. I am, however, curious why these have the same
> >>>>>> compatible when they have different sized regions allocated to them.
> >>>>>
> >>>>> Good question, didn't notice.  I've just checked the TRM and, in both
> >>>>> cases, the maximum register offset is within the 0x100 range.  Presumably
> >>>>> this is nothing but an inconsistency, as the syscons have been added in
> >>>>> separate commits.
> >>>>
> >>>> Is that TRM publicly available? I do find it curious that devices sound
> >>>> like they have different contents have the same compatible. In my view,
> >>>> that is incorrect and they should have unique compatibles if the
> >>>> contents (and therefore the programming model) differs.
> >>>
> >>> Don't know if there's an official location to get it from, but a quick
> >>> search on internet shows a few repos providing them, e.g. [1].
> >>>
> >>> Comparing "6.14 VO0_GRF Register Description" at pg. 777 with "6.15 VO1_GRF
> >>> Register Description" at pg. 786 (from Part1) reveals the layout is mostly
> >>> similar, with a few variations though.
> >>
> >> Page references and everything, thank you very much. I don't think those
> >> two GRFs should have the same compatibles, they're, as you say, similar
> >> but not identical. Seems like a bug to me!
> >>
> >> Heiko, what do you think?
> > 
> > Yes, while the register names sound similar, looking at the bit
> > definitions this evening revealed that they handle vastly different
> > settings.
> > 
> > So I guess we should fix the compatibles. They are all about graphics
> > stuff and HDMI actually is the first output, so right now WE can at least
> > still claim the no-users joker ;-)
> 
> I couldn't find any driver doing a lookup for them by compatible, so I
> think it's fine to fix them - should we go for "rockchip,rk3588-vo0-grf" and
> "rockchip,rk3588-vo1-grf", respectively?

yep. While things like the MIPICDPHY{0,1}_GRF really are identifcal and
serve two separate controllers ... vo0 and vo1 are very different entities,
so fixing the compatible to reflect that makes a lot of sense.


> vo0_grf seems to be used by the usbdp phy nodes:
> 
>     usbdp_phy0: phy@fed80000 {
>         compatible = "rockchip,rk3588-usbdp-phy";
>         [...]
>         rockchip,vo-grf = <&vo0_grf>;
>         [...]
> 
> Same for "usbdp_phy1: phy@fed90000".
> 
> While vo1_grf is present in:
> 
>     vop: vop@fdd90000 {
>         compatible = "rockchip,rk3588-vop";
>         [...]
>         rockchip,vo1-grf = <&vo1_grf>;
>         [...]
> 
> I guess it's too late to drop them while updating the related drivers
> accordingly, hence I wonder if we should keep using the phandles for this
> HDMI thing as well, for consistency reasons.

For the property naming, I guess it just tells the driver which "vo"-grf
to use, so the vop is more explicit in naming it vo1-grf even the vo-grf
in the usbdp phy won't hurt too much.

Of course we can still look up the grf by compatible and deprecate the
phandle references.


@Conor: just for me, did some shift happen in our understanding of dt-
best-practices in terms of syscon via phandle vs. syscon via compatible?

Because Rockchip boards are referencing their GRFs via phandes forever
but similar to the soc vs non-soc node thing, I'd like to stay on top of
best-practices ;-)


Heiko


> > Heiko
> > 
> >>
> >>> [1] https://github.com/FanX-Tek/rk3588-TRM-and-Datasheet
> >>>
> >>>>>
> >>>>>>> 	vo0_grf: syscon@fd5a6000 {
> >>>>>>> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> >>>>>>> 		reg = <0x0 0xfd5a6000 0x0 0x2000>;
> >>>>>>> 		clocks = <&cru PCLK_VO0GRF>;
> >>>>>>> 	};
> >>>>>>>
> >>>>>>> 	vo1_grf: syscon@fd5a8000 {
> >>>>>>> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> >>>>>>> 		reg = <0x0 0xfd5a8000 0x0 0x100>;
> >>>>>>> 		clocks = <&cru PCLK_VO1GRF>;
> >>>>>>> 	};
> >>>>
> > 
> 

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

On Thu, Aug 22, 2024 at 09:01:34AM +0200, Heiko Stübner wrote:
> @Conor: just for me, did some shift happen in our understanding of dt-
> best-practices in terms of syscon via phandle vs. syscon via compatible?
> 
> Because Rockchip boards are referencing their GRFs via phandes forever
> but similar to the soc vs non-soc node thing, I'd like to stay on top of
> best-practices ;-)

If IP blocks, and thus drivers, are going to be reused between devices,
using the phandles makes sense given that it is unlikely that syscon
nodes can make use of fallback compatibles due to bits within that "glue"
changing between devices. It also makes sense when there are multiple
instances of an IP on the device, which need to use different syscons.
My goal is to ask people why they are using these type of syscons
phandle properties, cos often they are not required at all - for example
with clocks where you effectively need a whole new driver for every
single soc and having a phandle property buys you nothing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re:Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Andy Yan]

Hi，

在 2024-08-22 19:59:43，"Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> 写道：
>On 8/22/24 11:41 AM, Conor Dooley wrote:
>> On Thu, Aug 22, 2024 at 09:01:34AM +0200, Heiko Stübner wrote:
>>> @Conor: just for me, did some shift happen in our understanding of dt-
>>> best-practices in terms of syscon via phandle vs. syscon via compatible?
>>>
>>> Because Rockchip boards are referencing their GRFs via phandes forever
>>> but similar to the soc vs non-soc node thing, I'd like to stay on top of
>>> best-practices ;-)
>> 
>> If IP blocks, and thus drivers, are going to be reused between devices,
>> using the phandles makes sense given that it is unlikely that syscon
>> nodes can make use of fallback compatibles due to bits within that "glue"
>> changing between devices. It also makes sense when there are multiple
>> instances of an IP on the device, which need to use different syscons.
>> My goal is to ask people why they are using these type of syscons
>> phandle properties, cos often they are not required at all - for example
>> with clocks where you effectively need a whole new driver for every
>> single soc and having a phandle property buys you nothing.
>
>That would be also the case for this HDMI controller - need to check the
>specs for the newer RK3576 SoC, but I expect the syscons would be quite
>different when compared to RK3588, hence we should keep making use of
>the phandles.


Yes，for rk3576，it shares the same HDMI IP block（hdmi controller and PHY），
of course reuse the driver of rk3588， but it has different GRF to depends on[0]:
which calls ioc_grf and vo0_grf:

I also believe that makeing use of phandle beneficial for different devices to reuse the same code.

hdmi: hdmi@27da0000 {
                compatible = "rockchip,rk3576-dw-hdmi";
                reg = <0x0 0x27da0000 0x0 0x10000>, <0x0 0x27db0000 0x0 0x10000>;
                interrupts = <GIC_SPI 338 IRQ_TYPE_LEVEL_HIGH>,
                             <GIC_SPI 339 IRQ_TYPE_LEVEL_HIGH>,
                             <GIC_SPI 340 IRQ_TYPE_LEVEL_HIGH>,
                             <GIC_SPI 341 IRQ_TYPE_LEVEL_HIGH>,
                             <GIC_SPI 367 IRQ_TYPE_LEVEL_HIGH>;
,            rockchip,grf = <&ioc_grf>;
             rockchip,vo0_grf = <&vo0_grf>;
             phys = <&hdptxphy_hdmi>;
             phy-names = "hdmi";


[0]https://github.com/armbian/linux-rockchip/blob/rk-6.1-rkr3/arch/arm64/boot/dts/rockchip/rk3576.dtsi#L3122C2-L3123C33

>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3335 bytes --]

On Fri, Aug 23, 2024 at 09:01:51AM +0800, Andy Yan wrote:
> 
> Hi，
> 
> 在 2024-08-22 19:59:43，"Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> 写道：
> >On 8/22/24 11:41 AM, Conor Dooley wrote:
> >> On Thu, Aug 22, 2024 at 09:01:34AM +0200, Heiko Stübner wrote:
> >>> @Conor: just for me, did some shift happen in our understanding of dt-
> >>> best-practices in terms of syscon via phandle vs. syscon via compatible?
> >>>
> >>> Because Rockchip boards are referencing their GRFs via phandes forever
> >>> but similar to the soc vs non-soc node thing, I'd like to stay on top of
> >>> best-practices ;-)
> >> 
> >> If IP blocks, and thus drivers, are going to be reused between devices,
> >> using the phandles makes sense given that it is unlikely that syscon
> >> nodes can make use of fallback compatibles due to bits within that "glue"
> >> changing between devices. It also makes sense when there are multiple
> >> instances of an IP on the device, which need to use different syscons.
> >> My goal is to ask people why they are using these type of syscons
> >> phandle properties, cos often they are not required at all - for example
> >> with clocks where you effectively need a whole new driver for every
> >> single soc and having a phandle property buys you nothing.
> >
> >That would be also the case for this HDMI controller - need to check the
> >specs for the newer RK3576 SoC, but I expect the syscons would be quite
> >different when compared to RK3588, hence we should keep making use of
> >the phandles.
> 
> 
> Yes，for rk3576，it shares the same HDMI IP block（hdmi controller and PHY），
> of course reuse the driver of rk3588， but it has different GRF to depends on[0]:
> which calls ioc_grf and vo0_grf:
> 
> I also believe that makeing use of phandle beneficial for different devices to reuse the same code.
> 
> hdmi: hdmi@27da0000 {
>                 compatible = "rockchip,rk3576-dw-hdmi";
>                 reg = <0x0 0x27da0000 0x0 0x10000>, <0x0 0x27db0000 0x0 0x10000>;
>                 interrupts = <GIC_SPI 338 IRQ_TYPE_LEVEL_HIGH>,
>                              <GIC_SPI 339 IRQ_TYPE_LEVEL_HIGH>,
>                              <GIC_SPI 340 IRQ_TYPE_LEVEL_HIGH>,
>                              <GIC_SPI 341 IRQ_TYPE_LEVEL_HIGH>,
>                              <GIC_SPI 367 IRQ_TYPE_LEVEL_HIGH>;
> ,            rockchip,grf = <&ioc_grf>;
>              rockchip,vo0_grf = <&vo0_grf>;

btw, I don't particularly like this naming - on another soc in the
future "vo0_grf" could be "vo1_grf". It is better to name them after
what they are providing to the hdmi controller, rather than what the grf
itself is called.

That said, if the grf is changing between socs, the offset within the
grf and what it provides to the hdmi controller may vary completely,
which makes having generic grf reference properties redundant.

>              phys = <&hdptxphy_hdmi>;
>              phy-names = "hdmi";
> 
> 
> [0]https://github.com/armbian/linux-rockchip/blob/rk-6.1-rkr3/arch/arm64/boot/dts/rockchip/rk3576.dtsi#L3122C2-L3123C33
> 
> >
> >_______________________________________________
> >Linux-rockchip mailing list
> >Linux-rockchip@lists.infradead.org
> >http://lists.infradead.org/mailman/listinfo/linux-rockchip

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Heiko Stübner]

Am Donnerstag, 22. August 2024, 10:41:10 CEST schrieb Conor Dooley:
> On Thu, Aug 22, 2024 at 09:01:34AM +0200, Heiko Stübner wrote:
> > @Conor: just for me, did some shift happen in our understanding of dt-
> > best-practices in terms of syscon via phandle vs. syscon via compatible?
> > 
> > Because Rockchip boards are referencing their GRFs via phandes forever
> > but similar to the soc vs non-soc node thing, I'd like to stay on top of
> > best-practices ;-)
> 
> If IP blocks, and thus drivers, are going to be reused between devices,
> using the phandles makes sense given that it is unlikely that syscon
> nodes can make use of fallback compatibles due to bits within that "glue"
> changing between devices. It also makes sense when there are multiple
> instances of an IP on the device, which need to use different syscons.
> My goal is to ask people why they are using these type of syscons
> phandle properties, cos often they are not required at all - for example
> with clocks where you effectively need a whole new driver for every
> single soc and having a phandle property buys you nothing.

I guess I'm of two minds here.

For me at least it makes sense to spell out the dependency to the
syscon in the devicetree and not just have that hidden away inside the
driver.

But on the other hand, we already have the per-soc configuration [0]
defining which grf bits needs to be accessed, so adding a

	.lanecfg1_grf_compat = "rockchip,rk3568-vo"

would not create overhad, as the grf regs and bits and rearranged
all the time anyway.


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n1652
taking DSI as an example, where this is even more obvious

Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

On Fri, Aug 23, 2024 at 12:47:50PM +0200, Heiko Stübner wrote:
> Am Donnerstag, 22. August 2024, 10:41:10 CEST schrieb Conor Dooley:
> > On Thu, Aug 22, 2024 at 09:01:34AM +0200, Heiko Stübner wrote:
> > > @Conor: just for me, did some shift happen in our understanding of dt-
> > > best-practices in terms of syscon via phandle vs. syscon via compatible?
> > > 
> > > Because Rockchip boards are referencing their GRFs via phandes forever
> > > but similar to the soc vs non-soc node thing, I'd like to stay on top of
> > > best-practices ;-)
> > 
> > If IP blocks, and thus drivers, are going to be reused between devices,
> > using the phandles makes sense given that it is unlikely that syscon
> > nodes can make use of fallback compatibles due to bits within that "glue"
> > changing between devices. It also makes sense when there are multiple
> > instances of an IP on the device, which need to use different syscons.
> > My goal is to ask people why they are using these type of syscons
> > phandle properties, cos often they are not required at all - for example
> > with clocks where you effectively need a whole new driver for every
> > single soc and having a phandle property buys you nothing.
> 
> I guess I'm of two minds here.
> 
> For me at least it makes sense to spell out the dependency to the
> syscon in the devicetree and not just have that hidden away inside the
> driver.
> 
> But on the other hand, we already have the per-soc configuration [0]
> defining which grf bits needs to be accessed, so adding a
> 
> 	.lanecfg1_grf_compat = "rockchip,rk3568-vo"
> 
> would not create overhad, as the grf regs and bits and rearranged
> all the time anyway.

Right, that's the other side of it. Raw phandles aren't that great if
the bits are gonna move around the register and you have to use the
match data to figure out where they are. phandle + offset is the other
option for that kind of scenario, particularly in cases where there are
multiple copies of a block on an SoC and each uses either a different
syscon or a different set of registers within one.

> 
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n1652
> taking DSI as an example, where this is even more obvious
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 2/4] drm/bridge: synopsys: Add DW HDMI QP TX Controller support library

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On Mon, Aug 19, 2024 at 01:29:29AM GMT, Cristian Ciocaltea wrote:
> +static irqreturn_t dw_hdmi_qp_main_hardirq(int irq, void *dev_id)
> +{
> +	struct dw_hdmi_qp *hdmi = dev_id;
> +	struct dw_hdmi_qp_i2c *i2c = hdmi->i2c;
> +	u32 stat;
> +
> +	stat = dw_hdmi_qp_read(hdmi, MAINUNIT_1_INT_STATUS);
> +
> +	i2c->stat = stat & (I2CM_OP_DONE_IRQ | I2CM_READ_REQUEST_IRQ |
> +			    I2CM_NACK_RCVD_IRQ);
> +
> +	if (i2c->stat) {
> +		dw_hdmi_qp_write(hdmi, i2c->stat, MAINUNIT_1_INT_CLEAR);
> +		complete(&i2c->cmp);
> +	}
> +
> +	if (stat)
> +		return IRQ_HANDLED;
> +
> +	return IRQ_NONE;
> +}

If the scrambler is enabled, you need to deal with hotplug. On hotplug,
the monitor will drop its TMDS ratio and scrambling status, but the
driver will keep assuming it's been programmed.

If you don't have a way to deal with hotplug yet, then I'd suggest to
just drop the scrambler setup for now.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/bridge: synopsys: Add DW HDMI QP TX Controller support library

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]

On Sat, Aug 31, 2024 at 01:21:48AM GMT, Cristian Ciocaltea wrote:
> On 8/27/24 11:58 AM, Maxime Ripard wrote:
> > On Mon, Aug 19, 2024 at 01:29:29AM GMT, Cristian Ciocaltea wrote:
> >> +static irqreturn_t dw_hdmi_qp_main_hardirq(int irq, void *dev_id)
> >> +{
> >> +	struct dw_hdmi_qp *hdmi = dev_id;
> >> +	struct dw_hdmi_qp_i2c *i2c = hdmi->i2c;
> >> +	u32 stat;
> >> +
> >> +	stat = dw_hdmi_qp_read(hdmi, MAINUNIT_1_INT_STATUS);
> >> +
> >> +	i2c->stat = stat & (I2CM_OP_DONE_IRQ | I2CM_READ_REQUEST_IRQ |
> >> +			    I2CM_NACK_RCVD_IRQ);
> >> +
> >> +	if (i2c->stat) {
> >> +		dw_hdmi_qp_write(hdmi, i2c->stat, MAINUNIT_1_INT_CLEAR);
> >> +		complete(&i2c->cmp);
> >> +	}
> >> +
> >> +	if (stat)
> >> +		return IRQ_HANDLED;
> >> +
> >> +	return IRQ_NONE;
> >> +}
> > 
> > If the scrambler is enabled, you need to deal with hotplug. On hotplug,
> > the monitor will drop its TMDS ratio and scrambling status, but the
> > driver will keep assuming it's been programmed.
> > 
> > If you don't have a way to deal with hotplug yet, then I'd suggest to
> > just drop the scrambler setup for now.
> 
> Thanks for the heads up!
> 
> HPD is partially handled by the RK platform driver, which makes use of
> drm_helper_hpd_irq_event(). Since the bridge sets DRM_BRIDGE_OP_DETECT
> flag, the dw_hdmi_qp_bridge_detect() callback gets executed, which in turn
> verifies the PHY status via ->read_hpd() implemented as
> dw_hdmi_qp_rk3588_read_hpd() in the platform driver.

It's not only about hotplug detection, it's also about what happens
after you've detected a disconnection / reconnection.

The framework expects to keep the current mode as is, despite the
monitor not being setup to use the scrambler anymore, and the display
remains black.

> During my testing so far it worked reliably when switching displays with
> different capabilities.  I don't have a 4K@60Hz display at the moment, but
> used the HDMI RX port on the Rock 5B board in a loopback connection to
> verify this mode, which triggered the high TMDS clock ratio and scrambling
> setup as well.

How did you test exactly?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/bridge: synopsys: Add DW HDMI QP TX Controller support library

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

On Tue, Sep 03, 2024 at 12:12:02AM GMT, Cristian Ciocaltea wrote:
> On 9/2/24 10:36 AM, Maxime Ripard wrote:
> > On Sat, Aug 31, 2024 at 01:21:48AM GMT, Cristian Ciocaltea wrote:
> >> On 8/27/24 11:58 AM, Maxime Ripard wrote:
> >>> On Mon, Aug 19, 2024 at 01:29:29AM GMT, Cristian Ciocaltea wrote:
> >>>> +static irqreturn_t dw_hdmi_qp_main_hardirq(int irq, void *dev_id)
> >>>> +{
> >>>> +	struct dw_hdmi_qp *hdmi = dev_id;
> >>>> +	struct dw_hdmi_qp_i2c *i2c = hdmi->i2c;
> >>>> +	u32 stat;
> >>>> +
> >>>> +	stat = dw_hdmi_qp_read(hdmi, MAINUNIT_1_INT_STATUS);
> >>>> +
> >>>> +	i2c->stat = stat & (I2CM_OP_DONE_IRQ | I2CM_READ_REQUEST_IRQ |
> >>>> +			    I2CM_NACK_RCVD_IRQ);
> >>>> +
> >>>> +	if (i2c->stat) {
> >>>> +		dw_hdmi_qp_write(hdmi, i2c->stat, MAINUNIT_1_INT_CLEAR);
> >>>> +		complete(&i2c->cmp);
> >>>> +	}
> >>>> +
> >>>> +	if (stat)
> >>>> +		return IRQ_HANDLED;
> >>>> +
> >>>> +	return IRQ_NONE;
> >>>> +}
> >>>
> >>> If the scrambler is enabled, you need to deal with hotplug. On hotplug,
> >>> the monitor will drop its TMDS ratio and scrambling status, but the
> >>> driver will keep assuming it's been programmed.
> >>>
> >>> If you don't have a way to deal with hotplug yet, then I'd suggest to
> >>> just drop the scrambler setup for now.
> >>
> >> Thanks for the heads up!
> >>
> >> HPD is partially handled by the RK platform driver, which makes use of
> >> drm_helper_hpd_irq_event(). Since the bridge sets DRM_BRIDGE_OP_DETECT
> >> flag, the dw_hdmi_qp_bridge_detect() callback gets executed, which in turn
> >> verifies the PHY status via ->read_hpd() implemented as
> >> dw_hdmi_qp_rk3588_read_hpd() in the platform driver.
> > 
> > It's not only about hotplug detection, it's also about what happens
> > after you've detected a disconnection / reconnection.
> > 
> > The framework expects to keep the current mode as is, despite the
> > monitor not being setup to use the scrambler anymore, and the display
> > remains black.
> 
> AFAICS, the ->atomic_enable() callback is always invoked upon
> reconnection, hence the scrambler gets properly (re)enabled via
> dw_hdmi_qp_setup().

No, it's not.

> >> During my testing so far it worked reliably when switching displays with
> >> different capabilities.  I don't have a 4K@60Hz display at the moment, but
> >> used the HDMI RX port on the Rock 5B board in a loopback connection to
> >> verify this mode, which triggered the high TMDS clock ratio and scrambling
> >> setup as well.
> > 
> > How did you test exactly?
> 
> I initially tested with Sway/wlroots having an app running
> (eglgears_wayland) while unplugging/replugging the HDMI connectors in
> every possible sequence I could think of (e.g. several times per
> display, switching to a different one, repeating, switching again, etc).
> 
> I've just retested the whole stuff with Weston and confirm it works as
> expected, i.e. no black screen (or bad capture stream for the 4K@60Hz
> case) after any of the reconnections.

Then I guess both sway and weston handle uevent and will change the
connector mode on reconnection.

It's not mandatory, and others will just not bother and still expect the
output to work.

I guess the easier you can test this with is modetest.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/bridge: synopsys: Add DW HDMI QP TX Controller support library

[Cristian Ciocaltea]

On 9/3/24 11:09 AM, Maxime Ripard wrote:
> On Tue, Sep 03, 2024 at 12:12:02AM GMT, Cristian Ciocaltea wrote:
>> On 9/2/24 10:36 AM, Maxime Ripard wrote:
>>> On Sat, Aug 31, 2024 at 01:21:48AM GMT, Cristian Ciocaltea wrote:
>>>> On 8/27/24 11:58 AM, Maxime Ripard wrote:
>>>>> On Mon, Aug 19, 2024 at 01:29:29AM GMT, Cristian Ciocaltea wrote:
>>>>>> +static irqreturn_t dw_hdmi_qp_main_hardirq(int irq, void *dev_id)
>>>>>> +{
>>>>>> +	struct dw_hdmi_qp *hdmi = dev_id;
>>>>>> +	struct dw_hdmi_qp_i2c *i2c = hdmi->i2c;
>>>>>> +	u32 stat;
>>>>>> +
>>>>>> +	stat = dw_hdmi_qp_read(hdmi, MAINUNIT_1_INT_STATUS);
>>>>>> +
>>>>>> +	i2c->stat = stat & (I2CM_OP_DONE_IRQ | I2CM_READ_REQUEST_IRQ |
>>>>>> +			    I2CM_NACK_RCVD_IRQ);
>>>>>> +
>>>>>> +	if (i2c->stat) {
>>>>>> +		dw_hdmi_qp_write(hdmi, i2c->stat, MAINUNIT_1_INT_CLEAR);
>>>>>> +		complete(&i2c->cmp);
>>>>>> +	}
>>>>>> +
>>>>>> +	if (stat)
>>>>>> +		return IRQ_HANDLED;
>>>>>> +
>>>>>> +	return IRQ_NONE;
>>>>>> +}
>>>>>
>>>>> If the scrambler is enabled, you need to deal with hotplug. On hotplug,
>>>>> the monitor will drop its TMDS ratio and scrambling status, but the
>>>>> driver will keep assuming it's been programmed.
>>>>>
>>>>> If you don't have a way to deal with hotplug yet, then I'd suggest to
>>>>> just drop the scrambler setup for now.
>>>>
>>>> Thanks for the heads up!
>>>>
>>>> HPD is partially handled by the RK platform driver, which makes use of
>>>> drm_helper_hpd_irq_event(). Since the bridge sets DRM_BRIDGE_OP_DETECT
>>>> flag, the dw_hdmi_qp_bridge_detect() callback gets executed, which in turn
>>>> verifies the PHY status via ->read_hpd() implemented as
>>>> dw_hdmi_qp_rk3588_read_hpd() in the platform driver.
>>>
>>> It's not only about hotplug detection, it's also about what happens
>>> after you've detected a disconnection / reconnection.
>>>
>>> The framework expects to keep the current mode as is, despite the
>>> monitor not being setup to use the scrambler anymore, and the display
>>> remains black.
>>
>> AFAICS, the ->atomic_enable() callback is always invoked upon
>> reconnection, hence the scrambler gets properly (re)enabled via
>> dw_hdmi_qp_setup().
> 
> No, it's not.
> 
>>>> During my testing so far it worked reliably when switching displays with
>>>> different capabilities.  I don't have a 4K@60Hz display at the moment, but
>>>> used the HDMI RX port on the Rock 5B board in a loopback connection to
>>>> verify this mode, which triggered the high TMDS clock ratio and scrambling
>>>> setup as well.
>>>
>>> How did you test exactly?
>>
>> I initially tested with Sway/wlroots having an app running
>> (eglgears_wayland) while unplugging/replugging the HDMI connectors in
>> every possible sequence I could think of (e.g. several times per
>> display, switching to a different one, repeating, switching again, etc).
>>
>> I've just retested the whole stuff with Weston and confirm it works as
>> expected, i.e. no black screen (or bad capture stream for the 4K@60Hz
>> case) after any of the reconnections.
> 
> Then I guess both sway and weston handle uevent and will change the
> connector mode on reconnection.
> 
> It's not mandatory, and others will just not bother and still expect the
> output to work.
> 
> I guess the easier you can test this with is modetest.

Indeed, modetest doesn't trigger a mode change on reconnection.
This is handled now in v6:

https://lore.kernel.org/all/20240906-b4-rk3588-bridge-upstream-v6-0-a3128fb103eb@collabora.com/

Thanks,
Cristian