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

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

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

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