Re: [PATCH v6 4/6] dt-bindings: clock: Add support for the MSM8998 mmcc

[Jeffrey Hugo <jhugo@codeaurora.org>]

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?

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.