Re: [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@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.

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