Re: [PATCH v8 01/10] dt-bindings: display: rockchip: analogix-dp: Fix hclk as third clock for RK3588

[Conor Dooley <conor@kernel.org>]

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

On Tue, May 26, 2026 at 07:27:55PM +0800, Damon Ding wrote:
> Hi Conor,
> 
> On 5/26/2026 8:54 AM, Damon Ding wrote:
> > Hi Conor,
> > 
> > On 5/26/2026 12:54 AM, Conor Dooley wrote:
> > > On Mon, May 25, 2026 at 08:53:22PM +0800, Damon Ding wrote:
> > > > RK3588 eDP controller requires HCLK_VO1 to access the VO1 GRF
> > > > registers and enable the video datapath.
> > > > 
> > > > Previously, the clock was enabled implicitly via the 'rockchip,vo-grf'
> > > > phandle reference, which allowed the eDP to work without explicitly
> > > > managing the hclk_vo1 clock. However, this is not safe or explicit.
> > > > 
> > > > To make the clock dependency explicit, enforce per-SoC clock-names
> > > > requirements:
> > > >   - RK3288: 2 clocks (dp, pclk)
> > > >   - RK3399: 3 clocks (dp, pclk, grf)
> > > >   - RK3588: 3 clocks (dp, pclk, hclk)
> > > > 
> > > > Do not reuse the 'grf' clock name for RK3588 because it represents
> > > > a different clock with distinct control logic:
> > > > - The 'grf' clock is only for GRF register access and is toggled
> > > >    dynamically during register access.
> > > > - The 'hclk' clock controls both GRF access and video datapath
> > > >    gating, and must remain enabled during probe.
> > > > 
> > > > Fixes: f855146263b1 ("dt-bindings: display: rockchip:
> > > > analogix-dp: Add support for RK3588")
> > > > Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> > > > 
> > > > ---
> > > > 
> > > > Changes in v4:
> > > > - Modify the commit msg.
> > > > 
> > > > Changes in v5:
> > > > - Enforce the correct third clock name on a per-compatible basis.
> > > > - Modify the commit msg simultaneously.
> > > > 
> > > > Changes in v6:
> > > > - Expand more detail commit msg about using hclk instead of grf clock.
> > > > 
> > > > Changes in v7:
> > > > - List all valid clock names at the top level, and constrain the clock
> > > >    count for each platform with minItems/maxItems in allOf.
> > > > 
> > > > Changes in v8:
> > > > - Fix indentation to 10 for enum in clock-names property.
> > > > ---
> > > >   .../rockchip/rockchip,analogix-dp.yaml        | 34 ++++++++++++++++++-
> > > >   1 file changed, 33 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/display/rockchip/
> > > > rockchip,analogix-dp.yaml b/Documentation/devicetree/bindings/
> > > > display/rockchip/rockchip,analogix-dp.yaml
> > > > index d99b23b88cc5..a1ab7a77bdd3 100644
> > > > --- a/Documentation/devicetree/bindings/display/rockchip/
> > > > rockchip,analogix-dp.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/rockchip/
> > > > rockchip,analogix-dp.yaml
> > > > @@ -26,7 +26,9 @@ properties:
> > > >       items:
> > > >         - const: dp
> > > >         - const: pclk
> > > > -      - const: grf
> > > > +      - enum:
> > > > +          - grf
> > > > +          - hclk
> > > >     power-domains:
> > > >       maxItems: 1
> > > > @@ -60,6 +62,32 @@ required:
> > > >   allOf:
> > > >     - $ref: /schemas/display/bridge/analogix,dp.yaml#
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            enum:
> > > > +              - rockchip,rk3288-dp
> > > > +    then:
> > > > +      properties:
> > > > +        clocks:
> > > > +          maxItems: 2
> > > > +        clock-names:
> > > > +          maxItems: 2
> > > > +
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            enum:
> > > > +              - rockchip,rk3399-edp
> > > > +    then:
> > > > +      properties:
> > > > +        clocks:
> > > > +          minItems: 3
> > > > +        clock-names:
> > > > +          minItems: 3
> > > 
> > > If you go back to v6, you'll see that I never asked you to remove the
> > > explict clock-names from here or below. Only the one from the 3288
> > > section. The minItems was an addition, not a replacement.
> > > 
> > > pw-bot: changes-requested
> > > 
> > 
> > Sorry for the misunderstanding. I will restore the explicit clock-names
> > definitions and fix this in next version.
> > 
> 
> Sorry to bother you. I attempted to place the explicit clock-names under
> minItems:
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml
> index a1ab7a77bdd3..ef03edf52de8 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml
> @@ -87,6 +87,10 @@ allOf:
>            minItems: 3
>          clock-names:
>            minItems: 3
> +          items:
> +            - const: dp
> +            - const: pclk
> +            - const: grf
> 
>    - if:
>        properties:
> @@ -100,6 +104,10 @@ allOf:
>            minItems: 3
>          clock-names:
>            minItems: 3
> +          items:
> +            - const: dp
> +            - const: pclk
> +            - const: hclk
>          resets:
>            minItems: 2
>          reset-names:
> 
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml
> 
> Then the following errors occurred:
> 
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   CHKDT   ./Documentation/devicetree/bindings
> /home/ding/drm-misc/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml:
> allOf:2:then:properties:clock-names: 'oneOf' conditional failed, one must be
> fixed:
>         False schema does not allow 3
>         [{'const': 'dp'}, {'const': 'pclk'}, {'const': 'grf'}] is too long
>         [{'const': 'dp'}, {'const': 'pclk'}, {'const': 'grf'}] is too short
>         1 was expected
>         3 is greater than the maximum of 2
>         hint: "minItems" is only needed if less than the "items" list length
>         from schema $id: http://devicetree.org/meta-schemas/items.yaml
> /home/ding/drm-misc/Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.yaml:
> allOf:3:then:properties:clock-names: 'oneOf' conditional failed, one must be
> fixed:
>         False schema does not allow 3
>         [{'const': 'dp'}, {'const': 'pclk'}, {'const': 'hclk'}] is too long
>         [{'const': 'dp'}, {'const': 'pclk'}, {'const': 'hclk'}] is too short
>         1 was expected
>         3 is greater than the maximum of 2
>         hint: "minItems" is only needed if less than the "items" list length
>         from schema $id: http://devicetree.org/meta-schemas/items.yaml
>   LINT    ./Documentation/devicetree/bindings
>   DTEX Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.example.dts
>   DTC [C] Documentation/devicetree/bindings/display/rockchip/rockchip,analogix-dp.example.dtb
> 
> Neither keeping only minItems nor only the explicit clock-names causes any
> errors. Would it be a better idea to keep just the explicit clock-names
> here?

Sure.

This feels like a bug because I have no idea where the "maximum of 2" is
coming from. I'll ask Rob about it.

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