Re: [PATCH 1/9] dt-bindings: clock: Add SM8550 GCC clock bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 17/11/2022 10:40, Abel Vesa wrote:
> On 22-11-17 10:08:05, Krzysztof Kozlowski wrote:
>> On 16/11/2022 11:47, Abel Vesa wrote:
>>> Add device tree bindings for global clock controller on SM8550 SoCs.
>>
>> Subject: drop second, redundant "bindings".
> 
> Sure thing, will drop.
> 
>>
>>>
>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>> ---
>>>  .../bindings/clock/qcom,gcc-sm8550.yaml       |  88 +++++++
>>>  include/dt-bindings/clock/qcom,gcc-sm8550.h   | 231 ++++++++++++++++++
>>>  2 files changed, 319 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc-sm8550.yaml
>>>  create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8550.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc-sm8550.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc-sm8550.yaml
>>> new file mode 100644
>>> index 000000000000..a2468167c8ab
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/qcom,gcc-sm8550.yaml
>>> @@ -0,0 +1,88 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/qcom,gcc-sm8550.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm Global Clock & Reset Controller Binding for SM8550
>>
>> You need to base on recent bindings. See commit ece3c3198182a1.
>>
> 
> Yep, will do.
> 
>>> +
>>> +maintainers:
>>> +  - Bjorn Andersson <andersson@kernel.org>
>>> +
>>> +description: |
>>> +  Qualcomm global clock control module which supports the clocks, resets and
>>> +  power domains on SM8550
>>
>> Ditto
>>
>>> +
>>> +  See also:
>>> +  - dt-bindings/clock/qcom,gcc-sm8550.h
>>
>> Ditto
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: qcom,sm8550-gcc
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: Board XO source
>>> +      - description: Sleep clock source
>>> +      - description: PCIE 0 Pipe clock source (Optional clock)
>>
>> Drop "(Optional clock)"
> 
> Sure, will drop. I based this on SM8450, but maybe that also needs an
> update.
> 
>>
>>> +      - description: PCIE 1 Pipe clock source (Optional clock)
>>> +      - description: PCIE 1 Phy Auxiliary 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)
>>> +    minItems: 2
>>
>> This does not look correct. Why clocks of GCC are inputs clocks to GCC?
> 
> Well, it is not a GCC clock. It is a fixed-clock fed into GCC.
> The name is taken from downstream, but I'm pretty sure the HW clock is
> named so. So I think we should keep it as is.

I don't get it. The "PCIE 0 Pipe clock source" is defined here in this
header:
+#define GCC_PCIE_0_PIPE_CLK_SRC	41
so what do you mean it is not GCC clock?

Second thing, you have only one GCC, so how these clocks can be
optional? Either they are there or they are not. Optional means usually
that you have two devices having same compatible and they can operate a
bit differently or some pins are not connected. It's not the case here.

Best regards,
Krzysztof