Date: Tue, 23 Jan 2024 16:28:34 +0800
From: Junyi Zhao <junyi.zhao@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc: Thierry Reding <thierry.reding@...il.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Kevin Hilman <khilman@...libre.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-amlogic@...ts.infradead.org,
 linux-pwm@...r.kernel.org, Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v4 1/6] dt-bindings: pwm: amlogic: fix s4 bindings



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@...gutronix.de> 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@...nel.org>
>>> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
>>> ---
>>>   .../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@...il.com>
>>>
>>> -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

Powered by blists - more mailing lists