Re: [PATCH v4 4/5] dt-bindings: clock: Add SM8350 GCC clock bindings

From: Bjorn Andersson
Date: Tue Jan 26 2021 - 09:19:33 EST


On Tue 26 Jan 02:00 CST 2021, Vinod Koul wrote:

> On 25-01-21, 11:25, Bjorn Andersson wrote:
> > On Sun 17 Jan 22:43 CST 2021, Vinod Koul wrote:
> >
> > > Add device tree bindings for global clock controller on SM8350 SoCs.
> > >
> > > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx>
> > > ---
> > > .../bindings/clock/qcom,gcc-sm8350.yaml | 96 +++++++
> > > include/dt-bindings/clock/qcom,gcc-sm8350.h | 261 ++++++++++++++++++
> > > 2 files changed, 357 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc-sm8350.yaml
> > > create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8350.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc-sm8350.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc-sm8350.yaml
> > > new file mode 100644
> > > index 000000000000..78f35832aa41
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/qcom,gcc-sm8350.yaml
> > > @@ -0,0 +1,96 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/clock/qcom,gcc-sm8350.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Qualcomm Global Clock & Reset Controller Binding for SM8350
> > > +
> > > +maintainers:
> > > + - Vinod Koul <vkoul@xxxxxxxxxx>
> > > +
> > > +description: |
> > > + Qualcomm global clock control module which supports the clocks, resets and
> > > + power domains on SM8350.
> > > +
> > > + See also:
> > > + - dt-bindings/clock/qcom,gcc-sm8350.h
> > > +
> > > +properties:
> > > + compatible:
> > > + const: qcom,gcc-sm8350
> > > +
> > > + clocks:
> > > + items:
> > > + - description: Board XO source
> > > + - description: Sleep clock source
> > > + - description: PLL test clock source (Optional clock)
> > > + - description: PCIE 0 Pipe clock source (Optional clock)
> > > + - description: PCIE 1 Pipe clock source (Optional clock)
> > > + - description: UFS card Rx symbol 0 clock source (Optional clock)
> > > + - description: UFS card Rx symbol 1 clock source (Optional clock)
> > > + - description: UFS card Tx symbol 0 clock source (Optional clock)
> > > + - description: UFS phy Rx symbol 0 clock source (Optional clock)
> > > + - description: UFS phy Rx symbol 1 clock source (Optional clock)
> > > + - description: UFS phy Tx symbol 0 clock source (Optional clock)
> > > + - description: USB3 phy wrapper pipe clock source (Optional clock)
> > > + - description: USB3 phy sec pipe clock source (Optional clock)
> > > + minItems: 2
> > > + maxItems: 13
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: bi_tcxo
> > > + - const: sleep_clk
> > > + - const: core_bi_pll_test_se # Optional clock
> > > + - const: pcie_0_pipe_clk # Optional clock
> > > + - const: pcie_1_pipe_clk # Optional clock
> > > + - const: ufs_card_rx_symbol_0_clk # Optional clock
> > > + - const: ufs_card_rx_symbol_1_clk # Optional clock
> > > + - const: ufs_card_tx_symbol_0_clk # Optional clock
> > > + - const: ufs_phy_rx_symbol_0_clk # Optional clock
> > > + - const: ufs_phy_rx_symbol_1_clk # Optional clock
> > > + - const: ufs_phy_tx_symbol_0_clk # Optional clock
> > > + - const: usb3_phy_wrapper_gcc_usb30_pipe_clk # Optional clock
> > > + - const: usb3_uni_phy_sec_gcc_usb30_pipe_clk # Optional clock
> > > + minItems: 2
> > > + maxItems: 13
> > > +
> > > + '#clock-cells':
> > > + const: 1
> > > +
> > > + '#reset-cells':
> > > + const: 1
> > > +
> > > + '#power-domain-cells':
> > > + const: 1
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > +required:
> > > + - compatible
> > > + - clocks
> > > + - clock-names
> > > + - reg
> > > + - '#clock-cells'
> > > + - '#reset-cells'
> > > + - '#power-domain-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/clock/qcom,rpmh.h>
> > > + clock-controller@100000 {
> > > + compatible = "qcom,gcc-sm8350";
> > > + reg = <0x00100000 0x1f0000>;
> > > + clocks = <&rpmhcc RPMH_CXO_CLK>,
> > > + <&sleep_clk>;
> > > + clock-names = "bi_tcxo", "sleep_clk";
> > > + #clock-cells = <1>;
> > > + #reset-cells = <1>;
> > > + #power-domain-cells = <1>;
> > > + };
> > > +
> > > +...
> > > diff --git a/include/dt-bindings/clock/qcom,gcc-sm8350.h b/include/dt-bindings/clock/qcom,gcc-sm8350.h
> > > new file mode 100644
> > > index 000000000000..2b289c5c109f
> > > --- /dev/null
> > > +++ b/include/dt-bindings/clock/qcom,gcc-sm8350.h
> > > @@ -0,0 +1,261 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > +/*
> > > + * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved.
> > > + * Copyright (c) 2020-2021, Linaro Limited
> > > + */
> > > +
> > > +#ifndef _DT_BINDINGS_CLK_QCOM_GCC_SM8350_H
> > > +#define _DT_BINDINGS_CLK_QCOM_GCC_SM8350_H
> > > +
> > > +/* GCC HW clocks */
> > > +#define CORE_BI_PLL_TEST_SE 0
> > > +#define PCIE_0_PIPE_CLK 1
> > > +#define PCIE_1_PIPE_CLK 2
> > > +#define UFS_CARD_RX_SYMBOL_0_CLK 3
> > > +#define UFS_CARD_RX_SYMBOL_1_CLK 4
> > > +#define UFS_CARD_TX_SYMBOL_0_CLK 5
> > > +#define UFS_PHY_RX_SYMBOL_0_CLK 6
> > > +#define UFS_PHY_RX_SYMBOL_1_CLK 7
> > > +#define UFS_PHY_TX_SYMBOL_0_CLK 8
> > > +#define USB3_PHY_WRAPPER_GCC_USB30_PIPE_CLK 9
> > > +#define USB3_UNI_PHY_SEC_GCC_USB30_PIPE_CLK 10
> > > +
> > > +/* GCC clocks */
> > > +#define GCC_AGGRE_NOC_PCIE_0_AXI_CLK 11
> > > +#define GCC_AGGRE_NOC_PCIE_1_AXI_CLK 12
> > > +#define GCC_AGGRE_NOC_PCIE_TBU_CLK 13
> > > +#define GCC_AGGRE_UFS_CARD_AXI_CLK 14
> > > +#define GCC_AGGRE_UFS_CARD_AXI_HW_CTL_CLK 15
> > > +#define GCC_AGGRE_UFS_PHY_AXI_CLK 16
> > > +#define GCC_AGGRE_UFS_PHY_AXI_HW_CTL_CLK 17
> > > +#define GCC_AGGRE_USB3_PRIM_AXI_CLK 18
> > > +#define GCC_AGGRE_USB3_SEC_AXI_CLK 19
> > > +#define GCC_BOOT_ROM_AHB_CLK 20
> > > +#define GCC_CAMERA_AHB_CLK 21
> >
> > You removed these from the driver, so no need to expose them in the
> > dt-binding either.
>
> I did think about that and thought maybe it is better to leave the
> defines. We can always update the driver to use if we ever felt the
> need.
>
> But then I dont think we will ever do that so makes sense, will update
> this and send with acks collected
>

Given that the actual value isn't significant (just need to be stable),
we can easily add those as new defines at the end of the list when that
day comes.

Regards,
Bjorn