[PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[AngeloGioacchino Del Regno]

Il 01/09/22 11:42, Krzysztof Kozlowski ha scritto:
> On 01/09/2022 11:04, AngeloGioacchino Del Regno wrote:
>> Il 31/08/22 15:19, Krzysztof Kozlowski ha scritto:
>>> On 31/08/2022 15:48, Johnson Wang wrote:
>>>> Add the new binding documentation for MediaTek frequency hopping
>>>> and spread spectrum clocking control.
>>>>
>>>> Co-developed-by: Edward-JW Yang <edward-jw.yang at mediatek.com>
>>>> Signed-off-by: Edward-JW Yang <edward-jw.yang at mediatek.com>
>>>> Signed-off-by: Johnson Wang <johnson.wang at mediatek.com>
>>>> ---
>>>>    .../bindings/arm/mediatek/mediatek,fhctl.yaml | 49 +++++++++++++++++++
>>>>    1 file changed, 49 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>>> new file mode 100644
>>>> index 000000000000..c5d76410538b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>>> @@ -0,0 +1,49 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: MediaTek frequency hopping and spread spectrum clocking control
>>>> +
>>>> +maintainers:
>>>> +  - Edward-JW Yang <edward-jw.yang at mediatek.com>
>>>> +
>>>> +description: |
>>>> +  Frequency hopping control (FHCTL) is a piece of hardware that control
>>>> +  some PLLs to adopt "hopping" mechanism to adjust their frequency.
>>>> +  Spread spectrum clocking (SSC) is another function provided by this hardware.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: mediatek,fhctl
>>>
>>> You need SoC/device specific compatibles. Preferably only SoC specific,
>>> without generic fallback, unless you can guarantee (while representing
>>> MediaTek), that generic fallback will cover all of their SoCs?
>>>
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  mediatek,hopping-ssc-percents:
>>>> +    description: |
>>>> +      Determine the enablement of frequency hopping feature and the percentage
>>>> +      of spread spectrum clocking for PLLs.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>>> +    items:
>>>> +      items:
>>>> +        - description: PLL id that is expected to enable frequency hopping.
>>>
>>> So the clocks are indices from some specific, yet unnamed
>>> clock-controller? This feels hacky. You should rather take here clock
>>> phandles (1) or integrate it into specific clock controller (2). The
>>> reason is that either your device does something on top of existing
>>> clocks (option 1, thus it takes clock as inputs) or it modifies existing
>>> clocks (option 2, thus it is integral part of clock-controller).
>>>
>>
>> FHCTL is a MCU that handles (some, or all, depending on what's supported on the
>> SoC and what's needed by the board) PLL frequency setting, doing it in steps and
>> avoiding overshooting and other issues.
>>
>> We had a pretty big conversation about this a while ago and the indices instead
>> of phandles is actually my fault, that happened because I initially proposed your
>> option 2 but then for a number of reasons we went with this kind of solution.
>>
>> I know it's going to be a long read, but the entire conversation is on the list [1]
>>
> 
> Sorry, but it's a hacky architecture where one device (which is a clock
> provider) and second device have no relationship in hardware description
> but both play with each other resources.

Yes, that's exactly how it is hardware-side. Except, just to be as clear as
possible, FHCTL plays with the clock provider, but *not* vice-versa.

> That's simply not a proper
> hardware description, so again:
> 
> 1. If this is separate device (as you indicated), then it needs
> expressing the dependencies and uses of other device resources.

Agreed. In this case, what about...

mediatek,hopping-ssc-percents = <&provider CLK_SOMEPLL 3>;

or would it be better to specify the clocks in a separated property?

clocks = <&provider CLK_SOMEPLL>, <&provider CLK_SOME_OTHER_PLL>;
mediatek,hopping-ssc-percents = <3>, <5>;

Thanks,
Angelo

> 
> 2. If this is not a separate device, but integral part of clock
> controller, then it would be fine but then probably should be child of
> that device.
> 
> Best regards,
> Krzysztof