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]

Date:   Thu, 1 Sep 2022 10:04:48 +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 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]

Cheers,
Angelo

[1]: 
https://patchwork.kernel.org/project/linux-mediatek/patch/20220612135414.3003-3-johnson.wang@mediatek.com/

> 
> Best regards,
> Krzysztof