| Message ID | 20240819-b4-rk3588-bridge-upstream-v4-3-6417c72a2749@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add initial support for the Rockchip RK3588 HDMI TX Controller | expand |
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 >
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? >> @@ -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. >> + # 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: 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
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
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 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. I'll leave it as is, if you don't mind. >>>> + # 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. 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. >> 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
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>; > >> };
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. [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>; >>>> }; >
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>; > >>>> }; > >
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>; >> >>>> }; >> >
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? 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. Thanks, Cristian > 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>; >>>>>>> }; >>>> >
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>; > >>>>>>> }; > >>>> > > >
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.
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.
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
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
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 > >
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
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 @@ -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: + - {} + - {} + - {} + - {} + # The next clocks are optional, but shall be specified in this + # order when present. + - description: TMDS/FRL link clock + - description: Video datapath clock + + 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. + +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>; + }; + }; + }; + }; + };
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(+)