[v1,1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema

  The AIC needs only the first two clocks: "aic" is a gate that's used
for gating the I2S controller when it's suspended, and "i2s" is the
system clock, from which the bit and frame clocks are derived. Both
clocks are therefore reasonably part of the AIC and should be passed
to the OS.

But the "ext" and "pll half" clocks are a little more questionable.
It appears these bindings were introduced when the schema was first
converted to YAML, but weren't present in the original .txt binding.
They are intended to be the possible parent clocks of "i2s".

The JZ4770 actually has three parents for its "i2s" clock, named
"ext", "pll0", and "pll1" in the Linux driver. The JZ4780 has two
parents but it doesn't have a "pll half" clock, instead it has an
"i2s_pll" clock which behaves much differently to the actual
"pll half" clock found on the JZ4740 & JZ4760. And there are other
Ingenic SoCs that share the JZ4780's clock layout, eg, the X1000.

Therefore, the bindings aren't really adequate for the JZ4770 and
a bit misleading for the JZ4780. Either we should fix the bindings,
or remove them entirely.

This patch opts to remove the bindings. There is a good case to be
made that "ext" and "pll half" don't belong here because they aren't
directly used by the AIC. They are only used to set the parent of
the "i2s" clock; they have no other effect on the AIC.

A good way to think of it is in terms of how the AIC constrains
clocks. The AIC can only generate the bit & frame clocks from the
system clock in certain ratios. Setting the sample rate effectively
constrains the frame clock, which, because of the clock dividers
controlled by the AIC, translates to constraints on the "i2s" clock.
Nothing in the AIC imposes a direct constraint on the parents of
the "i2s" clock, and the AIC does not need to enable or disable
the parents directly, so in principle the AIC doesn't need to be
aware of the parent clocks at all.

The choice of parent clock is still important, but the AIC doesn't
have enough information to apply such constraints itself. The sound
card does have that information because it knows how the AIC is
connected to other components. We need to use other DT mechanisms
to communicate those constraints at the sound card level, instead
of passing the clocks through to the AIC, and inventing ad-hoc ways
to plumb the constraints around behind the scenes.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 .../devicetree/bindings/sound/ingenic,aic.yaml         | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
  

Hi,

Le ven. 28 oct. 2022 à 11:34:16 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> The AIC needs only the first two clocks: "aic" is a gate that's used
> for gating the I2S controller when it's suspended, and "i2s" is the
> system clock, from which the bit and frame clocks are derived. Both
> clocks are therefore reasonably part of the AIC and should be passed
> to the OS.
> 
> But the "ext" and "pll half" clocks are a little more questionable.
> It appears these bindings were introduced when the schema was first
> converted to YAML, but weren't present in the original .txt binding.
> They are intended to be the possible parent clocks of "i2s".
> 
> The JZ4770 actually has three parents for its "i2s" clock, named
> "ext", "pll0", and "pll1" in the Linux driver. The JZ4780 has two
> parents but it doesn't have a "pll half" clock, instead it has an
> "i2s_pll" clock which behaves much differently to the actual
> "pll half" clock found on the JZ4740 & JZ4760. And there are other
> Ingenic SoCs that share the JZ4780's clock layout, eg, the X1000.
> 
> Therefore, the bindings aren't really adequate for the JZ4770 and
> a bit misleading for the JZ4780. Either we should fix the bindings,
> or remove them entirely.
> 
> This patch opts to remove the bindings. There is a good case to be
> made that "ext" and "pll half" don't belong here because they aren't
> directly used by the AIC. They are only used to set the parent of
> the "i2s" clock; they have no other effect on the AIC.
> 
> A good way to think of it is in terms of how the AIC constrains
> clocks. The AIC can only generate the bit & frame clocks from the
> system clock in certain ratios. Setting the sample rate effectively
> constrains the frame clock, which, because of the clock dividers
> controlled by the AIC, translates to constraints on the "i2s" clock.
> Nothing in the AIC imposes a direct constraint on the parents of
> the "i2s" clock, and the AIC does not need to enable or disable
> the parents directly, so in principle the AIC doesn't need to be
> aware of the parent clocks at all.
> 
> The choice of parent clock is still important, but the AIC doesn't
> have enough information to apply such constraints itself. The sound
> card does have that information because it knows how the AIC is
> connected to other components. We need to use other DT mechanisms
> to communicate those constraints at the sound card level, instead
> of passing the clocks through to the AIC, and inventing ad-hoc ways
> to plumb the constraints around behind the scenes.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Yes, it makes sense also because from a DT point of view, these clocks 
were redundant information. It's enough to know the i2s clock to also 
know its parents.

Acked-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  .../devicetree/bindings/sound/ingenic,aic.yaml         | 10 
> ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/ingenic,aic.yaml 
> b/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> index d607325f2f15..c4f9b3c2bde5 100644
> --- a/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> +++ b/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> @@ -37,15 +37,11 @@ properties:
>      items:
>        - description: AIC clock
>        - description: I2S clock
> -      - description: EXT clock
> -      - description: PLL/2 clock
> 
>    clock-names:
>      items:
>        - const: aic
>        - const: i2s
> -      - const: ext
> -      - const: pll half
> 
>    dmas:
>      items:
> @@ -82,10 +78,8 @@ examples:
>        interrupts = <18>;
> 
>        clocks = <&cgu JZ4740_CLK_AIC>,
> -               <&cgu JZ4740_CLK_I2S>,
> -               <&cgu JZ4740_CLK_EXT>,
> -               <&cgu JZ4740_CLK_PLL_HALF>;
> -      clock-names = "aic", "i2s", "ext", "pll half";
> +               <&cgu JZ4740_CLK_I2S>;
> +      clock-names = "aic", "i2s";
> 
>        dmas = <&dmac 25 0xffffffff>, <&dmac 24 0xffffffff>;
>        dma-names = "rx", "tx";
> --
> 2.38.1
>
  

On Fri, Oct 28, 2022 at 11:34:16AM +0100, Aidan MacDonald wrote:
> The AIC needs only the first two clocks: "aic" is a gate that's used
> for gating the I2S controller when it's suspended, and "i2s" is the
> system clock, from which the bit and frame clocks are derived. Both
> clocks are therefore reasonably part of the AIC and should be passed
> to the OS.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.
  

On Fri, 28 Oct 2022 11:34:16 +0100, Aidan MacDonald wrote:
> The AIC needs only the first two clocks: "aic" is a gate that's used
> for gating the I2S controller when it's suspended, and "i2s" is the
> system clock, from which the bit and frame clocks are derived. Both
> clocks are therefore reasonably part of the AIC and should be passed
> to the OS.
> 
> But the "ext" and "pll half" clocks are a little more questionable.
> It appears these bindings were introduced when the schema was first
> converted to YAML, but weren't present in the original .txt binding.
> They are intended to be the possible parent clocks of "i2s".
> 
> The JZ4770 actually has three parents for its "i2s" clock, named
> "ext", "pll0", and "pll1" in the Linux driver. The JZ4780 has two
> parents but it doesn't have a "pll half" clock, instead it has an
> "i2s_pll" clock which behaves much differently to the actual
> "pll half" clock found on the JZ4740 & JZ4760. And there are other
> Ingenic SoCs that share the JZ4780's clock layout, eg, the X1000.
> 
> Therefore, the bindings aren't really adequate for the JZ4770 and
> a bit misleading for the JZ4780. Either we should fix the bindings,
> or remove them entirely.
> 
> This patch opts to remove the bindings. There is a good case to be
> made that "ext" and "pll half" don't belong here because they aren't
> directly used by the AIC. They are only used to set the parent of
> the "i2s" clock; they have no other effect on the AIC.
> 
> A good way to think of it is in terms of how the AIC constrains
> clocks. The AIC can only generate the bit & frame clocks from the
> system clock in certain ratios. Setting the sample rate effectively
> constrains the frame clock, which, because of the clock dividers
> controlled by the AIC, translates to constraints on the "i2s" clock.
> Nothing in the AIC imposes a direct constraint on the parents of
> the "i2s" clock, and the AIC does not need to enable or disable
> the parents directly, so in principle the AIC doesn't need to be
> aware of the parent clocks at all.
> 
> The choice of parent clock is still important, but the AIC doesn't
> have enough information to apply such constraints itself. The sound
> card does have that information because it knows how the AIC is
> connected to other components. We need to use other DT mechanisms
> to communicate those constraints at the sound card level, instead
> of passing the clocks through to the AIC, and inventing ad-hoc ways
> to plumb the constraints around behind the scenes.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  .../devicetree/bindings/sound/ingenic,aic.yaml         | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>