[7/7] ASoC: dt-bindings: mediatek,mt8188-afe: add audio properties

  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(+)
  

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