Re: [PATCH v1 1/4] dt-bindings: clock: qcom: add bindings for dispcc on SM8450

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 23 Jun 06:47 CDT 2022, Dmitry Baryshkov wrote:

> Add device tree bindings for the display clock controller on Qualcomm
> SM8450 platform.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../bindings/clock/qcom,dispcc-sm8450.yaml    | 132 ++++++++++++++++++
>  1 file changed, 132 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,dispcc-sm8450.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8450.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8450.yaml
> new file mode 100644
> index 000000000000..953d20a25cfb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8450.yaml
> @@ -0,0 +1,132 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/qcom,dispcc-sm8450.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Display Clock & Reset Controller Binding for SM8450
> +
> +maintainers:
> +  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> +
> +description: |
> +  Qualcomm display clock control module which supports the clocks, resets and
> +  power domains on SM8450.
> +
> +  See also:
> +    dt-bindings/clock/qcom,dispcc-sm8450.h

Please prefix this with include/

> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sm8450-dispcc
> +
> +  clocks:
> +    items:

I really think we should include a reference to GCC_DISP_AHB_CLK here.

There are two cases here, either the implementation does what we do in
Linux and just always-on the clock from gcc, in which case there's
nothing in here to ensure probe order and that the clock is actually on
before dispcc probes.

The other case would be that the implementation doesn't always-on the
gcc clock, in which case we need the reference.

> +      - description: Board XO source
> +      - description: Board Always On XO source
> +      - description: Byte clock from DSI PHY0
> +      - description: Pixel clock from DSI PHY0
> +      - description: Byte clock from DSI PHY1
> +      - description: Pixel clock from DSI PHY1
> +      - description: Link clock from DP PHY0
> +      - description: VCO DIV clock from DP PHY0
> +      - description: Link clock from DP PHY1
> +      - description: VCO DIV clock from DP PHY1
> +      - description: Link clock from DP PHY2
> +      - description: VCO DIV clock from DP PHY2
> +      - description: Link clock from DP PHY3
> +      - description: VCO DIV clock from DP PHY3
> +      - description: sleep clock
> +
> +  clock-names:

Please switch the implementation to index-based lookup and drop the
clock-names.

> +    items:
> +      - const: bi_tcxo
> +      - const: bi_tcxo_ao
> +      - const: dsi0_phy_pll_out_byteclk
> +      - const: dsi0_phy_pll_out_dsiclk
> +      - const: dsi1_phy_pll_out_byteclk
> +      - const: dsi1_phy_pll_out_dsiclk
> +      - const: dp0_phy_pll_link_clk
> +      - const: dp0_phy_pll_vco_div_clk
> +      - const: dp1_phy_pll_link_clk
> +      - const: dp1_phy_pll_vco_div_clk
> +      - const: dp2_phy_pll_link_clk
> +      - const: dp2_phy_pll_vco_div_clk
> +      - const: dp3_phy_pll_link_clk
> +      - const: dp3_phy_pll_vco_div_clk
> +      - const: sleep_clk
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  '#reset-cells':
> +    const: 1
> +
> +  '#power-domain-cells':
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  power-domains:
> +    description:
> +      A phandle and PM domain specifier for the MMCX power domain.
> +    maxItems: 1
> +
> +  required-opps:
> +    description:
> +      A phandle to an OPP node describing required MMCX performance point.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +  - '#reset-cells'
> +  - '#power-domain-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/power/qcom-rpmpd.h>
> +    clock-controller@af00000 {
> +      compatible = "qcom,sm8450-dispcc";
> +      reg = <0x0af00000 0x10000>;
> +      clocks = <&rpmhcc RPMH_CXO_CLK>,
> +               <&rpmhcc RPMH_CXO_CLK_A>,
> +               <&dsi0_phy 0>,
> +               <&dsi0_phy 1>,
> +               <&dsi1_phy 0>,
> +               <&dsi1_phy 1>,
> +               <0>, <0>,
> +               <0>, <0>,
> +               <0>, <0>,
> +               <0>, <0>,

One per line please.

Thanks,
Bjorn

> +               <&sleep_clk>;
> +      clock-names = "bi_tcxo",
> +                    "bi_tcxo_ao",
> +                    "dsi0_phy_pll_out_byteclk",
> +                    "dsi0_phy_pll_out_dsiclk",
> +                    "dsi1_phy_pll_out_byteclk",
> +                    "dsi1_phy_pll_out_dsiclk",
> +                    "dp0_phy_pll_link_clk",
> +                    "dp0_phy_pll_vco_div_clk",
> +                    "dp1_phy_pll_link_clk",
> +                    "dp1_phy_pll_vco_div_clk",
> +                    "dp2_phy_pll_link_clk",
> +                    "dp2_phy_pll_vco_div_clk",
> +                    "dp3_phy_pll_link_clk",
> +                    "dp3_phy_pll_vco_div_clk",
> +                    "sleep_clk";
> +      #clock-cells = <1>;
> +      #reset-cells = <1>;
> +      #power-domain-cells = <1>;
> +      power-domains = <&rpmhpd SM8450_MMCX>;
> +      required-opps = <&rpmhpd_opp_low_svs>;
> +    };
> +...
> -- 
> 2.35.1
>