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