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@xxxxxxxxxxxxx> > >> --- > >> .../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@xxxxxxxxxxxxx> > >> + > >> +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:
signature.asc
Description: PGP signature
