| Message ID | 1569959842-8399-1-git-send-email-jhugo@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MSM8998 Multimedia Clock Controller | expand |
Quoting Jeffrey Hugo (2019-10-01 12:57:22) > diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.txt b/Documentation/devicetree/bindings/clock/qcom,mmcc.txt > index 8b0f7841af8d..a92f3cbc9736 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.txt > +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.txt > @@ -10,11 +10,32 @@ Required properties : > "qcom,mmcc-msm8960" > "qcom,mmcc-msm8974" > "qcom,mmcc-msm8996" > + "qcom,mmcc-msm8998" Can you convert this binding to YAML? Makes it easier to validate it against the dts files. > > - reg : shall contain base register location and length > - #clock-cells : shall contain 1 > - #reset-cells : shall contain 1 > > +For MSM8998 only: > + - clocks: a list of phandles and clock-specifier pairs, > + one for each entry in clock-names. > + - clock-names: "xo" for the xo clock. > + "gpll0" for the global pll 0 clock. > + "dsi0dsi" for the dsi0 pll dsi clock (required if dsi is > + enabled, optional otherwise). > + "dsi0byte" for the dsi0 pll byte clock (required if dsi > + is enabled, optional otherwise). > + "dsi1dsi" for the dsi1 pll dsi clock (required if dsi is > + enabled, optional otherwise). > + "dsi1byte" for the dsi1 pll byte clock (required if dsi > + is enabled, optional otherwise). > + "hdmipll" for the hdmi pll clock (required if hdmi is > + enabled, optional otherwise). > + "dpvco" for the displayport pll vco clock (required if > + dp is enabled, optional otherwise). > + "dplink" for the displayport pll link clock (required if > + dp is enabled, optional otherwise). I'm not sure why it's optional. The hardware is "fixed" in the sense that the dp phy is always there and connected to this hardware block. From a driver perspective I agree it's optional to be used, but from a DT perspective it's always there so it should be required.
On 11/7/2019 2:55 PM, Stephen Boyd wrote: > Quoting Jeffrey Hugo (2019-10-01 12:57:22) >> diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.txt b/Documentation/devicetree/bindings/clock/qcom,mmcc.txt >> index 8b0f7841af8d..a92f3cbc9736 100644 >> --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.txt >> +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.txt >> @@ -10,11 +10,32 @@ Required properties : >> "qcom,mmcc-msm8960" >> "qcom,mmcc-msm8974" >> "qcom,mmcc-msm8996" >> + "qcom,mmcc-msm8998" > > Can you convert this binding to YAML? Makes it easier to validate it > against the dts files. I'll look at this since I'm already resending due to the merge conflict. > >> >> - reg : shall contain base register location and length >> - #clock-cells : shall contain 1 >> - #reset-cells : shall contain 1 >> >> +For MSM8998 only: >> + - clocks: a list of phandles and clock-specifier pairs, >> + one for each entry in clock-names. >> + - clock-names: "xo" for the xo clock. >> + "gpll0" for the global pll 0 clock. >> + "dsi0dsi" for the dsi0 pll dsi clock (required if dsi is >> + enabled, optional otherwise). >> + "dsi0byte" for the dsi0 pll byte clock (required if dsi >> + is enabled, optional otherwise). >> + "dsi1dsi" for the dsi1 pll dsi clock (required if dsi is >> + enabled, optional otherwise). >> + "dsi1byte" for the dsi1 pll byte clock (required if dsi >> + is enabled, optional otherwise). >> + "hdmipll" for the hdmi pll clock (required if hdmi is >> + enabled, optional otherwise). >> + "dpvco" for the displayport pll vco clock (required if >> + dp is enabled, optional otherwise). >> + "dplink" for the displayport pll link clock (required if >> + dp is enabled, optional otherwise). > > I'm not sure why it's optional. The hardware is "fixed" in the sense > that the dp phy is always there and connected to this hardware block. > From a driver perspective I agree it's optional to be used, but from a > DT perspective it's always there so it should be required. > Sure, the DP phy is technically always there, but does a particular platform have an actual DP output connected to the phy? If not, why bother describing something that isn't even used? From a more practical sense its undefined how to actually get the DP clocks - the DP binding is implicitly a clock provider since it has #clock-cells, but it doesn't specify how to actually get the clocks. The DSI binding tells you which index is the dsi clock, and which is the byte clock. The HDMI binding is not a clock provider at all. Needs to be revised, which didn't appear trivial when I took a quick look while working on mmcc. I want to do the right thing here by specifying all the external clocks up front, and not have to worry about backwards compatibility with pre-existing DTs later on, but I also would like to focus on one problem at a time, and not go dig into all the problems with DP/HDMI before landing this, particularly as those components also rely on the mmcc. Is that justification enough for you? If not, how would you like to proceed? Make them required in the binding, and just have an invalid (per the binding) DT until all the problems get sorted out?
Quoting Jeffrey Hugo (2019-11-07 14:35:21) > On 11/7/2019 2:55 PM, Stephen Boyd wrote: > > Quoting Jeffrey Hugo (2019-10-01 12:57:22) > >> diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.txt b/Documentation/devicetree/bindings/clock/qcom,mmcc.txt > >> index 8b0f7841af8d..a92f3cbc9736 100644 > >> --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.txt > >> +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.txt > > > >> > >> - reg : shall contain base register location and length > >> - #clock-cells : shall contain 1 > >> - #reset-cells : shall contain 1 > >> > >> +For MSM8998 only: > >> + - clocks: a list of phandles and clock-specifier pairs, > >> + one for each entry in clock-names. > >> + - clock-names: "xo" for the xo clock. > >> + "gpll0" for the global pll 0 clock. > >> + "dsi0dsi" for the dsi0 pll dsi clock (required if dsi is > >> + enabled, optional otherwise). > >> + "dsi0byte" for the dsi0 pll byte clock (required if dsi > >> + is enabled, optional otherwise). > >> + "dsi1dsi" for the dsi1 pll dsi clock (required if dsi is > >> + enabled, optional otherwise). > >> + "dsi1byte" for the dsi1 pll byte clock (required if dsi > >> + is enabled, optional otherwise). > >> + "hdmipll" for the hdmi pll clock (required if hdmi is > >> + enabled, optional otherwise). > >> + "dpvco" for the displayport pll vco clock (required if > >> + dp is enabled, optional otherwise). > >> + "dplink" for the displayport pll link clock (required if > >> + dp is enabled, optional otherwise). > > > > I'm not sure why it's optional. The hardware is "fixed" in the sense > > that the dp phy is always there and connected to this hardware block. > > From a driver perspective I agree it's optional to be used, but from a > > DT perspective it's always there so it should be required. > > > > Sure, the DP phy is technically always there, but does a particular > platform have an actual DP output connected to the phy? If not, why > bother describing something that isn't even used? If the DP phy isn't connected then having it be marked as status = "disabled" is the typical approach to the problem. I agree it may not be used on some particular board using the SoC, but this is an SoC that is made once so we should be able to describe it regardless of how it's used by some board. > > From a more practical sense its undefined how to actually get the DP > clocks - the DP binding is implicitly a clock provider since it has > #clock-cells, but it doesn't specify how to actually get the clocks. > The DSI binding tells you which index is the dsi clock, and which is the > byte clock. > > The HDMI binding is not a clock provider at all. Needs to be revised, > which didn't appear trivial when I took a quick look while working on mmcc. > > I want to do the right thing here by specifying all the external clocks > up front, and not have to worry about backwards compatibility with > pre-existing DTs later on, but I also would like to focus on one problem > at a time, and not go dig into all the problems with DP/HDMI before > landing this, particularly as those components also rely on the mmcc. > > Is that justification enough for you? If not, how would you like to > proceed? Make them required in the binding, and just have an invalid > (per the binding) DT until all the problems get sorted out? > If we can make the clks 'required' and not cause the DTS checker system to blow up then I'll be happy. I hope we can have a property like: clocks = <&gcc KNOWN_CLK>, <0>, <0>, <&dp_phy 1>; where <0> means "I don't know right now", and that will be enough to please the checker and can be filled in later on when we sort out the HDMI and DP bindings. If this doesn't work then I guess I'll just have to get over it and complain to Rob Herring.
diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.txt b/Documentation/devicetree/bindings/clock/qcom,mmcc.txt index 8b0f7841af8d..a92f3cbc9736 100644 --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.txt +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.txt @@ -10,11 +10,32 @@ Required properties : "qcom,mmcc-msm8960" "qcom,mmcc-msm8974" "qcom,mmcc-msm8996" + "qcom,mmcc-msm8998" - reg : shall contain base register location and length - #clock-cells : shall contain 1 - #reset-cells : shall contain 1 +For MSM8998 only: + - clocks: a list of phandles and clock-specifier pairs, + one for each entry in clock-names. + - clock-names: "xo" for the xo clock. + "gpll0" for the global pll 0 clock. + "dsi0dsi" for the dsi0 pll dsi clock (required if dsi is + enabled, optional otherwise). + "dsi0byte" for the dsi0 pll byte clock (required if dsi + is enabled, optional otherwise). + "dsi1dsi" for the dsi1 pll dsi clock (required if dsi is + enabled, optional otherwise). + "dsi1byte" for the dsi1 pll byte clock (required if dsi + is enabled, optional otherwise). + "hdmipll" for the hdmi pll clock (required if hdmi is + enabled, optional otherwise). + "dpvco" for the displayport pll vco clock (required if + dp is enabled, optional otherwise). + "dplink" for the displayport pll link clock (required if + dp is enabled, optional otherwise). + Optional properties : - #power-domain-cells : shall contain 1 diff --git a/include/dt-bindings/clock/qcom,mmcc-msm8998.h b/include/dt-bindings/clock/qcom,mmcc-msm8998.h new file mode 100644 index 000000000000..ecbafdb930aa --- /dev/null +++ b/include/dt-bindings/clock/qcom,mmcc-msm8998.h @@ -0,0 +1,210 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2019, The Linux Foundation. All rights reserved. + */ + +#ifndef _DT_BINDINGS_CLK_MSM_MMCC_8998_H +#define _DT_BINDINGS_CLK_MSM_MMCC_8998_H + +#define MMPLL0 0 +#define MMPLL0_OUT_EVEN 1 +#define MMPLL1 2 +#define MMPLL1_OUT_EVEN 3 +#define MMPLL3 4 +#define MMPLL3_OUT_EVEN 5 +#define MMPLL4 6 +#define MMPLL4_OUT_EVEN 7 +#define MMPLL5 8 +#define MMPLL5_OUT_EVEN 9 +#define MMPLL6 10 +#define MMPLL6_OUT_EVEN 11 +#define MMPLL7 12 +#define MMPLL7_OUT_EVEN 13 +#define MMPLL10 14 +#define MMPLL10_OUT_EVEN 15 +#define BYTE0_CLK_SRC 16 +#define BYTE1_CLK_SRC 17 +#define CCI_CLK_SRC 18 +#define CPP_CLK_SRC 19 +#define CSI0_CLK_SRC 20 +#define CSI1_CLK_SRC 21 +#define CSI2_CLK_SRC 22 +#define CSI3_CLK_SRC 23 +#define CSIPHY_CLK_SRC 24 +#define CSI0PHYTIMER_CLK_SRC 25 +#define CSI1PHYTIMER_CLK_SRC 26 +#define CSI2PHYTIMER_CLK_SRC 27 +#define DP_AUX_CLK_SRC 28 +#define DP_CRYPTO_CLK_SRC 29 +#define DP_LINK_CLK_SRC 30 +#define DP_PIXEL_CLK_SRC 31 +#define ESC0_CLK_SRC 32 +#define ESC1_CLK_SRC 33 +#define EXTPCLK_CLK_SRC 34 +#define FD_CORE_CLK_SRC 35 +#define HDMI_CLK_SRC 36 +#define JPEG0_CLK_SRC 37 +#define MAXI_CLK_SRC 38 +#define MCLK0_CLK_SRC 39 +#define MCLK1_CLK_SRC 40 +#define MCLK2_CLK_SRC 41 +#define MCLK3_CLK_SRC 42 +#define MDP_CLK_SRC 43 +#define VSYNC_CLK_SRC 44 +#define AHB_CLK_SRC 45 +#define AXI_CLK_SRC 46 +#define PCLK0_CLK_SRC 47 +#define PCLK1_CLK_SRC 48 +#define ROT_CLK_SRC 49 +#define VIDEO_CORE_CLK_SRC 50 +#define VIDEO_SUBCORE0_CLK_SRC 51 +#define VIDEO_SUBCORE1_CLK_SRC 52 +#define VFE0_CLK_SRC 53 +#define VFE1_CLK_SRC 54 +#define MISC_AHB_CLK 55 +#define VIDEO_CORE_CLK 56 +#define VIDEO_AHB_CLK 57 +#define VIDEO_AXI_CLK 58 +#define VIDEO_MAXI_CLK 59 +#define VIDEO_SUBCORE0_CLK 60 +#define VIDEO_SUBCORE1_CLK 61 +#define MDSS_AHB_CLK 62 +#define MDSS_HDMI_DP_AHB_CLK 63 +#define MDSS_AXI_CLK 64 +#define MDSS_PCLK0_CLK 65 +#define MDSS_PCLK1_CLK 66 +#define MDSS_MDP_CLK 67 +#define MDSS_MDP_LUT_CLK 68 +#define MDSS_EXTPCLK_CLK 69 +#define MDSS_VSYNC_CLK 70 +#define MDSS_HDMI_CLK 71 +#define MDSS_BYTE0_CLK 72 +#define MDSS_BYTE1_CLK 73 +#define MDSS_ESC0_CLK 74 +#define MDSS_ESC1_CLK 75 +#define MDSS_ROT_CLK 76 +#define MDSS_DP_LINK_CLK 77 +#define MDSS_DP_LINK_INTF_CLK 78 +#define MDSS_DP_CRYPTO_CLK 79 +#define MDSS_DP_PIXEL_CLK 80 +#define MDSS_DP_AUX_CLK 81 +#define MDSS_BYTE0_INTF_CLK 82 +#define MDSS_BYTE1_INTF_CLK 83 +#define CAMSS_CSI0PHYTIMER_CLK 84 +#define CAMSS_CSI1PHYTIMER_CLK 85 +#define CAMSS_CSI2PHYTIMER_CLK 86 +#define CAMSS_CSI0_CLK 87 +#define CAMSS_CSI0_AHB_CLK 88 +#define CAMSS_CSI0RDI_CLK 89 +#define CAMSS_CSI0PIX_CLK 90 +#define CAMSS_CSI1_CLK 91 +#define CAMSS_CSI1_AHB_CLK 92 +#define CAMSS_CSI1RDI_CLK 93 +#define CAMSS_CSI1PIX_CLK 94 +#define CAMSS_CSI2_CLK 95 +#define CAMSS_CSI2_AHB_CLK 96 +#define CAMSS_CSI2RDI_CLK 97 +#define CAMSS_CSI2PIX_CLK 98 +#define CAMSS_CSI3_CLK 99 +#define CAMSS_CSI3_AHB_CLK 100 +#define CAMSS_CSI3RDI_CLK 101 +#define CAMSS_CSI3PIX_CLK 102 +#define CAMSS_ISPIF_AHB_CLK 103 +#define CAMSS_CCI_CLK 104 +#define CAMSS_CCI_AHB_CLK 105 +#define CAMSS_MCLK0_CLK 106 +#define CAMSS_MCLK1_CLK 107 +#define CAMSS_MCLK2_CLK 108 +#define CAMSS_MCLK3_CLK 109 +#define CAMSS_TOP_AHB_CLK 110 +#define CAMSS_AHB_CLK 111 +#define CAMSS_MICRO_AHB_CLK 112 +#define CAMSS_JPEG0_CLK 113 +#define CAMSS_JPEG_AHB_CLK 114 +#define CAMSS_JPEG_AXI_CLK 115 +#define CAMSS_VFE0_AHB_CLK 116 +#define CAMSS_VFE1_AHB_CLK 117 +#define CAMSS_VFE0_CLK 118 +#define CAMSS_VFE1_CLK 119 +#define CAMSS_CPP_CLK 120 +#define CAMSS_CPP_AHB_CLK 121 +#define CAMSS_VFE_VBIF_AHB_CLK 122 +#define CAMSS_VFE_VBIF_AXI_CLK 123 +#define CAMSS_CPP_AXI_CLK 124 +#define CAMSS_CPP_VBIF_AHB_CLK 125 +#define CAMSS_CSI_VFE0_CLK 126 +#define CAMSS_CSI_VFE1_CLK 127 +#define CAMSS_VFE0_STREAM_CLK 128 +#define CAMSS_VFE1_STREAM_CLK 129 +#define CAMSS_CPHY_CSID0_CLK 130 +#define CAMSS_CPHY_CSID1_CLK 131 +#define CAMSS_CPHY_CSID2_CLK 132 +#define CAMSS_CPHY_CSID3_CLK 133 +#define CAMSS_CSIPHY0_CLK 134 +#define CAMSS_CSIPHY1_CLK 135 +#define CAMSS_CSIPHY2_CLK 136 +#define FD_CORE_CLK 137 +#define FD_CORE_UAR_CLK 138 +#define FD_AHB_CLK 139 +#define MNOC_AHB_CLK 140 +#define BIMC_SMMU_AHB_CLK 141 +#define BIMC_SMMU_AXI_CLK 142 +#define MNOC_MAXI_CLK 143 +#define VMEM_MAXI_CLK 144 +#define VMEM_AHB_CLK 145 + +#define SPDM_BCR 0 +#define SPDM_RM_BCR 1 +#define MISC_BCR 2 +#define VIDEO_TOP_BCR 3 +#define THROTTLE_VIDEO_BCR 4 +#define MDSS_BCR 5 +#define THROTTLE_MDSS_BCR 6 +#define CAMSS_PHY0_BCR 7 +#define CAMSS_PHY1_BCR 8 +#define CAMSS_PHY2_BCR 9 +#define CAMSS_CSI0_BCR 10 +#define CAMSS_CSI0RDI_BCR 11 +#define CAMSS_CSI0PIX_BCR 12 +#define CAMSS_CSI1_BCR 13 +#define CAMSS_CSI1RDI_BCR 14 +#define CAMSS_CSI1PIX_BCR 15 +#define CAMSS_CSI2_BCR 16 +#define CAMSS_CSI2RDI_BCR 17 +#define CAMSS_CSI2PIX_BCR 18 +#define CAMSS_CSI3_BCR 19 +#define CAMSS_CSI3RDI_BCR 20 +#define CAMSS_CSI3PIX_BCR 21 +#define CAMSS_ISPIF_BCR 22 +#define CAMSS_CCI_BCR 23 +#define CAMSS_TOP_BCR 24 +#define CAMSS_AHB_BCR 25 +#define CAMSS_MICRO_BCR 26 +#define CAMSS_JPEG_BCR 27 +#define CAMSS_VFE0_BCR 28 +#define CAMSS_VFE1_BCR 29 +#define CAMSS_VFE_VBIF_BCR 30 +#define CAMSS_CPP_TOP_BCR 31 +#define CAMSS_CPP_BCR 32 +#define CAMSS_CSI_VFE0_BCR 33 +#define CAMSS_CSI_VFE1_BCR 34 +#define CAMSS_FD_BCR 35 +#define THROTTLE_CAMSS_BCR 36 +#define MNOCAHB_BCR 37 +#define MNOCAXI_BCR 38 +#define BMIC_SMMU_BCR 39 +#define MNOC_MAXI_BCR 40 +#define VMEM_BCR 41 +#define BTO_BCR 42 + +#define VIDEO_TOP_GDSC 1 +#define VIDEO_SUBCORE0_GDSC 2 +#define VIDEO_SUBCORE1_GDSC 3 +#define MDSS_GDSC 4 +#define CAMSS_TOP_GDSC 5 +#define CAMSS_VFE0_GDSC 6 +#define CAMSS_VFE1_GDSC 7 +#define CAMSS_CPP_GDSC 8 +#define BIMC_SMMU_GDSC 9 + +#endif