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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <955f7200-9d08-0d21-2d1a-5ccbd0f3a8af@collabora.com>
Date:   Thu, 1 Sep 2022 12:22:05 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Johnson Wang <johnson.wang@...iatek.com>, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, sboyd@...nel.org
Cc:     linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        Edward-JW Yang <edward-jw.yang@...iatek.com>
Subject: Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of
 MediaTek frequency hopping

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@...iatek.com>
>>>> Signed-off-by: Edward-JW Yang <edward-jw.yang@...iatek.com>
>>>> Signed-off-by: Johnson Wang <johnson.wang@...iatek.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@...iatek.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