Message ID | 20220831124850.7748-1-johnson.wang@mediatek.com (mailing list archive) |
---|---|
Headers | show |
Series | Introduce MediaTek frequency hopping driver | expand |
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@mediatek.com> > Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com> > Signed-off-by: Johnson Wang <johnson.wang@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@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). Best regards, Krzysztof
On 31/08/2022 15:48, Johnson Wang wrote: > Introduce MediaTek frequency hopping and spread spectrum clocking control > for MT8186. With one line introduction, you do not help us to understand this. :( Best regards, Krzysztof
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@mediatek.com> >> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com> >> Signed-off-by: Johnson Wang <johnson.wang@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@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] Cheers, Angelo [1]: https://patchwork.kernel.org/project/linux-mediatek/patch/20220612135414.3003-3-johnson.wang@mediatek.com/ > > Best regards, > Krzysztof
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@mediatek.com> >>> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com> >>> Signed-off-by: Johnson Wang <johnson.wang@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@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. 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
On 01/09/2022 11:04, AngeloGioacchino Del Regno wrote: > 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/ I see there hundreds of lines of quoted text without trimming to actual parts, so no, no one is going to read it. Best regards, Krzysztof
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@mediatek.com> >>>> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com> >>>> Signed-off-by: Johnson Wang <johnson.wang@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@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
On 01/09/2022 13:22, AngeloGioacchino Del Regno wrote: >> 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>; > I propose the last one - using standard clocks property and a matching table. Best regards, Krzysztof
Il 01/09/22 12:30, Krzysztof Kozlowski ha scritto: > On 01/09/2022 13:22, AngeloGioacchino Del Regno wrote: >>> 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>; >> > > I propose the last one - using standard clocks property and a matching > table. > Right. I like the last one a bit better, as well. Thanks for the advices! Angelo
On Wed, 2022-08-31 at 16:20 +0300, Krzysztof Kozlowski wrote: > On 31/08/2022 15:48, Johnson Wang wrote: > > Introduce MediaTek frequency hopping and spread spectrum clocking > > control > > for MT8186. > > With one line introduction, you do not help us to understand this. :( > > Best regards, > Krzysztof Hi Krzysztof, Ok, I will describe more in the next version. BRs, Johnson Wang
On Wed, 2022-08-31 at 16:19 +0300, Krzysztof Kozlowski wrote: > 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@mediatek.com> > > Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com> > > Signed-off-by: Johnson Wang <johnson.wang@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.yam > > l > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam > > l > > new file mode 100644 > > index 000000000000..c5d76410538b > > --- /dev/null > > +++ > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam > > l > > @@ -0,0 +1,49 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: > > https://urldefense.com/v3/__http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml*__;Iw!!CTRNKA9wMg0ARbw!ysl-bMp7yP9Ym70o6EVB8A36MBxcXGap8doEKR_SbaAQSy8-_RU5jvrWTjzETut_6eXNGut4j-3dY0q7xJdpQbmaOw$ > > > > +$schema: > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!ysl-bMp7yP9Ym70o6EVB8A36MBxcXGap8doEKR_SbaAQSy8-_RU5jvrWTjzETut_6eXNGut4j-3dY0q7xJezt7_RBw$ > > > > + > > +title: MediaTek frequency hopping and spread spectrum clocking > > control > > + > > +maintainers: > > + - Edward-JW Yang <edward-jw.yang@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? > Hi Krzysztof, At this moment, we plan to support FHCTL feature for MT8186 only. If you prefer SoC-specific compatble, we will improve that in the next version. Thanks for your suggestion. BRs, Johnson Wang > > + > > + 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). > > > Best regards, > Krzysztof
On 02/09/2022 08:39, Johnson Wang wrote: > On Wed, 2022-08-31 at 16:19 +0300, Krzysztof Kozlowski wrote: >> 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@mediatek.com> >>> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com> >>> Signed-off-by: Johnson Wang <johnson.wang@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.yam >>> l >>> b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam >>> l >>> new file mode 100644 >>> index 000000000000..c5d76410538b >>> --- /dev/null >>> +++ >>> b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam >>> l >>> @@ -0,0 +1,49 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: >>> https://urldefense.com/v3/__http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml*__;Iw!!CTRNKA9wMg0ARbw!ysl-bMp7yP9Ym70o6EVB8A36MBxcXGap8doEKR_SbaAQSy8-_RU5jvrWTjzETut_6eXNGut4j-3dY0q7xJdpQbmaOw$ >>> >>> +$schema: >>> https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!ysl-bMp7yP9Ym70o6EVB8A36MBxcXGap8doEKR_SbaAQSy8-_RU5jvrWTjzETut_6eXNGut4j-3dY0q7xJezt7_RBw$ >>> >>> + >>> +title: MediaTek frequency hopping and spread spectrum clocking >>> control >>> + >>> +maintainers: >>> + - Edward-JW Yang <edward-jw.yang@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? >> > > Hi Krzysztof, > > At this moment, we plan to support FHCTL feature for MT8186 only. > > If you prefer SoC-specific compatble, we will improve that in the next > version. Then make it only for mt8186. Best regards, Krzysztof