| Message ID | 20240831-b4-rk3588-bridge-upstream-v5-3-9503bece0136@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add initial support for the Rockchip RK3588 HDMI TX Controller | expand |
On Sat, Aug 31, 2024 at 12:55:31AM +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> > --- > .../rockchip/rockchip,rk3588-dw-hdmi-qp.yaml | 166 +++++++++++++++++++++ > 1 file changed, 166 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml > new file mode 100644 > index 000000000000..d2919ff6aa23 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml > @@ -0,0 +1,166 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/rockchip/rockchip,rk3588-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: > + items: > + - {} > + - {} > + - {} > + - {} > + - description: TMDS/FRL link clock > + - description: Video datapath clock Please define all clocks. > + > + clock-names: > + 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 Drop phy-names, not really useful if it copies the name of the block. > + > + 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: > + Some HDMI QP related data is accessed through SYS GRF regs. > + > + rockchip,vo-grf: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Additional HDMI QP related data is accessed through VO GRF regs. > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - interrupt-names > + - phys > + - phy-names > + - ports > + - resets > + - reset-names > + - rockchip,grf > + - rockchip,vo-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,vo-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>; Messed indentation. Best regards, Krzysztof
On 8/31/24 9:13 AM, Krzysztof Kozlowski wrote: > On Sat, Aug 31, 2024 at 12:55:31AM +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> >> --- >> .../rockchip/rockchip,rk3588-dw-hdmi-qp.yaml | 166 +++++++++++++++++++++ >> 1 file changed, 166 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml >> new file mode 100644 >> index 000000000000..d2919ff6aa23 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml >> @@ -0,0 +1,166 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/display/rockchip/rockchip,rk3588-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: >> + items: >> + - {} >> + - {} >> + - {} >> + - {} >> + - description: TMDS/FRL link clock >> + - description: Video datapath clock > > Please define all clocks. The other clocks are defined in the common binding, should we reiterate them? >> + >> + clock-names: >> + 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 > > Drop phy-names, not really useful if it copies the name of the block. Sure, will drop it. >> + >> + 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: >> + Some HDMI QP related data is accessed through SYS GRF regs. >> + >> + rockchip,vo-grf: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Additional HDMI QP related data is accessed through VO GRF regs. >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + - interrupts >> + - interrupt-names >> + - phys >> + - phy-names >> + - ports >> + - resets >> + - reset-names >> + - rockchip,grf >> + - rockchip,vo-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,vo-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>; > > Messed indentation. Ups, somehow I missed this.. Thanks for reviewing, Cristian
Cristian Ciocaltea wrote: > On 8/31/24 9:13 AM, Krzysztof Kozlowski wrote: > > > > Please define all clocks. > > The other clocks are defined in the common binding, should we reiterate > them? I would suggest yes, they should be reduplicated, if only to maintain consistency with all the other docs. A grep through the bridge docs shows that there are virtually none which use a "{}" placeholder like this. While it seems kind of like one might worry about "don't repeat yourself" syndrome, keep in mind this is not code, but human- used documentation. Having all the information available at a glance would seem to be the most convenient to the end (developer) user, so they aren't having to toggle between two separate files. Of course there may be some questions regarding docs becoming out of sync, but *ideally* we don't want to break backward compatibility with device trees (esp. given how I am imagining firmware integration to work on these platforms, as the RK3588 is at at least low-end desktop-grade performance and UEFI packages have already been built for it), though of course that doesn't mean adding new options is off the table. (FWIW, this is what I did in my now-withdrawn-at-your-request re-submission; I reduplicated the bindings as it seemed that's what others here were pushing for and thus that felt like the quickest way to get this important driver approved.) - Shimrra Shai
On 9/2/24 4:09 AM, Shimrra Shai wrote: > Cristian Ciocaltea wrote: >> On 8/31/24 9:13 AM, Krzysztof Kozlowski wrote: >>> >>> Please define all clocks. >> >> The other clocks are defined in the common binding, should we reiterate >> them? > > I would suggest yes, they should be reduplicated, if only to maintain > consistency with all the other docs. A grep through the bridge docs > shows that there are virtually none which use a "{}" placeholder like > this. Are you sure about that? This is precisely the approach followed by the upstream DW HDMI TX controller binding [1]. Moreover, I've already pointed this out in [2]. > While it seems kind of like one might worry about "don't > repeat yourself" syndrome, keep in mind this is not code, but human- > used documentation. Having all the information available at a glance > would seem to be the most convenient to the end (developer) user, so > they aren't having to toggle between two separate files. I think that's pretty subjective to be stated as a general rule, e.g. I don't have any problem toggling between multiple files as I regularly keep over 50 files opened in my IDE. Personally, I'd always go for the slightly less readable approach if I can avoid duplicating content. I'd suggest to follow the whole thread [2], as this topic has been already discussed. > Of course > there may be some questions regarding docs becoming out of sync, but > *ideally* we don't want to break backward compatibility with device > trees (esp. given how I am imagining firmware integration to work on > these platforms, as the RK3588 is at at least low-end desktop-grade > performance and UEFI packages have already been built for it), though > of course that doesn't mean adding new options is off the table. > > (FWIW, this is what I did in my now-withdrawn-at-your-request > re-submission; I reduplicated the bindings as it seemed that's what > others here were pushing for and thus that felt like the quickest way > to get this important driver approved.) This is not really a blocker for the series. Please remember to be patient while involved (one way or another) in the upstreaming process, as maintainers need time to review *all* the patches. This might be very important for you, but there are, usually, tons of other way more important things the maintainers need to handle in parallel. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml#n46 [2] https://lore.kernel.org/lkml/ec84bc0b-c4c2-4735-9f34-52bc3a852aaf@collabora.com/
diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml new file mode 100644 index 000000000000..d2919ff6aa23 --- /dev/null +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml @@ -0,0 +1,166 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/rockchip/rockchip,rk3588-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: + items: + - {} + - {} + - {} + - {} + - description: TMDS/FRL link clock + - description: Video datapath clock + + clock-names: + 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: + Some HDMI QP related data is accessed through SYS GRF regs. + + rockchip,vo-grf: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Additional HDMI QP related data is accessed through VO GRF regs. + +required: + - compatible + - reg + - clocks + - clock-names + - interrupts + - interrupt-names + - phys + - phy-names + - ports + - resets + - reset-names + - rockchip,grf + - rockchip,vo-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,vo-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>; + }; + }; + }; + }; + };
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> --- .../rockchip/rockchip,rk3588-dw-hdmi-qp.yaml | 166 +++++++++++++++++++++ 1 file changed, 166 insertions(+)