Re: [PATCH v4 1/6] dt-bindings: pwm: amlogic: fix s4 bindings

[Junyi Zhao]

On 2024/1/17 18:30, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed 17 Jan 2024 at 11:03, Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> > [[PGP Signed Part:Undecided]]
> > On Fri, Dec 22, 2023 at 12:16:49PM +0100, Jerome Brunet wrote:
> > > s4 has been added to the compatible list while converting the Amlogic PWM
> > > binding documentation from txt to yaml.
> > >
> > > However, on the s4, the clock bindings have different meaning compared to
> > > the previous SoCs.
> > >
> > > On the previous SoCs the clock bindings used to describe which input the
> > > PWM channel multiplexer should pick among its possible parents.
> > >
> > > This is very much tied to the driver implementation, instead of describing
> > > the HW for what it is. When support for the Amlogic PWM was first added,
> > > how to deal with clocks through DT was not as clear as it nowadays.
> > > The Linux driver now ignores this DT setting, but still relies on the
> > > hard-coded list of clock sources.
> > >
> > > On the s4, the input multiplexer is gone. The clock bindings actually
> > > describe the clock as it exists, not a setting. The property has a
> > > different meaning, even if it is still 2 clocks and it would pass the check
> > > when support is actually added.
> > >
> > > Also the s4 cannot work if the clocks are not provided, so the property no
> > > longer optional.
> >
> > s/no/is no/
> >
> > > Finally, for once it makes sense to see the input as being numbered
> > > somehow. No need to bother with clock-names on the s4 type of PWM.
> > >
> > > Fixes: 43a1c4ff3977 ("dt-bindings: pwm: Convert Amlogic Meson PWM binding")
> > > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > > Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> > > ---
> > >   .../devicetree/bindings/pwm/pwm-amlogic.yaml  | 67 ++++++++++++++++---
> > >   1 file changed, 58 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> > > index 527864a4d855..a1d382aacb82 100644
> > > --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> > > @@ -9,9 +9,6 @@ title: Amlogic PWM
> > >   maintainers:
> > >     - Heiner Kallweit <hkallweit1@xxxxxxxxx>
> > >
> > > -allOf:
> > > -  - $ref: pwm.yaml#
> > > -
> > >   properties:
> > >     compatible:
> > >       oneOf:
> > > @@ -43,12 +40,8 @@ properties:
> > >       maxItems: 2
> > >
> > >     clock-names:
> > > -    oneOf:
> > > -      - items:
> > > -          - enum: [clkin0, clkin1]
> > > -      - items:
> > > -          - const: clkin0
> > > -          - const: clkin1
> > > +    minItems: 1
> > > +    maxItems: 2
> > >
> > >     "#pwm-cells":
> > >       const: 3
> > > @@ -57,6 +50,55 @@ required:
> > >     - compatible
> > >     - reg
> > >
> > > +allOf:
> > > +  - $ref: pwm.yaml#
> > > +
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - amlogic,meson8-pwm
> > > +              - amlogic,meson8b-pwm
> > > +              - amlogic,meson-gxbb-pwm
> > > +              - amlogic,meson-gxbb-ao-pwm
> > > +              - amlogic,meson-axg-ee-pwm
> > > +              - amlogic,meson-axg-ao-pwm
> > > +              - amlogic,meson-g12a-ee-pwm
> > > +              - amlogic,meson-g12a-ao-pwm-ab
> > > +              - amlogic,meson-g12a-ao-pwm-cd
> > > +    then:
> > > +      # Historic bindings tied to the driver implementation
> > > +      # The clocks provided here are meant to be matched with the input
> > > +      # known (hard-coded) in the driver and used to select pwm clock
> > > +      # source. Currently, the linux driver ignores this.
> >
> > I admit I didn't understand the relevant difference between the old and
> > the new binding yet.
>
> Let's try to explain differently then:
> * So far each AML PWM IP has 2 pwm.
> * Up to G12, 4 input PLL/clocks are wired to the HW IP and there
>    mux/div/gate to select which input to take.
>    - The historic bindings just described how to setup each of the 2
>      internal muxes - 2 optionnal clocks.
>      The actual 4 inputs names from CCF are hard coded in
>      the driver. This is a pretty bad description. The driver has been
>      updated since then to use CCF to figure out the best parent
>      according to pwm rate so this setting is ignored now.
>    - The 'new' bindings (introduced in patch #2) fixes the problem above
>      but the meaning of the clock binding is different. It describes the
>      actual HW parents - 4 clocks, optionnal since some are not wired on
>      some PWM blocks. To avoid breaking the ABI, a new compatible for
>      these SoC is introduced.
>
> > (The driver currently doesn't support the s4, right?)
>
> Indeed. I know Amlogic is preparing the support. I could do it in this
> series but I prefer to encourage them to contribute. It should come
> shortly after this series is merged.
Yes. Amlogic is waiting these patches of Jerome. after these merge, we 
will start to rebase and submit s4 code.
-
Best Regards Junyi