| Message ID | 20230413104713.7174-8-trevor.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: mediatek: mt8188: revise AFE driver | expand |
On 13/04/2023 12:47, Trevor Wu wrote: > Assign top_a1sys_hp clock to 26M, and add apll1_d4 to clocks for switching > the parent of top_a1sys_hp dynamically > On the other hand, "mediatek,infracfg" is included for bus protection. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > --- > .../bindings/sound/mediatek,mt8188-afe.yaml | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml > index 82ccb32f08f2..03301d5082f3 100644 > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml > @@ -29,6 +29,10 @@ properties: > $ref: /schemas/types.yaml#/definitions/phandle > description: The phandle of the mediatek topckgen controller > > + mediatek,infracfg: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of the mediatek infracfg controller > + > power-domains: > maxItems: 1 > > @@ -37,6 +41,7 @@ properties: > - description: 26M clock > - description: audio pll1 clock > - description: audio pll2 clock > + - description: audio pll1 divide 4 > - description: clock divider for i2si1_mck > - description: clock divider for i2si2_mck > - description: clock divider for i2so1_mck > @@ -58,6 +63,7 @@ properties: > - const: clk26m > - const: apll1 > - const: apll2 > + - const: apll1_d4 Why do you add clocks in the middle? The order is strict, so you just broke all DTS. > - const: apll12_div0 > - const: apll12_div1 > - const: apll12_div2 > @@ -74,6 +80,12 @@ properties: > - const: i2si2_m_sel > - const: adsp_audio_26m > > + assigned-clocks: > + maxItems: 1 > + > + assigned-clock-parents: > + maxItems: 1 Usually these two are not needed. > + > mediatek,etdm-in1-cowork-source: > $ref: /schemas/types.yaml#/definitions/uint32 > description: > @@ -147,6 +159,8 @@ required: > - power-domains > - clocks > - clock-names > + - assigned-clocks > + - assigned-clock-parents Why making them required? Best regards, Krzysztof
On Sat, 2023-04-15 at 11:00 +0200, Krzysztof Kozlowski wrote: > > On 13/04/2023 12:47, Trevor Wu wrote: > > Assign top_a1sys_hp clock to 26M, and add apll1_d4 to clocks for > > switching > > the parent of top_a1sys_hp dynamically > > On the other hand, "mediatek,infracfg" is included for bus > > protection. > > > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > > --- > > .../bindings/sound/mediatek,mt8188-afe.yaml | 18 > > ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml > > b/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml > > index 82ccb32f08f2..03301d5082f3 100644 > > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > afe.yaml > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > afe.yaml > > @@ -29,6 +29,10 @@ properties: > > $ref: /schemas/types.yaml#/definitions/phandle > > description: The phandle of the mediatek topckgen controller > > > > + mediatek,infracfg: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of the mediatek infracfg controller > > + > > power-domains: > > maxItems: 1 > > > > @@ -37,6 +41,7 @@ properties: > > - description: 26M clock > > - description: audio pll1 clock > > - description: audio pll2 clock > > + - description: audio pll1 divide 4 > > - description: clock divider for i2si1_mck > > - description: clock divider for i2si2_mck > > - description: clock divider for i2so1_mck > > @@ -58,6 +63,7 @@ properties: > > - const: clk26m > > - const: apll1 > > - const: apll2 > > + - const: apll1_d4 > > Why do you add clocks in the middle? The order is strict, so you just > broke all DTS. > In DTS file, I only need to make sure that the order in clocks should be the same as clock-names, so I misunderstood that I can add the clock in the middle based on the clock type. Sorry, I didn't know the order is strict. I will move the new clock to the last one in v2. > > - const: apll12_div0 > > - const: apll12_div1 > > - const: apll12_div2 > > @@ -74,6 +80,12 @@ properties: > > - const: i2si2_m_sel > > - const: adsp_audio_26m > > > > + assigned-clocks: > > + maxItems: 1 > > + > > + assigned-clock-parents: > > + maxItems: 1 > > Usually these two are not needed. > OK. I will remove these two properties in v2. > > + > > mediatek,etdm-in1-cowork-source: > > $ref: /schemas/types.yaml#/definitions/uint32 > > description: > > @@ -147,6 +159,8 @@ required: > > - power-domains > > - clocks > > - clock-names > > + - assigned-clocks > > + - assigned-clock-parents > > Why making them required? As I mentioned in the commit message, switching the parent of top_a1sys_hp was included in the series. I make use of these two properties to initialize the clock to 26MHz, so APLL1 can be enabled only when it's really required. Thanks, Trevor >
Il 17/04/23 04:44, Trevor Wu (吳文良) ha scritto: > On Sat, 2023-04-15 at 11:00 +0200, Krzysztof Kozlowski wrote: >> >> On 13/04/2023 12:47, Trevor Wu wrote: >>> Assign top_a1sys_hp clock to 26M, and add apll1_d4 to clocks for >>> switching >>> the parent of top_a1sys_hp dynamically >>> On the other hand, "mediatek,infracfg" is included for bus >>> protection. >>> >>> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> >>> --- >>> .../bindings/sound/mediatek,mt8188-afe.yaml | 18 >>> ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml >>> b/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml >>> index 82ccb32f08f2..03301d5082f3 100644 >>> --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188- >>> afe.yaml >>> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188- >>> afe.yaml >>> @@ -29,6 +29,10 @@ properties: >>> $ref: /schemas/types.yaml#/definitions/phandle >>> description: The phandle of the mediatek topckgen controller >>> >>> + mediatek,infracfg: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: The phandle of the mediatek infracfg controller >>> + >>> power-domains: >>> maxItems: 1 >>> >>> @@ -37,6 +41,7 @@ properties: >>> - description: 26M clock >>> - description: audio pll1 clock >>> - description: audio pll2 clock >>> + - description: audio pll1 divide 4 >>> - description: clock divider for i2si1_mck >>> - description: clock divider for i2si2_mck >>> - description: clock divider for i2so1_mck >>> @@ -58,6 +63,7 @@ properties: >>> - const: clk26m >>> - const: apll1 >>> - const: apll2 >>> + - const: apll1_d4 >> >> Why do you add clocks in the middle? The order is strict, so you just >> broke all DTS. >> > > In DTS file, I only need to make sure that the order in clocks should > be the same as clock-names, so I misunderstood that I can add the clock > in the middle based on the clock type. > > Sorry, I didn't know the order is strict. I will move the new clock to > the last one in v2. > Actually, doing that is borderline-ok... there's no devicetree for MT8188 upstream, so that's not breaking anything at all. In any case, I agree that you should generally avoid doing that but I think that in this specific case it's fine; I'm not a devicetree maintainer though. P.S.: Trevor, next time please make reviewers aware of the fact that no 8188 devicetree is present upstream! Regards, Angelo
On Mon, 2023-04-17 at 09:55 +0200, AngeloGioacchino Del Regno wrote: > Il 17/04/23 04:44, Trevor Wu (吳文良) ha scritto: > > On Sat, 2023-04-15 at 11:00 +0200, Krzysztof Kozlowski wrote: > > > > > > On 13/04/2023 12:47, Trevor Wu wrote: > > > > Assign top_a1sys_hp clock to 26M, and add apll1_d4 to clocks > > > > for > > > > switching > > > > the parent of top_a1sys_hp dynamically > > > > On the other hand, "mediatek,infracfg" is included for bus > > > > protection. > > > > > > > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > > > > --- > > > > .../bindings/sound/mediatek,mt8188-afe.yaml | 18 > > > > ++++++++++++++++++ > > > > 1 file changed, 18 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > afe.yaml > > > > b/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > afe.yaml > > > > index 82ccb32f08f2..03301d5082f3 100644 > > > > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > afe.yaml > > > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > afe.yaml > > > > @@ -29,6 +29,10 @@ properties: > > > > $ref: /schemas/types.yaml#/definitions/phandle > > > > description: The phandle of the mediatek topckgen > > > > controller > > > > > > > > + mediatek,infracfg: > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > + description: The phandle of the mediatek infracfg > > > > controller > > > > + > > > > power-domains: > > > > maxItems: 1 > > > > > > > > @@ -37,6 +41,7 @@ properties: > > > > - description: 26M clock > > > > - description: audio pll1 clock > > > > - description: audio pll2 clock > > > > + - description: audio pll1 divide 4 > > > > - description: clock divider for i2si1_mck > > > > - description: clock divider for i2si2_mck > > > > - description: clock divider for i2so1_mck > > > > @@ -58,6 +63,7 @@ properties: > > > > - const: clk26m > > > > - const: apll1 > > > > - const: apll2 > > > > + - const: apll1_d4 > > > > > > Why do you add clocks in the middle? The order is strict, so you > > > just > > > broke all DTS. > > > > > > > In DTS file, I only need to make sure that the order in clocks > > should > > be the same as clock-names, so I misunderstood that I can add the > > clock > > in the middle based on the clock type. > > > > Sorry, I didn't know the order is strict. I will move the new clock > > to > > the last one in v2. > > > > Actually, doing that is borderline-ok... there's no devicetree for > MT8188 > upstream, so that's not breaking anything at all. > In any case, I agree that you should generally avoid doing that but I > think > that in this specific case it's fine; I'm not a devicetree maintainer > though. > > P.S.: Trevor, next time please make reviewers aware of the fact that > no 8188 > devicetree is present upstream! > Got it. Thanks. Hi krzysztof, Because there is no upstream mt8188 DTS, should I move the new clock to the end of clock list? If I move "apll1_d4" to the end of the list at binding file, when I upstream the devicetree node existing clocks and clock-names properties , should I follow the sequence defined in dt-bindings or can I have a new sequence based on the clock type or alphabet? Thanks, Trevor
On 18/04/2023 12:23, Trevor Wu (吳文良) wrote: >> Actually, doing that is borderline-ok... there's no devicetree for >> MT8188 >> upstream, so that's not breaking anything at all. >> In any case, I agree that you should generally avoid doing that but I >> think >> that in this specific case it's fine; I'm not a devicetree maintainer >> though. >> >> P.S.: Trevor, next time please make reviewers aware of the fact that >> no 8188 >> devicetree is present upstream! >> > Got it. Thanks. > > > Hi krzysztof, > > Because there is no upstream mt8188 DTS, should I move the new clock to > the end of clock list? What is the reason to add them in the middle? So far there was no argument, so always add at the end. If you have an argument, let's discuss it. > > If I move "apll1_d4" to the end of the list at binding file, when I > upstream the devicetree node existing clocks and clock-names properties > , should I follow the sequence defined in dt-bindings If you do not follow the sequence of bindings, you upstream incorrect DTS which does not follow ABI and fails the tests. Therefore yes, use the same order as your bindings define. > or can I have a > new sequence based on the clock type or alphabet? Sorry, I don't know what is the order of clock type and alphabet. If you mean anything else than bindings, then no, because how is it supposed to work then? Best regards, Krzysztof
On Tue, 2023-04-18 at 14:25 +0200, Krzysztof Kozlowski wrote: > On 18/04/2023 12:23, Trevor Wu (吳文良) wrote: > > > Actually, doing that is borderline-ok... there's no devicetree > > > for > > > MT8188 > > > upstream, so that's not breaking anything at all. > > > In any case, I agree that you should generally avoid doing that > > > but I > > > think > > > that in this specific case it's fine; I'm not a devicetree > > > maintainer > > > though. > > > > > > P.S.: Trevor, next time please make reviewers aware of the fact > > > that > > > no 8188 > > > devicetree is present upstream! > > > > > > > Got it. Thanks. > > > > > > Hi krzysztof, > > > > Because there is no upstream mt8188 DTS, should I move the new > > clock to > > the end of clock list? > > What is the reason to add them in the middle? So far there was no > argument, so always add at the end. If you have an argument, let's > discuss it. > No special reason. Just hope to sort the clock by the clock type. But it's possible to extend the clock list after we upstream MT8188 DTS, so it won't follow the order finally. I think it is fine to put the clock at the end. I will move it to the end in V2. > > > > If I move "apll1_d4" to the end of the list at binding file, when I > > upstream the devicetree node existing clocks and clock-names > > properties > > , should I follow the sequence defined in dt-bindings > > If you do not follow the sequence of bindings, you upstream incorrect > DTS which does not follow ABI and fails the tests. Therefore yes, use > the same order as your bindings define. > > > or can I have a > > new sequence based on the clock type or alphabet? > > Sorry, I don't know what is the order of clock type and alphabet. If > you > mean anything else than bindings, then no, because how is it supposed > to > work then? > > Got it. Thanks for the detailed explanation. I will follow the order as bindings when I update the DTS node. Thanks, Trevor
diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml index 82ccb32f08f2..03301d5082f3 100644 --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-afe.yaml @@ -29,6 +29,10 @@ properties: $ref: /schemas/types.yaml#/definitions/phandle description: The phandle of the mediatek topckgen controller + mediatek,infracfg: + $ref: /schemas/types.yaml#/definitions/phandle + description: The phandle of the mediatek infracfg controller + power-domains: maxItems: 1 @@ -37,6 +41,7 @@ properties: - description: 26M clock - description: audio pll1 clock - description: audio pll2 clock + - description: audio pll1 divide 4 - description: clock divider for i2si1_mck - description: clock divider for i2si2_mck - description: clock divider for i2so1_mck @@ -58,6 +63,7 @@ properties: - const: clk26m - const: apll1 - const: apll2 + - const: apll1_d4 - const: apll12_div0 - const: apll12_div1 - const: apll12_div2 @@ -74,6 +80,12 @@ properties: - const: i2si2_m_sel - const: adsp_audio_26m + assigned-clocks: + maxItems: 1 + + assigned-clock-parents: + maxItems: 1 + mediatek,etdm-in1-cowork-source: $ref: /schemas/types.yaml#/definitions/uint32 description: @@ -147,6 +159,8 @@ required: - power-domains - clocks - clock-names + - assigned-clocks + - assigned-clock-parents additionalProperties: false @@ -170,6 +184,7 @@ examples: clocks = <&clk26m>, <&apmixedsys 9>, //CLK_APMIXED_APLL1 <&apmixedsys 10>, //CLK_APMIXED_APLL2 + <&topckgen 136>, //CLK_TOP_APLL1_D4 <&topckgen 186>, //CLK_TOP_APLL12_CK_DIV0 <&topckgen 187>, //CLK_TOP_APLL12_CK_DIV1 <&topckgen 188>, //CLK_TOP_APLL12_CK_DIV2 @@ -188,6 +203,7 @@ examples: clock-names = "clk26m", "apll1", "apll2", + "apll1_d4", "apll12_div0", "apll12_div1", "apll12_div2", @@ -203,6 +219,8 @@ examples: "i2si1_m_sel", "i2si2_m_sel", "adsp_audio_26m"; + assigned-clocks = <&topckgen 83>; //CLK_TOP_A1SYS_HP + assigned-clock-parents = <&clk26m>; }; ...
Assign top_a1sys_hp clock to 26M, and add apll1_d4 to clocks for switching the parent of top_a1sys_hp dynamically On the other hand, "mediatek,infracfg" is included for bus protection. Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> --- .../bindings/sound/mediatek,mt8188-afe.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)